Skip to content

cilium, ipsec: Do revalidate_data_pull() early in do_decrypt() case#13500

Merged
aanm merged 1 commit intomasterfrom
data_pull
Oct 19, 2020
Merged

cilium, ipsec: Do revalidate_data_pull() early in do_decrypt() case#13500
aanm merged 1 commit intomasterfrom
data_pull

Conversation

@jrfastab
Copy link
Copy Markdown
Contributor

@jrfastab jrfastab commented Oct 11, 2020

When we created a lib for encryption the pull_data call was pulled
into the per protocol cases versus being called regardless of if
its been decrypted already or not.

Lets pull it back to the previous location.

Next we returned a drop code DROP_INVALID incorrectly as well
so lets make that an accept return code. We shouldn't be dropping
packets from here, lets let the stack or upper layer Cilium code do
that if it needs to.

Finally for same reason above lets allow all non esp packets to
go up the stack. The will still hit a Cilium policy program later
so let that code work as expected.

Fixes #13528

Signed-off-by: John Fastabend john.fastabend@gmail.com


@nebril bisected an encryption failure back to this patch 7ba0e83 and this corrects the difference between the old code and the new lib based code. However it seems that the pull should be unnecessary to me unless we have a miss in the cilium_host codebase as well?

@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 Oct 11, 2020
@jrfastab
Copy link
Copy Markdown
Contributor Author

retest-4.9

@jrfastab
Copy link
Copy Markdown
Contributor Author

K8s-1.19-kernel-4.9 passed lets try again.

@jrfastab
Copy link
Copy Markdown
Contributor Author

retest-4.9

@jrfastab
Copy link
Copy Markdown
Contributor Author

tbd patch needs a fixes tag.

@jrfastab
Copy link
Copy Markdown
Contributor Author

2/2 try again.

@jrfastab
Copy link
Copy Markdown
Contributor Author

retest-4.9

@jrfastab
Copy link
Copy Markdown
Contributor Author

3/3, try entire set now.

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-me-please

@aanm aanm marked this pull request as draft October 12, 2020 08:41
@nebril
Copy link
Copy Markdown
Member

nebril commented Oct 12, 2020

test-missed-k8s

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

@maintainer-s-little-helper
Copy link
Copy Markdown

Commits eba3a68263bb18332b3f6972fc5c706e7822c193, 48613bf7cce85447599a62355c92910c6291947b, 0a5e16d6186f4e005c62f2cd984f6d65dd4ea2d4 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 12, 2020
@jrfastab
Copy link
Copy Markdown
Contributor Author

So test-missed-k8s failed on encryption tests lets try leaving the code refactor in place, but revert the host changes.

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

@maintainer-s-little-helper
Copy link
Copy Markdown

Commits cfab5acf03ead0f0b1a0e1488eec92f80d940bbc, 8261efe98f5b36f4d6bd3f8d4a4e84b38b43355b, 033086efc12068e7e47fe6e95a6de93002b34bf7 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

@maintainer-s-little-helper
Copy link
Copy Markdown

Commits cfab5acf03ead0f0b1a0e1488eec92f80d940bbc, 8261efe98f5b36f4d6bd3f8d4a4e84b38b43355b, 033086efc12068e7e47fe6e95a6de93002b34bf7, bc17f82073de14b7432081548dee1a855cdffe03 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

@maintainer-s-little-helper
Copy link
Copy Markdown

Commits cfab5acf03ead0f0b1a0e1488eec92f80d940bbc, 8261efe98f5b36f4d6bd3f8d4a4e84b38b43355b, 033086efc12068e7e47fe6e95a6de93002b34bf7 do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

When moving encryption code into a library we made some errors. First,
we created a path where we redirect non-encrypted data to cilium_ifindex.
Usually this is fine except for the case where we need to use the
iptables snat/dnat rules on the traffic and we bypass those by doing
a BPF redirect. This manifested as a failure when running the cilium
test-connectivity example yaml. The host to pod service IP was broke.

Next, we also skipped doing data_pull in all cases which causes some
issues up the stack if we have not pulled the data in after popping
the IP header off.

Fixes: 9ed106a ("cilium: create lib for encryption")
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 13, 2020
@jrfastab
Copy link
Copy Markdown
Contributor Author

test-missed-k8s

@aanm aanm added the needs/triage This issue requires triaging to establish severity and next steps. label Oct 13, 2020
@jrfastab jrfastab marked this pull request as ready for review October 16, 2020 15:58
@jrfastab jrfastab requested a review from a team October 16, 2020 15:58
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Minor query on ctx->mark handling in case where pulling fails, plus it would be nice to have the PR description text in the commit message as well just for git log background.

Oh and CI looks like it needs a look.

Comment thread bpf/lib/encrypt.h Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do we need to clear ctx->mark in these cases?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Probably a nice touch to clear the mark so we don't try to decrypt a decrypted packet if the data_pull fails after encryption.

@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Oct 16, 2020
@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 Oct 16, 2020
@joestringer joestringer added the area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. label Oct 16, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot added ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Oct 16, 2020
@aanm aanm removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 16, 2020
@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 Oct 16, 2020
@jrfastab
Copy link
Copy Markdown
Contributor Author

@joestringer wrt CI, none of those tests should even have encryption enabled.

@joestringer joestringer removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Oct 16, 2020
@jrfastab
Copy link
Copy Markdown
Contributor Author

Suite-k8s-1.13.K8sDatapathConfig Encapsulation Check vxlan connectivity with per-endpoint routes; https://datastudio.google.com/s/n0v8X8zXamo

@jrfastab
Copy link
Copy Markdown
Contributor Author

K8sDatapathConfig Encapsulation Check iptables masquerading with random-fully: https://datastudio.google.com/s/poztQoaWOL8

Only a couple failures on this one, but seems unrelated to this PR.

@joestringer
Copy link
Copy Markdown
Member

K8sDatapathConfig Encapsulation Check iptables masquerading with random-fully is pretty new so we won't have a lot of data on its reliability. /cc @pchaigno

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-me-please

@jrfastab
Copy link
Copy Markdown
Contributor Author

test-me-please

@jrfastab
Copy link
Copy Markdown
Contributor Author

k8s-1.12-kernel-netnext failure: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.12-net-next/900/ (Suite-k8s-1.12.K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing and DSR)

@jrfastab
Copy link
Copy Markdown
Contributor Author

retest-net-next

@jrfastab
Copy link
Copy Markdown
Contributor Author

Runtime-4.9 failure: https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/2227/ (Suite-runtime.RuntimePolicies Tests Egress To World)

@jrfastab
Copy link
Copy Markdown
Contributor Author

retest-runtime

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/encryption Impacts encryption support such as IPSec, WireGuard, or kTLS. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CI: K8sDatapathConfig Transparent encryption DirectRouting Check connectivity with transparent encryption and direct routing

5 participants