MODULES-1469, MODULES-1470: Support alias (eth0:0), negation for iniface, outiface #433

hesco · 2014-11-10T18:40:02Z

Patch the data validation regexp and documentation.
This should resolve:
https://tickets.puppetlabs.com/browse/MODULES-1469
https://tickets.puppetlabs.com/browse/MODULES-1470

…on regexp, docs

underscorgan · 2014-11-21T23:29:11Z

@hesco This is causing failures in the acceptance tests:

  1) complex ruleset 2 contains appropriate rules                                                                                                                                                                                    [37/4850]
     Failure/Error: expect(r.stdout).to match(line)
       expected "# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014\n*raw\n:PREROUTING ACCEPT [216:25716]\n:OUTPUT ACCEPT [162:22404]\nCOMMIT\n# Completed on Thu Nov 20 23:22:48 2014\n# Generated by iptables-save v1.4.7 on Th
u Nov 20 23:22:48 2014\n*filter\n:INPUT DROP [0:0]\n:FORWARD DROP [0:0]\n:OUTPUT ACCEPT [40:4808]\n:LOCAL_INPUT - [0:0]\n:LOCAL_INPUT_PRE - [0:0]\n-A INPUT -m comment --comment \"001 LOCAL_INPUT_PRE\" -j LOCAL_INPUT_PRE \n-A INPUT -m comm
ent --comment \"010 INPUT allow established and related\" -m state --state RELATED,ESTABLISHED -j ACCEPT \n-A INPUT -d 127.0.0.0/8 ! -i lo -m comment --comment \"011 reject local traffic not on loopback interface\" -j REJECT --reject-with
 icmp-port-unreachable \n-A INPUT -i lo -m comment --comment \"012 accept loopback\" -j ACCEPT \n-A INPUT -p icmp -m comment --comment \"013 icmp destination-unreachable\" -m icmp --icmp-type 3 -j ACCEPT \n-A INPUT -s 10.0.0.0/8 -p icmp -
m comment --comment \"013 icmp echo-request\" -m icmp --icmp-type 8 -j ACCEPT \n-A INPUT -p icmp -m comment --comment \"013 icmp time-exceeded\" -m icmp --icmp-type 11 -j ACCEPT \n-A INPUT -p tcp -m multiport --dports 22 -m comment --comm
ent \"020 ssh\" -m state --state NEW -j ACCEPT \n-A INPUT -i eth0:3 -p tcp -m multiport --dports 443 -m comment --comment \"443 ssl on aliased interface\" -m state --state NEW -j ACCEPT \n-A INPUT -m comment --comment \"900 LOCAL_INPUT\"
-j LOCAL_INPUT \n-A INPUT -m comment --comment \"999 reject\" -j REJECT --reject-with icmp-host-prohibited \n-A FORWARD -m comment --comment \"010 allow established and related\" -m state --state RELATED,ESTABLISHED -j ACCEPT \n-A OUTPUT
! -o eth0:2 -p tcp -m multiport --dports 25 -m comment --comment \"025 smtp\" -m state --state NEW -j ACCEPT \nCOMMIT\n# Completed on Thu Nov 20 23:22:48 2014\n# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014\n*nat\n:PREROU
TING ACCEPT [0:0]\n:POSTROUTING ACCEPT [0:0]\n:OUTPUT ACCEPT [0:0]\n-A POSTROUTING -o eth0 -p tcp -m comment --comment \"090 ignore ipsec\" -m policy --dir out --pol ipsec -j ACCEPT \n-A POSTROUTING -d 10.0.0.0/8 -o eth0 -p tcp -m comment
 --comment \"093 ignore 10.0.0.0/8\" -j ACCEPT \n-A POSTROUTING -d 172.16.0.0/12 -o eth0 -p tcp -m comment --comment \"093 ignore 172.16.0.0/12\" -j ACCEPT \n-A POSTROUTING -d 192.168.0.0/16 -o eth0 -p tcp -m comment --comment \"093 ignor
e 192.168.0.0/16\" -j ACCEPT \nCOMMIT\n# Completed on Thu Nov 20 23:22:48 2014\n# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014\n*mangle\n:PREROUTING ACCEPT [222:26144]\n:INPUT ACCEPT [222:26144]\n:FORWARD ACCEPT [0:0]\n:O
UTPUT ACCEPT [166:23984]\n:POSTROUTING ACCEPT [166:23984]\nCOMMIT\n# Completed on Thu Nov 20 23:22:48 2014\n" to match /-A INPUT -d 127.0.0.0\/8 ! -i lo -p tcp -m comment --comment "011 reject local traffic not on loopback interface" -j R
EJECT/
       Diff:
       @@ -1,2 +1,49 @@
       -/-A INPUT -d 127.0.0.0\/8 ! -i lo -p tcp -m comment --comment "011 reject local traffic not on loopback interface" -j REJECT/
       +# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014
       +*raw
       +:PREROUTING ACCEPT [216:25716]
       +:OUTPUT ACCEPT [162:22404]
       +COMMIT
       +# Completed on Thu Nov 20 23:22:48 2014
       +# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014
       +*filter
       +:INPUT DROP [0:0]
       +:FORWARD DROP [0:0]
       +:OUTPUT ACCEPT [40:4808]
       +:LOCAL_INPUT - [0:0]
       +:LOCAL_INPUT_PRE - [0:0]
       +-A INPUT -m comment --comment "001 LOCAL_INPUT_PRE" -j LOCAL_INPUT_PRE
       +-A INPUT -m comment --comment "010 INPUT allow established and related" -m state --state RELATED,ESTABLISHED -j ACCEPT
       +-A INPUT -d 127.0.0.0/8 ! -i lo -m comment --comment "011 reject local traffic not on loopback interface" -j REJECT --reject-with icmp-port-unreachable
       +-A INPUT -i lo -m comment --comment "012 accept loopback" -j ACCEPT
       +-A INPUT -p icmp -m comment --comment "013 icmp destination-unreachable" -m icmp --icmp-type 3 -j ACCEPT
       +-A INPUT -s 10.0.0.0/8 -p icmp -m comment --comment "013 icmp echo-request" -m icmp --icmp-type 8 -j ACCEPT
       +-A INPUT -p icmp -m comment --comment "013 icmp time-exceeded" -m icmp --icmp-type 11 -j ACCEPT
       +-A INPUT -p tcp -m multiport --dports 22 -m comment --comment "020 ssh" -m state --state NEW -j ACCEPT
       +-A INPUT -i eth0:3 -p tcp -m multiport --dports 443 -m comment --comment "443 ssl on aliased interface" -m state --state NEW -j ACCEPT
       +-A INPUT -m comment --comment "900 LOCAL_INPUT" -j LOCAL_INPUT
       +-A INPUT -m comment --comment "999 reject" -j REJECT --reject-with icmp-host-prohibited
       +-A FORWARD -m comment --comment "010 allow established and related" -m state --state RELATED,ESTABLISHED -j ACCEPT
       +-A OUTPUT ! -o eth0:2 -p tcp -m multiport --dports 25 -m comment --comment "025 smtp" -m state --state NEW -j ACCEPT
       +COMMIT
       +# Completed on Thu Nov 20 23:22:48 2014
       +# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014
       +*nat
       +:PREROUTING ACCEPT [0:0]
       +:POSTROUTING ACCEPT [0:0]
       +:OUTPUT ACCEPT [0:0]
       +-A POSTROUTING -o eth0 -p tcp -m comment --comment "090 ignore ipsec" -m policy --dir out --pol ipsec -j ACCEPT
       +-A POSTROUTING -d 10.0.0.0/8 -o eth0 -p tcp -m comment --comment "093 ignore 10.0.0.0/8" -j ACCEPT
       +-A POSTROUTING -d 172.16.0.0/12 -o eth0 -p tcp -m comment --comment "093 ignore 172.16.0.0/12" -j ACCEPT
       +-A POSTROUTING -d 192.168.0.0/16 -o eth0 -p tcp -m comment --comment "093 ignore 192.168.0.0/16" -j ACCEPT
       +COMMIT
       +# Completed on Thu Nov 20 23:22:48 2014
       +# Generated by iptables-save v1.4.7 on Thu Nov 20 23:22:48 2014
       +*mangle
       +:PREROUTING ACCEPT [222:26144]
       +:INPUT ACCEPT [222:26144]
       +:FORWARD ACCEPT [0:0]
       +:OUTPUT ACCEPT [166:23984]
       +:POSTROUTING ACCEPT [166:23984]
       +COMMIT
       +# Completed on Thu Nov 20 23:22:48 2014
     # ./spec/acceptance/rules_spec.rb:271:in `block (4 levels) in <top (required)>'
     # ./spec/acceptance/rules_spec.rb:252:in `each'
     # ./spec/acceptance/rules_spec.rb:252:in `block (3 levels) in <top (required)>'
     # ./spec/acceptance/rules_spec.rb:250:in `block (2 levels) in <top (required)>'

Finished in 4 minutes 49.9 seconds
214 examples, 1 failure

Failed examples:

rspec ./spec/acceptance/rules_spec.rb:249 # complex ruleset 2 contains appropriate rules

I haven't had a chance to diagnose this yet, but if you want to debug that would certainly help!

hesco · 2014-11-22T01:05:43Z

Thanks Morgan for getting back with me on this.

The additional rule I added to the spec/acceptance/rules_spec.rb file (see lines 149-153 and 259), include this new rule. That revised test script passed the Travis CI tests. I am unsure whether the diff you reproduce above is truncated or not. The line counts in the diff suggest you expect 49 lines of output, but are getting back only two lines of output. But that does not make any sense to me, as this is working for me in my local environment. And Travis CI reports a green test as well.

The difference between these two rules relates to the --reject-with attribute tacked on the end, which my test does not anticipate. If including that in the expected output would resolve your concern for regression, I'd be fine with psuh'ing another commit into this PR. That these packets would be rejected as icmp unreachable is not inappropriate. And on closer examination, I see that my expected output limits this to the tcp protocol. The rule actually ought to apply to all protocols. I will also make that adjustment.

