policy: Add GetAuthTypes() #26116

jrajahalme · 2023-06-12T12:26:36Z

Track the required authentication mode for all selected remote identities
in a selector policy, and add GetAuthTypes() that can be used to find out
the required authentication modes between a local and remote security
identity.

We recently added authentication requirement support to Cilium Network Policy. This is currently plumbed to bpf datapath policymap values so that the datapath may enforce authentication status of traffic by consulting the new bpf authmap. Authmap is updated by Cilium Agent's auth manager (pkg/auth), currently based on the "authentication required" signals delivered from the datapath to the agent via signalmap. Once authentication is done, auth manager updates authmap and datapath starts accepting traffic.

The new functionality provided in this PR is needed for the auth manager to be able to decide if an expiring authentication needs to be renewed (re-authenticated). Without this functionality re-authentication between given Cilium security IDs (local/remote) will be done perpetually, or until either the security ID is removed, or the node with which the authentication was done, is removed. The new GetAuthTypes() function is intended to be called by the auth manager to potentially end re-authentication for the given local/remote security ID pair, when no AuthTypes are returned, indicating that the policy has changed to not require authentication between the given local/remote security ID any more.

#26068 uses this new functionality and is currently blocked for this functionality to be added.

pkg/policy/distillery.go

mhofstetter

LGTM - thanks @jrajahalme

only the small non-blocking question regarding the API (first matching auth type vs bool)

Use PolicyCache in unit tests. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

Track the required authentication mode for all selected remote identities in a selector policy, and add GetAuthType() that can be used to find out the required authentication mode between a local and remote security identity. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

Track and return multiple AuthTypes for GetAuthTypes(), add multiple auth types to the distillery unit test. While we currently plan to only support Spire AuthType, the policy engine is wired for supporting multiple auth types. Honor this in the new GetAuthTypes() internal API. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

jrajahalme · 2023-06-13T09:52:38Z

Added commit to keep track and report multiple AuthTypes between a pair of local and remote IDs.

jrajahalme · 2023-06-13T12:06:58Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' hit: #25958 (91.70% similarity)

aditighag · 2023-06-13T15:03:31Z

I'm going to remove my review as the changes seem specific to auth policy, and the PR doesn't give any context for what was earlier broken, and whether there are now tests covering that logic. Hopefully we can lean on @mhofstetter's review.
@jrajahalme Is there a way to decouple the auth policies related logic to a separate logical module within the policy package? It would be helpful to have a high-level summary of the logic (unless it's already in there somewhere?). /cc @joestringer

jrajahalme · 2023-06-14T06:48:31Z

/test-1.26-net-next

jrajahalme · 2023-06-14T09:17:51Z

the PR doesn't give any context for what was earlier broken, and whether there are now tests covering that logic.

Added context to the PR description. PR contains changes to the policy package unit tests covering the new functionality.

joestringer

Makes sense, just some super minor nits on a couple of style/docs aspects. Feel free to roll into the PR, submit separately, or disagree with me in the threads below :)

joestringer · 2023-06-16T04:52:16Z

pkg/policy/l4.go

