vyos-1x icon indicating copy to clipboard operation
vyos-1x copied to clipboard

firewall: T4694: Adding rt ipsec exists/missing match to firewall configs

Open talmakion opened this issue 1 year ago • 6 comments

Change Summary

Current firewall syntax only permits inbound matching of IPsec, using "meta ipsec", which indicates a packet was decrypted from IPsec encap.

This extends firewall capability to also be able to match outbound traffic using "rt ipsec", which indicates a packet will be encrypted into an IPsec encap.

"meta ipsec" is not valid (nft will refuse to load config) inside output hooks so additional care has been taken to ensure it doesn't end up in output or any inappropriate jump targets from an output chain.

In doing these checks, I've also added an infinite jump-target loop check to the validation pass.

Types of changes

  • [ ] Bug fix (non-breaking change which fixes an issue)
  • [x] New feature (non-breaking change which adds functionality)
  • [ ] Code style update (formatting, renaming)
  • [ ] Refactoring (no functional changes)
  • [ ] Migration from an old Vyatta component to vyos-1x, please link to related PR inside obsoleted component
  • [x] Other (please describe): Changes to existing firewall ipsec match syntax

Related Task(s)

  • https://vyos.dev/T4694
  • https://vyos.dev/T4667

Related PR(s)

  • https://github.com/vyos/vyos-1x/pull/3637

Component(s) name

  • firewall

Proposed changes

I've covered some concerns I have about my implementation in the forum: https://forum.vyos.io/t/outbound-ipsec-filtering-by-firewall-would-like-some-dev-opinions/14710.

  • Change ipsec match-ipsec/none to match-ipsec-in and match-none-in for fw rules
  • Add ipsec match-ipsec-out and match-none-out
    • *-in means "decrypted" and *-out means "encrypting", the names may benefit from tweaking to make intent clearer
  • Change all the points where the match-ipsec.xml.i include was used before, making sure the new includes (match-ipsec-in/out.xml.i) are used appropriately. There were a handful of spots where match-ipsec.xml.i had snuck back in for output hooked chains already (the common-rule-* includes)
  • Add the -out generators to rendered templates
  • Heavy modification to firewall config validators:
    • I needed to check for ipsec-in matches no matter how deeply nested under an output-hook chain(via jump-target) - this always generates an error.
    • Ended up retrofitting the jump-targets validator from root chains and for named custom chains. It checks for recursive loops and bad IPsec. However, it could be improved - it will not detect a cycle between 2 otherwise un-referenced named chains.
    • Alternatively, perhaps custom chains shouldn't be able to jump or have depth restrictions. The code is already capable of the latter (currently issues a depth warning at 7 jumps deep).

How to test

  • Configured VyOS testing pair in a "triangle" (LEFT -> FAKEWWW <- RIGHT)
  • 2 additional VMs behind LEFT and RIGHT for forward/prerouting chain testing
  • Applied numerous combinations of GRE & IPsec matches in logging/nflogging rules across all filter chains
  • Created several common IPsec & clear use cases for both ptp and forwarded traffic:
    • DMVPN clear
    • DMVPN encrypted
    • IPsec transport, PtP GRE encrypted
    • IPsec tunnel mode
  • Checking dmesg output and tcpdump on FAKEWWW for expected operation in hitting all the right rules
  • Combined with GRE-match PR for complete testing in a handful of logical and completely stupid scenarios
  • Created specific scenarios for blocking DMVPN traffic in the clear when SAs go down, without interfering with other GRE/IPsec traffic, checking coverage on the original ticket issues
  • Exercised validators and ensured all possible combinations of new matches to chains result in working nftables rules.

Smoketest result

Note, the groups failure below is also encountered in a clean rolling image:

