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

UFW (Uncomplicated FireWall) #748

Merged
merged 1 commit into from Apr 11, 2019
Merged

UFW (Uncomplicated FireWall) #748

merged 1 commit into from Apr 11, 2019

Conversation

@markasoftware
Copy link

markasoftware commented Apr 3, 2019

I have added the following types:

  • __ufw: Install and optionally enable UFW
  • __ufw_rule: Add a normal UFW rule, with all of the UFW command line options translated into cdist-friendly parameters.
  • __ufw_default: Set default policies (very simple).

This is my first time contributing to cdist, so I have a question: Since UFW's command line is idempotent, my types do not bother using explorers to check whether their work needs to be done -- gencode-remote always outputs something. An explorer would have to call ufw status verbose, which is not substantially faster than just doing the work I want to do. Is this OK?

Thanks for reviewing this!

cdist/conf/type/__ufw/man.rst Outdated Show resolved Hide resolved
@darko-poljak darko-poljak requested review from asteven and telmich Apr 4, 2019
@darko-poljak

This comment has been minimized.

Copy link
Contributor

darko-poljak commented Apr 4, 2019

@4nd3r You might be interested in this type. Wanna review? :)

@markasoftware

This comment has been minimized.

Copy link
Author

markasoftware commented Apr 4, 2019

After opening this I have realized there are a few changes I want to make: removing __ufw_default and including its functionality as --default_incoming, etc on __ufw, and adding a __ufw_route type which would be mostly copy paste from ufw rule, but for routes that do not terminate on the local machine (ufw has a slightly different syntax for these).

@darko-poljak

This comment has been minimized.

Copy link
Contributor

darko-poljak commented Apr 4, 2019

@markasoftware Ok, go ahead and improve your type :)

@4nd3r

This comment has been minimized.

Copy link

4nd3r commented Apr 4, 2019

@darko-poljak i'm from iptables generation and i have no experience with ufw 😿

but i will skim through a bit...

@4nd3r

This comment has been minimized.

Copy link

4nd3r commented Apr 4, 2019

would it be possible to generate all rules before adding them with ufw and then diff them with currently active rules? i, myself, wouldn't wrap ufw syntax into cdist parameters, but just use multiple --rule parameters which contains ufw syntax.

so, for example, you want to add rules foo, bar, baz and current active rules are foo and bar - rule baz will be added. or you want to add rules foo, bar and active rules are foo, bar, baz - baz will be removed.

active rules before: foo, bar
run: __ufw --rule 'foo'  --rule 'bar' --rule 'baz'
active rules after: foo, bar, baz

or

active rules before: foo, bar, baz
run: __ufw --rule 'foo' --rule 'bar'
active rules after: foo, bar

but this way you can only define ufw once per configuration run, because if you define ufw twice, then last ufw will remove first one's rules.

@darko-poljak

This comment has been minimized.

Copy link
Contributor

darko-poljak commented Apr 4, 2019

would it be possible to generate all rules before adding them with ufw and then diff them with currently active rules? i, myself, wouldn't wrap ufw syntax into cdist parameters, but just use multiple --rule parameters which contains ufw syntax.

so, for example, you want to add rules foo, bar, baz and current active rules are foo and bar - rule baz will be added. or you want to add rules foo, bar and active rules are foo, bar, baz - baz will be removed.

active rules before: foo, bar
run: __ufw --rule 'foo'  --rule 'bar' --rule 'baz'
active rules after: foo, bar, baz

or

active rules before: foo, bar, baz
run: __ufw --rule 'foo' --rule 'bar'
active rules after: foo, bar

but this way you can only define ufw once per configuration run, because if you define ufw twice, then last ufw will remove first one's rules.

There could be an option to leave old rules or remove them, where by default they won't be removed.
I also agree with --rule multiple parameter, instead of wrapping all ufw options in cdist type.

@markasoftware

This comment has been minimized.

Copy link
Author

markasoftware commented Apr 4, 2019

As soon as we put it in __ufw it becomes impossible for rules to be added in multiple locations.

There could be an option to leave old rules or remove them, where by default they won't be removed.
I also agree with --rule multiple parameter, instead of wrapping all ufw options in cdist type.

I may be wrong, but I was under the impression that if you attempt to instantiate the same object multiple times, all invocations after the first will be ignored regardless of how the type is written. We also have __firewalld_rule and __iptables_rule, not __firewalld and __iptables. Each rule is conceptually a separate "object" IMO.

I agree that wrapping UFW's syntax may be unnecessary though. If we follow __iptables_rule's lead, I could modify __ufw_rule so that the only parameter is rule, in UFW's format.

would it be possible to generate all rules before adding them with ufw and then diff them with currently active rules?

Current rules can be listed with ufw status or ufw show added. Neither necessarily shows rules in the exact same format as they were entered on the command line -- ufw status has a completely different format, and ufw show added removes unnecessary parts of the command line. Either way, converting between these formats and the CLI format to create a new rule is not trivial.

I think the proper solution to the diffing problem would be "singleton explorers" which run only once for a type, not once per object. Then a type like __ufw_rule would have a singleton explorer like rules_is which gathers the list of rules at the very start, before any of the gencodes run. Then each individual __ufw_rule gencode could check against that explorer very quickly, without running it again. Unfortunately, cdist does not support this (yet!). It might be possible to hack it together with messaging, though.

To summarize, the options are:

  • Use __ufw and add an optional multiple parameter --rule. Pros: better performance. Cons: Hard to convert between rule and status syntax, non-idiomatic (rules are logically separate objects). I don't like it.
  • Keep things as is. Pros: It works. Cons: Unnecessary wrapping of UFW syntax, relatively slow when rules already exist because it tries to add them unconditionally.
  • Keep separate __ufw_rule type, but take a --rule parameter that just accepts the command line syntax for the UFW rule instead of the current hodgepodge of parameters. Pros: Follows the precedent set by __firewalld_rule and __iptables_rule and simple for users who are used to UFW's cli. Cons: Relatively slow when rules already exist.

I am fine with removing the custom parameters, but I strongly believe __ufw_rule should be separate from __ufw.

@darko-poljak

This comment has been minimized.

Copy link
Contributor

darko-poljak commented Apr 5, 2019

@markasoftware Since I don't know ufw, I don't have arguments for the way this type is implemented, or how it should be implemented. I will trust you.
@markasoftware Do you have some changes in mind?

@telmich @asteven Do you have any remarks/suggestions?

@markasoftware

This comment has been minimized.

Copy link
Author

markasoftware commented Apr 6, 2019

After reading your comments and other types, I plan to change ufw rule to not wrap ufw syntax.

@markasoftware

This comment has been minimized.

Copy link
Author

markasoftware commented Apr 8, 2019

I have removed the syntax wrapping in __ufw_rule. Unless there are further comments I think this is ready to merge.

cdist/conf/type/__ufw/man.rst Outdated Show resolved Hide resolved
@markasoftware markasoftware force-pushed the markasoftware:ufw branch from 2f8e984 to ab6cfe2 Apr 9, 2019
cdist/conf/type/__ufw/man.rst Outdated Show resolved Hide resolved
@markasoftware markasoftware force-pushed the markasoftware:ufw branch from ab6cfe2 to 0ce96f0 Apr 11, 2019
@darko-poljak darko-poljak merged commit 1ba5f62 into ungleich:master Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.