Answer invalid_client when token endpoint client authentication fails

docs/6-oidc-upgrade.md

@@ -3,6 +3,32 @@
 This is an upgrade guide from versions 1 → 6. Review the changes and
 apply those relevant to your deployment.
 
+## Version 6.4 to 6.5
+
+New option, disabled by default:
+
+- `ModuleConfig::OPTION_DISALLOW_IMPLICIT_FOR_CONFIDENTIAL_CLIENTS`: if enabled,
+a confidential client requesting the implicit flow is refused with the
+`unauthorized_client` error, so it can only get tokens through the authorization
+code flow. Public clients are not affected.
+
+Changes a client can see at the token endpoint, when it exchanges an
+authorization code as a confidential client:
+
+- A client which presents no client authentication, or a client assertion which
+is refused for having no JWKS to check it with, a signature which does not
+verify, or the wrong Issuer or Audience, now gets `invalid_client`
+(RFC 6749 section 5.2, RFC 7521 section 4.2.1). It got `access_denied`. A client
+which compares the `error` member with the old value needs the new one.
+- A client assertion which can not be parsed, which includes one that has expired,
+lacks a required claim or has a Subject other than its Issuer, gets
+`invalid_client` too. It ended on the SimpleSAMLphp error page, with HTTP 500.
+- All of these answers are HTTP 401, with a `WWW-Authenticate` challenge when the
+request used the `Authorization` header. Their `error_description` does not say
+why the client was refused, which that of `access_denied` did: the reason is in
+the OP log, at `warning` level, with the `client_id`. A wrong client secret, which
+already got `invalid_client`, is logged the same way.
+
 ## Version 6.4.3 to 6.4.4
 
 Claim mappings with type `string`, including mappings without an explicit

src/Server/Grants/AuthCodeGrant.php

@@ -416,8 +416,11 @@ public function respondToAccessTokenRequest(
             $utilizedClientAuthenticationParams[] = ParamsEnum::CodeVerifier->value;
         }
 
+        // A client which neither authenticated nor sent a code_verifier: a backstop, since CodeVerifierRule refuses a
+        // client which is not confidential without a code_verifier, and ClientAuthenticationRule a confidential one
+        // which presents no authentication (RFC 6749 section 5.2).
         if (empty($utilizedClientAuthenticationParams)) {
-            throw OidcServerException::accessDenied('Client authentication not performed.');
+            throw OidcServerException::invalidClient($request);
         }
 
         // OAuth2 implementation

src/Server/RequestRules/Rules/ClientAuthenticationRule.php

@@ -19,6 +19,7 @@
 use SimpleSAML\OpenID\Codebooks\ClientAssertionTypesEnum;
 use SimpleSAML\OpenID\Codebooks\HttpMethodsEnum;
 use SimpleSAML\OpenID\Codebooks\ParamsEnum;
+use SimpleSAML\OpenID\Exceptions\JwsException;
 
 class ClientAuthenticationRule extends AbstractRule
 {
@@ -55,6 +56,22 @@ public function checkRule(
             return new Result($this->getKey(), null);
         }
 
+        // The refusal of a client which does not authenticate (RFC 6749 section 5.2, RFC 7521 section 4.2.1):
+        // invalid_client, HTTP 401, with a WWW-Authenticate challenge when the request used the Authorization header.
+        // The answer does not say why, the log does. A reason can quote what the request sent, before anything about
+        // the request has been verified: the library's answer to an unknown algorithm quotes the alg header, and its
+        // answer to a claim of the wrong type quotes the claim. So the reason is kept to printable ASCII of bounded
+        // length, which leaves it no line break of any kind.
+        $refuse = static function (string $reason) use ($client, $request, $loggerService): OidcServerException {
+            $reason = (string)preg_replace('/[^\x20-\x7E]+/', ' ', substr($reason, 0, 1000));
+            $loggerService->warning(
+                'Client authentication failed: ' . $reason,
+                ['client_id' => $client->getIdentifier()],
+            );
+
+            return OidcServerException::invalidClient($request);
+        };
+
         // Let's check if client secret is provided.
         /** @var ?string $clientSecret */
         $clientSecret = $this->requestParamsResolver->getFromRequestBasedOnAllowedMethods(
@@ -64,7 +81,7 @@ public function checkRule(
         ) ?? $request->getServerParams()['PHP_AUTH_PW'] ?? null;
 
         if ($clientSecret) {
-            hash_equals($client->getSecret(), $clientSecret) || throw OidcServerException::invalidClient($request);
+            hash_equals($client->getSecret(), $clientSecret) || throw $refuse('Client secret does not match.');
             return new Result($this->getKey(), ParamsEnum::ClientSecret->value);
         }
 
@@ -77,7 +94,7 @@ public function checkRule(
         );
 
         if (is_null($clientAssertionParam)) {
-            throw OidcServerException::accessDenied('Not a single client authentication method presented.');
+            throw $refuse('Not a single client authentication method presented.');
         }
 
         // private_key_jwt authentication method is used.
@@ -92,7 +109,14 @@ public function checkRule(
             throw OidcServerException::invalidRequest(ParamsEnum::ClientAssertionType->value);
         }
 
-        $clientAssertion = $this->requestParamsResolver->parseClientAssertionToken($clientAssertionParam);
+        // The library answers whatever is wrong with the token itself with a JwsException: a token which does not
+        // parse, a claim which is missing, of the wrong type or expired, or an Issuer other than the Subject. That is
+        // the client's fault. Anything else, such as the parser failing to start, is the OP's and stays an error.
+        try {
+            $clientAssertion = $this->requestParamsResolver->parseClientAssertionToken($clientAssertionParam);
+        } catch (JwsException $exception) {
+            throw $refuse('Client Assertion could not be parsed: ' . $exception->getMessage());
+        }
 
         // Check if the Client Assertion token has already been used. Only applicable if we have cache available.
         if ($this->protocolCache) {
@@ -103,23 +127,21 @@ public function checkRule(
             );
         }
 
-        ($jwks = $this->jwksResolver->forClient($client)) || throw OidcServerException::accessDenied(
+        ($jwks = $this->jwksResolver->forClient($client)) || throw $refuse(
             'Can not validate Client Assertion, client JWKS not available.',
         );
 
         try {
             $clientAssertion->verifyWithKeySet($jwks);
         } catch (\Throwable $exception) {
-            throw OidcServerException::accessDenied(
-                'Client Assertion validation failed: ' . $exception->getMessage(),
-            );
+            throw $refuse('Client Assertion validation failed: ' . $exception->getMessage());
         }
 
-        ($client->getIdentifier() === $clientAssertion->getIssuer()) || throw OidcServerException::accessDenied(
+        ($client->getIdentifier() === $clientAssertion->getIssuer()) || throw $refuse(
             'Invalid Client Assertion Issuer claim.',
         );
 
-        ($client->getIdentifier() === $clientAssertion->getSubject()) || throw OidcServerException::accessDenied(
+        ($client->getIdentifier() === $clientAssertion->getSubject()) || throw $refuse(
             'Invalid Client Assertion Subject claim.',
         );
 
@@ -133,7 +155,7 @@ public function checkRule(
         ];
 
         (!empty(array_intersect($expectedAudience, $clientAssertion->getAudience()))) ||
-        throw OidcServerException::accessDenied('Invalid Client Assertion Audience claim.');
+        throw $refuse('Invalid Client Assertion Audience claim.');
 
         // Everything seems ok. Save it in cache so we can check for reuse.
         $this->protocolCache?->set(