From 2fae5459c66e3f9dc11a9cfc34669dbda528c6ac Mon Sep 17 00:00:00 2001 From: Alex Ullrich Date: Fri, 22 Mar 2024 12:29:57 -0400 Subject: [PATCH 1/2] oauth: use provider-specific key for verification (fixes #248) --- app/tokens/oauth/oauth.go | 8 ++++---- app/tokens/oauth/oauth_test.go | 28 ++++++++++++++++++++-------- server/handlers/get_oauth_return.go | 4 ++-- server/handlers/util.go | 4 ++-- 4 files changed, 28 insertions(+), 16 deletions(-) diff --git a/app/tokens/oauth/oauth.go b/app/tokens/oauth/oauth.go index 276d70e768..9f17bbc3d8 100644 --- a/app/tokens/oauth/oauth.go +++ b/app/tokens/oauth/oauth.go @@ -36,21 +36,21 @@ func (c *Claims) Sign(signingKey jose.SigningKey) (string, error) { // Parse will deserialize a string into Claims if and only if the claims pass all validations. In // this case the token must contain a nonce already known from a different channel (like a cookie). -func Parse(tokenStr string, cfg *app.Config, nonce string) (*Claims, error) { +func Parse(tokenStr string, signingKey interface{}, authNURL string, nonce string) (*Claims, error) { token, err := jwt.ParseSigned(tokenStr) if err != nil { return nil, errors.Wrap(err, "ParseSigned") } claims := Claims{} - err = token.Claims(cfg.OAuthSigningKey, &claims) + err = token.Claims(signingKey, &claims) if err != nil { return nil, errors.Wrap(err, "Claims") } err = claims.Claims.Validate(jwt.Expected{ - Audience: jwt.Audience{cfg.AuthNURL.String()}, - Issuer: cfg.AuthNURL.String(), + Audience: jwt.Audience{authNURL}, + Issuer: authNURL, }) if err != nil { return nil, errors.Wrap(err, "Validate") diff --git a/app/tokens/oauth/oauth_test.go b/app/tokens/oauth/oauth_test.go index 6a761561c0..a866715b64 100644 --- a/app/tokens/oauth/oauth_test.go +++ b/app/tokens/oauth/oauth_test.go @@ -29,10 +29,12 @@ func TestOAuthToken(t *testing.T) { assert.True(t, token.Audience.Contains("https://authn.example.com")) assert.NotEmpty(t, token.IssuedAt) - tokenStr, err := token.Sign(jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey}) + key := jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey} + + tokenStr, err := token.Sign(key) require.NoError(t, err) - _, err = oauth.Parse(tokenStr, cfg, nonce) + _, err = oauth.Parse(tokenStr, key.Key, cfg.AuthNURL.String(), nonce) require.NoError(t, err) }) @@ -40,10 +42,12 @@ func TestOAuthToken(t *testing.T) { token, err := oauth.New(cfg, nonce, "https://app.example.com/return") require.NoError(t, err) - tokenStr, err := token.Sign(jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey}) + key := jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey} + + tokenStr, err := token.Sign(key) require.NoError(t, err) - _, err = oauth.Parse(tokenStr, cfg, "wrong") + _, err = oauth.Parse(tokenStr, key.Key, cfg.AuthNURL.String(), "wrong") assert.Error(t, err) }) @@ -52,11 +56,16 @@ func TestOAuthToken(t *testing.T) { AuthNURL: cfg.AuthNURL, OAuthSigningKey: []byte("old-a-reno"), } + oldKey := jose.SigningKey{Algorithm: jose.HS256, Key: oldCfg.OAuthSigningKey} + token, err := oauth.New(cfg, nonce, "https://app.example.com/return") require.NoError(t, err) - tokenStr, err := token.Sign(jose.SigningKey{Algorithm: jose.HS256, Key: oldCfg.OAuthSigningKey}) + + tokenStr, err := token.Sign(oldKey) require.NoError(t, err) - _, err = oauth.Parse(tokenStr, cfg, nonce) + + key := jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey} + _, err = oauth.Parse(tokenStr, key.Key, cfg.AuthNURL.String(), nonce) assert.Error(t, err) }) @@ -67,9 +76,12 @@ func TestOAuthToken(t *testing.T) { } token, err := oauth.New(&oldCfg, nonce, "https://app.example.com/return") require.NoError(t, err) - tokenStr, err := token.Sign(jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey}) + + key := jose.SigningKey{Algorithm: jose.HS256, Key: cfg.OAuthSigningKey} + + tokenStr, err := token.Sign(key) require.NoError(t, err) - _, err = oauth.Parse(tokenStr, cfg, nonce) + _, err = oauth.Parse(tokenStr, key.Key, cfg.AuthNURL.String(), nonce) assert.Error(t, err) }) } diff --git a/server/handlers/get_oauth_return.go b/server/handlers/get_oauth_return.go index caa7bd90b6..186cda58f2 100644 --- a/server/handlers/get_oauth_return.go +++ b/server/handlers/get_oauth_return.go @@ -16,8 +16,8 @@ func GetOauthReturn(app *app.App, providerName string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { provider := app.OauthProviders[providerName] - // verify the state and nonce - state, err := getState(app.Config, r) + // verify the state and nonce using the key associated with the provider's signing key. + state, err := getState(app.Config, r, provider.SigningKey().Key) if err != nil { app.Reporter.ReportRequestError(errors.Wrap(err, "getState"), r) failsafe := app.Config.ApplicationDomains[0].URL() diff --git a/server/handlers/util.go b/server/handlers/util.go index c3c6658c91..01dd93a1d0 100644 --- a/server/handlers/util.go +++ b/server/handlers/util.go @@ -31,12 +31,12 @@ func nonceCookie(cfg *app.Config, val string) *http.Cookie { } // getState returns a verified state token using the nonce cookie -func getState(cfg *app.Config, r *http.Request) (*oauth.Claims, error) { +func getState(cfg *app.Config, r *http.Request, verificationKey interface{}) (*oauth.Claims, error) { nonce, err := r.Cookie(cfg.OAuthCookieName) if err != nil { return nil, errors.Wrap(err, "Cookie") } - state, err := oauth.Parse(r.FormValue("state"), cfg, nonce.Value) + state, err := oauth.Parse(r.FormValue("state"), verificationKey, cfg.AuthNURL.String(), nonce.Value) if err != nil { return nil, errors.Wrap(err, "Parse") } From 82f3a06840dff9a5a6e07b904a3703cffa12b27b Mon Sep 17 00:00:00 2001 From: Alex Ullrich Date: Fri, 22 Mar 2024 12:35:32 -0400 Subject: [PATCH 2/2] update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3401e2bf9..3df936c602 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ Based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ## HEAD +### Fixed + +* Use provider-specific signing key for verification. + ## 1.18.1 ### Added