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

@@ -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

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
-}

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}
 }

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

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) {