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

[talmakion]

Change Summary

Allow firewall rules to match flags and fields in GRE protocol headers, primarily, the "key" field.

Adding this will allow firewall rules to distinguish individual GRE sessions and apply appropriate policy, such as blocking sending or forwarding certain tunnels in the clear if IPsec is down.

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):

Related Task(s)

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

Related PR(s)

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

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.

• Only matching flags and fields used by modern RFC2890 "extended GRE" - this is backwards-compatible, but does not match all possible flags.
• There are no nftables helpers for the GRE key field, which is critical to match individual tunnel sessions (more detail in the forum post)
  • nft expression syntax is not flexible enough for multiple field matches in a single rule and the key offset changes depending on flags.
  • Thus, clumsy compromise in requiring an explicit match on the "checksum" flag if a key is present, so we know where key will be. In most cases, nobody uses the checksum, but assuming it to be off or automatically adding a "not checksum" match unless told otherwise would be confusing
  • The automatic "flags key" check when specifying a key doesn't have similar validation, I added it first and it makes sense. I would still like to find a workaround to the "checksum" offset problem.
  • If we could add 2 rules from 1 config definition, we could match both cases with appropriate offsets, but this would break existing FW generation logic, logging, etc.
• Added a "gre-protocol" validator for the fields we can pass to nft's gre matches.
  • The protocol names are out of synch with other parts of the firewall def, but for eg, I can't call a completion+valueHelp "ipv6" without VyOS deciding to show an IPv6 pattern instead of my help text.
  • I've allowed arbitrary radix numbers for the ethertype, for eg, it's common to use 0x8100 for .1q instead of 33024. nft should accept these as well.

How to test

• Configured VyOS testing pair in a "triangle" (LEFT -> FAKEWWW <- RIGHT)
• 2 additional VMs behind LEFT and RIGHT for forward chain testing
• Applied numerous combinations of gre/ip6gre matches in logging/nflogging rules across all filter chains
• Created GRE/GRETAP point to point and thru-tunnels between LEFT and RIGHT and the VMs behind them
• Checking dmesg output and tcpdump on FAKEWWW for expected operation in hitting all the right rules

Starting firewall config (note ethtype 0x6558 is gretap):

firewall {
    ipv4 {
        forward {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST-LOG-ETHTYPE
                }
            }
        }
        input {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST-LOG-ETHTYPE
                }
            }
        }
        name TEST-LOG-CSUMMED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        checksum
                    }
                }
                log
                protocol gre
            }
        }
        name TEST-LOG-ETHTYPE {
            default-action return
            rule 10 {
                action continue
                gre {
                    inner-proto ip
                }
                log
                protocol gre
            }
            rule 20 {
                action continue
                gre {
                    inner-proto 0x6558
                }
                log
                protocol gre
            }
        }
        name TEST-LOG-KEYED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                    key 1234
                }
                log
                protocol gre
            }
            rule 20 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                    key 4321
                }
                log
                protocol gre
            }
            rule 30 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                }
                log
                protocol gre
            }
        }
        name TEST-LOG-UNKEYED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        not {
                            key
                        }
                    }
                }
                log
                protocol gre
            }
        }
        output {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST-LOG-ETHTYPE
                }
            }
        }
        prerouting {
            raw {
                rule 100 {
                    action jump
                    jump-target TEST-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST-LOG-ETHTYPE
                }
            }
        }
    }
    ipv6 {
        forward {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST6-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST6-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST6-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST6-LOG-ETHTYPE
                }
            }
        }
        input {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST6-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST6-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST6-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST6-LOG-ETHTYPE
                }
            }
        }
        name TEST6-LOG-CSUMMED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        checksum
                    }
                }
                log
                protocol gre
            }
        }
        name TEST6-LOG-ETHTYPE {
            default-action return
            rule 10 {
                action continue
                gre {
                    inner-proto ip
                }
                log
                protocol gre
            }
            rule 20 {
                action continue
                gre {
                    inner-proto 0x6558
                }
                log
                protocol gre
            }
        }
        name TEST6-LOG-KEYED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                    key 1234
                }
                log
                protocol gre
            }
            rule 20 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                    key 4321
                }
                log
                protocol gre
            }
            rule 30 {
                action continue
                gre {
                    flags {
                        key
                        not {
                            checksum
                        }
                    }
                }
                log
                protocol gre
            }
        }
        name TEST6-LOG-UNKEYED {
            default-action return
            rule 10 {
                action continue
                gre {
                    flags {
                        not {
                            key
                        }
                    }
                }
                log
                protocol gre
            }
        }
        output {
            filter {
                rule 100 {
                    action jump
                    jump-target TEST6-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST6-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST6-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST6-LOG-ETHTYPE
                }
            }
        }
        prerouting {
            raw {
                rule 100 {
                    action jump
                    jump-target TEST6-LOG-UNKEYED
                }
                rule 110 {
                    action jump
                    jump-target TEST6-LOG-KEYED
                }
                rule 120 {
                    action jump
                    jump-target TEST6-LOG-CSUMMED
                }
                rule 130 {
                    action jump
                    jump-target TEST6-LOG-ETHTYPE
                }
            }
        }
    }
}

This was a starting point, specific rules were fed directly into top level chains for testing outside of custom named chains and more yes/no logging firewall pairs were added to cover as many combinations of attributes & flags as possible.

The validators were also given exercise to make sure invalid flag/attribute combos weren't possible.

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

[talmakion]

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

[talmakion]

Fixed mistake in validator & rebased on current

[github-actions]

❌
Commit title 'Merge branch 'current' into feature/T4694/gre-match-fields' does not match the required format!. Valid title example: T99999: make IPsec secure

[github-actions]

👍 VyOS CLI smoketests finished successfully!

[talmakion]

Testing is completed, clumsy design but operational.

[nicolas-fort]

Can you extend firewall smoketest, and include a test case for this setup?

[talmakion]

@nicolas-fort Added smoketests in a separate PR for both IPsec & GRE match changes - they're all in the same file, so there's merge conflicts in combined testing.

Let me know if you'd like them split up into the separate PR branches instead.

[github-actions]

👍
No issues in unused-imports

[talmakion]

Split smoketests back into their respective PRs.

[dmbaturin]

I think the UI and the implementation can be improved.

[dmbaturin]

I think this description should be "802.1ad QinQ stacked VLAN" or similar. Saying "bridged" without explicitly mentioning "provider bridge" is just misleading, and even "provider bridge" isn't really a common term. "VLAN stacking" is intuitive and quite common.

[talmakion]

I'm updating it to track the same terminology as include/vlan-protocol.xml.i (slightly less verbose): "Provider Bridging (IEEE 802.1ad, Q-in-Q)".

If you think it's useful to mention VLAN stacking as well when stuffing QinQ into GRE, it's no big effort for me to tweak again.

Since it's being tweaked, I'll add an alias for gretap 0x6558 as well, as it's an explicitly supported GRE interface type in VyOS.

[dmbaturin]

This script is unnecessary, since the use case is already covered by the <regex> validation mechanism, which is a part of the validate-value executable and doesn't require spawning a subprocess (and spawning a Python interpreter is a notoriously slow operation).

[talmakion]

Main reason I did this was for the multi-radix validation (0xNNNN syntax is pretty common for protocols and ethertypes).

The OCaml numeric validator doesn't like radix prefixes, I'll pack this all into a single regex for now - is that something that might be useful for the validator?

[talmakion]

Also, Is there a neat or "proper" way to offer hex integers in valueHelp, or is it something that is best not done at all?

Right now I'm using:

        <valueHelp>
          <format>u32:0x0-0xFFFF</format>
          <description>Ethernet protocol number (hex)</description>
        </valueHelp>

This formats correctly in the completion under vbash. I picked through the vyatta-cfg node parser + completion scripts and couldn't immediately spot anywhere that definitely expects a base-10 integer, but I'm not familiar with the API and other internals that may treat those tags differently.

[dmbaturin]

The OCaml numeric validator doesn't like radix prefixes

The only reason for that is that I never thought it would be useful anywhere. It would be quite simple to add, although the UI for it is an interesting question. I made a task about it (https://vyos.dev/T6622), if you have any ideas, please leave a comment there.

[c-po]

This formats correctly in the completion under vbash

This is defined in /etc/bash_completion.d/vyatta-cfg

  case "$vtype" in
    _*)
      echo -n "${vtype#_}"
      ;;
    txt)
      echo -n '<text>'
      ;;
    u32)
      echo -n '<0-4294967295>'
      ;;
    u32:*)
      echo -n "<${vtype##u32:}>"
      ;;
   ...

