[UIAM] Cloud API key authentication #128440
Conversation
n1v0lg
added
>non-issue
:Security/Authentication
elasticsearchmachine
added
serverless-linked
| @@ -128,6 +129,10 @@ default ServiceAccountTokenStore getServiceAccountTokenStore(SecurityComponents | |||
| return null; | |||
| } | |||
|
|
|||
| default CloudApiKeyService getCloudApiKeyService(SecurityComponents components) { | |||
| return null; | |||
| } | |||
There was a problem hiding this comment.
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.
That was my original idea too. It would require moving quite a lot of classes around though since Authenticator is not accessible from SecurityExtension:
SecurityExtensionis inorg.elasticsearch.xpack.core.securitywhich does not have access toAuthenticatorAuthenticatoris inorg.elasticsearch.xpack.security.authcwhich 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.
One thing we could consider is injecting a cloud API key authenticator like class into ApiKeyAuthenticator
There was a problem hiding this comment.
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.
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.
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.
| this.allAuthenticators = List.of( | ||
| serviceAccountAuthenticator, | ||
| oAuth2TokenAuthenticator, | ||
| pluggableApiKeyAuthenticator, |
There was a problem hiding this comment.
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.
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