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

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.

This makes the merge lower risk, because we have some time to revert if needed. Once the change is confirmed to be working, we will add a second commit to drop the `key` column.

app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6

@@ -28,12 +28,11 @@ export default Ember.Controller.extend({
     },
 
     save() {
-      this.model
+      this.model.save().catch(popupAjaxError);
-        .save()
+    },
-        .then(() => {
+
-          this.transitionToRoute("adminApiKeys.show", this.model.id);
+    continue() {
-        })
+      this.transitionToRoute("adminApiKeys.show", this.model.id);
-        .catch(popupAjaxError);
     }
   }
 });

app/assets/javascripts/admin/models/api-key.js.es6

@@ -3,6 +3,7 @@ import AdminUser from "admin/models/admin-user";
 import RestModel from "discourse/models/rest";
 import { ajax } from "discourse/lib/ajax";
 import { computed } from "@ember/object";
+import { fmt } from "discourse/lib/computed";
 
 const ApiKey = RestModel.extend({
   user: computed("_user", {
@@ -19,17 +20,14 @@ const ApiKey = RestModel.extend({
     }
   }),
 
-  @discourseComputed("key")
-  shortKey(key) {
-    return `${key.substring(0, 4)}...`;
-  },
-
   @discourseComputed("description")
   shortDescription(description) {
     if (!description || description.length < 40) return description;
     return `${description.substring(0, 40)}...`;
   },
 
+  truncatedKey: fmt("truncated_key", "%@..."),
+
   revoke() {
     return ajax(`${this.basePath}/revoke`, {
       type: "POST"

app/assets/javascripts/admin/templates/api-keys-index.hbs

@@ -19,7 +19,7 @@
       <tr class={{if k.revoked_at "revoked"}}>
         <td class="key">
           {{#if k.revoked_at}}{{d-icon 'times-circle'}}{{/if}}
-          {{k.shortKey}}
+          {{k.truncatedKey}}
         </td>
         <td class="key-description">
           {{k.shortDescription}}

app/assets/javascripts/admin/templates/api-keys-new.hbs

@@ -4,24 +4,35 @@
 {{/link-to}}
 
 <div class="api-key">
-  {{#admin-form-row label="admin.api.description"}}
+  {{#if model.id}}
-    {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}}
+    {{#admin-form-row label="admin.api.key"}}
-  {{/admin-form-row}}
+      <div>{{model.key}}</div>
+    {{/admin-form-row}}
+    {{#admin-form-row}}
+      {{i18n "admin.api.not_shown_again"}}
+    {{/admin-form-row}}
+    {{#admin-form-row}}
+      {{d-button icon="angle-right" label="admin.api.continue" action=(action "continue") class="btn-primary"}}
+    {{/admin-form-row}}
+  {{else}}
+    {{#admin-form-row label="admin.api.description"}}
+      {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}}
+    {{/admin-form-row}}
 
-  {{#admin-form-row label="admin.api.user_mode"}}
+    {{#admin-form-row label="admin.api.user_mode"}}
-    {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}}
+      {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}}
-  {{/admin-form-row}}
+    {{/admin-form-row}}
 
-  {{#if showUserSelector}}
+    {{#if showUserSelector}}
-    {{#admin-form-row label="admin.api.user"}}
+      {{#admin-form-row label="admin.api.user"}}
-      {{user-selector single="true"
+        {{user-selector single="true"
-        usernames=model.username
+          usernames=model.username
-        placeholderKey="admin.api.user_placeholder"
+          placeholderKey="admin.api.user_placeholder"
-        }}  
+          }}  
+      {{/admin-form-row}}
+    {{/if}}
+    {{#admin-form-row}}
+      {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary"}}
     {{/admin-form-row}}
   {{/if}}
-
-  {{#admin-form-row}}
-    {{d-button icon="check" label="admin.api.save" action=(action "save") class="btn-primary" disabled=saveDisabled}}
-  {{/admin-form-row}}
 </div>

app/assets/javascripts/admin/templates/api-keys-show.hbs

@@ -6,7 +6,7 @@
 <div class="api-key">
   {{#admin-form-row label="admin.api.key"}}
     {{#if model.revoked_at}}{{d-icon 'times-circle'}}{{/if}}
-    <div>{{model.key}}</div>
+    {{model.truncatedKey}}
   {{/admin-form-row}}
 
   {{#admin-form-row label="admin.api.description"}}

app/models/api_key.rb

@@ -1,22 +1,36 @@
 # frozen_string_literal: true
 
 class ApiKey < ActiveRecord::Base
+  class KeyAccessError < StandardError; end
+
   belongs_to :user
   belongs_to :created_by, class_name: 'User'
 
   scope :active, -> { where("revoked_at IS NULL") }
   scope :revoked, -> { where("revoked_at IS NOT NULL") }
 
-  validates_presence_of :key
+  scope :with_key, ->(key) {
+                     hashed = self.hash_key(key)
+                     where(key_hash: hashed)
+                   }
 
   after_initialize :generate_key
 
   def generate_key
-    self.key ||= SecureRandom.hex(32)
+    if !self.key_hash
+      @key ||= SecureRandom.hex(32) # Not saved to DB
+      self.truncated_key = key[0..3]
+      self.key_hash = ApiKey.hash_key(key)
+    end
   end
 
-  def truncated_key
+  def key
-    self.key[0..3]
+    raise KeyAccessError.new "API key is only accessible immediately after creation" unless key_available?
+    @key
+  end
+
+  def key_available?
+    @key.present?
   end
 
   def self.last_used_epoch
@@ -42,6 +56,10 @@ class ApiKey < ActiveRecord::Base
       end
     end
   end
+
+  def self.hash_key(key)
+    Digest::SHA256.hexdigest key
+  end
 end
 
 # == Schema Information
@@ -49,7 +67,6 @@ end
 # Table name: api_keys
 #
 #  id            :integer          not null, primary key
-#  key           :string(64)       not null
 #  user_id       :integer
 #  created_by_id :integer
 #  created_at    :datetime         not null
@@ -59,9 +76,11 @@ end
 #  last_used_at  :datetime
 #  revoked_at    :datetime
 #  description   :text
+#  key_hash      :string           not null
+#  truncated_key :string           not null
 #
 # Indexes
 #
-#  index_api_keys_on_key      (key)
+#  index_api_keys_on_key_hash  (key_hash)
-#  index_api_keys_on_user_id  (user_id)
+#  index_api_keys_on_user_id   (user_id)
 #

app/serializers/api_key_serializer.rb

@@ -4,6 +4,7 @@ class ApiKeySerializer < ApplicationSerializer
 
   attributes :id,
              :key,
+             :truncated_key,
              :description,
              :last_used_at,
              :created_at,
@@ -16,4 +17,8 @@ class ApiKeySerializer < ApplicationSerializer
     !object.user_id.nil?
   end
 
+  def include_key?
+    # Only available when first created. Not stored in db
+    object.key_available?
+  end
 end

config/locales/client.en.yml

@@ -3394,6 +3394,8 @@ en:
         new_key: New API Key
         revoked: Revoked
         delete: Permenantly Delete
+        not_shown_again: This key will not be displayed again. Make sure you take a copy before continuing.
+        continue: Continue
 
       web_hooks:
         title: "Webhooks"

db/migrate/20191211170000_add_hashed_api_key.rb

@@ -0,0 +1,58 @@
+# frozen_string_literal: true
+class AddHashedApiKey < ActiveRecord::Migration[6.0]
+  def up
+    add_column(:api_keys, :key_hash, :string)
+    add_column(:api_keys, :truncated_key, :string)
+
+    execute(
+      <<~SQL
+        UPDATE api_keys
+        SET truncated_key = LEFT(key, 4)
+      SQL
+    )
+
+    batch_size = 500
+    begin
+      batch = DB.query <<-SQL
+        SELECT id, key
+        FROM api_keys
+        WHERE key_hash IS NULL
+        LIMIT #{batch_size}
+      SQL
+
+      to_update = []
+      for row in batch
+        hashed = Digest::SHA256.hexdigest row.key
+        to_update << { id: row.id, key_hash: hashed }
+      end
+
+      if to_update.size > 0
+        data_string = to_update.map { |r| "(#{r[:id]}, '#{r[:key_hash]}')" }.join(",")
+
+        DB.exec <<~SQL
+          UPDATE api_keys
+          SET key_hash = data.key_hash
+          FROM (values
+            #{data_string}
+          ) as data(id, key_hash)
+          WHERE api_keys.id = data.id
+        SQL
+      end
+    end until batch.length < batch_size
+
+    change_column_null :api_keys, :key_hash, false
+    change_column_null :api_keys, :truncated_key, false
+
+    add_index :api_keys, :key_hash
+
+    # The key column will be dropped in a post_deploy migration
+    # But allow it to be null in the meantime
+    Migration::SafeMigrate.disable!
+    change_column_null :api_keys, :key, true
+    Migration::SafeMigrate.enable!
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/auth/default_current_user_provider.rb

@@ -284,7 +284,7 @@ class Auth::DefaultCurrentUserProvider
   end
 
   def lookup_api_user(api_key_value, request)
-    if api_key = ApiKey.active.where(key: api_key_value).includes(:user).first
+    if api_key = ApiKey.active.with_key(api_key_value).includes(:user).first
       api_username = header_api_key? ? @env[HEADER_API_USERNAME] : request[API_USERNAME]
 
       # Check for deprecated api auth
@@ -333,7 +333,7 @@ class Auth::DefaultCurrentUserProvider
 
     RateLimiter.new(
       nil,
-      "admin_api_min_#{api_key}",
+      "admin_api_min_#{ApiKey.hash_key(api_key)}",
       GlobalSetting.max_admin_api_reqs_per_key_per_minute,
       60
     ).performed!

lib/tasks/api.rake

@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
-desc "find or generate a master api key with given description"
+desc "generate a master api key with given description"
-task "api_key:get_or_create_master", [:description] => :environment do |task, args|
+task "api_key:create_master", [:description] => :environment do |task, args|
   raise "Supply a description for the key" if !args[:description]
-  api_key = ApiKey.find_or_create_by!(description: args[:description], revoked_at: nil, user_id: nil)
+  api_key = ApiKey.create!(description: args[:description])
 
   puts api_key.key
 end

spec/fabricators/api_key_fabricator.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 Fabricator(:api_key) do
-  key '1dfb7d427400cb8ef18052fd412781af134cceca5725dd74f34bbc6b9e35ddc9'
+
 end

spec/models/api_key_spec.rb

@@ -8,18 +8,32 @@ describe ApiKey do
 
   it { is_expected.to belong_to :user }
   it { is_expected.to belong_to :created_by }
-  it { is_expected.to validate_presence_of :key }
 
   it 'generates a key when saving' do
-    key = ApiKey.new
+    api_key = ApiKey.new
-    key.save!
+    api_key.save!
-    initial_key = key.key
+    initial_key = api_key.key
     expect(initial_key.length).to eq(64)
 
     # Does not overwrite key when saving again
-    key.description = "My description here"
+    api_key.description = "My description here"
-    key.save!
+    api_key.save!
-    expect(key.reload.key).to eq(initial_key)
+    expect(api_key.reload.key).to eq(initial_key)
+  end
+
+  it 'does not have the key when loading later from the database' do
+    api_key = ApiKey.create!
+    expect(api_key.key_available?).to eq(true)
+    expect(api_key.key.length).to eq(64)
+
+    api_key = ApiKey.find(api_key.id)
+    expect(api_key.key_available?).to eq(false)
+    expect { api_key.key }.to raise_error(ApiKey::KeyAccessError)
+  end
+
+  it "can lookup keys based on their hash" do
+    key = ApiKey.create!.key
+    expect(ApiKey.with_key(key).length).to eq(1)
   end
 
   it "can calculate the epoch correctly" do

spec/requests/admin/api_controller_spec.rb

@@ -10,8 +10,8 @@ describe Admin::ApiController do
 
   fab!(:admin) { Fabricate(:admin) }
 
-  fab!(:key1) { Fabricate(:api_key, description: "my key") }
+  fab!(:key1, refind: false) { Fabricate(:api_key, description: "my key") }
-  fab!(:key2) { Fabricate(:api_key, user: admin) }
+  fab!(:key2, refind: false) { Fabricate(:api_key, user: admin) }
 
   context "as an admin" do
     before do
@@ -32,7 +32,8 @@ describe Admin::ApiController do
         expect(response.status).to eq(200)
         data = JSON.parse(response.body)["key"]
         expect(data["id"]).to eq(key1.id)
-        expect(data["key"]).to eq(key1.key)
+        expect(data["key"]).to eq(nil)
+        expect(data["truncated_key"]).to eq(key1.key[0..3])
         expect(data["description"]).to eq("my key")
       end
     end

spec/requests/users_controller_spec.rb

@@ -815,7 +815,7 @@ describe UsersController do
 
       context "with a regular api key" do
         fab!(:user) { Fabricate(:user) }
-        fab!(:api_key) { Fabricate(:api_key, user: user) }
+        fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) }
 
         it "won't create the user as active with a regular key" do
           post "/u.json",
@@ -828,7 +828,7 @@ describe UsersController do
 
       context "with an admin api key" do
         fab!(:admin) { Fabricate(:admin) }
-        fab!(:api_key) { Fabricate(:api_key, user: admin) }
+        fab!(:api_key, refind: false) { Fabricate(:api_key, user: admin) }
 
         it "creates the user as active with a an admin key" do
           SiteSetting.send_welcome_message = true
@@ -915,7 +915,7 @@ describe UsersController do
 
       context "with a regular api key" do
         fab!(:user) { Fabricate(:user) }
-        fab!(:api_key) { Fabricate(:api_key, user: user) }
+        fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) }
 
         it "won't create the user as staged with a regular key" do
           post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)
@@ -928,7 +928,7 @@ describe UsersController do
 
       context "with an admin api key" do
         fab!(:user) { Fabricate(:admin) }
-        fab!(:api_key) { Fabricate(:api_key, user: user) }
+        fab!(:api_key, refind: false) { Fabricate(:api_key, user: user) }
 
         it "creates the user as staged with a regular key" do
           post "/u.json", params: post_user_params.merge(staged: true, api_key: api_key.key)