bpf: Handle ICMPv6 NS/NA in host firewall

[pchaigno]

In IPv6 mode, when the host firewall is enabled and rules are enforced, we start dropping ICMPv6 packets that are required to route packets. In particular, we can notice the following drops in cilium monitor:

xx drop (Policy denied) flow 0x1fc0ef0 to endpoint 0, identity 1->0: fd00::c -> f00d::a0f:0:0:dfa1 DestinationUnreachable(NoRouteToDst)
xx drop (Policy denied) flow 0x0 to endpoint 0, identity 0->0: fd01::c -> fd01::b NeighborAdvertisement

The nodes need to be able to exchange ICMPv6 NS and NA messages to establish routes. We already handle the response to NS messages on ingress, but when the egress policies are enforced, we start dropping outgoing NS and NA messages.

This commit fixes that by allowing and rejecting ICMPv6 messages according to RFC4890 Section 4.4. No other verifications than the types' are performed on the messages' correctness or their source IP addresses. Such messages from the pods are already handled on their egress, so we're not at risk of spoofing from pods here.
Handling of echo request and reply messages does not conform to RFC4890 as they can be filtered by the host firewall. That is to be consistent with our handling of ICMPv4 messages.

With this commit, we also stop answering to NS and echo request messages from the BPF program on ingress to the host. This behavior had been broken by a695f53, but we will now explicitly stop replying to those messages and pass them up the stack instead.

Updates: #11799
Fixes: #10994
Fixes: #11507

[coveralls]

Coverage decreased (-0.02%) to 36.926% when pulling a4755fb on pr/pchaigno/host-fw-handle-icmpv6 into ca767ee on master.

[pchaigno]

test-me-please

[pchaigno]

K8S-1.17-Kernel-4.19 hit #10231. All other pipelines are passing.

[joestringer]

@jedsalazar @joestringer Since you looked into these sort of things recently, are there any concerns with allowing ICMPv6 NS/NA messages through the host firewall?

The recent issues were specifically for router solicitations/advertisements, for what it's worth. But similar to the logic you point out in your commit messages, this program is in the wrong place to have any security impact from pods generating such messages. (I note also that the reason this was bad for pods was based on a threat model of someone gaining arbitrary execution access to a pod; that's a different threat model than someone having access to your physical network)

I see the Host FW as an L3 security feature here, where routed traffic destined to the node is subject to policy. ICMPv6 NS/NA, RS/RA are more about L2 adjacency. I don't think it's our job to handle L2 network security here; users could legitimately configure their nodes directly adjacent to a router that sends IPv6 RAs and we will trust the underlying network. I think it's up to the user to configure their Linux boxes with the right settings for handling these messages. I note that in the PR as-is, this network configuration would actually break because we'd be dropping the router advertisements from the network. Fortunately, Cilium will log the drops pretty clearly in cilium monitor so if someone hit an issue with this, they should be able to find out reasonably quickly.

If we want to involve ourselves in L2 security features like this, we probably need to think it through and actually design it, complete with some clear way for users to configure it with what they anticipate the underlying network to legitimately allow; if we step in here and start messing with people's ICMPv6 peer discovery then I think we'll more likely break IPv6 in weird&wonderful ways while not actually improving security.

[joestringer]

Couple of discussion points, I think we should be really careful on how Cilium steps into L2 here.

[pchaigno]

test-me-please

[pchaigno]

Re-running 4.9 test now that the host firewall is enabled there by default (cf. #11969):
retest-4.9

[anfernee]

Just a friendly ask: Is there anything blocking this change to be merged?

[aanm]

Just a friendly ask: Is there anything blocking this change to be merged?

@anfernee only the review from @joestringer 😄

[pchaigno]

All tests are passing. The pending tests are just because some of the GitHub Actions were renamed, but they were already executed under their old names before.

[joestringer]

Looks good, minor nits on consistent use of CTX_ACT_OK / IS_ERR(...).

[pchaigno]

test-me-please