Skip to content

Add support for VRRP and IGMP in host fw#39872

Merged
borkmann merged 6 commits intocilium:mainfrom
aditighag:pr/aditighag/host-fw-vrrp
Jul 2, 2025
Merged

Add support for VRRP and IGMP in host fw#39872
borkmann merged 6 commits intocilium:mainfrom
aditighag:pr/aditighag/host-fw-vrrp

Conversation

@aditighag
Copy link
Copy Markdown
Member

@aditighag aditighag commented Jun 3, 2025

This PR adds support for VRRP and IGMP protocols in host firewall.

Commit highlights

  • Policy api/engine: The existing toPorts field is extended to support these protocols. As the protocols don't have transport ports, the existing validation check around empty ports is relaxed. Users can explicitly whitelist/deny these protocols using the toPorts rules, and these rules can be combined with the existing L3 rules.
  • BPF datapath: Conntrack will now pass packets from these protocols that would previously be dropped with DROP_CT_UNKNOWN_PROTO error. This is gated behind a config flag so that it won't break existing allow-all policies. The decision to allow/deny such traffic is delegated to user-defined policies. Also, the existing behavior of allowing reply traffic based on conntrack state is preserved for VRRP and IGMP protocols as well.

Additional notes

  • Policy api: Earlier version of this PR introduced a new field IPProtocols that would allow users to specify such IP protocols without transport ports using their IANA assigned numbers. This field was restricted to CCNP only using CEL rules. However, based on discussions with @jrajahalme and @squeed, we decided to reuse the existing toPorts field instead as the protocols don't need header fields matching similar to ICMP, and so the current use cases can be satisfied without introducing the new field. Note that the policy engine already supports policy matching with wildcard port field.
  • Policy enforcement for hosts and pods: Support for VRRP and IGMP protocols has been extended to both host and pod traffic, leveraging the shared policy enforcement and conntrack logic common to bpf_host and bpf_lxc programs. This PR, however, focuses on host firewall in terms of testing. Filed issue for bpf_lxc tests - Add BPF tests for extended IP protocols support for pods #40229.

Results from manual testing:

$ cat lock-down-hw.yaml
apiVersion: "cilium.io/v2"
kind: CiliumClusterwideNetworkPolicy
metadata:
  name: "lock-down-ingress-worker-node"
spec:
  description: "Allow selected traffic on egress of worker nodes"
  nodeSelector:
    matchLabels:
      node-access: policy
  egress:
  - toPorts:
    - ports:
      - port: "22"
        protocol: TCP
      - port: "443"
        protocol: TCP
      - port: "6443"
        protocol: TCP
      - port: "2379"
        protocol: TCP
      - port: "4240"
        protocol: TCP
      - port: "8472"
        protocol: UDP
      - port: "0"
        protocol: VRRP
  • Apply a host fw policy that allows L4 traffic on some ports but not VRRP. Traffic is denied with no match.
    Policy verdict log: flow 0x0 local EP ID 559, remote ID world, proto 112, egress, action deny, auth: disabled, match none, 172.19.0.4 -> 224.0.0.18
  • Extend the toPorts rule to explicitly allow VRRP. Traffic is allowed.
  • Explicitly deny VRRP traffic with toPorts rule. Traffic is denied with proto match.
    Policy verdict log: flow 0x0 local EP ID 559, remote ID world, proto 112, egress, action deny, auth: disabled, match Proto-Only, 172.19.0.4 -> 224.0.0.18
  • Extend deny rules with to/forCIDR rules for VRRP. Traffic is denied via L3 match.
    Policy verdict log: flow 0x0 local EP ID 559, remote ID 16777217, proto 112, egress, action deny, auth: disabled, match L3-Only, 172.19.0.4 -> 224.0.0.18

Follow-up: E2E CI test and Hubble proto extensions

Fixes: #31502
Fixes: #18347

Add support for VRRP and IGMP protocols in host firewall

@maintainer-s-little-helper

This comment was marked as resolved.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 3, 2025
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from 37cd550 to 6b88379 Compare June 3, 2025 17:27
@maintainer-s-little-helper

This comment was marked as resolved.

