Skip to content
Snippets Groups Projects
Unverified Commit 1ead699e authored by Noah Moss's avatar Noah Moss Committed by GitHub
Browse files

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
parent b0a48491
No related branches found
No related tags found
No related merge requests found
Hide whitespace changes
Inline Side-by-side
src/metabase/driver/postgres.clj
+ 16
− 8
View file @ 1ead699e
......@@ -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->
......
src/metabase/models/secret.clj
+ 56
− 46
View file @ 1ead699e
......@@ -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)
......
test/metabase/driver/postgres_test.clj
+ 40
− 17
View file @ 1ead699e
......@@ -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"))))))
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