Service Accounts - cache clear API #71605
Conversation
ywangd
added
>enhancement
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
| if (tokenNames.length == 0) { | ||
| // This is the wildcard case for tokenNames | ||
| req.keys(namespace + "/" + service + "/"); | ||
| } else { | ||
| req.keys(Arrays.stream(tokenNames).map(name -> namespace + "/" + service + "/" + name).toArray(String[]::new)); | ||
| } |
There was a problem hiding this comment.
This cache clearing URL is different from all the other ones in that the cache key is comprised of three parts, i.e. {namespace}/{service}/{name}. This makes it challenging to conform 100% to existing behaviours. For an exmple, it is not possible to use a single * for clearing everything, it would be more like /_security/service/*/*/credential/token/*/_clear_cache. This is doable but I think it is not really needed for now as it adds complexity for the key comparison logic, e.g. should namespace also support comma separated list?
So for now, I am keep this simple by taking the namespace and service fields literally and only allow * and comma separated list for the token name field. Since we only have a single service account now, this approach can still clear all caches with /_security/service/elastic/fleet-server/credential/token/*/_clear_cache. We should revisit this once we need to add another service account.
There was a problem hiding this comment.
I think this is fine.
I don't think it really makes sense to clear all tokens for all accounts (but not other credential types).
If we get to the point where we think something like this is needed, then maybe we actually want
POST /_security/service/_clear_cache
to clear all caches for all service accounts.
Alternatively, for App Privileges we only support clearing a whole application (not a specific privilege)
We could decide to do something that here if we wanted, and only support clearing by account, not token.
There was a problem hiding this comment.
Cool. I'll leave it as is then. Always clearing all tokens of a service account can provide some simplication, but not a lot. Since I already have things in place, I'd just keep them. Plus even with the clearing applicaiton privileges cache API, the URL format is something like .../{application}/_clear_cache which maps better to .../{token_name}/_clear_cache.
| if (false == success) { | ||
| // Do not cache failed attempt | ||
| cache.invalidate(token.getQualifiedName(), listenableCacheEntry); | ||
| } else { | ||
| logger.trace("cache service token [{}] authentication result", token.getQualifiedName()); | ||
| } |
There was a problem hiding this comment.
I made this change so the logic is mostly the same as the one of CachingUsernamePasswordRealm. Basically the cache now does not cache negative results. I think this is a better choice since service account will be well known and it is easy to cause cache thrashing if negative results are cached. This is probably the similar argument why CachingUsernamePasswordRealm does not cache negative results as well. Note that ApiKeyService cache is different in that it does cache negative results. But it can afford to do that because the ApiKey ID, unlike username/service account, is not well known.
| if (tokenNames.length == 0) { | ||
| // This is the wildcard case for tokenNames | ||
| req.keys(namespace + "/" + service + "/"); | ||
| } else { | ||
| req.keys(Arrays.stream(tokenNames).map(name -> namespace + "/" + service + "/" + name).toArray(String[]::new)); | ||
| } |
There was a problem hiding this comment.
I think this is fine.
I don't think it really makes sense to clear all tokens for all accounts (but not other credential types).
If we get to the point where we think something like this is needed, then maybe we actually want
POST /_security/service/_clear_cache
to clear all caches for all service accounts.
Alternatively, for App Privileges we only support clearing a whole application (not a specific privilege)
We could decide to do something that here if we wanted, and only support clearing by account, not token.
...in/java/org/elasticsearch/xpack/security/authc/service/CachingServiceAccountsTokenStore.java
Show resolved
Hide resolved
|
@tvernum The PR is updated to handle invalidate token names at the rest layer. It is now ready for another round. Thanks! |
This PR adds a new Rest endpoint to clear caches used by service account authentication.
This PR adds a new Rest endpoint to clear caches used by service account authentication.