Service Accounts - Initial bootstrap plumbing to add essential classes #70391

ywangd · 2021-03-15T13:29:29Z

This PR is the initial effort to add essential classes for service accounts. The classes are wired in places, but not yet been used. I also intentionally left out the actual credential store implementation. The changes are largely based on the PoC code with quite a few renames and rearragements. I think this is a good first commit which does not bring in too many changes.

ToDo:

Tests for the new classes

elasticmachine · 2021-03-15T13:29:32Z

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

ywangd · 2021-03-15T22:59:32Z

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

+        if (ElasticServiceAccounts.NAMESPACE.equals(token.getAccountId().namespace()) == false) {
+            logger.debug("only [{}] service accounts are supported, but received [{}]",
+                ElasticServiceAccounts.NAMESPACE, token.getAccountId().asPrincipal());
+            listener.onResponse(null);
+            return;
+        }
+
+        final ServiceAccount account = ACCOUNTS.get(token.getAccountId().serviceName());
+        if (account == null) {
+            logger.debug("the [{}] service account does not exist", token.getAccountId().asPrincipal());
+            listener.onFailure(null);
+            return;
+        }
+
+        if (serviceAccountsCredentialStore.authenticate(token)) {
+            listener.onResponse(success(account, token, nodeName));
+        } else {
+            final ParameterizedMessage message = new ParameterizedMessage(
+                "failed to authenticate service account [{}] with token name []",
+                token.getAccountId().asPrincipal(),
+                token.getTokenName());
+            logger.debug(message);
+            throw new ElasticsearchSecurityException(message.getFormattedMessage());
+        }


This part will be improved when we refactor the existing AuthenticationResult to be either generic or contain an Authentication object. But for now, I am throwing an exception to signal the termination of the authentication chain and null to mean continuation.
I think we can choose to not fallthrough (continue) in any case once a service account is extracted from the header. We touched on this in previous discussion, but I am not entirely sure now what we agreed upon. Fallthrough is the existing default behaviour, but we probably should be less lenient when it comes to service account. Please let me know your preference.

@tvernum ^^^

I think throwing an exception & terminating is the right approach.
The PoC code was originally written to handle API key auth, in which case there was an argument to be slightly more lenient because there was less guarantee that the header was intended to represent a service account.

Here, once we've deserialized a service account token correctly, I think it's safe to say that the client was intending to authenticate with that token and if that authc fails then the request fails too.

tvernum

LGTM

tvernum · 2021-03-16T02:07:40Z

...ity/src/main/java/org/elasticsearch/xpack/security/authc/service/ElasticServiceAccounts.java

+            null,
+            null,
+            null
+        ));


I think this descriptor is good for now, but we'll need to check it with the Fleet team later on.
It's not quite identical to the fleet_enroll role that that Fleet creates today, and we'll want to work through the differences.

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

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

tvernum · 2021-03-16T02:16:31Z

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

+                token.getAccountId().asPrincipal(),
+                token.getTokenName());
+            logger.debug(message);
+            throw new ElasticsearchSecurityException(message.getFormattedMessage());


Despite what I say about throwing below, I think this should be onFailure(new ElasticsearchSecurityException(..)) instead.

I thought about that, but it seems we mostly use onFailure for system related issues, e.g. security index is not available etc. Also because I plan to refactor this later to return an updated AuthenticationResult which uses terminate instead of onFailure for credential mismatch. So overall I think throwing is ok here and all throwing will become AuthenticationResult#terminate in future. Does it make sense?

mostly use onFailure for system related issues

That's not intentional. onFailure is for when exceptions happen in async code.

So once serviceAccountsCredentialStore.authenticate becomes asynchronous, this will need to be onFailure because it's too late to throw an exception - control has passed from the original thread to the callback.

In general, it is preferable for code that takes a listener argument to signal all outcomes via onFailure or onResponse and never throw or return

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

tvernum · 2021-03-16T02:19:13Z

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

+        final User user = account.asUser();
+        final Authentication.RealmRef authenticatedBy = new Authentication.RealmRef(REALM_NAME, REALM_TYPE, nodeName);
+        return new Authentication(user, authenticatedBy, null, Version.CURRENT, Authentication.AuthenticationType.TOKEN,
+            Map.of("_token_name", token.getTokenName()));


Why the leading _?

Not strictly necessary since we control service account entirely, but I think it is still better to use a reserved (with the leading _) name. It could be helpful on the client side for clarity.

