(TK-497) Allow requiring authorization via config setting

This commit addresses PDB-5261 which documents a transient bug that
allows the metrics endpoint to be accessed without authorization when
the trapperkeeper-metrics bootstrap config contains the authorization
service. The root of this problem is likely a race condition within
trapperkeeper.

This patch adds a trapperkeeper-metrics config option
metrics.metrics-webservice.jolokia.require-auth which defaults to true.
In order to use the trapperkeeper-metrics service without authorization,
this option must explicitly be set to false. When this option is set to
true and the authorization service is not loaded, the
IllegalArgumentException thrown by trapper-keeper's get-service method
is purposefully unhandled and crashes the program. Helpful log messages
are printed when this occurs.

Author: stelcodes

documentation/configuration.md

@@ -80,6 +80,10 @@ metrics: {
             # Default is true.
             enabled: false
 
+            # Enable or disable authorization for metrics endpoint
+            # Default is true.
+            require-auth: true
+
             # Configure any of the settings listed at:
             #   https://jolokia.org/reference/html/agents.html#war-agent-installation
             servlet-init-params: {

src/clj/puppetlabs/trapperkeeper/services/metrics/jolokia.clj

@@ -84,7 +84,10 @@
                        {:authorized true :message ""}))]
     (if auth-check-fn
       (log/info "Metrics access control using trapperkeeper-authorization is enabled.")
-      (log/warn "Metrics access control using trapperkeeper-authorization is disabled. Add the authorization service to the trapperkeeper bootstrap configuration file to enable it."))
+      (log/warn (str "Metrics access control using trapperkeeper-authorization is disabled. "
+                     "To enable it, add the authorization service to the trapperkeeper bootstrap "
+                     "configuration file and set the trapperkeeper-metrics config setting "
+                     "metrics.metrics-webservice.jolokia.require-auth to true (default).")))
     (proxy [AgentServlet] []
      ;; NOTE: An alternative to this method override would be to use defrecord
      ;; to create a class that can be set as `:logHandlerClass` in the servlet

src/clj/puppetlabs/trapperkeeper/services/metrics/metrics_core.clj

@@ -27,6 +27,7 @@
 
 (def JolokiaApiConfig
   {(schema/optional-key :enabled) schema/Bool
+   (schema/optional-key :require-auth) schema/Bool
    (schema/optional-key :servlet-init-params) jolokia/JolokiaConfig})
 
 (def MbeansApiConfig

src/clj/puppetlabs/trapperkeeper/services/metrics/metrics_service.clj

@@ -81,12 +81,21 @@
             options (if (nil? server)
                       {:servlet-init-params config}
                       {:servlet-init-params config :server-id (keyword server)})
-            auth-service (tk-services/maybe-get-service this :AuthorizationService)
-            auth-check-fn (if auth-service (partial tk-auth/authorization-check auth-service))]
-        (add-servlet-handler
-         (jolokia/create-servlet auth-check-fn)
-         route
-         options)))
+            require-auth? (get-in-config [:metrics :metrics-webservice :jolokia :require-auth] true)]
+        (if-not require-auth?
+          (add-servlet-handler (jolokia/create-servlet nil) route options)
+          (try
+            (let [auth-service (tk-services/get-service this :AuthorizationService)
+                  auth-check-fn (partial tk-auth/authorization-check auth-service)
+                  servlet (jolokia/create-servlet auth-check-fn)]
+              (add-servlet-handler servlet route options))
+            ;; Fail fast when auth is required but the auth service could not load
+            (catch IllegalArgumentException e
+              (log/error "metrics.metrics-webservice.jolokia.require-auth set to true (default)"
+                         "but trapperkeeper-authorization service was not found. To disable"
+                         "authorization change this setting to false.")
+              (throw e))))))
+
 
     context)
 

test/puppetlabs/trapperkeeper/services/metrics/metrics_service_test.clj

@@ -63,6 +63,7 @@
 
 (def metrics-service-config
   {:metrics {:server-id "localhost"
+             :metrics-webservice {:jolokia {:require-auth false}}
              :registries {:pl.test.reg {:reporters {:jmx {:enabled true}}}
                           :pl.other.reg {:reporters {:jmx {:enabled true}}}}}
    :webserver {:port 8180
@@ -226,15 +227,31 @@
             (is (= 403 (get body "status")))))))))
 
 (deftest metrics-service-with-tk-auth
-  (testing "tk-auth works when included in bootstrap"
-    (with-app-with-config
-      app
-      (conj services authorization-service/authorization-service)
-      (merge metrics-service-config auth-config ssl-webserver-config)
-      (let [resp (http-client/get "https://localhost:8180/metrics/v2/list" ssl-opts)]
-        (is (= 200 (:status resp))))
-      (let [resp (http-client/get "https://localhost:8180/metrics/v2" ssl-opts)]
-        (is (= 403 (:status resp)))))))
+  (let [config-with-require-auth
+        (-> (merge metrics-service-config auth-config ssl-webserver-config)
+          (assoc-in [:metrics :metrics-webservice :jolokia :require-auth] true))]
+
+    (testing "tk-auth works when included in bootstrap"
+      (with-app-with-config
+        app
+        (conj services authorization-service/authorization-service)
+        ;; This test requires authentication so we need to override the require-auth
+        ;; option that is set to false in metrics-service-config
+        config-with-require-auth
+        (let [resp (http-client/get "https://localhost:8180/metrics/v2/list" ssl-opts)]
+          (is (= 200 (:status resp))))
+        (let [resp (http-client/get "https://localhost:8180/metrics/v2" ssl-opts)]
+          (is (= 403 (:status resp))))))
+
+    (testing "exception occurs when require-auth option is true but auth service was not found"
+      ;; This test is like the previous, except the auth service is not listed as a dependency
+      (is (thrown-with-msg? IllegalArgumentException
+                            #"service ':AuthorizationService' does not exist"
+                            (with-app-with-config
+                              app
+                              ;; The auth service is purposefully left out here
+                              services
+                              config-with-require-auth))))))
 
 (deftest metrics-v1-endpoint-disabled-by-default
   (testing "metrics/v1 is disabled by default, returns 404"