diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index d5a5e2af8e..104a7ba9f4 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -5,9 +5,8 @@ package auth import ( "context" - "crypto/sha256" + "crypto/subtle" "encoding/base32" - "encoding/base64" "errors" "fmt" "net" @@ -24,6 +23,7 @@ import ( uuid "github.com/google/uuid" "golang.org/x/crypto/bcrypt" + "golang.org/x/oauth2" "xorm.io/builder" "xorm.io/xorm" ) @@ -31,7 +31,10 @@ import ( // Authorization codes should expire within 10 minutes per https://datatracker.ietf.org/doc/html/rfc6749#section-4.1.2 const oauth2AuthorizationCodeValidity = 10 * time.Minute -var ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated") +var ( + ErrOAuth2AuthorizationCodeInvalidated = errors.New("oauth2 authorization code already invalidated") + ErrOAuth2GrantStaleCounter = errors.New("oauth2 grant state changed during token refresh") +) // OAuth2Application represents an OAuth2 client (RFC 6749) type OAuth2Application struct { @@ -151,30 +154,40 @@ func (app *OAuth2Application) ContainsRedirectURI(redirectURI string) bool { // https://www.rfc-editor.org/rfc/rfc6819#section-5.2.3.3 // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest // https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics-12#section-3.1 - contains := func(s string) bool { - s = strings.TrimSuffix(strings.ToLower(s), "/") - for _, u := range app.RedirectURIs { - if strings.TrimSuffix(strings.ToLower(u), "/") == s { + redirectCandidates := []string{redirectURI} + if !app.ConfidentialClient { + loopbackRedirect, ok := normalizePublicClientRedirectURI(redirectURI) + if ok { + redirectCandidates = append(redirectCandidates, loopbackRedirect) + } + } + + for _, candidate := range redirectCandidates { + normalizedCandidate := normalizeRedirectURIForComparison(candidate) + for _, registeredURI := range app.RedirectURIs { + if normalizeRedirectURIForComparison(registeredURI) == normalizedCandidate { return true } } - return false } - if !app.ConfidentialClient { - uri, err := url.Parse(redirectURI) - // ignore port for http loopback uris following https://datatracker.ietf.org/doc/html/rfc8252#section-7.3 - if err == nil && uri.Scheme == "http" && uri.Port() != "" { - ip := net.ParseIP(uri.Hostname()) - if ip != nil && ip.IsLoopback() { - // strip port - uri.Host = uri.Hostname() - if contains(uri.String()) { - return true - } - } - } + + return false +} + +func normalizeRedirectURIForComparison(redirectURI string) string { + return strings.TrimSuffix(util.ToLowerASCII(redirectURI), "/") +} + +func normalizePublicClientRedirectURI(redirectURI string) (string, bool) { + parsedURI, err := url.Parse(redirectURI) + if err != nil || parsedURI.Scheme != "http" || parsedURI.Port() == "" { + return "", false } - return contains(redirectURI) + if ip := net.ParseIP(parsedURI.Hostname()); ip == nil || !ip.IsLoopback() { + return "", false + } + parsedURI.Host = parsedURI.Hostname() + return parsedURI.String(), true } // Base32 characters, but lowercased. @@ -424,22 +437,34 @@ func (code *OAuth2AuthorizationCode) Invalidate(ctx context.Context) error { return nil } +func (code *OAuth2AuthorizationCode) requiresCodeVerifier() bool { + return code.CodeChallengeMethod != "" || code.CodeChallenge != "" +} + +func deriveCodeChallenge(method, verifier string) (string, bool) { + switch method { + case "S256": + return oauth2.S256ChallengeFromVerifier(verifier), true + case "plain": + return verifier, true + default: + return "", false + } +} + // ValidateCodeChallenge validates the given verifier against the saved code challenge. This is part of the PKCE implementation. func (code *OAuth2AuthorizationCode) ValidateCodeChallenge(verifier string) bool { - switch code.CodeChallengeMethod { - case "S256": - // base64url(SHA256(verifier)) see https://tools.ietf.org/html/rfc7636#section-4.6 - h := sha256.Sum256([]byte(verifier)) - hashedVerifier := base64.RawURLEncoding.EncodeToString(h[:]) - return hashedVerifier == code.CodeChallenge - case "plain": - return verifier == code.CodeChallenge - case "": + if !code.requiresCodeVerifier() { return true - default: - // unsupported method -> return false + } + if verifier == "" || code.CodeChallengeMethod == "" { return false } + expectedChallenge, ok := deriveCodeChallenge(code.CodeChallengeMethod, verifier) + if !ok { + return false + } + return subtle.ConstantTimeCompare([]byte(expectedChallenge), []byte(code.CodeChallenge)) == 1 } // GetOAuth2AuthorizationByCode returns an authorization by its code @@ -504,15 +529,18 @@ func (grant *OAuth2Grant) GenerateNewAuthorizationCode(ctx context.Context, redi // IncreaseCounter increases the counter and updates the grant func (grant *OAuth2Grant) IncreaseCounter(ctx context.Context) error { - _, err := db.GetEngine(ctx).ID(grant.ID).Incr("counter").Update(new(OAuth2Grant)) + affected, err := db.GetEngine(ctx). + Where("id = ?", grant.ID). + And("counter = ?", grant.Counter). + Incr("counter"). + Update(new(OAuth2Grant)) if err != nil { return err } - updatedGrant, err := GetOAuth2GrantByID(ctx, grant.ID) - if err != nil { - return err + if affected == 0 { + return ErrOAuth2GrantStaleCounter } - grant.Counter = updatedGrant.Counter + grant.Counter++ return nil } diff --git a/models/auth/oauth2_test.go b/models/auth/oauth2_test.go index d72e1cb1d5..465dfb14f2 100644 --- a/models/auth/oauth2_test.go +++ b/models/auth/oauth2_test.go @@ -13,6 +13,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "golang.org/x/oauth2" ) func TestOAuth2AuthorizationCode(t *testing.T) { @@ -116,6 +117,47 @@ func TestOAuth2Application_ContainsRedirect_Slash(t *testing.T) { assert.False(t, app.ContainsRedirectURI("http://127.0.0.1/other")) } +func TestOAuth2Application_ContainsRedirectURI_ASCIIOnlyNormalization(t *testing.T) { + testCases := []struct { + name string + registered []string + redirectURI string + allowed bool + }{ + { + name: "exact-match", + registered: []string{"https://signin.example.test/callback"}, + redirectURI: "https://signin.example.test/callback", + allowed: true, + }, + { + name: "ascii-case-insensitive", + registered: []string{"https://signin.example.test/callback"}, + redirectURI: "https://signIN.example.test/callback", + allowed: true, + }, + { + name: "non-ascii-not-folded", + registered: []string{"https://signin.example.test/callback"}, + redirectURI: "https://signİn.example.test/callback", + allowed: false, + }, + { + name: "loopback-strips-port", + registered: []string{"http://127.0.0.1/callback"}, + redirectURI: "http://127.0.0.1:12345/callback", + allowed: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + app := &auth_model.OAuth2Application{RedirectURIs: tc.registered} + assert.Equal(t, tc.allowed, app.ContainsRedirectURI(tc.redirectURI)) + }) + } +} + func TestOAuth2Application_ValidateClientSecret(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) app := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Application{ID: 1}) @@ -193,6 +235,16 @@ func TestOAuth2Grant_IncreaseCounter(t *testing.T) { unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 2}) } +func TestOAuth2Grant_IncreaseCounterRejectsStaleCounter(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Counter: 1}) + stale := *grant + + assert.NoError(t, grant.IncreaseCounter(t.Context())) + err := stale.IncreaseCounter(t.Context()) + assert.ErrorIs(t, err, auth_model.ErrOAuth2GrantStaleCounter) +} + func TestOAuth2Grant_ScopeContains(t *testing.T) { assert.NoError(t, unittest.PrepareTestDatabase()) grant := unittest.AssertExistsAndLoadBean(t, &auth_model.OAuth2Grant{ID: 1, Scope: "openid profile"}) @@ -237,35 +289,38 @@ func TestRevokeOAuth2Grant(t *testing.T) { //////////////////// Authorization Code func TestOAuth2AuthorizationCode_ValidateCodeChallenge(t *testing.T) { - // test plain - code := &auth_model.OAuth2AuthorizationCode{ - CodeChallengeMethod: "plain", - CodeChallenge: "test123", - } - assert.True(t, code.ValidateCodeChallenge("test123")) - assert.False(t, code.ValidateCodeChallenge("ierwgjoergjio")) + s256Verifier := "s256-verifier" + s256Challenge := oauth2.S256ChallengeFromVerifier(s256Verifier) + missingVerifierChallenge := oauth2.S256ChallengeFromVerifier("verifier-not-supplied") - // test S256 - code = &auth_model.OAuth2AuthorizationCode{ - CodeChallengeMethod: "S256", - CodeChallenge: "CjvyTLSdR47G5zYenDA-eDWW4lRrO8yvjcWwbD_deOg", + testCases := []struct { + name string + method string + challenge string + verifier string + valid bool + }{ + {"plain-success", "plain", "plain-secret", "plain-secret", true}, + {"plain-failure", "plain", "plain-secret", "ierwgjoergjio", false}, + {"s256-success", "S256", s256Challenge, s256Verifier, true}, + {"s256-failure", "S256", s256Challenge, "wiogjerogorewngoenrgoiuenorg", false}, + {"unsupported-method", "monkey", "foiwgjioriogeiogjerger", "foiwgjioriogeiogjerger", false}, + {"no-pkce-configured", "", "", "", true}, + {"s256-missing-verifier", "S256", missingVerifierChallenge, "", false}, + {"plain-missing-verifier", "plain", "plain-secret", "", false}, + {"missing-method-with-challenge", "", "foierjiogerogerg", "", false}, + {"missing-method-rejects-even-matching-verifier", "", "foierjiogerogerg", "foierjiogerogerg", false}, } - assert.True(t, code.ValidateCodeChallenge("N1Zo9-8Rfwhkt68r1r29ty8YwIraXR8eh_1Qwxg7yQXsonBt")) - assert.False(t, code.ValidateCodeChallenge("wiogjerogorewngoenrgoiuenorg")) - // test unknown - code = &auth_model.OAuth2AuthorizationCode{ - CodeChallengeMethod: "monkey", - CodeChallenge: "foiwgjioriogeiogjerger", + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + code := &auth_model.OAuth2AuthorizationCode{ + CodeChallengeMethod: tc.method, + CodeChallenge: tc.challenge, + } + assert.Equal(t, tc.valid, code.ValidateCodeChallenge(tc.verifier)) + }) } - assert.False(t, code.ValidateCodeChallenge("foiwgjioriogeiogjerger")) - - // test no code challenge - code = &auth_model.OAuth2AuthorizationCode{ - CodeChallengeMethod: "", - CodeChallenge: "foierjiogerogerg", - } - assert.True(t, code.ValidateCodeChallenge("")) } func TestOAuth2AuthorizationCode_GenerateRedirectURI(t *testing.T) {