...curity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountToken.java

…tstrap

…ecurity/authc/service/ServiceAccountToken.java Co-authored-by: Tim Vernum <[email protected]>

ywangd · 2021-03-16T02:43:10Z

@tvernum Tests are added. Also addressed your comments. For throwing vs onFailure, please see my comment here. Please let me know if you are ok with it. Or we can always come back to change it later.

I'll be merging the PR once tests pass. They are currently failing for unrelated maven repo issue.

...curity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountToken.java

tvernum · 2021-03-16T02:49:40Z

For throwing vs onFailure, please see my comment here. Please let me know if you are ok with it. Or we can always come back to change it later.

We will need to change it once the code starts doing async work - but we can leave it until then.

ywangd · 2021-03-16T02:58:26Z

For throwing vs onFailure, please see my comment here. Please let me know if you are ok with it. Or we can always come back to change it later.

We will need to change it once the code starts doing async work - but we can leave it until then.

This is a good point. I was hoping to use listener.onResponse(terminate) when refactor happens. But the async work is needed earlier than the refactor. So it makes sense to use onFailure first. I updated the code to always use onFailure for errors.

ywangd · 2021-03-16T09:19:13Z

@elasticmachine run elasticsearch-ci/1 elasticsearch-ci/2 elasticsearch-ci/eql-correctness elasticsearch-ci/packaging-sample-windows

ywangd · 2021-03-16T09:25:01Z

@elasticmachine run elasticsearch-ci/2 run elasticsearch-ci/eql-correctness run elasticsearch-ci/packaging-sample-windows

…lastic#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 - 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

Initial bootstrap plumbing for adding essential classes.

db9cc00

ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.13.0 labels Mar 15, 2021

ywangd requested a review from tvernum March 15, 2021 13:29

elasticmachine added the Team:Security Meta label for security team label Mar 15, 2021

Fix test java compilation

f55d78f

ywangd commented Mar 15, 2021

View reviewed changes

Add tests for new classes

778e3ed

tvernum approved these changes Mar 16, 2021

View reviewed changes

ywangd and others added 4 commits March 16, 2021 13:26

Always throw error on auth failure

24af378

Merge remote-tracking branch 'origin/master' into service-account-boo…

770ce5e

…tstrap

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

0bde7bd

…ecurity/authc/service/ServiceAccountToken.java Co-authored-by: Tim Vernum <[email protected]>

address feedback

7ea2f27

tvernum reviewed Mar 16, 2021

View reviewed changes

...curity/src/main/java/org/elasticsearch/xpack/security/authc/service/ServiceAccountToken.java Outdated Show resolved Hide resolved

Use onFailure to replace throw

95bc249

ywangd merged commit 143202a into elastic:master Mar 16, 2021

ywangd added the backport pending label Mar 16, 2021

ywangd mentioned this pull request Apr 8, 2021

Service Accounts - features required for Fleet integration #71514

Merged

ywangd removed the backport pending label Apr 9, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Service Accounts - Initial bootstrap plumbing to add essential classes #70391

Service Accounts - Initial bootstrap plumbing to add essential classes #70391

ywangd commented Mar 15, 2021 •

edited

Loading

elasticmachine commented Mar 15, 2021

ywangd Mar 15, 2021

ywangd Mar 15, 2021

tvernum Mar 16, 2021 •

edited

Loading

tvernum left a comment

tvernum Mar 16, 2021

tvernum Mar 16, 2021

ywangd Mar 16, 2021

tvernum Mar 16, 2021

tvernum Mar 16, 2021

ywangd Mar 16, 2021

ywangd commented Mar 16, 2021

tvernum commented Mar 16, 2021

ywangd commented Mar 16, 2021

ywangd commented Mar 16, 2021

ywangd commented Mar 16, 2021

Service Accounts - Initial bootstrap plumbing to add essential classes #70391

Service Accounts - Initial bootstrap plumbing to add essential classes #70391

Conversation

ywangd commented Mar 15, 2021 • edited Loading

elasticmachine commented Mar 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Mar 16, 2021 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd commented Mar 16, 2021

tvernum commented Mar 16, 2021

ywangd commented Mar 16, 2021

ywangd commented Mar 16, 2021

ywangd commented Mar 16, 2021

ywangd commented Mar 15, 2021 •

edited

Loading

tvernum Mar 16, 2021 •

edited

Loading