Added support for ALLOWED_CLIENT_IDS (#9)

* Added support for ALLOWED_CLIENT_IDS * Review feedback * Support for loading secrets from files specified by env vars. * Prevent exceptions during request, correctly set json headers * Log gateway url * Refactored config secret file loading * Last review feedback * Review feedback --------- Co-authored-by: Joost Diepenmaat <[email protected]>
SURFnet · Sep 17, 2024 · b81e181 · b81e181
1 parent f0933e4
commit b81e181
Show file tree

Hide file tree

Showing 6 changed files with 230 additions and 62 deletions.
diff --git a/readme.md b/readme.md
@@ -24,6 +24,7 @@ GATEWAY_BASIC_AUTH_PASS             Password for gateway
 SURF_CONEXT_CLIENT_ID               SurfCONEXT client id for validation service
 SURF_CONEXT_CLIENT_SECRET           SurfCONEXT client secret for validation service
 SURF_CONEXT_INTROSPECTION_ENDPOINT  SurfCONEXT introspection endpoint
+ALLOWED_CLIENT_IDS                  Comma separated list of allowed SurfCONEXT client ids. 
 OOAPI_VERSION                       Ooapi version to pass through to gateway
 ```
 

diff --git a/src/nl/surf/eduhub/validator/service/authentication.clj b/src/nl/surf/eduhub/validator/service/authentication.clj
@@ -52,7 +52,7 @@
 ;; Take a authentication uri, basic auth credentials and a token extracted from the bearer token
 ;; and make a call to the authentication endpoint.
 ;; Returns the client id if authentication is successful, otherwise nil.
-(defn authenticate-token [uri token auth]
+(defn- authenticate-token [uri token auth]
   {:pre [(string? uri)
          (string? token)
          (map? auth)]}
@@ -75,7 +75,7 @@
       (log/error ex "Error in token-authenticator")
       nil)))
 
-(defn make-token-authenticator
+(defn- make-token-authenticator
   "Make a token authenticator that uses the OIDC `introspection-endpoint`.
 
   Returns a authenticator that tests the token using the given
@@ -89,7 +89,7 @@
                         token
                         auth)))
 
-(defn handle-request-with-token [request request-handler client-id]
+(defn- handle-request-with-token [request request-handler client-id]
   (if (nil? client-id)
     (response/status http-status/forbidden)
     ;; set client-id on request and response (for tracing)
@@ -116,3 +116,11 @@
       (if-let [token (bearer-token request)]
         (handle-request-with-token request f (authenticator token))
         (f request)))))
+
+(defn wrap-allowed-clients-checker [f allowed-client-id-set]
+  {:pre [(set? allowed-client-id-set)]}
+  (fn [{:keys [client-id] :as request}]
+    (if (and client-id (allowed-client-id-set client-id))
+      (f request)
+      {:body (if client-id "Unknown client id" "No client-id found")
+       :status http-status/forbidden})))
diff --git a/src/nl/surf/eduhub/validator/service/config.clj b/src/nl/surf/eduhub/validator/service/config.clj
@@ -0,0 +1,67 @@
+;; This file is part of eduhub-validator-service
+;;
+;; Copyright (C) 2022 SURFnet B.V.
+;;
+;; This program is free software: you can redistribute it and/or
+;; modify it under the terms of the GNU Affero General Public License
+;; as published by the Free Software Foundation, either version 3 of
+;; the License, or (at your option) any later version.
+;;
+;; This program is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; Affero General Public License for more details.
+;;
+;; You should have received a copy of the GNU Affero General Public
+;; License along with this program.  If not, see
+;; <https://www.gnu.org/licenses/>.
+
+(ns nl.surf.eduhub.validator.service.config
+  (:require [clojure.java.io :as io]
+            [clojure.string :as str]
+            [nl.jomco.envopts :as envopts]))
+
+(def opt-specs
+  {:gateway-url                        ["URL of gateway" :str
+                                        :in [:gateway-url]]
+   :gateway-basic-auth-user            ["Basic auth username of gateway" :str
+                                        :in [:gateway-basic-auth :user]]
+   :gateway-basic-auth-pass            ["Basic auth password of gateway" :str
+                                        :in [:gateway-basic-auth :pass]]
+   :allowed-client-ids                 ["Comma separated list of allowed SurfCONEXT client ids." :str
+                                        :in [:allowed-client-ids]]
+   :surf-conext-client-id              ["SurfCONEXT client id for validation service" :str
+                                        :in [:introspection-basic-auth :user]]
+   :surf-conext-client-secret          ["SurfCONEXT client secret for validation service" :str
+                                        :in [:introspection-basic-auth :pass]]
+   :surf-conext-introspection-endpoint ["SurfCONEXT introspection endpoint" :str
+                                        :in [:introspection-endpoint-url]]
+   :ooapi-version                      ["Ooapi version to pass through to gateway" :str
+                                        :in [:ooapi-version]]})
+
+(defn- file-secret-loader-reducer [env-map value-key]
+  (let [file-key (keyword (str (name value-key) "-file"))
+        path (file-key env-map)]
+    (cond
+      (nil? path)
+      env-map
+
+      (not (.exists (io/file path)))
+      (throw (ex-info (str "ENV var contains filename that does not exist: " path)
+                      {:filename path, :env-path file-key}))
+
+      (value-key env-map)
+      (throw (ex-info "ENV var contains secret both as file and as value"
+                      {:env-path [value-key file-key]}))
+
+      :else
+      (assoc env-map value-key (str/trim (slurp path))))))
+
+;; These ENV keys may alternatively have a form in which the secret is contained in a file.
+;; These ENV keys have a -file suffix, e.g.: gateway-basic-auth-pass-file
+(def env-keys-with-alternate-file-secret
+  [:gateway-basic-auth-user :gateway-basic-auth-pass :surf-conext-client-id :surf-conext-client-secret])
+
+(defn load-config-from-env [env-map]
+  (-> (reduce file-secret-loader-reducer env-map env-keys-with-alternate-file-secret)
+      (envopts/opts opt-specs)))
diff --git a/src/nl/surf/eduhub/validator/service/main.clj b/src/nl/surf/eduhub/validator/service/main.clj
@@ -19,26 +19,28 @@
 (ns nl.surf.eduhub.validator.service.main
   (:gen-class)
   (:require [babashka.http-client :as http]
+            [clojure.string :as str]
             [compojure.core :refer [defroutes GET]]
             [compojure.route :as route]
             [clojure.tools.logging :as log]
             [environ.core :refer [env]]
             [nl.jomco.envopts :as envopts]
             [nl.jomco.http-status-codes :as http-status]
             [nl.surf.eduhub.validator.service.authentication :as auth]
+            [nl.surf.eduhub.validator.service.config :as config]
             [ring.adapter.jetty :refer [run-jetty]]
             [ring.middleware.defaults :refer [wrap-defaults api-defaults]]
             [ring.middleware.json :refer [wrap-json-response]]))
 
 (defn validate-endpoint [endpoint-id {:keys [gateway-url gateway-basic-auth ooapi-version] :as _config}]
-  (log/info "validating endpoint: " endpoint-id)
+  (log/info (str "validating endpoint: " endpoint-id " - gateway-url: " gateway-url))
   (try
     (let [response (http/get (str gateway-url "courses")
                              {:headers {"x-route" (str "endpoint=" endpoint-id)
                                         "accept" (str "application/json; version=" ooapi-version)
                                         "x-envelope-response" "false"}
                               :basic-auth gateway-basic-auth
-                              :throws false})]
+                              :throw false})]
       (if (= (:status response) 200)
         {:valid true}
         {:valid false :message (str "Endpoint validation failed with status: " (:status response))}))
@@ -62,22 +64,6 @@
                      http-status/ok)})
         resp))))
 
-(def opt-specs
-  {:gateway-url                        ["URL of gateway" :str
-                                        :in [:gateway-url]]
-   :gateway-basic-auth-user            ["Basic auth username of gateway" :str
-                                        :in [:gateway-basic-auth :user]]
-   :gateway-basic-auth-pass            ["Basic auth password of gateway" :str
-                                        :in [:gateway-basic-auth :pass]]
-   :surf-conext-client-id              ["SurfCONEXT client id for validation service" :str
-                                        :in [:introspection-basic-auth :user]]
-   :surf-conext-client-secret          ["SurfCONEXT client secret for validation service" :str
-                                        :in [:introspection-basic-auth :pass]]
-   :surf-conext-introspection-endpoint ["SurfCONEXT introspection endpoint" :str
-                                        :in [:introspection-endpoint-url]]
-   :ooapi-version                      ["Ooapi version to pass through to gateway" :str
-                                        :in [:ooapi-version]]})
-
 (defn start-server [routes]
   (let [server (run-jetty routes {:port 3002 :join? false})
         handler ^Runnable (fn [] (.stop server))]
@@ -87,17 +73,19 @@
     server))
 
 (defn -main [& _]
-  (let [[config errs] (envopts/opts env opt-specs)
-        introspection-endpoint (:introspection-endpoint-url config)
-        introspection-auth (:introspection-basic-auth config)]
+  (let [[config errs] (config/load-config-from-env env)]
     (when errs
       (.println *err* "Error in environment configuration")
       (.println *err* (envopts/errs-description errs))
       (.println *err* "Available environment vars:")
-      (.println *err* (envopts/specs-description opt-specs))
+      (.println *err* (envopts/specs-description config/opt-specs))
       (System/exit 1))
-    (start-server (-> app-routes
-                      (wrap-validator config)
-                      (auth/wrap-authentication introspection-endpoint introspection-auth)
-                      (wrap-defaults api-defaults)
-                      wrap-json-response))))
+    (let [introspection-endpoint (:introspection-endpoint-url config)
+          introspection-auth     (:introspection-basic-auth config)
+          allowed-client-id-set  (set (str/split (:allowed-client-ids config) #","))]
+      (start-server (-> app-routes
+                        (wrap-validator config)
+                        (auth/wrap-allowed-clients-checker allowed-client-id-set)
+                        (auth/wrap-authentication introspection-endpoint introspection-auth)
+                        wrap-json-response
+                        (wrap-defaults api-defaults))))))
diff --git a/test/nl/surf/eduhub/validator/service/authentication_test.clj b/test/nl/surf/eduhub/validator/service/authentication_test.clj
@@ -58,40 +58,73 @@
     {:status http-status/ok
      :body {:active false}}))
 
+(defn- make-handler [introspection-endpoint basic-auth allowed-client-id-set]
+  (-> (fn [req]
+        (let [body {:client (:client-id req)}]
+          {:status http-status/ok
+           :body   body}))
+      (authentication/wrap-allowed-clients-checker allowed-client-id-set)
+      (authentication/wrap-authentication introspection-endpoint basic-auth)))
+
 (deftest token-validator
   ;; This binds the *dynamic* http client in clj-http.client
-  (reset! count-calls 0)
   (with-redefs [http/request mock-introspection]
     (let [introspection-endpoint "https://example.com"
-          basic-auth {:user "foo" :pass "bar"}
-          handler       (-> (fn [req]
-                              (let [body {:client (:client-id req)}]
-                                {:status http-status/ok
-                                 :body   body}))
-                            (authentication/wrap-authentication introspection-endpoint basic-auth))]
-      (is (= {:status http-status/ok
-              :body   {:client "institution_client_id"}
-              :client-id "institution_client_id"}
-             (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
-          "Ok when valid token provided")
-
-      (is (= {:status 200, :body {:client nil}}
-             (handler {}))
-          "Authorized without client when no token provided")
-
-      (is (= http-status/forbidden
-             (:status (handler {:headers {"authorization" (str "Bearer invalid-token")}})))
-          "Forbidden with invalid token")
-
-      (is (= 2 @count-calls)
-          "Correct number of calls to introspection-endpoint")
-
+          basic-auth {:user "foo" :pass "bar"}]
       (reset! count-calls 0)
-      (is (= {:status http-status/ok
-              :body   {:client "institution_client_id"}
-              :client-id "institution_client_id"}
-             (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
-          "CACHED: Ok when valid token provided")
-
-      (is (= 0 @count-calls)
-          "No more calls to introspection-endpoint"))))
+      (let [handler (make-handler introspection-endpoint basic-auth #{"institution_client_id"})]
+        (is (= {:status    http-status/ok
+                :body      {:client "institution_client_id"}
+                :client-id "institution_client_id"}
+               (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
+            "Ok when valid token provided")
+
+        (is (= {:status 403, :body "No client-id found"}
+               (handler {}))
+            "Not authorized when no token provided")
+
+        (is (= http-status/forbidden
+               (:status (handler {:headers {"authorization" (str "Bearer invalid-token")}})))
+            "Forbidden with invalid token")
+
+        (is (= 2 @count-calls)
+            "Correct number of calls to introspection-endpoint")
+
+        (reset! count-calls 0)
+        (is (= {:status    http-status/ok
+                :body      {:client "institution_client_id"}
+                :client-id "institution_client_id"}
+               (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
+            "CACHED: Ok when valid token provided")
+
+        (is (= 0 @count-calls)
+            "No more calls to introspection-endpoint"))
+
+      (let [handler (make-handler introspection-endpoint basic-auth #{"wrong_client_id"})]
+        (reset! count-calls 0)
+        (is (= {:status http-status/forbidden
+                :body   "Unknown client id"
+                :client-id "institution_client_id"}
+               (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
+            "Forbidden when valid token provided but client id is unknown")
+
+        (is (= {:status 403, :body "No client-id found"}
+               (handler {}))
+            "Not authorized when no token provided")
+
+        (is (= http-status/forbidden
+               (:status (handler {:headers {"authorization" (str "Bearer invalid-token")}})))
+            "Forbidden with invalid token")
+
+        (is (= 2 @count-calls)
+            "Correct number of calls to introspection-endpoint")
+
+        (reset! count-calls 0)
+        (is (= {:status    http-status/forbidden
+                :body      "Unknown client id"
+                :client-id "institution_client_id"}
+               (handler {:headers {"authorization" (str "Bearer " valid-token)}}))
+            "CACHED: Forbidden when valid token provided but client id is unknown")
+
+        (is (= 0 @count-calls)
+            "No more calls to introspection-endpoint")))))
diff --git a/test/nl/surf/eduhub/validator/service/config_test.clj b/test/nl/surf/eduhub/validator/service/config_test.clj
@@ -0,0 +1,71 @@
+;; This file is part of eduhub-validator-service
+;;
+;; Copyright (C) 2022 SURFnet B.V.
+;;
+;; This program is free software: you can redistribute it and/or
+;; modify it under the terms of the GNU Affero General Public License
+;; as published by the Free Software Foundation, either version 3 of
+;; the License, or (at your option) any later version.
+;;
+;; This program is distributed in the hope that it will be useful, but
+;; WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+;; Affero General Public License for more details.
+;;
+;; You should have received a copy of the GNU Affero General Public
+;; License along with this program.  If not, see
+;; <https://www.gnu.org/licenses/>.
+
+(ns nl.surf.eduhub.validator.service.config-test
+  (:require [clojure.test :refer [deftest is]]
+            [nl.surf.eduhub.validator.service.config :as config]
+            [nl.surf.eduhub.validator.service.main :as main])
+  (:import [clojure.lang ExceptionInfo]
+           [java.io File]))
+
+(def app (main/wrap-validator main/app-routes {}))
+
+(def default-env {:allowed-client-ids "default",
+                  :gateway-basic-auth-pass "default",
+                  :gateway-url "default",
+                  :ooapi-version "default",
+                  :surf-conext-client-id "default",
+                  :surf-conext-client-secret "default",
+                  :surf-conext-introspection-endpoint "default"})
+
+(def default-expected-value {:allowed-client-ids "default",
+                             :gateway-url "default",
+                             :ooapi-version "default",
+                             :gateway-basic-auth {:pass "default", :user "john200"},
+                             :introspection-basic-auth {:pass "default", :user "default"},
+                             :introspection-endpoint-url "default"})
+
+(defn- test-env [env]
+  (config/load-config-from-env (merge default-env env)))
+
+(deftest missing-secret
+  (is (= {:gateway-basic-auth-user "missing"}
+         (last (test-env {})))))
+
+(deftest only-value-secret
+  (let [env {:gateway-basic-auth-user "john200"}]
+    (is (= [default-expected-value]
+           (test-env env)))))
+
+(deftest only-file-secret
+  (let [path (.getAbsolutePath (File/createTempFile "test-secret" ".txt"))
+        env {:gateway-basic-auth-user-file path}]
+    (spit path "john200")
+    (is (= [default-expected-value]
+           (test-env env)))))
+
+(deftest only-file-secret-file-missing
+  (let [env {:gateway-basic-auth-user-file "missing-file"}]
+    (is (thrown? ExceptionInfo (test-env env)))))
+
+(deftest both-types-of-secret-specified
+    (let [path (.getAbsolutePath (File/createTempFile "test-secret" ".txt"))
+          env {:gateway-basic-auth-user "john200"
+               :gateway-basic-auth-user-file path}]
+      (spit path "john200")
+      (is (thrown? ExceptionInfo (test-env env)))))