diff --git a/pkg/op/client.go b/pkg/op/client.go index 86e3a4f8..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 } @@ -124,11 +124,24 @@ 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 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 != nil && oidcErr.ErrorType == oidc.ServerError { + return oidc.ErrServerError().WithParent(err) + } + 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..f144446e --- /dev/null +++ b/pkg/op/client_auth_error_test.go @@ -0,0 +1,226 @@ +package op_test + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "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" +) + +const ( + failAuthorizeClientIDSecret = "AuthorizeClientIDSecret" + failClientCredentials = "ClientCredentials" + failGetClientByClientID = "GetClientByClientID" +) + +// 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 (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 (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) +} + +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) +} + +// 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 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}, + } + + requests := []struct { + name 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: "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: "token token_exchange client lookup", + method: failGetClientByClientID, + endpoint: op.OpenIDProvider.TokenEndpoint, + form: url.Values{ + "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 _, 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 + } + } + 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/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/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. 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 {