Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr/gray/main/fix leak detection race #32569

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jschwinger233
Copy link
Member

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Please add yourself to the Users doc
  • Thanks for contributing!

Fixes: #issue-number

<!-- Enter the release note text here if needed or remove this section! -->

jschwinger233 and others added 8 commits May 16, 2024 14:40
Signed-off-by: gray <gray.liang@isovalent.com>
Otherwise the further patch for L7 proxy + IPsec fails pod-to-world
testcases.

To fix #31984, we are going to
change the routing for traffic from egress proxy: when tunnel is
disabled, the traffic  will be routed to cilium_host instead of eth0.
This leads to a consequence of changing packets' source IPs: from eth0's
IP to cilium_host's IP. The implication requires additional masquerading
for pod to world traffic, because these packets now have cilium_host's
IP as source, rather than eth0.

This patch ensures iptables masquerade rules are installed for pod to
world traffic, even when KPR is enabled.

Signed-off-by: gray <gray.liang@isovalent.com>
Signed-off-by: gray <gray.liang@isovalent.com>
…ckets

To ensure IPsec encryption for proxy forward packets, we added routing
rule to push them to cilium_host. This change caused side effects for
to-world traffic. This patch fixes the consequences of side effects.

Proxy will perform "SNAT" for to-world packets, but the new source address
is decided by routing rules. Previously, to-world packets are routed to
eth0, so proxy uses eth0's address for SNAT. Now with new routing rule
to push them to cilium_host instead of eth0, proxy uses cilium_host's
address for SNAT as the side effect.

This change makes to-world packets rely on "external" SNAT, which wasn't
required because proxy's SNAT worked perfectly. We need "external" SNAT
to change source address of to-world packets from cilium_host's IP to
eth0's IP. As IPsec doesn't work with KPR, the "external" SNAT mechanism
is iptables.

However, due to kernel's implementation details, an skb won't be
processed by nat POSTROUTING for twice. When the packet is routed to
cilium_host, it's the first time; when forwarded from cilium_net to
eth0, it's supposed to be the second time.

This is because, After the first POSTROUTING traversal, skb's ct (struct
nf_conn*)(skb->_nfct & ~7) has a status IPS_SRC_NAT_DONE to skip the
second traversal at all.

To avoid setting the IPS_SRC_NAT_DONE flag, this patch adds an iptables
rule `-j CT --notrack` for skip the first round iptables ct.

Signed-off-by: gray <gray.liang@isovalent.com>
Extend the conformance-ipsec-e2e GHA workflow to additionally check that
we don't leak any unencrypted packets during the connectivity test.
This aims to complement the validation already performed as part of
the connectivity tests by the Cilium CLI.

Specifically, we leverage bpftrace to analyze the packets forwarded by
the bridge device (used by kind), and report those that are not encrypted.
We flag packets with both the source and the destination belonging to
the IPv4/6 PodCIDR, and we consider the inner headers if packets are
encapsulated. In this case, we additionally skip packets originating
or targeting CiliumInternalIP addresses (as these are used for node-to-pod
traffic when running in tunnel mode, which is not encrypted by design).

Extra checks are finally added to always include packets originating
from the L7 and DNS proxies, as their source IP is not that of a pod.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
So that we can install the version we want.

Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Signed-off-by: Julian Wiedmann <jwi@isovalent.com>
Signed-off-by: gray <gray.liang@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 16, 2024
@jschwinger233
Copy link
Member Author

jschwinger233 commented May 16, 2024

/ci-ipsec-e2e

Result: https://github.com/cilium/cilium/actions/runs/9109346753/job/25042030204

All jobs failed due to

 Sanity check failed: detected no IPv4 connections from the L7 proxy. Is the filter correct?
Sanity check failed: detected no IPv6 connections from the L7 proxy. Is the filter correct?
Sanity check failed: detected no messages sent by the DNS proxy. Is the filter correct?

But no leak was detected! Try again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-merge/needs-release-note-label The author needs to describe the release impact of these changes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants