From 48e62e99f89419d12d60d19d16939966cac2660a Mon Sep 17 00:00:00 2001 From: jonathannewman Date: Tue, 8 Aug 2023 16:34:26 -0700 Subject: [PATCH] (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. --- .clj-kondo/config.edn | 3 ++- .../trapperkeeper/authorization/acl.clj | 16 ++++++------ .../authorization/ring_middleware.clj | 25 +++++++++++-------- .../authorization/authorization_service.clj | 20 ++++++++------- 4 files changed, 36 insertions(+), 28 deletions(-) diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index 0d6e7ca..dd348a2 100644 --- 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 index b420a5c..b39d95c 100644 --- 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 index 98577ca..d5e35df 100644 --- 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 index df51522..7cd82fa 100644 --- 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 {}}))