diff --git a/docs/libregraph-connect-api-v1.yaml b/docs/libregraph-connect-api-v1.yaml index 996d57a3..3a32fa6c 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: @@ -398,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: [] @@ -1950,6 +1974,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..6b413b4a 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 @@ -707,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) @@ -728,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") @@ -828,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 6461ee14..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") @@ -779,4 +819,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 }