Answer token endpoint failures with the RFC 6749 error codes, and look up the UserInfo client by azp

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="<issuer>"`.'
+              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

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.

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
 		}

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.

oidc/provider/handlers_test.go

@@ -352,6 +352,46 @@
 		}
 	})
 
+	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 @@
 			}
 		}
 	})
+
+	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
 }

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
 }