From b28675d6d885001c340871a0cdf45c85bac97651 Mon Sep 17 00:00:00 2001 From: John Swanson <john.swanson@metabase.com> Date: Thu, 15 Aug 2024 05:15:33 -0700 Subject: [PATCH] Allow disabling encryption for settings (#43085) (#46773) * Allow disabling encryption for settings For two settings, we want to disable encryption entirely. On the way out of the data source, we don't need to worry - the decryption is already backwards-compatible with non-encrypted data. On the way in, things are a bit less straightforward. The core of the change is replacing the `t2/deftransform` that *always* runs `encryption/maybe-encrypt` on the value with: - a define-after-select that always tries to decrypt the value (so we can handle existing encrypted values in the db) - a define-before-insert that encrypts the value if the `defsetting` told us to, and - a define-before-update that does the same thing. For now, set two settings to be never-encrypted: `analytics-uuid` and `anon-tracking-enabled`. --- src/metabase/analytics/snowplow.clj | 1 + src/metabase/core.clj | 1 + src/metabase/models/interface.clj | 5 --- src/metabase/models/setting.clj | 59 ++++++++++++++++++++++++++--- src/metabase/public_settings.clj | 1 + 5 files changed, 57 insertions(+), 10 deletions(-) diff --git a/src/metabase/analytics/snowplow.clj b/src/metabase/analytics/snowplow.clj index 8ce54d54452..5b07e784fd9 100644 --- a/src/metabase/analytics/snowplow.clj +++ b/src/metabase/analytics/snowplow.clj @@ -85,6 +85,7 @@ (deferred-tru (str "Unique identifier to be used in Snowplow analytics, to identify this instance of Metabase. " "This is a public setting since some analytics events are sent prior to initial setup.")) + :encryption :never :visibility :public :base setting/uuid-nonce-base :doc false) diff --git a/src/metabase/core.clj b/src/metabase/core.clj index e198e3252eb..7c05d914eeb 100644 --- a/src/metabase/core.clj +++ b/src/metabase/core.clj @@ -155,6 +155,7 @@ (ensure-audit-db-installed!) (init-status/set-progress! 0.95) + (settings/migrate-encrypted-settings!) ;; start scheduler at end of init! (task/start-scheduler!) (init-status/set-complete!) diff --git a/src/metabase/models/interface.clj b/src/metabase/models/interface.clj index a460f2e4cbd..e27e120c136 100644 --- a/src/metabase/models/interface.clj +++ b/src/metabase/models/interface.clj @@ -277,11 +277,6 @@ {:in encrypted-json-in :out cached-encrypted-json-out}) -(def transform-encrypted-text - "Transform for encrypted text." - {:in encryption/maybe-encrypt - :out encryption/maybe-decrypt}) - (defn normalize-visualization-settings "The frontend uses JSON-serialized versions of MBQL clauses as keys in `:column_settings`. This normalizes them to modern MBQL clauses so things work correctly." diff --git a/src/metabase/models/setting.clj b/src/metabase/models/setting.clj index 8c913bca7e7..398a074dd92 100644 --- a/src/metabase/models/setting.clj +++ b/src/metabase/models/setting.clj @@ -84,13 +84,13 @@ [metabase.api.common :as api] [metabase.config :as config] [metabase.events :as events] - [metabase.models.interface :as mi] [metabase.models.serialization :as serdes] [metabase.models.setting.cache :as setting.cache] [metabase.plugins.classloader :as classloader] [metabase.server.middleware.json] [metabase.util :as u] [metabase.util.date-2 :as u.date] + [metabase.util.encryption :as encryption] [metabase.util.i18n :refer [deferred-trs deferred-tru trs tru]] [metabase.util.log :as log] [metabase.util.malli :as mu] @@ -160,9 +160,6 @@ (methodical/defmethod t2/primary-keys :model/Setting [_model] [:key]) -(t2/deftransforms :model/Setting - {:value mi/transform-encrypted-text}) - (defmethod serdes/hash-fields :model/Setting [_setting] [:key]) @@ -274,6 +271,10 @@ ;; where this setting should be visible (default: :admin) [:visibility Visibility] + ;; should this setting be encrypted `:never` or `:maybe` (when `MB_ENCRYPTION_SECRET_KEY` is set). + ;; Defaults to `:maybe` + [:encryption [:enum :never :maybe]] + ;; should this setting be serialized? [:export? :boolean] @@ -327,7 +328,8 @@ (resolve-setting [k] (or (@registered-settings k) (throw (ex-info (tru "Unknown setting: {0}" k) - {:registered-settings + {::unknown-setting-error true + :registered-settings (sort (keys @registered-settings))}))))) (defn registered? @@ -386,6 +388,9 @@ (when (allows-database-local-values? setting) (core/get *database-local-values* setting-name)))) +(defn- prohibits-encryption? [setting] + (#{:never} (:encryption (resolve-setting setting)))) + (defn- allows-user-local-values? [setting] (#{:only :allowed} (:user-local (resolve-setting setting)))) @@ -974,6 +979,7 @@ :init nil :tag (default-tag-for-type setting-type) :visibility :admin + :encryption :maybe :export? false :sensitive? false :cache? true @@ -1518,3 +1524,46 @@ (:parse-error invalid-setting))) (log/warn (:parse-error invalid-setting) (format "Unable to parse setting %s" (:name invalid-setting)))))) + +(defn migrate-encrypted-settings! + "We have some settings that may currently be encrypted in the database that we'd like to disable encryption for. + This function just goes through all of them, checks to see if a value exists in the database, and re-saves it if + so. Toucan will handle decryption on the way out (if necessary) and the new value won't be encrypted. + + Note that we're completely working around the standard getters/setters here. This should be fine in this case + because: + - we're only doing anything when a value exists in the database, and + - we're setting the value to the exact same value that already exists - just a decrypted version." + [] + (doseq [setting (filter prohibits-encryption? (vals @registered-settings))] + ;; use a raw query to use `:for :update` + (t2/with-transaction [_conn] + (when-let [v (t2/select-one-fn :value :setting :key (setting-name setting) {:for :update})] + (when (not= (encryption/maybe-decrypt v) v) + ;; similarly, use `:setting` vs `:model/Setting` here to ensure the update is actually run even though Toucan + ;; thinks nothing has changed + (t2/update! :setting :key (setting-name setting) {:value (encryption/maybe-decrypt v)})))))) + +(defn- maybe-encrypt [setting-model] + ;; In tests, sometimes we need to insert/update settings that don't have definitions in the code and therefore can't + ;; be resolved. Fall back to maybe-encrypting these. + (let [resolved (try (resolve-setting (:key setting-model)) + (catch clojure.lang.ExceptionInfo e + (when (not (::unknown-setting-error (ex-data e))) + (throw e))))] + (cond-> setting-model + (or (nil? resolved) + (not (prohibits-encryption? resolved))) + (update :value encryption/maybe-encrypt)))) + +(t2/define-before-update :model/Setting + [setting] + (maybe-encrypt setting)) + +(t2/define-before-insert :model/Setting + [setting] + (maybe-encrypt setting)) + +(t2/define-after-select :model/Setting + [setting] + (update setting :value encryption/maybe-decrypt)) diff --git a/src/metabase/public_settings.clj b/src/metabase/public_settings.clj index 0b37da0bb0b..72493d73d1b 100644 --- a/src/metabase/public_settings.clj +++ b/src/metabase/public_settings.clj @@ -211,6 +211,7 @@ (application-name-for-setting-descriptions)) :type :boolean :default true + :encryption :never :visibility :public :audit :getter) -- GitLab