Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The client id should not be an auto increment integer. #576

Closed
tobiasthaden opened this issue Dec 11, 2017 · 16 comments
Closed

The client id should not be an auto increment integer. #576

tobiasthaden opened this issue Dec 11, 2017 · 16 comments

Comments

@tobiasthaden
Copy link

@tobiasthaden tobiasthaden commented Dec 11, 2017

For sure that was already discussed nevertheless client ids should not be an auto increment integer for several reasons:

  1. It shows information that should be hiddenable: The count of registered clients.
  2. For security reasons: In the event of that a secret key would be public, it is very easy to get the connected client id.

Please, share your thoughts.

@Modelizer
Copy link
Contributor

@Modelizer Modelizer commented Dec 13, 2017

I think UUID should be added as an additional feature. Simply by running a command

php artisan passport:uuid activate

would now perform making client id as UUID. This way passport would be kept simple and whoever want to use this functionality can easily inherit.

@tobiasthaden
Copy link
Author

@tobiasthaden tobiasthaden commented Dec 13, 2017

Related Issues and PRs:

@chadhobson
Copy link

@chadhobson chadhobson commented Dec 15, 2017

Mind-boggling that this STILL hasn't been done. Its a MAJOR breaking change if its really to replace Lucade's server.

@Modelizer
Copy link
Contributor

@Modelizer Modelizer commented Dec 16, 2017

The discussion is going on from Passport 0.1 version. I was expecting it to see it would be added in v2. But still, it is pending.

As per Taylor's comment, I remember he wants to keep Passport simple. But this is been demanded by a lot of people. That's why I said it should be added as an addition or optional feature.

I'll submit a PR if @taylorotwell accept it as an optional command to implement it.

@TimWolla
Copy link
Contributor

@TimWolla TimWolla commented Dec 23, 2017

For security reasons: In the event of that a secret key would be public, it is very easy to get the connected client id.

It's even worse. Quoting the email I sent to Taylor at the end of October without hearing back:

Hi

the /oauth/authorize route allows an attacker to enumerate all the
registered OAuth clients as the only value unknown to them (the
redirect_uri) is optional as of oauth2-server 5. See:
thephpleague/oauth2-server#439

A URL as constructed by an attacker then would look like this:

/oauth/authorize?client_id=CLIENT_ID&response_type=code&scope=

The attacker simply counts up the CLIENT_IDs to retrieve information
about the clients registered in the application:

The route then exposes the redirect_uri (by authorizing) as well as the
Client Name (when using the default view).

Instead of using an sequence value a string from a secure random
generator should be used for the user facing client_id.

Applying the fix to existing clients causes breakage, because the
access_token and refresh_token JWTs encode the client_id.

For our own application I fixed the issue by migrating the clients to a
secure client ID, preserving the old client ID in the database rows and
adding a middleware rewriting the numeric client ID to the secure one
for a few weeks in order to allow consumers to catch up on the change. I
needed to forcefully revoke any existing access tokens in the database
as well.

Best regards
Tim Düsterhus

@mits87
Copy link

@mits87 mits87 commented Jan 3, 2018

Hi, What is the current status of using UUID for client ID?

@josephxanderson
Copy link

@josephxanderson josephxanderson commented Jan 14, 2018

@tobiasthaden @mits87 I just wrote up a Gist on using model observers for a custom ID for Passport Clients. Check it out on GitHub Gists.

However, I am unsure if UUIDs will work. They should, but I'm not sure if the Passport code checks for the ID to be an integer anywhere in the code. But give it a try and let me know if this works for you. I am using it in production currently and have no issues.

@bankorh
Copy link

@bankorh bankorh commented Feb 3, 2018

@josephxanderson, your solution seems not to work correctly. From my understanding, although the value saved into the database is a UUID, but the Client model will return a wrong value for its ID. This is due to the data type of primary key is not overridden in the model so it is always be treated as an integer. Correct me if I'm wrong.

@josephxanderson
Copy link

@josephxanderson josephxanderson commented Feb 3, 2018

@bankorh I have been using this for quite a while and have not had an issue. The Client model returns exactly what's in the database. I think it would only matter if you don't update the oauth_clients table to use the data type you want. Give it a try and let me know if you run into any issues.

@bankorh
Copy link

@bankorh bankorh commented Feb 3, 2018

@josephxanderson in my case, the table data type is correct, and there's no error thrown. But do you notice that the client_id in oauth_personal_access_clients table didn't match any of the id in oauth_clients table?

I did run php artisan passport:install, and the result is always as following. It always show 0 or any random number, however, the Client ID should show UUID.

Personal access client created successfully.
Client ID: 0
Client Secret: pQg9jAd6SP3EwQ37UjTCXzj9ixTggdTinOaDdRF7
Password grant client created successfully.
Client ID: 0
Client Secret: IulAaS9EyLYjwWFtYMj4wRttqfrxw5pajDLFpSb3
@jh3er
Copy link

@jh3er jh3er commented Jul 1, 2018

@josephxanderson me too , im having the same issue when changing all the client_id in migrations to string data type. I hope someone could help with this issue.

@JTallis
Copy link

@JTallis JTallis commented Jul 27, 2018

@Modelizer 's idea would work perfectly here as by default, passport will still work with integers. This will avoid any breaking changes.

Plenty of valid points have repeatedly been made about how non-integer client IDs are preferred with the main one being If your client secret was leaked, it will be harder than going from 1 to N to find a match..

That said, you should have other concerns if your client secret managed to get leaked.

@diadal
Copy link

@diadal diadal commented Jul 28, 2018

I create a simple package for this, it convert client id in all the table to Uuid.
https://github.com/diadal/passport

@bankorh
Copy link

@bankorh bankorh commented Aug 5, 2018

Hey guys, I found a solution for this.

We can register Passport::useClientModel(YourUuidEnabledClient::class); and Passport::usePersonalAccessClientModel(Your UuidEnabledPersonalAccessClient::class).

Sample for my client class.

namespace App;

use Laravel\Passport\Client as BaseClient;

class Client extends BaseClient
{
    public function getIncrementing()
    {
        return false;
    }

    public function boot()
    {
        parent::boot();

        static::creating(function (Model $model) {
            if (!isset($model->attributes[$model->getKeyName()])) {
                $model->attributes[$model->getKeyName()] = Str::orderedUuid()->toString();
            }
        });
    }
}

I use a trait to generate the UUID, but I think the above code should work the same way.

NOTE: You need to use Passport::ignoreMigrations(); and publish the migrations, and update the IDs in migration tables to store UUID.

@mloberg
Copy link

@mloberg mloberg commented Aug 17, 2018

You can also update the migrations and use model listeners. https://mlo.io/blog/2018/08/17/laravel-passport-uuid/

@driesvints
Copy link
Member

@driesvints driesvints commented Oct 16, 2018

Closing this as a duplicate of #14. Please continue discussion there. Thanks.

@driesvints driesvints closed this Oct 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet