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

feat(auth): Implement SSO session expiry #29422

Merged
merged 8 commits into from
Nov 10, 2021
Merged

Conversation

JoshFerge
Copy link
Member

@JoshFerge JoshFerge commented Oct 19, 2021

#sync-getsentry

Problem

We'd like to have users reauthenticate with their SSO providers on a more frequent basis. Right now they simply expire with the django session timeout of two weeks. SSO sessions are stored in the django session as a list of comma-delimited org ids under the sso key.

Solution

  • Change the SSO sessions to be stored under separate keys, in the format sso_s:org_id. In the value of this, we'll store a dictionary (more on that later), with only one value currently, a utc timestamp of when the user last authenticated SSO.
  • When we verify the SSO, we'll also look at the auth_timestamp, and if this timestamp was further back than our expiry limit (20 hours), we'll return False which will force the user to re-authenticate for that organization.
  • The logic will currently look for the old key if the validation on the current one failed, as to not log out all of our SSO users at once. We'll remove this logic after two weeks, when all past values should be expired.

TODO:

  • confirm all tests pass with current session value
  • Change test fixtures to use new session values
  • create PR to remove deprecated logic, have ready to go for after two weeks
  • Test on staging, invite a few users to try out.

Future use cases:

  • We could invalidate SSO logins upon configuration changes by creating a nonce associated with a particular SSO config on the AuthProvider model. Each nonce would be stored in the SSO session dict and could be used to invalidate these sessions.
  • We could have it so that this expiration time value is configurable in the auth settings page, and /or use the SAML value SessionNotOnOrAfter

Footnotes

  • The stored value in the django cookie session will look like so:
{"sso_s:4": 1634683781.88369, "sso_s:2": 1634683791.690106}
  • Each org login will add ~30 bytes to the session cookie, which is about ~5-10% of what the value currently is. I don't think this is a problem, but if we end up adding more fields it could in theory be an issue. I'd like us to switch to the cache backend for our sessions, but this is out of scope for now.

sso.append(str(organization_id))
request.session[SSO_SESSION_KEY] = ",".join(sso)
key = sso_session_key_for_org_id(organization_id)
request.session[key] = {"auth_timestamp": datetime.now(tz=timezone.utc).timestamp()}
Copy link
Member Author

@JoshFerge JoshFerge Oct 20, 2021

Choose a reason for hiding this comment

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

could technically store it as a datetime object instead of the raw timestamp since this is pickled, but for size considerations think a raw float timestamp is better.

@dcramer
Copy link
Member

dcramer commented Oct 20, 2021

Aside one thing to remember about cookies is size matters. Its probably worth shrinking variable names in session keys/cookies as best as you can and defining constants to reference them.

Copy link
Contributor

@maxiuyuan maxiuyuan left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@RyanSkonnord RyanSkonnord left a comment

Choose a reason for hiding this comment

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

LGTM! Nicely factored, very easy to follow.

@JoshFerge
Copy link
Member Author

thanks for the reviews @RyanSkonnord @maxiuyuan. As I'm headed off for a 1.5 week vacation wednesday, i'm going to add "do not merge" and we can deploy when I get back. In the meantime will test on staging.

@JoshFerge JoshFerge added Deploy: Risky This is a particularly risky deploy Do Not Merge Don't merge labels Oct 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Deploy: Risky This is a particularly risky deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants