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

Add option to disable ExternalIP mitigation (CVE-2020-8554). #31513

Merged
merged 1 commit into from
Apr 17, 2024
Merged

Add option to disable ExternalIP mitigation (CVE-2020-8554). #31513

merged 1 commit into from
Apr 17, 2024

Conversation

kvaster
Copy link
Contributor

@kvaster kvaster commented Mar 19, 2024

This mitigation has it's own drawbacks for some setups. It prevents pods communication in same cluster via ExternalIP when DSR is enabled.

Fixes #28187

@kvaster kvaster requested review from a team as code owners March 19, 2024 21:53
@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 Mar 19, 2024
@kvaster kvaster requested review from joamaki and gandro March 19, 2024 21:53
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Mar 19, 2024
@kvaster kvaster force-pushed the disable-external-ip-mitigation branch 4 times, most recently from dad3ddf to 586279a Compare March 20, 2024 09:14
gandro
gandro approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Helm/Agent looks good to me!

@gandro gandro added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Mar 20, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Mar 20, 2024
@gandro gandro added the sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Mar 20, 2024
borkmann
borkmann approved these changes Mar 20, 2024
View reviewed changes
Copy link
Member

@borkmann borkmann left a comment

Choose a reason for hiding this comment

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

lgtm

derailed
derailed approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@derailed derailed left a comment

Choose a reason for hiding this comment

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

@kvaster LGTM - Thank you for this update!

julianwiedmann
julianwiedmann requested changes Mar 25, 2024
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Thank you for the fix! Just two smaller aspects worth addressing.

bpf/bpf_sock.c Outdated Show resolved Hide resolved
daemon/cmd/daemon_main.go Show resolved Hide resolved
@julianwiedmann julianwiedmann added area/loadbalancing Impacts load-balancing and Kubernetes service implementations feature/dsr Relates to Cilium's Direct-Server-Return feature for KPR. labels Mar 25, 2024
@kvaster kvaster force-pushed the disable-external-ip-mitigation branch from 586279a to 0f2ca7a Compare March 29, 2024 20:09
@kvaster kvaster requested a review from a team as a code owner March 29, 2024 20:09
@kvaster kvaster requested a review from a team as a code owner March 29, 2024 20:09
julianwiedmann
julianwiedmann reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

lgtm so far, ty! One suggestion below to get the BPF stylechecker ✔️ .

bpf/bpf_sock.c Outdated Show resolved Hide resolved
lambdanis
lambdanis approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@lambdanis lambdanis left a comment

Choose a reason for hiding this comment

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

Left one nit comment for docs, but looks good otherwise.

Documentation/network/kubernetes/kubeproxy-free.rst Outdated Show resolved Hide resolved
@kvaster kvaster force-pushed the disable-external-ip-mitigation branch 2 times, most recently from 69236fa to ac198aa Compare April 8, 2024 18:23
@julianwiedmann
Copy link
Member

/test

@kvaster
Copy link
Contributor Author

kvaster commented Apr 10, 2024

Telling the truth, I don't know why helm charts test is failing... I've rechecked it twice...

@julianwiedmann
Copy link
Member

Telling the truth, I don't know why helm charts test is failing... I've rechecked it twice...

https://github.com/cilium/cilium/actions/runs/8611642349/job/23599228700?pr=31513#step:3:111 is what's needed:

please run 'make -C install/kubernetes' and submit your changes

(it updates install/kubernetes/cilium/values.schema.json)

@kvaster kvaster force-pushed the disable-external-ip-mitigation branch from 8be5d81 to afe1d20 Compare April 16, 2024 14:11
@kvaster
Copy link
Contributor Author

kvaster commented Apr 16, 2024

It looks strange for me, cause I've done this several times before...
Anywa it's fixed now!

@kvaster
Add option to disable ExternalIP mitigation (CVE-2020-8554).
d238758 
This mitigation has it's own drawbacks for some setups.
It prevents pods communication in same cluster via ExternalIP when DSR is enabled.

Fixes cilium#28187

Signed-off-by: Viktor Kuzmin <kvaster@gmail.com>
@kvaster kvaster force-pushed the disable-external-ip-mitigation branch from afe1d20 to d238758 Compare April 16, 2024 14:36
@julianwiedmann
Copy link
Member

/test

julianwiedmann
julianwiedmann approved these changes Apr 17, 2024
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

lgtm, thank you!

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 17, 2024
@julianwiedmann julianwiedmann added the feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. label Apr 17, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Apr 17, 2024
Merged via the queue into cilium:main with commit 80ce110 Apr 17, 2024
61 of 62 checks passed
@brb brb mentioned this pull request Apr 24, 2024
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derailed derailed derailed approved these changes

@gandro gandro gandro approved these changes

@lmb lmb lmb approved these changes

@borkmann borkmann borkmann approved these changes

@joamaki joamaki joamaki approved these changes

@danehans danehans danehans approved these changes

@lambdanis lambdanis lambdanis approved these changes

@ysksuzuki ysksuzuki ysksuzuki approved these changes

@julianwiedmann julianwiedmann julianwiedmann approved these changes

Assignees
No one assigned
Labels
area/loadbalancing Impacts load-balancing and Kubernetes service implementations feature/dsr Relates to Cilium's Direct-Server-Return feature for KPR. feature/socket-lb Impacts the Socket-LB part of Cilium's kube-proxy replacement. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Regression in DSR for some setups
10 participants
@kvaster @julianwiedmann @derailed @gandro @lmb @borkmann @joamaki @danehans @lambdanis @ysksuzuki