diff --git a/.github/scripts/README.md b/.github/scripts/README.md
new file mode 100644
index 0000000000000000000000000000000000000000..faaf8e036f0367b636aab69540391899acda9e5e
--- /dev/null
+++ b/.github/scripts/README.md
@@ -0,0 +1,3 @@
+# GitHub Action Test Scripts
+
+Scripts related to running integration tests (ex: through GitHub actions)
diff --git a/.github/scripts/run-presto-kerberos-integration-test.sh b/.github/scripts/run-presto-kerberos-integration-test.sh
new file mode 100755
index 0000000000000000000000000000000000000000..7e2aac1e7c3e0d5ff0515ea3773372a36996d523
--- /dev/null
+++ b/.github/scripts/run-presto-kerberos-integration-test.sh
@@ -0,0 +1,68 @@
+#! /usr/bin/env bash
+# runs one or more Metabase test(s) against a Kerberized Presto instance
+set -eo pipefail
+
+# Need Java commands on $PATH, which apparently is not yet the case
+export PATH="$PATH:$JAVA_HOME/bin"
+
+# ensure java commmand is available
+which java
+
+# install clojure version needed for Metabase
+curl -O https://download.clojure.org/install/linux-install-1.10.3.933.sh
+chmod +x linux-install-1.10.3.933.sh
+./linux-install-1.10.3.933.sh
+
+RESOURCES_DIR=/app/source/resources
+
+# ensure the expected files are in place, in the resources dir
+if [ ! -f "$RESOURCES_DIR/ssl_keystore.jks" ]; then
+ echo "$RESOURCES_DIR/ssl_keystore.jks does not exist; cannot run test" >&2
+ exit 11
+fi
+
+if [ ! -f "$RESOURCES_DIR/krb5.conf" ]; then
+ echo "$RESOURCES_DIR/krb5.conf does not exist; cannot run test" >&2
+ exit 12
+fi
+
+if [ ! -f "$RESOURCES_DIR/client.keytab" ]; then
+ echo "$RESOURCES_DIR/client.keytab does not exist; cannot run test" >&2
+ exit 13
+fi
+
+# Copy the JDK cacerts file to our resources
+cp $JAVA_HOME/lib/security/cacerts $RESOURCES_DIR/cacerts-with-presto-ca.jks
+
+# Capture the Presto server self signed CA in PEM format
+openssl s_client -showcerts -connect presto-kerberos:7778 </dev/null \
+ | openssl x509 -outform PEM >$RESOURCES_DIR/presto-ssl-root-ca.pem
+
+# Convert the Presto server self signed CA to DER format
+openssl x509 -outform der -in $RESOURCES_DIR/presto-ssl-root-ca.pem -out $RESOURCES_DIR/presto-ssl-root-ca.der
+
+# Add Presto's self signed CA to the truststore
+keytool -noprompt -import -alias presto-kerberos -keystore $RESOURCES_DIR/cacerts-with-presto-ca.jks \
+ -storepass changeit -file $RESOURCES_DIR/presto-ssl-root-ca.der -trustcacerts
+
+ADDITIONAL_OPTS="SSLKeyStorePath=$RESOURCES_DIR/ssl_keystore.jks&SSLKeyStorePassword=presto\
+&SSLTrustStorePath=$RESOURCES_DIR/cacerts-with-presto-ca.jks&SSLTrustStorePassword=changeit"
+
+# Prepare dependencies
+source "./bin/prep.sh"
+prep_deps
+
+# Set up the environment variables pointing to all of this, and run some tests
+DRIVERS=presto-jdbc \
+MB_ENABLE_PRESTO_JDBC_DRIVER=true \
+MB_PRESTO_JDBC_TEST_HOST=presto-kerberos \
+MB_PRESTO_JDBC_TEST_PORT=7778 \
+MB_PRESTO_JDBC_TEST_SSL=true \
+MB_PRESTO_JDBC_TEST_KERBEROS=true \
+MB_PRESTO_JDBC_TEST_USER=bob@EXAMPLE.COM \
+MB_PRESTO_JDBC_TEST_KERBEROS_PRINCIPAL=bob@EXAMPLE.COM \
+MB_PRESTO_JDBC_TEST_KERBEROS_REMOTE_SERVICE_NAME=HTTP \
+MB_PRESTO_JDBC_TEST_KERBEROS_KEYTAB_PATH=$RESOURCES_DIR/client.keytab \
+MB_PRESTO_JDBC_TEST_KERBEROS_CONFIG_PATH=$RESOURCES_DIR/krb5.conf \
+MB_PRESTO_JDBC_TEST_ADDITIONAL_OPTIONS=$ADDITIONAL_OPTS \
+clojure -X:dev:test:drivers:drivers-dev :only metabase.driver.presto-jdbc-test
diff --git a/.github/workflows/presto-kerberos-integration-test.yml b/.github/workflows/presto-kerberos-integration-test.yml
new file mode 100644
index 0000000000000000000000000000000000000000..bbbbd6ebf7541ada5e276b9045f0c0639b2427cc
--- /dev/null
+++ b/.github/workflows/presto-kerberos-integration-test.yml
@@ -0,0 +1,70 @@
+name: Kerberized Presto Integration Test
+
+on:
+ pull_request:
+ push:
+ branches:
+ - master
+ - 'release**'
+ - 'feature**'
+ tags:
+ - '**'
+ paths:
+ - '**/presto_jdbc/**'
+ - '**/presto_jdbc.clj'
+
+jobs:
+ run-presto-kerberos-test:
+ runs-on: ubuntu-20.04
+ timeout-minutes: 40
+ steps:
+ - name: Install babashka
+ run: >
+ mkdir -p /tmp/babashka-install \
+ && cd /tmp/babashka-install \
+ && curl -sLO https://raw.githubusercontent.com/babashka/babashka/master/install \
+ && chmod +x install \
+ && sudo ./install \
+ && cd -
+ - name: Checkout Metabase repository
+ uses: actions/checkout@v2
+ with:
+ token: ${{ secrets.GITHUB_TOKEN }}
+ - name: Check out Presto Kerberos Docker Compose
+ uses: actions/checkout@v2
+ with:
+ repository: metabase/presto-kerberos-docker
+ ref: add-test_data-catalog
+ token: ${{ secrets.GITHUB_TOKEN }}
+ path: presto-kerberos-docker
+ - name: Bring up Presto+Kerberos cluster
+ run: cd presto-kerberos-docker && docker-compose up -d && cd ..
+ - name: Run Presto test query from command line (sanity check)
+ run: cd presto-kerberos-docker && ./test.sh && cd ..
+ # Since we are managing the Docker containers from the GitHub action container, we need to copy all the
+ # relevant resources now, into the resources dir for later consumption by the app
+ - name: Copy Presto SSL keystore to resources
+ run: docker cp presto-kerberos:/tmp/ssl_keystore.jks resources
+ - name: Copy krb5.conf file to resources
+ run: docker cp presto-kerberos:/etc/krb5.conf resources
+ - name: Copy client.keytab file to resources
+ run: docker cp presto-kerberos:/home/presto/client.keytab resources
+ - name: Checkout mba
+ uses: actions/checkout@v2
+ with:
+ repository: metabase/mba
+ ref: master
+ token: ${{ secrets.GITHUB_TOKEN }}
+ path: mba-src
+ - name: ls mba
+ run: ls -latr mba-src
+ - name: Symlink mba
+ run: cd mba-src && sudo ln -s $(pwd)/src/main.clj /usr/local/bin/mba && chmod +x /usr/local/bin/mba && cd ..
+ - name: Ensure mba
+ run: which mba
+ - name: Run Metabase via MBA
+ run: /home/runner/work/metabase/metabase/mba-src/src/main.clj --mb . --data-db postgres-data -n example.com up
+ - name: Run test script in MBA instance
+ run: >
+ mba --mb . --data-db postgres-data -n example.com \
+ run .github/scripts/run-presto-kerberos-integration-test.sh
diff --git a/modules/drivers/presto-jdbc/resources/metabase-plugin.yaml b/modules/drivers/presto-jdbc/resources/metabase-plugin.yaml
index 17a88566eb3664a88add47d9854741b0c3604784..64f2ef1c1ce89689bb675e963bea28665affef14 100644
--- a/modules/drivers/presto-jdbc/resources/metabase-plugin.yaml
+++ b/modules/drivers/presto-jdbc/resources/metabase-plugin.yaml
@@ -23,12 +23,53 @@ driver:
- name: schema
display-name: Schema (optional)
required: false
- - user
- - password
+ - merge:
+ - user
+ - required: false
+ - merge:
+ - password
+ - required: false
- ssl
- merge:
- additional-options
- placeholder: "trustServerCertificate=false"
+ - name: kerberos
+ type: boolean
+ display-name: Authenticate with Kerberos?
+ default: false
+ - name: kerberos-principal
+ display-name: Kerberos principal
+ placeholder: service/instance@REALM
+ required: false
+ - name: kerberos-remote-service-name
+ display-name: Kerberos coordinator service
+ placeholder: presto
+ required: false
+ - name: kerberos-use-canonical-hostname
+ type: boolean
+ display-name: Use canonical hostname
+ default: false
+ required: false
+ - name: kerberos-credential-cache-path
+ display-name: Kerberos credential cache file
+ placeholder: /tmp/kerberos-credential-cache
+ required: false
+ - name: kerberos-keytab-path
+ display-name: Kerberos keytab file
+ placeholder: /path/to/kerberos.keytab
+ required: false
+ - name: kerberos-config-path
+ display-name: Kerberos configuration file
+ placeholder: /etc/krb5.conf
+ required: false
+ - name: kerberos-service-principal-pattern
+ display-name: Presto coordinator Kerberos service principal pattern
+ placeholder: ${SERVICE}@${HOST}. ${SERVICE}
+ required: false
+ - name: additional-options
+ display-name: Additional JDBC options
+ placeholder: SSLKeyStorePath=/path/to/keystore.jks&SSLKeyStorePassword=whatever
+ required: false
init:
- step: load-namespace
namespace: metabase.driver.presto-common
diff --git a/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj b/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
index 6e839fbc128143a7bcf9a821e29fb7edde846b49..98e8dcb18f9ebab1845171a9483a3ced5df62cdd 100644
--- a/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
+++ b/modules/drivers/presto-jdbc/src/metabase/driver/presto_jdbc.clj
@@ -189,6 +189,39 @@
;;; | Connectivity |
;;; +----------------------------------------------------------------------------------------------------------------+
+;;; Kerberos related definitions
+(def ^:private ^:const kerb-props->url-param-names
+ {:kerberos-principal "KerberosPrincipal"
+ :kerberos-remote-service-name "KerberosRemoteServiceName"
+ :kerberos-use-canonical-hostname "KerberosUseCanonicalHostname"
+ :kerberos-credential-cache-path "KerberosCredentialCachePath"
+ :kerberos-keytab-path "KerberosKeytabPath"
+ :kerberos-service-principal-pattern "KerberosServicePrincipalPattern"
+ :kerberos-config-path "KerberosConfigPath"})
+
+(defn- details->kerberos-url-params [details]
+ (let [remove-blank-vals (fn [m] (into {} (remove (comp str/blank? val) m)))
+ ks (keys kerb-props->url-param-names)]
+ (-> (select-keys details ks)
+ remove-blank-vals
+ (set/rename-keys kerb-props->url-param-names))))
+
+(defn- prepare-addl-opts [{:keys [SSL kerberos additional-options] :as details}]
+ (let [det (if kerberos
+ (if-not SSL
+ (throw (ex-info (trs "SSL must be enabled to use Kerberos authentication")
+ {:db-details details}))
+ (update details
+ :additional-options
+ str
+ ;; add separator if there are already additional-options
+ (when-not (str/blank? additional-options) "&")
+ ;; convert Kerberos options map to URL string
+ (sql-jdbc.common/additional-opts->string :url (details->kerberos-url-params details))))
+ details)]
+ ;; in any case, remove the standalone Kerberos properties from details map
+ (apply dissoc (cons det (keys kerb-props->url-param-names)))))
+
(defn- db-name
"Creates a \"DB name\" for the given catalog `c` and (optional) schema `s`. If both are specified, a slash is
used to separate them. See examples at:
@@ -208,33 +241,43 @@
"Creates a spec for `clojure.java.jdbc` to use for connecting to Presto via JDBC, from the given `opts`."
[{:keys [host port catalog schema]
:or {host "localhost", port 5432, catalog ""}
- :as opts}]
- (-> (merge
- {:classname "com.facebook.presto.jdbc.PrestoDriver"
- :subprotocol "presto"
- :subname (db.spec/make-subname host port (db-name catalog schema))}
- (dissoc opts :host :port :db :catalog :schema))
+ :as details}]
+ (-> details
+ (merge {:classname "com.facebook.presto.jdbc.PrestoDriver"
+ :subprotocol "presto"
+ :subname (db.spec/make-subname host port (db-name catalog schema))})
+ prepare-addl-opts
+ (dissoc :host :port :db :catalog :schema :tunnel-enabled :engine :kerberos)
sql-jdbc.common/handle-additional-options))
+(defn- str->bool [v]
+ (if (string? v)
+ (Boolean/parseBoolean v)
+ v))
+
(defmethod sql-jdbc.conn/connection-details->spec :presto-jdbc
- [_ {ssl? :ssl, :as details-map}]
+ [_ details-map]
(let [props (-> details-map
(update :port (fn [port]
(if (string? port)
(Integer/parseInt port)
port)))
- (assoc :SSL ssl?)
+ (update :ssl str->bool)
+ (update :kerberos str->bool)
+ (assoc :SSL (:ssl details-map))
;; remove any Metabase specific properties that are not recognized by the PrestoDB JDBC driver, which is
;; very picky about properties (throwing an error if any are unrecognized)
;; all valid properties can be found in the JDBC Driver source here:
;; https://github.com/prestodb/presto/blob/master/presto-jdbc/src/main/java/com/facebook/presto/jdbc/ConnectionProperties.java
- (select-keys [:host :port :catalog :schema :additional-options ; needed for [jdbc-spec]
- ;; JDBC driver specific properties
- :user :password :socksProxy :httpProxy :applicationNamePrefix :disableCompression :SSL
- :SSLKeyStorePath :SSLKeyStorePassword :SSLTrustStorePath :SSLTrustStorePassword
- :KerberosRemoteServiceName :KerberosPrincipal :KerberosUseCanonicalHostname
- :KerberosConfigPath :KerberosKeytabPath :KerberosCredentialCachePath :accessToken
- :extraCredentials :sessionProperties :protocols :queryInterceptors]))]
+ (select-keys (concat
+ [:host :port :catalog :schema :additional-options ; needed for `jdbc-spec`
+ ;; JDBC driver specific properties
+ :kerberos ; we need our boolean property indicating if Kerberos is enabled
+ ; but the rest of them come from `kerb-props->url-param-names` (below)
+ :user :password :socksProxy :httpProxy :applicationNamePrefix :disableCompression :SSL
+ :SSLKeyStorePath :SSLKeyStorePassword :SSLTrustStorePath :SSLTrustStorePassword
+ :accessToken :extraCredentials :sessionProperties :protocols :queryInterceptors]
+ (keys kerb-props->url-param-names))))]
(jdbc-spec props)))
;;; +----------------------------------------------------------------------------------------------------------------+
diff --git a/modules/drivers/presto-jdbc/test/metabase/driver/presto_jdbc_test.clj b/modules/drivers/presto-jdbc/test/metabase/driver/presto_jdbc_test.clj
index c66b9d0b702ef946310295dbfd6b6f8b7513f334..0f74743fcdf11ae700f2f615f5034d99840fad5b 100644
--- a/modules/drivers/presto-jdbc/test/metabase/driver/presto_jdbc_test.clj
+++ b/modules/drivers/presto-jdbc/test/metabase/driver/presto_jdbc_test.clj
@@ -200,3 +200,20 @@
;; JSON blob)
(let [db-details (assoc (:details (mt/db)) :let-user-control-scheduling false)]
(is (nil? (database-api/test-database-connection :presto-jdbc db-details)))))))
+
+(deftest kerberos-properties-test
+ (testing "Kerberos related properties are set correctly"
+ (let [details {:host "presto-server"
+ :port 7778
+ :catalog "my-catalog"
+ :kerberos true
+ :ssl true
+ :kerberos-config-path "/path/to/krb5.conf"
+ :kerberos-principal "alice@DOMAIN.COM"
+ :kerberos-remote-service-name "HTTP"
+ :kerberos-keytab-path "/path/to/client.keytab"}
+ jdbc-spec (sql-jdbc.conn/connection-details->spec :presto-jdbc details)]
+ (is (= (str "//presto-server:7778/my-catalog?KerberosPrincipal=alice@DOMAIN.COM"
+ "&KerberosRemoteServiceName=HTTP&KerberosKeytabPath=/path/to/client.keytab"
+ "&KerberosConfigPath=/path/to/krb5.conf")
+ (:subname jdbc-spec))))))
diff --git a/modules/drivers/presto-jdbc/test/metabase/test/data/presto_jdbc.clj b/modules/drivers/presto-jdbc/test/metabase/test/data/presto_jdbc.clj
index 421c2be9d9c49baa0af98b25e7a20ffffe874c27..bcc706263354ecab59bdc6fd2eebc9524cff5a39 100644
--- a/modules/drivers/presto-jdbc/test/metabase/test/data/presto_jdbc.clj
+++ b/modules/drivers/presto-jdbc/test/metabase/test/data/presto_jdbc.clj
@@ -38,13 +38,21 @@
(defmethod tx/dbdef->connection-details :presto-jdbc
[_ _ {:keys [database-name]}]
- {:host (tx/db-test-env-var-or-throw :presto-jdbc :host "localhost")
- :port (tx/db-test-env-var :presto-jdbc :port "8080")
- :user (tx/db-test-env-var-or-throw :presto-jdbc :user "metabase")
- :additional-options (tx/db-test-env-var :presto-jdbc :additional-options nil)
- :ssl (tx/db-test-env-var :presto-jdbc :ssl "false")
- :catalog (dash->underscore database-name)
- :schema (tx/db-test-env-var :presto-jdbc :schema nil)})
+ {:host (tx/db-test-env-var-or-throw :presto-jdbc :host "localhost")
+ :port (tx/db-test-env-var :presto-jdbc :port "8080")
+ :user (tx/db-test-env-var-or-throw :presto-jdbc :user "metabase")
+ :additional-options (tx/db-test-env-var :presto-jdbc :additional-options nil)
+ :ssl (tx/db-test-env-var :presto-jdbc :ssl "false")
+ :kerberos (tx/db-test-env-var :presto-jdbc :kerberos "false")
+ :kerberos-principal (tx/db-test-env-var :presto-jdbc :kerberos-principal nil)
+ :kerberos-remote-service-name (tx/db-test-env-var :presto-jdbc :kerberos-remote-service-name nil)
+ :kerberos-use-canonical-hostname (tx/db-test-env-var :presto-jdbc :kerberos-use-canonical-hostname nil)
+ :kerberos-credential-cache-path (tx/db-test-env-var :presto-jdbc :kerberos-credential-cache-path nil)
+ :kerberos-keytab-path (tx/db-test-env-var :presto-jdbc :kerberos-keytab-path nil)
+ :kerberos-config-path (tx/db-test-env-var :presto-jdbc :kerberos-config-path nil)
+ :kerberos-service-principal-pattern (tx/db-test-env-var :presto-jdbc :kerberos-service-principal-pattern nil)
+ :catalog (dash->underscore database-name)
+ :schema (tx/db-test-env-var :presto-jdbc :schema nil)})
(defmethod execute/execute-sql! :presto-jdbc
[& args]
diff --git a/src/metabase/driver/sql_jdbc/common.clj b/src/metabase/driver/sql_jdbc/common.clj
index e306fa06062c73364fe477af164912256bfc11df..205667b31dcb45a8a214351c9b967e04edcaa841 100644
--- a/src/metabase/driver/sql_jdbc/common.clj
+++ b/src/metabase/driver/sql_jdbc/common.clj
@@ -1,6 +1,41 @@
(ns metabase.driver.sql-jdbc.common
(:require [clojure.string :as str]))
+(def ^:private valid-separator-styles #{:url :comma :semicolon})
+
+(defn conn-str-with-additional-opts
+ "Adds `additional-opts` (a string) to the given `connection-string` based on the given `separator-style`. See
+ documentation for `handle-additional-options` for further details."
+ {:added "0.41.0", :arglists '([connection-string separator-style additional-opts])}
+ [connection-string separator-style additional-opts]
+ {:pre [(string? connection-string)
+ (or (nil? additional-opts) (string? additional-opts))
+ (contains? valid-separator-styles separator-style)]}
+ (str connection-string (when-not (str/blank? additional-opts)
+ (str (case separator-style
+ :comma ","
+ :semicolon ";"
+ :url (if (str/includes? connection-string "?")
+ "&"
+ "?"))
+ additional-opts))))
+
+(defn additional-opts->string
+ "Turns a map of `additional-opts` into a single string, based on the `separator-style`."
+ {:added "0.41.0"}
+ [separator-style additional-opts]
+ {:pre [(or (nil? additional-opts) (map? additional-opts)) (contains? valid-separator-styles separator-style)]}
+ (when (some? additional-opts)
+ (reduce-kv (fn [m k v]
+ (str m
+ (when (seq m)
+ (case separator-style :comma "," :semicolon ";" :url "&"))
+ (if (keyword? k)
+ (name k)
+ (str k))
+ "="
+ v)) "" additional-opts)))
+
(defn handle-additional-options
"If DETAILS contains an `:addtional-options` key, append those options to the connection string in CONNECTION-SPEC.
(Some drivers like MySQL provide this details field to allow special behavior where needed).
@@ -16,11 +51,4 @@
([{connection-string :subname, :as connection-spec} {additional-options :additional-options, :as details} & {:keys [seperator-style]
:or {seperator-style :url}}]
(-> (dissoc connection-spec :additional-options)
- (assoc :subname (str connection-string (when (seq additional-options)
- (str (case seperator-style
- :comma ","
- :semicolon ";"
- :url (if (str/includes? connection-string "?")
- "&"
- "?"))
- additional-options)))))))
+ (assoc :subname (conn-str-with-additional-opts connection-string seperator-style additional-options)))))
diff --git a/test/metabase/driver/sql_jdbc/common_test.clj b/test/metabase/driver/sql_jdbc/common_test.clj
new file mode 100644
index 0000000000000000000000000000000000000000..abebee5df1aab6057ff56831138f84dd7dc4219c
--- /dev/null
+++ b/test/metabase/driver/sql_jdbc/common_test.clj
@@ -0,0 +1,20 @@
+(ns metabase.driver.sql-jdbc.common-test
+ (:require [clojure.test :as t]
+ [metabase.driver.sql-jdbc.common :as sql-jdbc.common]))
+
+(t/deftest conn-str-with-additional-opts-testc
+ (t/testing "conn-str-with-additional-opts combined with additional-opts->string works as expected"
+ (doseq [[exp-str sep-style conn-str opts] [["localhost:4321" :comma "localhost:4321" nil]
+ ["localhost:4321" :comma "localhost:4321" {}]
+ ["localhost:4321,a=1" :comma "localhost:4321" {:a 1}]
+ ["localhost:4321,a=1,b=2" :comma "localhost:4321" {:a 1 :b 2}]
+ ["localhost:4321" :semicolon "localhost:4321" nil]
+ ["localhost:4321" :semicolon "localhost:4321" {}]
+ ["localhost:4321;a=1" :semicolon "localhost:4321" {:a 1}]
+ ["localhost:4321;a=1;b=2" :semicolon "localhost:4321" {:a 1 :b 2}]
+ ["localhost:4321" :url "localhost:4321" nil]
+ ["localhost:4321" :url "localhost:4321" {}]
+ ["localhost:4321?a=1" :url "localhost:4321" {:a 1}]
+ ["localhost:4321?a=1&b=2" :url "localhost:4321" {:a 1 :b 2}]]]
+ (let [opts-str (sql-jdbc.common/additional-opts->string sep-style opts)]
+ (t/is (= exp-str (sql-jdbc.common/conn-str-with-additional-opts conn-str sep-style opts-str)))))))