From e2730eecd59689c0f67c5b98f4b73bf0544591f1 Mon Sep 17 00:00:00 2001 From: hijiki51 Date: Sat, 23 Sep 2023 13:28:25 +0900 Subject: [PATCH 1/2] fix: decode id_token_hint with correct signer BREAKING CHANGE: this commit changed OpenIDConnectTokenStrategy interface. closes #769 --- compose/compose_openid.go | 7 +++---- handler/openid/flow_explicit_auth_test.go | 2 +- handler/openid/flow_hybrid_test.go | 2 +- handler/openid/flow_implicit_test.go | 2 +- handler/openid/strategy.go | 2 ++ handler/openid/strategy_jwt.go | 13 +++++++++++++ handler/openid/validator.go | 14 ++++---------- internal/id_token_strategy.go | 17 ++++++++++++++++- 8 files changed, 41 insertions(+), 18 deletions(-) diff --git a/compose/compose_openid.go b/compose/compose_openid.go index 29b1d3a7..49c7bcd0 100644 --- a/compose/compose_openid.go +++ b/compose/compose_openid.go @@ -7,7 +7,6 @@ import ( "github.com/ory/fosite" "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" - "github.com/ory/fosite/token/jwt" ) // OpenIDConnectExplicitFactory creates an OpenID Connect explicit ("authorize code flow") grant handler. @@ -19,7 +18,7 @@ func OpenIDConnectExplicitFactory(config fosite.Configurator, storage interface{ IDTokenHandleHelper: &openid.IDTokenHandleHelper{ IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy), }, - OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(jwt.Signer), config), + OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(openid.OpenIDConnectTokenStrategy), config), Config: config, } } @@ -50,7 +49,7 @@ func OpenIDConnectImplicitFactory(config fosite.Configurator, storage interface{ IDTokenHandleHelper: &openid.IDTokenHandleHelper{ IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy), }, - OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(jwt.Signer), config), + OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(openid.OpenIDConnectTokenStrategy), config), } } @@ -76,6 +75,6 @@ func OpenIDConnectHybridFactory(config fosite.Configurator, storage interface{}, IDTokenStrategy: strategy.(openid.OpenIDConnectTokenStrategy), }, OpenIDConnectRequestStorage: storage.(openid.OpenIDConnectRequestStorage), - OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(jwt.Signer), config), + OpenIDConnectRequestValidator: openid.NewOpenIDConnectRequestValidator(strategy.(openid.OpenIDConnectTokenStrategy), config), } } diff --git a/handler/openid/flow_explicit_auth_test.go b/handler/openid/flow_explicit_auth_test.go index 2373444b..594d5363 100644 --- a/handler/openid/flow_explicit_auth_test.go +++ b/handler/openid/flow_explicit_auth_test.go @@ -40,7 +40,7 @@ func makeOpenIDConnectExplicitHandler(ctrl *gomock.Controller, minParameterEntro IDTokenHandleHelper: &IDTokenHandleHelper{ IDTokenStrategy: j, }, - OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j.Signer, config), + OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j, config), Config: config, }, store } diff --git a/handler/openid/flow_hybrid_test.go b/handler/openid/flow_hybrid_test.go index 943fa665..1d856294 100644 --- a/handler/openid/flow_hybrid_test.go +++ b/handler/openid/flow_hybrid_test.go @@ -79,7 +79,7 @@ func makeOpenIDConnectHybridHandler(minParameterEntropy int) OpenIDConnectHybrid IDTokenStrategy: idStrategy, }, Config: config, - OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j.Signer, config), + OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j, config), OpenIDConnectRequestStorage: storage.NewMemoryStore(), } } diff --git a/handler/openid/flow_implicit_test.go b/handler/openid/flow_implicit_test.go index baf282c9..ab661165 100644 --- a/handler/openid/flow_implicit_test.go +++ b/handler/openid/flow_implicit_test.go @@ -56,7 +56,7 @@ func makeOpenIDConnectImplicitHandler(minParameterEntropy int) OpenIDConnectImpl IDTokenHandleHelper: &IDTokenHandleHelper{ IDTokenStrategy: idStrategy, }, - OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j.Signer, config), + OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j, config), Config: config, } } diff --git a/handler/openid/strategy.go b/handler/openid/strategy.go index f5353d0b..e0be795a 100644 --- a/handler/openid/strategy.go +++ b/handler/openid/strategy.go @@ -8,8 +8,10 @@ import ( "time" "github.com/ory/fosite" + "github.com/ory/fosite/token/jwt" ) type OpenIDConnectTokenStrategy interface { GenerateIDToken(ctx context.Context, lifespan time.Duration, requester fosite.Requester) (token string, err error) + DecodeIDToken(ctx context.Context, requester fosite.Requester, token string) (*jwt.Token, error) } diff --git a/handler/openid/strategy_jwt.go b/handler/openid/strategy_jwt.go index ecd75d8f..a8d515d4 100644 --- a/handler/openid/strategy_jwt.go +++ b/handler/openid/strategy_jwt.go @@ -231,3 +231,16 @@ func (h DefaultStrategy) GenerateIDToken(ctx context.Context, lifespan time.Dura token, _, err = h.Signer.Generate(ctx, claims.ToMapClaims(), sess.IDTokenHeaders()) return token, err } + +// DecodeIDToken decodes a JWT string. +func (h DefaultStrategy) DecodeIDToken(ctx context.Context, _ fosite.Requester, token string) (*jwt.Token, error) { + idtoken, err := h.Signer.Decode(ctx, token) + var ve *jwt.ValidationError + if errors.As(err, &ve) && ve.Has(jwt.ValidationErrorExpired) { + // Expired tokens are ok + } else if err != nil { + return nil, errorsx.WithStack(fosite.ErrServerError.WithWrap(err).WithDebug(err.Error())) + } + + return idtoken, nil +} diff --git a/handler/openid/validator.go b/handler/openid/validator.go index f2531494..fdc10897 100644 --- a/handler/openid/validator.go +++ b/handler/openid/validator.go @@ -11,10 +11,7 @@ import ( "github.com/ory/x/errorsx" - "github.com/pkg/errors" - "github.com/ory/fosite" - "github.com/ory/fosite/token/jwt" "github.com/ory/go-convenience/stringslice" ) @@ -26,11 +23,11 @@ type openIDConnectRequestValidatorConfigProvider interface { } type OpenIDConnectRequestValidator struct { - Strategy jwt.Signer + Strategy OpenIDConnectTokenStrategy Config openIDConnectRequestValidatorConfigProvider } -func NewOpenIDConnectRequestValidator(strategy jwt.Signer, config openIDConnectRequestValidatorConfigProvider) *OpenIDConnectRequestValidator { +func NewOpenIDConnectRequestValidator(strategy OpenIDConnectTokenStrategy, config openIDConnectRequestValidatorConfigProvider) *OpenIDConnectRequestValidator { return &OpenIDConnectRequestValidator{ Strategy: strategy, Config: config, @@ -133,11 +130,8 @@ func (v *OpenIDConnectRequestValidator) ValidatePrompt(ctx context.Context, req return nil } - tokenHint, err := v.Strategy.Decode(ctx, idTokenHint) - var ve *jwt.ValidationError - if errors.As(err, &ve) && ve.Has(jwt.ValidationErrorExpired) { - // Expired tokens are ok - } else if err != nil { + tokenHint, err := v.Strategy.DecodeIDToken(ctx, req, idTokenHint) + if err != nil { return errorsx.WithStack(fosite.ErrInvalidRequest.WithHint("Failed to validate OpenID Connect request as decoding id token from id_token_hint parameter failed.").WithWrap(err).WithDebug(err.Error())) } diff --git a/internal/id_token_strategy.go b/internal/id_token_strategy.go index 330adeae..1359b1dc 100644 --- a/internal/id_token_strategy.go +++ b/internal/id_token_strategy.go @@ -13,8 +13,8 @@ import ( time "time" gomock "github.com/golang/mock/gomock" - fosite "github.com/ory/fosite" + jwt "github.com/ory/fosite/token/jwt" ) // MockOpenIDConnectTokenStrategy is a mock of OpenIDConnectTokenStrategy interface. @@ -40,6 +40,21 @@ func (m *MockOpenIDConnectTokenStrategy) EXPECT() *MockOpenIDConnectTokenStrateg return m.recorder } +// DecodeIDToken mocks base method. +func (m *MockOpenIDConnectTokenStrategy) DecodeIDToken(arg0 context.Context, arg1 fosite.Requester, arg2 string) (*jwt.Token, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DecodeIDToken", arg0, arg1, arg2) + ret0, _ := ret[0].(*jwt.Token) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DecodeIDToken indicates an expected call of DecodeIDToken. +func (mr *MockOpenIDConnectTokenStrategyMockRecorder) DecodeIDToken(arg0, arg1, arg2 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DecodeIDToken", reflect.TypeOf((*MockOpenIDConnectTokenStrategy)(nil).DecodeIDToken), arg0, arg1, arg2) +} + // GenerateIDToken mocks base method. func (m *MockOpenIDConnectTokenStrategy) GenerateIDToken(arg0 context.Context, arg1 time.Duration, arg2 fosite.Requester) (string, error) { m.ctrl.T.Helper() From 263e457b95c0fbe91f818323fc14018f563305fa Mon Sep 17 00:00:00 2001 From: hijiki51 Date: Sat, 23 Sep 2023 14:17:00 +0900 Subject: [PATCH 2/2] test: add test for DecodeIDToken --- handler/openid/strategy_jwt_test.go | 85 +++++++++++++++++++++++++++++ internal/id_token_strategy.go | 1 + 2 files changed, 86 insertions(+) diff --git a/handler/openid/strategy_jwt_test.go b/handler/openid/strategy_jwt_test.go index fff45519..42d9c64d 100644 --- a/handler/openid/strategy_jwt_test.go +++ b/handler/openid/strategy_jwt_test.go @@ -10,8 +10,10 @@ import ( "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/ory/fosite" + "github.com/ory/fosite/internal/gen" "github.com/ory/fosite/token/jwt" ) @@ -283,3 +285,86 @@ func TestJWTStrategy_GenerateIDToken(t *testing.T) { }) } } + +func TestJWTStrategy_DecodeIDToken(t *testing.T) { + var j = &DefaultStrategy{ + Signer: &jwt.DefaultSigner{ + GetPrivateKey: func(_ context.Context) (interface{}, error) { + return key, nil + }}, + Config: &fosite.Config{ + MinParameterEntropy: fosite.MinParameterEntropy, + }, + } + + var anotherKey = gen.MustRSAKey() + + var genIDToken = func(c jwt.IDTokenClaims) string { + s, _, err := j.Generate(context.TODO(), c.ToMapClaims(), jwt.NewHeaders()) + require.NoError(t, err) + return s + } + + var token string + var decoder *DefaultStrategy + for k, c := range []struct { + description string + setup func() + expectErr bool + }{ + { + description: "should pass with valid token", + setup: func() { + token = genIDToken(jwt.IDTokenClaims{ + Subject: "peter", + RequestedAt: time.Now(), + ExpiresAt: time.Now().Add(time.Hour), + }) + decoder = j + }, + expectErr: false, + }, + { + description: "should pass even though token is expired", + setup: func() { + token = genIDToken(jwt.IDTokenClaims{ + Subject: "peter", + RequestedAt: time.Now(), + ExpiresAt: time.Now().Add(-time.Hour), + }) + decoder = j + }, + expectErr: false, + }, + { + description: "should fail because token is decoded with wrong key", + setup: func() { + token = genIDToken(jwt.IDTokenClaims{ + Subject: "peter", + RequestedAt: time.Now(), + ExpiresAt: time.Now().Add(time.Hour), + }) + decoder = &DefaultStrategy{ + Signer: &jwt.DefaultSigner{ + GetPrivateKey: func(_ context.Context) (interface{}, error) { + return anotherKey, nil + }}, + Config: &fosite.Config{ + MinParameterEntropy: fosite.MinParameterEntropy, + }, + } + }, + expectErr: true, + }, + } { + t.Run(fmt.Sprintf("case=%d/description=%s", k, c.description), func(t *testing.T) { + c.setup() + req := fosite.NewAccessRequest(&DefaultSession{}) + idtoken, err := decoder.DecodeIDToken(context.Background(), req, token) + assert.Equal(t, c.expectErr, err != nil, "%d: %+v", k, err) + if !c.expectErr { + assert.NotNil(t, idtoken) + } + }) + } +} diff --git a/internal/id_token_strategy.go b/internal/id_token_strategy.go index 1359b1dc..c8c13716 100644 --- a/internal/id_token_strategy.go +++ b/internal/id_token_strategy.go @@ -13,6 +13,7 @@ import ( time "time" gomock "github.com/golang/mock/gomock" + fosite "github.com/ory/fosite" jwt "github.com/ory/fosite/token/jwt" )