From ca5564c1e0bde3a078a8cfdc24e2eccd8b66a390 Mon Sep 17 00:00:00 2001 From: theluckystrike <51033404+theluckystrike@users.noreply.github.com> Date: Wed, 7 Oct 2026 05:28:54 +0000 Subject: [PATCH 1/2] fix(op): pass a storage server_error through client authentication Client authentication wrapped every Storage error in a new invalid_client or unauthorized_client error. RequestError maps the HTTP status from the outermost oidc.Error, so a server_error that the Storage returned for a database fault reached the client as 401 invalid_client instead of 500 server_error. I added clientAuthError in pkg/op/client.go. It returns the Storage's own oidc.Error when its type is server_error and wraps every other error in the given base error as before. The 8 client authentication wrap sites use it, in client.go, server_legacy.go (2), token_client_credentials.go, token_code.go, token_exchange.go, token_request.go and token_revocation.go. Other typed Storage errors keep the invalid_client answer, so a Storage cannot leak client existence through a different error type. No exported signature changes. pkg/op/client_auth_error_test.go covers AuthorizeClientCredentialsClient, AuthorizeClientIDSecret and a POST to the token endpoint of op.NewOpenIDProvider with basic auth for the client_credentials and authorization_code grants. A plain Storage error (wrong secret) still answers 401 invalid_client. Fixes #990 Commands I ran with go1.26.4 darwin/arm64. Main c9e2385 plus the new test file $ go test -count=1 -run 'StorageErrors' -v ./pkg/op/ --- FAIL: TestAuthorizeClientCredentialsClient_StorageErrors/typed_server_error_passes_through expected: "server_error" actual : "invalid_client" --- FAIL: TestAuthorizeClientIDSecret_StorageErrors/typed_server_error_passes_through expected: "server_error" actual : "invalid_client" --- FAIL: TestTokenEndpoint_ClientAuthStorageErrors/client_credentials/typed_server_error_passes_through expected: 500 actual : 401 --- FAIL: TestTokenEndpoint_ClientAuthStorageErrors/authorization_code/typed_server_error_passes_through expected: 500 actual : 401 (the 4 plain_error_stays_invalid_client subtests PASS) FAIL github.com/zitadel/oidc/v3/pkg/op 1.416s This branch $ go test -count=1 -run 'StorageErrors' -v ./pkg/op/ (all 8 subtests PASS) ok github.com/zitadel/oidc/v3/pkg/op 0.986s $ go test -race -count=1 ./pkg/... ok github.com/zitadel/oidc/v3/pkg/client 3.872s ok github.com/zitadel/oidc/v3/pkg/client/rp 3.915s ok github.com/zitadel/oidc/v3/pkg/client/rp/cli 1.595s ok github.com/zitadel/oidc/v3/pkg/client/rs 5.606s ok github.com/zitadel/oidc/v3/pkg/crypto 2.753s ok github.com/zitadel/oidc/v3/pkg/http 4.408s ok github.com/zitadel/oidc/v3/pkg/oidc 4.009s ok github.com/zitadel/oidc/v3/pkg/op 10.268s ok github.com/zitadel/oidc/v3/pkg/strings 4.149s (main c9e2385 gives the same 9 ok lines) $ gofmt -l pkg/op (no output) $ go vet ./pkg/op/ (no output) $ staticcheck -tags '' ./... (2026.2.1) $ staticcheck -tags no_otel ./... $ staticcheck -tags create_regression_data ./... (no findings for all three) End to end over real HTTP (httptest.NewServer, op.NewProvider with the example storage, Storage returns oidc.ErrServerError() from AuthorizeClientIDSecret and ClientCredentials) POST /oauth/token grant_type=client_credentials, basic auth main 401 {"error":"invalid_client"} this branch 500 {"error":"server_error"} The same Storage fault on authorization_code, revocation and device authorization goes from 401 or 400 on main to 500 server_error on this branch, through op.NewProvider and through op.RegisterLegacyServer. Legacy server introspection goes from 400 unauthorized_client to 500 server_error. The op.Provider introspection handler writes 401 for every error and stays as it is. A plain Storage error gives the same response on main and on this branch in every case. Not run here are CodeQL and the Go 1.27 CI job. --- pkg/op/client.go | 13 ++- pkg/op/client_auth_error_test.go | 153 +++++++++++++++++++++++++++++ pkg/op/server_legacy.go | 4 +- pkg/op/token_client_credentials.go | 2 +- pkg/op/token_code.go | 2 +- pkg/op/token_exchange.go | 2 +- pkg/op/token_request.go | 2 +- pkg/op/token_revocation.go | 2 +- 8 files changed, 172 insertions(+), 8 deletions(-) create mode 100644 pkg/op/client_auth_error_test.go diff --git a/pkg/op/client.go b/pkg/op/client.go index 86e3a4f8..c84a3982 100644 --- a/pkg/op/client.go +++ b/pkg/op/client.go @@ -124,11 +124,22 @@ func ClientBasicAuth(r *http.Request, storage Storage) (clientID string, err err return "", oidc.ErrInvalidClient().WithParent(ErrInvalidAuthHeader) } if err := storage.AuthorizeClientIDSecret(r.Context(), clientID, clientSecret); err != nil { - return "", oidc.ErrUnauthorizedClient().WithParent(err) + return "", clientAuthError(oidc.ErrUnauthorizedClient(), err) } return clientID, nil } +// clientAuthError returns the server_error a Storage reported during client +// authentication as is, so a server side fault is not answered as a client +// fault. Any other error is wrapped in base. +func clientAuthError(base *oidc.Error, err error) *oidc.Error { + var oidcErr *oidc.Error + if errors.As(err, &oidcErr) && oidcErr.ErrorType == oidc.ServerError { + return oidcErr + } + return base.WithParent(err) +} + type ClientProvider interface { Decoder() httphelper.Decoder Storage() Storage diff --git a/pkg/op/client_auth_error_test.go b/pkg/op/client_auth_error_test.go new file mode 100644 index 00000000..ebf6dd56 --- /dev/null +++ b/pkg/op/client_auth_error_test.go @@ -0,0 +1,153 @@ +package op_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/zitadel/oidc/v3/example/server/storage" + "github.com/zitadel/oidc/v3/pkg/oidc" + "github.com/zitadel/oidc/v3/pkg/op" + "github.com/zitadel/oidc/v3/pkg/op/mock" +) + +var clientAuthStorageErrorTests = []struct { + name string + storageErr error + wantType string + wantStatus int +}{ + { + name: "typed server_error passes through", + storageErr: oidc.ErrServerError().WithParent(errors.New("db down")), + wantType: string(oidc.ServerError), + wantStatus: http.StatusInternalServerError, + }, + { + name: "plain error stays invalid_client", + storageErr: errors.New("client not found"), + wantType: string(oidc.InvalidClient), + wantStatus: http.StatusUnauthorized, + }, +} + +type clientCredentialsErrorStorage struct { + err error +} + +func (s clientCredentialsErrorStorage) ClientCredentials(context.Context, string, string) (op.Client, error) { + return nil, s.err +} + +func (s clientCredentialsErrorStorage) ClientCredentialsTokenRequest(context.Context, string, []string) (op.TokenRequest, error) { + return nil, errors.New("not reached") +} + +func TestAuthorizeClientCredentialsClient_StorageErrors(t *testing.T) { + for _, tt := range clientAuthStorageErrorTests { + t.Run(tt.name, func(t *testing.T) { + req := &oidc.ClientCredentialsRequest{ClientID: "id", ClientSecret: "secret"} + _, err := op.AuthorizeClientCredentialsClient(context.Background(), req, clientCredentialsErrorStorage{err: tt.storageErr}) + var oidcErr *oidc.Error + require.ErrorAs(t, err, &oidcErr) + assert.Equal(t, tt.wantType, string(oidcErr.ErrorType)) + }) + } +} + +func TestAuthorizeClientIDSecret_StorageErrors(t *testing.T) { + for _, tt := range clientAuthStorageErrorTests { + t.Run(tt.name, func(t *testing.T) { + s := mock.NewMockStorage(gomock.NewController(t)) + s.EXPECT().AuthorizeClientIDSecret(gomock.Any(), "id", "secret").Return(tt.storageErr) + err := op.AuthorizeClientIDSecret(context.Background(), "id", "secret", s) + var oidcErr *oidc.Error + require.ErrorAs(t, err, &oidcErr) + assert.Equal(t, tt.wantType, string(oidcErr.ErrorType)) + }) + } +} + +// failingClientStorage wraps the example storage and fails the client +// authentication lookups with a configurable error. +type failingClientStorage struct { + *storage.Storage + err error +} + +func (s *failingClientStorage) AuthorizeClientIDSecret(context.Context, string, string) error { + return s.err +} + +func (s *failingClientStorage) ClientCredentials(context.Context, string, string) (op.Client, error) { + return nil, s.err +} + +func TestTokenEndpoint_ClientAuthStorageErrors(t *testing.T) { + grants := []struct { + name string + clientID string + secret string + form url.Values + }{ + { + name: "client_credentials", + clientID: "sid1", + secret: "verysecret", + form: url.Values{"grant_type": {string(oidc.GrantTypeClientCredentials)}, "scope": {"openid"}}, + }, + { + name: "authorization_code", + clientID: "web", + secret: "secret", + form: url.Values{ + "grant_type": {string(oidc.GrantTypeCode)}, + "code": {"code"}, + "redirect_uri": {"https://example.com"}, + }, + }, + } + for _, g := range grants { + for _, tt := range clientAuthStorageErrorTests { + t.Run(g.name+"/"+tt.name, func(t *testing.T) { + s := &failingClientStorage{Storage: storage.NewStorage(storage.NewUserStore(testIssuer)), err: tt.storageErr} + provider, err := op.NewOpenIDProvider(testIssuer, testConfig, s, op.WithAllowInsecure()) + require.NoError(t, err) + + // A valid code, so that the code flow reaches client authentication. + ctx := op.ContextWithIssuer(context.Background(), testIssuer) + authReq, err := s.CreateAuthRequest(ctx, &oidc.AuthRequest{ + ClientID: "web", + RedirectURI: "https://example.com", + Scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID}, + ResponseType: oidc.ResponseTypeCode, + }, "id1") + require.NoError(t, err) + require.NoError(t, s.SaveAuthCode(ctx, authReq.GetID(), "code")) + require.NoError(t, s.AuthRequestDone(authReq.GetID())) + + req := httptest.NewRequest(http.MethodPost, provider.TokenEndpoint().Relative(), strings.NewReader(g.form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + req.SetBasicAuth(g.clientID, g.secret) + rec := httptest.NewRecorder() + provider.ServeHTTP(rec, req) + + var body struct { + Error string `json:"error"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body), rec.Body.String()) + assert.Equal(t, tt.wantStatus, rec.Code, rec.Body.String()) + assert.Equal(t, tt.wantType, body.Error) + }) + } + } +} diff --git a/pkg/op/server_legacy.go b/pkg/op/server_legacy.go index 79a148f2..358c7f5d 100644 --- a/pkg/op/server_legacy.go +++ b/pkg/op/server_legacy.go @@ -215,7 +215,7 @@ func (s *LegacyServer) VerifyClient(ctx context.Context, r *Request[ClientCreden } client, err := s.provider.Storage().GetClientByClientID(ctx, r.Data.ClientID) if err != nil { - return nil, oidc.ErrInvalidClient().WithParent(err) + return nil, clientAuthError(oidc.ErrInvalidClient(), err) } switch client.AuthMethod() { @@ -382,7 +382,7 @@ func (s *LegacyServer) authenticateResourceClient(ctx context.Context, cc *Clien return "", oidc.ErrInvalidClient().WithDescription("client_assertion not supported") } if err := s.provider.Storage().AuthorizeClientIDSecret(ctx, cc.ClientID, cc.ClientSecret); err != nil { - return "", oidc.ErrUnauthorizedClient().WithParent(err) + return "", clientAuthError(oidc.ErrUnauthorizedClient(), err) } return cc.ClientID, nil } diff --git a/pkg/op/token_client_credentials.go b/pkg/op/token_client_credentials.go index 0c43c545..52a85bcd 100644 --- a/pkg/op/token_client_credentials.go +++ b/pkg/op/token_client_credentials.go @@ -98,7 +98,7 @@ func AuthorizeClientCredentialsClient(ctx context.Context, request *oidc.ClientC client, err := storage.ClientCredentials(ctx, request.ClientID, request.ClientSecret) if err != nil { - return nil, oidc.ErrInvalidClient().WithParent(err) + return nil, clientAuthError(oidc.ErrInvalidClient(), err) } if !ValidateGrantType(client, oidc.GrantTypeClientCredentials) { diff --git a/pkg/op/token_code.go b/pkg/op/token_code.go index 753709fd..57699d4d 100644 --- a/pkg/op/token_code.go +++ b/pkg/op/token_code.go @@ -100,7 +100,7 @@ func AuthorizeCodeClient(ctx context.Context, tokenReq *oidc.AccessTokenRequest, client, err = exchanger.Storage().GetClientByClientID(ctx, tokenReq.ClientID) if err != nil { - return nil, nil, oidc.ErrInvalidClient().WithParent(err) + return nil, nil, clientAuthError(oidc.ErrInvalidClient(), err) } if client.AuthMethod() == oidc.AuthMethodPrivateKeyJWT { return nil, nil, oidc.ErrInvalidClient().WithDescription("private_key_jwt not allowed for this client") diff --git a/pkg/op/token_exchange.go b/pkg/op/token_exchange.go index 08b3a9d6..9250668f 100644 --- a/pkg/op/token_exchange.go +++ b/pkg/op/token_exchange.go @@ -364,7 +364,7 @@ func AuthorizeTokenExchangeClient(ctx context.Context, clientID, clientSecret st client, err = exchanger.Storage().GetClientByClientID(ctx, clientID) if err != nil { - return nil, oidc.ErrInvalidClient().WithParent(err) + return nil, clientAuthError(oidc.ErrInvalidClient(), err) } return client, nil diff --git a/pkg/op/token_request.go b/pkg/op/token_request.go index a8cd0d68..78f1ea63 100644 --- a/pkg/op/token_request.go +++ b/pkg/op/token_request.go @@ -158,7 +158,7 @@ func AuthorizeClientIDSecret(ctx context.Context, clientID, clientSecret string, err := storage.AuthorizeClientIDSecret(ctx, clientID, clientSecret) if err != nil { - return oidc.ErrInvalidClient().WithDescription("invalid client_id / client_secret").WithParent(err) + return clientAuthError(oidc.ErrInvalidClient().WithDescription("invalid client_id / client_secret"), err) } return nil } diff --git a/pkg/op/token_revocation.go b/pkg/op/token_revocation.go index ec792a6a..e020ffac 100644 --- a/pkg/op/token_revocation.go +++ b/pkg/op/token_revocation.go @@ -121,7 +121,7 @@ func ParseTokenRevocationRequest(r *http.Request, revoker Revoker) (token, token } client, err := revoker.Storage().GetClientByClientID(r.Context(), req.ClientID) if err != nil { - return "", "", "", oidc.ErrInvalidClient().WithParent(err) + return "", "", "", clientAuthError(oidc.ErrInvalidClient(), err) } if req.ClientSecret == "" { if client.AuthMethod() != oidc.AuthMethodNone { From 28daa191cc40bf51e4e2bff19b01f3aac7f32985 Mon Sep 17 00:00:00 2001 From: theluckystrike <51033404+theluckystrike@users.noreply.github.com> Date: Tue, 13 Oct 2026 07:23:10 +0700 Subject: [PATCH 2/2] fix(op): answer a storage server_error with a new server_error clientAuthError returned the *oidc.Error that errors.As matched inside the Storage error. That dropped an outer StatusError, so a 503 from op.NewStatusError became 500 on the LegacyServer token, revocation and introspection endpoints. It also handed the Storage's returnParent flag to MarshalJSON, which panics on a nil parent, and it cut the rest of the error chain. The helper now returns oidc.ErrServerError().WithParent(err), so the whole Storage error stays the parent. It checks for a typed nil *oidc.Error before reading ErrorType. ClientJWTAuth uses the helper too. The Storage docs of GetClientByClientID, AuthorizeClientIDSecret and ClientCredentials now say when to return oidc.ErrServerError(). TestClientAuthStorageErrors sends token, revocation and introspection requests to the Provider and to the LegacyServer through authorizePaths, and fails each client authentication lookup with a server_error, a server_error with a nil parent returned to the client, a server_error in a 503 StatusError, a plain error and a typed client error. Each of the 9 call sites has a failing row. TestClientBasicAuth and TestClientJWTAuth gain server_error, error chain, typed client error and typed nil rows, asserted with ErrorAs and the error type. The test generates the RSA key once. --- pkg/op/client.go | 14 +- pkg/op/client_auth_error_test.go | 283 +++++++++++++++++++------------ pkg/op/client_test.go | 98 ++++++++++- pkg/op/storage.go | 6 + 4 files changed, 289 insertions(+), 112 deletions(-) diff --git a/pkg/op/client.go b/pkg/op/client.go index c84a3982..2ee59f3f 100644 --- a/pkg/op/client.go +++ b/pkg/op/client.go @@ -101,7 +101,7 @@ func ClientJWTAuth(ctx context.Context, ca oidc.ClientAssertionParams, verifier profile, err := VerifyJWTAssertion(ctx, ca.ClientAssertion, verifier.JWTProfileVerifier(ctx)) if err != nil { - return "", oidc.ErrUnauthorizedClient().WithParent(err).WithDescription("JWT assertion failed") + return "", clientAuthError(oidc.ErrUnauthorizedClient().WithDescription("JWT assertion failed"), err) } return profile.Issuer, nil } @@ -129,13 +129,15 @@ func ClientBasicAuth(r *http.Request, storage Storage) (clientID string, err err return clientID, nil } -// clientAuthError returns the server_error a Storage reported during client -// authentication as is, so a server side fault is not answered as a client -// fault. Any other error is wrapped in base. +// clientAuthError answers a server_error a Storage reported during client +// authentication with a new server_error, so a server side fault is not +// answered as a client fault. The Storage error stays its parent, so a +// StatusError in the chain keeps its status code. Any other error is wrapped +// in base. func clientAuthError(base *oidc.Error, err error) *oidc.Error { var oidcErr *oidc.Error - if errors.As(err, &oidcErr) && oidcErr.ErrorType == oidc.ServerError { - return oidcErr + if errors.As(err, &oidcErr) && oidcErr != nil && oidcErr.ErrorType == oidc.ServerError { + return oidc.ErrServerError().WithParent(err) } return base.WithParent(err) } diff --git a/pkg/op/client_auth_error_test.go b/pkg/op/client_auth_error_test.go index ebf6dd56..f144446e 100644 --- a/pkg/op/client_auth_error_test.go +++ b/pkg/op/client_auth_error_test.go @@ -10,144 +10,217 @@ import ( "strings" "testing" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/zitadel/oidc/v3/example/server/storage" "github.com/zitadel/oidc/v3/pkg/oidc" "github.com/zitadel/oidc/v3/pkg/op" - "github.com/zitadel/oidc/v3/pkg/op/mock" ) -var clientAuthStorageErrorTests = []struct { - name string - storageErr error - wantType string - wantStatus int -}{ - { - name: "typed server_error passes through", - storageErr: oidc.ErrServerError().WithParent(errors.New("db down")), - wantType: string(oidc.ServerError), - wantStatus: http.StatusInternalServerError, - }, - { - name: "plain error stays invalid_client", - storageErr: errors.New("client not found"), - wantType: string(oidc.InvalidClient), - wantStatus: http.StatusUnauthorized, - }, -} - -type clientCredentialsErrorStorage struct { - err error -} - -func (s clientCredentialsErrorStorage) ClientCredentials(context.Context, string, string) (op.Client, error) { - return nil, s.err -} +const ( + failAuthorizeClientIDSecret = "AuthorizeClientIDSecret" + failClientCredentials = "ClientCredentials" + failGetClientByClientID = "GetClientByClientID" +) -func (s clientCredentialsErrorStorage) ClientCredentialsTokenRequest(context.Context, string, []string) (op.TokenRequest, error) { - return nil, errors.New("not reached") +// failingClientStorage wraps the example storage and fails the client +// authentication lookup named by method with err. +type failingClientStorage struct { + *storage.Storage + method string + err error } -func TestAuthorizeClientCredentialsClient_StorageErrors(t *testing.T) { - for _, tt := range clientAuthStorageErrorTests { - t.Run(tt.name, func(t *testing.T) { - req := &oidc.ClientCredentialsRequest{ClientID: "id", ClientSecret: "secret"} - _, err := op.AuthorizeClientCredentialsClient(context.Background(), req, clientCredentialsErrorStorage{err: tt.storageErr}) - var oidcErr *oidc.Error - require.ErrorAs(t, err, &oidcErr) - assert.Equal(t, tt.wantType, string(oidcErr.ErrorType)) - }) +func (s *failingClientStorage) AuthorizeClientIDSecret(ctx context.Context, clientID, clientSecret string) error { + if s.method == failAuthorizeClientIDSecret { + return s.err } + return s.Storage.AuthorizeClientIDSecret(ctx, clientID, clientSecret) } -func TestAuthorizeClientIDSecret_StorageErrors(t *testing.T) { - for _, tt := range clientAuthStorageErrorTests { - t.Run(tt.name, func(t *testing.T) { - s := mock.NewMockStorage(gomock.NewController(t)) - s.EXPECT().AuthorizeClientIDSecret(gomock.Any(), "id", "secret").Return(tt.storageErr) - err := op.AuthorizeClientIDSecret(context.Background(), "id", "secret", s) - var oidcErr *oidc.Error - require.ErrorAs(t, err, &oidcErr) - assert.Equal(t, tt.wantType, string(oidcErr.ErrorType)) - }) +func (s *failingClientStorage) ClientCredentials(ctx context.Context, clientID, clientSecret string) (op.Client, error) { + if s.method == failClientCredentials { + return nil, s.err } + return s.Storage.ClientCredentials(ctx, clientID, clientSecret) } -// failingClientStorage wraps the example storage and fails the client -// authentication lookups with a configurable error. -type failingClientStorage struct { - *storage.Storage - err error +func (s *failingClientStorage) GetClientByClientID(ctx context.Context, clientID string) (op.Client, error) { + if s.method == failGetClientByClientID { + return nil, s.err + } + return s.Storage.GetClientByClientID(ctx, clientID) } -func (s *failingClientStorage) AuthorizeClientIDSecret(context.Context, string, string) error { - return s.err +// clientAuthAnswer is the response a handler gives for a client +// authentication failure that is not a server_error. +type clientAuthAnswer struct { + errorType string + status int } -func (s *failingClientStorage) ClientCredentials(context.Context, string, string) (op.Client, error) { - return nil, s.err -} +func TestClientAuthStorageErrors(t *testing.T) { + // One storage for all subtests, so that the RSA key is generated once. + s := storage.NewStorage(storage.NewUserStore(testIssuer)) + + // A valid code, so that the Provider's code flow reaches client authentication. + ctx := op.ContextWithIssuer(context.Background(), testIssuer) + authReq, err := s.CreateAuthRequest(ctx, &oidc.AuthRequest{ + ClientID: "web", + RedirectURI: "https://example.com", + Scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID}, + ResponseType: oidc.ResponseTypeCode, + }, "id1") + require.NoError(t, err) + const code = "client-auth-storage-errors" + require.NoError(t, s.SaveAuthCode(ctx, authReq.GetID(), code)) + require.NoError(t, s.AuthRequestDone(authReq.GetID())) + + codeForm := url.Values{ + "grant_type": {string(oidc.GrantTypeCode)}, + "code": {code}, + "redirect_uri": {"https://example.com"}, + } + invalidClient := map[string]clientAuthAnswer{ + "Provider": {string(oidc.InvalidClient), http.StatusUnauthorized}, + "op.Server": {string(oidc.InvalidClient), http.StatusBadRequest}, + } -func TestTokenEndpoint_ClientAuthStorageErrors(t *testing.T) { - grants := []struct { + requests := []struct { name string - clientID string - secret string + method string + endpoint func(op.OpenIDProvider) *op.Endpoint form url.Values + // basicAuth sends web:secret in the Authorization header, + // otherwise the form carries the client credentials. + basicAuth bool + // want lists the handlers to test, by authorizePath name. + want map[string]clientAuthAnswer }{ { - name: "client_credentials", - clientID: "sid1", - secret: "verysecret", - form: url.Values{"grant_type": {string(oidc.GrantTypeClientCredentials)}, "scope": {"openid"}}, + name: "token client_credentials", + method: failClientCredentials, + endpoint: op.OpenIDProvider.TokenEndpoint, + form: url.Values{"grant_type": {string(oidc.GrantTypeClientCredentials)}, "scope": {oidc.ScopeOpenID}}, + basicAuth: true, + // The LegacyServer returns the ClientCredentials error as is. + want: map[string]clientAuthAnswer{"Provider": invalidClient["Provider"]}, + }, + { + name: "token authorization_code client lookup", + method: failGetClientByClientID, + endpoint: op.OpenIDProvider.TokenEndpoint, + form: codeForm, + basicAuth: true, + want: invalidClient, + }, + { + name: "token authorization_code secret check", + method: failAuthorizeClientIDSecret, + endpoint: op.OpenIDProvider.TokenEndpoint, + form: codeForm, + basicAuth: true, + want: invalidClient, }, { - name: "authorization_code", - clientID: "web", - secret: "secret", + name: "token token_exchange client lookup", + method: failGetClientByClientID, + endpoint: op.OpenIDProvider.TokenEndpoint, form: url.Values{ - "grant_type": {string(oidc.GrantTypeCode)}, - "code": {"code"}, - "redirect_uri": {"https://example.com"}, + "grant_type": {string(oidc.GrantTypeTokenExchange)}, + "subject_token": {"token"}, + "subject_token_type": {string(oidc.AccessTokenType)}, }, + basicAuth: true, + want: invalidClient, + }, + { + name: "revocation client lookup", + method: failGetClientByClientID, + endpoint: op.OpenIDProvider.RevocationEndpoint, + form: url.Values{"token": {"token"}, "client_id": {"web"}, "client_secret": {"secret"}}, + want: invalidClient, + }, + { + name: "introspection secret check", + method: failAuthorizeClientIDSecret, + endpoint: op.OpenIDProvider.IntrospectionEndpoint, + form: url.Values{"token": {"token"}}, + basicAuth: true, + // The Provider introspection handler answers every error with 401. + want: map[string]clientAuthAnswer{"op.Server": {string(oidc.UnauthorizedClient), http.StatusBadRequest}}, + }, + } + + dbErr := errors.New("db down") + storageErrors := []struct { + name string + err error + // serverError is true when the answer must be a server_error. + serverError bool + // legacyStatus overrides the 500 of a server_error on the LegacyServer. + legacyStatus int + }{ + { + name: "server_error", + err: oidc.ErrServerError().WithParent(dbErr), + serverError: true, + }, + { + name: "server_error returning a nil parent", + err: oidc.ErrServerError().WithReturnParentToClient(true), + serverError: true, + }, + { + name: "server_error with status 503", + err: op.NewStatusError(oidc.ErrServerError().WithParent(dbErr), http.StatusServiceUnavailable), + serverError: true, + legacyStatus: http.StatusServiceUnavailable, + }, + { + name: "plain error", + err: errors.New("client not found"), + }, + { + name: "typed client error", + err: oidc.ErrInvalidRequest().WithDescription("rejected by storage"), }, } - for _, g := range grants { - for _, tt := range clientAuthStorageErrorTests { - t.Run(g.name+"/"+tt.name, func(t *testing.T) { - s := &failingClientStorage{Storage: storage.NewStorage(storage.NewUserStore(testIssuer)), err: tt.storageErr} - provider, err := op.NewOpenIDProvider(testIssuer, testConfig, s, op.WithAllowInsecure()) - require.NoError(t, err) - - // A valid code, so that the code flow reaches client authentication. - ctx := op.ContextWithIssuer(context.Background(), testIssuer) - authReq, err := s.CreateAuthRequest(ctx, &oidc.AuthRequest{ - ClientID: "web", - RedirectURI: "https://example.com", - Scopes: oidc.SpaceDelimitedArray{oidc.ScopeOpenID}, - ResponseType: oidc.ResponseTypeCode, - }, "id1") - require.NoError(t, err) - require.NoError(t, s.SaveAuthCode(ctx, authReq.GetID(), "code")) - require.NoError(t, s.AuthRequestDone(authReq.GetID())) - - req := httptest.NewRequest(http.MethodPost, provider.TokenEndpoint().Relative(), strings.NewReader(g.form.Encode())) - req.Header.Set("Content-Type", "application/x-www-form-urlencoded") - req.SetBasicAuth(g.clientID, g.secret) - rec := httptest.NewRecorder() - provider.ServeHTTP(rec, req) - - var body struct { - Error string `json:"error"` + + for _, req := range requests { + for _, se := range storageErrors { + fs := &failingClientStorage{Storage: s, method: req.method, err: se.err} + provider, err := op.NewOpenIDProvider(testIssuer, testConfig, fs, op.WithAllowInsecure()) + require.NoError(t, err) + + for _, path := range authorizePaths(provider) { + want, ok := req.want[path.name] + if !ok { + continue + } + if se.serverError { + want = clientAuthAnswer{string(oidc.ServerError), http.StatusInternalServerError} + if path.name == "op.Server" && se.legacyStatus != 0 { + want.status = se.legacyStatus + } } - require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body), rec.Body.String()) - assert.Equal(t, tt.wantStatus, rec.Code, rec.Body.String()) - assert.Equal(t, tt.wantType, body.Error) - }) + t.Run(path.name+"/"+req.name+"/"+se.name, func(t *testing.T) { + r := httptest.NewRequest(http.MethodPost, req.endpoint(provider).Relative(), strings.NewReader(req.form.Encode())) + r.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if req.basicAuth { + r.SetBasicAuth("web", "secret") + } + rec := httptest.NewRecorder() + path.handler.ServeHTTP(rec, r) + + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body), rec.Body.String()) + assert.Equal(t, want.status, rec.Code, rec.Body.String()) + assert.Equal(t, want.errorType, body["error"], rec.Body.String()) + assert.NotContains(t, body, "parent", rec.Body.String()) + }) + } } } } diff --git a/pkg/op/client_test.go b/pkg/op/client_test.go index b772ba56..9d849ea9 100644 --- a/pkg/op/client_test.go +++ b/pkg/op/client_test.go @@ -3,16 +3,20 @@ package op_test import ( "context" "errors" + "fmt" "io" "net/http" "net/http/httptest" "net/url" "strings" "testing" + "time" + "github.com/go-jose/go-jose/v4" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + tu "github.com/zitadel/oidc/v3/internal/testutil" httphelper "github.com/zitadel/oidc/v3/pkg/http" "github.com/zitadel/oidc/v3/pkg/oidc" "github.com/zitadel/oidc/v3/pkg/op" @@ -24,7 +28,37 @@ type testClientJWTProfile struct{} func (testClientJWTProfile) JWTProfileVerifier(context.Context) *op.JWTProfileVerifier { return nil } +// keyErrJWTProfile verifies JWT assertions with a key storage that fails with err. +type keyErrJWTProfile struct { + err error +} + +func (p keyErrJWTProfile) JWTProfileVerifier(context.Context) *op.JWTProfileVerifier { + return op.NewJWTProfileVerifier(p, tu.ValidIssuer, time.Minute, 0) +} + +func (p keyErrJWTProfile) GetKeyByIDAndClientID(context.Context, string, string) (*jose.JSONWebKey, error) { + return nil, p.err +} + +// assertErrorType asserts that err is an *oidc.Error of type wantType. +// require.ErrorIs alone cannot tell the error type, as it also matches a +// Storage error that is the parent of another type. +func assertErrorType(t *testing.T, err error, wantType string) { + t.Helper() + if wantType == "" { + return + } + var oidcErr *oidc.Error + require.ErrorAs(t, err, &oidcErr) + assert.Equal(t, wantType, string(oidcErr.ErrorType)) +} + func TestClientJWTAuth(t *testing.T) { + errKey := errors.New("key not found") + errDB := errors.New("db down") + assertion, _ := tu.ValidJWTProfileAssertion() + type args struct { ctx context.Context ca oidc.ClientAssertionParams @@ -35,6 +69,7 @@ func TestClientJWTAuth(t *testing.T) { args args wantClientID string wantErr error + wantType string }{ { name: "empty assertion", @@ -56,11 +91,32 @@ func TestClientJWTAuth(t *testing.T) { }, wantErr: oidc.ErrParse, }, + { + name: "key storage error", + args: args{ + context.Background(), + oidc.ClientAssertionParams{ClientAssertion: assertion}, + keyErrJWTProfile{err: errKey}, + }, + wantErr: errKey, + wantType: string(oidc.UnauthorizedClient), + }, + { + name: "key storage server_error", + args: args{ + context.Background(), + oidc.ClientAssertionParams{ClientAssertion: assertion}, + keyErrJWTProfile{err: oidc.ErrServerError().WithParent(errDB)}, + }, + wantErr: errDB, + wantType: string(oidc.ServerError), + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { gotClientID, err := op.ClientJWTAuth(tt.args.ctx, tt.args.ca, tt.args.verifier) require.ErrorIs(t, err, tt.wantErr) + assertErrorType(t, err, tt.wantType) assert.Equal(t, tt.wantClientID, gotClientID) }) } @@ -68,6 +124,15 @@ func TestClientJWTAuth(t *testing.T) { func TestClientBasicAuth(t *testing.T) { errWrong := errors.New("wrong secret") + errDB := errors.New("db down") + errTypedNil := fmt.Errorf("authorize: %w", (*oidc.Error)(nil)) + errClient := oidc.ErrInvalidClient() + errTimeout := fmt.Errorf("authorize: %w", errors.Join(oidc.ErrServerError(), context.DeadlineExceeded)) + failingStorage := func(err error) op.Storage { + s := mock.NewMockStorage(gomock.NewController(t)) + s.EXPECT().AuthorizeClientIDSecret(gomock.Any(), "foo", "bar").Return(err) + return s + } type args struct { username string @@ -79,6 +144,7 @@ func TestClientBasicAuth(t *testing.T) { storage op.Storage wantClientID string wantErr error + wantType string }{ { name: "no args", @@ -111,7 +177,36 @@ func TestClientBasicAuth(t *testing.T) { s.EXPECT().AuthorizeClientIDSecret(gomock.Any(), "foo", "wrong").Return(errWrong) return s }(), - wantErr: errWrong, + wantErr: errWrong, + wantType: string(oidc.UnauthorizedClient), + }, + { + name: "storage server_error", + args: &args{username: "foo", password: "bar"}, + storage: failingStorage(oidc.ErrServerError().WithParent(errDB)), + wantErr: errDB, + wantType: string(oidc.ServerError), + }, + { + name: "storage server_error keeps the error chain", + args: &args{username: "foo", password: "bar"}, + storage: failingStorage(errTimeout), + wantErr: context.DeadlineExceeded, + wantType: string(oidc.ServerError), + }, + { + name: "storage client error stays wrapped", + args: &args{username: "foo", password: "bar"}, + storage: failingStorage(errClient), + wantErr: errClient, + wantType: string(oidc.UnauthorizedClient), + }, + { + name: "storage typed nil oidc error", + args: &args{username: "foo", password: "bar"}, + storage: failingStorage(errTypedNil), + wantErr: errTypedNil, + wantType: string(oidc.UnauthorizedClient), }, { name: "auth error", @@ -136,6 +231,7 @@ func TestClientBasicAuth(t *testing.T) { gotClientID, err := op.ClientBasicAuth(r, tt.storage) require.ErrorIs(t, err, tt.wantErr) + assertErrorType(t, err, tt.wantType) assert.Equal(t, tt.wantClientID, gotClientID) }) } diff --git a/pkg/op/storage.go b/pkg/op/storage.go index 973805b4..a61ceb81 100644 --- a/pkg/op/storage.go +++ b/pkg/op/storage.go @@ -78,6 +78,8 @@ type CanTerminateSessionFromRequest interface { } type ClientCredentialsStorage interface { + // ClientCredentials returns oidc.ErrServerError() only for a genuine server fault; an unknown + // client or a wrong secret is a plain error, and a server_error answers 500. ClientCredentials(ctx context.Context, clientID, clientSecret string) (Client, error) ClientCredentialsTokenRequest(ctx context.Context, clientID string, scopes []string) (TokenRequest, error) } @@ -126,7 +128,11 @@ var ErrInvalidRefreshToken = errors.New("invalid_refresh_token") type OPStorage interface { // GetClientByClientID loads a Client. The returned Client is never cached and is only used to // handle the current request. + // It returns oidc.ErrServerError() only for a genuine server fault; an unknown client is a + // plain error, and a server_error answers 500. GetClientByClientID(ctx context.Context, clientID string) (Client, error) + // AuthorizeClientIDSecret returns oidc.ErrServerError() only for a genuine server fault; an + // unknown client or a wrong secret is a plain error, and a server_error answers 500. AuthorizeClientIDSecret(ctx context.Context, clientID, clientSecret string) error // SetUserinfoFromScopes is deprecated and should have an empty implementation for now. // Implement SetUserinfoFromRequest instead.