From 02d83290f92b44e37dee8ee49db7831ccce43ffc Mon Sep 17 00:00:00 2001 From: Simon Eisenmann Date: Wed, 14 Oct 2026 14:09:54 +0200 Subject: [PATCH 1/2] Answer TokenHandler failures with invalid_grant and invalid_client A refresh token which failed to parse, verify or validate, being malformed, expired or signed with a key lico does not hold, was answered with invalid_request. RFC 6749 section 5.2 assigns that case to invalid_grant, and the difference is what a client acts on: an invalid grant means the user has to sign in again, an invalid request means the request itself is broken. TokenRequest.Validate now answers with it. A failed client lookup at the token endpoint, for an unknown client_id or a wrong secret alike, was answered with access_denied, which is a code of the authorization endpoint (RFC 6749 section 4.1.2.1). It now answers invalid_client. oidc-go does not define that code, so it is added as ErrorCodeOAuth2InvalidClient in oidc/errors.go. Section 5.2 also asks that a client which authenticated through the Authorization header be answered with 401 and a WWW-Authenticate challenge of the same scheme. TokenRequest gains ClientAuthenticationBasic, set when the Basic header supplied the credentials, and in that case TokenHandler answers an invalid_client with 401 and WWW-Authenticate: Basic realm="". Credentials sent in the body keep the 400. This matters once clients are long-lived and pre-registered rather than minted per connection by dynamic registration. Such clients outlive their refresh tokens and key rotations, and a wrong secret pasted into a client dialog is the likeliest way an operator meets invalid_client. The API spec now documents which failures of the token endpoint answer invalid_grant and which invalid_client, adds invalid_client to the OIDCError enum, and describes the 401 with WWW-Authenticate: Basic. TestTokenRefresh gains an expired and a malformed refresh token, both expecting invalid_grant. The new TestTokenClientAuthentication covers an unknown client and a wrong secret, each sent in the body and in the header, for which the test provider gains a confidential client with a secret. --- docs/libregraph-connect-api-v1.yaml | 28 +++++++- oidc/errors.go | 6 ++ oidc/payload/token.go | 13 +++- oidc/provider/handlers.go | 16 ++++- oidc/provider/handlers_test.go | 106 ++++++++++++++++++++++++++++ oidc/provider/provider_test.go | 19 +++++ 6 files changed, 182 insertions(+), 6 deletions(-) diff --git a/docs/libregraph-connect-api-v1.yaml b/docs/libregraph-connect-api-v1.yaml index 996d57a3..1c64fe95 100644 --- a/docs/libregraph-connect-api-v1.yaml +++ b/docs/libregraph-connect-api-v1.yaml @@ -353,6 +353,14 @@ paths: matching `token_endpoint_auth_methods_supported`. A client registered with `token_endpoint_auth_method: none` is public and sends no secret. + The code grant fails with `invalid_grant` for an unknown, used or expired code, a + code issued to another client, a `redirect_uri` other than the one of the + authorization request, a missing `code_challenge` for a client which must use PKCE, + and a missing or wrong `code_verifier`. The refresh grant fails with + `invalid_grant` for a refresh token which is malformed, expired, not verifiable or + issued to another client. An unknown client, a wrong secret or, on the code grant, + a `redirect_uri` the client has not registered fail with `invalid_client`. + The provider dispatches this endpoint by exact path match without router-level method restriction, so a GET reaches the handler and is rejected by it with a JSON error rather than a 405. @@ -378,9 +386,22 @@ paths: '400': description: | OAuth2 errors on this endpoint are returned with status 400, among them - `invalid_request`, `invalid_grant`, `invalid_scope`, `invalid_target`, - `unsupported_grant_type`, `invalid_token`, `insufficient_scope` and - `access_denied`. + `invalid_request`, `invalid_client`, `invalid_grant`, `invalid_scope`, + `invalid_target`, `unsupported_grant_type`, `invalid_token`, + `insufficient_scope` and `access_denied`. + content: + application/json: + schema: + $ref: '#/components/schemas/OIDCError' + '401': + description: | + `invalid_client` for a client which authenticated with the `Authorization` + header, as RFC 6749 section 5.2 requires. Carries a challenge of that scheme. + headers: + WWW-Authenticate: + description: '`Basic realm=""`.' + schema: + type: string content: application/json: schema: @@ -1950,6 +1971,7 @@ components: type: string enum: - invalid_request + - invalid_client - invalid_grant - unauthorized_client - access_denied diff --git a/oidc/errors.go b/oidc/errors.go index dae54e6f..ff0465e8 100644 --- a/oidc/errors.go +++ b/oidc/errors.go @@ -37,6 +37,12 @@ const ( // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2. It is not // defined by oidc-go. ErrorCodeOAuth2InvalidScope = "invalid_scope" + + // ErrorCodeOAuth2InvalidClient is the OAuth2 error code for a token request + // whose client is unknown or failed to authenticate, as defined in + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2. It is not + // defined by oidc-go. + ErrorCodeOAuth2InvalidClient = "invalid_client" ) // OAuth2Error defines a general OAuth2 error with id and decription. diff --git a/oidc/payload/token.go b/oidc/payload/token.go index 733af1d4..f7f9a8b3 100644 --- a/oidc/payload/token.go +++ b/oidc/payload/token.go @@ -57,6 +57,10 @@ type TokenRequest struct { RefreshToken *jwt.Token `schema:"-"` Scopes map[string]bool `schema:"-"` Resource string `schema:"-"` + + // ClientAuthenticationBasic is true when the client authenticated with + // the Authorization header, which decides how a failure is answered. + ClientAuthenticationBasic bool `schema:"-"` } // DecodeTokenRequest return a TokenRequest holding the provided @@ -100,11 +104,13 @@ func DecodeTokenRequest(req *http.Request, providerMetadata *oidc.WellKnown) (*T // Use client ID and secret if no client_id was passed to the request directly. tr.ClientID = clientID tr.ClientSecret = clientSecret + tr.ClientAuthenticationBasic = true } else if clientID != "" { if tr.ClientID == clientID { // Update the client secret, if the ID is a match. This replaces // a directly given secret. tr.ClientSecret = clientSecret + tr.ClientAuthenticationBasic = true } } @@ -150,7 +156,12 @@ func (tr *TokenRequest) Validate(keyFunc jwt.Keyfunc, claims jwt.Claims) error { return nil, fmt.Errorf("Not validated") }) if err != nil { - return konnectoidc.NewOAuth2Error(oidc.ErrorCodeOAuth2InvalidRequest, err.Error()) + // A refresh token which is malformed, expired or signed with a + // key we do not hold is an invalid grant, as defined in + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2. + // It tells the client to sign in again rather than to fix its + // request. + return konnectoidc.NewOAuth2Error(oidc.ErrorCodeOAuth2InvalidGrant, err.Error()) } tr.RefreshToken = refreshToken } diff --git a/oidc/provider/handlers.go b/oidc/provider/handlers.go index 47637572..1e02ad3d 100644 --- a/oidc/provider/handlers.go +++ b/oidc/provider/handlers.go @@ -434,7 +434,10 @@ func (p *Provider) TokenHandler(rw http.ResponseWriter, req *http.Request) { } clientDetails, err = p.clients.Lookup(ctx, tr.ClientID, tr.ClientSecret, lookupRedirectURI, "", false) if err != nil { - err = konnectoidc.NewOAuth2Error(oidc.ErrorCodeOAuth2AccessDenied, err.Error()) + // Unknown client and failed client authentication alike, as defined in + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2. + // access_denied belongs to the authorization endpoint. + err = konnectoidc.NewOAuth2Error(konnectoidc.ErrorCodeOAuth2InvalidClient, err.Error()) goto done } if clientDetails != nil && clientDetails.Registration != nil { @@ -636,7 +639,16 @@ done: if err != nil { switch err.(type) { case *konnectoidc.OAuth2Error: - err = utils.WriteJSON(rw, http.StatusBadRequest, err, "") + status := http.StatusBadRequest + if tr != nil && tr.ClientAuthenticationBasic && konnectoidc.IsErrorWithID(err, konnectoidc.ErrorCodeOAuth2InvalidClient) { + // A client which authenticated with the Authorization header + // is answered with 401 and a challenge of the same scheme, as + // https://datatracker.ietf.org/doc/html/rfc6749#section-5.2 + // requires. + status = http.StatusUnauthorized + rw.Header().Set("WWW-Authenticate", fmt.Sprintf("Basic realm=%q", p.issuerIdentifier)) + } + err = utils.WriteJSON(rw, status, err, "") if err != nil { p.logger.WithError(err).Errorln("token request failed writing response") return diff --git a/oidc/provider/handlers_test.go b/oidc/provider/handlers_test.go index 6461ee14..3f032879 100644 --- a/oidc/provider/handlers_test.go +++ b/oidc/provider/handlers_test.go @@ -779,4 +779,110 @@ func TestTokenRefresh(t *testing.T) { } } }) + + t.Run("a refresh token which does not verify is an invalid grant", func(t *testing.T) { + // Expired and malformed alike tell the client to sign in again, which + // is what invalid_grant means and invalid_request does not. + expired, err := provider.makeJWT(ctx, provider.signingMethodDefault, &konnect.RefreshTokenClaims{ + TokenType: konnect.TokenTypeRefreshToken, + RegisteredClaims: jwt.RegisteredClaims{ + Issuer: config.IssuerIdentifier, + Subject: "unittestsubject", + Audience: jwt.ClaimStrings{"unittestclient"}, + ExpiresAt: jwt.NewNumericDate(time.Now().Add(-time.Hour)), + IssuedAt: jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)), + }, + }) + if err != nil { + t.Fatal(err) + } + + for name, token := range map[string]string{ + "expired": expired, + "malformed": "not.a.token", + } { + status, response := refresh(t, url.Values{ + "grant_type": []string{oidc.GrantTypeRefreshToken}, + "client_id": []string{"unittestclient"}, + "refresh_token": []string{token}, + }) + if status != http.StatusBadRequest { + t.Errorf("%s: token returned wrong status code: got %v want %v, response %v", name, status, http.StatusBadRequest, response) + } + if errorID, _ := response["error"].(string); errorID != oidc.ErrorCodeOAuth2InvalidGrant { + t.Errorf("%s: error was incorrect, got %v, want %s", name, response["error"], oidc.ErrorCodeOAuth2InvalidGrant) + } + } + }) +} + +// TestTokenClientAuthentication covers a client which is unknown or fails to +// authenticate at the token endpoint. Both are invalid_client, and a client +// which authenticated with the Authorization header is challenged with 401. +func TestTokenClientAuthentication(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + httpServer, _, router, config := NewTestProvider(ctx, t) + defer httpServer.Close() + + for _, tc := range []struct { + name string + clientID string + clientSecret string + basic bool + wantStatus int + wantChallenge bool + }{ + {name: "unknown client", clientID: "unknownclient", wantStatus: http.StatusBadRequest}, + {name: "wrong secret in the request body", clientID: testConfidentialClientID, clientSecret: "wrong", wantStatus: http.StatusBadRequest}, + {name: "wrong secret in the Authorization header", clientID: testConfidentialClientID, clientSecret: "wrong", basic: true, wantStatus: http.StatusUnauthorized, wantChallenge: true}, + {name: "unknown client in the Authorization header", clientID: "unknownclient", clientSecret: "secret", basic: true, wantStatus: http.StatusUnauthorized, wantChallenge: true}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := httptest.NewRecorder() + router.ServeHTTP(rr, newCodeTokenRequest(t, config.TokenPath, tc.clientID, tc.clientSecret, tc.basic)) + + if rr.Code != tc.wantStatus { + t.Errorf("token returned wrong status code: got %v want %v, body %s", rr.Code, tc.wantStatus, rr.Body.String()) + } + response := make(map[string]interface{}) + if err := json.Unmarshal(rr.Body.Bytes(), &response); err != nil { + t.Fatalf("failed to decode token response: %v, body %s", err, rr.Body.String()) + } + if errorID, _ := response["error"].(string); errorID != konnectoidc.ErrorCodeOAuth2InvalidClient { + t.Errorf("error was incorrect, got %v, want %s", response["error"], konnectoidc.ErrorCodeOAuth2InvalidClient) + } + challenge := rr.Header().Get("WWW-Authenticate") + if strings.HasPrefix(challenge, "Basic ") != tc.wantChallenge || (challenge != "") != tc.wantChallenge { + t.Errorf("WWW-Authenticate was incorrect, got %q, want a Basic challenge: %v", challenge, tc.wantChallenge) + } + }) + } +} + +// newCodeTokenRequest returns an authorization code token request of the +// provided client, authenticated in the request body or with HTTP Basic. +func newCodeTokenRequest(t *testing.T, tokenPath, clientID, clientSecret string, basic bool) *http.Request { + t.Helper() + + form := url.Values{ + "grant_type": []string{oidc.GrantTypeAuthorizationCode}, + "code": []string{"unittestcode"}, + "redirect_uri": []string{"http://localhost/callback"}, + } + if !basic { + form.Set("client_id", clientID) + form.Set("client_secret", clientSecret) + } + req, err := http.NewRequest("POST", tokenPath, strings.NewReader(form.Encode())) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + if basic { + req.SetBasicAuth(url.QueryEscape(clientID), url.QueryEscape(clientSecret)) + } + + return req } diff --git a/oidc/provider/provider_test.go b/oidc/provider/provider_test.go index 54acf472..67b6f42b 100644 --- a/oidc/provider/provider_test.go +++ b/oidc/provider/provider_test.go @@ -101,6 +101,13 @@ const ( // the plain unittestclient. const testPKCEClientID = "unittestpkceclient" +// testConfidentialClientID is a client with a secret, for the tests of client +// authentication at the token endpoint. +const ( + testConfidentialClientID = "unittestconfidentialclient" + testConfidentialClientSecret = "unittestsecret" +) + func newTestResourceRegistry(ctx context.Context, t *testing.T) *resources.Registry { fn := filepath.Join(t.TempDir(), "identifier-registration.yaml") conf := "resources:\n" + @@ -155,6 +162,18 @@ func newTestClientRegistry(ctx context.Context, t *testing.T) *clients.Registry t.Fatal(err) } + // A confidential client, which has to present its secret. + err = registry.Register(&clients.ClientRegistration{ + ID: testConfidentialClientID, + Secret: testConfidentialClientSecret, + Name: "Unit test confidential client", + ApplicationType: oidc.ApplicationTypeNative, + RedirectURIs: []string{"http://localhost/callback"}, + }) + if err != nil { + t.Fatal(err) + } + return registry } From 66af4802e8f1a935a89a914d64e88f7c552d3345 Mon Sep 17 00:00:00 2001 From: Simon Eisenmann Date: Wed, 14 Oct 2026 14:10:07 +0200 Subject: [PATCH 2/2] Look up UserInfoHandler's client by azp, not Audience[0] UserInfoHandler took the client from claims.Audience[0], both for the p.clients.Get lookup that decides whether user info is answered signed and for the LookupContext.Audience it hands to the identity manager, which documents that value as the client ID. Since resource indicators (RFC 8707) a token bound to a resource carries the resource as its audience and the client in azp. For such a token the registration lookup missed, so a client which had asked for userinfo_signed_response_alg was answered with unsigned JSON, and the backend was told that the resource was the client. Both now use claims.AuthorizedPartyOrAudience(), as every other lookup of a client by token already does. This also matters for a later change: client IDs which are URLs will be resolved by fetching them, and a lookup by audience would then request the resource URL whenever its host is allowed. The API spec now says that userinfo accepts resource-bound tokens and takes their client from azp. TestAuthorizeAndTokenWithResource gains a subtest which gives the test client a signed user info algorithm, obtains a resource-bound token and expects signed user info whose aud is the client. Without the fix it receives unsigned JSON. --- docs/libregraph-connect-api-v1.yaml | 3 +++ oidc/provider/handlers.go | 8 ++++-- oidc/provider/handlers_test.go | 40 +++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/docs/libregraph-connect-api-v1.yaml b/docs/libregraph-connect-api-v1.yaml index 1c64fe95..3a32fa6c 100644 --- a/docs/libregraph-connect-api-v1.yaml +++ b/docs/libregraph-connect-api-v1.yaml @@ -419,6 +419,9 @@ paths: Only bearer token authentication is accepted here; the identifier session cookie is not consulted. + + An access token bound to a resource is accepted too. Its client is then the one + named in `azp` rather than `aud`, see `AccessTokenClaims`. operationId: getUserInfo security: - bearerAuth: [] diff --git a/oidc/provider/handlers.go b/oidc/provider/handlers.go index 1e02ad3d..6b413b4a 100644 --- a/oidc/provider/handlers.go +++ b/oidc/provider/handlers.go @@ -719,6 +719,10 @@ func (p *Provider) UserInfoHandler(rw http.ResponseWriter, req *http.Request) { var requestedClaimsMap []*payload.ClaimsRequestMap var authorizedScopes map[string]bool + // The client is azp for a token bound to a resource, whose audience is the + // resource rather than the client. + clientID := claims.AuthorizedPartyOrAudience() + userID, sessionRef := p.getUserIDAndSessionRefFromClaims(&claims.RegisteredClaims, claims.AuthorizedParty, claims.SessionClaims, claims.IdentityClaims) ctx := konnect.NewClaimsContext(konnect.NewRequestContext(req.Context(), req), claims) @@ -740,7 +744,7 @@ func (p *Provider) UserInfoHandler(rw http.ResponseWriter, req *http.Request) { authorizedScopes = claims.AuthorizedScopes() auth, found, err = currentIdentityManager.Fetch(ctx, userID, sessionRef, authorizedScopes, requestedClaimsMap, authorizedScopes, &identity.LookupContext{ - Audience: claims.Audience[0], + Audience: clientID, }) if err != nil { p.logger.WithFields(utils.ErrorAsFields(err)).Errorln("identity manager fetch failed") @@ -840,7 +844,7 @@ done: // Support returning signed user info if the registered client requested it // as specified in https://openid.net/specs/openid-connect-core-1_0.html#UserInfoResponse and // https://openid.net/specs/openid-connect-registration-1_0.html#ClientMetadata - registration, _ := p.clients.Get(ctx, claims.Audience[0]) + registration, _ := p.clients.Get(ctx, clientID) if registration != nil { if registration.RawUserInfoSignedResponseAlg != "" { // Get alg. diff --git a/oidc/provider/handlers_test.go b/oidc/provider/handlers_test.go index 3f032879..3ced580c 100644 --- a/oidc/provider/handlers_test.go +++ b/oidc/provider/handlers_test.go @@ -352,6 +352,46 @@ func TestAuthorizeAndTokenWithResource(t *testing.T) { } }) + t.Run("userinfo answers for the client, not the resource", func(t *testing.T) { + // A client which asked for signed user info gets it only when the + // handler finds its registration, which a lookup by the audience of + // a resource-bound token does not. + registration, _ := provider.clients.Get(ctx, "unittestclient") + registration.RawUserInfoSignedResponseAlg = provider.signingMethodDefault.Alg() + defer func() { + registration.RawUserInfoSignedResponseAlg = "" + }() + + code := authorize(t, testResource).Get("code") + status, response := token(t, code, testResource) + if status != http.StatusOK { + t.Fatalf("token returned wrong status code: got %v want %v, response %v", status, http.StatusOK, response) + } + + req, err := http.NewRequest("GET", config.UserInfoPath, http.NoBody) + if err != nil { + t.Fatal(err) + } + req.Header.Set("Authorization", "Bearer "+response["access_token"].(string)) + + rr := httptest.NewRecorder() + router.ServeHTTP(rr, req) + + if rr.Code != http.StatusOK { + t.Fatalf("userinfo returned wrong status code: got %v want %v, body %s", rr.Code, http.StatusOK, rr.Body.String()) + } + if contentType := rr.Header().Get("Content-Type"); contentType != "application/jwt" { + t.Fatalf("userinfo was not signed, content type %q, body %s", contentType, rr.Body.String()) + } + claims := jwt.MapClaims{} + if _, err := jwt.ParseWithClaims(rr.Body.String(), claims, provider.validateJWT); err != nil { + t.Fatal(err) + } + if audience, _ := claims["aud"].(string); audience != "unittestclient" { + t.Errorf("userinfo audience was incorrect, got %v, want unittestclient", claims["aud"]) + } + }) + t.Run("without a resource the token stays bound to the client", func(t *testing.T) { code := authorize(t, "").Get("code")