From 73a198207738a6d762961538e1ffd2adf4bc20a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tim=20M=C3=B6hlmann?= Date: Tue, 24 Oct 2023 18:07:20 +0300 Subject: [PATCH] fix(server): do not get client by id for introspection (#467) As introspection is a Oauth mechanism for resource servers only, it does not make sense to get an oidc client by ID. The original OP did not do this and now we make the server behavior similar. --- pkg/op/server.go | 4 ++-- pkg/op/server_http.go | 37 ++++++++++++++++++++++++------------ pkg/op/server_http_test.go | 13 ++++--------- pkg/op/server_legacy.go | 21 ++++++++++++++++++-- pkg/op/token_intospection.go | 5 +++++ 5 files changed, 55 insertions(+), 25 deletions(-) diff --git a/pkg/op/server.go b/pkg/op/server.go index a9cdcf5..829618c 100644 --- a/pkg/op/server.go +++ b/pkg/op/server.go @@ -127,7 +127,7 @@ type Server interface { // Introspect handles the OAuth 2.0 Token Introspection endpoint. // https://datatracker.ietf.org/doc/html/rfc7662 // The recommended Response Data type is [oidc.IntrospectionResponse]. - Introspect(context.Context, *ClientRequest[oidc.IntrospectionRequest]) (*Response, error) + Introspect(context.Context, *Request[IntrospectionRequest]) (*Response, error) // UserInfo handles the UserInfo endpoint and returns Claims about the authenticated End-User. // https://openid.net/specs/openid-connect-core-1_0.html#UserInfo @@ -329,7 +329,7 @@ func (UnimplementedServer) DeviceToken(ctx context.Context, r *ClientRequest[oid return nil, unimplementedGrantError(oidc.GrantTypeDeviceCode) } -func (UnimplementedServer) Introspect(ctx context.Context, r *ClientRequest[oidc.IntrospectionRequest]) (*Response, error) { +func (UnimplementedServer) Introspect(ctx context.Context, r *Request[IntrospectionRequest]) (*Response, error) { return nil, unimplementedError(r) } diff --git a/pkg/op/server_http.go b/pkg/op/server_http.go index 750f7a9..6d379c6 100644 --- a/pkg/op/server_http.go +++ b/pkg/op/server_http.go @@ -102,7 +102,7 @@ func (s *webServer) createRouter() { s.endpointRoute(s.endpoints.Authorization, s.authorizeHandler) s.endpointRoute(s.endpoints.DeviceAuthorization, s.withClient(s.deviceAuthorizationHandler)) s.endpointRoute(s.endpoints.Token, s.tokensHandler) - s.endpointRoute(s.endpoints.Introspection, s.withClient(s.introspectionHandler)) + s.endpointRoute(s.endpoints.Introspection, s.introspectionHandler) s.endpointRoute(s.endpoints.Userinfo, s.userInfoHandler) s.endpointRoute(s.endpoints.Revocation, s.withClient(s.revocationHandler)) s.endpointRoute(s.endpoints.EndSession, s.endSessionHandler) @@ -136,7 +136,21 @@ func (s *webServer) withClient(handler clientHandler) http.HandlerFunc { } func (s *webServer) verifyRequestClient(r *http.Request) (_ Client, err error) { - if err = r.ParseForm(); err != nil { + cc, err := s.parseClientCredentials(r) + if err != nil { + return nil, err + } + return s.server.VerifyClient(r.Context(), &Request[ClientCredentials]{ + Method: r.Method, + URL: r.URL, + Header: r.Header, + Form: r.Form, + Data: cc, + }) +} + +func (s *webServer) parseClientCredentials(r *http.Request) (_ *ClientCredentials, err error) { + if err := r.ParseForm(); err != nil { return nil, oidc.ErrInvalidRequest().WithDescription("error parsing form").WithParent(err) } cc := new(ClientCredentials) @@ -160,13 +174,7 @@ func (s *webServer) verifyRequestClient(r *http.Request) (_ Client, err error) { if cc.ClientAssertion != "" && cc.ClientAssertionType != oidc.ClientAssertionTypeJWTAssertion { return nil, oidc.ErrInvalidRequest().WithDescription("invalid client_assertion_type %s", cc.ClientAssertionType) } - return s.server.VerifyClient(r.Context(), &Request[ClientCredentials]{ - Method: r.Method, - URL: r.URL, - Header: r.Header, - Form: r.Form, - Data: cc, - }) + return cc, nil } func (s *webServer) authorizeHandler(w http.ResponseWriter, r *http.Request) { @@ -378,8 +386,13 @@ func (s *webServer) deviceTokenHandler(w http.ResponseWriter, r *http.Request, c resp.writeOut(w) } -func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request, client Client) { - if client.AuthMethod() == oidc.AuthMethodNone { +func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request) { + cc, err := s.parseClientCredentials(r) + if err != nil { + WriteError(w, r, err, s.getLogger(r.Context())) + return + } + if cc.ClientSecret == "" && cc.ClientAssertion == "" { WriteError(w, r, oidc.ErrInvalidClient().WithDescription("client must be authenticated"), s.getLogger(r.Context())) return } @@ -392,7 +405,7 @@ func (s *webServer) introspectionHandler(w http.ResponseWriter, r *http.Request, WriteError(w, r, oidc.ErrInvalidRequest().WithDescription("token missing"), s.getLogger(r.Context())) return } - resp, err := s.server.Introspect(r.Context(), newClientRequest(r, request, client)) + resp, err := s.server.Introspect(r.Context(), newRequest(r, &IntrospectionRequest{cc, request})) if err != nil { WriteError(w, r, err, s.getLogger(r.Context())) return diff --git a/pkg/op/server_http_test.go b/pkg/op/server_http_test.go index 86fe7ed..4eac4a0 100644 --- a/pkg/op/server_http_test.go +++ b/pkg/op/server_http_test.go @@ -1001,14 +1001,12 @@ func Test_webServer_introspectionHandler(t *testing.T) { tests := []struct { name string decoder httphelper.Decoder - client Client r *http.Request want webServerResult }{ { name: "decoder error", decoder: schema.NewDecoder(), - client: newClient(clientTypeUserAgent), r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")), want: webServerResult{ wantStatus: http.StatusBadRequest, @@ -1018,8 +1016,7 @@ func Test_webServer_introspectionHandler(t *testing.T) { { name: "public client", decoder: testDecoder, - client: newClient(clientTypeNative), - r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")), + r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123")), want: webServerResult{ wantStatus: http.StatusBadRequest, wantBody: `{"error":"invalid_client", "error_description":"client must be authenticated"}`, @@ -1028,8 +1025,7 @@ func Test_webServer_introspectionHandler(t *testing.T) { { name: "token missing", decoder: testDecoder, - client: newClient(clientTypeWeb), - r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("foo=bar")), + r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123&client_secret=SECRET")), want: webServerResult{ wantStatus: http.StatusBadRequest, wantBody: `{"error":"invalid_request", "error_description":"token missing"}`, @@ -1038,8 +1034,7 @@ func Test_webServer_introspectionHandler(t *testing.T) { { name: "unimplemented Introspect called", decoder: testDecoder, - client: newClient(clientTypeWeb), - r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("token=xxx")), + r: httptest.NewRequest(http.MethodPost, "/", strings.NewReader("client_id=123&client_secret=SECRET&token=xxx")), want: webServerResult{ wantStatus: UnimplementedStatusCode, wantBody: `{"error":"server_error", "error_description":"/ not implemented on this server"}`, @@ -1053,7 +1048,7 @@ func Test_webServer_introspectionHandler(t *testing.T) { decoder: tt.decoder, logger: slog.Default(), } - runWebServerClientTest(t, s.introspectionHandler, tt.r, tt.client, tt.want) + runWebServerTest(t, s.introspectionHandler, tt.r, tt.want) }) } } diff --git a/pkg/op/server_legacy.go b/pkg/op/server_legacy.go index 8939543..deb1abc 100644 --- a/pkg/op/server_legacy.go +++ b/pkg/op/server_legacy.go @@ -315,13 +315,30 @@ func (s *LegacyServer) DeviceToken(ctx context.Context, r *ClientRequest[oidc.De return NewResponse(resp), nil } -func (s *LegacyServer) Introspect(ctx context.Context, r *ClientRequest[oidc.IntrospectionRequest]) (*Response, error) { +func (s *LegacyServer) authenticateResourceClient(ctx context.Context, cc *ClientCredentials) (string, error) { + if cc.ClientAssertion != "" { + if jp, ok := s.provider.(ClientJWTProfile); ok { + return ClientJWTAuth(ctx, oidc.ClientAssertionParams{ClientAssertion: cc.ClientAssertion}, jp) + } + 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 cc.ClientID, nil +} + +func (s *LegacyServer) Introspect(ctx context.Context, r *Request[IntrospectionRequest]) (*Response, error) { + clientID, err := s.authenticateResourceClient(ctx, r.Data.ClientCredentials) + if err != nil { + return nil, err + } response := new(oidc.IntrospectionResponse) tokenID, subject, ok := getTokenIDAndSubject(ctx, s.provider, r.Data.Token) if !ok { return NewResponse(response), nil } - err := s.provider.Storage().SetIntrospectionFromToken(ctx, response, tokenID, subject, r.Client.GetID()) + err = s.provider.Storage().SetIntrospectionFromToken(ctx, response, tokenID, subject, clientID) if err != nil { return NewResponse(response), nil } diff --git a/pkg/op/token_intospection.go b/pkg/op/token_intospection.go index 21b79c3..9c45ef8 100644 --- a/pkg/op/token_intospection.go +++ b/pkg/op/token_intospection.go @@ -65,3 +65,8 @@ func ParseTokenIntrospectionRequest(r *http.Request, introspector Introspector) return req.Token, clientID, nil } + +type IntrospectionRequest struct { + *ClientCredentials + *oidc.IntrospectionRequest +}