8000 Policy: Fix auth policies with proxy redirects by jrajahalme · Pull Request #37685 · cilium/cilium · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Policy: Fix auth policies with proxy redirects #37685

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

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

jrajahalme
Copy link
Member

When covered entries with lower proxy port priority have different explicit auth types, the proxy port (and priority) must be propagated to those entries instead of deleting them. Same applies in reverse, when adding an explicit auth entry the proxy port and priority must be propagated from a covering entry with higher priority proxy port, if any.

Keep the non-auth policy handling simple by branching to authPreferredInsert early, and take care of deny & higher priority proxy port logic in the same loops with the auth propagation. This required changing 'CoveringKeysWithSameID' and 'SubsetKeysWithSameID' to also iterate keys equal to the 'newKey', so that a deny key would not be overridden by an allow key, for example.

Add a new unit test to prevent future regressions.

Fixes: #36056

Auth policy is properly maintained also when covered by proxy redirects.

@jrajahalme jrajahalme added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. release-blocker/1.17 This issue will prevent the release of the next version of Cilium. needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Feb 17, 2025
@jrajahalme jrajahalme requested a review from a team as a code owner February 17, 2025 17:14
@jrajahalme jrajahalme requested a review from doniacld February 17, 2025 17:14
@jrajahalme
Copy link
Member Author

/test

Copy link
Contributor
@doniacld doniacld left a comment

Choose a reason for hiding this comment

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

Nice work!
nit: mispellingpropagaed in your second commit message.

@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 Feb 18, 2025
@squeed squeed self-requested a review February 18, 2025 12:37
@joestringer joestringer added dont-merge/waiting-for-review Requires further review before merging. and removed ready-to-merge This PR has passed all tests and received consensus from code owners to merge. labels Feb 18, 2025
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the policy-auth-fix branch 2 times, most recently from 4431442 to 68cba00 Compare February 19, 2025 16:43
@jrajahalme
Copy link
Member Author

Added more testing

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme requested a review from squeed February 19, 2025 17:07
@jrajahalme jrajahalme removed the dont-merge/waiting-for-review Requires further review before merging. label Feb 20, 2025
@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 Feb 20, 2025
Only add a delete into ChangeState if the key was not added on the same
round of 'changes'.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Proxy port with a higher precedence should be propagated to a covered
entry instead of deleting it, if the covered entry has a different
explicit auth type than the covering proxy redirect entry.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
When covered entries with lower proxy port priority have different
explicit auth types, the proxy port (and priority) must be propagated to
those entries instead of deleting them. Same applies in reverse, when
adding an explicit auth entry the proxy port and priority must be
propagated from a covering entry with higher priority proxy port, if any.

Keep the non-auth policy handling simple by branching to
authPreferredInsert early, and take care of deny & higher priority proxy
port logic in the same loops with the auth propagation. This required
changing 'CoveringKeysWithSameID' and 'SubsetKeysWithSameID' to also
iterate keys equal to the 'newKey', so that a deny key would not be
overridden by an allow key, for example.

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
@jrajahalme
Copy link
Member Author

Fixed spelling mistake on the last commit message.

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme added this pull request to the merge queue Feb 20, 2025
Merged via the queue into cilium:main with commit 7e38bb5 Feb 20, 2025
63 checks passed
@jrajahalme jrajahalme deleted the policy-auth-fix branch February 20, 2025 15:17
@nbusseneau nbusseneau mentioned this pull request Feb 27, 2025
17 tasks
@nbusseneau nbusseneau added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels Feb 27, 2025
@github-actions github-actions bot added backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. and removed backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. labels Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-blocker/1.17 This issue will prevent the release of the next version of Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants
0