Implement verification of API keys#35318
Conversation
This change implements the verification of api keys in the ApiKeyService. There is no integration into the AuthenticationService as part of this change; this will be done in a future change. Verification of an API key involves validating the provided key with the hash stored in the document and then ensuring that the token is not expired. A conscious decision has been made to always validate the hash and then check expiration. This is done to prevent leaking that a given key has expired.
jaymode
added
>non-issue
:Security/Authentication
|
Pinging @elastic/es-security |
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Show resolved
Hide resolved
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Show resolved
Hide resolved
tvernum
left a comment
tvernum
left a comment
There was a problem hiding this comment.
I have one question/concern, but otherwise LGTM.
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Show resolved
Hide resolved
| final User apiKeyUser = new User(principal, roleNames, null, null, metadata, true); | ||
| listener.onResponse(AuthenticationResult.success(apiKeyUser)); | ||
| } else { | ||
| listener.onResponse(AuthenticationResult.unsuccessful("api key is expired", null)); |
There was a problem hiding this comment.
. A conscious decision has been made to always validate the hash
and then check expiration. This is done to prevent leaking that a given
key has expired.
Can you please elaborate on your reasoning behind this? I don't necessarily see a value in masking whether the key is invalid or expired.
Also wouldn't the AuthenticationResult message leak that to the requester anyhow?
There was a problem hiding this comment.
Can you please elaborate on your reasoning behind this?
My reasoning is that an attacker can currently differentiate between a non-existent api key and an existing one based on timing, which allows the attacker to obtain the ID of a key that exists and then begin a brute force attack on a single ID. If they can also differentiate between one that exists and is expired, this helps them as well. Smart guesses/a vulnerability in our key generation process could wind up making the ability to find a non-expired ID useful to an attacker.
That said, if you still feel there is not much value in this I am happy to reconsider.
There was a problem hiding this comment.
Sorry, I missed the second question.
Also wouldn't the AuthenticationResult message leak that to the requester anyhow?
No, the authentication result message is used for logging and is not returned to the user.
There was a problem hiding this comment.
Can you please elaborate on your reasoning behind this?
That said, if you still feel there is not much value in this I am happy to reconsider.
Thanks for the clarification! No, I agree with you that this is a valuable protection layer, let's keep it !
x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
Outdated
Show resolved
Hide resolved
5 participants
This change implements the verification of api keys in the
ApiKeyService. There is no integration into the AuthenticationService
as part of this change; this will be done in a future change.
Verification of an API key involves validating the provided key with
the hash stored in the document and then ensuring that the token is not
expired. A conscious decision has been made to always validate the hash
and then check expiration. This is done to prevent leaking that a given
key has expired.