Mb db connection uri parsing (#14716)

* Parse mb-db-connection-uri when it contains a password

Using the raw jdbc string when its of the form
`username:password@host:port` causes problems if passed directly to
jdbc. Clojure.java.jdbc has some special handling for username and
passwords

```clojure
(if-let [user-info (.getUserInfo uri)]
  {:user (first (str/split user-info #":"))
   :password (second (str/split user-info #":"))})
```

Our logic here is:
1. if mb-db-connection-uri is set and not an older style password,
return that, possibly prepending jdbc: to it
2. if mb-db-connection-uri is set and is an older style passowrd, then
we use the old parsing code which doesn't support all of the crazy
stuff individual connection strings may offer and return a db-spec
3. if all the individual parts are supplied (mb-db-host, mb-db-port,
etc) are used to construct a db-spec

* Correct what old style means for conn-uri

accidentally thought user@host/metabase?password=password was ok. Any
credentially in the host spot is not supported

* Cleanup documentation

don't refer to old password style but call it inline credentials

* Fixups for connection parsing

- ensure we turn postgres: to postgresql: in connection string
- ensure we warn about postgres ssl edge case when using raw
  connection string
- make warnings testable

* Space in inline credentials warning

* Strip jdbc from connection when checking if inline credentials

* Fix for old credential style

src/metabase/db/env.clj

@@ -4,6 +4,10 @@
   enviornment variables e.g. `MB_DB_TYPE`, `MB_DB_HOST`, etc. `MB_DB_CONNECTION_URI` is used preferentially if both
   are specified.
 
+  The `MB_DB_CONNECTION_URI` is unparsed and passed to the jdbc driver with once exception: if it includes credentials
+  like `username:password@host:port`. In this case we parse this and log. This functionality will be removed at some
+  point so.
+
   There are two ways we specify JDBC connection information in Metabase code:
 
   1. As a 'connection details' map that is meant to be UI-friendly; this is the actual map we save when creating a
@@ -18,11 +22,15 @@
   Normally you should use the equivalent functions in `metabase.db.connection` which can be overridden rather than
   using this namespace directly."
   (:require [clojure.java.io :as io]
+            [clojure.string :as str]
             [clojure.tools.logging :as log]
+            [clojure.walk :as walk]
             [metabase.config :as config]
             [metabase.db.spec :as db.spec]
             [metabase.util :as u]
-            [metabase.util.i18n :refer [trs]]))
+            [metabase.util.i18n :refer [trs]]
+            [ring.util.codec :as codec])
+  (:import java.net.URI))
 
 (defn- get-db-file
   "Takes a filename and converts it to H2-compatible filename."
@@ -50,20 +58,146 @@
      (let [db-file-name (config/config-str :mb-db-file)]
        (get-db-file db-file-name)))))
 
-(defn- format-connection-uri
-  "Prepends \"jdbc:\" to the connection-uri string if needed."
+(def ^:private jdbc-connection-regex
+  "Regex to be used only when `:mb-db-connection-uri` includes credentials like `username:password@host:port`."
+  #"^(jdbc:)?([^:/@]+)://(?:([^:/@]+)(?::([^:@]+))?@)?([^:@]+)(?::(\d+))?/([^/?]+)(?:\?(.*))?$")
+
+(declare connection-details->jdbc-spec)
+
+(defn- suspicious-postgres-details?
+  "If postgres connection seems iffy. #8908
+  https://github.com/metabase/metabase/issues/8908"
+  [details]
+  (and (= (:ssl details) "true")
+       (not (:sslmode details))))
+
+(defn- parse-connection-string
+  "Parse a DB connection URI like
+  `postgres://cam@localhost.com:5432/cams_cool_db?ssl=true&sslfactory=org.postgresql.ssl.NonValidatingFactory` and
+  return a broken-out map. This should not be used unless the connection string uses the old style where the
+  credentials are included in the like `username:password@host:port`."
+  [uri]
+  (when-let [[_ _ protocol user pass host port db query] (re-matches jdbc-connection-regex uri)]
+    (let [protocol (case (keyword protocol)
+                     :postgres   :postgres
+                     :postgresql :postgres
+                     :mysql      :mysql
+                     :h2         :h2)
+          details
+          (connection-details->jdbc-spec
+           protocol
+           (merge {:type protocol}
+
+                  (case (keyword protocol)
+                    :h2 {:db db}
+                    {:user     user
+                     :password pass
+                     :host     host
+                     :port     port
+                     :dbname   db})
+                  (some-> query
+                          codec/form-decode
+                          walk/keywordize-keys)))]
+      ;; If someone is using Postgres and specifies `ssl=true` they might need to specify `sslmode=require`. Let's let
+      ;; them know about that to make their lives a little easier. See
+      ;; https://github.com/metabase/metabase/issues/8908 for more details.
+      {:connection details
+       :diags (cond-> #{:env.warning/inline-credentials}
+                (and (= protocol :postgres) (suspicious-postgres-details? details))
+                (conj :env.warning/postgres-ssl))})))
+
+(defn- fixup-connection-string
+  "When we allow a raw connection string as our connection, we still perform a few fixups:
+  - ensure it begins with jdbc:
+  - we allow `postgres:` and must change that to `postgresql:`
+  - warn if postgres ssl settings might cause issues
+
+  Return is a map of {:connection string|spec :diags #{info or warnings}}"
   [connection-uri]
-  (if-let [uri connection-uri]
-    (if (re-find #"^jdbc:" uri)
-      uri
-      (str "jdbc:" uri))))
+  (when connection-uri
+    (reduce (fn [{:keys [connection] :as m} {:keys [pred diag fix]}]
+              (if (pred connection)
+                (-> m (update :connection fix) (update :diags conj diag))
+                m))
+            {:connection connection-uri
+             :diags #{}}
+            [{:pred #(not (.startsWith ^String % "jdbc:"))
+              :diag :env.info/prepend-jdbc
+              :fix  #(str "jdbc:" %)}
+             {:pred #(re-find #"postgres:" %)
+              :diag :env.info/change-to-postgresql
+              :fix #(str/replace % "postgres:" "postgresql:")}
+             {:pred (fn parse-query-for-postgres
+                      [^String conn]
+                      (when (re-find #"postgres(?:ql)?:" conn)
+                        ;; jdbc:postgresql: is an opaque URI not subject to further parsing. Strip that off and we can
+                        ;; use the structural .getQuery from the URI rather than parsing ourselves
+                        (when-let [details (some-> (.getQuery (URI. (str/replace conn #"^jdbc:" "")))
+                                                   codec/form-decode
+                                                   walk/keywordize-keys)]
+                          (suspicious-postgres-details? details))))
+              :fix identity
+              :diag :env.warning/postgres-ssl}])))
+
+(defn old-credential-style?
+  "Parse a jdbc connection uri to check for older style credential passing like:
+  mysql://foo:password@172.17.0.2:3306/metabase"
+  [connection-uri]
+  (when connection-uri
+    ;; strip the jdbc prefix off so its a parseable and not opaque URI
+    (let [uri (URI. (str/replace connection-uri #"^jdbc:" ""))]
+      ;; this is how clojure.java.jdbc does it
+      (some? (.getUserInfo uri)))))
+
+(defn- connection-from-jdbc-string
+  "If connection string uses the form `username:password@host:port`, use our custom parsing to return a jdbc spec and
+  warn about this deprecated behavior. If not, return the jdbc string as is since our parsing does not offer all of
+  the options of using a raw jdbc string.
+  Return is a map of {:connection string|spec :diags #{info or warnings}}"
+  [conn-string]
+  (when conn-string
+    (if (old-credential-style? conn-string)
+        ;; prefer not parsing as we don't handle all features of connection strings
+        (parse-connection-string conn-string)
+        (fixup-connection-string conn-string))))
+
+(defn- log-inline-credentials! []
+  (log/warn
+   (u/format-color 'red
+       (str
+        (trs "Warning: using credentials provided inline is deprecated. ")
+        (trs "Change to using the credentials as a query parameter: `?password=your-password&user=user`.")))))
+
+(defn- log-postgres-ssl []
+  (log/warn (trs "Warning: Postgres connection string with `ssl=true` detected.")
+            (trs "You may need to add `?sslmode=require` to your application DB connection string.")
+            (trs "If Metabase fails to launch, please add it and try again.")
+            (trs "See https://github.com/metabase/metabase/issues/8908 for more details.")))
+
+(defn- emit-diags! [diagnostic]
+  (case diagnostic
+    :env.info/change-to-postgresql  (log/info (trs "Replaced 'postgres:' with 'postgresql:' in connection string"))
+    :env.info/prepend-jdbc          (log/info (trs "Prepended 'jdbc:' onto connection string"))
+    :env.warning/inline-credentials (log-inline-credentials!)
+    :env.warning/postgres-ssl       (log-postgres-ssl)
+    (log/warn (trs "Unknown diagnostic in db connection: {0}" diagnostic))))
+
+(def ^:private connection-string-or-spec
+  "Uses `:mb-db-connection-uri` and is either:
+  - the jdbc connection string if it does not use inline credentials, or
+  - a db-spec parsed by this code if it does use inline credentials, or
+  - nil when this value is not set."
+  (delay (when-let [conn-uri (config/config-str :mb-db-connection-uri)]
+           (let [{:keys [connection diags]} (connection-from-jdbc-string conn-uri)]
+             (run! emit-diags! diags)
+             connection))))
 
-(def ^:private connection-string
-  (delay (format-connection-uri (config/config-str :mb-db-connection-uri))))
+(defn- connection-string-or-spec->db-type [x]
+  (cond
+    (map? x) (:type x)
 
-(defn- connection-string->db-type [s]
-  (when s
-    (let [[_ subprotocol] (re-find #"^(?:jdbc:)?([^:]+):" s)]
+    (string? x)
+    (let [[_ subprotocol] (re-find #"^(?:jdbc:)?([^:]+):" x)]
       (try
         (case (keyword subprotocol)
           :postgres   :postgres
@@ -78,7 +212,7 @@
                           {:subprotocol subprotocol})))))))
 
 (def ^:private connection-string-db-type
-  (delay (connection-string->db-type @connection-string)))
+  (delay (connection-string-or-spec->db-type @connection-string-or-spec)))
 
 (def db-type
   "Keyword type name of the application DB details specified by environment variables. Matches corresponding driver
@@ -126,5 +260,5 @@
 (def jdbc-spec
   "`clojure.java.jdbc` spec map for the application DB, using the details map derived from environment variables."
   (delay
-    (or @connection-string
+    (or @connection-string-or-spec
         (connection-details->jdbc-spec @db-type @db-connection-details))))

test/metabase/db/env_test.clj

@@ -2,7 +2,7 @@
   (:require [clojure.test :refer :all]
             [metabase.db.env :as mdb.env]))
 
-(deftest connection-string->db-type-test
+(deftest connection-string-or-spec->db-type-test
   (doseq [[subprotocol expected] {"postgres"   :postgres
                                   "postgresql" :postgres
                                   "mysql"      :mysql
@@ -12,19 +12,97 @@
                                   (str protocol ":abc")
                                   (str protocol ":cam@localhost/my_db?password=123456")
                                   (str protocol "://localhost/my_db")]]
-    (testing (pr-str (list 'connection-string->db-type url))
+    (testing (pr-str (list 'connection-string-or-spec->db-type url))
       (is (= expected
-             (#'mdb.env/connection-string->db-type url)))))
+             (#'mdb.env/connection-string-or-spec->db-type url)))))
   (testing "Should throw an Exception for an unsupported subprotocol"
     (is (thrown-with-msg?
          clojure.lang.ExceptionInfo
          #"Unsupported application database type: \"sqlserver\""
-         (#'mdb.env/connection-string->db-type "jdbc:sqlserver://bad")))))
+         (#'mdb.env/connection-string-or-spec->db-type "jdbc:sqlserver://bad"))))
+  (testing "Finds type from jdbc-spec"
+    (let [spec {:password "password",
+                :characterSetResults "UTF8",
+                :characterEncoding "UTF8",
+                :type :mysql,
+                :classname "org.mariadb.jdbc.Driver",
+                :subprotocol "mysql",
+                :useSSL false,
+                :zeroDateTimeBehavior "convertToNull",
+                :user "foo",
+                :subname "//172.17.0.2:3306/metabase",
+                :useCompression true,
+                :useUnicode true}]
+      (is (= :mysql (#'mdb.env/connection-string-or-spec->db-type spec))))))
 
-(deftest format-connection-uri-test
-  (let [conn-uri "postgresql://localhost:metabase?username=johndoe"
-        jdbc-conn-uri (str "jdbc:" conn-uri)]
-    (doseq [[input expected] [[conn-uri jdbc-conn-uri]
-                              [jdbc-conn-uri jdbc-conn-uri]
-                              [nil nil]]]
-      (is (= expected (#'mdb.env/format-connection-uri input))))))
+(deftest fixup-connection-string-test
+  (let [correct-uri "jdbc:postgresql://localhost:metabase?username=johndoe"]
+    (doseq [[input expected-diags expected]
+            [["postgres://localhost:metabase?username=johndoe"
+              #{:env.info/prepend-jdbc :env.info/change-to-postgresql}
+              correct-uri]
+             ["jdbc:postgres://localhost:metabase?username=johndoe"
+              #{:env.info/change-to-postgresql}
+              correct-uri]
+             [correct-uri #{} correct-uri]
+             [nil nil nil]]]
+      (let [{:keys [connection diags]} (#'mdb.env/fixup-connection-string input)]
+        (is (= expected connection) input)
+        (is (= expected-diags diags) input)))))
+
+(deftest old-credential-style?-test
+  (are [old? conn-uri] (is (= old? (#'mdb.env/old-credential-style? conn-uri)) conn-uri)
+    false "jdbc:mysql://172.17.0.2:3306/metabase?password=password&user=user"
+    false "mysql://172.17.0.2:3306/metabase?password=password&user=user"
+    ;; prefixed with jdbc makes an "opaque" URI which doesn't parse
+    true  "jdbc:mysql://foo@172.17.0.2:3306/metabase?password=password"
+    true  "mysql://foo@172.17.0.2:3306/metabase?password=password"
+    true  "mysql://foo:password@172.17.0.2:3306/metabase"))
+
+(deftest connection-from-jdbc-string-test
+  (let [parsed {:classname   "org.mariadb.jdbc.Driver"
+                :subprotocol "mysql"
+                :subname     "//172.17.0.2:3306/metabase"
+                :type        :mysql
+                :user        "foo"
+                :password    "password"
+                :dbname      "metabase"}]
+    (testing "handles mixed (just password in query params) old style just fine"
+      (let [conn-uri "mysql://foo@172.17.0.2:3306/metabase?password=password"]
+        (is (= {:connection parsed
+                :diags #{:env.warning/inline-credentials}}
+               (#'mdb.env/connection-from-jdbc-string conn-uri)))))
+    (testing "When using old-style passwords parses and returns jdbc specs"
+      (let [conn-uri "mysql://foo:password@172.17.0.2:3306/metabase"]
+        (is (= {:connection parsed
+                :diags #{:env.warning/inline-credentials}}
+               (#'mdb.env/connection-from-jdbc-string conn-uri))))))
+  (testing "handles credentials in query params"
+    (let [conn-uri "mysql://172.17.0.2:3306/metabase?user=user&password=password"]
+      (is (= {:connection (str "jdbc:" conn-uri)
+              :diags #{:env.info/prepend-jdbc}}
+             (#'mdb.env/connection-from-jdbc-string conn-uri)))))
+  (testing "warns about postgres ssl issue #8908"
+    (testing "when it parses due to inline credentials"
+      (let [conn-uri "postgresql://mb@172.17.0.2:5432/metabase?ssl=true"]
+        (is (= {:connection
+                {:ssl "true",
+                 :OpenSourceSubProtocolOverride true,
+                 :password nil,
+                 :type :postgres,
+                 :classname "org.postgresql.Driver",
+                 :subprotocol "postgresql",
+                 :dbname "metabase",
+                 :user "mb",
+                 :subname "//172.17.0.2:5432/metabase"},
+                :diags #{:env.warning/postgres-ssl :env.warning/inline-credentials}}
+               (#'mdb.env/connection-from-jdbc-string conn-uri)))))
+    (testing "when it doesn't parse as well"
+      (doseq [conn-uri ["jdbc:postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"
+                        "postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"
+                        "postgres://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true"]]
+        (let [{:keys [connection diags]} (#'mdb.env/connection-from-jdbc-string conn-uri)]
+          (is (= "jdbc:postgresql://172.17.0.2:5432/metabase?user=mb&password=pw&ssl=true" connection))
+          (is (contains? diags :env.warning/postgres-ssl))))))
+  (testing "handles nil"
+    (is (nil? (#'mdb.env/connection-from-jdbc-string nil)))))