Integrate api keys with the authentication service #35555
Conversation
This commit integrates usage of the api key service into the authentication service for verification of api keys. A bug was fixed in the validation of api keys where the structure of the document was not being used properly. Additionally, unit tests have been added for authentication with api keys.
jaymode
added
>non-issue
:Security/Authentication
|
Pinging @elastic/es-security |
bizybot
left a comment
bizybot
left a comment
There was a problem hiding this comment.
The overall changes LGTM, I have a comment regarding termination on the expired API key.
| threadContext.putHeader("Authorization", headerValue); | ||
| ElasticsearchSecurityException e = expectThrows(ElasticsearchSecurityException.class, | ||
| () -> authenticateBlocking("_action", message, null)); | ||
| assertThat(e.getMessage(), containsString("missing authentication token")); |
There was a problem hiding this comment.
I think this should be Authentication using apikey failed - api key is expired.
The reason this test passed is since we do not terminate authentication chain when encountered with expired API key.
I think we should terminate if expired API key is presented, no other realms should attempt to authenticate if we know that it has expired, wdyt?
There was a problem hiding this comment.
I think @bizybot is correct - we probably need some of the failure cases in ApiKeyService.authenticateWithApiKeyIfPresent to have a terminate status instead of continue.
If I'm passing an API Key over TLS, then it would be very strange (and hard to debug) if the authentication use the API Key right up until it expired and then suddenly switched to PKI auth.
5 participants
This commit integrates usage of the api key service into the
authentication service for verification of api keys. A bug was fixed in
the validation of api keys where the structure of the document was not
being used properly. Additionally, unit tests have been added for
authentication with api keys.