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

Feature/hash api keys #2249

Merged
merged 13 commits into from
Nov 9, 2024
Merged

Conversation

acelaya
Copy link
Member

@acelaya acelaya commented Nov 5, 2024

Closes #2193

This PR improves how Shlink handles API keys, by hashing them before they are persisted, improving security.

The hashing algorithm used for keys is SHA256, for two reasons mainly:

  • It generates consistent hashes for the same value, allowing to continue checking valid API keys using the same process as now.
  • It adds very little overhead, which means we can hash provided keys in a request in order to validate them without any noticeable impact in the request time.

Since API keys are v4 UUIDs, they provide enough uniqueness and entropy, so there's no need for a more advanced hashing algorithm, with salts, iterations, etc. SHA256 should be fine for this use case.

Backwards compatibility

It s very important to not break any existing instances when people update to this version. Because of that, existing API keys will be hashed for everything to continue working, but since it's possible people was trusting Shlink to list their keys, all plain-text values have been set in the name field.

Once updated, Shlink now provides a new api-key:rename console command that they can use to set more meaningful names to their keys and securely save the plain-text values.

For newly created API keys via api-key:generate, the process has changed a bit. The name is now recommended to be provided, and Shlink will verify it is unique.

However, if a name is not provided, Shlink will generate one using a redacted version of the API key. For example, if the key is 8fff91e5-11ce-452a-8be9-8660ee3b966c, the name will be 8fff91e5-****-****-****-************.

The api-key:disable command has also been slightly changed. For backwards compatibility, it still allows to disable keys by the key value itself (api-key:disable 8fff91e5-11ce-452a-8be9-8660ee3b966c), but that is now considered deprecated.

Instead, users should provide the name and indicate it via --by-name: api-key:disable the-name --by-name.


TODO:

  • Set plain-text API key in the name filed
  • Expose API key name only, not key
  • Hash existing keys, and make all checks be done against the encrypted key
  • When new keys are created without name, set a redacted version of the key as name (142e0b2c-****-****-****-************)
  • Make list short URLs always display API key names, never keys
  • Make names for newly created keys unique, or warn/error-out if they are not
  • Make api-key:disable command expect the name, optionally listing all names if none is provided. Try to keep it backwards compatible
  • Add new api-key:rename command, to change the name of an API key

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

Attention: Patch coverage is 97.16312% with 4 lines in your changes missing coverage. Please review.

Project coverage is 93.61%. Comparing base (79c5418) to head (7e573bd).
Report is 15 commits behind head on develop.

Files with missing lines Patch % Lines
module/CLI/src/Command/Api/GenerateKeyCommand.php 77.77% 2 Missing ⚠️
module/Core/src/ShortUrl/Entity/ShortUrl.php 0.00% 2 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #2249      +/-   ##
=============================================
+ Coverage      93.53%   93.61%   +0.07%     
- Complexity      1611     1635      +24     
=============================================
  Files            270      271       +1     
  Lines           5631     5716      +85     
=============================================
+ Hits            5267     5351      +84     
- Misses           364      365       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@acelaya acelaya marked this pull request as ready for review November 8, 2024 08:36
@acelaya acelaya merged commit 92ad6d2 into shlinkio:develop Nov 9, 2024
31 checks passed
@acelaya acelaya deleted the feature/hash-api-keys branch November 9, 2024 08:14
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.

Store API key hashes, instead of plain-text values
1 participant