@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from 6b88379 to 4c67cf7 Compare June 9, 2025 17:27
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 9, 2025
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch 2 times, most recently from 7fed78d to 6c0b3ba Compare June 10, 2025 16:20
@aditighag aditighag changed the title Add support for VRRP and IGMP to host fw Add support for VRRP and IGMP in host fw Jun 10, 2025
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch 4 times, most recently from 768049e to e85b4fa Compare June 10, 2025 20:01
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from 2612e11 to 8e5e17a Compare June 12, 2025 21:58
@maintainer-s-little-helper

This comment was marked as resolved.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 12, 2025
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from 8e5e17a to c69a751 Compare June 12, 2025 23:07
@maintainer-s-little-helper

This comment was marked as resolved.

@aditighag
Copy link
Copy Markdown
Member Author

@squeed I've pushed a new version that extends the existing toPorts field in the policy spec to allow/deny these protocols. I've added more context to the PR description. PTAL!

@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from c69a751 to 39cd758 Compare June 13, 2025 00:51
@maintainer-s-little-helper

This comment was marked as resolved.

@aditighag aditighag added the release-blocker/1.18 This issue will prevent the release of the next version of Cilium. label Jun 13, 2025
@aditighag aditighag force-pushed the pr/aditighag/host-fw-vrrp branch from 39cd758 to 3d98df1 Compare June 13, 2025 03:20
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jun 13, 2025
@aditighag
Copy link
Copy Markdown
Member Author

@ti-mo @squeed @gentoo-root PTAL

Changes pushed in the latest revision:

  • Renamed the config flag to enable support for extended IP protocols.
  • Set the field in the loader objects that were missed previously.
  • Renamed the new bpf test file.

@aditighag

This comment was marked as outdated.

@aditighag

This comment was marked as outdated.

2 similar comments
@aditighag

This comment was marked as outdated.

@aditighag
Copy link
Copy Markdown
Member Author

/test

Copy link
Copy Markdown
Member

@dylandreimerink dylandreimerink left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good for my codeowners

Copy link
Copy Markdown
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the time to look at the changes in detail and only covered doc changes, sorry - Your changes look good, but Casey has an open PR that cleans up the docs and you may want to adjust accordingly, see below.

Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the neat pull request! Very easy to review 🙏

I have a couple questions/concerns below.

aditighag added 6 commits July 1, 2025 13:43
Pass traffic with IP protocols like
VRRP and IGMP in conntrack datapath. The traffic 
will be allowed/denied per user-defined policies.

It's guarded behind a configuration flag to
avoid allowing this traffic by default, especially
for users who may have previously applied allow-all
policies.

Signed-off-by: Aditi Ghag <[email protected]>
This is a preparatory commit to
add helper functions for adding
L4 ingress policy entries to
match on protocol bits.

Signed-off-by: Aditi Ghag <[email protected]>
This commit extends the network policy spec to allow/deny
protocols such as VRRP and IGMP that don't have transport
ports. Note that if protocol is set to ANY and port is omitted,
the policy may permit any *L4* protocols (TCP, UDP, and SCTP).

Signed-off-by: Aditi Ghag <[email protected]>
Extend policy support to include protocols such
as VRRP and IGMP, which do not use transport-layer ports.

For these protocols, the port field in toPorts rules
will be either omitted or explicitly set to 0.
Policy matching logic treats the port field as a
wildcard in such cases, and matches on the
protocol specified in the policy key.

To enable this, relax port validation to allow toPorts
rules without a port field when applicable.

Signed-off-by: Aditi Ghag <[email protected]>
@aditighag
Copy link
Copy Markdown
Member Author

/test

Copy link
Copy Markdown
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pointers to previous discussions! LGTM!

I think my latest comments can be addressed as follow ups since they're more about documentation.

@aditighag
Copy link
Copy Markdown
Member Author

api review is obsoleted by - a31a4ed.

@joestringer
Copy link
Copy Markdown
Member

@aditighag We already cut the release candidate for v1.18, so typically we would not backport new features into the release at this time, especially something that changes API behavior. This has risk to break behavior and users are not likely to pick up on that at the last minute prior to release. Has there been some discussion about the backport proposal?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/host-firewall Impacts the host firewall or the host endpoint. release-note/major This PR introduces major new functionality to Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Projects

No open projects
Status: Released

Development

Successfully merging this pull request may close these issues.

VRRP traffic dropped by Cilium - CT: Unknown L4 protocol Inconsistent behavior related to VRRP packets drops