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

v1.13 backport: Policy deny override redirect #26749

Merged

Conversation

jrajahalme
Copy link
Member

Once this PR is merged, you can update the PR labels via:

for pr in 26344; do contrib/backporting/set-labels.py $pr done 1.13; done

or with

make add-labels BRANCH=v1.13 ISSUES=26344

[ upstream commit 9f52abb ]

Export DenyPreferredInsertWithChanges and make it revertible by taking a
map of old values as a new optional argument.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
[ upstream commit 8aa89ef ]

Use DenyPreferredInsert instead of directly manipulating policy map state
to make sure deny entries are not overridden by new proxy redirect
entries.

Prior to this fix it was possible for a proxy redirect to be pushed onto
the policy map when it should have been overridden by a deny at least in
these cases:

- L3-only deny with L3/L4 redirect: No redirect should be added as the L3
  is denied

- L3-only deny with L4-only redirect: L4-only redirect should be added
  and an L3/L4 deny should also be added, but the L3/L4 deny is only added
  by deny preferred insert, and is missed when the map is manipulated
  directly. A new test case verifies this.

It is clear that in the latter case the addition of the redirect can not
be completely blocked, so we can't fix this by making AllowsL4 more
restrictive. But also in the former case it is possible that the deny
rule only covers a subset of security identities, while the redirect rule
covers some of the same security identities, but also some more that
should not be blocked. Hence the correct fix here is to leave AllowsL4 to
be L3-independent, and cover these cases with deny preferred insert
instead of adding redirect entries to the map directly.

This commit also contains a related change that allows a redirect entry
to be updated, maybe with a changed proxy port. I've not seen evidence
that this is currently fixing a bug, but it feels like a real
possibility.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme jrajahalme added kind/backports This PR provides functionality previously merged into master. backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. labels Jul 11, 2023
@jrajahalme jrajahalme requested a review from a team as a code owner July 11, 2023 07:36
@jibi
Copy link
Member

jibi commented Jul 11, 2023

/test-backport-1.13

@jibi jibi requested a review from julianwiedmann July 11, 2023 13:24
@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 Jul 11, 2023
@julianwiedmann julianwiedmann merged commit 4cc43b6 into cilium:v1.13 Jul 11, 2023
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. 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
Development

Successfully merging this pull request may close these issues.

None yet

3 participants