Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add nftables::simplerule #33

Merged
merged 25 commits into from
Dec 10, 2020
Merged

Conversation

nbarrientos
Copy link
Collaborator

@nbarrientos nbarrientos commented Dec 3, 2020

Add a Firewall-like interface covering some simple cases to ease migration of rules.

Closes #17.

@LionelCons
Copy link

Good start!

My initial comments (subject to discussion) are the following.

I do not see where $setname is used.

There should be both dport and sport parameters. They should accept an integer or an array of integers.

Similarly, there should be saddr and daddr parameters accepting a string or an array of strings.

There is the question of IPv4 versus IPv6. I wonder whether the proto should be for instance tcp (= TCP for IPv4 and IPv6) or tcp4 (= TCP for IPv4) or tcp6 (= TCP for IPv6). If not, we might need a way to differentiate ip and ip6.

@nbarrientos
Copy link
Collaborator Author

I do not see where $setname is used.

Yup, it's a leftover (I used nftables::set as starting point). It's fixed now, thanks.

There should be both dport and sport parameters. They should accept an integer or an array of integers.

I've implemented a richer dport, allowing a single port, a range and an array of ports. The same could be done for sport.

Similarly, there should be saddr and daddr parameters accepting a string or an array of strings.

This is tricky as the matcher for the rule would be different if the address (or the range in CIDR notation) is an IPv4 or an IPv6 one. It could be a single parameter of type Array[Variant[Stdlib::IP::Address::V6, Stdlib::IP::Address::V4]] and then in the template depending on the type of the element we do ip or ip6 in the statement.

@LionelCons
Copy link

Thanks for these changes.

For ports, please note that there is also the question of port names. Do we allow port=>'http'? If yes then it is more flexible but there will be less data validation (I mean more invalid data allowed).

For addresses, there is a similar question: do we allow set names like in saddr=>'@trusted-ips'?

In this tradeoff between flexibility and data validation I don't know where to put the cursor...

@nbarrientos
Copy link
Collaborator Author

nbarrientos commented Dec 3, 2020

I've just added a proposed implementation for daddr and the counter bit. I won't add the dual saddr and sport until we've reached satisfactory implementations and interfaces for their counterparts. I'll stop here for the time being, lemme know what you think.

@nbarrientos
Copy link
Collaborator Author

For ports, please note that there is also the question of port names. Do we allow port=>'http'? If yes then it is more flexible but there will be less data validation (I mean more invalid data allowed).

Yes, it makes sense, but what about dport => ['http', 'https']? That should be legal too.

@traylenator
Copy link
Collaborator

traylenator commented Dec 4, 2020

Yes, it makes sense, but what about dport => ['http', 'https']? That should be legal too.

No. Espeically since you are permitted numerics in service names the validation loss is too great and your never know what is in the /etc/services file on the destination host exactly.

@duritong
Copy link
Collaborator

duritong commented Dec 4, 2020

I like it so far

@nbarrientos
Copy link
Collaborator Author

So, shall I implement saddr and sport and document the interface? What do we finally do about the destination table? What about iifname and oifname?

templates/simplerule.epp Outdated Show resolved Hide resolved
@traylenator traylenator added the enhancement New feature or request label Dec 9, 2020
@traylenator
Copy link
Collaborator

traylenator commented Dec 9, 2020

So, shall I implement saddr and sport and document the interface? What do we finally do about the destination table? What about iifname and oifname?

saddr and sport I would say are useful, apart from anything else it's presumably same code as daddr and dport.

As for i/oname can leave for now and add later if requested. It would be reasnable request.

@nbarrientos nbarrientos force-pushed the simplerule branch 3 times, most recently from 508e0a0 to 1af67e4 Compare December 9, 2020 11:51
@nbarrientos
Copy link
Collaborator Author

sport and saddr added. Some documentation added, too.

manifests/simplerule.pp Outdated Show resolved Hide resolved
@nbarrientos nbarrientos marked this pull request as ready for review December 9, 2020 13:44
@nbarrientos nbarrientos force-pushed the simplerule branch 2 times, most recently from 6e56b8f to 908a691 Compare December 9, 2020 14:19
@duritong
Copy link
Collaborator

