v9 Client Credentials grant broken

[shades684]

Since version 9 the client credentials grant fills the subject of the jwt token with the client identifier
On line 56 of League\OAuth2\Server\Grant\ClientCredentialsGrant the userIdentifier is set to null
On line 107 of League\OAuth2\Server\Entities\Traits\AccessTokenTrait the SubjectIdentifier is therefor filled with de identifier of the client

When using the given jwt token:
On line 132 of League\OAuth2\Server\AuthorizationValidators\BearerTokenValidator the oauth_user_id is set to the value of sub (which is the subject identifier mentioned above)

The definition of sub says:

sub (subject): Subject of the JWT (the user)

This value if used in
Line 91 of League\Bundle\OAuth2ServerBundle\Security\Authenticator checks if sub should return a NullUser if we don't have a user, but now it doesn't and we get an error.

I expect the SubjectIdentifier of the AccessTokenTrait should be nullable and not return the client id in the subject if we have no user.
I don't know if this has been done intentionally or my assumptions are correct.

[hafezdivandari]

Using "client credentials" grant, the client is the resource owner, so is the subject of the JWT token.

[shades684]

I can agree with that, but that doesn't make it the user, so maybe the oauth_user_id shouldn't be filled with the sub if the sub is filled with the client id?

[ajgarlag]

@shades684 Is this issue related to thephpleague/oauth2-server-bundle#191?

[hafezdivandari]

I can agree with that, but that doesn't make it the user, so maybe the oauth_user_id shouldn't be filled with the sub if the sub is filled with the client id?

I understand what you mean and it is a naming problem IMO. The oauth_user_id could be oauth_owner_id maybe, or adding a oauth_owner_type with 'client'|'user' may help. But the user is the one who use the token to access the authenticated resource, so the client is the resource owner and also is the user!

We also rely on the new behavior of v9 on Laravel Passport to check whether the token's resource owner is the client itself or not.

You may check the situation too like we did and set oauth_user_id to null?

if ($request->oauth_user_id === $request->oauth_client_id) {
    $request->oauth_user_id = null;
}

[shades684]

@ajgarlag bartholtbos explains the same problem in that issue, but I think he's searching for the solution in the wrong project.
This project fills the oauth_user_id with the client identifier of the sub. But the identifier in the sub is not a user identifier.

@hafezdivandari though i think your solution will work, it feels like we're testing if 2 ids from seperate places are the same. Theoretically they could actually be the same id.

@hafezdivandari
An option might be to add a custom field to the jwt describing what the sub contains? I really don't see a better way to set the oauth_user_id to null unless the sub is left to null when no user identifier exists.

[hafezdivandari]

though i think your solution will work, it feels like we're testing if 2 ids from seperate places are the same. Theoretically they could actually be the same id.

@shades684 you are right, this can't be used if client and user IDs are integers for example. We are using UUID for client IDs on Passport so we are safe. But your point is totally valid.

An option might be to add a custom field to the jwt describing what the sub contains? I really don't see a better way to set the oauth_user_id to null unless the sub is left to null when no user identifier exists.

I agree. It seems wrong to map sub into oauth_user_id without having additional describing attribute!

[hafezdivandari]

Ok, I finally found the related RFC9068 which is too important to know what the sub should be:

sub REQUIRED - as defined in Section 4.1.2 of [RFC7519]. In cases of access tokens obtained through grants where a resource owner is involved, such as the authorization code grant, the value of "sub" SHOULD correspond to the subject identifier of the resource owner. In cases of access tokens obtained through grants where no resource owner is involved, such as the client credentials grant, the value of "sub" SHOULD correspond to an identifier the authorization server uses to indicate the client application. See Section 5 for more details on this scenario. Also, see Section 6 for a discussion about how different choices in assigning "sub" values can impact privacy.

[Sephster]

This is a tricky one to resolve. It's correct that the subject should be filled with the client ID when using the client credentials grant as noted by @hafezdivandari.

The problem is populating the oauth_user_id field with it when it is clearly not a user ID as it could cause confusion with implementations.

I'm not entirely sure what the best way forwards is with this. We could rename the request field to oauth_owner_id or something similar in a later release but if we did this, it would then be correct to assign the sub to this field and I assume you'd still have issues as you want to know when no resource owner is present? Am I understanding that correctly?

My gut is telling me we change the name of this in a later release to satisfy when the sub switches between a client ID or resource owner ID and that a modified trait is used in the League Bundle to reinstate the old behaviour for Symfony users in the meantime.

[hafezdivandari]

Adding a custom claim sub_type may help:

// src/Entities/Traits/AccessTokenTrait.php

 private function convertToJWT(): Token
 {
     // ...

     return $this->jwtConfiguration->builder()
         // ...
         ->relatedTo($this->getSubjectIdentifier())
+        ->withClaim('sub_type', $this->getSubjectType())
         ->getToken($this->jwtConfiguration->signer(), $this->jwtConfiguration->signingKey());
 }

+ private function getSubjectType(): string
+ {
+     return is_null($this->getUserIdentifier()) ? 'client' : 'user';
+ }

 private function getSubjectIdentifier(): string
 {
     return $this->getUserIdentifier() ?? $this->getClient()->getIdentifier();
 }

// src/AuthorizationValidators/BearerTokenValidator.php

 public function validateAuthorization(ServerRequestInterface $request): ServerRequestInterface
 {
     // ...

     return $request
         ->withAttribute('oauth_access_token_id', $claims->get('jti'))
         ->withAttribute('oauth_client_id', $claims->get('aud')[0])
-        ->withAttribute('oauth_user_id', $claims->get('sub'))
+        ->withAttribute('oauth_user_id', $claims->get('sub_type') === 'user' ? $claims->get('sub') : null)
+        ->withAttribute('oauth_owner_id', $claims->get('sub'))
+        ->withAttribute('oauth_owner_type', $claims->get('sub_type'))
         ->withAttribute('oauth_scopes', $claims->get('scopes'));
 }

PS: Adding custom claims would be easier after #1328

[iampedropiedade]

Hello.

Can you please let me know if this has been fixed?

I had to downgrade to thephpleague/oauth2-server-bundle v0.8 which uses league/oauth2-server v8.3 for the client credentials to work.
Is there a manual fix to make it work with V9?

Here's with v8

And here's with v9:

[ajgarlag]

After reading Section 5 of RFC9068, I think the Authorization server should ensure that if the sub and aud[0] values are the same, the resource owner is always a client. So, there would be no need to convey the sub_type claim in the JWT.

Authorization servers should prevent scenarios where clients can affect the value of the "sub" claim in ways that could confuse resource servers. For example, if the authorization server elects to use the client_id as the "sub" value for access tokens issued using the client credentials grant, the authorization server should prevent clients from registering an arbitrary client_id value, as this would allow malicious clients to select the sub of a high-privilege resource owner and confuse any authorization logic on the resource server relying on the "sub" value. For more details, please refer to Section 4.14 of [OAuth2.Security.BestPractices].

[hafezdivandari]

I think the Authorization server should ensure that if the sub and aud[0] values are the same, the resource owner is always a client. So, there would be no need to convey the sub_type claim in the JWT.

The problem is that, based on the implementation of the auth server, the sub and aud[0] may be the same but the resource owner is not a client. For example assume the client ID of '3' and User ID of '3', the sub and aud[0] are the same but we can't say that if the User is the resource owner or the client! So I think we may have to explicitly add sub_type!

[ajgarlag]

The problem is that, based on the implementation of the auth server, the sub and aud[0] may be the same but the resource owner is not a client. For example assume the client ID of '3' and User ID of '3', the sub and aud[0] are the same but we can't say that if the User is the resource owner or the client! So I think we may have to explicitly add sub_type!

I see the issue, but if this project follows the RFC9068, it should not generate the same sub claim for the client_id 3 and the user_id 3.

If the project does not follow the RFC9068, which is not referenced in the README, I see no problem with the old behavior: having an empty string as the sub claim when the resource owner is a client.

[hafezdivandari]

Yes, but some legacy projects may have used incremental IDs for both clients and users repositories without knowing the consequences. Nothing stops the implemented auth server to do so, I mean this package doesn't / can't enforce how the implemented auth server handles client / user IDs and no, not everyone reads all the related RFCs before implementing unfortunately!

However, as Sephster said on this comment, this package should follow RFC9068 and mention it on the readme.

[ajgarlag]

Yes, but I think that the library can make some string manipulation in getSubjectIdentifier so that when the client ID and the user ID have the same value, they generate different sub claims.

The problem for me is to find what type of string manipulation should be done. I've coded a quick POC using json_encode.

Anyway, I hope this issue can be fixed soon.

diff --git a/src/AuthorizationValidators/BearerTokenValidator.php b/src/AuthorizationValidators/BearerTokenValidator.php
index 0442dd48..2a8f443c 100644
--- a/src/AuthorizationValidators/BearerTokenValidator.php
+++ b/src/AuthorizationValidators/BearerTokenValidator.php
@@ -125,11 +125,14 @@ class BearerTokenValidator implements AuthorizationValidatorInterface
             throw OAuthServerException::accessDenied('Access token has been revoked');
         }
 
+        $clientCredentials = json_decode($claims->get('sub')) === $claims->get('aud')[0];
+
         // Return the request with additional attributes
         return $request
             ->withAttribute('oauth_access_token_id', $claims->get('jti'))
             ->withAttribute('oauth_client_id', $claims->get('aud')[0])
-            ->withAttribute('oauth_user_id', $claims->get('sub'))
+            ->withAttribute('oauth_user_id', $clientCredentials ? '' : $claims->get('sub'))
+            ->withAttribute('oauth_owner_id', $clientCredentials ? $claims->get('aud')[0] : $claims->get('sub'))
             ->withAttribute('oauth_scopes', $claims->get('scopes'));
     }
 }
diff --git a/src/Entities/Traits/AccessTokenTrait.php b/src/Entities/Traits/AccessTokenTrait.php
index 6b1387b5..f5928de4 100644
--- a/src/Entities/Traits/AccessTokenTrait.php
+++ b/src/Entities/Traits/AccessTokenTrait.php
@@ -104,6 +104,6 @@ trait AccessTokenTrait
      */
     private function getSubjectIdentifier(): string
     {
-        return $this->getUserIdentifier() ?? $this->getClient()->getIdentifier();
+        return $this->getUserIdentifier() ?? json_encode($this->getClient()->getIdentifier());
     }
 }