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

Client_ID #14

Closed
lucasmichot opened this issue Aug 18, 2016 · 71 comments
Closed

Client_ID #14

lucasmichot opened this issue Aug 18, 2016 · 71 comments

Comments

@lucasmichot
Copy link
Contributor

@lucasmichot lucasmichot commented Aug 18, 2016

It feels a bit strange to get a client ID as the primary key of oauth_clients @taylorotwell
Would it not make sense to provide a uuid field for that ?

@jbrooksuk
Copy link
Member

@jbrooksuk jbrooksuk commented Aug 18, 2016

I was wondering the same, UUID, or at least a random string of a given length seems more natural.

Loading

@lucadegasperi
Copy link

@lucadegasperi lucadegasperi commented Aug 19, 2016

Having an auto incrementing key as the client id could expose some unwanted info about the system (like how many clients are registered in the app) the easiest solution would be to hash the id when it's exposed to the outside world. Something like what the https://github.com/vinkla/laravel-hashids package does.

Loading

@nueko
Copy link

@nueko nueko commented Aug 22, 2016

Agreed, the spec tells it should be a string (VSCHAR) https://tools.ietf.org/html/rfc6749#page-71 https://tools.ietf.org/html/rfc6749#section-2.2

Loading

@lucasmichot
Copy link
Contributor Author

@lucasmichot lucasmichot commented Aug 22, 2016

I feel like using a random string in a new indexed key field would be a starting point for this issue...
Any thought on that @taylorotwell ?

Loading

@nueko
Copy link

@nueko nueko commented Aug 23, 2016

Hi, I made this #49 PR. I use md5 to hash userId and name. Is it enough?

Loading

@jbrooksuk
Copy link
Member

@jbrooksuk jbrooksuk commented Aug 23, 2016

md5 is slow, I agree with @lucasmichot, a random string should be enough.

Loading

@nueko
Copy link

@nueko nueko commented Aug 23, 2016

@jbrooksuk, what about unique client_id?

Loading

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Sep 2, 2016

It's fine that it's auto incrementing. Client IDs are public values and there is not a security risk to exposing one. A UUID4 value is not sequential and is not nearly as efficient to index in the database as an auto incrementing value.

Loading

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Sep 2, 2016

That being said, I don't really care either way. It would be a major version break to switch to UUIDs now though.

Loading

@taylorotwell
Copy link
Member

@taylorotwell taylorotwell commented Sep 2, 2016

Some are not understanding that Client IDs are public on Twitter, so I'll post an example here. If you are an OAuth server supporting authorization_code grants your consumers have to include their client IDs directly in the URL when they redirect to your application. For example, I just tried to login to GitLab using GitHub OAuth and could easily just grab GitLab's client ID straight from the URL:

bbe1fe17fd3206756805

Loading

@lucadegasperi
Copy link

@lucadegasperi lucadegasperi commented Sep 2, 2016

@taylorotwell Have you had a look at @vinkla's package? It would solve this problem without altering the database structure. In your bridge to the league's package you would only need to decrypt the string to get the client id and viceversa. The only information leaking from a numeric auto-incrementing client id (which is public) would be the number of clients the service has registered, which sometimes is not desirable.

Loading

@vinothkannans
Copy link

@vinothkannans vinothkannans commented Sep 12, 2016

Let's say I got a client_secret somehow for an unknown client of example.com. So I don't know client_id. In this case, I can easily find out client_id by checking OAuth using 1 to N numbers as client_id in a for loop. But it is not possible (at least not easy) in UUID or random string.

It would be a major version break to switch to UUIDs now though.

I hope at least you can make it is happen in next major version.

Loading

@elynnaie
Copy link

@elynnaie elynnaie commented Sep 13, 2016

I get that security through obscurity is bad, but I still don't want my clients to be able to guess other's ids, and as @lucadegasperi said, it

could expose some unwanted info about the system (like how many clients are registered in the app)

which is the main reason I care about the issue.

Loading

@nikkuang
Copy link

@nikkuang nikkuang commented Sep 27, 2016

As @lucadegasperi suggest, using @vinkla's package.

Might help someone. Here's what I did


use Laravel\Passport\Http\Controllers\AccessTokenController as PassportAccessTokenController;
use Zend\Diactoros\Response as Psr7Response;
use Psr\Http\Message\ServerRequestInterface;

class AccessTokenController extends PassportAccessTokenController
{

    /**
     * Authorize a client to access the user's account.
     *
     * @param  ServerRequestInterface $request
     *
     * @return Response
     */
    public function issueToken (ServerRequestInterface $request)
    {
        $request = $this->overrideRequest($request);

        $response = $this->withErrorHandling(function () use ($request) {
            return $this->server->respondToAccessTokenRequest($request, new Psr7Response);
        });

        if ($response->getStatusCode() < 200 || $response->getStatusCode() > 299) {
            return $response;
        }

        $payload = json_decode($response->getBody()->__toString(), true);

        if (isset($payload[ 'access_token' ])) {
            $this->revokeOtherAccessTokens($payload);
        }

        return $response;
    }

    /*
     * Recreate Request
     */
    public function overrideRequest (ServerRequestInterface $request)
    {
        $requestParameters = (array)$request->getParsedBody();

        if (isset($requestParameters[ 'client_id' ])) {
            $requestParameters[ 'client_id' ] = $this->decode($requestParameters[ 'client_id' ]);
        }

        return new ServerRequest(
            $request->getServerParams(),
            $request->getUploadedFiles(),
            $request->getUri(),
            $request->getMethod(),
            $request->getBody(),
            $request->getHeaders(),
            $request->getCookieParams(),
            $request->getQueryParams(),
            $requestParameters,
            $request->getProtocolVersion()
        );
    }

    public function decode ($value)
    {
        $result = Hashids::decode($value);
        if (count($result) > 0) {
            return $result[ 0 ];
        }
        return -1;
    }
}

Just register the new controller on your preferred route or override the default passport route.

Loading

@ronnievisser
Copy link

@ronnievisser ronnievisser commented Oct 11, 2016

what do we need to do to make the hashid visible to the user? do we need to overwrite the ClientController@forUser method?

Loading

@heisian
Copy link

@heisian heisian commented Nov 2, 2016

Let's think about this:

Why do we want UUIDs or a non-integer non-primary key value for client IDs?

  1. It just seems strange. My client ID is 73? No other public API for any major service uses such a simple client ID scheme. They always use variations of UUID or random hash.
  2. One commenter mentioned being able to guess a client ID easily. I agree with this one, having a UUID or hash doesn't improve security much, but it does prevent an attack where all one would have to do is increment the ID to move onto the next client. @taylorotwell 's response is that client ID's are always publicly available anyways, citing that the ID is right in the URL. Valid as well, but isn't security in the end about increasing complexity? It would take a bit more work for me to intercept the requests being sent to GitLab and nab the client IDs than it would for me to create a script that simply iterates through a set of integers.
  3. Is it more secure? Not vs. an attack where you'd run through all possible values of a client UUID. It's just that it would take longer for an attacker to hit a set of valid client UUIDs vs. integer IDs. But I think that using UUIDs is such a trivial thing to implement that you might as well just go ahead and do it.

That being said, I'll work on a proper pull request for this and get back to you guys when I have something.

Loading

@elynnaie
Copy link

@elynnaie elynnaie commented Nov 2, 2016

@heisian I would add one thing to your list:

  1. It gives away how many clients there currently are. This has nothing to do with security and more to do with revealing sensitive business information. In fact, someone could periodically create a new client and then be able to graph my project's adoption over time, which is not something I want revealed to the public.

At any rate, I did implement this in a fork of my own. I did not bother to write tests because I had no intention of pull requesting it to this project, but feel free to check it out and use it for inspiration or as a base to start with, if you want.

master...myfarms:master

Loading

@heisian
Copy link

@heisian heisian commented Nov 2, 2016

@denaje thanks! Def a good help to get me started.

Loading

@heisian
Copy link

@heisian heisian commented Nov 4, 2016

Hey guys, check my pull request. I don't think Taylor will merge it in since he thinks this thread wholly unnecessary, but my code allows using client UUIDs without introducing breaking changes.

To use UUIDs you would simply add Passport::useClientUUIDs to your AuthServiceProvider.php. If later on you decide not to use UUIDs, you can just remove that call.

If you already have Passport installed, have no fear - I've included a passport:uuid command that will add a uuid column and populate your existing clients as necessary.

If the pull request isn't available you can view my fork: https://github.com/heisian/passport

Loading

@themsaid
Copy link
Member

@themsaid themsaid commented Nov 21, 2016

No plans for this in Passport 1.0 at least.

Loading

@corbosman
Copy link
Contributor

@corbosman corbosman commented Jan 27, 2017

Another reason to want to use non-integers is migration from other oauth2 servers. People migrating from lucadegasperi/oauth2-server-laravel, which was the standard oauth2 server before passport, will need to change all their clients to modify the client id. It would be much nicer if one could migrate without client impact.

Loading

@heisian
Copy link

@heisian heisian commented Jan 27, 2017

I will do the due diligence here and keep my fork up-to-date w/ latest Passport versions. My company's API depends on it and as stated before I much, much prefer the UUID scheme for client IDs.

That being said there's still some loose ends - I don't use vue (I'm only interested in JSON responses, no views whatsoever from my API), so if anyone who does use vue could test those portions of the package to make sure they're working correctly that'd be a huge help.

Will be looking at merging any changes from laravel/passport 2.0 soon, although at a quick glance a lot of those changes were already sitting on master, which is where I originally forked my version from.

The crux of the matter here IMO is retaining integer row_ids for indexing & internal lookup, while client UUIDs get used externally by users of the API.

Loading

@heisian
Copy link

@heisian heisian commented Feb 6, 2017

Latest changes from the 2.0 branch (v2.0.2) of laravel/passport have been merged in. Please notify of any issues in my fork: https://github.com/heisian/passport

Loading

@deividaspetraitis
Copy link

@deividaspetraitis deividaspetraitis commented Mar 6, 2017

As @corbosman mentioned migrations are quite complex without having UUID`s in oficial package. lucadegasperi/oauth2-server-laravel is deprecated and from 5.4 we are forced to migrate to passport that's weird about such breaking changes in application.

Loading

@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

Loading

@mloberg
Copy link

@mloberg mloberg commented Aug 17, 2018

You can use UUIDs by updating the migrations and using model listeners. https://mlo.io/blog/2018/08/17/laravel-passport-uuid/

Loading

@baig772
Copy link

@baig772 baig772 commented Nov 28, 2018

Hey guys, check my pull request. I don't think Taylor will merge it in since he thinks this thread wholly unnecessary, but my code allows using client UUIDs without introducing breaking changes.

To use UUIDs you would simply add Passport::useClientUUIDs to your AuthServiceProvider.php. If later on you decide not to use UUIDs, you can just remove that call.

If you already have Passport installed, have no fear - I've included a passport:uuid command that will add a uuid column and populate your existing clients as necessary.

If the pull request isn't available you can view my fork: https://github.com/heisian/passport

How can I install your fork using composer?

Loading

@heisian
Copy link

@heisian heisian commented Nov 28, 2018

@baig772 I'm not sure if I would recommend it - it is waaay out of date in comparison w/ the latest passport. At my current company we use node on the backend (which I would steer clear of IMO!), so I haven't been in Laravel land for a while.

@jjoao07 I would highly recommend trying @ssmulders's solution instead. I forget the specifics, but his workaround is better than the one I originally had come up with in my fork.

Loading

@billriess
Copy link
Contributor

@billriess billriess commented Dec 20, 2018

Will there be an official movement on this? @taylorotwell

Loading

@driesvints
Copy link
Member

@driesvints driesvints commented Dec 20, 2018

@billriess eventually yeah probably but in the meantime any PRs for this are welcome.

Loading

@grEvenX
Copy link

@grEvenX grEvenX commented Jan 7, 2019

@driesvints There was already a PR on this ~2 years ago: #49.
There was a comment in this thread that md5 is slow, but since that's only used when actually creating the client, I wouldn't think that it would be a big concern?

Any thoughts on re-opening that PR or use the approach there as a possible solution?

Loading

@driesvints
Copy link
Member

@driesvints driesvints commented Jan 7, 2019

@grEvenX not sure tbh. Don't have time atm to dig really deep into it.

Loading

@billriess
Copy link
Contributor

@billriess billriess commented Jan 7, 2019

There is also this one from a while back: #168

Loading

@diadal
Copy link

@diadal diadal commented Aug 19, 2019

Not sure you can try this https://github.com/diadal/passport

Loading

@driesvints
Copy link
Member

@driesvints driesvints commented Aug 20, 2019

@AmirrezaNasiri this is open source software. If you need this then feel free to put some work into a PR.

Loading

@driesvints
Copy link
Member

@driesvints driesvints commented Aug 20, 2019

@grEvenX I think if a new PR was attempted with a very good an thorough explanation and tests that it has a chance of being merged in. Best if you targeted master.

I'm planning on doing some Passport work in September after Laracon EU.

Loading

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca sebastiaanluca commented Dec 21, 2019

I'm currently in the process of setting up a Laravel Passport API. Enabling UUIDs was quite trivial given the few blog posts about it, yet having a more difficult experience enabling hashed client secrets. Will share my progress and code once I have it figured out.

One thing I want to note is that you probably want to use UUIDs for your API clients (either by storing incremental IDs in your database and using HashIds or something similar to do dynamic routing or, even better, just use UUIDs in your database). And more importantly, that client secrets should be hashed, not encrypted, just like Laravel hashes user passwords. AFAIK there's no need for the OAuth server to know the actual secret, just verify the request's secret with the one in the database when issuing tokens.

@driesvints I managed to get salted/hashed client secrets working the same way as user passwords when issuing tokens and accessing client credentials protected endpoints.

Just had to setup the correct db field, enable hashing in the client model, and change the following lines in \Laravel\Passport\Bridge\ClientRepository::validateClient():

return ! $record->confidential() || hash_equals($record->secret, (string) $clientSecret);
return ! $record->confidential() || app(Hasher::class)->check((string) $clientSecret, $record->secret);

Apparently all checks (even the ones in the CheckClientCredentials middleware go through this method. Still have to check if I can override the bridge ClientRepository (for now) by overriding the binding.

Going into weekend/holiday mode, but can probably whip up a PR after. Any tips you can give me on creating a PR with higher chances of getting merged? This feature has to be optional, I suppose? Or do you want to enforce security and require hashed client secrets and introduce a breaking change? I'll add as much documentation and tests as I can.

Loading

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca sebastiaanluca commented Dec 22, 2019

Working WIP for hashed client secrets: #1145.


Not sure how to go about properly introducing UUIDs in the current setup. It'll either be:

  • A breaking change without the option to upgrade (otherwise all existing APIs would have to change their IDs)
  • An optional setting for those starting new projects or willing to accept the downside of upgrading existing IDs themselves (using a fresh migration including UUID fields)
  • Or a solution that introduces an encrypting/decrypting hashids middleware like discussed to mask the internal database IDs (less desirable since they're not real UUIDs then)

Feedback appreciated.

Loading

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca sebastiaanluca commented Dec 27, 2019

For those interested, the PR to optionally use hashed secrets has been merged: #1145 (comment). Will probably be available in the next (major, minor?) release.

Not planning on doing a PR to introduce UUIDs as I have no answer on the questions above and you can implement this yourself quite easily by extending the client model.

Loading

@thamibn
Copy link

@thamibn thamibn commented Jan 9, 2020

using uuid i am getting error when i run php artisan passport:install

General error: 1364 Field 'id' doesn't have a default value

even after moding the boot method of AppServiceProvider.php to somethign like this


<?php

namespace App\Providers;

use Illuminate\Support\Str;
use Laravel\Passport\Client;
use Laravel\Passport\Passport;
use Illuminate\Support\Facades\Schema;
use Illuminate\Support\ServiceProvider;

class AppServiceProvider extends ServiceProvider
{
    /**
     * Register any application services.
     *
     * @return void
     */
    public function register()
    {
        Schema::defaultStringLength(191);
        Passport::ignoreMigrations();
//        Passport::useClientModel(\App\Models\Passport\Client::class);

        if ($this->app->isLocal()) {
            $this->app->register(TelescopeServiceProvider::class);
        }
    }

    /**
     * Bootstrap any application services.
     *
     * @return void
     */

    public function boot()
    {
        Client::creating(function (Client $client){
            $client->incrementing = false;
            $client->id = Str::uuid();
        });

        Client::retrieved(function (Client $client){
            $client->incrementing = false;
        });

    }
}

Loading

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca sebastiaanluca commented Jan 9, 2020

Here's a Passport client model you can use that takes care of this for you: https://gist.github.com/sebastiaanluca/d4b5d2b5611fe9320b7ffa525c5ea0fa. Also be sure to have a correct migration, including $table->uuid('id')->primary();.

Edit: using passport:install is AFAIK not supported. The installation command does not create clients with UUIDs, but relies on the auto incremented ID field which you don't have when using UUIDs. You need to manually set those up.

Loading

@thamibn
Copy link

@thamibn thamibn commented Jan 9, 2020

@sebastiaanluca thanks for your quick reply so how can i create them UUIDs except using the command ``passport:install ```

Loading

@thamibn
Copy link

@thamibn thamibn commented Jan 9, 2020

@sebastiaanluca cause i have tried UUIDs for user table and it works if i use postman however if use the command db:seed it generate the error of id need default value, does that mean when using commands the boot function is not triggered?

Loading

@thamibn
Copy link

@thamibn thamibn commented Jan 10, 2020

@sebastiaanluca i have used that model like Passport::useClientModel(\App\Models\Passport\Client::class); but still i am getting the error? on the AppServiceProvider@register

Loading

@sebastiaanluca
Copy link
Contributor

@sebastiaanluca sebastiaanluca commented Jan 10, 2020

I was mixing things up yesterday, sorry 😅 Field 'id' doesn't have a default value means that Passport wants to create a client, but there's no value for that ID field specified since the migration doesn't allow it to be nullable. Since your code seems correct (as per https://codeburst.io/laravel-passport-assigning-a-uuid-to-your-oauth-clients-211a9bbd5a6e and https://mlo.io/blog/2018/08/17/laravel-passport-uuid/ for instance), I assume it's either a migration thing or something else you've set up differently.

But I'd ask on the Laracasts or laravel.io forums as this issue isn't meant to support non-existing features. I'm sure they can help you better there.

Loading

@thamibn
Copy link

@thamibn thamibn commented Jan 10, 2020

@sebastiaanluca thanks for your help it is now working i made a mistake on the migrations and change all 'id' fields to uuid, only client_id and user_id fields needs to be uuid since my user model also uses uuid.

test repo https://github.com/thamibn/testrepo.git

Loading

@laravel laravel deleted a comment from errawn Feb 10, 2020
@grEvenX
Copy link

@grEvenX grEvenX commented Feb 29, 2020

With the possibility to override the OAuthClient and allow $keyType and $incrementing to be set, is there still a reason to keep this issue open?

Loading

@driesvints
Copy link
Member

@driesvints driesvints commented May 4, 2020

We've implemented a PR to automatically configure clients with UUID's when you set them up using the passport:install command. This will be in tomorrow's release.

Given the gigantic impact with making UUID's the default we'll not be reconsidering that particularly approach. At least now there'll be a native way to do this.

Loading

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