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

Policy auth precedence fix #26331

Merged
merged 19 commits into from Jul 12, 2023
Merged

Conversation

jrajahalme
Copy link
Member

@jrajahalme jrajahalme commented Jun 17, 2023

Note for review: This is blocked by #26344 and needs to be rebased after it merges. First two commits are from this.

Explicitly specified authentication mode should apply to all traffic that matches the given policy map key. For example, if an L3-only policy specified that authentication is required, then it should be required also on all L3/L4-policies on the same L3, even if not specified in the L3/L4-policy. Similarly, an L3-only auth policy should apply to to all ports on the given L3, even if there is an L4-only policy that has no authentication requirement. In the most general case, an Allow-all policy with authentication requirement should apply to all traffic, unless authentication is explicitly disabled for specific L3, L4, or both.

To achieve this, we must scan all the policy map keys, when adding a new entry, as the new entry's auth type may have to be overridden by the explicit auth type of a more general entry, or the auth type of more specific entries may have to be updated due to the explicit auth type on the new entry. This is somewhat similar to how deny rules take precedence over more specific entries.

To limit the additional computational cost of this map scanning, we track the use of authentication (and deny rules) on the ingress/egress policy level, and skip the scanning that is not necessary when the policy does not contain any authentication (or deny) rules.

The commit to merge DerivedFromRules (from PR #26346) is included here to allow for the unit tests in the last commit to reliably track the DerivedFromRules labels of the entries added by toMapState().

@jrajahalme jrajahalme requested review from a team as code owners June 17, 2023 12:25
@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 Jun 17, 2023
@jrajahalme jrajahalme marked this pull request as draft June 17, 2023 12:25
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme force-pushed the policy-auth-precedence-fix branch 2 times, most recently from 77f52e7 to e710b4f Compare June 18, 2023 08:01
@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test

@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme marked this pull request as ready for review June 19, 2023 14:32
@jrajahalme jrajahalme self-assigned this Jun 20, 2023
@jrajahalme jrajahalme added release-note/misc This PR makes changes that have no direct user impact. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. area/servicemesh GH issues or PRs regarding servicemesh release-blocker/1.14 This issue will prevent the release of the next version of Cilium. kind/bug This is a bug in the Cilium logic. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 20, 2023
@joestringer joestringer self-requested a review June 20, 2023 21:53
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.0 Jul 10, 2023
@jrajahalme
Copy link
Member Author

/test

@jrajahalme jrajahalme removed the request for review from aditighag July 12, 2023 13:27
@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 12, 2023
@jrajahalme jrajahalme requested a review from nebril July 12, 2023 13:46
@jrajahalme jrajahalme merged commit 4547c0a into cilium:main Jul 12, 2023
64 of 65 checks passed
@jibi jibi removed the affects/v1.14 This issue affects v1.14 branch label Jul 12, 2023
@jibi jibi mentioned this pull request Jul 13, 2023
13 tasks
@jrajahalme jrajahalme added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Jul 13, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.0 Jul 13, 2023
@aanm aanm added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. labels Jul 14, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.14 to Backport done to v1.14 in 1.14.0 Jul 14, 2023
jrajahalme added a commit to jrajahalme/cilium that referenced this pull request Dec 12, 2023
Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: cilium#26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
github-merge-queue bot pushed a commit that referenced this pull request Dec 19, 2023
Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
YutaroHayakawa pushed a commit that referenced this pull request Dec 20, 2023
[ upstream commit 3d3f69c ]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
YutaroHayakawa pushed a commit that referenced this pull request Dec 20, 2023
[ upstream commit 3d3f69c ]

[ backporter's note: Fix a minor conflict comes from the variable name
  change (key -> ms).]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
YutaroHayakawa pushed a commit that referenced this pull request Dec 20, 2023
[ upstream commit 3d3f69c ]

[ backporter's note: Fix a minor conflict comes from the variable name
  change (key -> ms).]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
pippolo84 pushed a commit to pippolo84/cilium that referenced this pull request Jan 2, 2024
[ upstream commit 3d3f69c ]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: cilium#26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
YutaroHayakawa pushed a commit that referenced this pull request Jan 10, 2024
[ upstream commit 3d3f69c ]

[ backporter's note: Fix a minor conflict comes from the variable name
  change (key -> ms).]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
dylandreimerink pushed a commit that referenced this pull request Jan 12, 2024
[ upstream commit 3d3f69c ]

[ backporter's note: Fix a minor conflict comes from the variable name
  change (key -> ms).]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Yutaro Hayakawa <yutaro.hayakawa@isovalent.com>
dylandreimerink pushed a commit that referenced this pull request Jan 12, 2024
[ upstream commit 3d3f69c ]

Datapath equalness of the old and new entry must be evaluated before
entries are merged, afterwards the entries will be the same and the key
is not added in ChangeState.Adds, which may lead to a policy map entry
not being updated during incremental updates.

Fixes: #26331

Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>
Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.14 The backport for Cilium 1.14.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.14 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
No open projects
1.14.0
Backport done to v1.14
Development

Successfully merging this pull request may close these issues.

None yet

6 participants