Support OpenID tokens for getting using information #122
Conversation
With OpenID flow, instead of using /userinfo endpoint, an ID token issued by the authorisation server is used. Information in this token ususally includes extra params and options, not available in userinfo response.
Key generation is slow(-ish) so usual sleeps of 50ms sometimes not enough, that makes tests flaky.
Pull Request Test Coverage Report for Build 2568262682
💛 - Coveralls |
Weirdly coveralls thinks this method is not covered, because it is tested in another package. However there isn't much to test really, so at best I can check jwks URL is correctly served.
Actual login flow is tested already, and these two new methods are called in provider/openid_test.go. However the coverage tool is not detecting these calls, and instead seems to be requiring the methods to be called in the matching test file. So this test is a weird artifact to make coverage tool happy.
golang-jwt library is trying to validate iat claim of the ID token and due to not accounting for clock skew, validation pretty randomly fails. There is an open issue golang-jwt/jwt#98 and seems like that is fixed in v4. However it is still unclear why iat is validation in the first place, that's not required by RFC and doesn't seem like the right thing to do. Only nbf and exp claims should be used for token lifetime validity check. Also, update README to show how to configure OpenID providers.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| @@ -74,7 +90,8 @@ | |||
| } | |||
|
|
|||
| state := r.URL.Query().Get("state") | |||
| callbackURL := fmt.Sprintf("%s?code=g0ZGZmNjVmOWI&state=%s", d.Provider.conf.RedirectURL, state) | |||
| redirectURI := r.URL.Query().Get("redirect_uri") | |||
| callbackURL := fmt.Sprintf("%s?code=g0ZGZmNjVmOWI&state=%s", redirectURI, state) | |||
Check warning
Code scanning / CodeQL
Open URL redirect
| "time" | ||
|
|
||
| "github.com/go-pkgz/rest" | ||
| "github.com/golang-jwt/jwt" | ||
| jwtv4 "github.com/golang-jwt/jwt/v4" |
There was a problem hiding this comment.
is there a good reason to have both v4 and pre-gomod 3.x.x ? I think v4 was mostly compat with v3, at least i recall migrating some projects to v4 without any significant problem.
There was a problem hiding this comment.
That's what keyfunc is using, I don't think they have v3 compatible version anymore. If you are considering upgrading auth to jwt/v4, I'm happy to do that (as a separate PR) as well.
| endpoint oauth2.Endpoint | ||
| scopes []string | ||
| mapUser func(UserData, []byte) token.User // map info from InfoURL to User | ||
| conf oauth2.Config | ||
| keyfunc jwt.Keyfunc |
There was a problem hiding this comment.
maybe make the relation between these to more explicit with internal struct{fn,mutex}?
| parser := jwt.Parser{ | ||
| // claims validation is not considering clock skew and randomly failing with iat validation | ||
| // nbf and exp are validated below | ||
| SkipClaimsValidation: true, |
There was a problem hiding this comment.
I get the idea and the need, but still makes me worry. Maybe there is a way to do it safer somehow without skip validation?
There was a problem hiding this comment.
Upgrading to jwt/v4 should solve this issue as well, they added clock skew recently.
| func (p Oauth2Handler) makeRedirURL(path string) string { | ||
| elems := strings.Split(path, "/") | ||
| newPath := strings.Join(elems[:len(elems)-1], "/") | ||
|
|
||
| return strings.TrimSuffix(p.URL, "/") + strings.TrimSuffix(newPath, "/") + urlCallbackSuffix | ||
| } | ||
|
|
||
| func (p *Oauth2Handler) tryInitJWKSKeyfunc() error { |
There was a problem hiding this comment.
I'm not sure what exactly this thing does, but it fills like the goal of this "try" is to set/init p.keyfunc once? If so, how about sync.Once? Will it make the intent more clear?
| @@ -74,7 +90,8 @@ | |||
| } | |||
|
|
|||
| state := r.URL.Query().Get("state") | |||
| callbackURL := fmt.Sprintf("%s?code=g0ZGZmNjVmOWI&state=%s", d.Provider.conf.RedirectURL, state) | |||
| redirectURI := r.URL.Query().Get("redirect_uri") | |||
| callbackURL := fmt.Sprintf("%s?code=g0ZGZmNjVmOWI&state=%s", redirectURI, state) | |||
There was a problem hiding this comment.
isn't this change will break back compat for dev provider?
|
This feature is very useful. Is there anything needed for merging this PR? |
Some providers (e.g. Cognito) provide only minimal subset of user attributes in
/userinfoendpoint, but much more in the ID tokens. That can be helpful to pass user metadata from the provider to the app. This PR adds support of OpenID tokens as a source of user info, as an extension to the OAuth2 flow.OpenID flow is only supported for custom providers at the moment and require JWKS URL configuration. Switching existing providers (e.g.
google) to OpenID is TBD, but probably not required. Using OpenID Connect (.well-known/openid-configurationURLs) is out of scope for this PR.Apple flow basically is OpenID, with some customisations on how client secret is passed across. So potentially these two flows can be merged together later on.