diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj
index d8ebb1d23836f17fff6c8b34768c3b90a4be264c..45ed8e4ec11393e3c7bf40cd4232cf352e0bc7c4 100644
--- a/src/metabase/models/interface.clj
+++ b/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]
diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj
index fd80041abc20e55e337e9604edb7993e25d9db8b..8c6029af06b7f52b934ed169da7a23cf64b58b2f 100644
--- a/src/metabase/models/setting.clj
+++ b/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.
diff --git a/src/metabase/util.clj b/src/metabase/util.clj
index 9d39e7ad08da90faf39d42a830250f236a66f4fd..dca42ebe128c2bcb31233c135a71ea401c10647b 100644
--- a/src/metabase/util.clj
+++ b/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))))
diff --git a/src/metabase/util/encryption.clj b/src/metabase/util/encryption.clj
index fd920c98e479c8e84c9a472acf04d4d3cc8d0d9b..bcd2624ad6dfd2930e7ca4d41573945867ca68a9 100644
--- a/src/metabase/util/encryption.clj
+++ b/src/metabase/util/encryption.clj
@@ -1,5 +1,6 @@
(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)))
diff --git a/test/metabase/models/setting_test.clj b/test/metabase/models/setting_test.clj
index cf617ab39ec60da3fc1759968595fdd9613a7108..b4a76c5eb2b5a7e05be85d31a77c8153cb8ef4de 100644
--- a/test/metabase/models/setting_test.clj
+++ b/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)))
diff --git a/test/metabase/util/encryption_test.clj b/test/metabase/util/encryption_test.clj
index a858a6b1243f5060b8291ecc9ac9000e685d2155..026a6388ff4f6d8397b7652ea098ab706eddb507 100644
--- a/test/metabase/util/encryption_test.clj
+++ b/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")))))