From 80ce622caa00f975465cd1563509f6c65d880fa1 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Mon, 26 Aug 2024 14:58:38 +0200 Subject: [PATCH] cleanup(oidc): Verify logout tokens useing golang-jwt golang-jwt provides all the necessary functionality to parse and verify LogoutTokens. This gets us rid of the direct go-jose dependency and quite a bit of custom crafted jwt verification code. --- go.mod | 2 +- ocis-pkg/oidc/client.go | 108 +++++------------ ocis-pkg/oidc/client_test.go | 227 +++++++++++++++-------------------- ocis-pkg/oidc/metadata.go | 33 +---- ocis-pkg/oidc/options.go | 17 ++- 5 files changed, 146 insertions(+), 241 deletions(-) diff --git a/go.mod b/go.mod index 67972ac18a..a8068106e0 100644 --- a/go.mod +++ b/go.mod @@ -23,7 +23,6 @@ require ( github.com/ggwhite/go-masker v1.1.0 github.com/go-chi/chi/v5 v5.1.0 github.com/go-chi/render v1.0.3 - github.com/go-jose/go-jose/v3 v3.0.3 github.com/go-ldap/ldap/v3 v3.4.8 github.com/go-ldap/ldif v0.0.0-20200320164324-fd88d9b715b3 github.com/go-micro/plugins/v4/client/grpc v1.2.1 @@ -195,6 +194,7 @@ require ( github.com/go-git/go-billy/v5 v5.5.0 // indirect github.com/go-git/go-git/v5 v5.11.0 // indirect github.com/go-ini/ini v1.67.0 // indirect + github.com/go-jose/go-jose/v3 v3.0.3 // indirect github.com/go-jose/go-jose/v4 v4.0.2 // indirect github.com/go-kit/log v0.2.1 // indirect github.com/go-logfmt/logfmt v0.5.1 // indirect diff --git a/ocis-pkg/oidc/client.go b/ocis-pkg/oidc/client.go index 741f679372..4558a24140 100644 --- a/ocis-pkg/oidc/client.go +++ b/ocis-pkg/oidc/client.go @@ -1,9 +1,7 @@ package oidc import ( - "bytes" "context" - "encoding/base64" "encoding/json" "errors" "fmt" @@ -17,7 +15,6 @@ import ( "github.com/MicahParks/keyfunc/v2" goidc "github.com/coreos/go-oidc/v3/oidc" - "github.com/go-jose/go-jose/v3" "github.com/golang-jwt/jwt/v5" "github.com/owncloud/ocis/v2/ocis-pkg/log" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" @@ -95,6 +92,7 @@ func NewOIDCClient(opts ...Option) OIDCClient { httpClient: options.HTTPClient, accessTokenVerifyMethod: options.AccessTokenVerifyMethod, JWKSOptions: options.JWKSOptions, // TODO I don't like that we pass down config options ... + JWKS: options.JWKS, providerLock: &sync.Mutex{}, jwksLock: &sync.Mutex{}, remoteKeySet: options.KeySet, @@ -319,75 +317,46 @@ func (c *oidcClient) verifyAccessTokenJWT(token string) (RegClaimsWithSID, jwt.M } func (c *oidcClient) VerifyLogoutToken(ctx context.Context, rawToken string) (*LogoutToken, error) { + var claims LogoutToken if err := c.lookupWellKnownOpenidConfiguration(ctx); err != nil { return nil, err } - jws, err := jose.ParseSigned(rawToken) - if err != nil { - return nil, err - } - // Throw out tokens with invalid claims before trying to verify the token. This lets - // us do cheap checks before possibly re-syncing keys. - payload, err := parseJWT(rawToken) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt: %v", err) - } - var token LogoutToken - if err := json.Unmarshal(payload, &token); err != nil { - return nil, fmt.Errorf("oidc: failed to unmarshal claims: %v", err) + jwks := c.getKeyfunc() + if jwks == nil { + return nil, errors.New("error initializing jwks keyfunc") } - //4. Verify that the Logout Token contains a sub Claim, a sid Claim, or both. - if token.Subject == "" && token.SessionId == "" { - return nil, fmt.Errorf("oidc: logout token must contain either sub or sid and MAY contain both") - } - //5. Verify that the Logout Token contains an events Claim whose value is JSON object containing the member name http://schemas.openid.net/event/backchannel-logout. - if token.Events.Event == nil { - return nil, fmt.Errorf("oidc: logout token must contain logout event") - } - //6. Verify that the Logout Token does not contain a nonce Claim. - var n struct { - Nonce *string `json:"nonce"` - } - json.Unmarshal(payload, &n) - if n.Nonce != nil { - return nil, fmt.Errorf("oidc: nonce on logout token MUST NOT be present") - } - // Check issuer. - if !c.skipIssuerValidation && token.Issuer != c.issuer { - return nil, fmt.Errorf("oidc: logout token issued by a different provider, expected %q got %q", c.issuer, token.Issuer) - } - - switch len(jws.Signatures) { - case 0: - return nil, fmt.Errorf("oidc: logout token not signed") - case 1: - // do nothing - default: - return nil, fmt.Errorf("oidc: multiple signatures on logout token not supported") - } - - sig := jws.Signatures[0] + // From the backchannel-logout spec: Like ID Tokens, selection of the + // algorithm used is governed by the id_token_signing_alg_values_supported + // Discovery parameter and the id_token_signed_response_alg Registration + // parameter when they are used; otherwise, the value SHOULD be the default + // of RS256 supportedSigAlgs := c.algorithms if len(supportedSigAlgs) == 0 { supportedSigAlgs = []string{RS256} } - if !contains(supportedSigAlgs, sig.Header.Algorithm) { - return nil, fmt.Errorf("oidc: logout token signed with unsupported algorithm, expected %q got %q", supportedSigAlgs, sig.Header.Algorithm) - } - - gotPayload, err := c.remoteKeySet.VerifySignature(goidc.ClientContext(ctx, c.httpClient), rawToken) + _, err := jwt.ParseWithClaims(rawToken, &claims, jwks.Keyfunc, jwt.WithValidMethods(supportedSigAlgs), jwt.WithIssuer(c.issuer)) if err != nil { - return nil, fmt.Errorf("failed to verify signature: %v", err) + c.Logger.Debug().Err(err).Msg("Failed to parse logout token") + return nil, err + } + // Basic token validation has happened in ParseWithClaims (signature, + // issuer, audience, ...). Now for some logout token specific checks. + // 1. Verify that the Logout Token contains a sub Claim, a sid Claim, or both. + if claims.Subject == "" && claims.SessionId == "" { + return nil, fmt.Errorf("oidc: logout token must contain either sub or sid and MAY contain both") + } + // 2. Verify that the Logout Token contains an events Claim whose value is JSON object containing the member name http://schemas.openid.net/event/backchannel-logout. + if claims.Events.Event == nil { + return nil, fmt.Errorf("oidc: logout token must contain logout event") + } + // 3. Verify that the Logout Token does not contain a nonce Claim. + if claims.Nonce != nil { + return nil, fmt.Errorf("oidc: nonce on logout token MUST NOT be present") } - // Ensure that the payload returned by the square actually matches the payload parsed earlier. - if !bytes.Equal(gotPayload, payload) { - return nil, errors.New("oidc: internal error, payload parsed did not match previous payload") - } - - return &token, nil + return &claims, nil } func unmarshalResp(r *http.Response, body []byte, v interface{}) error { @@ -402,24 +371,3 @@ func unmarshalResp(r *http.Response, body []byte, v interface{}) error { } return fmt.Errorf("expected Content-Type = application/json, got %q: %v", ct, err) } - -func contains(sli []string, ele string) bool { - for _, s := range sli { - if s == ele { - return true - } - } - return false -} - -func parseJWT(p string) ([]byte, error) { - parts := strings.Split(p, ".") - if len(parts) < 2 { - return nil, fmt.Errorf("oidc: malformed jwt, expected 3 parts got %d", len(parts)) - } - payload, err := base64.RawURLEncoding.DecodeString(parts[1]) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt payload: %v", err) - } - return payload, nil -} diff --git a/ocis-pkg/oidc/client_test.go b/ocis-pkg/oidc/client_test.go index d876079917..5fa498c2de 100644 --- a/ocis-pkg/oidc/client_test.go +++ b/ocis-pkg/oidc/client_test.go @@ -2,152 +2,124 @@ package oidc_test import ( "context" - "crypto/ecdsa" - "crypto/elliptic" "crypto/rand" "crypto/rsa" - "fmt" "testing" - goidc "github.com/coreos/go-oidc/v3/oidc" - "github.com/go-jose/go-jose/v3" + "github.com/MicahParks/keyfunc/v2" + "github.com/golang-jwt/jwt/v5" "github.com/owncloud/ocis/v2/ocis-pkg/oidc" ) type signingKey struct { - keyID string // optional - priv interface{} - pub interface{} - alg jose.SignatureAlgorithm -} - -// sign creates a JWS using the private key from the provided payload. -func (s *signingKey) sign(t testing.TB, payload []byte) string { - privKey := &jose.JSONWebKey{Key: s.priv, Algorithm: string(s.alg), KeyID: s.keyID} - - signer, err := jose.NewSigner(jose.SigningKey{Algorithm: s.alg, Key: privKey}, nil) - if err != nil { - t.Fatal(err) - } - jws, err := signer.Sign(payload) - if err != nil { - t.Fatal(err) - } - - data, err := jws.CompactSerialize() - if err != nil { - t.Fatal(err) - } - return data -} - -func (s *signingKey) jwk() jose.JSONWebKey { - return jose.JSONWebKey{Key: s.pub, Use: "sig", Algorithm: string(s.alg), KeyID: s.keyID} + priv interface{} + jwks *keyfunc.JWKS } func TestLogoutVerify(t *testing.T) { tests := []logoutVerificationTest{ { name: "good token", - logoutToken: ` { - "iss": "https://foo", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", - "events": { - "http://schemas.openid.net/event/backchannel-logout": {} - } - }`, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "sub": "248289761001", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), }, { - name: "invalid issuer", - issuer: "https://bar", - logoutToken: `{"iss":"https://foo"}`, - config: goidc.Config{ - SkipExpiryCheck: true, - }, + name: "invalid issuer", + issuer: "https://bar", + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo1", + "sub": "248289761001", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), wantErr: true, }, { name: "invalid sig", - logoutToken: `{ - "iss": "https://foo", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", - "events": { - "http://schemas.openid.net/event/backchannel-logout": {} - } - }`, - config: goidc.Config{ - SkipExpiryCheck: true, - }, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "sub": "248289761001", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), verificationKey: newRSAKey(t), wantErr: true, }, { name: "no sid and no sub", - logoutToken: ` { - "iss": "https://foo", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "events": { - "http://schemas.openid.net/event/backchannel-logout": {} - } - }`, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), wantErr: true, }, { name: "Prohibited nonce present", - logoutToken: ` { - "iss": "https://foo", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "nonce" : "prohibited", - "events": { - "http://schemas.openid.net/event/backchannel-logout": {} - } - }`, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "sub": "248289761001", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", + "nonce": "123", + "events": map[string]interface{}{ + "http://schemas.openid.net/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), wantErr: true, }, { name: "Wrong Event string", - logoutToken: ` { - "iss": "https://foo", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", - "events": { - "not a logout event": {} - } - }`, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "sub": "248289761001", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", + "events": map[string]interface{}{ + "http://blah.blah.blash/event/backchannel-logout": struct{}{}, + }, + }), signKey: newRSAKey(t), wantErr: true, }, { name: "No Event string", - logoutToken: ` { - "iss": "https://foo", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "iat": 1471566154, - "jti": "bWJq", - "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", - }`, + logoutToken: jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": "https://foo", + "sub": "248289761001", + "aud": "s6BhdRkqt3", + "iat": 1471566154, + "jti": "bWJq", + "sid": "08a5019c-17e1-4977-8f42-65a12843ea02", + }), signKey: newRSAKey(t), wantErr: true, }, @@ -165,7 +137,7 @@ type logoutVerificationTest struct { issuer string // JWT payload (just the claims). - logoutToken string + logoutToken *jwt.Token // Key to sign the ID Token with. signKey *signingKey @@ -173,40 +145,31 @@ type logoutVerificationTest struct { // testing invalid signatures. verificationKey *signingKey - config goidc.Config wantErr bool } -type testVerifier struct { - jwk jose.JSONWebKey -} - -func (t *testVerifier) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { - jws, err := jose.ParseSigned(jwt) - if err != nil { - return nil, fmt.Errorf("oidc: malformed jwt: %v", err) - } - return jws.Verify(&t.jwk) -} - func (v logoutVerificationTest) runGetToken(t *testing.T) (*oidc.LogoutToken, error) { - token := v.signKey.sign(t, []byte(v.logoutToken)) + // token := v.signKey.sign(t, []byte(v.logoutToken)) + v.logoutToken.Header["kid"] = "1" + token, err := v.logoutToken.SignedString(v.signKey.priv) + if err != nil { + t.Fatal(err) + } ctx, cancel := context.WithCancel(context.Background()) defer cancel() issuer := "https://foo" - var ks goidc.KeySet + var jwks *keyfunc.JWKS if v.verificationKey == nil { - ks = &testVerifier{v.signKey.jwk()} + jwks = v.signKey.jwks } else { - ks = &testVerifier{v.verificationKey.jwk()} + jwks = v.verificationKey.jwks } pm := oidc.ProviderMetadata{} verifier := oidc.NewOIDCClient( oidc.WithOidcIssuer(issuer), - oidc.WithKeySet(ks), - oidc.WithConfig(&v.config), + oidc.WithJWKS(jwks), oidc.WithProviderMetadata(&pm), ) @@ -228,13 +191,15 @@ func newRSAKey(t testing.TB) *signingKey { if err != nil { t.Fatal(err) } - return &signingKey{"", priv, priv.Public(), jose.RS256} -} + givenKey := keyfunc.NewGivenRSA( + &priv.PublicKey, + keyfunc.GivenKeyOptions{Algorithm: jwt.SigningMethodRS256.Alg()}, + ) + jwks := keyfunc.NewGiven( + map[string]keyfunc.GivenKey{ + "1": givenKey, + }, + ) -func newECDSAKey(t *testing.T) *signingKey { - priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) - if err != nil { - t.Fatal(err) - } - return &signingKey{"", priv, priv.Public(), jose.ES256} + return &signingKey{priv, jwks} } diff --git a/ocis-pkg/oidc/metadata.go b/ocis-pkg/oidc/metadata.go index 2d2708a0f3..662d9ec522 100644 --- a/ocis-pkg/oidc/metadata.go +++ b/ocis-pkg/oidc/metadata.go @@ -59,35 +59,12 @@ type ProviderMetadata struct { // Logout Token defines an logout Token type LogoutToken struct { - // The URL of the server which issued this token. OpenID Connect - // requires this value always be identical to the URL used for - // initial discovery. - // - // Note: Because of a known issue with Google Accounts' implementation - // this value may differ when using Google. - // - // See: https://developers.google.com/identity/protocols/OpenIDConnect#obtainuserinfo - Issuer string `json:"iss"` // example "https://server.example.com" - - // A unique string which identifies the end user. - Subject string `json:"sub"` //"248289761001" - - // The client ID, or set of client IDs, that this token is issued for. For - // common uses, this is the client that initialized the auth flow. - // - // This package ensures the audience contains an expected value. - Audience jwt.ClaimStrings `json:"aud"` // "s6BhdRkqt3" - - // When the token was issued by the provider. - IssuedAt *jwt.NumericDate `json:"iat"` - + jwt.RegisteredClaims // The Session Id - SessionId string `json:"sid"` - - Events LogoutEvent `json:"events"` - - // Jwt Id - JwtID string `json:"jti"` + SessionId string `json:"sid"` + Events LogoutEvent `json:"events"` + // Note: This is just here to be able to check for nonce being absent + Nonce *string `json:"nonce"` } // LogoutEvent defines a logout Event diff --git a/ocis-pkg/oidc/options.go b/ocis-pkg/oidc/options.go index 149433ebaf..fbfc5fe7a9 100644 --- a/ocis-pkg/oidc/options.go +++ b/ocis-pkg/oidc/options.go @@ -3,6 +3,7 @@ package oidc import ( "net/http" + "github.com/MicahParks/keyfunc/v2" "github.com/owncloud/ocis/v2/ocis-pkg/log" "github.com/owncloud/ocis/v2/services/proxy/pkg/config" @@ -22,7 +23,14 @@ type Options struct { OIDCIssuer string // JWKSOptions to use when retrieving keys JWKSOptions config.JWKS - // KeySet to use when verifiing signatures + // the JWKS keyset to use for verifying signatures of Access- and + // Logout-Tokens + // this option is mostly needed for unit test. To avoid fetching the keys + // from the issuer + JWKS *keyfunc.JWKS + // KeySet to use when verifiing signatures of jwt encoded + // user info responses + // TODO move userinfo verification to use jwt/keyfunc as well KeySet KeySet // AccessTokenVerifyMethod to use when verifying access tokens // TODO pass a function or interface to verify? an AccessTokenVerifier? @@ -80,6 +88,13 @@ func WithJWKSOptions(val config.JWKS) Option { } } +// WithJWKS provides a function to set the JWKS option (mainly useful for testing). +func WithJWKS(val *keyfunc.JWKS) Option { + return func(o *Options) { + o.JWKS = val + } +} + // WithKeySet provides a function to set the KeySet option. func WithKeySet(val KeySet) Option { return func(o *Options) {