-
Notifications
You must be signed in to change notification settings - Fork 25.2k
[UIAM] Cloud API key authentication #128440
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
base: main
Are you sure you want to change the base?
Conversation
@@ -128,6 +129,10 @@ default ServiceAccountTokenStore getServiceAccountTokenStore(SecurityComponents | |||
return null; | |||
} | |||
|
|||
default CloudApiKeyService getCloudApiKeyService(SecurityComponents components) { | |||
return null; | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't love this. Is there a reason you specifically want a pluggable CloudApiKeyService
rather than just additional authenticators?
That is, I would have opted to have a more generic extension point and push most of the CloudApiKey
code into the extension itself rather than in core security.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my original idea too. It would require moving quite a lot of classes around though since Authenticator
is not accessible from SecurityExtension
:
SecurityExtension
is inorg.elasticsearch.xpack.core.security
which does not have access toAuthenticator
Authenticator
is inorg.elasticsearch.xpack.security.authc
which depends onorg.elasticsearch.xpack.core.security
Moving it would require quite a few classes getting pulled up -- it feels like something we can follow up on in a bigger refactor subsequently but let me know if it's too big a blocker for you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we could consider is injecting a cloud API key authenticator like class into ApiKeyAuthenticator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed a commit that uses a CustomApiKeyAuthenticator
extension point -- as discussed on Slack.
@@ -47,6 +47,7 @@ public class Subject { | |||
public enum Type { | |||
USER, | |||
API_KEY, | |||
CLOUD_API_KEY, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have to check and handle BWC implications with addition of the new subject type and realm.
It's a bit awkward that certain implementation details of a custom cloud API key authentication are in the core, but I don't see a simple way (at the moment) that we can avoid this without massive authentication refactoring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this PR only introduces the new subject type without actually using it, I think it should be fine to handle the BWC and introduce a new transport version in a followup PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.allAuthenticators = List.of( | ||
serviceAccountAuthenticator, | ||
oAuth2TokenAuthenticator, | ||
pluggableApiKeyAuthenticator, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it even make sense to add authenticator to the chain if it's a noop?
We do add others when they are not enabled (e.g OAuth2 token and API key authenticator), but I'm wondering if we should change that (not necessarily in this PR)?
* can be plugged into the authenticator chain. Module dependencies prevent us from introducing a direct extension point for | ||
* an {@link Authenticator}. | ||
*/ | ||
public class PluggableApiKeyAuthenticator implements Authenticator { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing we should consider is adding (in a followup) the APM authentication metrics.
Pinging @elastic/es-security (Team:Security) |
This PR adds a new cloud API key subject type and authenticator, and an extension point for internal plugins to inject custom implementations for cloud API key authentication.
Resolves: ES-11882