Add check for assertion expiration during the SAML auth flow #42915

miveronese · 2022-10-13T02:24:39Z

This PR is related to the investigation and work to support refresh tokens for different auth providers.

It sets an expiration date/time for sessions based on the value Conditions.NotOnOrAfter which can be present in a SAML response and informs when an assertion will expire.

Context:

Currently, we support refresh tokens for GL, and there’s work to be merged that will add support for GH.
But what about other auth providers? Do their tokens used in the auth flow expire? Can these tokens be refreshed? Can we support this?

While investigating if SAML uses refresh tokens and, in case it doesn’t, what can expire in an auth flow with SAML, I confirmed that:

SAML doesn’t do refresh tokens.
SAML assertions (the message that comes from the IdP to a service provider — SG is the service provider — and tells that a user is signed in) can indeed expire.
Users can optionally set SAML assertions expiration time. If a user doesn’t set that information, most IdPs set a default value for the assertion to be expired.
But Sourcegraph is not honoring that information. Meaning: if an assertion has an expiration date, we do nothing.
In practice, if a user logins into SG via SAML and the assertion that is sent from the IdP to SG has an expiration time of 60 minutes, instead of the user being logged out of SG after 60 minutes, we keep them logged in.
According to a note in the code, we do that on purpose to avoid a bad user experience.
The same note explains this decision based on the info that some assertions would expire after 60 seconds. But this information seems incorrect. I couldn’t find any IdP whose assertions expire after 60 secs. 60 minutes (Auth0, Azure AD for example) or 74 minutes (Ping Identity), are the most common default values.

I opened this PR based on the idea that we should respect the assertion expiration time.

Test plan

Unit tests updated and passed. Manually tested that, when an assertion expires, the user has to re-login.

miveronese · 2022-10-13T02:30:09Z

enterprise/cmd/frontend/internal/auth/saml/user_test.go

 unnormalizedUsername: "bob@example.com",
 displayName: "Bob Yang",
