Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: decode id_token_hint with correct signer #770

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions compose/compose_openid.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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,
}
}
Expand Down Expand Up @@ -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),
}
}

Expand All @@ -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),
}
}
2 changes: 1 addition & 1 deletion handler/openid/flow_explicit_auth_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion handler/openid/flow_hybrid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
}
}
Expand Down
2 changes: 1 addition & 1 deletion handler/openid/flow_implicit_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func makeOpenIDConnectImplicitHandler(minParameterEntropy int) OpenIDConnectImpl
IDTokenHandleHelper: &IDTokenHandleHelper{
IDTokenStrategy: idStrategy,
},
OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j.Signer, config),
OpenIDConnectRequestValidator: NewOpenIDConnectRequestValidator(j, config),
Config: config,
}
}
Expand Down
2 changes: 2 additions & 0 deletions handler/openid/strategy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
13 changes: 13 additions & 0 deletions handler/openid/strategy_jwt.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
85 changes: 85 additions & 0 deletions handler/openid/strategy_jwt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
})
}
}
14 changes: 4 additions & 10 deletions handler/openid/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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,
Expand Down Expand Up @@ -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()))
}

Expand Down
16 changes: 16 additions & 0 deletions internal/id_token_strategy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading