Support OpenID tokens for getting using information by alek-sys · Pull Request #122 · go-pkgz/auth

alek-sys · 2022-06-22T07:53:38Z

Some providers (e.g. Cognito) provide only minimal subset of user attributes in /userinfo endpoint, 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-configuration URLs) 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.

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.

makeRedirURL should work from a request, but it's not part of this PR

Key generation is slow(-ish) so usual sleeps of 50ms sometimes not enough, that makes tests flaky.

coveralls · 2022-06-22T08:41:38Z

Pull Request Test Coverage Report for Build 2568262682

29 of 29 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.4%) to 96.099%

Totals
Change from base Build 2443879937:	0.4%
Covered Lines:	271
Relevant Lines:	282

💛 - 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.

provider/dev_provider.go

 				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)


umputun

a few, mostly minor concerns. Some could be due to my quick review withour diving deep into the problem area.

umputun · 2022-07-04T17:04:30Z

provider/oauth2.go


 	"github.com/go-pkgz/rest"
 	"github.com/golang-jwt/jwt"
+	jwtv4 "github.com/golang-jwt/jwt/v4"


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.

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.

umputun · 2022-07-04T17:07:50Z

provider/oauth2.go

 	scopes   []string
 	mapUser  func(UserData, []byte) token.User // map info from InfoURL to User
 	conf     oauth2.Config
+	keyfunc  jwt.Keyfunc


maybe make the relation between these to more explicit with internal struct{fn,mutex}?

umputun · 2022-07-04T17:12:39Z

provider/oauth2.go

+	parser := jwt.Parser{
+		// claims validation is not considering clock skew and randomly failing with iat validation
+		// nbf and exp are validated below
+		SkipClaimsValidation: true,


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?

Upgrading to jwt/v4 should solve this issue as well, they added clock skew recently.

umputun · 2022-07-04T17:16:27Z

provider/oauth2.go

 	return strings.TrimSuffix(p.URL, "/") + strings.TrimSuffix(newPath, "/") + urlCallbackSuffix
 }
+
+func (p *Oauth2Handler) tryInitJWKSKeyfunc() error {


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?

umputun · 2022-07-04T17:19:53Z

provider/dev_provider.go

 				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)


isn't this change will break back compat for dev provider?

tendant · 2022-10-10T05:19:24Z

This feature is very useful. Is there anything needed for merging this PR?

umputun

the PR has been open for a while, so I took a fresh look. my previous inline comments still mostly stand - 3 of 5 weren't addressed. beyond those, found a few new issues:

clock skew logic is inverted for exp - oauth2.go:279: time.Now().Add(clockSkew) makes the exp check stricter (rejects tokens 10s early). should be time.Now().Add(-clockSkew) to allow tokens that expired up to 10s ago. the nbf direction is correct
sub claim is literal %s - dev_provider.go:118: "sub": "%s" is never passed through fmt.Sprintf, so the sub claim value is literally %s. test masks this because it overrides sub via CustomizeIDTokenFn
value receiver on loadUserFromIDToken - oauth2.go:250: since tryInitJWKSKeyfunc uses a pointer receiver, calling it from a value receiver method means the keyfunc is set on a local copy and lost. if the initial JWKS load fails at startup, every subsequent request will re-fetch from the endpoint
missing iss/aud validation - oauth2.go:263-286: with SkipClaimsValidation: true, only exp/nbf are manually checked. OpenID Connect spec requires validating iss (matches provider) and aud (contains client_id). without this, a valid ID token from the same IdP but for a different application would be accepted
import grouping - third-party packages (keyfunc, errors, jwt) mixed into the stdlib import block in both oauth2.go and dev_provider.go

also, the branch needs a rebase - it's significantly behind master, causing test failures in token/TestJWT_Parse and race conditions in telegram_test.go that were already fixed on master.

no scope creep detected, all changes connect to the PR purpose. the feature itself addresses a real need and the JWKS key rotation handling with rate limiting is well done.

alek-sys added 6 commits June 21, 2022 09:34

Add support of OpenID providers

a528a70

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.

Allow to add custom OpenID providers

242fcfc

Refactor keys loading to retry on token request

e3fd054

Only support OpenID for custom providers

89969d4

Fix typos

7411bff

Cleanup

8026918

makeRedirURL should work from a request, but it's not part of this PR

alek-sys requested a review from umputun as a code owner June 22, 2022 07:53

alek-sys added 2 commits June 22, 2022 09:09

Only generate private key if OpenID is enabled

e5a20c7

Key generation is slow(-ish) so usual sleeps of 50ms sometimes not enough, that makes tests flaky.

Fix linter issues and refactor to reduce cyclomatic complexity

ab93f7d

alek-sys added 3 commits June 22, 2022 10:11

Make sure AddDevOpenIDProvider is called in auth_test.go

41a0ffe

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.

github-advanced-security bot found potential problems Jun 24, 2022

View reviewed changes

alek-sys added 2 commits June 24, 2022 13:48

Wrap errors when loading user info

d6b8f26

Tidy up

3bf3b62

umputun requested changes Jul 4, 2022

View reviewed changes

umputun requested changes Feb 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Comments

Support OpenID tokens for getting using information#122

Support OpenID tokens for getting using information#122
alek-sys wants to merge 13 commits intogo-pkgz:masterfrom
alek-sys:master

alek-sys commented Jun 22, 2022

Uh oh!

coveralls commented Jun 22, 2022 •

edited

Loading

Uh oh!

Check warning

umputun left a comment

Uh oh!

umputun Jul 4, 2022

Uh oh!

alek-sys Jul 5, 2022

Uh oh!

umputun Jul 4, 2022

Uh oh!

umputun Jul 4, 2022

Uh oh!

alek-sys Jul 5, 2022

Uh oh!

umputun Jul 4, 2022

Uh oh!

umputun Jul 4, 2022

Uh oh!

tendant commented Oct 10, 2022

Uh oh!

umputun left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Comments

Conversation

alek-sys commented Jun 22, 2022

Uh oh!

coveralls commented Jun 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2568262682

💛 - Coveralls

Uh oh!

Check warning

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

umputun Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

alek-sys Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

umputun Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

umputun Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

alek-sys Jul 5, 2022

Choose a reason for hiding this comment

Uh oh!

umputun Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

umputun Jul 4, 2022

Choose a reason for hiding this comment

Uh oh!

tendant commented Oct 10, 2022

Uh oh!

umputun left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 22, 2022 •

edited

Loading