+expirationTime: "2018-05-20T17:13:04.795Z",
 }); !reflect.DeepEqual(info, want) {


Would love to add a test case for when Conditons.NotOnOrAfter is not present as this is an optional field. But I couldn't figure out how the certificates and config used in the existing tests were set so I could do the same... 🤔

I think it's just set in the testAuthnResponse at the bottom. Search for "NotOnOrAfter" and you'll see it there. You can maybe just make a testAuthnResponseNoExpiry and then take that part of the response out

miveronese · 2022-10-13T04:06:11Z

enterprise/cmd/frontend/internal/auth/saml/middleware.go

+// It uses github.com/russellhaering/gosaml2 and (unlike authHandler1) makes it possible to support
 // multiple auth providers with SAML and expose more SAML functionality.
 func authHandler(db database.DB, w http.ResponseWriter, r *http.Request, next http.Handler, isAPIRequest bool) {
 // Delegate to SAML ACS and metadata endpoint handlers.


This is a new idea that I need to validate. Not sure if this will work, but I think it is worth trying as it could allow us to refresh expired assertions without the user having to be asked to re-login.

In func authHandler, check if the current session was SAML initiated.

Check whether the assertion has expired (look into user_external_accounts> accountData > NotOnOrAfter field)

if the assertion has expired and this is not an API request, do a redirect to the authn request url using isPassive = true

The user won't see any 401 and won't be asked to log in again; a new assertion will be generated

(Also I need to double check if isPassive is supported by the main IdPs)

So the user, when auto-logged-out would simply be redirected to the identity-provider to login again and then back to sourcegraph? That would be nice.

mrnugget · 2022-10-13T14:18:15Z

(I fear I know too little of SAML to be of value here. I'll need to get more context to accurately judge this)

pjlast · 2022-10-14T07:40:54Z

enterprise/cmd/frontend/internal/auth/saml/middleware.go

+if info.expirationTime != "" {
+exp = time.Until(t)
+}


You'll want to do this check before the time.Parse, since if it is empty, then the time.Parse would already have errored and returned a 500

oops, totally! thanks for pointing this out!

pjlast · 2022-10-14T07:43:06Z

enterprise/cmd/frontend/internal/auth/saml/user.go

+if decodedResponse.Assertions[0].Conditions != nil && decodedResponse.Assertions[0].Conditions.NotOnOrAfter != "" {
+notOnOrAfter = decodedResponse.Assertions[0].Conditions.NotOnOrAfter
+}


Suggested change

if decodedResponse.Assertions[0].Conditions != nil && decodedResponse.Assertions[0].Conditions.NotOnOrAfter != "" {

notOnOrAfter = decodedResponse.Assertions[0].Conditions.NotOnOrAfter

}

if decodedResponse.Assertions[0].Conditions != nil {

notOnOrAfter = decodedResponse.Assertions[0].Conditions.NotOnOrAfter

}

NotOnOrAfter can also be optional, so we also need to check if it is present or we will have a nil pointer issue.

NotOnOrAfter is defined as a string, so it can't be nil, it can only be "". But you can remove this check here, because the local variable notOnOrAfter is also defined as a string, so worst case you'll be assigning the empty string to an empty string, meaning it stays the same anyways. But it's one less thing for the reader of the code to process mentally

miveronese · 2022-10-14T15:50:55Z

(I fear I know too little of SAML to be of value here. I'll need to get more context to accurately judge this)

@mrnugget , I realized that my PR description didn't help much in providing context, so I updated it giving with more details and context. Hopefully, I am not creating more confusion.

https://github.com/sourcegraph/sourcegraph/pull/42915#issuecomment-1277690014

mrnugget · 2022-10-17T12:25:02Z

@mrnugget , I realized that my PR description didn't help much in providing context, so I updated it giving with more details and context. Hopefully, I am not creating more confusion.

Now that's a nice PR description!! Well done!

mrnugget

LGTM, but I wonder whether we shouldn't implemented what you sketched out here first https://github.com/sourcegraph/sourcegraph/pull/42915/files#r994093907 to not ruin UX?

mrnugget · 2022-10-17T12:33:21Z

enterprise/cmd/frontend/internal/auth/saml/user.go

+if err != nil {
+return nil, errors.Errorf("parsing SAML encoded response: %+v", err)
+}
+if decodedResponse.Assertions[0].Conditions != nil && decodedResponse.Assertions[0].Conditions.NotOnOrAfter != "" {


Do we need a if len(decodedResponse.Assertions) > 0 check here? Have to admit that the [0] make me a bit light-headed.

miveronese added 3 commits October 12, 2022 17:05

Check when a SAML asssertion has expired

1ce6c61

Check if Conditions are present

77fa9fe

Fix typos

a88e75b

cla-bot bot added the cla-signed label Oct 13, 2022

github-actions bot added the team/iam label Oct 13, 2022

miveronese commented Oct 13, 2022

View reviewed changes

miveronese requested a review from a team October 13, 2022 02:30

miveronese commented Oct 13, 2022

View reviewed changes

pjlast reviewed Oct 14, 2022

View reviewed changes

mrnugget approved these changes Oct 17, 2022

View reviewed changes

eseliger removed the iam label Oct 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add check for assertion expiration during the SAML auth flow #42915

Add check for assertion expiration during the SAML auth flow #42915

Uh oh!

miveronese commented Oct 13, 2022 •

edited

Loading

miveronese Oct 13, 2022

pjlast Oct 14, 2022

miveronese Oct 13, 2022 •

edited

Loading

mrnugget Oct 17, 2022

mrnugget commented Oct 13, 2022

pjlast Oct 14, 2022

miveronese Oct 14, 2022

pjlast Oct 14, 2022

miveronese Oct 14, 2022

pjlast Oct 14, 2022

miveronese commented Oct 14, 2022

mrnugget commented Oct 17, 2022

mrnugget left a comment

mrnugget Oct 17, 2022

Labels

5 participants

Add check for assertion expiration during the SAML auth flow #42915

Are you sure you want to change the base?

Add check for assertion expiration during the SAML auth flow #42915

Uh oh!

Conversation

miveronese commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context:

Test plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miveronese Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrnugget commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miveronese commented Oct 14, 2022

mrnugget commented Oct 17, 2022

mrnugget left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

5 participants

miveronese commented Oct 13, 2022 •

edited

Loading

miveronese Oct 13, 2022 •

edited

Loading