[Test] Service Accounts - Remove colon from invalid token name generator #71099
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ywangd
added
>test
|
Pinging @elastic/es-security (Team:Security) |
ywangd
commented
Mar 31, 2021
Comment on lines
+133
to
+136
| @Override | ||
| public String toString() { | ||
| return getQualifiedName(); | ||
| } |
There was a problem hiding this comment.
I am pretty sure we had this method previously but somehow it seemed to be lost in between rebases refactors, rebases and merges.
tvernum
reviewed
Mar 31, 2021
| @@ -33,7 +33,7 @@ | |||
| ); | |||
|
|
|||
| private static final Set<Character> INVALID_TOKEN_NAME_CHARS = Set.of( | |||
| '!', '"', '#', '$', '%', '&', '\'', '(', ')', '*', '+', ',', '.', '/', ':', ';', '<', '=', '>', '?', '@', '[', | |||
| '!', '"', '#', '$', '%', '&', '\'', '(', ')', '*', '+', ',', '.', '/', ';', '<', '=', '>', '?', '@', '[', | |||
| '\\', ']', '^', '`', '{', '|', '}', '~', ' ', '\t', '\n', '\r'); | |||
There was a problem hiding this comment.
Would a newline have a similar parsing issue in this test?
There was a problem hiding this comment.
Oh, I just realised it's not file parsing it's the new token format. So, to answer my question, "No, newline will fail in the expected way".
tvernum
approved these changes
Mar 31, 2021
ywangd
added a commit
to ywangd/elasticsearch
that referenced
this pull request
Apr 8, 2021
…tor (elastic#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.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Resolves: #71094