-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Reduce api key verification cost #112878
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
Reduce api key verification cost #112878
Conversation
| // the difference between 16 and 18 effectively results in the same "encoded" API Key that's sent in HTTP request headers, | ||
| // dues to base64 padding | ||
| final SecureString apiKey = getBase64SecureRandomString(request.getType() == ApiKey.Type.CROSS_CLUSTER ? 16 : 18); | ||
| assert ApiKey.Type.CROSS_CLUSTER != request.getType() || API_KEY_SECRET_LENGTH == apiKey.length() | ||
| : "Invalid API key (name=[" + request.getName() + "], type=[" + request.getType() + "], length=[" + apiKey.length() + "])"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @n1v0lg pointed out, we'd need to support the current length for cross cluster API keys.
Maybe other (external, i.e. non-ES) consumers rely on the "api_key" length not changing, but hopefully they use the "encoded" one in the API response, which doesn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g.:
{
"id" : "bZ0Y7JEBuvKKBRFbe4xD",
"name" : "test",
"api_key" : "BGEQu0kTIU8x7b63k9Np41dE",
"encoded" : "YlowWTdKRUJ1dktLQlJGYmU0eEQ6QkdFUXUwa1RJVTh4N2I2M2s5TnA0MWRF"
}
The encoded is still 60 chars, but api_key changes from 22 to 24.
| .map(Hasher::name) | ||
| .map(name -> name.toLowerCase(Locale.ROOT)) | ||
| .filter(name -> (name.startsWith("pbkdf2") || name.startsWith("bcrypt"))) | ||
| .filter(name -> (name.startsWith("pbkdf2") || name.startsWith("bcrypt") || "ssha256".equals(name))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be allowed only for API keys, not for passwords (api keys are random long secrets).
| public static final Setting<String> PASSWORD_HASHING_ALGORITHM = XPackSettings.defaultStoredHashAlgorithmSetting( | ||
| "xpack.security.authc.api_key.hashing.algorithm", | ||
| (s) -> Hasher.PBKDF2.name() | ||
| (s) -> Hasher.SSHA256.name() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is currently no PBKDF2 option with cost lower than 1000. If we add a new hasher type/cost we need to worry about BWC (old node not understanding keys generated by new nodes).
PoC