Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612

jfreden · 2025-04-10T14:40:01Z

This PR adds support for providing a custom ServiceAccountTokenStore implementation in a SecurityExtension. If at least one SecurityExtension provides a custom ServiceAccountTokenStore the native and file based stores will be disabled and replaced by a read only CompositeServiceAccountTokenStore of all extension provided token stores.

jfreden · 2025-04-24T09:10:36Z

...n/core/src/main/java/org/elasticsearch/xpack/core/security/authc/service/ServiceAccount.java

@@ -5,7 +5,7 @@
 * 2.0.


These had to move since they're now part of the SecurityExtension interface through ServiceAccountTokenStore.

jfreden · 2025-04-24T09:13:49Z

...ava/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountTokenStoreTests.java

@@ -53,22 +55,30 @@ public void stop() {
        }
    }

+    private ServiceAccountToken newMockServiceAccountToken(ServiceAccountId accountId, String tokenName, SecureString secret) {


Added this mocking instead of making the ServiceAccountToken constructor public. The ServiceAccountToken needs to be in core since it's part of the security extension interface. The ServiceAccountService could also be moved to core, but that involves moving all service account definitions to core, which feels like too big of a refactor to justify.

jfreden · 2025-04-24T09:23:07Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        );
+        components.add(indexServiceAccountTokenStore);
+
+        final FileServiceAccountTokenStore fileServiceAccountTokenStore = new FileServiceAccountTokenStore(


The FileServiceAccountTokenStore is required by guice injection into TransportGetServiceAccountNodesCredentialsAction so have to create it even if it's not used.

It's techinically possible to not create TransportGetServiceAccountNodesCredentialsAction (and other similar actions) as well if the extension store is found. It might be worth the effort since it is strange to have the API available when the underlying mechanism is effectively disabled.

I agree with the line of reasoning, except I think it would be better for the action to exist and fail rather that to have a generic 404 in the Rest API.

Doing that right sounds like a multi-line / multi-file change, so perhaps we can revisit it in a future PR.

TransportGetServiceAccountNodesCredentialsAction used to rely on a FileServiceAccountTokenStore implementation being provided as a component. I've now changed it to be a ReadOnlyServiceAccountTokenStore. The security extension provides an instance of ReadOnlyServiceAccountTokenStore that will fail any attempts to do local node token lookup, hence the API will fail. This allows us to still create TransportGetServiceAccountNodesCredentialsAction but fail the action if called in serverless.

It's techinically possible to not create TransportGetServiceAccountNodesCredentialsAction (and other similar actions) as well if the extension store is found. It might be worth the effort since it is strange to have the API available when the underlying mechanism is effectively disabled.

I don't see any other places where transport actions are conditionally excluded in the security plugin, so that would be a new pattern I think? I think I prefer having the actions there, but the backing store preventing them from being used. Let me know if you prefer the 404 approach @ywangd .

transport actions are conditionally excluded in the security plugin

We do exclude most actions if security is disabled? That said, I don't have a strong opinion on whether it should be 404. I also agree that we can tackle this separately.

jfreden · 2025-04-24T09:47:31Z

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

@@ -149,14 +159,23 @@ public void createIndexToken(
        CreateServiceAccountTokenRequest request,
        ActionListener<CreateServiceAccountTokenResponse> listener
    ) {
+        if (indexServiceAccountTokenStore == null) {


We set indexServiceAccountTokenStore to null when an extension provides their own store. This is currently only used in serverless, so we'd never hit any of these. The impacted actions are:

GetServiceAccountNodesCredentialsAction - Not needed since it's not used in serverless and the same creds are on all nodes.

GetServiceAccountAction - Not needed since it's not used in serverless.

DeleteServiceAccountTokenAction - Not needed since it's not used in serverless and this is immutable in serverless.

CreateServiceAccountTokenAction - Not needed since it's not used in serverless and this is immutable in serverless.

ClearServiceAccountTokenStoreCacheAction - Not needed since it's not used in serverless, we might want this in the future to clear the cache.

If a custom ServiceAccountTokenStore is provided outside the serverless context it might be unexpected that some of these are not working. Any thoughts on that? My thinking is that we can only override the read-only store and in doing so the read-write store (index) is disabled.

If we skip creating the APIs as suggested earlier, we should be able to avoid some of these issues?

It seems possible and could be useful to support GetServiceAccountCredentials. But we can disable it for now and work on it as a follow-up.

If a custom ServiceAccountTokenStore is provided outside the serverless context it might be unexpected that some of these are not working.

I'd suggest we tighten the code to only allow internal implementation similar to this. There is no need to support other implementation.

There is no need to support other implementation.

👍

Since this is implemented as a SecurityExtension we don't have visibility into which plugin this is (like in the linked example). I have changed the approach to conditionally create the IndexServiceAccountTokenStore and FileServiceAccountTokenStore and moved it all to a createServiceAccountService method.

I think we can check the the number of provisions is 1 and the SecurityExtension's class name to be internal similar to the linked example?

Sorry, I didn't notice the internal check at first. I've added validation for that. Thanks!

…_account_auth

elasticsearchmachine · 2025-04-24T12:42:10Z

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

elasticsearchmachine · 2025-04-24T12:42:10Z

Hi @jfreden, I've created a changelog YAML for you.

ywangd

With the proposed changes, we assume no cluster-scoped service accounts. This is probably true and Tim and you may have already discussed and agreed on it. If that is the case, the changes look mostly good to me.

ywangd · 2025-05-01T05:36:43Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+            components.addAll(extensionTokenStores);
+            serviceAccountService = new ServiceAccountService(
+                client,
+                new CompositeServiceAccountTokenStore(extensionTokenStores, client.threadPool().getThreadContext())


I'd prefer we ensure there is only a single extensionTokenStore and avoid the composite wrapping.

That would be my inclination as well. We have no reason to support multiple stores, and we don't really know whether 2 arbitrary implementations would co-exist correctly.

ywangd · 2025-05-01T05:41:05Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        );
+        components.add(indexServiceAccountTokenStore);
+
+        final FileServiceAccountTokenStore fileServiceAccountTokenStore = new FileServiceAccountTokenStore(


It's techinically possible to not create TransportGetServiceAccountNodesCredentialsAction (and other similar actions) as well if the extension store is found. It might be worth the effort since it is strange to have the API available when the underlying mechanism is effectively disabled.

ywangd · 2025-05-01T05:51:49Z

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

+        this.readOnlyServiceAccountTokenStore = indexServiceAccountTokenStore != null
+            ? new CompositeServiceAccountTokenStore(
+                List.of(readOnlyServiceAccountTokenStore, indexServiceAccountTokenStore),
+                client.threadPool().getThreadContext()
+            )
+            : readOnlyServiceAccountTokenStore;


I'd rather pass in the readOnly store prepared and avoid creating it here by combining the index store.

ywangd · 2025-05-01T06:07:00Z

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

@@ -149,14 +159,23 @@ public void createIndexToken(
        CreateServiceAccountTokenRequest request,
        ActionListener<CreateServiceAccountTokenResponse> listener
    ) {
+        if (indexServiceAccountTokenStore == null) {


If we skip creating the APIs as suggested earlier, we should be able to avoid some of these issues?

It seems possible and could be useful to support GetServiceAccountCredentials. But we can disable it for now and work on it as a follow-up.

If a custom ServiceAccountTokenStore is provided outside the serverless context it might be unexpected that some of these are not working.

I'd suggest we tighten the code to only allow internal implementation similar to this. There is no need to support other implementation.

tvernum · 2025-05-05T06:48:14Z

With the proposed changes, we assume no cluster-scoped service accounts.

Yes, that is the intent. We do not have any environments where we want to support index-based service tokens and a customised token store.

If we ever did have that, then an extension could supply an implementation of IndexServiceAccountTokenStore as needed, but I doubt it will come up.

tvernum

I think this looks pretty good. I haven't had a chance to review the tests, so I don't want to approve just yet, but I'm happy for you to merge if @ywangd approves it.

Otherwise, I'll come back to it in a day or 2.

tvernum · 2025-05-05T06:52:42Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        );
+        components.add(indexServiceAccountTokenStore);
+
+        final FileServiceAccountTokenStore fileServiceAccountTokenStore = new FileServiceAccountTokenStore(


I agree with the line of reasoning, except I think it would be better for the action to exist and fail rather that to have a generic 404 in the Rest API.

Doing that right sounds like a multi-line / multi-file change, so perhaps we can revisit it in a future PR.

tvernum · 2025-05-05T06:54:40Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+            components.addAll(extensionTokenStores);
+            serviceAccountService = new ServiceAccountService(
+                client,
+                new CompositeServiceAccountTokenStore(extensionTokenStores, client.threadPool().getThreadContext())


That would be my inclination as well. We have no reason to support multiple stores, and we don't really know whether 2 arbitrary implementations would co-exist correctly.

tvernum · 2025-05-05T06:57:37Z

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

+        this.readOnlyServiceAccountTokenStore = indexServiceAccountTokenStore != null
+            ? new CompositeServiceAccountTokenStore(
+                List.of(readOnlyServiceAccountTokenStore, indexServiceAccountTokenStore),
+                client.threadPool().getThreadContext()
+            )
+            : readOnlyServiceAccountTokenStore;


tvernum · 2025-05-05T06:58:44Z

...rity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountService.java

@@ -149,14 +159,23 @@ public void createIndexToken(
        CreateServiceAccountTokenRequest request,
        ActionListener<CreateServiceAccountTokenResponse> listener
    ) {
+        if (indexServiceAccountTokenStore == null) {


There is no need to support other implementation.

👍

…_account_auth

ywangd

LGTM

The ReadOnlyServiceAccountTokenStore interface looks a bit weird to me. I think being readable is by default and as such it should be part of the base ServiceAccountTokenStore interface. The specialized interface should be mutable, i.e. index backed token store. Nevertheless, we can work on this separately.

ywangd · 2025-05-06T03:34:43Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+            // Completely handover service account token management to the extension if provided,
+            // this will disable the index managed
+            // service account tokens managed through the service account token API
+            components.add(new PluginComponentBinding<>(ReadOnlyServiceAccountTokenStore.class, extensionStore.get()));


Related this comment #126612 (comment), I suggest we also checks the SecurityExtension's name to ensure it is an internal one, i.e. not support 3rd party implementation.

ywangd · 2025-05-06T04:10:32Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -1250,6 +1247,56 @@ Collection<Object> createComponents(
        return components;
    }

+    private ServiceAccountService createServiceAccountService(


Can we add a test for the logic in this method?

Added testing for this. Thanks!

…_account_auth

jfreden · 2025-05-06T12:30:52Z

The ReadOnlyServiceAccountTokenStore interface looks a bit weird to me. I think being readable is by default and as such it should be part of the base ServiceAccountTokenStore interface. The specialized interface should be mutable, i.e. index backed token store. Nevertheless, we can work on this separately.

I agree so I changed this to be a NodeLocalServiceAccountTokenStore instead, that's only implemented by the FileServiceAccountTokenStore. When an internal extension has overridden the ServiceAccountTokenStore an implementation of NodeLocalServiceAccountTokenStore that fail all requests is used.

elasticsearchmachine added serverless-linked Added by automation, don't add manually v9.1.0 labels Apr 10, 2025

jfreden force-pushed the add_project_service_account_auth branch from ec8d56c to 98aa7e4 Compare April 23, 2025 09:47

Add Project Service Account Auth

3dec54b

jfreden force-pushed the add_project_service_account_auth branch from 98aa7e4 to 3dec54b Compare April 23, 2025 09:56

jfreden added 2 commits April 23, 2025 16:57

fixup! Token source

fcab6e2

fixup! Bug

0d027db

jfreden commented Apr 24, 2025

View reviewed changes

fixup!

6a64ba5

jfreden changed the title ~~Add per-project service account auth~~ Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions Apr 24, 2025

jfreden commented Apr 24, 2025

View reviewed changes

Merge remote-tracking branch 'upstream/main' into add_project_service…

cd95dc1

…_account_auth

jfreden added :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) >enhancement labels Apr 24, 2025

jfreden marked this pull request as ready for review April 24, 2025 12:41

jfreden requested review from ywangd and tvernum April 24, 2025 12:42

elasticsearchmachine added the Team:Security Meta label for security team label Apr 24, 2025

Update docs/changelog/126612.yaml

c679f7d

github-actions bot deployed to docs-preview April 24, 2025 12:42 View deployment

ywangd reviewed May 1, 2025

View reviewed changes

tvernum reviewed May 5, 2025

View reviewed changes

jfreden added 2 commits May 5, 2025 15:50

fixup! Code reiew comments

35e379e

Merge remote-tracking branch 'upstream/main' into add_project_service…

5340793

…_account_auth

github-actions bot deployed to docs-preview May 5, 2025 13:55 View deployment

fixup! Remov log

5628f9e

github-actions bot deployed to docs-preview May 5, 2025 14:01 View deployment

[CI] Auto commit changes from spotless

6ff864f

github-actions bot deployed to docs-preview May 5, 2025 14:09 View deployment

fixup! binding

cbe9cec

github-actions bot deployed to docs-preview May 5, 2025 14:23 View deployment

fixup! Test - change interface

91ab630

github-actions bot deployed to docs-preview May 5, 2025 14:45 View deployment

ywangd approved these changes May 6, 2025

View reviewed changes

fixup! Code review comments

4e2b7cf

github-actions bot deployed to docs-preview May 6, 2025 09:15 View deployment

fixup! Code review comments

9da5400

github-actions bot deployed to docs-preview May 6, 2025 09:18 View deployment

[CI] Auto commit changes from spotless

8dee42d

github-actions bot deployed to docs-preview May 6, 2025 09:26 View deployment

fixup! checkstyle

1375e1c

github-actions bot deployed to docs-preview May 6, 2025 09:41 View deployment

Merge remote-tracking branch 'upstream/main' into add_project_service…

cebaa8a

…_account_auth

github-actions bot deployed to docs-preview May 6, 2025 11:14 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612

Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612

jfreden commented Apr 10, 2025 •

edited

Loading

jfreden Apr 24, 2025

jfreden Apr 24, 2025

jfreden Apr 24, 2025

ywangd May 1, 2025

tvernum May 5, 2025

jfreden May 5, 2025

ywangd May 6, 2025

jfreden Apr 24, 2025

ywangd May 1, 2025

tvernum May 5, 2025

jfreden May 5, 2025

ywangd May 6, 2025

jfreden May 6, 2025

elasticsearchmachine commented Apr 24, 2025

elasticsearchmachine commented Apr 24, 2025

ywangd left a comment

ywangd May 1, 2025

tvernum May 5, 2025

ywangd May 1, 2025

ywangd May 1, 2025

tvernum May 5, 2025

ywangd May 1, 2025

tvernum commented May 5, 2025

tvernum left a comment

tvernum May 5, 2025

tvernum May 5, 2025

tvernum May 5, 2025

tvernum May 5, 2025

ywangd left a comment

ywangd May 6, 2025

ywangd May 6, 2025

jfreden May 6, 2025

jfreden commented May 6, 2025

Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612

Are you sure you want to change the base?

Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612

Conversation

jfreden commented Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 24, 2025

elasticsearchmachine commented Apr 24, 2025

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum commented May 5, 2025

tvernum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfreden commented May 6, 2025

jfreden commented Apr 10, 2025 •

edited

Loading