Client_ID

[lucasmichot]

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]

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

[lucadegasperi]

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.

[nueko]

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

[lucasmichot]

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 ?

[nueko]

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

[jbrooksuk]

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

[nueko]

@jbrooksuk, what about unique client_id?

[taylorotwell]

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.

[taylorotwell]

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

[taylorotwell]

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

[lucadegasperi]

@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.

[vinothkannans]

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.

[elynnaie]

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.

[nikkuang]

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.

[ronnievisser]

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

[heisian]

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.

[elynnaie]

@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.

https://github.com/laravel/passport/compare/master...myfarms:master

[heisian]

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

[heisian]

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

[themsaid]

No plans for this in Passport 1.0 at least.

[corbosman]

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.

[heisian]

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.

[heisian]

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

[deividaspetraitis]

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.

[salmanhijazi]

@nikkuang I tried your method, but I get this error:

Class 'App\Http\Controllers\ServerRequest' not found

Do I need to add something else apart from these?

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

Thanks!

[nikkuang]

@salmanhijazi try adding this use Zend\Diactoros\ServerRequest;

[salmanhijazi]

@nikkuang Thank you!

[simonhamp]

Big props to @nikkuang for sharing a simple implementation. With a minor adjustment, it works perfectly with the latest version of Passport!

FWIW, I find this implementation a lot nicer than adding UUIDs to the database, which is overly complicated. As the Client ID hashes are never stored, they can all easily be 'revoked' simply by changing the settings in your config/hashids.php (salt or length), which is a great way of locking everything down in a worst-case scenario.

[hfmikep]

I'll agree that I really don't like giving a major partner the client id "1". I will implement the HashIDs solution for now, but I would love it if @taylorotwell would add some a client_id field in oauth_clients that could be used for lookup. Could default to a random string or allow you to put anything you want it the field (md5, uuid, etc.).

[hfmikep]

So using @nikkuang idea, I decided to go a different route. Simply using middleware to translate the client_id request parameter from hashids back to integers. It looks something like this. This allows both integers and hashids, a good solution in case you already have existing clients using the integers.

If you do not want to allow integers to pass right through, you can remove the ! is_numeric check.

This solution should not break with any future passport updates.

<?php

namespace App\Http\Middleware;

use Closure;
use Hashids;

/**
 * Class ClientHashId
 *
 * @package App\Http\Middleware
 */
class ClientHashId
{

    /**
     * Handle an incoming request.
     *
     */
    public function handle($request, Closure $next)
    {

        if ( $request->offsetExists('client_id') ) {

            $client_id = $request->offsetGet( 'client_id' );

                        // Remove this "if" statement, if you do not want to allow the integer client_ids.
            if ( ! is_numeric( $client_id ) ) {

                $result = Hashids::connection('client_id')->decode( $client_id );
                if ( count($result) > 0 ) {
                    $request->offsetSet( 'client_id', $result[0] );
                } else {
                    $request->offsetSet( 'client_id', -1 );
                }
            }
        }

        return $next($request);
    }

}

In your app\Http\Kernel.php you need to register it in your $routedMiddleware

    protected $routeMiddleware = [
        'client.hashid' => \App\Http\Middleware\ClientHashId::class,
        'throttle'   => \Illuminate\Routing\Middleware\ThrottleRequests::class,
    ];

In routes/web.php I overwrite the route with the new middleware.

//oAuth
Route::post( 'oauth/token', '\Laravel\Passport\Http\Controllers\AccessTokenController@issueToken' )->middleware(['throttle','client.hashid']);

In config/hashids.php I added a connection specifically for client_id:

'connections' => [

        'main' => [
            'salt' => 'add_your_salt_here',
            'length' => '8',
        ],

        'client_id' => [
            'salt' => 'add_your_salt_here',
            'length' => '32',
        ],

    ],

[corbosman]

