bpf: fix missing ipv6 ct entry for SNATed pod traffic

[oblazek]

When combining HostFW with kube-proxy based masquerading
host traffic will have HOST_ID for both - packet mark based
src_id and ip cache based src_id.
While SNATed pod traffic will only have HOST_ID from the
ipcache (as it doesn't pass through the iptables rules that
set the mark).
Therefore for SNATed traffic we get HOST_ID only from
ipcache and not from packet mark it means we are handling
SNATed traffic from pod that needs to have a conntrack entry
so that return traffic doesn't go through host firewall and
is skipped.

This commit fixes the issue and adds a missing ct entry for
SNATed traffic.

cc @julianwiedmann

Fixes: #26886

related discussion: https://cilium.slack.com/archives/CDKG8NNHK/p1698155364279479

bpf: Fix drop of IPv6 reply traffic when 1) pod-originating connection is SNATed by iptables, and 2) Host Firewall is enabled.

[oblazek]

/test

[julianwiedmann]

Hi @oblazek , ty for looking into this! Will have a closer look ASAP.

But just to set some context for review, as the description doesn't capture my current understanding.
With ENABLE_HOST_FIREWALL set, we need to handle two cases in the egress path:

• actual host traffic. Here we want to apply egress policy.
• SNATed pod traffic (when not using BPF-masquerading). Here we don't apply egress policy. But we still create a CT entry, so that replies can naturally bypass ingress policy.

We differentiate between the two cases by comparing the mark-derived identity with the ipcache-derived identity. Host traffic will have HOST_ID for both, while SNATed pod traffic will only have HOST_ID from the ipcache (as it doesn't pass through the iptables rule that sets the mark).

Throwing --install-iptables-rules=false into this picture means that legitimate host traffic will not be marked either, and thus is mis-identified as "SNATed pod traffic". And so while we create a CT entry for it (and the reply traffic flow works), we don't actually apply egress policy to it when we should.

Do you agree with that assessment, or is there still something missing from the picture?

[oblazek]

Throwing --install-iptables-rules=false into this picture means that legitimate host traffic will not be marked either, and thus is mis-identified as "SNATed pod traffic". And so while we create a CT entry for it (and the reply traffic flow works), we don't actually apply egress policy to it when we should.

Do you agree with that assessment, or is there still something missing from the picture?

You are totally right, if iptables are not present, there is no mark and return __ipv6_host_policy_egress(ctx, src_id == HOST_ID,... can't be relied on. So at least there should be a logical OR:
src_id == HOST_ID || ipcache_srcid == HOST_ID so that egress policy is correctly enforced.

I believe the same should be done for ipv4

[oblazek]

/test

[julianwiedmann]

Throwing --install-iptables-rules=false into this picture means that legitimate host traffic will not be marked either, and thus is mis-identified as "SNATed pod traffic". And so while we create a CT entry for it (and the reply traffic flow works), we don't actually apply egress policy to it when we should.
Do you agree with that assessment, or is there still something missing from the picture?

You are totally right, if iptables are not present, there is no mark and return __ipv6_host_policy_egress(ctx, src_id == HOST_ID,... can't be relied on. So at least there should be a logical OR: src_id == HOST_ID || ipcache_srcid == HOST_ID so that egress policy is correctly enforced.

I believe the same should be done for ipv4

agree, but this needs to be a conditional change (only when --install-iptables-rules=false is set).

[oblazek]

updated, now the PR should only contain ipv6 whitelisting related changes.. will do the rest in the followup PR

[oblazek]

/test

[julianwiedmann]

Ty @oblazek ! There's one part where we need to check if the mark-based identity and ipcache-based identity match up (see below). But it's missing the ipcache-based identity ... could you have a look into passing that through, along what IPv4 does?

[oblazek]

Hmm now the CB_IPCACHE_SRC_LABEL part is com...

yeah I must have missed that when working on different branches :(

[oblazek]

/test

[oblazek]

the 2 pipeline fails are not related to this fix

[YutaroHayakawa]

I made an initial look. The logic to set CB_IPCACHE_SRC_LABEL is still missing, but otherwise, the logic looks correct to me. I found some small naming inconsistency and one potential bug.

[YutaroHayakawa]

Now LGTM 👍

[julianwiedmann]

/test

[julianwiedmann]

ty!