From 4c9ca24ccfc7485c86ac2afe58f5a883d27af446 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 12 Dec 2019 11:45:00 +0000
Subject: [PATCH] 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.
---
 .../controllers/admin-api-keys-new.js.es6     | 11 ++--
 .../javascripts/admin/models/api-key.js.es6   |  8 +--
 .../admin/templates/api-keys-index.hbs        |  2 +-
 .../admin/templates/api-keys-new.hbs          | 43 +++++++++-----
 .../admin/templates/api-keys-show.hbs         |  2 +-
 app/models/api_key.rb                         | 33 ++++++++---
 app/serializers/api_key_serializer.rb         |  5 ++
 config/locales/client.en.yml                  |  2 +
 .../20191211170000_add_hashed_api_key.rb      | 58 +++++++++++++++++++
 lib/auth/default_current_user_provider.rb     |  4 +-
 lib/tasks/api.rake                            |  6 +-
 spec/fabricators/api_key_fabricator.rb        |  2 +-
 spec/models/api_key_spec.rb                   | 28 ++++++---
 spec/requests/admin/api_controller_spec.rb    |  7 ++-
 spec/requests/users_controller_spec.rb        |  8 +--
 15 files changed, 163 insertions(+), 56 deletions(-)
 create mode 100644 db/migrate/20191211170000_add_hashed_api_key.rb

diff --git a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6 b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6
index c04e6abec9b..e4f2869b21b 100644
--- a/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6
+++ b/app/assets/javascripts/admin/controllers/admin-api-keys-new.js.es6
@@ -28,12 +28,11 @@ export default Ember.Controller.extend({
     },
 
     save() {
-      this.model
-        .save()
-        .then(() => {
-          this.transitionToRoute("adminApiKeys.show", this.model.id);
-        })
-        .catch(popupAjaxError);
+      this.model.save().catch(popupAjaxError);
+    },
+
+    continue() {
+      this.transitionToRoute("adminApiKeys.show", this.model.id);
     }
   }
 });
diff --git a/app/assets/javascripts/admin/models/api-key.js.es6 b/app/assets/javascripts/admin/models/api-key.js.es6
index 7a20c288aea..06d861fa4a3 100644
--- a/app/assets/javascripts/admin/models/api-key.js.es6
+++ b/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"
diff --git a/app/assets/javascripts/admin/templates/api-keys-index.hbs b/app/assets/javascripts/admin/templates/api-keys-index.hbs
index a0d5bf9f77e..7943a5b9e2a 100644
--- a/app/assets/javascripts/admin/templates/api-keys-index.hbs
+++ b/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}}
diff --git a/app/assets/javascripts/admin/templates/api-keys-new.hbs b/app/assets/javascripts/admin/templates/api-keys-new.hbs
index 15f25b1ce17..4ddc37befd4 100644
--- a/app/assets/javascripts/admin/templates/api-keys-new.hbs
+++ b/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"}}
-    {{input value=model.description maxlength="255" placeholder=(i18n "admin.api.description_placeholder")}}
-  {{/admin-form-row}}
+  {{#if model.id}}
+    {{#admin-form-row label="admin.api.key"}}
+      <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"}}
-    {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}}
-  {{/admin-form-row}}
+    {{#admin-form-row label="admin.api.user_mode"}}
+      {{combo-box content=userModes value=userMode onSelect=(action "changeUserMode")}}
+    {{/admin-form-row}}
 
-  {{#if showUserSelector}}
-    {{#admin-form-row label="admin.api.user"}}
-      {{user-selector single="true"
-        usernames=model.username
-        placeholderKey="admin.api.user_placeholder"
-        }}  
+    {{#if showUserSelector}}
+      {{#admin-form-row label="admin.api.user"}}
+        {{user-selector single="true"
+          usernames=model.username
+          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>
\ No newline at end of file
diff --git a/app/assets/javascripts/admin/templates/api-keys-show.hbs b/app/assets/javascripts/admin/templates/api-keys-show.hbs
index 725c987a22c..7639a438f9d 100644
--- a/app/assets/javascripts/admin/templates/api-keys-show.hbs
+++ b/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"}}
diff --git a/app/models/api_key.rb b/app/models/api_key.rb
index 91722b36030..6abc38b548f 100644
--- a/app/models/api_key.rb
+++ b/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
-    self.key[0..3]
+  def key
+    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_user_id  (user_id)
+#  index_api_keys_on_key_hash  (key_hash)
+#  index_api_keys_on_user_id   (user_id)
 #
diff --git a/app/serializers/api_key_serializer.rb b/app/serializers/api_key_serializer.rb
index c743b97d52e..310b210a01d 100644
--- a/app/serializers/api_key_serializer.rb
+++ b/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
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 5f9055905fa..dbbdc95de54 100644
--- a/config/locales/client.en.yml
+++ b/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"
diff --git a/db/migrate/20191211170000_add_hashed_api_key.rb b/db/migrate/20191211170000_add_hashed_api_key.rb
new file mode 100644
index 00000000000..2811239c5dc
--- /dev/null
+++ b/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
diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb
index 3bc8957ea33..bdd304ef1b1 100644
--- a/lib/auth/default_current_user_provider.rb
+++ b/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!
diff --git a/lib/tasks/api.rake b/lib/tasks/api.rake
index bf8c37ecd31..f6f838d83dc 100644
--- a/lib/tasks/api.rake
+++ b/lib/tasks/api.rake
@@ -1,9 +1,9 @@
 # frozen_string_literal: true
 
-desc "find or generate a master api key with given description"
-task "api_key:get_or_create_master", [:description] => :environment do |task, args|
+desc "generate a master api key with given description"
+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
diff --git a/spec/fabricators/api_key_fabricator.rb b/spec/fabricators/api_key_fabricator.rb
index f2f766baee4..2f75d78c0bb 100644
--- a/spec/fabricators/api_key_fabricator.rb
+++ b/spec/fabricators/api_key_fabricator.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 Fabricator(:api_key) do
-  key '1dfb7d427400cb8ef18052fd412781af134cceca5725dd74f34bbc6b9e35ddc9'
+
 end
diff --git a/spec/models/api_key_spec.rb b/spec/models/api_key_spec.rb
index aaaf7196041..c842f5421cc 100644
--- a/spec/models/api_key_spec.rb
+++ b/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
-    key.save!
-    initial_key = key.key
+    api_key = ApiKey.new
+    api_key.save!
+    initial_key = api_key.key
     expect(initial_key.length).to eq(64)
 
     # Does not overwrite key when saving again
-    key.description = "My description here"
-    key.save!
-    expect(key.reload.key).to eq(initial_key)
+    api_key.description = "My description here"
+    api_key.save!
+    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
diff --git a/spec/requests/admin/api_controller_spec.rb b/spec/requests/admin/api_controller_spec.rb
index 217ef19dc55..9c48d74732e 100644
--- a/spec/requests/admin/api_controller_spec.rb
+++ b/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!(:key2) { Fabricate(:api_key, user: admin) }
+  fab!(:key1, refind: false) { Fabricate(:api_key, description: "my key") }
+  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
diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb
index 179bedc690f..7c7b370b085 100644
--- a/spec/requests/users_controller_spec.rb
+++ b/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)