Skip to content

[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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented May 26, 2025

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

@n1v0lg n1v0lg self-assigned this May 26, 2025
@n1v0lg n1v0lg added >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels May 26, 2025
@elasticsearchmachine elasticsearchmachine added serverless-linked Added by automation, don't add manually v9.1.0 labels May 26, 2025
@@ -128,6 +129,10 @@ default ServiceAccountTokenStore getServiceAccountTokenStore(SecurityComponents
return null;
}

default CloudApiKeyService getCloudApiKeyService(SecurityComponents components) {
return null;
}
Copy link
Contributor

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.

Copy link
Contributor Author

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 in org.elasticsearch.xpack.core.security which does not have access to Authenticator
  • Authenticator is in org.elasticsearch.xpack.security.authc which depends on org.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.

Copy link
Contributor Author

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

cc @slobodanadamovic

Copy link
Contributor Author

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,
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've decided to lean towards the safer side, so I added a new transport version and checks in f1965d3, and tests in bd19d18.

this.allAuthenticators = List.of(
serviceAccountAuthenticator,
oAuth2TokenAuthenticator,
pluggableApiKeyAuthenticator,
Copy link
Contributor

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 {
Copy link
Contributor

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.

@slobodanadamovic slobodanadamovic marked this pull request as ready for review June 3, 2025 11:06
@slobodanadamovic slobodanadamovic requested a review from tvernum June 3, 2025 11:06
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Jun 3, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) serverless-linked Added by automation, don't add manually Team:Security Meta label for security team v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants