Password Grant should not require client_secret

[billriess]

thephpleague/oauth2-server#889

According to this discussion, thephpleague/oauth2-server no longer requires client_secret to be passed for password grants but Passport does require it. Can we make this optional as it is in thephpleague/oauth2-server?

[driesvints]

Afaik I've seen this pop up before but we need v8 to be released before we can make changes? @Sephster

[billriess]

According to https://github.com/thephpleague/oauth2-server/blob/master/CHANGELOG.md#500-rc1---2016-03-24

Support for public clients was added in 5.0.0-RC1 back in 2016.

[billriess]

So if the client has no secret set then it will work, maybe the change needed is to ask whether or not the client created via artisan is a public or private client?

The documentation should mention that client_secret is optional as long as it is not set in the database.

[Sephster]

In the current implementation you can omit the client secret in the request for an access token using a password grant. Instead you must use HTTP Basic Auth to validate with the auth server.

I don't think the existing implementation supports public clients but I haven't tested this out fully.

[billriess]

In Passport, omitting the client_secret will work but only if the oauth_client has a NULL secret value in the database. If the secret is set, to anything, it will require the client_secret to be passed. I'm assuming this is by design. This is not documented, however, and using artisan passport:client --password will automatically set a secret making this more of a hidden feature.

[Sephster]

I don't think this is the case but again, I would need time to test this. The current validation works as follows:

list($basicAuthUser, $basicAuthPassword) = $this->getBasicAuthCredentials($request);
$clientId = $this->getRequestParameter('client_id', $request, $basicAuthUser);

if ($clientId === null) {
    throw OAuthServerException::invalidRequest('client_id');
}

// If the client is confidential require the client secret
$clientSecret = $this->getRequestParameter('client_secret', $request, $basicAuthPassword);

In the above, we first try to get a client ID and password using basic auth. We then try to get the client secret that has been passed in as a request parameter, and if this isn't present, we use the basic auth password to authenticate.

We then force validation on the client secret (whether this is via basic auth or a request parameter) and if this validation fails e.g. we don't get a valid client, an authentication error is issued.

[billriess]

Hmm, in Passport I can currently use the flow I described above.

I generate a new oauth_client, then NULL the secret on that client in the database. Next I use Postman to issue:

POST /oauth/token

grant_type:password
client_id:{{client_id}}
username:{{username}}
password:{{password}}
scope:{{scope}}

Which returns a valid access_token.

[J5Dev]

Just tested this, and it seems the migrations set the secret column on oauth_clients to not allow null. For this to work, you need to manually add the Passport migrations, and set the column to be nullable.

Laravel version: 5.7.28
Passport Version: 7.2.0

[billriess]

This makes sense, I'm extending Passport and had to copy the migrations in to add other functionally (multi-auth) but what I added would not affect the secret being required or not.

[Sephster]

Passport checks the client secret like so:

if ($mustValidateSecret &&                                                                                      
    ! hash_equals($record->secret, (string) $clientSecret)) {                                                   
    return;                                                                                                     
}         

If you don't pass a client secret, the default value is null which is cast to a string above. That would become just a blank string. I'm assuming your secret in the database is also a blank string which is why your implementation is working.

At the moment though, I don't think the library supports the passing of no secret at all officially.

[billriess]

Passport checks the client secret like so:

if ($mustValidateSecret &&                                                                                      
    ! hash_equals($record->secret, (string) $clientSecret)) {                                                   
    return;                                                                                                     
}         

If you don't pass a client secret, the default value is null which is cast to a string above. That would become just a blank string. I'm assuming your secret in the database is also a blank string which is why your implementation is working.

At the moment though, I don't think the library supports the passing of no secret at all officially.

You're right, this is exactly what's happening. So you can get around it by having an empty string set as the secret. There doesn't seem to be any validation against whether client_secret is passed in the request. Since its null then cast as a string it matches just like you said.

[Sephster]

Yeah I think this is a fortunate (for you) quirk of the implementation. I will add an issue to the League's repo to see if we can get this supported officially. I know in the changelogs it says this was done but I don't believe this to be the case. It might have been removed at a later date.

Edit: Sorry I realise we already have an issue open for this. Interestingly, this was discussed back in 2015 and the decision was taken to not support public clients using the password grant by default. See this comment here thephpleague/oauth2-server#153 (comment) - It might be the case that we continue with this decision but I would have to look into it further

[billriess]

@Sephster Thanks for looking into this. While I agree this may require a bigger discussion, perhaps in phpleague first, I do believe this should be allowed. At the end of the day, the developer should know the different grant types and risks each type proposes. In reality, there really is no difference between requiring and not requiring the client_secret in a SPA or Mobile Application since those sources can't be trusted in the first place. Leaving out the client_secret in this case makes more sense because its one less piece of information you need to expose. The only thing an "attacker" can do is phish, which is certainly a risk, but there really is no 100% in all of the oauth flow that would prevent that anyway.

[sg3s]

This is very opinionated, but you should probably just not use or allow password grants in the first place, forget without a secret, which would imply you are using it with a public client, being even less secure.

For SPA and mobile development the authorization code grant with PKCE is highly recommended and current best practice. This greatly limits the potential attack surface vs a password grant.

Please refer to the RFC specifying this for the multitude in reasons behind it.
https://tools.ietf.org/html/rfc8252

[driesvints]

@sg3s currently waiting for the v8 release of oauth2-server before we can add support for PKCE in Passport. See #837

[sg3s]

@driesvints Got it. Was not aware it blocked PKCE support in Passport but I know of the changes in v8. Doesn't change what the IETF recommendations are though.

[Sephster]

I've released version 8. Cheers for everyone's patience. 👍

[driesvints]

@Sephster thanks. Now we can add this to Passport as well. If anyone wants to get this into Passport, feel free to send in a PR to the master branch.

[matt-allan]

Version 8 requires the client_secret again for password grant so there is nothing that can be done in Passport at the moment. This issue should probably be closed in favor of thephpleague/oauth2-server#889.

[driesvints]

@matt-allan okay, thanks.

[matt-allan]

Just in case anyone is still following this issue, this will be possible in the next major version:

passport/tests/BridgeClientRepositoryTest.php

Line 77 in d77c727

public function test_public_client_password_grant_is_permitted()

[jlubeck]

@matt-allan would that be v8? Any idea if that is going to be this year? Thank you!

[driesvints]

I still haven't found time to work on it, sorry. But I think that within a week or two I'll have some availability.

[donmbelembe]

V8 is out but i'm still confused on how to work with this

[Mina-R-Meshriky]

I don't think that this issue should be closed, as the PKCE is only implemented for the "Authorization Grant" and is still not implemented for the password grant

[matt-allan]

@Mina-R-Meshriky PKCE is a technique to mitigate the authorization code being intercepted. Because the password grant does not use an authorization code it isn’t possible to use the PKCE extension.

[Mina-R-Meshriky]

@matt-allan you are right my bad the Specification clearly states that the PKCE is for the Authorization Code Grant.

[borgogelli]

@matt-allan , please could you give us some clarifications ?
Is it possibile with Passport v9 to implement a password grant flow without using any client secret ?
If yes, what values should I use for client_secret and client_id in my rest api call ?
Thank you in advance

[matt-allan]

Yes, as far as I know this should work as indicated by the tests.

You need to create a 'public' client, either via the console command (php artisan passport:client --public) or the UI (don't check the confidential checkbox).

After doing that you would pass the client_id Passport provides you and you would not pass a client_secret in your rest API call.

That being said, I would strongly recommend using the authorization code flow instead if you would like to create a public client. You can skip the authorization prompt if it's a first party client. It's more secure and provides other benefits as explained here. I wrote a guide and an example app showing how to use the auth code flow with a public client here.