That might be a good workaround until laraval joins the rest of the secure world. It's embarrassing passing our oauth clients client_id '9' while the rest of the world uses unguessable strings. Ive had to explain the reason several times now.

[SerdarSanri]

So it looks like after a year still no progress on this. We are still using client_id 5 in a corporate world. @taylorotwell any plans on implementing something related to this?

[corbosman]

Foursquare: ZYDPLLBWSK3MVQJSIYHB1OR2JXCY0X2C5UJ2QAR2MAAIT5Q Github: 6779ef20e75817b79602 Google: 292085223830.apps.googleusercontent.com Instagram: f2a1ed52710d4533bde25be6da03b6e3 SoundCloud: 269d98e4922fb3895e9ae2108cbb5064 Github: 6779ef20e75817b79602 Laravel: 1

There are all kinds of reason to treat this public string at least as unguessable. I'll name a few:

• anyone that is a client would know how many clients you have and can guess their client id
• there is a perception issue, where people that are used to receiving random character strings and are suddenly passed the id '2' think it's not secure. (and they do have a bit of a point).
• it is less secure because an attacker could craft phishing strings more easily because the client_id is not random.
• as a maintainer of the oauth server I have to keep explaining this to people

[dingdayu]

Really sorry, until now, there is still no official guide.

[ssmulders]

I agree wholeheartedly that the client_id shouldn't be based on the integer index. If not for the sole reason that it looks so damn ugly. @hfmikep's middleware to translate Hashid'd integer incoming requests works perfectly! There was only thing I found missing: making that far superiorclient_id visible / accessible to the end-user.

So I continued by adding some code to actually serve the hashed integer as theclient_id, so the incrementing integer is hidden from plain sight.

Just add the following controller to the controllers folder:

<?php

namespace App\Http\Controllers;

use Illuminate\Http\Request;
use Laravel\Passport\Http\Controllers\ClientController as PassportClientController;

class ClientController extends PassportClientController
{
    /**
     * Get all of the clients for the authenticated user.
     *
     * @param  \Illuminate\Http\Request $request
     * @return mixed
     */
    public function forUser(Request $request)
    {
        $userId = $request->user()->getKey();
        $clients = $this->clients->activeForUser($userId)->makeVisible('secret');

        foreach ($clients as $index => $client)
        {
            $clients[$index]['client_id'] = \Hashids::connection('client_id')->encode($clients[$index]['id']);
        }

        return $clients;
    }
}

And then add this line to your web.php:

Route::get('/oauth/clients', 'ClientController@forUser')->middleware('auth');

By doing this + what @hfmikep posted above, you've basically got the whole thing covered from request to response in a way that should be compatible with future releases, and requires no migrations.

When you use the Frontend VueJS Quickstart all you need to do is change {{ client.id }} to {{ client.client_id }} in Clients.vue and you're good to go!

I may have missed something but this was sufficient for my immediate needs. Here's what you end up with:

Client ID: RzvkwOgDrdep4Woq3n906L5EQA8mxXjq

Client Secret: xCPMQImPNZPP9IuYzh4ljVTEvuw4bYmZAnsLjxso

Voilá! 🎉

[heisian]

@ssmulders Glancing at this really quickly, I'm not sure I see how a client would pass in a hash to retrieve their record.

[ssmulders]

@heisian it doesn't ;-) That's why I referenced @hfmikep a few times. His post contains the ClientHashId middleware that tackles the hash > integer conversion while mine takes care of the integer > hash side of the problem / solution.

Didn't want to bloat the threat any further by copying everything into one post but I'll start work on a package. Should be a good exercise 🚀

[ssmulders]

And done! Please have a look and test it out: https://github.com/ssmulders/hashed-passport

It now works for both the requests and responses by adding a new middleware called hashed_passport. Add this to any route that would normally send or receive the Client ID as an integer to make it send and accept the hashed Client ID :)

[heisian]

@ssmulders This is a cool idea and has a few advantages over my solution:

• Doesn't modify Passport
• Doesn't require any migrations or alter any data
• Can be disabled without any other side-effects

I feel like I would need to use it myself to find any downsides, but just glancing at the repo seems good.

Nice work!

[ssmulders]

@heisian Thanks Tim! Haven't tested it on the full oAuth flow as I'm still getting to grips with the full standard. It's verified to work for the password and client_credentials grants.

So if anyone else is able to test it with the authorization_code grant, please create a pull request and I'll merge it in 👍

[corbosman]

Nice, now all we need is to have the secrets not stored in plaintext. That is such a naieve thing to do.

[heisian]

@corbosman I think because secrets are just random strings and not user-defined, like passwords, the idea is that there's no real need to encrypt them - the fallback being that the secret can just be reset.

Going through the RFC OAuth spec can shed more light on this.

[simonhamp]

@corbosman There’s no benefit in encrypting or hashing client secrets IMHO.

If someone manages to get to the database storing them, you’ve got bigger problems.

They should also be of little use on their own. You should need client details AND user details to get at anything remotely useful.

If your API allows tokens generated via a client_credentials grant to access user data, you should probably think about your security model.

If an attacker is able to get client credentials over the wire (which is super easy), hashing them won’t have been any help.

The main considerations for client secrets are guessability and uniqueness. Sufficiently long, random strings that can be changed if compromised is fine.

[ssmulders]

@corbosman @heisian Have a look at version 1.1 of ssmulders/hashed-passport that has just been pushed. I've included the option to use encrypted client secrets. Again, non-intrusive. All that's needed is to run php artisan hashed_passport:install to trigger a migration change() and add HashedPassport::enableEncrytion() to the AppServiceProvider This alters the maximum number of characters on the secret column, and then uses a ClientObserver to encrypt and decrypt the secret as it enters and leaves the database.

It fully supports rollbacks and can even be uninstalled at a later date, decrypting all the secrets outside of the migration flow for maximum compatibility.

The use of an Observer instead of Middleware for the response is an improvement as well. The hashed client id is now available everywhere in the application through the client_id parameter on the \Laravel\Passport\Client model.

@simonhamp I agree, you definitely do have bigger problems then ;-) However there are still some benefits IMHO. One upside is that something sensitive is stored with encryption. So if the database is breached the hashed User passwords will be useless and the encrypted oAuth Clients will be as well (granted that the DB and Application are on separate servers). So someone with ill intentions can't just start accessing your API by copy/pasting the id and secret from the database. It's another roadblock.

@taylorotwell rightfully argued that there's no benefit to using a hashed id either and look how big this thread ended up ;-) There's a big range of projects and use-cases that Laravel ends up in so it's impossible (in my opinion) to argue 100% in favour of either side. So hopefully making it optional in the package should do the trick!

[corbosman]

Thanks for the work @ssmulders. Very nice. I think these features add a lot of value to passport.

[ssmulders]

I just released 1.2 of Hashed Passport and I think that about does it feature-wise. It now includes:

• Hashed Passport client IDs
• Optional encryption of client secrets (does require a migration, but a minor one)
• Optional custom salt (default is to use the APP_KEY)

Everything is setup in such a way that it's 100% non-intrusive and reversible. I did move some of the functionality out of the migration, that felt a bit too hacky and might lead to problems down the road. This functionality has now been moved to php artisan hashed_passport:install and uninstall.

Open a feature request if you'd like to see anything else!

[jacinto-joao]

Hi @heisian how do you implement your solution?

I tried following what you suggested, but I can't get it working. First attempt: ` public function boot() { $this->registerPolicies();

    Passport::useClientUUIDs();
    Passport::ignoreMigrations();
}

` results:

second attempt trying to get your package via composer

But can't get it working. I'm I missing something, please.

I'm using laravel 5.6, upgraded from 5.5

regards

[diadal]

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

[mloberg]

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

[baig772]

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?