-
Notifications
You must be signed in to change notification settings - Fork 25.3k
Add Support for Providing a custom ServiceAccountTokenStore through SecurityExtensions #126612
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
Changes from 3 commits
3dec54b
fcab6e2
0d027db
6a64ba5
cd95dc1
c679f7d
35e379e
5340793
5628f9e
6ff864f
cbe9cec
91ab630
4e2b7cf
9da5400
8dee42d
1375e1c
cebaa8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -105,6 +105,6 @@ public int compareTo(TokenInfo o) { | |
|
||
public enum TokenSource { | ||
INDEX, | ||
FILE; | ||
FILE | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -208,6 +208,7 @@ | |
import org.elasticsearch.xpack.core.security.authc.RealmConfig; | ||
import org.elasticsearch.xpack.core.security.authc.RealmSettings; | ||
import org.elasticsearch.xpack.core.security.authc.Subject; | ||
import org.elasticsearch.xpack.core.security.authc.service.ServiceAccountTokenStore; | ||
import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper; | ||
import org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken; | ||
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine; | ||
|
@@ -310,6 +311,7 @@ | |
import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; | ||
import org.elasticsearch.xpack.security.authc.jwt.JwtRealm; | ||
import org.elasticsearch.xpack.security.authc.service.CachingServiceAccountTokenStore; | ||
import org.elasticsearch.xpack.security.authc.service.CompositeServiceAccountTokenStore; | ||
import org.elasticsearch.xpack.security.authc.service.FileServiceAccountTokenStore; | ||
import org.elasticsearch.xpack.security.authc.service.IndexServiceAccountTokenStore; | ||
import org.elasticsearch.xpack.security.authc.service.ServiceAccountService; | ||
|
@@ -915,12 +917,54 @@ Collection<Object> createComponents( | |
this.realms.set(realms); | ||
|
||
systemIndices.getMainIndexManager().addStateListener(nativeRoleMappingStore::onSecurityIndexStateChange); | ||
|
||
final CacheInvalidatorRegistry cacheInvalidatorRegistry = new CacheInvalidatorRegistry(); | ||
cacheInvalidatorRegistry.registerAlias("service", Set.of("file_service_account_token", "index_service_account_token")); | ||
components.add(cacheInvalidatorRegistry); | ||
systemIndices.getMainIndexManager().addStateListener(cacheInvalidatorRegistry::onSecurityIndexStateChange); | ||
|
||
final IndexServiceAccountTokenStore indexServiceAccountTokenStore = new IndexServiceAccountTokenStore( | ||
settings, | ||
threadPool, | ||
getClock(), | ||
client, | ||
systemIndices.getMainIndexManager(), | ||
clusterService, | ||
cacheInvalidatorRegistry | ||
); | ||
components.add(indexServiceAccountTokenStore); | ||
|
||
final FileServiceAccountTokenStore fileServiceAccountTokenStore = new FileServiceAccountTokenStore( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's techinically possible to not create There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Doing that right sounds like a multi-line / multi-file change, so perhaps we can revisit it in a future PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
environment, | ||
resourceWatcherService, | ||
threadPool, | ||
clusterService, | ||
cacheInvalidatorRegistry | ||
); | ||
components.add(fileServiceAccountTokenStore); | ||
cacheInvalidatorRegistry.registerAlias("service", Set.of("file_service_account_token", "index_service_account_token")); | ||
|
||
List<ServiceAccountTokenStore> extensionTokenStores = securityExtensions.stream() | ||
.map(extension -> extension.getServiceAccountTokenStore(extensionComponents)) | ||
.filter(Objects::nonNull) | ||
.toList(); | ||
|
||
ServiceAccountService serviceAccountService; | ||
|
||
if (extensionTokenStores.isEmpty()) { | ||
serviceAccountService = new ServiceAccountService(client, fileServiceAccountTokenStore, indexServiceAccountTokenStore); | ||
} else { | ||
// 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 | ||
logger.debug("Service account authentication handled by extension, disabling file and index token stores"); | ||
components.addAll(extensionTokenStores); | ||
serviceAccountService = new ServiceAccountService( | ||
client, | ||
new CompositeServiceAccountTokenStore(extensionTokenStores, client.threadPool().getThreadContext()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer we ensure there is only a single extensionTokenStore and avoid the composite wrapping. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
); | ||
// TODO Should this also register with the cacheInvalidatorRegistry? | ||
} | ||
|
||
components.add(serviceAccountService); | ||
|
||
systemIndices.getMainIndexManager().addStateListener(cacheInvalidatorRegistry::onSecurityIndexStateChange); | ||
final NativePrivilegeStore privilegeStore = new NativePrivilegeStore( | ||
settings, | ||
client, | ||
|
@@ -1004,33 +1048,6 @@ Collection<Object> createComponents( | |
); | ||
components.add(apiKeyService); | ||
|
||
final IndexServiceAccountTokenStore indexServiceAccountTokenStore = new IndexServiceAccountTokenStore( | ||
settings, | ||
threadPool, | ||
getClock(), | ||
client, | ||
systemIndices.getMainIndexManager(), | ||
clusterService, | ||
cacheInvalidatorRegistry | ||
); | ||
components.add(indexServiceAccountTokenStore); | ||
|
||
final FileServiceAccountTokenStore fileServiceAccountTokenStore = new FileServiceAccountTokenStore( | ||
environment, | ||
resourceWatcherService, | ||
threadPool, | ||
clusterService, | ||
cacheInvalidatorRegistry | ||
); | ||
components.add(fileServiceAccountTokenStore); | ||
|
||
final ServiceAccountService serviceAccountService = new ServiceAccountService( | ||
client, | ||
fileServiceAccountTokenStore, | ||
indexServiceAccountTokenStore | ||
); | ||
components.add(serviceAccountService); | ||
|
||
final RoleProviders roleProviders = new RoleProviders( | ||
reservedRolesStore, | ||
fileRolesStore.get(), | ||
|
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.
These had to move since they're now part of the
SecurityExtension
interface throughServiceAccountTokenStore
.