From b4baf9e757c0fbf914c311cd64cedd149a699adc Mon Sep 17 00:00:00 2001 From: Matej Buday Date: Wed, 9 Jun 2021 16:20:14 +0200 Subject: [PATCH] feat: add client secret rotation support add new interface and implement it for --- README.md | 2 +- client.go | 28 +++++++++++----- client_authentication.go | 22 ++++++++++++- client_authentication_test.go | 47 +++++++++++++++++++++++++++ client_test.go | 16 +++++---- internal/client.go | 14 ++++++++ introspection_request_handler.go | 2 +- introspection_request_handler_test.go | 18 ++++++++++ storage/memory.go | 26 ++++++++------- 9 files changed, 146 insertions(+), 29 deletions(-) diff --git a/README.md b/README.md index dea14ba07..f92ef52df 100644 --- a/README.md +++ b/README.md @@ -227,7 +227,7 @@ import "github.com/ory/fosite/compose" import "github.com/ory/fosite/storage" // This is the example storage that contains: -// * an OAuth2 Client with id "my-client" and secret "foobar" capable of all oauth2 and open id connect grant and response types. +// * an OAuth2 Client with id "my-client" and secrets "foobar" and "foobaz" capable of all oauth2 and open id connect grant and response types. // * a User for the resource owner password credentials grant type with username "peter" and password "secret". // // You will most likely replace this with your own logic once you set up a real world application. diff --git a/client.go b/client.go index c7cf6c5c6..5d7d69cca 100644 --- a/client.go +++ b/client.go @@ -52,6 +52,13 @@ type Client interface { GetAudience() Arguments } +// ClientWithSecretRotation extends Client interface by a method providing a slice of rotated secrets. +type ClientWithSecretRotation interface { + Client + // GetRotatedHashes returns a slice of hashed secrets used for secrets rotation. + GetRotatedHashes() [][]byte +} + // OpenIDConnectClient represents a client capable of performing OpenID Connect requests. type OpenIDConnectClient interface { // GetRequestURIs is an array of request_uri values that are pre-registered by the RP for use at the OP. Servers MAY @@ -88,14 +95,15 @@ type ResponseModeClient interface { // DefaultClient is a simple default implementation of the Client interface. type DefaultClient struct { - ID string `json:"id"` - Secret []byte `json:"client_secret,omitempty"` - RedirectURIs []string `json:"redirect_uris"` - GrantTypes []string `json:"grant_types"` - ResponseTypes []string `json:"response_types"` - Scopes []string `json:"scopes"` - Audience []string `json:"audience"` - Public bool `json:"public"` + ID string `json:"id"` + Secret []byte `json:"client_secret,omitempty"` + RotatedSecrets [][]byte `json:"rotated_secrets,omitempty"` + RedirectURIs []string `json:"redirect_uris"` + GrantTypes []string `json:"grant_types"` + ResponseTypes []string `json:"response_types"` + Scopes []string `json:"scopes"` + Audience []string `json:"audience"` + Public bool `json:"public"` } type DefaultOpenIDConnectClient struct { @@ -133,6 +141,10 @@ func (c *DefaultClient) GetHashedSecret() []byte { return c.Secret } +func (c *DefaultClient) GetRotatedHashes() [][]byte { + return c.RotatedSecrets +} + func (c *DefaultClient) GetScopes() Arguments { return c.Scopes } diff --git a/client_authentication.go b/client_authentication.go index 8e611cc1c..d6dc2682d 100644 --- a/client_authentication.go +++ b/client_authentication.go @@ -244,13 +244,33 @@ func (f *Fosite) DefaultClientAuthenticationStrategy(ctx context.Context, r *htt } // Enforce client authentication - if err := f.Hasher.Compare(ctx, client.GetHashedSecret(), []byte(clientSecret)); err != nil { + if err := f.checkClientSecret(ctx, client, []byte(clientSecret)); err != nil { return nil, errorsx.WithStack(ErrInvalidClient.WithWrap(err).WithDebug(err.Error())) } return client, nil } +func (f *Fosite) checkClientSecret(ctx context.Context, client Client, clientSecret []byte) error { + var err error + err = f.Hasher.Compare(ctx, client.GetHashedSecret(), clientSecret) + if err == nil { + return nil + } + cc, ok := client.(ClientWithSecretRotation) + if !ok { + return err + } + for _, hash := range cc.GetRotatedHashes() { + err = f.Hasher.Compare(ctx, hash, clientSecret) + if err == nil { + return nil + } + } + + return err +} + func findPublicKey(t *jwt.Token, set *jose.JSONWebKeySet, expectsRSAKey bool) (interface{}, error) { keys := set.Keys if len(keys) == 0 { diff --git a/client_authentication_test.go b/client_authentication_test.go index bf232ad66..63c7f0bd4 100644 --- a/client_authentication_test.go +++ b/client_authentication_test.go @@ -183,6 +183,12 @@ func TestAuthenticateClient(t *testing.T) { }, { d: "should pass because client is confidential and id and secret match in post body", + client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: []byte("invalid_hash"), RotatedSecrets: [][]byte{barSecret}}, TokenEndpointAuthMethod: "client_secret_post"}, + form: url.Values{"client_id": []string{"foo"}, "client_secret": []string{"bar"}}, + r: new(http.Request), + }, + { + d: "should pass because client is confidential and id and rotated secret match in post body", client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: barSecret}, TokenEndpointAuthMethod: "client_secret_post"}, form: url.Values{"client_id": []string{"foo"}, "client_secret": []string{"bar"}}, r: new(http.Request), @@ -207,6 +213,18 @@ func TestAuthenticateClient(t *testing.T) { form: url.Values{}, r: &http.Request{Header: clientBasicAuthHeader("foo", "bar")}, }, + { + d: "should pass because client is confidential and id and rotated secret match in header", + client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: []byte("invalid_hash"), RotatedSecrets: [][]byte{barSecret}}, TokenEndpointAuthMethod: "client_secret_basic"}, + form: url.Values{}, + r: &http.Request{Header: clientBasicAuthHeader("foo", "bar")}, + }, + { + d: "should pass because client is confidential and id and rotated secret match in header", + client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: []byte("invalid_hash"), RotatedSecrets: [][]byte{[]byte("invalid"), barSecret}}, TokenEndpointAuthMethod: "client_secret_basic"}, + form: url.Values{}, + r: &http.Request{Header: clientBasicAuthHeader("foo", "bar")}, + }, { d: "should fail because auth method is not client_secret_basic", client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: barSecret}, TokenEndpointAuthMethod: "client_secret_post"}, @@ -221,6 +239,13 @@ func TestAuthenticateClient(t *testing.T) { r: &http.Request{Header: clientBasicAuthHeader("foo", "baz")}, expectErr: ErrInvalidClient, }, + { + d: "should fail because client is confidential and neither secret nor rotated does match in header", + client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: barSecret, RotatedSecrets: [][]byte{barSecret}}, TokenEndpointAuthMethod: "client_secret_basic"}, + form: url.Values{}, + r: &http.Request{Header: clientBasicAuthHeader("foo", "baz")}, + expectErr: ErrInvalidClient, + }, { d: "should fail because client id is not encoded using application/x-www-form-urlencoded", client: &DefaultOpenIDConnectClient{DefaultClient: &DefaultClient{ID: "foo", Secret: barSecret}, TokenEndpointAuthMethod: "client_secret_basic"}, @@ -512,6 +537,28 @@ func TestAuthenticateClient(t *testing.T) { } } +// func TestCheckClientSecret(t *testing.T) { +// hasher := &BCrypt{WorkFactor: 6} +// f := &Fosite{ +// JWKSFetcherStrategy: NewDefaultJWKSFetcherStrategy(), +// Store: storage.NewMemoryStore(), +// Hasher: hasher, +// TokenURL: "token-url", +// } + +// for k, tc := range []struct { +// d string +// client *DefaultOpenIDConnectClient +// secret []byte +// err error +// }{} { +// t.Run(fmt.Sprintf("case=%d/description=%s", k, tc.d), func(t *testing.T) { +// err := f.checkClientSecret(context.Background(), tc.client, tc.secret) +// assert.Equal(t, tc.err, err) +// }) +// } +// } + func TestAuthenticateClientTwice(t *testing.T) { const at = "urn:ietf:params:oauth:client-assertion-type:jwt-bearer" diff --git a/client_test.go b/client_test.go index baf07d588..c2b87a491 100644 --- a/client_test.go +++ b/client_test.go @@ -29,17 +29,19 @@ import ( func TestDefaultClient(t *testing.T) { sc := &DefaultClient{ - ID: "1", - Secret: []byte("foobar-"), - RedirectURIs: []string{"foo", "bar"}, - ResponseTypes: []string{"foo", "bar"}, - GrantTypes: []string{"foo", "bar"}, - Scopes: []string{"fooscope"}, + ID: "1", + Secret: []byte("foobar-"), + RotatedSecrets: [][]byte{[]byte("foobar-1"), []byte("foobar-2")}, + RedirectURIs: []string{"foo", "bar"}, + ResponseTypes: []string{"foo", "bar"}, + GrantTypes: []string{"foo", "bar"}, + Scopes: []string{"fooscope"}, } assert.Equal(t, sc.ID, sc.GetID()) assert.Equal(t, sc.RedirectURIs, sc.GetRedirectURIs()) assert.Equal(t, sc.Secret, sc.GetHashedSecret()) + assert.Equal(t, sc.RotatedSecrets, sc.GetRotatedHashes()) assert.EqualValues(t, sc.ResponseTypes, sc.GetResponseTypes()) assert.EqualValues(t, sc.GrantTypes, sc.GetGrantTypes()) assert.EqualValues(t, sc.Scopes, sc.GetScopes()) @@ -48,6 +50,8 @@ func TestDefaultClient(t *testing.T) { sc.ResponseTypes = []string{} assert.Equal(t, "code", sc.GetResponseTypes()[0]) assert.Equal(t, "authorization_code", sc.GetGrantTypes()[0]) + + var _ ClientWithSecretRotation = sc } func TestDefaultResponseModeClient_GetResponseMode(t *testing.T) { diff --git a/internal/client.go b/internal/client.go index 73f721003..f9fc9c425 100644 --- a/internal/client.go +++ b/internal/client.go @@ -77,6 +77,20 @@ func (mr *MockClientMockRecorder) GetHashedSecret() *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetHashedSecret", reflect.TypeOf((*MockClient)(nil).GetHashedSecret)) } +// GetHashedSecret mocks base method +func (m *MockClient) GetRotatedHashes() [][]byte { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetRotatedHashes") + ret0, _ := ret[0].([][]byte) + return ret0 +} + +// GetHashedSecret indicates an expected call of GetHashedSecret +func (mr *MockClientMockRecorder) GetRotatedHashes() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetRotatedHashes", reflect.TypeOf((*MockClient)(nil).GetRotatedHashes)) +} + // GetID mocks base method func (m *MockClient) GetID() string { m.ctrl.T.Helper() diff --git a/introspection_request_handler.go b/introspection_request_handler.go index 893ba21fe..2dd896d02 100644 --- a/introspection_request_handler.go +++ b/introspection_request_handler.go @@ -155,7 +155,7 @@ func (f *Fosite) NewIntrospectionRequest(ctx context.Context, r *http.Request, s } // Enforce client authentication - if err := f.Hasher.Compare(ctx, client.GetHashedSecret(), []byte(clientSecret)); err != nil { + if err := f.checkClientSecret(ctx, client, []byte(clientSecret)); err != nil { return &IntrospectionResponse{Active: false}, errorsx.WithStack(ErrRequestUnauthorized.WithHint("OAuth 2.0 Client credentials are invalid.")) } } diff --git a/introspection_request_handler_test.go b/introspection_request_handler_test.go index 31d7c0339..374ff9862 100644 --- a/introspection_request_handler_test.go +++ b/introspection_request_handler_test.go @@ -203,6 +203,24 @@ func TestNewIntrospectionRequest(t *testing.T) { }, isActive: true, }, + { + description: "should pass with basic auth if username and password not encoded", + setup: func() { + f.TokenIntrospectionHandlers = TokenIntrospectionHandlers{validator} + httpreq = &http.Request{ + Method: "POST", + Header: http.Header{ + //Basic Authorization with username=my-client and password=foobaz + "Authorization": []string{"Basic bXktY2xpZW50OmZvb2Jheg=="}, + }, + PostForm: url.Values{ + "token": []string{"introspect-token"}, + }, + } + validator.EXPECT().IntrospectToken(ctx, "introspect-token", gomock.Any(), gomock.Any(), gomock.Any()).Return(TokenUse(""), nil) + }, + isActive: true, + }, } { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { c.setup() diff --git a/storage/memory.go b/storage/memory.go index bada5036e..f27310f1d 100644 --- a/storage/memory.go +++ b/storage/memory.go @@ -110,20 +110,22 @@ func NewExampleStore() *MemoryStore { IDSessions: make(map[string]fosite.Requester), Clients: map[string]fosite.Client{ "my-client": &fosite.DefaultClient{ - ID: "my-client", - Secret: []byte(`$2a$10$IxMdI6d.LIRZPpSfEwNoeu4rY3FhDREsxFJXikcgdRRAStxUlsuEO`), // = "foobar" - RedirectURIs: []string{"http://localhost:3846/callback"}, - ResponseTypes: []string{"id_token", "code", "token", "id_token token", "code id_token", "code token", "code id_token token"}, - GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, - Scopes: []string{"fosite", "openid", "photos", "offline"}, + ID: "my-client", + Secret: []byte(`$2a$10$IxMdI6d.LIRZPpSfEwNoeu4rY3FhDREsxFJXikcgdRRAStxUlsuEO`), // = "foobar" + RotatedSecrets: [][]byte{[]byte(`$2y$10$X51gLxUQJ.hGw1epgHTE5u0bt64xM0COU7K9iAp.OFg8p2pUd.1zC `)}, // = "foobaz", + RedirectURIs: []string{"http://localhost:3846/callback"}, + ResponseTypes: []string{"id_token", "code", "token", "id_token token", "code id_token", "code token", "code id_token token"}, + GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, + Scopes: []string{"fosite", "openid", "photos", "offline"}, }, "encoded:client": &fosite.DefaultClient{ - ID: "encoded:client", - Secret: []byte(`$2a$10$A7M8b65dSSKGHF0H2sNkn.9Z0hT8U1Nv6OWPV3teUUaczXkVkxuDS`), // = "encoded&password" - RedirectURIs: []string{"http://localhost:3846/callback"}, - ResponseTypes: []string{"id_token", "code", "token", "id_token token", "code id_token", "code token", "code id_token token"}, - GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, - Scopes: []string{"fosite", "openid", "photos", "offline"}, + ID: "encoded:client", + Secret: []byte(`$2a$10$A7M8b65dSSKGHF0H2sNkn.9Z0hT8U1Nv6OWPV3teUUaczXkVkxuDS`), // = "encoded&password" + RotatedSecrets: nil, + RedirectURIs: []string{"http://localhost:3846/callback"}, + ResponseTypes: []string{"id_token", "code", "token", "id_token token", "code id_token", "code token", "code id_token token"}, + GrantTypes: []string{"implicit", "refresh_token", "authorization_code", "password", "client_credentials"}, + Scopes: []string{"fosite", "openid", "photos", "offline"}, }, }, Users: map[string]MemoryUserRelation{