Verify main vs extra JWT bearers differently #596

NickMeves · 2020-05-30T21:23:56Z

When using the configured provider JWT Verifier, it makes
sense to use the provider CreateSessionStateFromBearerToken
method. For any extra JWT Issuers, they should use a generic
default verifier.

#592

How Has This Been Tested?

Unit tests added for legacy/generic bearer IDToken -> SessionState.

TODO: deploy to a local kubernetes cluster and validate standard human OIDC with k8s service account tokens as an extra jwt issuer. UPDATE: DONE

Checklist:

My change requires a change to the documentation or CHANGELOG.
I have updated the documentation/CHANGELOG accordingly.
I have created a feature (non-master) branch for my PR.

NickMeves · 2020-05-30T21:27:41Z

oauthproxy.go

 		if err != nil {
-			logger.Printf("failed to verify bearer token: %v", err)


I removed this and defaulted to the logging happening only if all verifiers failed. My users have been confused with error messages when 1st or 2nd verifiers on the array fail and the 3rd extraJwt Verifier succeeded (seeing the failed words even though it succeeded was confusing)

NickMeves · 2020-05-30T21:31:50Z

providers/provider_default_test.go

+	assert.Equal(t, rawIDToken, session.AccessToken)
+	assert.Equal(t, rawIDToken, session.IDToken)
+	assert.Equal(t, "", session.RefreshToken)
+	assert.Equal(t, "123456789", session.Email)


What is the overall architectural rationale for forcing a session.Email to a claim.Subject when an email claim is missing? And not using the claim.Subject at all in the session and setting both the session.Email and session.User to the Email?

oauth2-proxy/providers/provider_default.go

Line 155 in f7b28cb

if claims.Email == "" {

I left the existing logic untouched -- but shouldn't session.User == claim.Subject (That's what my OIDC provider results in going the normal route).

oauth2-proxy/providers/provider_default.go

Line 165 in f7b28cb

User: claims.Email,

And should we allow session.Email to be "" if the IDToken is missing the claim? And adjust the email domains validator to accept it if its set to * (if set to anything else it would fail since it is missing a @ character)

User ID claim was added so that we could make sure that a particular claim was added, to be considered the user, but the work hasn't been completed so it's only half way through

As for falling back, I'm not sure why we are doing that, i think we currently require all sessions to have a user and email field that's not empty, so I guess that's why? I think I need to do some archaeology on this

NickMeves · 2020-06-01T15:29:14Z

On further reflection, I think this is definitely a bug?

oauth2-proxy/providers/provider_default.go

Line 165 in a17c488

User: claims.Email,

I had a user make a web app that used this proxy for use with CLI tools that did OIDC via PKSE and passed the IDToken as the bearer. He got the X-Forwarded-User as the email claim, vs all the other normal webapps made with our OneLogin OIDC provider had the subject claim as the X-Forwarded-User.

I was very confused at the time, but this is definitely why.

providers/provider_default.go

JoelSpeed

Had a read through, looks pretty good so far, left a couple of comments

Please fixup the conflicts and add a changelog entry

providers/provider_default.go

JoelSpeed · 2020-06-02T18:38:23Z

providers/provider_default_test.go

+	assert.Equal(t, rawIDToken, session.AccessToken)
+	assert.Equal(t, rawIDToken, session.IDToken)
+	assert.Equal(t, "", session.RefreshToken)
+	assert.Equal(t, "123456789", session.Email)


User ID claim was added so that we could make sure that a particular claim was added, to be considered the user, but the work hasn't been completed so it's only half way through

As for falling back, I'm not sure why we are doing that, i think we currently require all sessions to have a user and email field that's not empty, so I guess that's why? I think I need to do some archaeology on this

NickMeves · 2020-06-02T19:23:23Z

I still owe you an interactive test on this, I haven't dockerized this branch yet to test how it all works deployed as a sidecar in a local kubernetes environment that is setup for OIDC projected service account tokens.

NickMeves · 2020-06-02T20:18:17Z

Should I make the default_provider aware of the --user-id-claim flag when mapping claims to a session.Email just the same as the OIDC provider?

NickMeves · 2020-06-03T00:38:57Z

@JoelSpeed Let me know what you find out as far as the history of the forced session.Email and not allowing it to be "".

This commit is my take on allowing it: 1dd6ba5

I can ditch it if needed (or cherry pick it to a separate PR to keep this PR purely about the bearer bugs).

NickMeves · 2020-06-03T17:30:08Z

I still owe you an interactive test on this, I haven't dockerized this branch yet to test how it all works deployed as a sidecar in a local kubernetes environment that is setup for OIDC projected service account tokens.

This is done, looks good in a kubernetes environment allowing both OneLogin IDTokens (configured provider) OR kubernetes projected service account tokens (extra bearer JWT):

[2020/06/03 17:15:09] [oauthproxy.go:283] Skipping JWT tokens from configured OIDC issuer: "https://greenhouse.onelogin.com/oidc/2"
[2020/06/03 17:15:09] [oauthproxy.go:285] Skipping JWT tokens from extra JWT issuer: "http://oidc.default.svc.cluster.local=sample-app"
...
192.168.144.128 - nick.meves@greenhouse.io [2020/06/03 17:21:27] sample-app.local GET 127.0.0.1:5000 "/sso" HTTP/1.1 "Ruby" 200 1336 0.003
192.168.144.128 - system:serviceaccount:default:dummy [2020/06/03 17:23:46] sample-app.local GET 127.0.0.1:5000 "/sso" HTTP/1.1 "Ruby" 200 1174 0.005

NickMeves · 2020-06-03T17:36:38Z

I had the /sso endpoint just spit back the X-Forwarded-* auth headers, you can see the behavior allowing empty session.Email in the service account token output (the experimental commit we can remove from this this PR if desired):

OneLogin OIDC Human IDToken:

{
  "data": {
     "username":  "[REDACTED]",
     "prefered_username": "nick.meves@greenhouse.io",
     "email":  "nick.meves@greenhouse.io"
  }
}

Kubernetes Projected Service Account IDToken:

{
  "data": {
    "username": "system:serviceaccount:default:dummy",
    "prefered_username": null,
    "email": null
  }
}

JoelSpeed · 2020-06-03T18:19:56Z

So I did some archaeology today, my conclusion is that, if the scope contained email, then the response and the userinfo endpoint should have the email value, but if email is not in the scope, then we don't need for it to fallback to anything I don't think

As far as I can tell, the OAuth2 Proxy doesn't require the email to be returned from the provider anywhere, so I think we can get away with not setting it. It's probably better that I rework this into #560

This will likely be a breaking change, though, technically better than the existing behaviour.
I think we should probably look into the user-id-claim work and work out how we move forward with that.

NickMeves · 2020-06-03T18:27:29Z

So I did some archaeology today, my conclusion is that, if the scope contained email, then the response and the userinfo endpoint should have the email value, but if email is not in the scope, then we don't need for it to fallback to anything I don't think

As far as I can tell, the OAuth2 Proxy doesn't require the email to be returned from the provider anywhere, so I think we can get away with not setting it. It's probably better that I rework this into #560

This will likely be a breaking change, though, technically better than the existing behaviour.
I think we should probably look into the user-id-claim work and work out how we move forward with that.

I can remove that commit, isn't the end of the world to have a session.Email that isn't an email in cases where the subject claim isn't an email (with it as blank or a non-email from the subject -- both routes require --emails-domains=* to validate appropriately).

I'm not sure this is best handled in the #560 -- its that weird middle ground for how do we agnostically treat extra JWT bearers that aren't from the main provider in a generic way.

IE my case, I like my OIDC provider settings have the user-id claim to email like the default, but I don't want to change that to something just to appease compatibility with whatever extra JWT Bearer Verifiers I add since their IDToken structures could feasibly be all over the place (subject/audience is likely the only thing we can rely on it having).

JoelSpeed

@NickMeves Can you just confirm this is good to go from your perspective, I believe it is and have no further comments

@steakunderscore I'd appreciate if you could also review this one before merging

NickMeves · 2020-06-08T02:13:00Z

providers/provider_default_test.go

+	rawIDToken, err := jwt.NewWithClaims(jwt.SigningMethodRS256, minimalIDToken).SignedString(key)
+	assert.NoError(t, err)
+	// Pass to a dummy Verifier to get an oidc.IDToken from the rawIDToken for our actual test below
+	idToken, err := verifier.Verify(context.Background(), rawIDToken)


There's got to be a cleaner way to get an oidc.IDToken from a rawIDToken string...? Getting it through a dummy oidc.NewVerifier worked, but just feels sloppy to me -- but nothing stood out to me when I tried to dig through oidc 🤷

It is just a struct at the end of the day, you could theoretically construct it, but then you can't set the private methods. I think what you've done is probably the best we can do sadly

NickMeves · 2020-06-08T02:16:37Z

@NickMeves Can you just confirm this is good to go from your perspective, I believe it is and have no further comments

@steakunderscore I'd appreciate if you could also review this one before merging

Yup, ready to go. I ran some manual tests in a local kubernetes environment and it looked good.

Only area I had a lingering question is the new unit test I added that feels a little sloppy.

NickMeves · 2020-06-19T17:49:13Z

Give me the heads up when we're ready and I'll rebase before the merge. I think I keep dismissing your reviews when I push up rebases 😄

JoelSpeed · 2020-06-19T18:42:47Z

I think let's just get this merged, can you rebase?

When using the configured provider JWT Verifier, it makes sense to use the provider `CreateSessionStateFromBearerToken` method. For any extra JWT Issuers, they should use a generic default verifier.

NickMeves requested review from JoelSpeed and steakunderscore as code owners May 30, 2020 21:23

NickMeves force-pushed the extra-jwt-token-session branch from 5b26754 to a547010 Compare May 30, 2020 21:25

NickMeves commented May 30, 2020

View reviewed changes

NickMeves commented Jun 1, 2020

View reviewed changes

providers/provider_default.go Show resolved Hide resolved

JoelSpeed added the bug label Jun 2, 2020

JoelSpeed reviewed Jun 2, 2020

View reviewed changes

NickMeves force-pushed the extra-jwt-token-session branch from b08147b to f2bf5a3 Compare June 3, 2020 00:36

NickMeves force-pushed the extra-jwt-token-session branch from f2bf5a3 to 1dd6ba5 Compare June 3, 2020 00:44

NickMeves force-pushed the extra-jwt-token-session branch 3 times, most recently from 09df48d to 7d5c8ff Compare June 4, 2020 19:16

JoelSpeed previously approved these changes Jun 7, 2020

View reviewed changes

NickMeves commented Jun 8, 2020

View reviewed changes

NickMeves dismissed JoelSpeed’s stale review via 3d29d24 June 12, 2020 21:52

NickMeves force-pushed the extra-jwt-token-session branch from 7d5c8ff to 3d29d24 Compare June 12, 2020 21:52

JoelSpeed previously approved these changes Jun 14, 2020

View reviewed changes

NickMeves dismissed JoelSpeed’s stale review via 0d190e9 June 14, 2020 20:24

NickMeves force-pushed the extra-jwt-token-session branch from 3d29d24 to 0d190e9 Compare June 14, 2020 20:24

JoelSpeed previously approved these changes Jun 19, 2020

View reviewed changes

Nick Meves added 3 commits June 19, 2020 11:47

Verify main v extra JWT bearers differently

788d8ec

When using the configured provider JWT Verifier, it makes sense to use the provider `CreateSessionStateFromBearerToken` method. For any extra JWT Issuers, they should use a generic default verifier.

Set User = Subject in ExtraJWTBearer sessions

c2c1caa

Improve default CreateSessionStateFromBearerToken tests

a3eef17

NickMeves dismissed JoelSpeed’s stale review via a3eef17 June 19, 2020 18:49

NickMeves force-pushed the extra-jwt-token-session branch from 0d190e9 to a3eef17 Compare June 19, 2020 18:49

JoelSpeed approved these changes Jun 25, 2020

View reviewed changes

JoelSpeed merged commit 3686b0b into oauth2-proxy:master Jun 25, 2020

JoelSpeed mentioned this pull request Jun 25, 2020

Validate Bearer IDToken with proper provider when ExtraJwtIssuers set #592

Closed

NickMeves mentioned this pull request Jul 4, 2020

[FEATURE] support machine users by adding option to disable materialization of email address #658

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify main vs extra JWT bearers differently #596

Verify main vs extra JWT bearers differently #596

NickMeves commented May 30, 2020 •

edited

Loading

NickMeves May 30, 2020

NickMeves May 30, 2020

JoelSpeed Jun 2, 2020

NickMeves commented Jun 1, 2020

JoelSpeed left a comment •

edited

Loading

JoelSpeed Jun 2, 2020

NickMeves commented Jun 2, 2020

NickMeves commented Jun 2, 2020

NickMeves commented Jun 3, 2020 •

edited

Loading

NickMeves commented Jun 3, 2020

NickMeves commented Jun 3, 2020

JoelSpeed commented Jun 3, 2020

NickMeves commented Jun 3, 2020

JoelSpeed left a comment

NickMeves Jun 8, 2020

JoelSpeed Jun 12, 2020

NickMeves commented Jun 8, 2020

NickMeves commented Jun 19, 2020

JoelSpeed commented Jun 19, 2020

		if err != nil {
		logger.Printf("failed to verify bearer token: %v", err)

Verify main vs extra JWT bearers differently #596

Verify main vs extra JWT bearers differently #596

Conversation

NickMeves commented May 30, 2020 • edited Loading

How Has This Been Tested?

Checklist:

NickMeves May 30, 2020

Choose a reason for hiding this comment

NickMeves May 30, 2020

Choose a reason for hiding this comment

JoelSpeed Jun 2, 2020

Choose a reason for hiding this comment

NickMeves commented Jun 1, 2020

JoelSpeed left a comment • edited Loading

Choose a reason for hiding this comment

JoelSpeed Jun 2, 2020

Choose a reason for hiding this comment

NickMeves commented Jun 2, 2020

NickMeves commented Jun 2, 2020

NickMeves commented Jun 3, 2020 • edited Loading

NickMeves commented Jun 3, 2020

NickMeves commented Jun 3, 2020

JoelSpeed commented Jun 3, 2020

NickMeves commented Jun 3, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

NickMeves Jun 8, 2020

Choose a reason for hiding this comment

JoelSpeed Jun 12, 2020

Choose a reason for hiding this comment

NickMeves commented Jun 8, 2020

NickMeves commented Jun 19, 2020

JoelSpeed commented Jun 19, 2020

NickMeves commented May 30, 2020 •

edited

Loading

JoelSpeed left a comment •

edited

Loading

NickMeves commented Jun 3, 2020 •

edited

Loading