+						l4Policy.AuthMap = make(AuthMap, 1)
+					}
+					authTypes := l4Policy.AuthMap[cs]
+					if authTypes == nil {
+						authTypes = make(AuthTypes, 1)


nit: Why 1? Presumably these should be sized based on the number of authentication rules for any particular endpoint (well, local endpoint identity) instead?

Background I'm thinking here is that maybe we are trying to set the default map size in order to micro-optimize for memory allocation, but we end up shooting ourselves in the foot because we force reallocations if there's more than one authenticated peer for the endpoint?

I guess overall you might argue that overall "number of unique peers an app connects to" amortizes towards one peer identity and hence selected by one rule 🤔.

A cursory search around Go default map capacity seems to suggest one bucket, eight entries... do we think there's a strong reason to be particular about this aspect here?

I'm thinking worst case we could end up recalculating new selectorPolicy on a regular basis, and every time we first allocate a map that's too small, then allocate again to a size that fits the policy. Mildly wasteful.

Anyway this is not a big deal, just seems like it may be premature optimization.

joestringer · 2023-06-16T04:57:25Z

pkg/policy/distillery.go

+// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID, if
+// any, otherwise returns nil.


nit: ID can end up ambiguous, it'd be nice to give the reader one less thing to be confused about - stating clearly that the inputs are expected to be security identities. If by any weird course we end up plugging the wrong id numbers in, it could help point out the mistake.

Suggested change

// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID, if

// any, otherwise returns nil.

// GetAuthTypes returns the AuthTypes required by the policy between the

// security identities localID and remoteID, if any, otherwise returns nil.

joestringer · 2023-06-16T05:15:10Z

CI is failing anyway so we can't merge this until that's resolved.

joestringer · 2023-06-16T05:29:05Z

Typically I'd expect kind/feature to go along with release-note/minor or release-note/major. This PR looks like it only introduces new unused internal APIs That's fine, and it can help review to have smaller PRs like this 👍 . Given this, I changed it to kind/enhancement. Furthermore, given that it's required for bugfix #26068, I'm fine to merge this whenever we resolve the CI issues and any remaining review feedback iteration. I don't see the point in trying to subject this to feature freeze and hold back what's ultimately a bugfix for an exciting new feature that'll go into the upcoming 1.14 release.

jrajahalme · 2023-06-16T09:59:24Z

One non-required test failing, mlh marked this as ready-to-merge.

jrajahalme added 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. labels Jun 12, 2023

jrajahalme requested a review from mhofstetter June 12, 2023 12:26

jrajahalme requested a review from a team as a code owner June 12, 2023 12:26

jrajahalme requested a review from aditighag June 12, 2023 12:26

maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jun 12, 2023

jrajahalme added the release-note/misc This PR makes changes that have no direct user impact. label Jun 12, 2023

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 Jun 12, 2023

jrajahalme force-pushed the policy-auth-info branch 2 times, most recently from 686db21 to f7c3735 Compare June 12, 2023 12:29

mhofstetter reviewed Jun 12, 2023

View reviewed changes

pkg/policy/distillery.go Outdated Show resolved Hide resolved

mhofstetter approved these changes Jun 12, 2023

View reviewed changes

jrajahalme added 3 commits June 13, 2023 12:50

policy: Cache selector policies in distillery test

e3ed476

Use PolicyCache in unit tests. Signed-off-by: Jarno Rajahalme <jarno@isovalent.com>

jrajahalme force-pushed the policy-auth-info branch from f7c3735 to 53a473b Compare June 13, 2023 09:51

jrajahalme changed the title ~~policy: Add GetAuthType()~~ policy: Add GetAuthTypes() Jun 13, 2023

mhofstetter added the feature/authentication label Jun 13, 2023

jrajahalme self-assigned this Jun 13, 2023

maintainer-s-little-helper bot mentioned this pull request Jun 13, 2023

CI: Cilium K8s Client connection reset by peer #25958

Closed

jrajahalme added the kind/feature This introduces new functionality. label Jun 14, 2023

jrajahalme requested review from joestringer and removed request for aditighag June 14, 2023 15:33

joestringer approved these changes Jun 16, 2023

View reviewed changes

joestringer mentioned this pull request Jun 16, 2023

auth: policy based auth map GC #26068

Merged

joestringer added kind/enhancement This would improve or streamline existing functionality. and removed kind/feature This introduces new functionality. labels Jun 16, 2023

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 Jun 16, 2023

jrajahalme merged commit 54c5d36 into cilium:main Jun 16, 2023
64 of 65 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

policy: Add GetAuthTypes() #26116

policy: Add GetAuthTypes() #26116

jrajahalme commented Jun 12, 2023 •

edited

mhofstetter left a comment

jrajahalme commented Jun 13, 2023

jrajahalme commented Jun 13, 2023 •

edited by maintainer-s-little-helper bot

aditighag commented Jun 13, 2023 •

edited

jrajahalme commented Jun 14, 2023

jrajahalme commented Jun 14, 2023

joestringer left a comment

joestringer Jun 16, 2023

joestringer Jun 16, 2023

joestringer Jun 16, 2023

joestringer commented Jun 16, 2023

joestringer commented Jun 16, 2023

jrajahalme commented Jun 16, 2023 •

edited

		// GetAuthTypes returns the AuthTypes required by the policy between the localID and remoteID, if
		// any, otherwise returns nil.

policy: Add GetAuthTypes() #26116

policy: Add GetAuthTypes() #26116

Conversation

jrajahalme commented Jun 12, 2023 • edited

mhofstetter left a comment

Choose a reason for hiding this comment

jrajahalme commented Jun 13, 2023

jrajahalme commented Jun 13, 2023 • edited by maintainer-s-little-helper bot

aditighag commented Jun 13, 2023 • edited

jrajahalme commented Jun 14, 2023

jrajahalme commented Jun 14, 2023

joestringer left a comment

Choose a reason for hiding this comment

joestringer Jun 16, 2023

Choose a reason for hiding this comment

joestringer Jun 16, 2023

Choose a reason for hiding this comment

joestringer Jun 16, 2023

Choose a reason for hiding this comment

joestringer commented Jun 16, 2023

joestringer commented Jun 16, 2023

jrajahalme commented Jun 16, 2023 • edited

jrajahalme commented Jun 12, 2023 •

edited

jrajahalme commented Jun 13, 2023 •

edited by maintainer-s-little-helper bot

aditighag commented Jun 13, 2023 •

edited

jrajahalme commented Jun 16, 2023 •

edited