Skip to content

Service Accounts - New CLI tool for managing file tokens #70454

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

Merged
merged 9 commits into from
Mar 17, 2021

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Mar 16, 2021

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 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.

PS: There are some stub test methods in FileServiceAccountsTokenStoreTests, I will complete them before merging.

@ywangd ywangd added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v8.0.0 v7.13.0 labels Mar 16, 2021
@ywangd ywangd requested a review from tvernum March 16, 2021 12:55
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Mar 16, 2021
@elasticmachine
Copy link
Collaborator

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

@ywangd
Copy link
Member Author

ywangd commented Mar 17, 2021

Tests are in place. I also replace the usage of the term "credential" with "token", e.g. ServiceAccountsCredentialStore is now ServiceAccountsTokenStore. I think a consistent name helps reading and understanding, plus it truly is a bearer "token".

Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

}
}
}, Property.NodeScope);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised that we need so much boilerplate in order to add another hashing algorithm. Do we need an issue to refactor them down into some amount of shared code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can be part of #66840. The implication is 8.0 only, but I think it is fine.

final String principal = tuple.v1();
final String tokenName = tuple.v2();
if (false == ServiceAccountService.isServiceAccountPrincipal(principal)) {
throw new UserException(ExitCodes.NO_USER, "Unknown service account principal: [" + principal + "]");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be helpful to list the known principals. Otherwise I can see someone being confused when fleet doesn't work (should be elastic/fleet)

}
tokenHashes.put(token.getQualifiedName(), hasher.hash(token.getSecret()));
FileServiceAccountsTokenStore.writeFile(serviceTokensFile, tokenHashes);
terminal.println("SERVICE_TOKEN " + token.getQualifiedName() + " = " + token.asBearerString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a followup we should think about what this output should be, and what guarantees we can offer.
Orchestrators will need to parse the output, so it should be really precise.

This format might be exactly what we want, but we should validate that assumption.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the Fleet integration would test this output. But I'll make it explicit in communication when integration happens. The format itself follows the pattern we used for the setup-password tool.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cloud doesn't call setup-password though, so it's a reasonable assumption, but we need to test it.

@ywangd ywangd merged commit 994499d into elastic:master Mar 17, 2021
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Apr 8, 2021
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.
ywangd added a commit that referenced this pull request Apr 9, 2021
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team v7.13.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants