support AWS Cognito, which is not completely OIDC compliant #257

DallanQ · 2020-07-15T17:43:43Z

I've been using go-oidc and needed to support AWS Cognito, which varies from the OIDC spec in a couple of places. This PR makes go-oidc handle the inconsistencies. I'm happy to make changes or add tests. Just wanted to submit it for consideration.

ericchiang · 2020-07-16T00:03:45Z

Can this be conditioned on a specific issuer URL? E.g. so it only applies to AWS cognito?

ericchiang · 2020-07-16T00:04:15Z

oidc.go

@@ -192,6 +193,14 @@ type UserInfo struct {
 	claims []byte
 }

+// UserInfoRaw allows us to decode both compliant and non-compliant Cognito userInfo
+type UserInfoRaw struct {


Would prefer to have this unexported so

type userInfoRaw struct

good point. I made it unexported.

ericchiang · 2020-07-16T00:04:35Z

oidc.go

@@ -365,6 +380,29 @@ type claimSource struct {
 	AccessToken string `json:"access_token"`
 }

+// Cognito reports email_verified as a string in userInfo, so try decoding as a bool or a string
+type StringAsBool bool


Same here, this should be

type stringBool bool

I made this change as well.

DallanQ · 2020-07-16T16:20:29Z

I conditioned the audience check on matching the cognito issuer in Verify. I didn't condition the UserInfo function to unmarshal into either UserInfo or userInfoRaw depending upon the issuer because it would require parsing the token to extract the issuer. But I'm happy to do that if you like. Or I could first unmarshal into UserInfo and if an error is returned, then parse the token, check the issuer, and if the issuer matches cognito then unmarshal into userInfoRaw (which I'd probably want to rename to userInfoCognito then).

DallanQ · 2020-07-17T05:21:23Z

i'm happy to make any other improvements.

ericchiang

Need to add test cases, and please squash your commits:

go-oidc/verify_test.go

Line 29 in bc56623

func TestVerify(t *testing.T) {

go-oidc/oidc_test.go

Line 358 in bc56623

func TestUserInfoEndpoint(t *testing.T) {

Otherwise lgtm

ericchiang · 2020-07-17T05:40:09Z

Also would you mind including a link to the AWS Cognito OpenID Connect docs as a comment somewhere in the code? After a bit of searching I can't find anything.

DallanQ · 2020-07-21T00:50:54Z

I added tests, links to documentation, and squashed the commits. There's not a lot of documentation available though. I found these issues through trial and error.

I want to make sure I'm thinking correctly. Auth0 sets aud on access tokens (as well as id tokens) and I've been passing Auth0 access tokens into Verify and verifying the audience. Cognito sets client_id on access tokens but not aud, which is why I added the client_id check for Cognito. (The client_id on a Cognito access token has the same value as aud.) But maybe instead of trying to verify the client ID for an access token I should set the SkipClientIDCheck flag in my code to skip client ID verification when I pass an access token to Verify, in which case I should remove the special-casing for Cognito client_id in Verify? I like the idea of having Verify verify the audience on access tokens, but is that incorrect? I'm not an expert in how to use OIDC.

Also, I couldn't find the public/private keypair you used so I generated a new one and used that for the tests.

ericchiang · 2020-07-21T03:52:05Z

Yes you can do the following without modifying go-oidc, you can even make this logic conditional on AWS Cognito:

c := &oidc. Config{
    SkipClientIDCheck: true,
}
v := provider.Verifier(c)
idToken, err := v.Verify(ctx, rawIDToken)
if err != nil {
    // ...
}
var claims struct {
    ClientID string `json:"client_id"`
}
if err := idToken.Claims(&claims); err != nil {
    // ...
}
if claims.ClientID != clientID {
    // return error
}

Would it be better to change this PR to just do the user info fixes and document oidc.Config.ClientID better? Specifically that it verifies the "aud" claim and if you want to skip that validation just provide the oidc.Config.SkipClientIDCheck option? As a library maintainer I'd obvious prefer that since I don't have to deal with providers that go off spec :)

DallanQ · 2020-07-22T03:09:13Z

In hindsight I agree that's the best approach. I reverted the changes to Verify, added a comment, and just left the changes to UserInfo.

ericchiang

Few comments, but lgtm otherwise

ericchiang · 2020-07-26T17:03:00Z

verify.go

@@ -205,6 +205,10 @@ func parseClaim(raw []byte, name string, v interface{}) error {
 //
 //    token, err := verifier.Verify(ctx, rawIDToken)
 //
+// Note that AWS Cognito access tokens include client_id instead of aud, so to verify a


Mind dropping this? We don't call out other providers like this. I can follow up with a doc change for the Config struct to specify that it's checking the "aud" claim for the client ID.

Is there a reason this wasn't dropped?

oversight. it's been removed now.

ericchiang · 2020-07-26T17:05:23Z

oidc.go

@@ -192,6 +193,18 @@ type UserInfo struct {
 	claims []byte
 }

+// userInfoRaw allows us to decode both compliant and non-compliant Cognito userInfo
+// Cognito returns email_verified as a string instead of as a bool.
+// This is not documented by AWS as far as I can tell, but see for example


nit: Comments generally aren't in the first person.

Maybe just a comment above EmailVerified

type userInfoRaw struct { Subject string `json:"sub"` Profile string `json:"profile"` Email string `json:"email"` // Handle providers that return email_verified as a string. // https://forums.aws.amazon.com/thread.jspa?messageID=949441&#949441 // https://discuss.elastic.co/t/openid-error-after-authenticating-against-aws-cognito/206018/11 EmailVerified stringAsBool `json:"email_verified"` }

I deleted the comments above and added the comments above EmailVerified.

ericchiang · 2020-07-26T17:05:49Z

oidc.go

@@ -367,6 +385,29 @@ type claimSource struct {
 	AccessToken string `json:"access_token"`
 }

+// Cognito reports email_verified as a string in userInfo, so try decoding as a bool or a string


nit: drop the comment here.

DallanQ · 2020-08-12T15:09:50Z

Is there anything else you need for this to get merged?

… instead of a bool

DallanQ · 2020-08-23T02:10:02Z

I believe I have made all of the changes requested.

DallanQ · 2020-08-31T17:10:28Z

Is everything ok with this PR?

DallanQ · 2020-09-08T00:41:12Z

just following up on this PR. Is it ok to merge?

DallanQ · 2020-09-08T15:38:59Z

Thank you!

ericchiang reviewed Jul 16, 2020

View reviewed changes

ericchiang reviewed Jul 17, 2020

View reviewed changes

DallanQ force-pushed the v2 branch from e023975 to 9f7005f Compare July 21, 2020 00:42

DallanQ force-pushed the v2 branch from 9f7005f to 9da7e7f Compare July 22, 2020 03:06

ericchiang approved these changes Jul 26, 2020

View reviewed changes

DallanQ force-pushed the v2 branch from 9da7e7f to 5c3c8c1 Compare August 2, 2020 23:34

add support for AWS Cognito, which returns email_verified as a string…

2b28d0c

… instead of a bool

DallanQ force-pushed the v2 branch from 5c3c8c1 to 2b28d0c Compare August 23, 2020 02:07

ericchiang merged commit 0a5cd33 into coreos:v2 Sep 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support AWS Cognito, which is not completely OIDC compliant #257

support AWS Cognito, which is not completely OIDC compliant #257

DallanQ commented Jul 15, 2020

ericchiang commented Jul 16, 2020

ericchiang Jul 16, 2020

DallanQ Jul 16, 2020

ericchiang Jul 16, 2020

DallanQ Jul 16, 2020

DallanQ commented Jul 16, 2020 •

edited

Loading

DallanQ commented Jul 17, 2020

ericchiang left a comment

ericchiang commented Jul 17, 2020

DallanQ commented Jul 21, 2020 •

edited

Loading

ericchiang commented Jul 21, 2020

DallanQ commented Jul 22, 2020

ericchiang left a comment

ericchiang Jul 26, 2020

ericchiang Aug 14, 2020

DallanQ Aug 23, 2020

ericchiang Jul 26, 2020

DallanQ Aug 2, 2020

ericchiang Jul 26, 2020

DallanQ Aug 2, 2020

DallanQ commented Aug 12, 2020

DallanQ commented Aug 23, 2020

DallanQ commented Aug 31, 2020

DallanQ commented Sep 8, 2020

DallanQ commented Sep 8, 2020

support AWS Cognito, which is not completely OIDC compliant #257

support AWS Cognito, which is not completely OIDC compliant #257

Conversation

DallanQ commented Jul 15, 2020

ericchiang commented Jul 16, 2020

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

DallanQ commented Jul 16, 2020 • edited Loading

DallanQ commented Jul 17, 2020

ericchiang left a comment

Choose a reason for hiding this comment

ericchiang commented Jul 17, 2020

DallanQ commented Jul 21, 2020 • edited Loading

ericchiang commented Jul 21, 2020

DallanQ commented Jul 22, 2020

ericchiang left a 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

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

DallanQ commented Aug 12, 2020

DallanQ commented Aug 23, 2020

DallanQ commented Aug 31, 2020

DallanQ commented Sep 8, 2020

DallanQ commented Sep 8, 2020

DallanQ commented Jul 16, 2020 •

edited

Loading

DallanQ commented Jul 21, 2020 •

edited

Loading