From cc632e8193b460edfe13a758eb25d19e927c1c20 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 4 Aug 2022 10:53:33 -0700
Subject: [PATCH] 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 | 79 ++++++++++---------
docs/developers-guide/driver-changelog.md | 8 ++
.../src/metabase/driver/snowflake.clj | 8 +-
src/metabase/driver.clj | 6 +-
src/metabase/driver/mysql.clj | 18 ++---
src/metabase/driver/sql_jdbc/connection.clj | 36 ++++++---
src/metabase/util/ssh.clj | 19 +++--
test/metabase/util/ssh_test.clj | 12 +--
8 files changed, 109 insertions(+), 77 deletions(-)
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
index c28c693b4a2..9e86bf61672 100644
--- a/.clj-kondo/config.edn
+++ b/.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
diff --git a/docs/developers-guide/driver-changelog.md b/docs/developers-guide/driver-changelog.md
index 887f8053af8..c693e3eff27 100644
--- a/docs/developers-guide/driver-changelog.md
+++ b/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
diff --git a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj b/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
index 5b533ce6590..2798aa7c759 100644
--- a/modules/drivers/snowflake/src/metabase/driver/snowflake.clj
+++ b/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]
diff --git a/src/metabase/driver.clj b/src/metabase/driver.clj
index 0a059649b91..7b34e2789c0 100644
--- a/src/metabase/driver.clj
+++ b/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)
diff --git a/src/metabase/driver/mysql.clj b/src/metabase/driver/mysql.clj
index b1625da9f1b..2eb5a0b9349 100644
--- a/src/metabase/driver/mysql.clj
+++ b/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]
diff --git a/src/metabase/driver/sql_jdbc/connection.clj b/src/metabase/driver/sql_jdbc/connection.clj
index 09224fe7dc1..c81d478e847 100644
--- a/src/metabase/driver/sql_jdbc/connection.clj
+++ b/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)))
diff --git a/src/metabase/util/ssh.clj b/src/metabase/util/ssh.clj
index abb9ea49317..8fba4d37408 100644
--- a/src/metabase/util/ssh.clj
+++ b/src/metabase/util/ssh.clj
@@ -1,4 +1,7 @@
(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)))
diff --git a/test/metabase/util/ssh_test.clj b/test/metabase/util/ssh_test.clj
index 1bad54252be..02fefe5327e 100644
--- a/test/metabase/util/ssh_test.clj
+++ b/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
--
GitLab