Encrypt Settings

src/metabase/models/interface.clj

@@ -68,6 +68,10 @@
   :in  encrypted-json-in
   :out (comp cached-encrypted-json-out u/jdbc-clob->str))
 
+(models/add-type! :encrypted-text
+  :in  encryption/maybe-encrypt
+  :out (comp encryption/maybe-decrypt u/jdbc-clob->str))
+
 (defn compress
   "Compress OBJ, returning a byte array."
   [obj]

src/metabase/models/setting.clj

@@ -49,7 +49,7 @@
 (u/strict-extend (class Setting)
   models/IModel
   (merge models/IModelDefaults
-         {:types (constantly {:value :clob})}))
+         {:types (constantly {:value :encrypted-text})}))
 
 
 (def ^:private Type
@@ -192,8 +192,13 @@
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
 (defn- update-setting! [setting-name new-value]
-  (db/update-where! Setting {:key setting-name}
-    :value new-value))
+  ;; This is indeed a very annoying way of having to do things, but `update-where!` doesn't call `pre-update` (in case
+  ;; it updates thousands of objects). So we need to manually trigger `pre-update` behavior by calling `do-pre-update`
+  ;; so that `value` can get encrypted if `MB_ENCRYPTION_SECRET_KEY` is in use. Then take that possibly-encrypted
+  ;; value and pass that into `update-where!`.
+  (let [{maybe-encrypted-new-value :value} (models/do-pre-update Setting {:value new-value})]
+    (db/update-where! Setting {:key setting-name}
+      :value maybe-encrypted-new-value)))
 
 (defn- set-new-setting!
   "Insert a new row for a Setting with SETTING-NAME and SETTING-VALUE.

src/metabase/util.clj

@@ -515,7 +515,7 @@
                   (select-nested-keys v nested-keys))})))
 
 (defn base64-string?
-  "Is S a Base-64 encoded string?"
+  "Is `s` a Base-64 encoded string?"
   ^Boolean [s]
   (boolean (when (string? s)
              (re-find #"^[0-9A-Za-z/+]+=*$" s))))

src/metabase/util/encryption.clj

 (ns metabase.util.encryption
-  "Utility functions for encrypting and decrypting strings using AES256 CBC + HMAC SHA512 and the `MB_ENCRYPTION_SECRET_KEY` env var."
+  "Utility functions for encrypting and decrypting strings using AES256 CBC + HMAC SHA512 and the
+  `MB_ENCRYPTION_SECRET_KEY` env var."
   (:require [buddy.core
              [codecs :as codecs]
              [crypto :as crypto]
@@ -8,54 +9,62 @@
             [clojure.tools.logging :as log]
             [environ.core :as env]
             [metabase.util :as u]
+            [puppetlabs.i18n.core :refer [trs]]
             [ring.util.codec :as codec]))
 
 (defn secret-key->hash
-  "Generate a 64-byte byte array hash of SECRET-KEY using 100,000 iterations of PBKDF2+SHA512."
+  "Generate a 64-byte byte array hash of `secret-key` using 100,000 iterations of PBKDF2+SHA512."
   ^bytes [^String secret-key]
   (kdf/get-bytes (kdf/engine {:alg        :pbkdf2+sha512
                               :key        secret-key
                               :iterations 100000}) ; 100,000 iterations takes about ~160ms on my laptop
                  64))
 
-;; apperently if you're not tagging in an arglist, `^bytes` will set the `:tag` metadata to `clojure.core/bytes` (ick) so you have to do `^{:tag 'bytes}` instead
+;; apperently if you're not tagging in an arglist, `^bytes` will set the `:tag` metadata to `clojure.core/bytes` (ick)
+;; so you have to do `^{:tag 'bytes}` instead
 (defonce ^:private ^{:tag 'bytes} default-secret-key
   (when-let [secret-key (env/env :mb-encryption-secret-key)]
     (when (seq secret-key)
       (assert (>= (count secret-key) 16)
-        "MB_ENCRYPTION_SECRET_KEY must be at least 16 characters.")
+        (trs "MB_ENCRYPTION_SECRET_KEY must be at least 16 characters."))
       (secret-key->hash secret-key))))
 
 ;; log a nice message letting people know whether DB details encryption is enabled
 (log/info
- (format "DB details encryption is %s for this Metabase instance. %s"
-         (if default-secret-key "ENABLED" "DISABLED")
-         (u/emoji (if default-secret-key "🔐" "🔓")))
- "\nSee"
- "http://www.metabase.com/docs/latest/operations-guide/start.html#encrypting-your-database-connection-details-at-rest"
- "for more information.")
+ (if default-secret-key
+   (trs "Saved credentials encryption is ENABLED for this Metabase instance.")
+   (trs "Saved credentials encryption is DISABLED for this Metabase instance."))
+ (u/emoji (if default-secret-key "🔐" "🔓"))
+ (trs "\nFor more information, see")
+ "https://www.metabase.com/docs/latest/operations-guide/start.html#encrypting-your-database-connection-details-at-rest")
 
 (defn encrypt
-  "Encrypt string S as hex bytes using a SECRET-KEY (a 64-byte byte array), by default the hashed value of `MB_ENCRYPTION_SECRET_KEY`."
+  "Encrypt string `s` as hex bytes using a `secret-key` (a 64-byte byte array), by default the hashed value of
+  `MB_ENCRYPTION_SECRET_KEY`."
   (^String [^String s]
    (encrypt default-secret-key s))
   (^String [^String secret-key, ^String s]
    (let [initialization-vector (nonce/random-bytes 16)]
-     (codec/base64-encode (byte-array (concat initialization-vector
-                                              (crypto/encrypt (codecs/to-bytes s) secret-key initialization-vector {:algorithm :aes256-cbc-hmac-sha512})))))))
+     (codec/base64-encode
+      (byte-array
+       (concat initialization-vector
+               (crypto/encrypt (codecs/to-bytes s) secret-key initialization-vector
+                               {:algorithm :aes256-cbc-hmac-sha512})))))))
 
 (defn decrypt
-  "Decrypt string S  using a SECRET-KEY (a 64-byte byte array), by default the hashed value of `MB_ENCRYPTION_SECRET_KEY`."
+  "Decrypt string `s` using a `secret-key` (a 64-byte byte array), by default the hashed value of
+  `MB_ENCRYPTION_SECRET_KEY`."
   (^String [^String s]
    (decrypt default-secret-key s))
   (^String [secret-key, ^String s]
    (let [bytes                           (codec/base64-decode s)
          [initialization-vector message] (split-at 16 bytes)]
-     (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array initialization-vector) {:algorithm :aes256-cbc-hmac-sha512})))))
+     (codecs/bytes->str (crypto/decrypt (byte-array message) secret-key (byte-array initialization-vector)
+                                        {:algorithm :aes256-cbc-hmac-sha512})))))
 
 
 (defn maybe-encrypt
-  "If `MB_ENCRYPTION_SECRET_KEY` is set, return an encrypted version of S; otherwise return S as-is."
+  "If `MB_ENCRYPTION_SECRET_KEY` is set, return an encrypted version of `s`; otherwise return `s` as-is."
   (^String [^String s]
    (maybe-encrypt default-secret-key s))
   (^String [secret-key, ^String s]
@@ -65,7 +74,7 @@
      s)))
 
 (defn maybe-decrypt
-  "If `MB_ENCRYPTION_SECRET_KEY` is set and S is encrypted, decrypt S; otherwise return S as-is."
+  "If `MB_ENCRYPTION_SECRET_KEY` is set and `s` is encrypted, decrypt `s`; otherwise return `s` as-is."
   (^String [^String s]
    (maybe-decrypt default-secret-key s))
   (^String [secret-key, ^String s]
@@ -75,7 +84,10 @@
        (catch Throwable e
          (if (u/base64-string? s)
            ;; if we can't decrypt `s`, but it *is* encrypted, log an error message and return `nil`
-           (log/error "Cannot decrypt encrypted details. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?" (.getMessage e))
+           (log/error
+            (trs "Cannot decrypt encrypted credentials. Have you changed or forgot to set MB_ENCRYPTION_SECRET_KEY?")
+            (.getMessage e)
+            (u/filtered-stacktrace e))
            ;; otherwise return S without decrypting. It's probably not decrypted in the first place
            s)))
      s)))

test/metabase/models/setting_test.clj

@@ -2,6 +2,10 @@
   (:require [expectations :refer :all]
             [metabase.models.setting :as setting :refer [defsetting Setting]]
             [metabase.test.util :refer :all]
+            [metabase.util :as u]
+            [metabase.util
+             [encryption :as encryption]
+             [encryption-test :as encryption-test]]
             [puppetlabs.i18n.core :refer [tru]]
             [toucan.db :as db]))
 
@@ -167,7 +171,12 @@
 
 ;; all
 (expect
-  {:key :test-setting-2, :value "TOUCANS", :description "Test setting - this only shows up in dev (2)",  :is_env_setting false, :env_name "MB_TEST_SETTING_2", :default "[Default Value]"}
+  {:key            :test-setting-2
+   :value          "TOUCANS"
+   :description    "Test setting - this only shows up in dev (2)"
+   :is_env_setting false
+   :env_name       "MB_TEST_SETTING_2"
+   :default        "[Default Value]"}
   (do (set-settings! nil "TOUCANS")
       (some (fn [setting]
               (when (re-find #"^test-setting-2$" (name (:key setting)))
@@ -176,8 +185,18 @@
 
 ;; all
 (expect
-  [{:key :test-setting-1, :value nil, :is_env_setting true, :env_name "MB_TEST_SETTING_1", :description "Test setting - this only shows up in dev (1)", :default "Using $MB_TEST_SETTING_1"}
-   {:key :test-setting-2, :value "S2", :is_env_setting false, :env_name "MB_TEST_SETTING_2",  :description "Test setting - this only shows up in dev (2)", :default "[Default Value]"}]
+  [{:key            :test-setting-1
+    :value          nil
+    :is_env_setting true
+    :env_name       "MB_TEST_SETTING_1"
+    :description    "Test setting - this only shows up in dev (1)"
+    :default        "Using $MB_TEST_SETTING_1"}
+   {:key            :test-setting-2
+    :value          "S2"
+    :is_env_setting false,
+    :env_name       "MB_TEST_SETTING_2"
+    :description    "Test setting - this only shows up in dev (2)"
+    :default        "[Default Value]"}]
   (do (set-settings! nil "S2")
       (for [setting (setting/all)
             :when   (re-find #"^test-setting-\d$" (name (:key setting)))]
@@ -269,3 +288,32 @@
     (toucan-name "Banana Beak")
     ;; ok, make sure the setting was set
     (toucan-name)))
+
+
+;;; ----------------------------------------------- Encrypted Settings -----------------------------------------------
+
+(defn- actual-value-in-db [setting-key]
+  (-> (db/query {:select [:value]
+                 :from   [:setting]
+                 :where  [:= :key (name setting-key)]})
+      first :value u/jdbc-clob->str))
+
+;; If encryption is *enabled*, make sure Settings get saved as encrypted!
+(expect
+  (encryption-test/with-secret-key "ABCDEFGH12345678"
+    (toucan-name "Sad Can")
+    (u/base64-string? (actual-value-in-db :toucan-name))))
+
+;; make sure it can be decrypted as well...
+(expect
+  "Sad Can"
+  (encryption-test/with-secret-key "12345678ABCDEFGH"
+    (toucan-name "Sad Can")
+    (encryption/decrypt (actual-value-in-db :toucan-name))))
+
+;; But if encryption is not enabled, of course Settings shouldn't get saved as encrypted.
+(expect
+  "Sad Can"
+  (encryption-test/with-secret-key nil
+    (toucan-name "Sad Can")
+    (actual-value-in-db :toucan-name)))

test/metabase/util/encryption_test.clj

@@ -5,6 +5,18 @@
             [metabase.test.util :as tu]
             [metabase.util.encryption :as encryption]))
 
+(defn do-with-secret-key [^String secret-key, f]
+  (with-redefs [encryption/default-secret-key (when (seq secret-key)
+                                                (encryption/secret-key->hash secret-key))]
+    (f)))
+
+(defmacro with-secret-key
+  "Run `body` with the encryption secret key temporarily bound to `secret-key`. Useful for testing how functions behave
+  with and without encryption disabled."
+  {:style/indent 1}
+  [^String secret-key, & body]
+  `(do-with-secret-key ~secret-key (fn [] ~@body)))
+
 (def ^:private secret   (encryption/secret-key->hash "Orw0AAyzkO/kPTLJRxiyKoBHXa/d6ZcO+p+gpZO/wSQ="))
 (def ^:private secret-2 (encryption/secret-key->hash "0B9cD6++AME+A7/oR7Y2xvPRHX3cHA2z7w+LbObd/9Y="))
 
@@ -49,7 +61,7 @@
 
 (expect
   (some (fn [[_ _ message]]
-          (str/includes? message (str "Cannot decrypt encrypted details. Have you changed or forgot to set "
+          (str/includes? message (str "Cannot decrypt encrypted credentials. Have you changed or forgot to set "
                                       "MB_ENCRYPTION_SECRET_KEY? Message seems corrupt or manipulated.")))
         (tu/with-log-messages
           (encryption/maybe-decrypt secret-2 (encryption/encrypt secret "WOW")))))