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

OIDC connector insecureSkipEmailVerified flag is not used #1455

Closed
pbochynski opened this issue May 28, 2019 · 2 comments · Fixed by #1456
Closed

OIDC connector insecureSkipEmailVerified flag is not used #1455

pbochynski opened this issue May 28, 2019 · 2 comments · Fixed by #1456

Comments

@pbochynski
Copy link
Contributor

The documentation says:

Some providers return claims without "email_verified", when they had no usage of emails verification in enrollement process or if they are acting as a proxy for another IDP etc AWS Cognito with an upstream SAML IDP
This can be overridden with the below option
insecureSkipEmailVerified: true

But if "email_vierified" claim is missing the flag is never checked. It works only if the field exists and has boolean value. The code responsible for it is in here:

dex/connector/oidc/oidc.go

Lines 220 to 242 in 49e59fb

emailVerified, found := claims["email_verified"].(bool)
if !found {
return identity, errors.New("missing \"email_verified\" claim")
}
hostedDomain, _ := claims["hd"].(string)
if len(c.hostedDomains) > 0 {
found := false
for _, domain := range c.hostedDomains {
if hostedDomain == domain {
found = true
break
}
}
if !found {
return identity, fmt.Errorf("oidc: unexpected hd claim %v", hostedDomain)
}
}
if c.insecureSkipEmailVerified {
emailVerified = true
}

The configuration check should be moved up and claim email_vierified should not be retrieved if the flag is set to true

@pbochynski pbochynski changed the title OIDC connector insecureSkipEmailVerified flag is not used in case of missing emailVerified claim OIDC connector insecureSkipEmailVerified flag is not used May 28, 2019
@srenatus
Copy link
Contributor

That sounds accurate. Looks like this nuance got introduced in #1448, and I might even have asked for it... 😅 (All blame is on me!)

I'll look into it. Thanks!

@pbochynski
Copy link
Contributor Author

Thanks for the quick response and really quick fix. I'm impressed!

srenatus added a commit to srenatus/dex that referenced this issue May 28, 2019
mmrath pushed a commit to mmrath/dex that referenced this issue Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants