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

Make tokens identifiable with prefix and checksum #459

Merged
merged 4 commits into from
Aug 15, 2023

Conversation

marzvrover
Copy link
Contributor

@marzvrover marzvrover commented Aug 7, 2023

This PR adds prefixes and CRC32 checksums to tokens.

This makes generated tokens more identifiable to increase security posture by enabling the use of secret scanning technologies such as GitHub's secret scanning. With custom prefixes this allows developers onboard secrets they mint to to the GitHub secret scanning partner program.

laravel/framework#47977

Disclosure: I work for GitHub and I am the main engineer involved with the secret scanning partner program.

@@ -542,7 +542,7 @@ protected function getPackageProviders($app)
return [SanctumServiceProvider::class];
}

public function invalidTokenDataProvider(): array
public static function invalidTokenDataProvider(): array
Copy link
Contributor Author

@marzvrover marzvrover Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a phpunit warning, but I'm happy to separate it from the PR if it is too off topic.

@taylorotwell taylorotwell merged commit 7bd4e5a into laravel:3.x Aug 15, 2023
6 checks passed
@taylorotwell
Copy link
Member

Thanks 👍

@@ -45,9 +45,16 @@ public function tokenCan(string $ability)
*/
public function createToken(string $name, array $abilities = ['*'], DateTimeInterface $expiresAt = null)
{
$plainTextToken = sprintf(
'%s%s%s',
config('sanctum.token_prefix', ''),
Copy link

@mkarnicki mkarnicki Sep 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will never be empty (unless the env var is defined with empty value), because the vendor's sanctum.php defines a laravel_sanctum_ default value, so the second argument will never be used.

If I understand correctly, this has been included in a minor version release 3.2.6. We use Laravel Sanctum version 3.2. We accidentally realized the tokens now have a "laravel_sanctum_" prefix unless the env var is redefined with empty content, which is unexpected. So we had to define an empty SANCTUM_TOKEN_PREFIX env var to avoid the prefix being prepended.

How is this not a breaking change?

Thanks for any feedback you may have.

Copy link

@mx-jhinz mx-jhinz Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I strongly agree - this is a Breaking-Change!
In addition, our applications validate against the specific structure of the PAT and now all PAT have become longer and incompatible.
This change has to be reverted.

Copy link

@stefro stefro Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, a simple solution could be to make the default '' in this config value:

'token_prefix' => env('SANCTUM_TOKEN_PREFIX', 'laravel_sanctum_'),

Right now we're exposing laravel_sanctum_ in our API keys. Some of us don't want to expose our stack details, it should be optional IMO.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've released a hotfix for this. Please let us know if that works (v3.3.0)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @driesvints!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I'm afk, but based on the change in the code the unexpected change in behavior should be gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants