Add Kerberos support to Presto JDBC Driver (#16307)

* Add Kerberos support to Presto JDBC Driver

Adding Kerberos properties as DB details properties

Refactoring utility methods for dealing with additional options

Adding test for Kerb properties -> connection string

Adding GitHub action to run integration test

Start to move logic for capturing necessary files to new script, and calling that from mba run

Adjust settings in `metabase.test.data.presto-jdbc` to capture test parameters

.github/scripts/README.md

+# GitHub Action Test Scripts
+
+Scripts related to running integration tests (ex: through GitHub actions)

.github/scripts/run-presto-kerberos-integration-test.sh

+#! /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

.github/workflows/presto-kerberos-integration-test.yml

+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

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

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)))
 
 ;;; +----------------------------------------------------------------------------------------------------------------+

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))))))

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]

src/metabase/driver/sql_jdbc/common.clj

 (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)))))

test/metabase/driver/sql_jdbc/common_test.clj

+(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)))))))