FEATURE: Hash API keys in the database (PR #8438)

:warning:This change is completely irreversible. Once the migrations are run, the plain text keys will be deleted from the database. Marking as a draft PR for now to avoid accidental merging, but this is ready for review.

API keys are now only visible when first created. After that, only the first four characters are stored in the database for identification, along with an sha256 hash of the full key. This makes key usage easier to audit, and ensures attackers would not have access to the live site in the event of a database leak.

Many of the changes in this diff are to implement the new post-create UI, which looks like:

Screenshot 2019-11-29 at 15 20 09

From a security perspective, the key files to review are:

  • app/models/api_key.rb

  • lib/auth/default_current_user_provider.rb

  • db/migrate/20191128113434_add_hashed_api_key.rb

  • db/post_migrate/20191128113435_remove_key_from_api_keys.rb

@danielwaterworth I had some issues with fab! in the tests, because it ‘refinds’ the record from the database after the initial save. The keys are kept temporarily as instance variables, so this refind was causing the key to be lost. I set refind:false in these places, but would be interested if you know of a cleaner solution.


You’ve signed the CLA, davidtaylorhq. Thank you! This pull request is ready for review.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:


Guess it could be nice to have this in api-key model:

import { fmt } from "discourse/lib/computed";

truncatedKey: fmt("truncated_key", "%@...")

It’s minor but, it avoid repeating the ... each time we use it, and I like the idea of limit as much as possible snake_case in hbs files, obviously we will always have some…

sorry missed it was a draft…

Using a div here fixes the “double click & copy includes whitespace in firefox” issue. Unfortunately a span does not seem to help :cry:

1 Like

Oh ok, didn’t know this, totally fine :+1:

This looks great! :+1:

With mini sql you get row.key, can sha256 be done direct in pg?

Minor, key.key is odd maybe api_key.key

can sha256 be done direct in pg

Not without loading the pgcrypto module, which we don’t currently use

K so we can do a larger batches and we can batch update table, just to minimise db trips during migration, cause this can lock stuff up

I wonder if to decrease risk we phase the change

This week we make it so all new keys use new system, in a couple of weeks we remove old system, can even keep all the new ui, just need to keep the old key column around?

This week we make it so all new keys use new system, in a couple of weeks we remove old system, can even keep all the new ui, just need to keep the old key column around?

Yes, that is the plan. Right now the column will be kept. The only risk is that newly created keys will be lost if we have to roll back. I think that’s probably ok.

My only concern is that we temporarily lock up user api key table during the migration on rogue sites, not sure if it would impact anything but api traffic though, so it is not too high

I am good to have this merged once the migration comments are addressed.

I love this change, it makes our system safer!

Thanks for the review @SamSaffron - I updated the migration to batch the changes in groups of 500, and use a single statement for the update. If that looks good to you, I’ll merge this first thing tomorrow and deploy it to a few place.

1 Like