Skip to content

Allow IdP initiated SAML login with session containing expired token.#59686

Merged
azasypkin merged 3 commits intoelastic:masterfrom
azasypkin:issue-59629-saml-idp-login-expired
Apr 23, 2020
Merged

Allow IdP initiated SAML login with session containing expired token.#59686
azasypkin merged 3 commits intoelastic:masterfrom
azasypkin:issue-59629-saml-idp-login-expired

Conversation

@azasypkin
Copy link
Copy Markdown
Contributor

@azasypkin azasypkin commented Mar 9, 2020 •
edited
Loading

As in proposed in #59629 in this PR we're trying to leverage existing session during IdP initiated SAML login even if it contains expired access token.

Notable changes:

How to test:

  1. Log in with SAML (via IdP or User initiated flow, it doesn't matter)
  2. Delete all access tokens from Elasticsearch:
POST localhost:9200/.security-tokens/_delete_by_query?pretty&refresh=true
Authorization: Basic xxxx
Accept: application/json
Content-Type: application/json

{
  "query": {
    "match": {
      "doc_type": "token"
    }
  }
}
  1. Try to log in to Kibana via IdP initiated flow using the same or another user in the same browsing context (so that browser sends SAML Response with the existing cookie with deleted tokens)

Note: You may want to close browser tab (not window) after step 1 so that Kibana doesn't automatically log you out.

Blocked by: elastic/elasticsearch#53323
Fixes: #59629

"Release Note: previously user couldn't log in with SAML Identity Provider Initiated flow (e.g. from Okta Dashboard) if they already had an existing, but expired session. Now it should be possible."

@azasypkin azasypkin added blocked Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// Feature:Security/Authentication Platform Security - Authentication v7.7.0 labels Mar 9, 2020
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Mar 9, 2020

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

@azasypkin azasypkin force-pushed the issue-59629-saml-idp-login-expired branch from b6b7545 to ca014a7 Compare March 25, 2020 13:18
azasypkin
azasypkin commented Mar 25, 2020
View reviewed changes
x-pack/plugins/security/server/authentication/providers/saml.ts Outdated
Copy link
Copy Markdown
Contributor Author

@azasypkin azasypkin Mar 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: we can keep this if we decide to fix the issue without ES part in elastic/elasticsearch#53323.

@azasypkin azasypkin force-pushed the issue-59629-saml-idp-login-expired branch from ca014a7 to af4e956 Compare March 25, 2020 16:26
@azasypkin azasypkin removed the blocked label Apr 21, 2020
@azasypkin azasypkin force-pushed the issue-59629-saml-idp-login-expired branch from af4e956 to a32f60f Compare April 21, 2020 10:07
@azasypkin azasypkin marked this pull request as ready for review April 21, 2020 11:53
@azasypkin azasypkin requested a review from a team as a code owner April 21, 2020 11:53
@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Apr 21, 2020

@legrego @jportner heya, tagging you both for review, just whoever gets first to this PR 🙂 Not urgent, can wait. Thanks!

@azasypkin azasypkin requested review from jportner and legrego April 21, 2020 12:00
legrego
legrego approved these changes Apr 23, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Tested locally, works great.

x-pack/plugins/security/server/authentication/tokens.ts
this.logger.debug(`Failed to invalidate refresh token: ${err.message}`);
// We don't re-throw the error here to have a chance to invalidate access token if it's provided.
invalidationError = err;

Copy link
Copy Markdown
Member

@legrego legrego Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At some point (not now), it might make sense to consolidate the error handling logic for the access and refresh tokens, since they appear nearly identical. I fear these will accidentally diverge at some point, and we'll have another subtle auth bug on our hands

Copy link
Copy Markdown
Contributor Author

@azasypkin azasypkin Apr 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree, every time I change this code I stare at it and want to unify, but always defer for one reason or another 🙈

@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Apr 23, 2020

@elasticmachine merge upstream

@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Apr 23, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 23, 2020

💚 Build Succeeded

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@azasypkin azasypkin merged commit 95ac47d into elastic:master Apr 23, 2020
@azasypkin azasypkin deleted the issue-59629-saml-idp-login-expired branch April 23, 2020 17:15
@azasypkin azasypkin mentioned this pull request Apr 23, 2020
Merged
azasypkin added a commit to azasypkin/kibana that referenced this pull request Apr 23, 2020
@azasypkin
Allow IdP initiated SAML login with session containing expired token. (…
04ced83 
…elastic#59686)
@azasypkin azasypkin mentioned this pull request Apr 23, 2020
Merged
azasypkin added a commit to azasypkin/kibana that referenced this pull request Apr 23, 2020
@azasypkin
Allow IdP initiated SAML login with session containing expired token. (…
b46b112 
…elastic#59686)
@legrego legrego mentioned this pull request Apr 23, 2020
Closed
@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Apr 24, 2020 •
edited
Loading

7.x/7.8.0: a15a1e0
7.7/7.7.1: 7827fa5

@azasypkin azasypkin removed the request for review from jportner April 24, 2020 08:01
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 27, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 27, 2020
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 28, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 29, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@azasypkin
Copy link
Copy Markdown
Contributor Author

azasypkin commented Apr 29, 2020

Calm down, @kibanamachine, I know and told you the reason in #59686 (comment) already!

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 30, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

10 similar comments
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 1, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 4, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 5, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 6, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 7, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 8, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 11, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 12, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 13, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 14, 2020

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label May 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@legrego legrego legrego approved these changes

Assignees

No one assigned

Labels

backport pending Feature:Security/Authentication Platform Security - Authentication release_note:fix Team:Security Platform Security: Auth, Users, Roles, Spaces, Audit Logging, etc t// v7.7.1 v7.8.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@azasypkin @elasticmachine @kibanamachine @legrego