Please clarify if this is the only point of failure in this test, or if I am mis-reading this somehow? Are you really only getting two lines of output for the 49 expected?

-/-A INPUT -d 127.0.0.0\/8 ! -i lo -p tcp -m comment --comment "011 reject local traffic not on loopback interface" -j REJECT/
+-A INPUT -d 127.0.0.0/8 ! -i lo -m comment --comment "011 reject local traffic not on loopback interface" -j REJECT --reject-with icmp-port-unreachable

elyscape · 2014-11-22T08:04:58Z

Are you really only getting two lines of output for the 49 expected?

It's getting 49 lines of output, none of which match that particular line. Adding the --reject-with icmp-port-unreachable is part of it, but your rules_spec.rb file also doesn't have port => 'tcp', listed on the "011 reject local traffic not on loopback interface" rule you added. Your test expects that to be there. Additionally, your test:

/-A INPUT -d 127.0.0.0\/8 ! -i lo -p tcp -m comment --comment "011 reject local traffic not on loopback interface" -j REJECT/,

doesn't escape the quotes. All those changes together should resolve the issue.

hesco · 2014-11-22T15:01:47Z

@elyscape and @mhaskel : thanks for the guidance. Travis seems happy still. See please if that passes the acceptance tests in your environment or if you need anything further from me on this. Thanks. -- Hugh

This should resolve the last of the acceptance test issues.

underscorgan · 2014-12-02T22:13:41Z

@hesco I've submitted a PR against your branch resolving the last of the acceptance test issues: hesco#1

Other than merging that, if you could rebase and squash the work to a smaller number of commits that would be great.

Thanks again!

Fix tests for puppetlabs#433

…on regexp, docs This is a combination of 8 commits. Adds unit tests for (in|out)iface negation and interface alias Add acceptance test for interface alias and negation for (in|out)iface standard usage: -A INPUT -d 127.0.0.0/8 ! -i lo -j REJECT Add test cases to conversion hash for interface aliases and negation Add (in|out)iface negation to Rule inversion list in README Fixes to failing acceptance test, update README for REJECT ! -i lo rule Fix tests for puppetlabs#433 This should resolve the last of the acceptance test issues.

hesco · 2014-12-03T04:35:25Z

merged, rebased, squashed, travis-happy and ready for consideration by upstream project.
Thanks for that final patch. -- Hugh

underscorgan · 2014-12-03T15:26:16Z

@hesco did you git push --force after squashing? I'm still seeing 11 commits. Ideally we'd like to see a smaller number of commits before merging.

hesco · 2014-12-03T18:42:11Z

Resolved by #435

hesco added 6 commits November 10, 2014 13:31

Support alias (eth0:0), negation for iniface, outiface. data validati…

2cf171d

…on regexp, docs

Adds unit tests for (in|out)iface negation and interface alias

f339945

Add acceptance test for interface alias and negation for (in|out)iface

47125db

standard usage: -A INPUT -d 127.0.0.0/8 ! -i lo -j REJECT

73914b5

Add test cases to conversion hash for interface aliases and negation

3c2d07f

Add (in|out)iface negation to Rule inversion list in README

7cb5fc2

underscorgan added the tests-fail label Nov 21, 2014

Fixes to failing acceptance test, update README for REJECT ! -i lo rule

8f8524e

Fix tests for puppetlabs#433

6490838

This should resolve the last of the acceptance test issues.

hesco added 3 commits December 2, 2014 18:59

Merge pull request #1 from mhaskel/test_fixes

346bc83

Fix tests for puppetlabs#433

Merge branch 'master' of https://github.com/hesco/puppetlabs-firewall

b76c3b1

underscorgan added feature and removed tests-fail labels Dec 3, 2014

underscorgan mentioned this pull request Dec 3, 2014

MODULES-1469 MODULES-1470 Support alias (eth0:0), negation for iniface, ... #435

Merged

hesco closed this Dec 3, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MODULES-1469, MODULES-1470: Support alias (eth0:0), negation for iniface, outiface #433

MODULES-1469, MODULES-1470: Support alias (eth0:0), negation for iniface, outiface #433

hesco commented Nov 10, 2014

underscorgan commented Nov 21, 2014

hesco commented Nov 22, 2014

elyscape commented Nov 22, 2014

hesco commented Nov 22, 2014

underscorgan commented Dec 2, 2014

hesco commented Dec 3, 2014

underscorgan commented Dec 3, 2014

hesco commented Dec 3, 2014

MODULES-1469, MODULES-1470: Support alias (eth0:0), negation for iniface, outiface #433

MODULES-1469, MODULES-1470: Support alias (eth0:0), negation for iniface, outiface #433

Conversation

hesco commented Nov 10, 2014

underscorgan commented Nov 21, 2014

hesco commented Nov 22, 2014

elyscape commented Nov 22, 2014

hesco commented Nov 22, 2014

underscorgan commented Dec 2, 2014

hesco commented Dec 3, 2014

underscorgan commented Dec 3, 2014

hesco commented Dec 3, 2014