Service Accounts - Authentication with file tokens #70543
Conversation
ywangd
added
>enhancement
:Security/Authentication
|
Pinging @elastic/es-security (Team:Security) |
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine update branch |
| boolean authenticate(ServiceAccountToken token); | ||
| void authenticate(ServiceAccountToken token, ActionListener<Boolean> listener); |
There was a problem hiding this comment.
If the service token gets more sophisticated in the future, e.g. metadata etc, we may need to a listener that can take rich object. But for now, Boolean is sufficent.
|
@elasticmachine update branch |
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| public abstract class CachingServiceAccountsTokenStore implements ServiceAccountsTokenStore, CacheInvalidatorRegistry.CacheInvalidator { |
There was a problem hiding this comment.
It is possible to consolidate this new class, CachingUsernamePasswordRealm and the auth cache of ApiKeyService into something like CachingCredentialStore. I can raise an issue and deal with it separately.
| } | ||
| }, e -> { | ||
| logger.debug(new ParameterizedMessage("Failed to validate service account token for request [{}]", request), e); | ||
| listener.onFailure(request.exceptionProcessingRequest(e, null)); |
There was a problem hiding this comment.
I think we need to revisit the null here - we should be auditing with an AuthenticationToken since we have a credential.
There was a problem hiding this comment.
Good point. I made ServiceAccountToken implement AuthenticationToken and used it here. For this purpose, I also altered (reverted) the code to check for the token in AuthenticationService and this is actually what your poc code was suggesting.
| import java.util.concurrent.ExecutionException; | ||
| import java.util.concurrent.atomic.AtomicBoolean; | ||
|
|
||
| public abstract class CachingServiceAccountsTokenStore implements ServiceAccountsTokenStore, CacheInvalidatorRegistry.CacheInvalidator { |
There was a problem hiding this comment.
I wonder if this would be better using composition rather than inheritance.
Then we can have
new CachingTokenStore(new CompositeTokenStore( new FileTokenStore(), new IndexTokenStore() ) )
And have a single cache across all stores.
There was a problem hiding this comment.
Having a single cache for all stores is something I tried to avoid. For example, the invalidateAll will invalidate all entries even when only one of the store needs it. Also if the same key exists in both stores, in a bad case, they could evict each other and cause cache thrashing. We could solve all these by possibly prefix the key with the store type. But I wonder what is the better tradeoff?
There was a problem hiding this comment.
@tvernum Rest of the PR is updated to address your comment. I didn't touch this part though. Let me know if my above comment makes sense. Thanks!
...curity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountToken.java
Outdated
Show resolved
Hide resolved
…ecurity/authc/service/ServiceAccountToken.java Co-authored-by: Tim Vernum <[email protected]>
|
@tvernum I updated the PR based on our discussion about the token's format and serialisation. I removed the token type handling in |
| * <li>The {@link #getSecret() secret credential} for that token</li> | ||
| * </ol> | ||
| */ | ||
| public class ServiceAccountToken implements AuthenticationToken, Closeable { |
There was a problem hiding this comment.
Out of interest, why is this in core? I can't see any reason why it needs to be.
There was a problem hiding this comment.
No it does not need to be. It was a mistake. I noticed UsernamePasswordToken is in the core package and assumed that was the place to go. But now I realised all other token types are in fact in the security package. I moved this class and ServiceAccount back to security.
...e/src/main/java/org/elasticsearch/xpack/core/security/authc/service/ServiceAccountToken.java
Outdated
Show resolved
Hide resolved
...e/src/main/java/org/elasticsearch/xpack/core/security/authc/service/ServiceAccountToken.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| public static ServiceAccountToken newToken(ServiceAccountId accountId, String tokenName) { | ||
| return new ServiceAccountToken(accountId, tokenName, UUIDs.randomBase64UUIDSecureString()); |
There was a problem hiding this comment.
Should we validate the tokenName here?
There was a problem hiding this comment.
It is validated by the caller. But your comment prompted me to do some more thinkings. I now moved the validation logic inside the constructor so it is always triggered.
As a side effect, this means when we try parse a service account token from bearer string, an invalid token name will cause the parse to fail and continue down to authentication chain. Before this change, the behaviour was authentication would fail (because there would be no match for the qualified token name) and authentication chain would stop. I think this behaviour change is OK and actually more consistent. Because we also have validation logic inside ServiceAccountId which would behave the same as the new behaviour. In general, I think parsing failure should include anything that is more of a format rule and not business specific. An differentiating example is the service accout namespace, currently it only supports elastic. But it is a business rule, not a formatting requirement.
| final String tokenName3 = randomInvalidTokenName(); | ||
| assertThat(ServiceAccountToken.isValidTokenName(tokenName3), is(false)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Do we not also need tests for building/parsing the bearer token?
There was a problem hiding this comment.
I have those in ServiceAccountServiceTests. But I agree we could use more tests. So I added them to both here and ServiceAccountServiceTests
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine update branch |
This the 3rd PR for service accounts. It adds support for authentication with file tokens. It also adds a cache for performance so that expensive pbkdf2 hashing does not have to be performed on every request. Adding a cache comes with its own housekeeping work around invalidation. This PR ensures that cache gets invalidated when underlying token file is changed. It does not implement APIs for active invalidation. It will be handled in a separate PR after the API token is in place.
* Service Accounts - Initial bootstrap plumbing for essential classes (#70391) This PR is the initial effort to add essential classes for service accounts to lay down the foundation of future works. The classes are wired in places, but not yet been used. Also intentionally left out the actual credential store implementation. It is a good first commit which does not bring in too many changes. * Service Accounts - New CLI tool for managing file tokens (#70454) This is the second PR for service accounts. It adds a new CLI tool elasticsearch-service-tokens to manage file tokens. The file tokens are stored in the service_tokens file under the config directory. Out of the planned create, remove and list sub-commands, this PR only implements the create function since it is the most important one. The other two sub-commands will be handled in separate PRs. * Service Accounts - Authentication with file tokens (#70543) This the 3rd PR for service accounts. It adds support for authentication with file tokens. It also adds a cache for performance so that expensive pbkdf2 hashing does not have to be performed on every request. Adding a cache comes with its own housekeeping work around invalidation. This PR ensures that cache gets invalidated when underlying token file is changed. It does not implement APIs for active invalidation. It will be handled in a separate PR after the API token is in place. * [Test] Service Account - fix test assumption * [Test] Service Accounts - handle token names with leading hyphen (#70983) The CLI tool needs an option terminator (--) for another option names that begin with a hyphen. Otherwise it errors out with message of "not recognized option". The service account token name can begin with a hyphen. Hence we need to use -- when it is the case. An example of equivalent command line is ./bin/elasticsearch-service-tokens create elastic/fleet -- -lead-with-hyphen. * Service Accounts - Fleet integration (#70724) This PR implements rest of the pieces needed for Fleet integration, including: * Get service account role descriptor for authorization * API for creating service account token and storing in the security index * API for list tokens for a service account * New named privilege for manage service account * Mandate HTTP TLS for both service account auth and service account related APIs * Tests for API key related operations using service account * [Test] Service Accounts - Remove colon from invalid token name generator (#71099) The colon character is interpreted as the separate between token name and token secret. So if a token name contains a colon, it is in theory invalid. But the parser takes only the part before the colon as the token name and thus consider it as a valid token name. Subsequent authentication will still fail. But for tests, this generates a different exception and fails the expectation. This PR removes the colon char from being used to generate invalid token names for simplicity. * Fix for 7.x quirks
This the 3rd PR for service accounts. It adds support for authentication with file tokens. It also adds a cache for performance so that expensive pbkdf2 hashing does not have to be performed on every request. Adding a cache comes with its own housekeeping work around invalidation. This PR ensures that cache gets invalidated when underlying token file is changed. It does not implement APIs for active invalidation. I plan to have them as a separate PR (after the API token is in place).