Using

    <valueHelp>
      <format>u32:0x0-0xffff</format>
      <description>Numeric IP port</description>
    </valueHelp>

will yield <0x0-0xffff> Numeric IP port

and

    <valueHelp>
      <format>_0x0-0xffff</format>
      <description>Numeric IP port</description>
    </valueHelp>

will yield 0x0-0xffff Numeric IP port

[talmakion]

Updated with requested changes, smoketests & generated rules look OK and re-ran through a short underlying test set with real traffic.

I cannot figure out a way to do a CLI-level range validation on integers with a hex prefix (0xNNNN, for ethertype) without a custom validator, this has been moved into conf_mode. I can drop this convenience altogether if this doesn't match preferred style, but it's common to use these for ethertypes and protocol numbers.

I've added 'gretap' as an ethertype alias in GRE matches, there doesn't seem to be a nice standard name for this (Cisco likes 'eogre', there's also 'gre-teb', 'l2gre' or 'teb'). I've stuck with the tunnel type name already used by VyOS.

Code comments have been tweaked around a bit as well for clarity.

[talmakion]

I'm not entirely happy with my help text that follows the gre flags <FLAG> [unset] schema either, I may tweak this again if I think of a better way to explain that node structure.

Potentially, can also switch to gre flags <FLAG> set|unset, but my future RSI likes keeping things short.

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[github-actions]

Conflicts have been resolved. A maintainer will review the pull request shortly.

[github-actions]

CI integration ❌ failed!

Details

CI logs

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

[c-po]

I'm not entirely happy with my help text that follows the gre flags <FLAG> [unset] schema either, I may tweak this again if I think of a better way to explain that node structure.

Potentially, can also switch to gre flags <FLAG> set|unset, but my future RSI likes keeping things short.

I understand it this way: gre flags key checks if the optional key is present and gre flags key unset does not check if the optional key is present? This feels weird.

It should simply be: gre flags has-key check if optional key is present - if CLI node is unset - optional key checking is ignored.

[c-po]

Can you please rebase this PR to the latest current?

[talmakion]

I understand it this way: gre flags key checks if the optional key is present and gre flags key unset does not check if the optional key is present? This feels weird.

It should simply be: gre flags has-key check if optional key is present - if CLI node is unset - optional key checking is ignored.

If I understand you - not quite, those are explicit checks for whether a flag bit is set or not set in the GRE header structure. That means there are 3 match states: "Don't care" (no node at all), "Is Set" or gre flags key, "Not Set" or gre flags key unset.

There are 2 flags we need to care about if we're checking for an optional key, because the packet structure changes depending on how they are set and there's no builtin lookups, so I'm doing raw transport header bitfields lookups to match the key value.

The other flags are exposed more for completeness than anything else - somebody may find them useful.

I don't really like the way this works, the protocol guts shouldn't need to be this exposed to the user. The alternatives would be using something as capable as BPF, since nftables can't handle it in a single rule (iptables with BPF expressions could do these multiple flagged-bitwise matches in a single rule), or re-engineering the nft config generation (and anything depending on it) to allow multiple rules in nft output from a single firewall config rule.

The alternative I went with was simplicity of implementation and guiding the user if they're not sure - where it requires a setting for the "checksum" flag it gives a recommendation, and automatically assumes the "key" flag to be set if we want to match a key, disallowing an "unset" match.

[dmbaturin]

If I understand you - not quite, those are explicit checks for whether a flag bit is set or not set in the GRE header structure.

I do believe we should have a way to match only packets where the GRE key is not set, and the CLI for it will, unfortunately, be somewhat awkward because the issue is awkward. I think the current syntax is acceptable.

[c-po]

If I understand you - not quite, those are explicit checks for whether a flag bit is set or not set in the GRE header structure. That means there are 3 match states: "Don't care" (no node at all), "Is Set" or gre flags key, "Not Set" or gre flags key unset.

Thanks for the clarification.

[talmakion]

Applied requested changes & rebased