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

Inconsistent treatment of the action attribute #28

Open
pwalz opened this Issue Jun 28, 2017 · 5 comments

Comments

Projects
None yet
2 participants
@pwalz

pwalz commented Jun 28, 2017

Cookbook version

3.1.0

Chef-client version

12.21.1 or 13.1.31

Platform Details

Ubuntu 14.04

Scenario:

There seem to be some lingering inconsistencies in how the ufw cookbook treats the action attribute of a firewall rule. The example files (such as examples/roles/fw_example.rb) and examples in the readme treat it as a UFW action (deny or allow), while recipes/default.rb seems to lean toward treating it as a Chef action (see line 50). I think either treatment could be fine, but it needs to be consistent throughout the cookbook. I have a personal preference for treating it as a UFW action because that would be more backwards-compatible; the UFW cookbook could assume the Chef action for a firewall_rule should always be :create.

Steps to Reproduce:

  1. Take the example role attributes from README.md and apply them to your node
  2. Add ufw::default to your node's run list
  3. Run chef-client

Expected Result:

One of the rules created in UFW should have an action of Deny.

Actual Result:

chef-client bombs out when it tries to create the "block Tomcat" firewall_rule with a deny action.

Error output and the .kitchen.yml file that I'm using are available here: https://gist.github.com/pwalz/c36ced87559b56c51603755513a09102

@iennae

This comment has been minimized.

Show comment
Hide comment
@iennae

iennae Jun 28, 2017

Contributor

Thanks for reporting. I think the issue here is the documentation. It looks like firewall_rule has different attributes.

Contributor

iennae commented Jun 28, 2017

Thanks for reporting. I think the issue here is the documentation. It looks like firewall_rule has different attributes.

@iennae

This comment has been minimized.

Show comment
Hide comment
@iennae

iennae Jun 28, 2017

Contributor

I changed the configuration to

attributes:
      firewall:
        rules:
          - tftp: { }
          - http: { port: 80 }
          - block tomcat from 192.168.1.0/24: { port: 8080, source: 192.168.1.0/24, command: deny }
          - Allow access to udp 1.2.3.4 port 5469 from 1.2.3.5 port 5469: { protocol: udp, port: 5469, source: 1.2.3.4, destination: 1.2.3.5, dest_port: 5469 }
          - allow to tcp ports 8000-8010 from 192.168.1.0/24: { port: 8000..8010, source: 192.168.1.0/24, protocol: tcp }

and get a successful converge. Note that port_range got merged in the firewall cookbook to port.

Contributor

iennae commented Jun 28, 2017

I changed the configuration to

attributes:
      firewall:
        rules:
          - tftp: { }
          - http: { port: 80 }
          - block tomcat from 192.168.1.0/24: { port: 8080, source: 192.168.1.0/24, command: deny }
          - Allow access to udp 1.2.3.4 port 5469 from 1.2.3.5 port 5469: { protocol: udp, port: 5469, source: 1.2.3.4, destination: 1.2.3.5, dest_port: 5469 }
          - allow to tcp ports 8000-8010 from 192.168.1.0/24: { port: 8000..8010, source: 192.168.1.0/24, protocol: tcp }

and get a successful converge. Note that port_range got merged in the firewall cookbook to port.

@pwalz

This comment has been minimized.

Show comment
Hide comment
@pwalz

pwalz Jun 28, 2017

Thanks - that does allow the converge to succeed, but it also results in the rule getting applied with an allow action, not deny (which, obviously, is a bad thing). This is because ufw::default doesn't copy the command parameter into the resulting firewall_rule resource.

Also, providing a range for the port attribute doesn't work - ufw::default converts it to an int when passing the attribute to the firewall_rule resource, so the resulting rule only applies to one port at the beginning of the provided range.

I'm happy to submit PRs for these problems - I just needed some direction on whether the desired solution was to make a breaking change for the action parameter (and the answer seems to be yes).

pwalz commented Jun 28, 2017

Thanks - that does allow the converge to succeed, but it also results in the rule getting applied with an allow action, not deny (which, obviously, is a bad thing). This is because ufw::default doesn't copy the command parameter into the resulting firewall_rule resource.

Also, providing a range for the port attribute doesn't work - ufw::default converts it to an int when passing the attribute to the firewall_rule resource, so the resulting rule only applies to one port at the beginning of the provided range.

I'm happy to submit PRs for these problems - I just needed some direction on whether the desired solution was to make a breaking change for the action parameter (and the answer seems to be yes).

@iennae

This comment has been minimized.

Show comment
Hide comment
@iennae

iennae Jun 28, 2017

Contributor

PR's gladly accepted. I've been looking at the firewall_resource and realizing that the range doesn't seem to work correctly.

Contributor

iennae commented Jun 28, 2017

PR's gladly accepted. I've been looking at the firewall_resource and realizing that the range doesn't seem to work correctly.

@iennae iennae referenced this issue Jun 28, 2017

Closed

Fixing use of firewall_rule in default recipe. #29

0 of 4 tasks complete
@iennae

This comment has been minimized.

Show comment
Hide comment
@iennae

iennae Jun 28, 2017

Contributor

Take a look at the PR I've started so that we don't duplicate work. Let me know what you think.

Contributor

iennae commented Jun 28, 2017

Take a look at the PR I've started so that we don't duplicate work. Let me know what you think.

@cl-lab-k cl-lab-k referenced a pull request that will close this issue Sep 28, 2018

Open

use command attribute instead of action attribute #38

1 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment