Fix SSH tunnel leaks when testing connections (#24446)

* Fix SSH tunnel leaks when testing connections

* Appease kondo =(

* Add some more NOTES

.clj-kondo/config.edn

@@ -342,45 +342,48 @@
                                         saml20-clj.core                                                 saml
                                         toucan.db                                                       db
                                         toucan.models                                                   models}}}
- :lint-as {metabase.api.common/let-404 clojure.core/let
-           metabase.db.data-migrations/defmigration clojure.core/def
-           metabase.query-processor.error-type/deferror clojure.core/def
-           metabase.models.setting/defsetting clj-kondo.lint-as/def-catch-all
-           metabase.mbql.schema.macros/defclause clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/defenterprise clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/defenterprise-schema clj-kondo.lint-as/def-catch-all
-           metabase.public-settings.premium-features/define-premium-feature clojure.core/def
-           metabase.sync.util/sum-for clojure.core/for
-           metabase.sync.util/with-emoji-progress-bar clojure.core/let
-           metabase.driver.sql-jdbc.execute.diagnostic/capturing-diagnostic-info clojure.core/fn
-           metabase.util.files/with-open-path-to-resource clojure.core/let
-           metabase.util.ssh/with-ssh-tunnel clojure.core/let
-           metabase.db.liquibase/with-liquibase clojure.core/let
-           metabase.models.setting.multi-setting/define-multi-setting clojure.core/def
-           metabase.integrations.ldap/with-ldap-connection clojure.core/fn
-           metabase.test/are+ clojure.test/are
-           metabase.test/defdataset clojure.core/def
-           metabase.test/with-temp-file clojure.core/let
-           metabase.test/with-open-channels clojure.core/let
-           metabase.test/with-user-in-groups clojure.core/let
-           metabase.test.data.interface/defdataset clojure.core/def
-           metabase.test.data.interface/defdataset-edn clojure.core/def
-           metabase-enterprise.serialization.test-util/with-random-dump-dir clojure.core/let
-           metabase.driver.mongo.util/with-mongo-connection clojure.core/let
-           metabase.driver.mongo.query-processor/mongo-let clojure.core/let
-           toucan.db/with-call-counting clojure.core/fn
 
-           potemkin.types/defprotocol+ clojure.core/defprotocol
-           potemkin/defprotocol+ clojure.core/defprotocol
-           potemkin.types/defrecord+ clojure.core/defrecord
-           potemkin/defrecord+ clojure.core/defrecord
-           potemkin.types/deftype+ clojure.core/deftype
-           potemkin/deftype+ clojure.core/deftype
-           clojurewerkz.quartzite.jobs/defjob clojure.core/defn
-           honeysql.util/defalias clojure.core/def
-           honeysql.helpers/defhelper clj-kondo.lint-as/def-catch-all
-           clojure.core.logic/defne clj-kondo.lint-as/def-catch-all
-           monger.operators/defoperator clojure.core/def}
+ :lint-as
+ {clojure.core.logic/defne                                                        clj-kondo.lint-as/def-catch-all
+  clojurewerkz.quartzite.jobs/defjob                                              clojure.core/defn
+  honeysql.helpers/defhelper                                                      clj-kondo.lint-as/def-catch-all
+  honeysql.util/defalias                                                          clojure.core/def
+  metabase-enterprise.serialization.test-util/with-random-dump-dir                clojure.core/let
+  metabase.api.common/let-404                                                     clojure.core/let
+  metabase.db.data-migrations/defmigration                                        clojure.core/def
+  metabase.db.liquibase/with-liquibase                                            clojure.core/let
+  metabase.driver.mongo.query-processor/mongo-let                                 clojure.core/let
+  metabase.driver.mongo.util/with-mongo-connection                                clojure.core/let
+  metabase.driver.sql-jdbc.connection/with-connection-spec-for-testing-connection clojure.core/let
+  metabase.driver.sql-jdbc.execute.diagnostic/capturing-diagnostic-info           clojure.core/fn
+  metabase.integrations.ldap/with-ldap-connection                                 clojure.core/fn
+  metabase.mbql.schema.macros/defclause                                           clj-kondo.lint-as/def-catch-all
+  metabase.models.setting.multi-setting/define-multi-setting                      clojure.core/def
+  metabase.models.setting/defsetting                                              clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/defenterprise                         clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/defenterprise-schema                  clj-kondo.lint-as/def-catch-all
+  metabase.public-settings.premium-features/define-premium-feature                clojure.core/def
+  metabase.query-processor.error-type/deferror                                    clojure.core/def
+  metabase.sync.util/sum-for                                                      clojure.core/for
+  metabase.sync.util/with-emoji-progress-bar                                      clojure.core/let
+  metabase.test.data.interface/defdataset                                         clojure.core/def
+  metabase.test.data.interface/defdataset-edn                                     clojure.core/def
+  metabase.test/are+                                                              clojure.test/are
+  metabase.test/defdataset                                                        clojure.core/def
+  metabase.test/with-open-channels                                                clojure.core/let
+  metabase.test/with-temp-file                                                    clojure.core/let
+  metabase.test/with-user-in-groups                                               clojure.core/let
+  metabase.util.files/with-open-path-to-resource                                  clojure.core/let
+  metabase.util.ssh/with-ssh-tunnel                                               clojure.core/let
+  monger.operators/defoperator                                                    clojure.core/def
+  potemkin.types/defprotocol+                                                     clojure.core/defprotocol
+  potemkin.types/defrecord+                                                       clojure.core/defrecord
+  potemkin.types/deftype+                                                         clojure.core/deftype
+  potemkin/defprotocol+                                                           clojure.core/defprotocol
+  potemkin/defrecord+                                                             clojure.core/defrecord
+  potemkin/deftype+                                                               clojure.core/deftype
+  toucan.db/with-call-counting                                                    clojure.core/fn}
+
  :hooks
  {:analyze-call
   {metabase.api.common/defendpoint                            hooks.metabase.api.common/defendpoint

docs/developers-guide/driver-changelog.md

@@ -4,6 +4,14 @@ title: Driver interface changelog
 
 # Driver Interface Changelog
 
+## Metabase 0.45.0
+
+- `metabase.driver.sql-jdbc.connection/details->connection-spec-for-testing-connection` has been removed in Metabase
+  0.45.0, because it leaked SSH tunnels. See [#24445](https://github.com/metabase/metabase/issues/24445). If you are
+  using this function, please update your code to use
+  `metabase.driver.sql-jdbc.connection/with-connection-spec-for-testing-connection` instead, which properly cleans up
+  after itself.
+
 ## Metabase 0.43.0
 
 - The `:expressions` map in an MBQL query now uses strings as keys rather than keywords (see

modules/drivers/snowflake/src/metabase/driver/snowflake.clj

@@ -313,10 +313,10 @@
 (defmethod driver/can-connect? :snowflake
   [driver {:keys [db], :as details}]
   (and ((get-method driver/can-connect? :sql-jdbc) driver details)
-       (let [spec (sql-jdbc.conn/details->connection-spec-for-testing-connection driver details)
-             sql  (format "SHOW OBJECTS IN DATABASE \"%s\";" db)]
-         (jdbc/query spec sql)
-         true)))
+       (sql-jdbc.conn/with-connection-spec-for-testing-connection [spec [driver details]]
+         (let [sql (format "SHOW OBJECTS IN DATABASE \"%s\";" db)]
+           (jdbc/query spec sql)
+           true))))
 
 (defmethod driver/normalize-db-details :snowflake
   [_ database]

src/metabase/driver.clj

@@ -670,7 +670,11 @@
 (defmulti incorporate-ssh-tunnel-details
   "A multimethod for driver-specific behavior required to incorporate details for an opened SSH tunnel into the DB
   details. In most cases, this will simply involve updating the :host and :port (to point to the tunnel entry point,
-  instead of the backing database server), but some drivers may have more specific behavior."
+  instead of the backing database server), but some drivers may have more specific behavior.
+
+  WARNING! Implementations of this method may create new SSH tunnels, which need to be cleaned up. DO NOT USE THIS
+  METHOD DIRECTLY UNLESS YOU ARE GOING TO BE CLEANING UP ANY CREATED TUNNELS! Instead, you probably want to
+  use [[metabase.util.ssh/with-ssh-tunnel]]. See #24445 for more information."
   {:added "0.39.0" :arglists '([driver db-details])}
   dispatch-on-uninitialized-driver
   :hierarchy #'hierarchy)

src/metabase/driver/mysql.clj

@@ -69,19 +69,19 @@
      (if (mariadb? metadata) min-supported-mariadb-version min-supported-mysql-version)))
 
 (defn- warn-on-unsupported-versions [driver details]
-  (let [jdbc-spec (sql-jdbc.conn/details->connection-spec-for-testing-connection driver details)]
+  (sql-jdbc.conn/with-connection-spec-for-testing-connection [jdbc-spec [driver details]]
     (jdbc/with-db-metadata [metadata jdbc-spec]
       (when (unsupported-version? metadata)
         (log/warn
          (u/format-color 'red
-             (str
-              "\n\n********************************************************************************\n"
-              (trs "WARNING: Metabase only officially supports MySQL {0}/MariaDB {1} and above."
-                   min-supported-mysql-version
-                   min-supported-mariadb-version)
-              "\n"
-              (trs "All Metabase features may not work properly when using an unsupported version.")
-              "\n********************************************************************************\n")))))))
+                         (str
+                          "\n\n********************************************************************************\n"
+                          (trs "WARNING: Metabase only officially supports MySQL {0}/MariaDB {1} and above."
+                               min-supported-mysql-version
+                               min-supported-mariadb-version)
+                          "\n"
+                          (trs "All Metabase features may not work properly when using an unsupported version.")
+                          "\n********************************************************************************\n")))))))
 
 (defmethod driver/can-connect? :mysql
   [driver details]

src/metabase/driver/sql_jdbc/connection.clj

@@ -249,24 +249,34 @@
 ;;; |                                             metabase.driver impls                                              |
 ;;; +----------------------------------------------------------------------------------------------------------------+
 
-(defn details->connection-spec-for-testing-connection
-  "Return an appropriate JDBC connection spec to test whether a set of connection details is valid (i.e., implementing
-  `can-connect?`)."
-  [driver details]
-  (let [details-with-tunnel (driver/incorporate-ssh-tunnel-details
-                             driver
-                             (update details :port #(or % (default-ssh-tunnel-target-port driver))))]
-    (connection-details->spec driver details-with-tunnel)))
+(defn do-with-connection-spec-for-testing-connection
+  "Impl for [[with-connection-spec-for-testing-connection]]."
+  [driver details f]
+  (let [details (update details :port #(or % (default-ssh-tunnel-target-port driver)))]
+    (ssh/with-ssh-tunnel [details-with-tunnel details]
+      (let [spec (connection-details->spec driver details-with-tunnel)]
+        (f spec)))))
+
+(defmacro with-connection-spec-for-testing-connection
+  "Execute `body` with an appropriate [[clojure.java.jdbc]] connection spec based on connection `details`. Handles SSH
+  tunneling as needed and properly cleans up after itself.
+
+    (with-connection-spec-for-testing-connection [jdbc-spec [:my-driver conn-details]]
+      (do-something-with-spec jdbc-spec)"
+  {:added "0.45.0", :style/indent 1}
+  [[jdbc-spec-binding [driver details]] & body]
+  `(do-with-connection-spec-for-testing-connection ~driver ~details (^:once fn* [~jdbc-spec-binding] ~@body)))
 
 (defn can-connect-with-spec?
-  "Can we connect to a JDBC database with `clojure.java.jdbc` `jdbc-spec` and run a simple query?"
+  "Can we connect to a JDBC database with [[clojure.java.jdbc]] `jdbc-spec` and run a simple query?"
   [jdbc-spec]
   (let [[first-row] (jdbc/query jdbc-spec ["SELECT 1"])
         [result]    (vals first-row)]
-    (= 1 result)))
+    (= result 1)))
 
 (defn can-connect?
-  "Default implementation of `driver/can-connect?` for SQL JDBC drivers. Checks whether we can perform a simple `SELECT
-  1` query."
+  "Default implementation of [[driver/can-connect?]] for SQL JDBC drivers. Checks whether we can perform a simple
+  `SELECT 1` query."
   [driver details]
-  (can-connect-with-spec? (details->connection-spec-for-testing-connection driver details)))
+  (with-connection-spec-for-testing-connection [jdbc-spec [driver details]]
+    (can-connect-with-spec? jdbc-spec)))

src/metabase/util/ssh.clj

 (ns metabase.util.ssh
+  "SSH tunnel support for JDBC-based DWs. TODO -- it seems like this code is JDBC-specific, or at least big parts of
+  this all. We should consider moving some or all of this code to a new namespace like
+  `metabase.driver.sql-jdbc.connection.ssh-tunnel` or something like that."
   (:require [clojure.tools.logging :as log]
             [metabase.driver :as driver]
             [metabase.public-settings :as public-settings]
@@ -45,9 +48,9 @@
           keypair           (GenericUtils/head ids)]
       (.addPublicKeyIdentity session keypair))))
 
-(defn start-ssh-tunnel!
+(defn- start-ssh-tunnel!
   "Opens a new ssh tunnel and returns the connection along with the dynamically assigned tunnel entrance port. It's the
-  callers responsibility to call `close-tunnel` on the returned connection object."
+  callers responsibility to call [[close-tunnel!]] on the returned connection object."
   [{:keys [^String tunnel-host ^Integer tunnel-port ^String tunnel-user tunnel-pass tunnel-private-key
            tunnel-private-key-passphrase host port]}]
   {:pre [(integer? port)]}
@@ -102,8 +105,9 @@
       details-with-tunnel)
     details))
 
+;; TODO Seems like this definitely belongs in [[metabase.driver.sql-jdbc.connection]] or something like that.
 (defmethod driver/incorporate-ssh-tunnel-details :sql-jdbc
-  [_ db-details]
+  [_driver db-details]
   (cond (not (use-ssh-tunnel? db-details))
         ;; no ssh tunnel in use
         db-details
@@ -118,6 +122,7 @@
   "Close a running tunnel session"
   [details]
   (when (and (use-ssh-tunnel? details) (ssh-tunnel-open? details))
+    (log/tracef "Closing SSH tunnel: %s" (:tunnel-session details))
     (.close ^ClientSession (:tunnel-session details))))
 
 (defn do-with-ssh-tunnel
@@ -133,10 +138,12 @@
           (log/trace (u/format-color 'cyan "<< CLOSED SSH TUNNEL >>")))))
     (f details)))
 
+;;; TODO -- I think `with-ssh-tunnel-details` or something like that would be a better name for this. Since it doesn't
+;;; actually give you a tunnel. It just gives you connection details that include a tunnel in there.
 (defmacro with-ssh-tunnel
   "Starts an ssh tunnel, and binds the supplied name to a database
   details map with it's values adjusted to use the tunnel"
-  [[name details] & body]
+  [[details-binding details] & body]
   `(do-with-ssh-tunnel ~details
-     (fn [~name]
-       ~@body)))
+                       (fn [~details-binding]
+                         ~@body)))

test/metabase/util/ssh_test.clj

@@ -119,7 +119,7 @@
 
 ;; correct password
 (deftest connects-with-correct-password
-  (ssh/start-ssh-tunnel!
+  (#'ssh/start-ssh-tunnel!
    {:tunnel-user ssh-username
     :tunnel-host "127.0.0.1"
     :tunnel-port ssh-mock-server-with-password-port
@@ -131,7 +131,7 @@
 (deftest throws-exception-on-incorrect-password
   (is (thrown?
        org.apache.sshd.common.SshException
-       (ssh/start-ssh-tunnel!
+       (#'ssh/start-ssh-tunnel!
         {:tunnel-user ssh-username
          :tunnel-host "127.0.0.1"
          :tunnel-port ssh-mock-server-with-password-port
@@ -142,7 +142,7 @@
 ;; correct ssh key
 (deftest connects-with-correct-ssh-key
   (is (some?
-       (ssh/start-ssh-tunnel!
+       (#'ssh/start-ssh-tunnel!
         {:tunnel-user        ssh-username
          :tunnel-host        "127.0.0.1"
          :tunnel-port        ssh-mock-server-with-publickey-port
@@ -154,7 +154,7 @@
 (deftest throws-exception-on-incorrect-ssh-key
   (is (thrown?
        org.apache.sshd.common.SshException
-       (ssh/start-ssh-tunnel!
+       (#'ssh/start-ssh-tunnel!
         {:tunnel-user        ssh-username
          :tunnel-host        "127.0.0.1"
          :tunnel-port        ssh-mock-server-with-publickey-port
@@ -165,7 +165,7 @@
 ;; correct ssh key
 (deftest connects-with-correct-ssh-key-and-passphrase
   (is (some?
-       (ssh/start-ssh-tunnel!
+       (#'ssh/start-ssh-tunnel!
         {:tunnel-user                   ssh-username
          :tunnel-host                   "127.0.0.1"
          :tunnel-port                   ssh-mock-server-with-publickey-passphrase-port
@@ -177,7 +177,7 @@
 (deftest throws-exception-on-incorrect-ssh-key-and-passphrase
   (is (thrown?
        java.io.StreamCorruptedException
-       (ssh/start-ssh-tunnel!
+       (#'ssh/start-ssh-tunnel!
         {:tunnel-user                   ssh-username
          :tunnel-host                   "127.0.0.1"
          :tunnel-port                   ssh-mock-server-with-publickey-passphrase-port