Add stricter validation for api key expiration time #103973
Conversation
jfreden
added
>enhancement
:Security/Security
|
Hi @jfreden, I've created a changelog YAML for you. |
jfreden
force-pushed
the
add-api-exp-restrictions
branch
from
a7e5cc8
to
2f3275b
Compare
|
Pinging @elastic/es-security (Team:Security) |
There was a problem hiding this comment.
Wow sorry I really let this one sit way too long. I have a suggestion around moving the validation logic to the request classes instead.
@jakelandis I wanted to get a sanity check on not treating this as a breaking change. I don't think it is, but it's good to call out and discuss explicitly:
Today, it's possible to specify a 0 or negative expiration time when creating an API key. This means the API key is invalid immediately. That really doesn't make any sense -- the only reason I see this happening is users misunderstanding expiration (e.g., maybe they assume -1 means no expiration?).
This PR adds validation that rejects bogus (0 or negative) expiration times. For the update API key APIs this is non-controversial since we're adding the validation in the same minor as we added the field.
We're also adding this check to the create API key API; there the field has been around for a long time. Like I said, I don't see a valid use case for supplying bogus values, and adding the validation is a small but nice improvement. So I'd say, let's move ahead with it and not treat is as a breaking change. Would you agree here?
Thanks!
| ); | ||
| if (workflowsValidationException != null) { | ||
| listener.onFailure(workflowsValidationException); | ||
| List<IllegalArgumentException> failedValidations = Stream.of( |
There was a problem hiding this comment.
I think this check makes more sense in the request classes (CreateApiKeyRequest and BaseUpdateApiKeyRequest) -- that way we fail early, and keep request validation in one-ish place. The reason validateWorkflowsRestrictionConstraints (and other validation) is here is because that validation requires access to things we only (or at least conveniently) have access to at the service level (e.g., transportVersion).
There was a problem hiding this comment.
Yes, good point. I've moved it. Thank you!
This is a breaking change. It might be one that goes primarily unnoticed, but it is still breaking. Introduction of this change would be subject to REST API compatibility. We could go through the process of breaking change (approval, deprecation, compatibility) but not sure if this one is worth the effort since given that due to our history, the bar is pretty high. |
|
Thanks @jakelandis and @n1v0lg ! So our options are:
I think I'm leaning towards (1). What do you think @jakelandis and @n1v0lg ? |
|
@jakelandis noted, thanks! My rationale for proposing this change is that it feels more like a bug fix -- essentially, the only thing you get out of the "BWC"-behavior is unusable API keys. However, this is subjective and strictly speaking this is for sure a breaking change. There may be an automated test pipeline in a customer's infrastructure that is creating bogus API keys (where it doesn't matter) but would suddenly break due to this change, or some other edge case I'm not thinking of. @jfreden my (weak) preference would be to go for (2) -- consistency is good, but the Create API key API behavior feels essentially like an oversight/bug so not necessarily worth inheriting. Just my 2 cents though, I don't feel strongly here! I definitely don't think it's worth going through the breaking changes process. |
|
I would also have (weak) preference for (2) assuming there are no BWC concerns. I can see testing usecase to create a pre-expired API key, but much harder to rationalize a usecase for pre-expiring while updating an API key. |
jfreden
force-pushed
the
add-api-exp-restrictions
branch
from
2f3275b
to
e2caf86
Compare
|
Thanks for the input @jakelandis and @n1v0lg ! I've changed it to (2). |
This is a follow up from: #103453