duritong commented Dec 9, 2020

all good, but you need to rebase

@traylenator
Copy link
Collaborator

@LionelCons anything to add ?

@LionelCons
Copy link

Yes, I have something to add...

This is excellent work, thank you very much!

@nbarrientos nbarrientos merged commit 3fe51d6 into voxpupuli:master Dec 10, 2020
figless pushed a commit to figless/puppet-nftables that referenced this pull request Aug 25, 2021
bc1b0f1 Release 1.0.0 (voxpupuli#49)
5d71ec6 Merge pull request voxpupuli#56 from traylenator/ports
94a8062 Use Stdlib::Port everywhere in place of Integer
b1085d8 Merge pull request voxpupuli#55 from traylenator/moredocs
c868cae Update manifests/set.pp
13f4e4c Docs for nftables::set
b3040dd Merge pull request voxpupuli#42 from duritong/terminology
04176b0 switch naming to puppetserver
3820575 Merge pull request voxpupuli#47 from cernops/issue45
948ebc9 Prefix custom tables with custom- so they're loaded
bacf254 Merge pull request voxpupuli#48 from cernops/config_template
c2800a3 Merge pull request voxpupuli#50 from traylenator/moretests
2075a72 Correct NFS udp and tcp port matching
cfcafde test that all classes can be included
d875244 test that bad configuration leaves service running
cba0cb8 Merge pull request voxpupuli#52 from cernops/simplerule_reference
b46c9ce Remove a blank separating the doc string and the code
c7e37bd Merge pull request voxpupuli#51 from bastelfreak/puppet7
e0be819 Enable Puppet 7 support
3fe51d6 Merge pull request voxpupuli#33 from cernops/simplerule
c5418fd Validate table spec
04f5c03 Fix context name (removes dup)
294a38f Implement intended failure
fcb1d35 Auto fill simple table configuration
4d63add Refresh REFERENCE
42e7f3e Relax type validation in template
5527702 Align template parameters
f1ef02c Encapsulate addr-related exprs in Nftables::Addr
09b07e5 Encapsulate port-related exprs in Nftables::Port
6739966 Sort template parameters alphabetically
3a469f2 Implement nftables::simplerule::saddr
abb04c9 Mention nftables::simplerule in the README
5944b9c Allow some other types of verdicts
2f28cce Document nftables::simplerule's parameters
af15de4 Recommend using nftables::rule
77abc10 Implement nftables::simplerule::sport
fb58f7b Remove double spacing
6793d28 Handle dport internally always as an array
467ea4e Lint fixes
2cc5430 Remove optional modifier on $table
2489f93 Correct error message
4ec9461 Re-document and add example
d43ced4 Implement nftables:;simplerule::counter
aaa3717 Implement nftables:;simplerule::daddr
d38aab5 Test passing a port without protocol
316bc3f Allow IPv4 and IPv6 only rules
3a52fb4 Richer dport
fb65734 s/setname/rulename
83382bb Add nftables::simplerule
f0bd879 Merge pull request voxpupuli#34 from traylenator/dedupe_flush
354a3ea Merge pull request voxpupuli#44 from traylenator/formatting
b978500 Correct layout of ignore chain example
ce22630 Remove duplicate flush on reload
03d8e69 Merge pull request voxpupuli#41 from traylenator/rubocop
139ec11 Merge pull request voxpupuli#43 from cernops/doc_typos
1330c27 Add a hint about changing default output configuration
8ded326 Fix typo in class name
4ed97e5 Add a separation between the header and the content
620da9a Add remark about the global chain
0f31ffb Fix grammatical error
1ffab17 Add full stop
7e5b657 rubocop:auto_correct fixes
da8956d Enable rubocop check
492ca83 Disable Disable TrailingCommaInArguments early
c4b1b93 Comment why firewalld_enable parameter is required (voxpupuli#40)
bd5145a Add basic configuration validation acceptance test (voxpupuli#38)

git-subtree-dir: code
git-subtree-split: bc1b0f1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule API
5 participants