Skip to content
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

GET API Key API filter out expired and/or invalidated API keys #97995

Closed
albertzaharovits opened this issue Jul 27, 2023 · 7 comments · Fixed by #98259
Closed

GET API Key API filter out expired and/or invalidated API keys #97995

albertzaharovits opened this issue Jul 27, 2023 · 7 comments · Fixed by #98259
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team

Comments

@albertzaharovits
Copy link
Contributor

Description

The Get API key information API can currently filter the returned keys by:

  • key id/name
  • keys owned by the calling user
  • keys owned by other users, by realm_name and username

Irrespective of the filtering used, both invalidated and expired (but not yet deleted) keys will be included in the response.
In the response, keys have the invalidated boolean property, as well as the expiration property that contains the UNIX timestamp past of which the key is no longer valid for authentication.

I propose we introduce a way for the API to support retrieving:

  • only valid API keys (not return invalidated keys) - using a boolean request parameter
  • keys not expired before a certain timestamp - using a timestamp parameter that defaults to "now" - also note that keys' experiation date is always checked against the local node's clock
@albertzaharovits albertzaharovits added >enhancement :Security/Security Security issues without another label labels Jul 27, 2023
@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Jul 27, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@albertzaharovits
Copy link
Contributor Author

CC @shasts @lloydmeta

@n1v0lg
Copy link
Contributor

n1v0lg commented Jul 27, 2023

Overall in favor of this -- I think it's a very natural extension to the API.

qq @shasts @lloydmeta for your concrete use case: do you care about distinguishing between expired vs. invalidated API keys? To me a good abstraction here would be to filter out "inactive" API keys, i.e., add a flag like include_active_only (naming TBD) that would return API keys that are neither invalidated, nor expired.

@albertzaharovits
Copy link
Contributor Author

We discussed it and the team is broadly in favor of it.
One aspect that was brought up in the conv, which is what Nikolaj is referring to in the above, is whether we need to expose the separation between expired and invalidated keys in the APIs interface.

Other questions of the same type, that can help us decide the interface change to the API:

  • do we foresee the API being used to show only invalidated or only expired keys?
  • do we foresee the API being used to show API keys that will expire at some specific time in the future/past?

The include_active_only boolean parameter is neat and simple, my only worry is that it can become unclear when used in conjuction, if we also end up with parameters for invalidated and/or expiration in the future.

@lloydmeta
Copy link
Member

Good question; I don't think we really care about distinguishing between expired vs invalidated, so something like include_active_only makes sense to me.

For the most part, we've switched over to the Query API keys API, and IMO can rely on that if we find really need to make a distinction.

@n1v0lg
Copy link
Contributor

n1v0lg commented Jul 28, 2023

Thanks @lloydmeta -- that was my thinking as well: the Query API is suited for more granular queries -- the Get API gives you more basic, abstracted functionality. FWIW, even if we decide to later add individual filter flags, we could add validation to ensure that include_active_only isn't used in bogus combination with these (e.g., include_active_only=True&invalidated=True or similar) or just address this in docs.

I don't think we will need this though -- to me the important bit is giving users the ability to fetch active API keys; for further drilling down on the data they can leverage the Query API. There would be some complexity in adding support for expiration in particular: to stay backwards compatible we would need to exclude this filter so we can't simply default to now for it. Nothing too challenging, I just prefer the simplicity of include_active_only here.

@albertzaharovits
Copy link
Contributor Author

include_active_only ++

elasticsearchmachine pushed a commit that referenced this issue Aug 9, 2023
This PR adds a flag `active_only` to the [Get API key
API](https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-api-key.html)
to optionally retrieve active-only (i.e., neither expired nor
invalidated) API keys. The flag defaults to false, i.e., by default,
expired and invalidate API keys **are** included in the response
(therefore we keep BWC). 

An example API call is:

```
GET /_security/api_key?active_only=true
```

The `active_only` flag can be used with realm and username based flags
(e.g., `active_only=true&username=X`) as well as `name` and `id`. In the
case it's used with `id` for an existing but non-active key the response
code is 404. This is consistent with the `owner` flag behavior.

Closes #97995
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants