Skip to content
Snippets Groups Projects
Unverified Commit d514791a authored by Robert Roland's avatar Robert Roland Committed by GitHub
Browse files

"upsert" sensitive password-like fields during DB update (#13587) 

When a database is updated via the admin interface, any sensitive data
(private keys, passwords, etc) should be handled on an "upsert" basis -
only modify those fields if they were changed from the "protected
password" value.

This affects all database drivers.

Resolves #13442
parent dc39729d
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
frontend/src/metabase/entities/databases/forms.js
+ 16
− 11
View file @ d514791a
...@@ -10,22 +10,22 @@ import MetadataSyncScheduleWidget from "metabase/admin/databases/components/widg ...@@ -10,22 +10,22 @@ import MetadataSyncScheduleWidget from "metabase/admin/databases/components/widg
import CacheFieldValuesScheduleWidget from "metabase/admin/databases/components/widgets/CacheFieldValuesScheduleWidget"; import CacheFieldValuesScheduleWidget from "metabase/admin/databases/components/widgets/CacheFieldValuesScheduleWidget";
const DATABASE_DETAIL_OVERRIDES = { const DATABASE_DETAIL_OVERRIDES = {
"tunnel-enabled": (engine, details) => ({ "tunnel-enabled": (engine, details, id) => ({
title: t`Use an SSH-tunnel for database connections`, title: t`Use an SSH-tunnel for database connections`,
description: t`Some database installations can only be accessed by connecting through an SSH bastion host. This option also provides an extra layer of security when a VPN is not available. Enabling this is usually slower than a direct connection.`, description: t`Some database installations can only be accessed by connecting through an SSH bastion host. This option also provides an extra layer of security when a VPN is not available. Enabling this is usually slower than a direct connection.`,
}), }),
"use-jvm-timezone": (engine, details) => ({ "use-jvm-timezone": (engine, details, id) => ({
title: t`Use the Java Virtual Machine (JVM) timezone`, title: t`Use the Java Virtual Machine (JVM) timezone`,
description: t`We suggest you leave this off unless you're doing manual timezone casting in many or most of your queries with this data.`, description: t`We suggest you leave this off unless you're doing manual timezone casting in many or most of your queries with this data.`,
}), }),
"use-srv": (engine, details) => ({ "use-srv": (engine, details, id) => ({
title: t`Use DNS SRV when connecting`, title: t`Use DNS SRV when connecting`,
description: t`Using this option requires that provided host is a FQDN. If connecting to an Atlas cluster, you might need to enable this option. If you don't know what this means, leave this disabled.`, description: t`Using this option requires that provided host is a FQDN. If connecting to an Atlas cluster, you might need to enable this option. If you don't know what this means, leave this disabled.`,
}), }),
"client-id": (engine, details) => ({ "client-id": (engine, details, id) => ({
description: getClientIdDescription(engine, details), description: getClientIdDescription(engine, details),
}), }),
"auth-code": (engine, details) => ({ "auth-code": (engine, details, id) => ({
description: ( description: (
<div> <div>
<div>{getAuthCodeLink(engine, details)}</div> <div>{getAuthCodeLink(engine, details)}</div>
...@@ -33,8 +33,13 @@ const DATABASE_DETAIL_OVERRIDES = { ...@@ -33,8 +33,13 @@ const DATABASE_DETAIL_OVERRIDES = {
</div> </div>
), ),
}), }),
"service-account-json": (engine, details) => ({ "service-account-json": (engine, details, id) => ({
validate: value => { validate: value => {
// this field is only required if this is a new entry
if (id) {
return null;
}
if (!value) { if (!value) {
return t`required`; return t`required`;
} }
...@@ -46,22 +51,22 @@ const DATABASE_DETAIL_OVERRIDES = { ...@@ -46,22 +51,22 @@ const DATABASE_DETAIL_OVERRIDES = {
return null; return null;
}, },
}), }),
"tunnel-private-key": (engine, details) => ({ "tunnel-private-key": (engine, details, id) => ({
title: t`SSH private key`, title: t`SSH private key`,
placeholder: t`Paste the contents of your ssh private key here`, placeholder: t`Paste the contents of your ssh private key here`,
type: "text", type: "text",
}), }),
"tunnel-private-key-passphrase": (engine, details) => ({ "tunnel-private-key-passphrase": (engine, details, id) => ({
title: t`Passphrase for the SSH private key`, title: t`Passphrase for the SSH private key`,
}), }),
"tunnel-auth-option": (engine, details) => ({ "tunnel-auth-option": (engine, details, id) => ({
title: t`SSH Authentication`, title: t`SSH Authentication`,
options: [ options: [
{ name: t`SSH Key`, value: "ssh-key" }, { name: t`SSH Key`, value: "ssh-key" },
{ name: t`Password`, value: "password" }, { name: t`Password`, value: "password" },
], ],
}), }),
"ssl-cert": (engine, details) => ({ "ssl-cert": (engine, details, id) => ({
title: t`Server SSL certificate chain`, title: t`Server SSL certificate chain`,
placeholder: t`Paste the contents of the server's SSL certificate chain here`, placeholder: t`Paste the contents of the server's SSL certificate chain here`,
type: "text", type: "text",
...@@ -234,7 +239,7 @@ function getFieldsForEngine(engine, details, id) { ...@@ -234,7 +239,7 @@ function getFieldsForEngine(engine, details, id) {
horizontal: field.type === "boolean", horizontal: field.type === "boolean",
initial: field.default, initial: field.default,
readOnly: field.readOnly || false, readOnly: field.readOnly || false,
...(overrides && overrides(engine, details)), ...(overrides && overrides(engine, details, id)),
}); });
} }
return fields; return fields;
......
src/metabase/api/database.clj
+ 27
− 15
View file @ d514791a
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
(:require [clojure.string :as str] (:require [clojure.string :as str]
[clojure.tools.logging :as log] [clojure.tools.logging :as log]
[compojure.core :refer [DELETE GET POST PUT]] [compojure.core :refer [DELETE GET POST PUT]]
[medley.core :as m]
[metabase [metabase
[config :as config] [config :as config]
[driver :as driver] [driver :as driver]
...@@ -533,6 +534,19 @@ ...@@ -533,6 +534,19 @@
;;; --------------------------------------------- PUT /api/database/:id ---------------------------------------------- ;;; --------------------------------------------- PUT /api/database/:id ----------------------------------------------
(defn upsert-sensitive-fields
"Replace any sensitive values not overriden in the PUT with the original values"
[database details]
(when details
(merge (:details database)
(reduce
(fn [details k]
(if (= protected-password (get details k))
(m/update-existing details k (constantly (get-in database [:details k])))
details))
details
database/sensitive-fields))))
(api/defendpoint PUT "/:id" (api/defendpoint PUT "/:id"
"Update a `Database`." "Update a `Database`."
[id :as {{:keys [name engine details is_full_sync is_on_demand description caveats points_of_interest schedules [id :as {{:keys [name engine details is_full_sync is_on_demand description caveats points_of_interest schedules
...@@ -547,9 +561,7 @@ ...@@ -547,9 +561,7 @@
auto_run_queries (s/maybe s/Bool)} auto_run_queries (s/maybe s/Bool)}
(api/check-superuser) (api/check-superuser)
(api/let-404 [database (Database id)] (api/let-404 [database (Database id)]
(let [details (if-not (= protected-password (:password details)) (let [details (upsert-sensitive-fields database details)
details
(assoc details :password (get-in database [:details :password])))
conn-error (when (some? details) conn-error (when (some? details)
(assert (some? engine)) (assert (some? engine))
(test-database-connection engine details)) (test-database-connection engine details))
...@@ -565,18 +577,18 @@ ...@@ -565,18 +577,18 @@
;; that seems like the kind of thing that will almost never work in any practical way ;; that seems like the kind of thing that will almost never work in any practical way
;; TODO - this means one cannot unset the description. Does that matter? ;; TODO - this means one cannot unset the description. Does that matter?
(api/check-500 (db/update-non-nil-keys! Database id (api/check-500 (db/update-non-nil-keys! Database id
(merge (merge
{:name name {:name name
:engine engine :engine engine
:details details :details details
:is_full_sync full-sync? :is_full_sync full-sync?
:is_on_demand (boolean is_on_demand) :is_on_demand (boolean is_on_demand)
:description description :description description
:caveats caveats :caveats caveats
:points_of_interest points_of_interest :points_of_interest points_of_interest
:auto_run_queries auto_run_queries} :auto_run_queries auto_run_queries}
(when schedules (when schedules
(schedule-map->cron-strings schedules))))) (schedule-map->cron-strings schedules)))))
(let [db (Database id)] (let [db (Database id)]
(events/publish-event! :database-update db) (events/publish-event! :database-update db)
;; return the DB with the expanded schedules back in place ;; return the DB with the expanded schedules back in place
......
test/metabase/api/database_test.clj
+ 123
− 10
View file @ d514791a
...@@ -13,6 +13,7 @@ ...@@ -13,6 +13,7 @@
[metabase.driver.util :as driver.u] [metabase.driver.util :as driver.u]
[metabase.mbql.schema :as mbql.s] [metabase.mbql.schema :as mbql.s]
[metabase.models [metabase.models
[database :as database :refer [protected-password]]
[permissions :as perms] [permissions :as perms]
[permissions-group :as perms-group]] [permissions-group :as perms-group]]
[metabase.sync [metabase.sync
...@@ -197,16 +198,17 @@ ...@@ -197,16 +198,17 @@
:features (driver.u/features :h2)} :features (driver.u/features :h2)}
(into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id))))))))) (into {} (db/select-one [Database :name :engine :details :is_full_sync], :id db-id)))))))))
(testing "should be able to set `auto_run_queries`" (mt/with-log-level :info
(testing "when creating a Database" (testing "should be able to set `auto_run_queries`"
(is (= {:auto_run_queries false} (testing "when creating a Database"
(select-keys (create-db-via-api! {:auto_run_queries false}) [:auto_run_queries])))) (is (= {:auto_run_queries false}
(testing "when updating a Database" (select-keys (create-db-via-api! {:auto_run_queries false}) [:auto_run_queries]))))
(mt/with-temp Database [{db-id :id} {:engine ::test-driver}] (testing "when updating a Database"
(let [updates {:auto_run_queries false}] (mt/with-temp Database [{db-id :id} {:engine ::test-driver}]
((mt/user->client :crowberto) :put 200 (format "database/%d" db-id) updates)) (let [updates {:auto_run_queries false}]
(is (= false ((mt/user->client :crowberto) :put 200 (format "database/%d" db-id) updates))
(db/select-one-field :auto_run_queries Database, :id db-id)))))))) (is (= false
(db/select-one-field :auto_run_queries Database, :id db-id)))))))))
(deftest fetch-database-metadata-test (deftest fetch-database-metadata-test
(testing "GET /api/database/:id/metadata" (testing "GET /api/database/:id/metadata"
...@@ -941,3 +943,114 @@ ...@@ -941,3 +943,114 @@
(is (schema= [{:schema (s/eq schema-name) (is (schema= [{:schema (s/eq schema-name)
s/Keyword s/Any}] s/Keyword s/Any}]
((mt/user->client :rasta) :get 200 url)))))))))))) ((mt/user->client :rasta) :get 200 url))))))))))))
(deftest upsert-sensitive-values-test
(testing "empty maps are okay"
(is (= {}
(database-api/upsert-sensitive-fields {} {}))))
(testing "no details updates are okay"
(is (= nil
(database-api/upsert-sensitive-fields nil nil))))
(testing "fields are replaced"
(is (= {:use-service-account nil
:dataset-id "dacort"
:use-jvm-timezone false
:service-account-json "{\"foo\": \"bar\"}"
:password "foo"
:pass "bar"
:tunnel-pass "quux"
:tunnel-private-key "foobar"
:tunnel-private-key-passphrase "fooquux"
:access-token "foobarfoo"
:refresh-token "foobarquux"}
(database-api/upsert-sensitive-fields {:description nil
:name "customer success BQ"
:details {:use-service-account nil
:dataset-id "dacort"
:service-account-json "{}"
:use-jvm-timezone false
:password "password"
:pass "pass"
:tunnel-pass "tunnel-pass"
:tunnel-private-key "tunnel-private-key"
:tunnel-private-key-passphrase "tunnel-private-key-passphrase"
:access-token "access-token"
:refresh-token "refresh-token"}
:id 2}
{:service-account-json "{\"foo\": \"bar\"}"
:password "foo"
:pass "bar"
:tunnel-pass "quux"
:tunnel-private-key "foobar"
:tunnel-private-key-passphrase "fooquux"
:access-token "foobarfoo"
:refresh-token "foobarquux"}))))
(testing "no fields are replaced"
(is (= {:use-service-account nil
:dataset-id "dacort"
:use-jvm-timezone false
:service-account-json "{}"
:password "password"
:pass "pass"
:tunnel-pass "tunnel-pass"
:tunnel-private-key "tunnel-private-key"
:tunnel-private-key-passphrase "tunnel-private-key-passphrase"
:access-token "access-token"
:refresh-token "refresh-token"}
(database-api/upsert-sensitive-fields {:description nil
:name "customer success BQ"
:details {:use-service-account nil
:dataset-id "dacort"
:use-jvm-timezone false
:service-account-json "{}"
:password "password"
:pass "pass"
:tunnel-pass "tunnel-pass"
:tunnel-private-key "tunnel-private-key"
:tunnel-private-key-passphrase "tunnel-private-key-passphrase"
:access-token "access-token"
:refresh-token "refresh-token"}
:id 2}
{:service-account-json protected-password
:password protected-password
:pass protected-password
:tunnel-pass protected-password
:tunnel-private-key protected-password
:tunnel-private-key-passphrase protected-password
:access-token protected-password
:refresh-token protected-password}))))
(testing "only one field is replaced"
(is (= {:use-service-account nil
:dataset-id "dacort"
:use-jvm-timezone false
:service-account-json "{}"
:password "new-password"
:pass "pass"
:tunnel-pass "tunnel-pass"
:tunnel-private-key "tunnel-private-key"
:tunnel-private-key-passphrase "tunnel-private-key-passphrase"
:access-token "access-token"
:refresh-token "refresh-token"}
(database-api/upsert-sensitive-fields {:description nil
:name "customer success BQ"
:details {:use-service-account nil
:dataset-id "dacort"
:use-jvm-timezone false
:service-account-json "{}"
:password "password"
:pass "pass"
:tunnel-pass "tunnel-pass"
:tunnel-private-key "tunnel-private-key"
:tunnel-private-key-passphrase "tunnel-private-key-passphrase"
:access-token "access-token"
:refresh-token "refresh-token"}
:id 2}
{:service-account-json protected-password
:password "new-password"
:pass protected-password
:tunnel-pass protected-password
:tunnel-private-key protected-password
:tunnel-private-key-passphrase protected-password
:access-token protected-password
:refresh-token protected-password})))))
\ No newline at end of file
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment