From f8b00daa1adb1f3663acadfcaaa6d611b930f376 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Fri, 19 Jan 2024 11:57:11 +0200 Subject: [PATCH] fix: allow expired ID token hint to end sessions This change adds a specific error for expired ID Token hints, including too old "issued at" and "max auth age". The error is returned VerifyIDTokenHint so that the end session handler can choose to ignore this error. This fixes the behavior to be in line with [OpenID Connect RP-Initiated Logout 1.0, section 4](https://openid.net/specs/openid-connect-rpinitiated-1_0.html#ValidationAndErrorHandling). --- pkg/oidc/verifier.go | 2 +- pkg/op/session.go | 3 +- pkg/op/verifier_id_token_hint.go | 36 ++++++++++---- pkg/op/verifier_id_token_hint_test.go | 71 ++++++++++++++------------- 4 files changed, 66 insertions(+), 46 deletions(-) diff --git a/pkg/oidc/verifier.go b/pkg/oidc/verifier.go index 42fbb20..fe28857 100644 --- a/pkg/oidc/verifier.go +++ b/pkg/oidc/verifier.go @@ -57,7 +57,7 @@ var ( ErrNonceInvalid = errors.New("nonce does not match") ErrAcrInvalid = errors.New("acr is invalid") ErrAuthTimeNotPresent = errors.New("claim `auth_time` of token is missing") - ErrAuthTimeToOld = errors.New("auth time of token is to old") + ErrAuthTimeToOld = errors.New("auth time of token is too old") ErrAtHash = errors.New("at_hash does not correspond to access token") ) diff --git a/pkg/op/session.go b/pkg/op/session.go index c33627f..c933659 100644 --- a/pkg/op/session.go +++ b/pkg/op/session.go @@ -2,6 +2,7 @@ package op import ( "context" + "errors" "net/http" "net/url" "path" @@ -68,7 +69,7 @@ func ValidateEndSessionRequest(ctx context.Context, req *oidc.EndSessionRequest, } if req.IdTokenHint != "" { claims, err := VerifyIDTokenHint[*oidc.IDTokenClaims](ctx, req.IdTokenHint, ender.IDTokenHintVerifier(ctx)) - if err != nil { + if err != nil && !errors.As(err, &IDTokenHintExpiredError{}) { return nil, oidc.ErrInvalidRequest().WithDescription("id_token_hint invalid").WithParent(err) } session.UserID = claims.GetSubject() diff --git a/pkg/op/verifier_id_token_hint.go b/pkg/op/verifier_id_token_hint.go index 6143252..b5ec72e 100644 --- a/pkg/op/verifier_id_token_hint.go +++ b/pkg/op/verifier_id_token_hint.go @@ -2,6 +2,7 @@ package op import ( "context" + "errors" "github.com/zitadel/oidc/v3/pkg/oidc" ) @@ -27,8 +28,23 @@ func NewIDTokenHintVerifier(issuer string, keySet oidc.KeySet, opts ...IDTokenHi return verifier } +type IDTokenHintExpiredError struct { + error +} + +func (e IDTokenHintExpiredError) Unwrap() error { + return e.error +} + +func (e IDTokenHintExpiredError) Is(err error) bool { + return errors.Is(err, e.error) +} + // VerifyIDTokenHint validates the id token according to -// https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation +// https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation. +// In case of an expired token both the Claims and first encountered expiry related error +// is returned of type [IDTokenHintExpiredError]. In that case the caller can choose to still +// trust the token for cases like logout, as signature and other verifications succeeded. func VerifyIDTokenHint[C oidc.Claims](ctx context.Context, token string, v *IDTokenHintVerifier) (claims C, err error) { var nilClaims C @@ -49,20 +65,20 @@ func VerifyIDTokenHint[C oidc.Claims](ctx context.Context, token string, v *IDTo return nilClaims, err } - if err = oidc.CheckExpiration(claims, v.Offset); err != nil { - return nilClaims, err - } - - if err = oidc.CheckIssuedAt(claims, v.MaxAgeIAT, v.Offset); err != nil { - return nilClaims, err - } - if err = oidc.CheckAuthorizationContextClassReference(claims, v.ACR); err != nil { return nilClaims, err } + if err = oidc.CheckExpiration(claims, v.Offset); err != nil { + return claims, IDTokenHintExpiredError{err} + } + + if err = oidc.CheckIssuedAt(claims, v.MaxAgeIAT, v.Offset); err != nil { + return claims, IDTokenHintExpiredError{err} + } + if err = oidc.CheckAuthTime(claims, v.MaxAge); err != nil { - return nilClaims, err + return claims, IDTokenHintExpiredError{err} } return claims, nil } diff --git a/pkg/op/verifier_id_token_hint_test.go b/pkg/op/verifier_id_token_hint_test.go index e514a76..5d7f7a1 100644 --- a/pkg/op/verifier_id_token_hint_test.go +++ b/pkg/op/verifier_id_token_hint_test.go @@ -71,21 +71,23 @@ func TestVerifyIDTokenHint(t *testing.T) { tests := []struct { name string tokenClaims func() (string, *oidc.IDTokenClaims) - wantErr bool + wantClaims bool + wantErr error }{ { name: "success", tokenClaims: tu.ValidIDToken, + wantClaims: true, }, { name: "parse err", tokenClaims: func() (string, *oidc.IDTokenClaims) { return "~~~~", nil }, - wantErr: true, + wantErr: oidc.ErrParse, }, { name: "invalid signature", tokenClaims: func() (string, *oidc.IDTokenClaims) { return tu.InvalidSignatureToken, nil }, - wantErr: true, + wantErr: oidc.ErrSignatureUnsupportedAlg, }, { name: "wrong issuer", @@ -96,29 +98,7 @@ func TestVerifyIDTokenHint(t *testing.T) { tu.ValidACR, tu.ValidAMR, tu.ValidClientID, tu.ValidSkew, "", ) }, - wantErr: true, - }, - { - name: "expired", - tokenClaims: func() (string, *oidc.IDTokenClaims) { - return tu.NewIDToken( - tu.ValidIssuer, tu.ValidSubject, tu.ValidAudience, - tu.ValidExpiration.Add(-time.Hour), tu.ValidAuthTime, tu.ValidNonce, - tu.ValidACR, tu.ValidAMR, tu.ValidClientID, tu.ValidSkew, "", - ) - }, - wantErr: true, - }, - { - name: "wrong IAT", - tokenClaims: func() (string, *oidc.IDTokenClaims) { - return tu.NewIDToken( - tu.ValidIssuer, tu.ValidSubject, tu.ValidAudience, - tu.ValidExpiration, tu.ValidAuthTime, tu.ValidNonce, - tu.ValidACR, tu.ValidAMR, tu.ValidClientID, -time.Hour, "", - ) - }, - wantErr: true, + wantErr: oidc.ErrIssuerInvalid, }, { name: "wrong acr", @@ -129,7 +109,31 @@ func TestVerifyIDTokenHint(t *testing.T) { "else", tu.ValidAMR, tu.ValidClientID, tu.ValidSkew, "", ) }, - wantErr: true, + wantErr: oidc.ErrAcrInvalid, + }, + { + name: "expired", + tokenClaims: func() (string, *oidc.IDTokenClaims) { + return tu.NewIDToken( + tu.ValidIssuer, tu.ValidSubject, tu.ValidAudience, + tu.ValidExpiration.Add(-time.Hour), tu.ValidAuthTime, tu.ValidNonce, + tu.ValidACR, tu.ValidAMR, tu.ValidClientID, tu.ValidSkew, "", + ) + }, + wantClaims: true, + wantErr: IDTokenHintExpiredError{oidc.ErrExpired}, + }, + { + name: "IAT too old", + tokenClaims: func() (string, *oidc.IDTokenClaims) { + return tu.NewIDToken( + tu.ValidIssuer, tu.ValidSubject, tu.ValidAudience, + tu.ValidExpiration, tu.ValidAuthTime, tu.ValidNonce, + tu.ValidACR, tu.ValidAMR, tu.ValidClientID, time.Hour, "", + ) + }, + wantClaims: true, + wantErr: IDTokenHintExpiredError{oidc.ErrIatToOld}, }, { name: "expired auth", @@ -140,7 +144,8 @@ func TestVerifyIDTokenHint(t *testing.T) { tu.ValidACR, tu.ValidAMR, tu.ValidClientID, tu.ValidSkew, "", ) }, - wantErr: true, + wantClaims: true, + wantErr: IDTokenHintExpiredError{oidc.ErrAuthTimeToOld}, }, } for _, tt := range tests { @@ -148,14 +153,12 @@ func TestVerifyIDTokenHint(t *testing.T) { token, want := tt.tokenClaims() got, err := VerifyIDTokenHint[*oidc.IDTokenClaims](context.Background(), token, verifier) - if tt.wantErr { - assert.Error(t, err) - assert.Nil(t, got) + require.ErrorIs(t, err, tt.wantErr) + if tt.wantClaims { + assert.Equal(t, got, want, "claims") return } - require.NoError(t, err) - require.NotNil(t, got) - assert.Equal(t, got, want) + assert.Nil(t, got, "claims") }) } }