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

talmakion · 2024-06-10T08:25:14Z

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)
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
Other (please describe): Changes to existing firewall ipsec match syntax

Related Task(s)

Related PR(s)

firewall: T4694: Adding GRE flags & fields matches to firewall rules #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:

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

github-actions · 2024-06-10T08:25:36Z

👍
No issues in PR Title / Commit Title

talmakion · 2024-06-11T11:33:26Z

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

talmakion · 2024-06-16T11:54:57Z

Updated to include config migration scripts & rebase

github-actions · 2024-07-02T14:46:23Z

👍
No issues in PR Title / Commit Title

talmakion · 2024-07-03T10:00:31Z

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 · 2024-07-06T15:17:28Z

Added smoketests in #3800 for both IPsec & GRE match changes - they're all in the same file, so there's merge conflicts in combined testing.

…figs * 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 * 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 improper IPsec matches. * Added "test_ipsec_metadata_match" and "test_cyclic_jump_validation" smoketests

github-actions · 2024-07-08T14:40:42Z

👍
No issues in unused-imports

github-actions · 2024-07-08T14:43:10Z

CI integration ❌ failed!

Details

CI logs

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

talmakion · 2024-07-08T14:45:14Z

Split smoketests back into their respective PRs.

c-po · 2024-07-29T05:47:15Z

This PR broke smoketests

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

talmakion · 2024-07-29T08:44:43Z

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

This patch on vyos#3616 will only attempt to fix ipsec matches in rules if the firewall config tree passed to migrate_chain() has rules attached.

github-actions bot assigned talmakion Jun 10, 2024

github-actions bot added the current label Jun 10, 2024

talmakion changed the title ~~Adding rt ipsec exists/missing match to firewall configs~~ firewall: T4694: Adding rt ipsec exists/missing match to firewall configs Jun 10, 2024

talmakion mentioned this pull request Jun 11, 2024

firewall: T4694: Adding GRE flags & fields matches to firewall rules #3637

Merged

12 tasks

talmakion force-pushed the feature/T4694/outbound-ipsec branch 2 times, most recently from cf2cd64 to 05a35e6 Compare June 16, 2024 11:51

talmakion force-pushed the feature/T4694/outbound-ipsec branch from 05a35e6 to a779004 Compare July 2, 2024 14:46

talmakion force-pushed the feature/T4694/outbound-ipsec branch from a779004 to 28da5d9 Compare July 3, 2024 09:57

talmakion marked this pull request as ready for review July 3, 2024 10:00

talmakion requested a review from a team as a code owner July 3, 2024 10:00

talmakion requested review from dmbaturin, sarthurdev, zdc, jestabro, sever-sever and nicolas-fort July 3, 2024 10:00

talmakion mentioned this pull request Jul 6, 2024

firewall: T4694: Adding smoketests for GRE and IPsec match PRs #3800

Closed

12 tasks

talmakion force-pushed the feature/T4694/outbound-ipsec branch from 28da5d9 to 4a37ee6 Compare July 8, 2024 14:40

dmbaturin approved these changes Jul 25, 2024

View reviewed changes

sever-sever approved these changes Jul 28, 2024

View reviewed changes

sever-sever merged commit e2bf881 into vyos:current Jul 28, 2024
11 of 12 checks passed

talmakion mentioned this pull request Jul 29, 2024

firewall: T4694: incomplete node checks in migration script #3895

Merged

12 tasks

talmakion deleted the feature/T4694/outbound-ipsec branch July 30, 2024 03:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

talmakion commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

talmakion commented Jun 11, 2024

talmakion commented Jun 16, 2024

github-actions bot commented Jul 2, 2024 •

edited

Loading

talmakion commented Jul 3, 2024

talmakion commented Jul 6, 2024 •

edited

Loading

github-actions bot commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

talmakion commented Jul 8, 2024

c-po commented Jul 29, 2024

talmakion commented Jul 29, 2024

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

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

Conversation

talmakion commented Jun 10, 2024 • edited Loading

Change Summary

Types of changes

Related Task(s)

Related PR(s)

Component(s) name

Proposed changes

How to test

Smoketest result

Checklist:

github-actions bot commented Jun 10, 2024 • edited Loading

talmakion commented Jun 11, 2024

talmakion commented Jun 16, 2024

github-actions bot commented Jul 2, 2024 • edited Loading

talmakion commented Jul 3, 2024

talmakion commented Jul 6, 2024 • edited Loading

github-actions bot commented Jul 8, 2024

github-actions bot commented Jul 8, 2024

Details

talmakion commented Jul 8, 2024

c-po commented Jul 29, 2024

talmakion commented Jul 29, 2024

talmakion commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jun 10, 2024 •

edited

Loading

github-actions bot commented Jul 2, 2024 •

edited

Loading

talmakion commented Jul 6, 2024 •

edited

Loading