auth: implement re-authentication in case of rotated certificates #25927
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhofstetter
added
kind/enhancement
mhofstetter
commented
Jun 6, 2023
|
|
||
| func handleAuthentication(a *authManager, k authKey, reAuth bool) { | ||
| if a.markPendingAuth(k) { | ||
| go func(key authKey) { |
There was a problem hiding this comment.
Kind of wonder whether we really want to keep a dedicated go-routine for every (re-)authentication request.
I tend to an implementation with a worker-pool which is working on the auth requests. This would be easier to test and prevent the manager from overload the authhandlers (e.g. SPIRE).
But keeping it as is for the context of this PR.
This commit refactors the authentication triggered by the signal map to use the hive job framework. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
This commit introduces mutual auth re-authentication. Whenever an authhandler is emiting CertificateRotatedEvents, an authentication will be triggered. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter
force-pushed
the
pr/mhofstetter/authmanager-reauthentication
branch
from
9c18657
to
2fdb7fe
Compare
meyskens
approved these changes
Jun 6, 2023
There was a problem hiding this comment.
LGTM looks like a clean approach :)
A worker pool would be a good improvement for drop-in deployments of spire where it would immediately create spire IDs at the same time so they all get the same renewal schedule. However it could also be a good follow up pr imo.
|
/test Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed: Click to show.Test NameFailure OutputJenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/500/ If it is a flake and a GitHub issue doesn't already exist to track it, comment Then please upload the Jenkins artifacts to that issue. |
maintainer-s-little-helper
bot
added
the
ready-to-merge
giorio94
added a commit
to giorio94/cilium
that referenced
this pull request
Jun 8, 2023
This commit adds back the stream package import, which appears to be missing due to a merge race between cilium#25927 and cilium#25934. Fixes: eb653b6 ("auth: use Resource.Observe for jobs") Fixes: ebb6fc3 ("auth: implement re-authentication in case of rotated certificates") Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
dylandreimerink
pushed a commit
that referenced
this pull request
Jun 8, 2023
This commit adds back the stream package import, which appears to be missing due to a merge race between #25927 and #25934. Fixes: eb653b6 ("auth: use Resource.Observe for jobs") Fixes: ebb6fc3 ("auth: implement re-authentication in case of rotated certificates") Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
mhofstetter
added a commit
to mhofstetter/cilium
that referenced
this pull request
Jun 16, 2023
Until now, auth map entries were garbage collected based on the following criterias: * related identity has been deleted * related node has been deleted * entry has been expired The initial goal was that expiration will cover the case where no longer a policy is enforcing authentication. But the introduction of re-authentication (cilium#25927) changed this, because the entries would have re-authenticated "forever" (until identity or node would have been deleted). Therefore, this commit introduces some rudimentary garbage collection based on policies by periodically checking whether a policy is still enforcing authentication between two identities. If not, the auth map entry gets deleted. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
tklauser
pushed a commit
that referenced
this pull request
Jun 16, 2023
Until now, auth map entries were garbage collected based on the following criterias: * related identity has been deleted * related node has been deleted * entry has been expired The initial goal was that expiration will cover the case where no longer a policy is enforcing authentication. But the introduction of re-authentication (#25927) changed this, because the entries would have re-authenticated "forever" (until identity or node would have been deleted). Therefore, this commit introduces some rudimentary garbage collection based on policies by periodically checking whether a policy is still enforcing authentication between two identities. If not, the auth map entry gets deleted. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
romanspb80
pushed a commit
to romanspb80/cilium
that referenced
this pull request
Jun 22, 2023
This commit adds back the stream package import, which appears to be missing due to a merge race between cilium#25927 and cilium#25934. Fixes: eb653b6 ("auth: use Resource.Observe for jobs") Fixes: ebb6fc3 ("auth: implement re-authentication in case of rotated certificates") Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
romanspb80
pushed a commit
to romanspb80/cilium
that referenced
this pull request
Jun 22, 2023
Until now, auth map entries were garbage collected based on the following criterias: * related identity has been deleted * related node has been deleted * entry has been expired The initial goal was that expiration will cover the case where no longer a policy is enforcing authentication. But the introduction of re-authentication (cilium#25927) changed this, because the entries would have re-authenticated "forever" (until identity or node would have been deleted). Therefore, this commit introduces some rudimentary garbage collection based on policies by periodically checking whether a policy is still enforcing authentication between two identities. If not, the auth map entry gets deleted. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces mutual auth re-authentication. Whenever an
authhandler is emiting CertificateRotatedEvents, an authentication will
be triggered.
In addition, the existing authentication flow has been refactored to use the hive job framework.
Fixes #25475