Skip to content

SAML logout implicitly invalidate the associated refresh token #40523

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

Open
albertzaharovits opened this issue Mar 27, 2019 · 3 comments
Open
Assignees
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team

Comments

@albertzaharovits
Copy link
Contributor

Currently, for the SAML logout action, the access_token is required and it is then invalidated. The associated refresh_token is an optional parameter. If present, it will be invalidated as well.

I propose we implicitly invalidate the associated refresh_token and remove this parameter. I believe the simple case of not invalidating the refresh_token during logout is trappy.
WDYT? @jkakavas @tvernum

@albertzaharovits albertzaharovits added >enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Mar 27, 2019
@albertzaharovits albertzaharovits self-assigned this Mar 27, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@albertzaharovits albertzaharovits changed the title SAML logout implicitly invalidate the assocaited refresh token SAML logout implicitly invalidate the associated refresh token Mar 27, 2019
@jkakavas
Copy link
Member

Adding the link to a similar discussion we recently had for more context: #38475 (review)

I'll add some other questions that we can handle in the same effort ( we can very well decide them to be out of scope) :

  • Should we implicitly invalidate the associated access token when the refresh token is invalidated ?
  • Should we implicitly invalidate the associated refresh token when the access token is invalidated ?
  • Should realm logout be a special case for the above ?

My view is

  • Should we implicitly invalidate the associated access token when the refresh token is invalidated ?

Yes, let's follow the RFC in that. I'd prefer to do this by adding a parameter in the REST API with a default value to true, than handling it internally with no escape hatch.

  • Should we implicitly invalidate the associated refresh token when the access token is invalidated ?

I'm less inclined to do this (MAY in the RFC). This is also connected to how I feel about

  • Should realm logout be a special case for the above ?

Regardless of what we do above, I think that having an invalidateTokens() that is not necessarily exposed in the Token API but can be called by realms logout is a very good idea.

One final comment regarding the RFC, is that as it is also stated in the RFC itself, the revocation procedure should satisfy the functionality needs, the security policy and the threat model of the system and the specifics of implementation, so we can take its SHOULDs with a grain of MAY.

@tvernum
Copy link
Contributor

tvernum commented Apr 1, 2019

I propose we implicitly invalidate the associated refresh_token and remove this parameter.

I agree. When I wrote the API before Kibana was ready to use it, I didn't want to say You have to have a refresh_token to call this, because that could make it impossible to call the API in some circumstances.
But always invalidating the associated refresh token makes sense.

@rjernst rjernst added the Team:Security Meta label for security team label May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) Team:Security Meta label for security team
Projects
None yet
Development

No branches or pull requests

5 participants