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.

Sign up for GitHub

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

Jump to bottom

Pr/v1.10 backport 2022 11 30 #22454

Merged
merged 2 commits into from
Dec 2, 2022
Merged

Pr/v1.10 backport 2022 11 30 #22454

merged 2 commits into from
Dec 2, 2022

Conversation

tommyp1ckles
Copy link
Contributor

@tommyp1ckles tommyp1ckles commented Nov 30, 2022 •
edited 

$ for pr in 22361; do contrib/backporting/set-labels.py $pr done 1.10; done

tommyp1ckles and others added 2 commits November 30, 2022 13:41
@tommyp1ckles
fqdn: dnsproxy: fix forwarding of the security identity for cluster mesh
23dd5dd 
[ upstream commit 8264fd4 ]

The commit 44c1def wrongly forwarded only lower 16 bits of the original
identity. This might corrupt identities when cluster-id is not zero (as the
cluster-id is encoded in bits 16..23 of the identity) and leads to policy drops
due to unknown identity, e.g.

    xx drop (Policy denied) flow 0xd1a7add4 to endpoint 3966, file bpf_lxc.c line 2032, , identity 47657->157516: 10.2.3.223:55853 -> 10.2.3.206:53 udp

(Here the security identity 47657 doesn't exist, as it should actually be equal
to 0x10000|47657 = 113193.)

Fix this by also storing bits 16..23 of the identity in the skb mark according
to the datapath ABI, i.e., skb mark should be equal to (id << 16) | (id >> 16).

Fixes: 44c1def ("fqdn: dnsproxy: forward the original security identity")

Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@aspsk @tommyp1ckles
fqdn: dnsproxy: fix forwarding of the original security identity for TCP
efce836 
[ upstream commit cf3cc16 ]

In case of TCP this is not enough to do net.Dial + setsockopt(SO_MARK), as in
this case TCP SYN will have a wrong identity, e.g.:

    Policy verdict log: flow 0x7a95a133 local EP ID 393, remote ID 14616, proto 6, egress, action redirect, match L3-L4, 10.244.1.122:42437 -> 10.244.1.120:53 tcp SYN
    Policy verdict log: flow 0x907eaa19 local EP ID 458, remote ID host, proto 6, ingress, action allow, match L3-Only, 172.19.0.2:56276 -> 10.244.1.120:53 tcp SYN

Here the second message has wrong identity (host). We still allow the traffic,
as the origin is local host and the coredns is running on the same host, but
this will not work for a remote host if ingress policy doesn't allow
remote-node identity.) To fix this we need to pass a Control parameter to Dial,
so that setsockopt(2) is called before the connect(2). With such a change we
now see the correct identity in case of TCP:

    Policy verdict log: flow 0xeb7902a9 local EP ID 393, remote ID 14616, proto 6, egress, action redirect, match L3-L4, 10.244.1.122:36661 -> 10.244.1.120:53 tcp SYN
    Policy verdict log: flow 0x4efbc5a0 local EP ID 458, remote ID 41903, proto 6, ingress, action allow, match L3-L4, 172.19.0.2:40508 -> 10.244.1.120:53 tcp SYN

Fixes: 44c1def ("fqdn: dnsproxy: forward the original security identity")

Signed-off-by: Anton Protopopov <aspsk@isovalent.com>
Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.10 kind/backports This PR provides functionality previously merged into master. labels Nov 30, 2022
@tommyp1ckles tommyp1ckles marked this pull request as ready for review November 30, 2022 21:44
@tommyp1ckles tommyp1ckles requested a review from a team as a code owner November 30, 2022 21:44
@tommyp1ckles
Copy link
Contributor Author

tommyp1ckles commented Nov 30, 2022 •
edited by maintainer-s-little-helper bot

/test-backport-1.10

Job 'Cilium-PR-K8s-GKE' failed:

Click to show.

Test Name

K8sIstioTest Istio Bookinfo Demo Tests bookinfo inter-service connectivity

Failure Output

FAIL: Pods are not ready after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-GKE so I can create one.

@aanm
Copy link
Member

aanm commented Nov 30, 2022

The Base Image Lint / Lint image build logic (pull_request) failure is being addressed in #22458

christarazi
christarazi approved these changes Dec 2, 2022
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

CI passed except for known failures. Reviewed the backport and it looks equivalent to upstream. Ready to merge.

@christarazi christarazi added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 2, 2022
@joestringer joestringer merged commit abc9d1d into cilium:v1.10 Dec 2, 2022
@joestringer joestringer mentioned this pull request Dec 15, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
kind/backports This PR provides functionality previously merged into master. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tommyp1ckles @aanm @christarazi @joestringer @aspsk