From 1ead699e7a184114f54f065be3a1f37745e5b9a4 Mon Sep 17 00:00:00 2001
From: Noah Moss <32746338+noahmoss@users.noreply.github.com>
Date: Fri, 9 Sep 2022 15:20:04 -0400
Subject: [PATCH] Fix issues with postgres PKCS-12 SSL connections (#25299)
* use .p12 file extension for pkcs-12 keys
* test
* fix messed up indentation
* default to empty string ssl password
* address feedback
* fix tests
---
src/metabase/driver/postgres.clj | 24 ++++--
src/metabase/models/secret.clj | 102 ++++++++++++++-----------
test/metabase/driver/postgres_test.clj | 57 +++++++++-----
3 files changed, 112 insertions(+), 71 deletions(-)
diff --git a/src/metabase/driver/postgres.clj b/src/metabase/driver/postgres.clj
index 87651288d52..033abfe39c5 100644
--- a/src/metabase/driver/postgres.clj
+++ b/src/metabase/driver/postgres.clj
@@ -196,9 +196,9 @@
(comp (mapcat get-typenames)
(map keyword))
(jdbc/query (sql-jdbc.conn/db->pooled-connection-spec database)
- [(str "SELECT nspname, typname "
- "FROM pg_type t JOIN pg_namespace n ON n.oid = t.typnamespace "
- "WHERE t.oid IN (SELECT DISTINCT enumtypid FROM pg_enum e)")])))
+ [(str "SELECT nspname, typname "
+ "FROM pg_type t JOIN pg_namespace n ON n.oid = t.typnamespace "
+ "WHERE t.oid IN (SELECT DISTINCT enumtypid FROM pg_enum e)")])))
(def ^:private ^:dynamic *enum-types* nil)
@@ -358,7 +358,7 @@
;; Postgres is not happy with JSON fields which are in group-bys or order-bys
;; being described twice instead of using the alias.
;; Therefore, force the alias, but only for JSON fields to avoid ambiguity.
-;; The alias names in JSON fields are unique wrt nfc path"
+;; The alias names in JSON fields are unique wrt nfc path
(defmethod sql.qp/apply-top-level-clause
[:postgres :breakout]
[driver clause honeysql-form {breakout-fields :breakout, _fields-fields :fields :as query}]
@@ -509,9 +509,16 @@
"inet" :type/IPAddress
nil))
+(defn- pkcs-12-key-value?
+ "If a value was uploaded for the SSL key, return whether it's using the PKCS-12 format."
+ [ssl-key-value]
+ (when ssl-key-value
+ (= (second (re-find secret/uploaded-base-64-prefix-pattern ssl-key-value))
+ "x-pkcs12")))
+
(defn- ssl-params
"Builds the params to include in the JDBC connection spec for an SSL connection."
- [db-details]
+ [{:keys [ssl-key-value] :as db-details}]
(let [ssl-root-cert (when (contains? #{"verify-ca" "verify-full"} (:ssl-mode db-details))
(secret/db-details-prop->secret-map db-details "ssl-root-cert"))
ssl-client-key (when (:ssl-use-client-auth db-details)
@@ -531,13 +538,14 @@
(assoc :sslrootcert (secret/value->file! ssl-root-cert :postgres))
(has-value? ssl-client-key)
- (assoc :sslkey (secret/value->file! ssl-client-key :postgres))
+ (assoc :sslkey (secret/value->file! ssl-client-key :postgres (when (pkcs-12-key-value? ssl-key-value) ".p12")))
(has-value? ssl-client-cert)
(assoc :sslcert (secret/value->file! ssl-client-cert :postgres))
- (has-value? ssl-key-pw)
- (assoc :sslpassword (secret/value->string ssl-key-pw))
+ ;; Pass an empty string as password if none is provided; otherwise the driver will prompt for one
+ true
+ (assoc :sslpassword (or (secret/value->string ssl-key-pw) ""))
true
(as-> params ;; from outer cond->
diff --git a/src/metabase/models/secret.clj b/src/metabase/models/secret.clj
index c6ee61bc318..7d78295a516 100644
--- a/src/metabase/models/secret.clj
+++ b/src/metabase/models/secret.clj
@@ -62,48 +62,54 @@
`driver?` is an optional argument that is only used if an ostensibly existing file value (i.e. `:file-path`) can't be
resolved, in order to render a more user-friendly error message (by looking up the display names of the connection
- properties involved)."
+ properties involved).
+
+ `ext?` is an optional argument that sets the file extension used for the temporary file, if one needs to be created."
{:added "0.42.0"}
- ^File [{:keys [connection-property-name id value] :as secret} driver?]
- (if (= :file-path (:source secret))
- (let [secret-val (value->string secret)
- ^File existing-file (File. secret-val)]
- (if (.exists existing-file)
- existing-file
- (let [error-source (cond
- id
- (tru "Secret ID {0}" id)
-
- (and connection-property-name driver?)
- (let [secret-props (-> (driver/connection-properties driver?)
- conn-props->secret-props-by-name)]
- (tru "File path for {0}" (-> (get secret-props connection-property-name)
- :display-name)))
-
- :else
- (tru "Path"))]
- (throw (ex-info (tru "{0} points to non-existent file: {1}" error-source secret-val)
- {:file-path secret-val
- :secret secret})))))
- (let [^File tmp-file (doto (File/createTempFile "metabase-secret_" nil)
- ;; make the file only readable by owner
- (.setReadable false false)
- (.setReadable true true)
- (.deleteOnExit))]
- (log/tracef "Creating temp file for secret %s value at %s" (or id "") (.getAbsolutePath tmp-file))
- (with-open [out (io/output-stream tmp-file)]
- (let [^bytes v (cond
- (string? value)
- (.getBytes ^String value "UTF-8")
-
- (bytes? value)
- ^bytes value)]
- (.write out v)))
- tmp-file)))
+ (^File [secret]
+ (value->file!* secret nil))
+ (^File [secret driver?]
+ (value->file!* secret driver? nil))
+ (^File [{:keys [connection-property-name id value] :as secret} driver? ext?]
+ (if (= :file-path (:source secret))
+ (let [secret-val (value->string secret)
+ ^File existing-file (File. secret-val)]
+ (if (.exists existing-file)
+ existing-file
+ (let [error-source (cond
+ id
+ (tru "Secret ID {0}" id)
+
+ (and connection-property-name driver?)
+ (let [secret-props (-> (driver/connection-properties driver?)
+ conn-props->secret-props-by-name)]
+ (tru "File path for {0}" (-> (get secret-props connection-property-name)
+ :display-name)))
+
+ :else
+ (tru "Path"))]
+ (throw (ex-info (tru "{0} points to non-existent file: {1}" error-source secret-val)
+ {:file-path secret-val
+ :secret secret})))))
+ (let [^File tmp-file (doto (File/createTempFile "metabase-secret_" ext?)
+ ;; make the file only readable by owner
+ (.setReadable false false)
+ (.setReadable true true)
+ (.deleteOnExit))]
+ (log/tracef "Creating temp file for secret %s value at %s" (or id "") (.getAbsolutePath tmp-file))
+ (with-open [out (io/output-stream tmp-file)]
+ (let [^bytes v (cond
+ (string? value)
+ (.getBytes ^String value "UTF-8")
+
+ (bytes? value)
+ ^bytes value)]
+ (.write out v)))
+ tmp-file))))
(def
^java.io.File
- ^{:arglists '([{:keys [connection-property-name id value] :as secret} driver?])}
+ ^{:arglists '([{:keys [connection-property-name id value] :as secret} & [driver? ext?]])}
value->file!
"Returns the value of the given `secret` instance in the form of a file. If the given instance has a `:file-path` as
its source, a `File` referring to that is returned. Otherwise, the `:value` is written to a temporary file, which is
@@ -111,13 +117,15 @@
`driver?` is an optional argument that is only used if an ostensibly existing file value (i.e. `:file-path`) can't be
resolved, in order to render a more user-friendly error message (by looking up the display names of the connection
- properties involved)."
+ properties involved).
+
+ `ext?` is an optional argument that sets the file extension used for the temporary file, if one needs to be created."
(memoize/memo
(with-meta value->file!*
- {::memoize/args-fn (fn [[secret _driver?]]
+ {::memoize/args-fn (fn [[secret _driver? ext?]]
;; not clear if value->string could return nil due to the cond so we'll just cache on a key
;; that is unique
- [(vec (:value secret))])})))
+ [(vec (:value secret)) ext?])})))
(defn get-sub-props
"Return a map of secret subproperties for the property `connection-property-name`."
@@ -126,7 +134,9 @@
sub-prop #(keyword (str connection-property-name "-" (name %)))]
(zipmap sub-prop-types (map sub-prop sub-prop-types))))
-(def ^:private uploaded-base-64-pattern #"^data:application/([^;]*);base64,")
+(def uploaded-base-64-prefix-pattern
+ "Regex for parsing base64 encoded file uploads."
+ #"^data:application/([^;]*);base64,")
(defn db-details-prop->secret-map
"Returns a map containing `:value` and `:source` for the given `conn-prop-nm`. `conn-prop-nm` is expected to be the
@@ -149,13 +159,13 @@
value (cond
;; ssl-root-certs will need their prefix removed, and to be base 64 decoded (#20319)
(and (value-kw details) (#{"ssl-client-cert" "ssl-root-cert"} conn-prop-nm)
- (re-find uploaded-base-64-pattern (value-kw details)))
- (-> (value-kw details) (str/replace-first uploaded-base-64-pattern "") u/decode-base64)
+ (re-find uploaded-base-64-prefix-pattern (value-kw details)))
+ (-> (value-kw details) (str/replace-first uploaded-base-64-prefix-pattern "") u/decode-base64)
(and (value-kw details) (#{"ssl-key"} conn-prop-nm)
- (re-find uploaded-base-64-pattern (value-kw details)))
+ (re-find uploaded-base-64-prefix-pattern (value-kw details)))
(.decode (java.util.Base64/getDecoder)
- (str/replace-first (value-kw details) uploaded-base-64-pattern ""))
+ (str/replace-first (value-kw details) uploaded-base-64-prefix-pattern ""))
;; the -value suffix was specified; use that
(value-kw details)
diff --git a/test/metabase/driver/postgres_test.clj b/test/metabase/driver/postgres_test.clj
index 53eb5828c71..b31526a8b44 100644
--- a/test/metabase/driver/postgres_test.clj
+++ b/test/metabase/driver/postgres_test.clj
@@ -75,6 +75,7 @@
:user "camsaul"
:ssl true
:sslmode "require"
+ :sslpassword ""
:ApplicationName config/mb-version-and-process-identifier}
(sql-jdbc.conn/connection-details->spec :postgres
{:ssl true
@@ -106,6 +107,7 @@
:sslkey "my-key"
:sslfactory "myfactoryoverride"
:sslrootcert "myrootcert"
+ :sslpassword ""
:ApplicationName config/mb-version-and-process-identifier}
(sql-jdbc.conn/connection-details->spec :postgres
{:ssl true
@@ -258,7 +260,7 @@
(mt/with-temp Database [database {:engine :postgres, :details (assoc details :dbname db-name)}]
(let [sync! #(sync/sync-database! database)]
;; create a main partitioned table and two partitions for it
- (exec! spec ["CREATE TABLE part_vals (val bigint NOT NULL) PARTITION BY RANGE (\"val\")";"
+ (exec! spec ["CREATE TABLE part_vals (val bigint NOT NULL) PARTITION BY RANGE (\"val\");"
"CREATE TABLE part_vals_0 (val bigint NOT NULL);"
"ALTER TABLE ONLY part_vals ATTACH PARTITION part_vals_0 FOR VALUES FROM (0) TO (1000);"
"CREATE TABLE part_vals_1 (val bigint NOT NULL);"
@@ -317,18 +319,18 @@
Field [val-field {:table_id (u/the-id table)
:nfc_path [:jsons "values" "qty"]
:database_type "integer"}]]
- (qp.store/with-store
- (qp.store/fetch-and-store-database! (u/the-id database))
- (qp.store/fetch-and-store-tables! [(u/the-id table)])
- (qp.store/fetch-and-store-fields! [(u/the-id val-field)])
- (let [field-clause [:field (u/the-id val-field) {:binning
- {:strategy :num-bins,
- :num-bins 100,
- :min-value 0.75,
- :max-value 54.0,
- :bin-width 0.75}}]]
- (is (= ["((floor((((complicated_identifiers.jsons#>> ?::text[])::integer - 0.75) / 0.75)) * 0.75) + 0.75)" "{values,qty}"]
- (hsql/format (sql.qp/->honeysql :postgres field-clause)))))))))))
+ (qp.store/with-store
+ (qp.store/fetch-and-store-database! (u/the-id database))
+ (qp.store/fetch-and-store-tables! [(u/the-id table)])
+ (qp.store/fetch-and-store-fields! [(u/the-id val-field)])
+ (let [field-clause [:field (u/the-id val-field) {:binning
+ {:strategy :num-bins,
+ :num-bins 100,
+ :min-value 0.75,
+ :max-value 54.0,
+ :bin-width 0.75}}]]
+ (is (= ["((floor((((complicated_identifiers.jsons#>> ?::text[])::integer - 0.75) / 0.75)) * 0.75) + 0.75)" "{values,qty}"]
+ (hsql/format (sql.qp/->honeysql :postgres field-clause)))))))))))
(deftest json-alias-test
(mt/test-driver :postgres
@@ -559,7 +561,7 @@
:parameters
[{:type "text"
:target ["dimension" ["template-tag" "user"]]
- :value "4f01dcfd-13f7-430c-8e6f-e505c0851027"}])))))
+ :value "4f01dcfd-13f7-430c-8e6f-e505c0851027"}]))))))
(testing "Check that we can filter by multiple UUIDs for SQL Field filters"
(is (= [[1 #uuid "4f01dcfd-13f7-430c-8e6f-e505c0851027"]
[3 #uuid "da1d6ecc-e775-4008-b366-c38e7a2e8433"]]
@@ -577,7 +579,7 @@
[{:type "text"
:target ["dimension" ["template-tag" "user"]]
:value ["4f01dcfd-13f7-430c-8e6f-e505c0851027"
- "da1d6ecc-e775-4008-b366-c38e7a2e8433"]}]))))))))))
+ "da1d6ecc-e775-4008-b366-c38e7a2e8433"]}])))))))))
(mt/defdataset ip-addresses
@@ -962,7 +964,7 @@
(deftest can-set-ssl-key-via-gui
(testing "ssl key can be set via the gui (#20319)"
(with-redefs [secret/value->file!
- (fn [{:keys [connection-property-name value] :as _secret} _driver?]
+ (fn [{:keys [connection-property-name value] :as _secret} & [_driver? _ext?]]
(str "file:" connection-property-name "=" value))]
(is (= "file:ssl-key=/clientkey.pkcs12"
(:sslkey
@@ -972,7 +974,7 @@
:ssl-client-cert-path "/client.pem"
:ssl-key-options "local"
:ssl-key-password-value "sslclientkeypw!"
- :ssl-key-path "/clientkey.pkcs12", ;; <-- this is what is set via ui.
+ :ssl-key-path "/clientkey.pkcs12" ;; <-- this is what is set via ui.
:ssl-mode "verify-ca"
:ssl-root-cert-options "local"
:ssl-root-cert-path "/root.pem"
@@ -985,3 +987,24 @@
:user "bcm"
:password "abcdef123"
:port 5432})))))))
+
+(deftest pkcs-12-extension-test
+ (testing "Uploaded PKCS-12 SSL keys are stored in a file with the .p12 extension (#20319)"
+ (is (true?
+ (-> (#'postgres/ssl-params
+ {:ssl true
+ :ssl-key-options "uploaded"
+ :ssl-key-value "data:application/x-pkcs12;base64,SGVsbG8="
+ :ssl-mode "require"
+ :ssl-use-client-auth true
+ :tunnel-enabled false
+ :advanced-options false
+ :dbname "metabase"
+ :engine :postgres
+ :host "localhost"
+ :user "bcm"
+ :password "abcdef123"
+ :port 5432})
+ :sslkey
+ .getAbsolutePath
+ (str/ends-with? ".p12"))))))
--
GitLab