(main) address linting issues, add additional logging

This addresses some of the clj-kondo linting issues found and adds logging for failure paths to help diagnose issues in the field.
puppetlabs · Aug 8, 2023 · 48e62e9 · 48e62e9
1 parent f7a8e77
commit 48e62e9
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 28 deletions.
diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn
@@ -1,2 +1,3 @@
 {:linters {:refer-all {:exclude [clojure.test slingshot.test]}}
- :output {:linter-name true}}
+ :output {:linter-name true}
+ :lint-as {slingshot.slingshot/try+ clojure.core/try}}
diff --git a/src/puppetlabs/trapperkeeper/authorization/acl.clj b/src/puppetlabs/trapperkeeper/authorization/acl.clj
@@ -1,5 +1,6 @@
 (ns puppetlabs.trapperkeeper.authorization.acl
   (:require [clojure.set :refer [intersection]]
+            [clojure.string :as string]
             [puppetlabs.i18n.core :refer [trs]]
             [puppetlabs.ssl-utils.core :refer [subject-alt-name-oid]]
             [slingshot.slingshot :refer [throw+]]
@@ -124,8 +125,8 @@
   matching."
   [domain :- schema/Str]
   (-> domain
-      (clojure.string/lower-case)
-      (clojure.string/split #"\.")
+      (string/lower-case)
+      (string/split #"\.")
       reverse))
 
 (schema/defn ^:always-validate new-domain :- ACE
@@ -184,7 +185,7 @@
     (re-matches #"^/.*/$" certname)
     {:auth-type auth-type
      :match :regex
-     :value (clojure.string/replace certname #"^/(.*)/$" "$1")}
+     :value (string/replace certname #"^/(.*)/$" "$1")}
 
     :else
     (throw+
@@ -206,7 +207,7 @@
   "substiture $1, $2... by the same index in the captures vector"
   [in :- schema/Str
    captures :- [schema/Str]]
-  (clojure.string/replace in #"\$(\d+)" #(nth captures (- (read-string (second %)) 1))))
+  (string/replace in #"\$(\d+)" #(nth captures (- (read-string (second %)) 1))))
 
 (schema/defn interpolate-backreference :- ACE
   "change all possible backreferences in ace patterns to values from the
@@ -215,7 +216,7 @@
    captures :- [schema/Str]]
   (if (= match :backreference)
     (new-domain auth-type
-                {:certname (clojure.string/join "." (map #(substitute-backreference % captures)
+                {:certname (string/join "." (map #(substitute-backreference % captures)
                                                          (reverse (ace :value))))})
     ace))
 
@@ -263,11 +264,10 @@
                           ;; potentially translate from oid -> shortname
                           k' (get oid-map' (name k) k)
                           ext-value (get extensions k' false)
-                          given-names-match? (fn [k] (not
-                                                      (empty?
+                          given-names-match? (fn [k] (seq
                                                        (intersection (set (get ext-value k))
                                                                      (set (wrap-scalar
-                                                                           (get ace-value k)))))))]
+                                                                           (get ace-value k))))))]
                       (if ext-value
                         (if (= :subject-alt-name k')
                           (reduce (fn [acc key] (or acc (given-names-match? key)))

diff --git a/src/puppetlabs/trapperkeeper/authorization/ring_middleware.clj b/src/puppetlabs/trapperkeeper/authorization/ring_middleware.clj
@@ -38,7 +38,7 @@
 (defn warn-if-header-value-non-nil
   "Log a warning message if the supplied header-val is non-empty."
   [header-name header-val]
-  (if header-val
+  (when header-val
     (log/warn (trs "The HTTP header {0} was specified with {1} but the allow-header-cert-info was either not set, or was set to false. This header will be ignored." header-name header-val))))
 
 (defn legacy-openssl-dn->cn
@@ -52,15 +52,16 @@
   nil."
   [dn]
   (when-not (nil? dn)
-    (some #(if (= "CN" (first %1)) (second %1))
+    (some #(when (= "CN" (first %1)) (second %1))
           (map #(str/split %1 #"=" 2) (str/split dn #"/")))))
 
 (defn warn-for-empty-common-name
   "Log a warning message if the supplied common-name is empty (nil or empty
   string."
   [common-name empty-message]
-  (when (empty? common-name)
-    (log/warn (trs "{0} Treating client as ''unauthenticated''." empty-message)))
+  (if (empty? common-name)
+    (log/warn (trs "{0} Treating client as ''unauthenticated''." empty-message))
+    (log/trace (trs "Common name is {0}" common-name)))
   common-name)
 
 (defn request->name*
@@ -73,12 +74,13 @@
     (not allow-header-cert-info)
       (do
         (warn-if-header-value-non-nil header-dn-name header-dn-val)
-        (if-let [certificate (:ssl-client-cert request)]
+        (when-let [certificate (:ssl-client-cert request)]
           (let [error-message (trs "CN could not be found in certificate DN")
                 cert-dn (-> certificate (.getSubjectDN) (.getName))]
             (warn-for-empty-common-name
              (ssl-utils/get-cn-from-x509-certificate certificate)
-             (format "%s: %s." error-message cert-dn)))))
+             (format "%s: %s." error-message cert-dn)))
+          (log/debug (i18n/trs "No certificate found in request for name resolution."))))
     (empty? header-dn-val)
       nil
     (ssl-utils/valid-x500-name? header-dn-val)
@@ -118,6 +120,7 @@
   [request name allow-header-cert-info]
   (let [header-client-verify-val (get-in request
                                          [:headers header-client-verify-name])]
+    (log/trace (i18n/trs "header-client-verify-val: " header-client-verify-val))
     (cond
       (not allow-header-cert-info)
         (do
@@ -127,9 +130,9 @@
       (= header-client-verify-val "SUCCESS") true
       :else
         (do
-          (if (not (empty? name))
+          (when (seq name)
             ; Translator note: {1} is the header name, {2} is the header value
-            (log/errorf (trs "Client with CN ''{0}'' was not verified by ''{1}'' header: ''{2}''"
+            (log/error (trs "Client with CN ''{0}'' was not verified by ''{1}'' header: ''{2}''"
                              name
                              header-client-verify-name
                              header-client-verify-val)))
@@ -158,7 +161,7 @@
   "Return an X509Certificate or nil from a string encoded for transmission
   in an HTTP header."
   [header-cert-val]
-  (if header-cert-val
+  (when header-cert-val
     (let [pem        (header-cert->pem header-cert-val)
           certs      (pem->certs pem)
           cert-count (count certs)]
@@ -213,6 +216,8 @@
         extensions (request->extensions request
                                         allow-header-cert-info
                                         oid-map)]
+    (log/trace (i18n/trs "Authorized name: {0}" name))
+    (log/trace (i18n/trs "Allow-header-cert-info: {0}" allow-header-cert-info))
     (->
       request
       (ring/set-authorized-name name)
@@ -221,7 +226,7 @@
                                            (verified? request
                                                       name
                                                       allow-header-cert-info)
-                                           (not (empty? name))))
+                                           (seq name)))
       (ring/set-authorized-certificate (request->cert request
                                                       allow-header-cert-info)))))
 

diff --git a/src/puppetlabs/trapperkeeper/services/authorization/authorization_service.clj b/src/puppetlabs/trapperkeeper/services/authorization/authorization_service.clj
@@ -34,15 +34,17 @@
          is-permitted? (if-let [rbac-service (maybe-get-service this :RbacConsumerService)]
                          (partial rbac/is-permitted? rbac-service))
          token->subject (if-let [rbac-service (maybe-get-service this :RbacConsumerService)]
-                          (partial rbac/valid-token->subject rbac-service))]
-     (log/debug (trs "Transformed auth.conf rules:\n{0}" (ks/pprint-to-string rules)))
-     (-> context
-         (assoc :is-permitted? is-permitted?)
-         (assoc :token->subject token->subject)
-         (assoc-in [:rules] rules)
-         (assoc-in [:allow-header-cert-info] (get config
-                                                  :allow-header-cert-info
-                                                  false)))))
+                          (partial rbac/valid-token->subject rbac-service))
+         result (-> context
+                    (assoc :is-permitted? is-permitted?)
+                    (assoc :token->subject token->subject)
+                    (assoc-in [:rules] rules)
+                    (assoc-in [:allow-header-cert-info] (get config
+                                                             :allow-header-cert-info
+                                                             false)))]
+     (log/debug (trs "Transformed auth.conf rules:\n{0}" (pr-str rules)))
+     (log/debug (trs "Configuration:\n {0}" (pr-str result)))
+     result))
 
   (authorization-check [this request]
    (authorization-check this request {:oid-map {}}))