# python3 /usr/libexec/vyos/tests/smoke/cli/test_firewall.py 
test_bridge_basic_rules (__main__.TestFirewall.test_bridge_basic_rules) ... ok
test_flow_offload (__main__.TestFirewall.test_flow_offload) ... ok
test_geoip (__main__.TestFirewall.test_geoip) ... ok
test_groups (__main__.TestFirewall.test_groups) ... FAIL
test_ipv4_advanced (__main__.TestFirewall.test_ipv4_advanced) ... ok
test_ipv4_basic_rules (__main__.TestFirewall.test_ipv4_basic_rules) ... ok
test_ipv4_dynamic_groups (__main__.TestFirewall.test_ipv4_dynamic_groups) ... ok
test_ipv4_global_state (__main__.TestFirewall.test_ipv4_global_state) ... ok
test_ipv4_mask (__main__.TestFirewall.test_ipv4_mask) ... ok
test_ipv4_state_and_status_rules (__main__.TestFirewall.test_ipv4_state_and_status_rules) ... ok
test_ipv4_synproxy (__main__.TestFirewall.test_ipv4_synproxy) ... ok
test_ipv6_advanced (__main__.TestFirewall.test_ipv6_advanced) ... ok
test_ipv6_basic_rules (__main__.TestFirewall.test_ipv6_basic_rules) ... ok
test_ipv6_dynamic_groups (__main__.TestFirewall.test_ipv6_dynamic_groups) ... ok
test_ipv6_mask (__main__.TestFirewall.test_ipv6_mask) ... ok
test_nested_groups (__main__.TestFirewall.test_nested_groups) ... ok
test_source_validation (__main__.TestFirewall.test_source_validation) ... ok
test_sysfs (__main__.TestFirewall.test_sysfs) ... ok
test_timeout_sysctl (__main__.TestFirewall.test_timeout_sysctl) ... ok
test_zone_basic (__main__.TestFirewall.test_zone_basic) ... ok
test_zone_flow_offload (__main__.TestFirewall.test_zone_flow_offload) ... ok

======================================================================
FAIL: test_groups (__main__.TestFirewall.test_groups)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/libexec/vyos/tests/smoke/cli/test_firewall.py", line 152, in test_groups
    self.verify_nftables(nftables_search, 'ip vyos_filter')
  File "/usr/libexec/vyos/tests/smoke/cli/base_vyostest_shim.py", line 122, in verify_nftables
    self.assertTrue(not matched if inverse else matched, msg=search)
AssertionError: False is not true : ['elements = { 192.0.2.5, 192.0.2.8,']

----------------------------------------------------------------------
Ran 21 tests in 184.628s

FAILED (failures=1)

Checklist:

  • [x] I have read the CONTRIBUTING document
  • [x] I have linked this PR to one or more Phabricator Task(s)
  • [x] I have run the components SMOKETESTS if applicable
  • [x] My commit headlines contain a valid Task id
  • [ ] My change requires a change to the documentation
  • [ ] I have updated the documentation accordingly

talmakion avatar Jun 10 '24 08:06 talmakion

👍 No issues in PR Title / Commit Title

github-actions[bot] avatar Jun 10 '24 08:06 github-actions[bot]

Please excuse the badly formatted commit message, this isn't intended as a final PR.

talmakion avatar Jun 11 '24 11:06 talmakion

Updated to include config migration scripts & rebase

talmakion avatar Jun 16 '24 11:06 talmakion

👍 No issues in PR Title / Commit Title

github-actions[bot] avatar Jul 02 '24 14:07 github-actions[bot]

This one is the important one to fix the original ticketed issues, but it contains a few design decisions I'm not too sure about. My PR notes and linked forum post contain more information.

talmakion avatar Jul 03 '24 10:07 talmakion

Added smoketests in https://github.com/vyos/vyos-1x/pull/3800 for both IPsec & GRE match changes - they're all in the same file, so there's merge conflicts in combined testing.

talmakion avatar Jul 06 '24 15:07 talmakion

👍 No issues in unused-imports

github-actions[bot] avatar Jul 08 '24 14:07 github-actions[bot]

CI integration ❌ failed!

Details

CI logs

  • CLI Smoketests ❌ failed
  • Config tests ❌ failed
  • RAID1 tests ❌ failed

github-actions[bot] avatar Jul 08 '24 14:07 github-actions[bot]

Split smoketests back into their respective PRs.

talmakion avatar Jul 08 '24 14:07 talmakion

This PR broke smoketests

https://github.com/vyos/vyos-rolling-nightly-builds/actions/runs/10135928753/job/28023994368#step:15:1896

c-po avatar Jul 29 '24 05:07 c-po

I've pushed a fix to the branch this PR was tracking, do you want a new PR?

talmakion avatar Jul 29 '24 08:07 talmakion