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

datapath: optionally disable SIP verification #16134

Merged
merged 1 commit into from May 21, 2021

Conversation

oblazek
Copy link
Contributor

@oblazek oblazek commented May 13, 2021

Enable an old datapath option for disabling source IP
verification which prevents IP spoofing. In some cases
it might be beneficial for users to have this option
(for example something like DHCP discover which sends
a packet with srcIP 0.0.0.0).

Signed-off-by: Ondrej Blazek ondra.blazkuj@gmail.com

The macro DISABLE_SIP_VERIFICATION has been unused,
the new datapath option enables it.

@oblazek oblazek requested a review from a team as a code owner May 13, 2021 06:20
@oblazek oblazek requested a review from a team May 13, 2021 06:20
@oblazek oblazek requested review from a team as code owners May 13, 2021 06:20
@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 13, 2021
@oblazek oblazek requested a review from jrajahalme May 13, 2021 06:20
@oblazek oblazek requested a review from ti-mo May 13, 2021 06:20
@pchaigno pchaigno added 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. labels May 14, 2021
@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 May 14, 2021
@pchaigno pchaigno changed the title datapath: optionaly disable SIP verification datapath: optionally disable SIP verification May 14, 2021
@pchaigno
Copy link
Member

@oblazek The tests are failing because some generated files need to be regenerated.

@oblazek
Copy link
Contributor Author

oblazek commented May 14, 2021

@oblazek The tests are failing because some generated files need to be regenerated.

Oh, I accidentally modified autogenerated file, will try to fix. Thanks.

@oblazek
Copy link
Contributor Author

oblazek commented May 17, 2021

Code should be ready now, but could not figure out how to keep naming in this format DisableSIPVerification vs DisableSipVerification which is generated by the swagger.
One thing I am unsure what to do with build_commits / Check if build works for every commit (pull_request) failing.

Details show this:

../pkg/endpoint/bpf.go:1485:32: e.DatapathConfiguration.DisableSIPVerification undefined (type models.EndpointDatapathConfiguration has no field or method DisableSIPVerification

which I have fixed in the latest commit.

@pchaigno
Copy link
Member

Details show this:

../pkg/endpoint/bpf.go:1485:32: e.DatapathConfiguration.DisableSIPVerification undefined (type models.EndpointDatapathConfiguration has no field or method DisableSIPVerification

which I have fixed in the latest commit.

That GitHub action ensures your commits don't break git bisect. It checks your commit history is clean. I think you just need to squash your commits to fix it.

@oblazek
Copy link
Contributor Author

oblazek commented May 17, 2021

Details show this:

../pkg/endpoint/bpf.go:1485:32: e.DatapathConfiguration.DisableSIPVerification undefined (type models.EndpointDatapathConfiguration has no field or method DisableSIPVerification

which I have fixed in the latest commit.

That GitHub action ensures your commits don't break git bisect. It checks your commit history is clean. I think you just need to squash your commits to fix it.

Ah, got it.. thought gh has a way of squashing commits with a simple checkbox like gitlab has (but is hidden by default).

Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! Let's try to keep acronyms capitalized on non-autogenerated files wherever possible. Only the field in EndpointDatapathConfiguration should contain Sip, the rest should be capitalized. 👍

pkg/datapath/config.go Outdated Show resolved Hide resolved
pkg/endpoint/cache.go Outdated Show resolved Hide resolved
Enable an old datapath option for disabling source IP
verification which prevents IP spoofing. In some cases
it might be beneficial for users to have this option
(for example something like DHCP discover which sends
a packet with srcIP 0.0.0.0).

Signed-off-by: Ondrej Blazek <ondra.blazkuj@gmail.com>
@pchaigno
Copy link
Member

Smoke tests should already cover these changes and I don't think we need to run the full end-to-end tests given the option is never enabled in CI. All team reviews are covered. Marking as ready to merge.

@pchaigno pchaigno added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 21, 2021
@twpayne twpayne merged commit 9c72798 into cilium:master May 21, 2021
@michaelraskansky
Copy link

Hello, where can I set the parameter to disable sip verification? I can't find any reference in the documentation?

Thanks.

@pchaigno
Copy link
Member

pchaigno commented Dec 8, 2021

@michaelraskansky I don't believe it's currently possible to enable it unless you integrate directly with Cilium. I would be in favor of adding a flag to enable it globally for the agent. Though I'm wondering what's your use case to skip this check?

@michaelraskansky
Copy link

Hi @pchaigno , thanks for the quick reply.
I am routing traffic from external IP networks via pods within the Kuberenets cluster.
The external networks are connected with a tunneling interface to the pods namespace.

Something like this.
External IP Network < ---------> (wg0) Pod (eth0) <---------> Internet

michaelraskansky added a commit to michaelraskansky/cilium that referenced this pull request Dec 29, 2021
Add flag to disable sip veification.
This will allow to configure the datapath so it dose'nt
drop packets due to invalid source ip in the datapath.

This is helpful when routing IP payload from external ip networks
 through kubernets via ip tunnels. See cilium#16134 for more infomations.

Signed-off-by: Michael Raskansky <michaelraskansky@gmail.com>
michaelraskansky added a commit to michaelraskansky/cilium that referenced this pull request Mar 28, 2022
Add flag to disable sip veification.
This will allow to configure the datapath so it dose'nt
drop packets due to invalid source ip in the datapath.

This is helpful when routing IP payload from external ip networks
 through kubernets via ip tunnels. See cilium#16134 for more infomations.

Signed-off-by: Michael Raskansky <michaelraskansky@gmail.com>
Jiang1155 pushed a commit to Jiang1155/cilium that referenced this pull request Apr 6, 2022
Add flag to disable sip veification.
This will allow to configure the datapath so it dose'nt
drop packets due to invalid source ip in the datapath.

This is helpful when routing IP payload from external ip networks
 through kubernets via ip tunnels. See cilium#16134 for more infomations.

Signed-off-by: Michael Raskansky <michaelraskansky@gmail.com>

check epTemplate.DatapathConfiguration only once

Signed-off-by: Michael Raskansky <michaelraskansky@gmail.com>

check epTemplate.DatapathConfiguration only once
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants