Client credentials are not secure for documented use case

[soundsgoodsofar]

Reading the documentation (https://laravel.com/docs/5.6/passport#client-credentials-grant-tokens) makes it sound like a token issued for a client grant would be secure to use for internal machine-to-machine authentication.

However the CheckClientCredentials middleware does not validate that a token is actually a client credential. It will accept a token generated for anything--password grants included. This means that if these routes are reachable, anyone could call these internal endpoints doing who knows what.

The documentation is misleading and dangerous. It makes it sound like these tokens can be used to secure API calls from internal services, but implementing it as documented does nothing of the sort.

I thought maybe I could use scopes to limit the permissions on a per-client basis but that doesn't work either! Any client can request any scopes. Client credentials don't have an associated user, so I can't do user-based permissions either. Seems like client credentials aren't a fully baked concept.

[soundsgoodsofar]

For anyone else trying to sort out how to use this, I duplicated CheckClientCredentials.php to my own class, SecureCheckClientCredentials.php. Change the middleware registration in Kernel.php.

Then you can add something like this after the $psr try-catch block.

$client = Client::findOrFail($psr->getAttribute('oauth_client_id'));

if ($client->password_client || $client->personal_access_client) {
    throw new AuthorizationException;
}

You could just check the client ID directly if you want to restrict to a specific client, or do whatever you want. Personally I'd like to have an attribute, Client->internal_client, the same way we have it for password and personal_access, but I'm trying to do as little surgery as possible and this works for my purposes.

[alexandcote]

Can we bypass this issue with a scope ?

Edit: I just found the anwser: No -> #195 (comment)

[robbytaylor]

@soundsgoodsofar, thanks for that workaround. Did you try opening a PR to put that change into Passport's CheckClientCredentials?

[taylorotwell]

Hmm, maybe I'm confused but it seems to me the different grants are just different ways of obtaining a token. It is still up to you to make sure whatever given token can perform a given action?

It feels like I would probably do this by checking the token directly in the route...

Something maybe like (from within user model - accessToken is already available as a protected property):

public function canDoSomething()
{
     return $this->accessToken && $this->accessToken->client->something();
}

[robbytaylor]

Hmm, maybe I'm confused but it seems to me the different grants are just different ways of obtaining a token. It is still up to you to make sure whatever given token can perform a given action?

This is a valid point. But the name of the CheckClientCredentials middleware, and the associated documentation implies that the CheckClientCredentials middleware is going to verify that the given token is a client_credentials token and can be used without any additional authorisation checks.

It seems like there's two options. One is forcing the application to do further checking of the token, as you say. But in that case it seems like the name of the CheckClientCredentials middleware and the documentation needs to be updated to make it clear that the only check that's being made is that a token has been issued by a valid client.

The other option is to provide a way of doing what the documentation implies is currently available, that is using the CheckClientCredentials middleware to ensure that a route can only be used internally for machine-to-machine requests. This is what I was trying to do with #748.

It feels like I would probably do this by checking the token directly in the route...

Something maybe like (from within user model - accessToken is already available as a protected property):

public function canDoSomething()
{
    return $this->accessToken && $this->accessToken->client->something();
}

I don't think this code would work when a client credentials grant token is required as they're not associated with a particular user.

Scopes already provide a mechanism for enforcing restrictions on what a token can do. And Passport already provides middleware which can be used to check the scopes of a particular token. However, as it stands, scopes can't safely be used to secure routes which should only be accessible internally for machine-to-machine requests. Because any client can issue any scope a user could obtain a password grant token and request a scope which is being used to protect an internal route.

Adding the ability to restrict the scopes a particular client can issue (as in #747) would mean that one client could be used to issue password grant tokens to users while another client could be used to issue client credentials grant tokens for internal use. Only client issuing client credentials grant tokens would be able to issue the scope which would be used to protect the internal route.

Since it's likely that the client issuing password grant tokens would be a public client whereas a confidential client would required to issue client credentials grant tokens it should already be the case that application developers are using two separate clients in each situation.

This issue aside I think #747 is a useful feature which is available in other OAuth libraries. There's a previous discussion on this at #195

I'm sorry if you feel I didn't reply to your comment soon enough or jumped the gun with my pull requests. I opened both PRs before your comment above and have been away since your comment.

[Sephster]

I think that the naming of checkClientCredentials is fairly self explanatory although I can see how people could become confused with the documentation. Surely it is better to just clarify this middleware in the docs to avoid confusion? Personally I would not have thought this middleware would enforce a client credentials grant issued token although I see how some have arrived at this conclusion.

[hackel]

I think there's a lot of confusion because people just don't know a lot about OAuth. The Laravel documentation certainly doesn't need to teach people everything. I'm still just wrapping my head around it.

CheckClientCredentials does exactly what it says: checks that the client credentials (the Bearer token provided by the client) are valid and have not been revoked. The client is only checked to make sure it's valid when the token is first issued (client exists and matches client secret). When you delete (revoke) a client, all of that client's tokens also get revoked, which will be caught by CheckClientCredentials.

This is exactly the same as the auth:api middleware with an authorization/password token. The token just stores the user ID, but it's still up to you to ensure that that user has access to particular resources.

Remember, Passport is for authentication, not authorization. Scopes are not permissions. A scope is basically a request from the end user to grant a 3rd party access to their personal data. It doesn't really make sense with client credentials, since the end user (another server) likely doesn't have any personal information stored to grant itself access to.

What I ended up doing was to simply create a middleware to authorize routes based on the client ID stored in the token. (I actually associate permissions to clients, but that's an implementation detail.) Something like what is described in laravel/framework#24140 would also be helpful, but perhaps too much of an edge case to be included in the framework proper.

[robbytaylor]

What I ended up doing was to simply create a middleware to authorize routes based on the client ID stored in the token.

It's great that your application is simple enough that you can get away with just doing that. But this still sounds like a workaround that it isn't going to scale for more complicated systems. If you wanted to add or change clients that can access this endpoint you'd need to edit your application code.

It sounds like you'd be better off checking for a particular scope to avoid hard coding client IDs in your application. To be able to do that safely though you'd also need to be confident that a normal user can't obtain a token with the scope you're using internally.

[lloy0076]

I asked a similar thing in #753 and the way I've interpreted Laravel Passport's docs is that the provided CheckClientCredentials middleware actually checks that the token was granted using the client_credentials.

What else would such a class do?

use Laravel\Passport\Http\Middleware\CheckClientCredentials;

// But that allows ANY valid token to pass even one gotten via `password` 
// and NOT `client_credentials`. 
//
// HUH?

Yet, this seems to be an almost copy paste of the upstream code itself.

• https://tools.ietf.org/html/rfc6749 (Oauth2 itself)
• https://tools.ietf.org/html/rfc6750 (Bearer Tokens)

...don't seem to have an opinion on it at all.

[robbytaylor]

Having looked into this more, it looks having a whitelist of scopes for clients is a fairly common feature in OAuth libraries, e.g.

http://bshaffer.github.io/oauth2-server-php-docs/overview/scope/
https://identityserver.github.io/Documentation/docsv2/overview/simplestOAuth.html

It was discussed, and subsequently implemented, in league/oauth2-server in 2013. See thephpleague/oauth2-server#24.

The issue here is essentially that client credentials grant tokens can't currently be used to secure endpoints which should only be accessible internally. Adding the ability for clients to have a whitelist of scopes which they can grant tokens for fixes this issue.

There might still be confusion of the naming of the CheckClientCredentials middleware but this could be resolved by expanding the documentation to cover whitelisting scopes by client.

@taylorotwell, could we please revisit #747? This has other benefits unrelated to this issue. For example, if an application has a public client (as defined in https://tools.ietf.org/html/rfc6749#section-2.1), then it may be desirable to limit the scopes that the tokens issued by that client can have.

Alternatively, a centralised OAuth server might use oauth_clients to represent 3rd party applications which can access the system. A client whitelist of scopes would then ensure that a 3rd party application can't request scopes from users which they shouldn't be able to. It might not always be entirely up to users to determine which scopes an application should be able to request on their behalf. Maybe application developers have to pay for API access and the application's level of access depends on their payment plan as well as what the user allows?

[willtj]

@taylorotwell I agree with this:

Hmm, maybe I'm confused but it seems to me the different grants are just different ways of obtaining a token. It is still up to you to make sure whatever given token can perform a given action?

And I would want to basically do what you suggest:

public function canDoSomething()
{
    return $this->accessToken && $this->accessToken->client->something();
}

But rather than adding methods to client or checking the client at all, as I understand it the standard OAuth approach would be to check a property of the token itself, leaving the responsibility for verifying clients with the token issuing code. This approach would also continue to work if in future Passport were to support having an isolated OAuth token issuing server, where the token consumers would have no access to client.

At the moment we can't reliably use scopes in this way because any client can issue a token with any scope. @robbytaylor's change in #747 would address that in a way that's consistent with other libraries' implementations, so it would be great to have that included in Passport.

[driesvints]

Hey everyone. I've marked this as a feature request and also assigned a label so Taylor can have a look later. Thanks for your patience.

[billriess]

Is there any movement on this? What would be the best approach to limit a client_credentials grant to certain scopes?

[robbytaylor]

@billriess, I did it by adding the code in https://github.com/laravel/passport/pull/747/files in my application. You can create your own ScopeRepository and Client classes which extend the Passport classes.

You can make Passport use your classes by using Passport::useClientModel() and $this->app->bind(PassportScopeRepository::class, ApplicationScopeRepository::class) in a service provider.

One thing that's missing from that PR is a database migration to add the scopes field to the database.

[billriess]

@robbytaylor, Thanks for the advice. One question, how are you storing the scopes in the database? I assume your migration would look something like this:

Schema::table('oauth_clients', function (Blueprint $table) {
    $table->text('scopes')->nullable();
});

Then you would store ['some-scope', 'some-other-scope'] on that column?

[robbytaylor]

@billriess, when I implemented I was using MongoDB as the database so the data was stored as an array. But I think you could store it in mysql as a JSON column and then store an array of scopes as you suggest.

[billriess]

@robbytaylor Ah thanks, makes sense.

I may build this out further and add an oauth_scopes table with a oauth_client_scope pivot.

[matt-allan]

I was wondering, if the CheckClientCredentials isn't supposed to verify the access token was granted using the client_credentials grant what is it supposed to do?

The first thing it does is validate the Access token:

$psr = $this->server->validateAuthenticatedRequest($psr);

...which is just a simplified version of what the TokenGuard already does:

return $this->server->validateAuthenticatedRequest($psr);

So you can already use the auth:api middleware for that.

Next it checks the scopes:

        if (in_array('*', $tokenScopes = $psr->getAttribute('oauth_scopes'))) {
            return;
        }

        foreach ($scopes as $scope) {
            if (! in_array($scope, $tokenScopes)) {
                throw new MissingScopeException($scope);
            }
        }

...but you can use the CheckScopes middleware for that.

Then I realized, you can't use the auth middleware because there isn't a User. So that makes sense, you can't use the normal auth middleware because there isn't a user to authenticate.

@lloy0076 I think that answers your question, let me know if that makes sense.

[jasonmccallister]

Not to stir the pot, but if this is allowing access to resources that you are trying to protect, how is this not a security/bug issue?

[matt-allan]

The middleware is still verifying the token and the scopes; it doesn't allow anyone to access the resources.

It's only a security issue if you expected the CheckClientCredentials middleware to restrict the route to access tokens created with the client_credentials grant_type, and were protecting a resource that should only be accessible to client_credentials clients.

Since the documentation doesn't state that the middleware is supposed to work like that I wouldn't consider it a bug. It's certainly a security issue if you assume it will work like that but it's not a security vulnerability in the library.

[driesvints]

Will be in the next major release.

[andrew-belac]

Having looked into this more, it looks having a whitelist of scopes for clients is a fairly common feature in OAuth libraries, e.g.

http://bshaffer.github.io/oauth2-server-php-docs/overview/scope/
https://identityserver.github.io/Documentation/docsv2/overview/simplestOAuth.html

It was discussed, and subsequently implemented, in league/oauth2-server in 2013. See thephpleague/oauth2-server#24.

The issue here is essentially that client credentials grant tokens can't currently be used to secure endpoints which should only be accessible internally. Adding the ability for clients to have a whitelist of scopes which they can grant tokens for fixes this issue.

There might still be confusion of the naming of the CheckClientCredentials middleware but this could be resolved by expanding the documentation to cover whitelisting scopes by client.

@taylorotwell, could we please revisit #747? This has other benefits unrelated to this issue. For example, if an application has a public client (as defined in https://tools.ietf.org/html/rfc6749#section-2.1), then it may be desirable to limit the scopes that the tokens issued by that client can have.

Alternatively, a centralised OAuth server might use oauth_clients to represent 3rd party applications which can access the system. A client whitelist of scopes would then ensure that a 3rd party application can't request scopes from users which they shouldn't be able to. It might not always be entirely up to users to determine which scopes an application should be able to request on their behalf. Maybe application developers have to pay for API access and the application's level of access depends on their payment plan as well as what the user allows?

I will probably do this as well. But OAuth2 if for Authorization. OIDC is for Authentication. (https://oauth.net/2/ - "OAuth 2.0 is the industry-standard protocol for authorization" The IAAM systems I have worked with all allow you to associate specific scopes and resources servers to secific application identities. They don't allow you to request scopes with identities that don't have permission for those scopes. In this case of a Laravel application which issues the access token through Passport and also handles the API request you can do the check internally to see what access they have but the point of the getting an acess token is to authorize the bearer to perform some set actions. This is very useful when the system performing the action is not responsible for making the authorization decision.

My long story is that it would be good and appropriate to limit a passport CLIENT_CREDENTIALS Client to a specific set of scopes. It would also be good if audit logging frameworks would be able to work with the "machine" identities seamlessly as well as with normal user models.

[driesvints]

@andrew-belac see #1272