Handle token-like values in encryption key rotation (#15796)

Co-authored-by: Raimon Grau <raimonster@gmail.com>

src/metabase/cmd.clj

@@ -57,7 +57,7 @@
       (println "Dump complete")
       (system-exit! 0))
     (catch Throwable e
-      (log/error e "MB_ENCRYPTION_SECRET_KEY does not correcty decrypt the existing data")
+      (log/error e "Failed to dump application database to H2 file")
       (system-exit! 1))))
 
 (defn ^:command profile

src/metabase/cmd/dump_to_h2.clj

@@ -37,7 +37,7 @@
      (when-not keep-existing?
        (copy.h2/delete-existing-h2-database-files! h2-filename))
      (copy/copy!  (mdb.conn/db-type) (mdb.conn/jdbc-spec) :h2 h2-jdbc-spec)
-     (if dump-plaintext?
+     (when dump-plaintext?
        (binding [mdb.conn/*db-type* :h2
                  mdb.conn/*jdbc-spec* h2-jdbc-spec
                  db/*db-connection* h2-jdbc-spec

src/metabase/cmd/rotate_encryption_key.clj

@@ -6,6 +6,7 @@
             [metabase.models :refer [Database Setting]]
             [metabase.models.setting.cache :as cache]
             [metabase.util.encryption :as encrypt]
+            [metabase.util.i18n :refer [trs]]
             [toucan.db :as db]))
 
 (defn rotate-encryption-key!
@@ -17,19 +18,17 @@
                               (encrypt/validate-and-hash-secret-key to-key))
                      identity)]
     (jdbc/with-db-transaction [t-conn (mdb.conn/jdbc-spec)]
+      (doseq [[id details] (db/select-id->field :details Database)]
+        (when (encrypt/possibly-encrypted-string? details)
+          (throw (ex-info (trs "Can't decrypt app db with MB_ENCRYPTION_SECRET_KEY") {:database-id id})))
+        (jdbc/update! t-conn
+                      :metabase_database
+                      {:details (encrypt-fn (json/encode details))}
+                      ["metabase_database.id = ?" id]))
       (doseq [[key value] (db/select-field->field :key :value Setting)]
-        (when (encrypt/possibly-encrypted-string? value)
-          (throw (Exception. "MB_ENCRYPTION_SECRET_KEY does not correctly decrypt files")))
         (if (= key "settings-last-updated")
           (cache/update-settings-last-updated!)
           (jdbc/update! t-conn
                         :setting
                         {:value (encrypt-fn value)}
-                        ["setting.key = ?" key])))
-      (doseq [[id details] (db/select-id->field :details Database)]
-        (jdbc/update! t-conn
-                      :metabase_database
-                      {:details (encrypt-fn (json/encode details))}
-                      ["metabase_database.id = ?" id])
-        (when (encrypt/possibly-encrypted-string? details)
-          (throw (Exception. "MB_ENCRYPTION_SECRET_KEY does not correctly decrypt files")))))))
+                        ["setting.key = ?" key]))))))

test/metabase/cmd/rotate_encryption_key_test.clj

 (ns metabase.cmd.rotate-encryption-key-test
-  (:require [clojure.java.io :as io]
-            [clojure.java.jdbc :as jdbc]
+  (:require [clojure.java.jdbc :as jdbc]
             [clojure.string :as str]
             [clojure.test :refer :all]
             [metabase.cmd :as cmd]
@@ -11,12 +10,27 @@
             [metabase.db.spec :as db.spec]
             [metabase.driver :as driver]
             [metabase.models :refer [Database Setting]]
-            [metabase.models.interface :as models]
+            [metabase.models.interface :as interface]
             [metabase.test :as mt]
             [metabase.test.data.interface :as tx]
             [metabase.util.encryption :as encrypt]
             [metabase.util.encryption-test :as eu]
-            [toucan.db :as db]))
+            [toucan.db :as db]
+            [toucan.models :as models]))
+
+(defn do-with-model-type
+  [mtype in-type-fns f]
+  (let [type-fns        (var-get #'models/type-fns)
+        before-type-fns @type-fns]
+    (swap! type-fns update mtype merge in-type-fns)
+    (try
+      (f)
+      (finally
+        (reset! type-fns before-type-fns)))))
+
+(defmacro with-model-type
+  [mtype type-fns & body]
+  `(do-with-model-type ~mtype ~type-fns (fn [] ~@body)))
 
 (defn- persistent-jdbcspec
   "Return a jdbc spec for the specified `db-type` on the db `db-name`. In case of H2, makes the connection persistent
@@ -29,13 +43,9 @@
     :postgres (db.spec/postgres (tx/dbdef->connection-details :postgres :db {:database-name db-name}))
     :mysql (db.spec/mysql (tx/dbdef->connection-details :mysql :db {:database-name db-name}))))
 
-(defn- abs-path
-  [path]
-  (.getAbsolutePath (io/file path)))
-
-(defn- raw-value [key]
+(defn- raw-value [keyy]
   (:value (first (jdbc/query mdb.connection/*jdbc-spec*
-                             ["select value from setting where setting.key=?;" key]))))
+                             ["select value from setting where setting.key=?;" keyy]))))
 
 (deftest cmd-rotate-encryption-key-errors-when-failed-test
   (with-redefs [rotate-encryption-key! #(throw "err")
@@ -53,7 +63,7 @@
                               "yHa/6VEQuIItMyd5CNcgV9nXvzZcX6bWmiY0oOh6pLU="
                               "BCQbKNVu6N8TQ2BwyTC0U0oCBqsvFVr2uhEM/tRgJUM="]]
       (mt/test-drivers #{:postgres :h2 :mysql}
-        (with-redefs [models/cached-encrypted-json-out #'models/encrypted-json-out]
+        (with-model-type :encrypted-json {:out #'interface/encrypted-json-out}
           (binding [mdb.connection/*db-type*   driver/*driver*
                     mdb.connection/*jdbc-spec* (persistent-jdbcspec driver/*driver* db-name)
                     db/*db-connection*         (persistent-jdbcspec driver/*driver* db-name)
@@ -61,59 +71,55 @@
             (when-not (= driver/*driver* :h2)
               (tx/create-db! driver/*driver* {:database-name db-name}))
             (load-from-h2/load-from-h2! h2-fixture-db-file)
-            (db/insert! Setting {:key "setting0", :value "val0"})
+            (db/insert! Setting {:key "nocrypt", :value "unencrypted value"})
             (db/insert! Setting {:key "settings-last-updated", :value original-timestamp})
             (eu/with-secret-key k1
-              (db/insert! Setting {:key "setting1", :value "val1"})
+              (db/insert! Setting {:key "k1crypted", :value "encrypted with k1"})
               (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"}))
 
             (testing "rotating with the same key is a noop"
               (eu/with-secret-key k1
                 (rotate-encryption-key! k1)
                 ;; plain->newkey
-                (is (not (= "val0" (raw-value "setting0"))))
-                (is (= "val0" (db/select-one-field :value Setting :key "setting0")))
-                ;; oldkey->newkey
-                (is (not (= "val1" (raw-value "setting1"))))
-                (is (= "val1" (db/select-one-field :value Setting :key "setting1")))))
+                (testing "for unencrypted values"
+                  (is (not= "unencrypted value" (raw-value "nocrypt")))
+                  (is (= "unencrypted value" (db/select-one-field :value Setting :key "nocrypt"))))
+                ;; samekey->samekey
+                (testing "for values encrypted with the same key"
+                  (is (not= "encrypted with k1" (raw-value "k1crypted")))
+                  (is (= "encrypted with k1" (db/select-one-field :value Setting :key "k1crypted"))))))
 
             (testing "settings-last-updated is updated AND plaintext"
-              (is (not (= original-timestamp (raw-value "settings-last-updated"))))
+              (is (not= original-timestamp (raw-value "settings-last-updated")))
               (is (not (encrypt/possibly-encrypted-string? (raw-value "settings-last-updated")))))
 
             (testing "rotating with a new key is recoverable"
               (eu/with-secret-key k1 (rotate-encryption-key! k2))
-              (eu/with-secret-key k2
-                (is (= "val0" (db/select-one-field :value Setting :key "setting0")))
-                (is (= {:db "/tmp/test.db"} (db/select-one-field :details Database :id 1))))
-              (eu/with-secret-key k1
-                (is (not (= "val0" (db/select-one-field :value Setting :key "setting0"))))
-                (is (not (= "{\"db\":\"/tmp/test.db\"}" (db/select-one-field :details Database :id 1))))))
-
-            (testing "full rollback when a setting looks encrypted with a different key than the current one"
-              (eu/with-secret-key k3
-                (db/insert! Setting {:key "setting3", :value "val3"}))
-              (eu/with-secret-key k2
-                (db/insert! Setting {:key "setting2", :value "val2"})
-                (is (thrown? Throwable (rotate-encryption-key! k3))))
-              (eu/with-secret-key k3
-                (is (not (= "val2" (:value (first (db/select Setting :key [:= "setting2"]))))))
-                (is (= "val3" (:value (first (db/select Setting :key [:= "setting3"])))))))
+              (testing "with new key"
+                (eu/with-secret-key k2
+                  (is (= "unencrypted value" (db/select-one-field :value Setting :key "nocrypt")))
+                  (is (= {:db "/tmp/test.db"} (db/select-one-field :details Database :id 1)))))
+              (testing "but not with old key"
+                (eu/with-secret-key k1
+                  (is (not= "unencrypted value" (db/select-one-field :value Setting :key "nocrypt")))
+                  (is (not= "{\"db\":\"/tmp/test.db\"}" (db/select-one-field :details Database :id 1))))))
 
-            (testing "full rollback when a database looks encrypted with a different key than the current one"
+            (testing "full rollback when a database details looks encrypted with a different key than the current one"
               (eu/with-secret-key k3
-                (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"}))
+                (db/insert! Database {:name "k3", :engine :mysql, :details "{\"db\":\"/tmp/k3.db\"}"}))
               (eu/with-secret-key k2
-                (is (thrown? Throwable (rotate-encryption-key! k3))))
+                (db/insert! Database {:name "k2", :engine :mysql, :details "{\"db\":\"/tmp/k2.db\"}"})
+                (is (thrown? clojure.lang.ExceptionInfo (rotate-encryption-key! k3))))
               (eu/with-secret-key k3
-                (is (= {:db "/tmp/test.db"} (db/select-one-field :details Database :id 1)))))
+                (is (not= {:db "/tmp/k2.db"} (db/select-one-field :details Database :name "k2")))
+                (is (= {:db "/tmp/k3.db"} (db/select-one-field :details Database :name "k3")))))
 
             (testing "rotate-encryption-key! to nil decrypts the encrypted keys"
-              (db/delete! Setting :key "setting3")
               (db/update! Database 1 {:details "{\"db\":\"/tmp/test.db\"}"})
+              (db/update-where! Database {:name "k3"} :details "{\"db\":\"/tmp/test.db\"}")
               (eu/with-secret-key k2
                 (rotate-encryption-key! nil))
-              (is (= "val0" (raw-value "setting0"))))
+              (is (= "unencrypted value" (raw-value "nocrypt"))))
 
             (testing "short keys fail to rotate"
               (is (thrown? Throwable (rotate-encryption-key! "short"))))))))))