Repository: CSCfi/rems (clojure)

Issue:
Adding 'state' parameter into the OIDC flow
Australia has a national identity service for research institutes (AAF) that we would like to tie into REMS as an OIDC login flow.

Unfortunately (though possibly sensibly) - they **require** the use of the `state` parameter in the OIDC flow as part of their security posture (CSRF etc). I realise that is more than the OIDC spec requires - but I don't feel I can argue too hard with them about it.

I notice that the use of state has been considered in REMS!

https://github.com/CSCfi/rems/blob/b1cd1e3821e5701e5bf80cb78913d51519a16ab2/src/clj/rems/auth/oidc.clj#L26

Was wondering if we could put in a request for this to be completed. I can possibly have a crack at it if there are no objections?

a) at the very minimum - I can produce a `state` parameter for the OIDC flow but not actually confirm it later (this would probably fix my immediate needs)
b) complete the implementation of actual `state` checking - would require storing the state value somewhere in a db table (or cache?) - and which point I'm less confident..

Pull request description:
Close #3367.

# Checklist for author

Remove items that aren't applicable, check items that are done.

## Reviewability
- [x] Link to issue

## Backwards compatibility
- [x] Config is backwards compatible

## Documentation
- [x] Update changelog if necessary
- [x] Update docs/ (if applicable)
- [x] ADR for major architectural decisions or experiments
- [x] New config options in config-defaults.edn

## Testing
- [x] Valuable features are integration / browser / acceptance tested automatically

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c772c4bd2..b0647a863 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@ have notable changes.
 Changes since v2.38
 
 ### Additions
+- Use `:oidc-use-state` and value `:csrf-token`to configure REMS to send a CSRF token in `state` parameter to the login server and then check its value in the callback. (#3367)
 - REMS now includes user simulator, which is a developer tool intended for load testing REMS. The initial version is fairly basic and does not have a lot of features, but it can be used to test system performance and stability. See `docs/development.md` for more details.
 
 ### Changes
diff --git a/docs/architecture/006-authentication.md b/docs/architecture/006-authentication.md
index 152269af2..21f279d30 100644
--- a/docs/architecture/006-authentication.md
+++ b/docs/architecture/006-authentication.md
@@ -134,6 +134,10 @@ Even though in security contexts using well known and tested implementations is
 
 HTTP GET operations must not be used for any mutable operations in the future. Before, the strict cookie policy protected against this, but in the future the lax policy will allow cross site GET requests to be done with the session that the user already has on REMS. Our CORS should protect against data leaks with cross site GET requests.
 
+# Update 2024-12-13 `state` parameter
+
+It's now possible to configure REMS to pass and check a CSRF token into the IdP using the `state` parameter. This is a requirement in some cases.
+
 # Future considerations
 
 ## Single sign out
diff --git a/docs/configuration.md b/docs/configuration.md
index 044ddf378..d21a8b45f 100644
--- a/docs/configuration.md
+++ b/docs/configuration.md
@@ -54,6 +54,7 @@ The `:oidc` authentication method has the following configuration options:
 * `:oidc-additional-authorization-parameters` - additional query parameters to add to the OIDC authorization_endpoint url when logging in
 * `:oidc-logout-redirect-url` - to which URL a user is redirected to after a successful logout? (defaults to "/")
 * `:oidc-perform-revoke-on-logout` - should REMS POST to the `revocation_endpoint` received from OIDC metadata? (defaults to true)
+* `:oidc-use-state` – (optional) when the value is `:csrf-token`, REMS will pass a random CSRF token to login server using `state` parameter. Then REMS will check the return value in callback. (default is not to send)
 
 See details and formats from [config-defaults.edn](https://github.com/CSCfi/rems/blob/master/resources/config-defaults.edn).
 
diff --git a/resources/config-defaults.edn b/resources/config-defaults.edn
index 366669dfc..8e4572401 100644
--- a/resources/config-defaults.edn
+++ b/resources/config-defaults.edn
@@ -107,6 +107,10 @@
  :oidc-logout-redirect-url "/"
  ;; Should REMS POST to the revocation_endpoint received from OIDC metadata?
  :oidc-perform-revoke-on-logout true
+ ;; Should REMS use `state` parameter in the OIDC login.
+ ;; If set to `:csrf-token` then REMS will send the `state` parameter
+ ;; and check the return value in the callback. (default is not to send)
+ :oidc-use-state nil
 
  ;; Sending email.
  ;;
diff --git a/src/clj/rems/auth/oidc.clj b/src/clj/rems/auth/oidc.clj
index 67f1a674f..2a5aedb0a 100644
--- a/src/clj/rems/auth/oidc.clj
+++ b/src/clj/rems/auth/oidc.clj
@@ -4,8 +4,8 @@
             [clojure.test :refer :all]
             [clojure.tools.logging :as log]
             [compojure.core :refer [GET defroutes]]
-            [medley.core :refer [find-first]]
-            [rems.common.util :refer [getx]]
+            [medley.core :refer [dissoc-in find-first]]
+            [rems.common.util :refer [andstr getx]]
             [rems.config :refer [env oidc-configuration]]
             [rems.db.user-mappings]
             [rems.ga4gh :as ga4gh]
@@ -14,16 +14,23 @@
             [rems.plugins :as plugins]
             [rems.service.users]
             [ring.util.response :refer [redirect]])
-  (:import [java.time Instant]))
+  (:import [java.util UUID]
+           [java.time Instant]))
 
-(defn login-url []
+(defn maybe-create-oidc-csrf-token []
+  (case (get env :oidc-use-state)
+    :csrf-token (str (UUID/randomUUID))
+
+    nil))
+
+(defn login-url [{:keys [oidc-csrf-token]}]
   (str (:authorization_endpoint oidc-configuration)
        "?response_type=code"
        "&client_id=" (getx env :oidc-client-id)
        "&redirect_uri=" (getx env :public-url) "oidc-callback"
        "&scope=" (getx env :oidc-scopes)
        (getx env :oidc-additional-authorization-parameters)
-       #_"&state=STATE")) ; FIXME We could use the state for intelligent redirect. Also check if we need it for CSRF protection as Auth0 docs say.
+       (andstr "&state=" oidc-csrf-token)))
 
 (defn logout-url []
   "/oidc-logout")
@@ -123,6 +130,23 @@
     (save-user-mappings! user-data (:userid user))
     user))
 
+(defn maybe-check-csrf-token [request]
+  (let [original-csrf-token (get-in request [:session :oidc-csrf-token])
+        received-csrf-token (get-in request [:params :state])]
+    (cond
+      (not= :csrf-token (:oidc-use-state env))
+      nil ; nothing to check
+
+      (nil? original-csrf-token)
+      (throw (IllegalArgumentException. (pr-str {:original-csrf-token original-csrf-token})))
+
+      (nil? received-csrf-token)
+      (throw (IllegalArgumentException. (pr-str {:received-csrf-token received-csrf-token})))
+
+      (not= original-csrf-token received-csrf-token)
+      (throw (IllegalArgumentException. (pr-str {:original-csrf-token original-csrf-token
+                                                 :received-csrf-token received-csrf-token}))))))
+
 (defn oidc-callback [request]
   (let [error (get-in request [:params :error])
         code (get-in request [:params :code])]
@@ -135,7 +159,9 @@
               (redirect "/error?key=:t.login.errors/unknown"))
 
           :else
-          (let [response (-> (http/post (:token_endpoint oidc-configuration)
+          (let [_ (maybe-check-csrf-token request)
+
+                response (-> (http/post (:token_endpoint oidc-configuration)
                                         ;; NOTE Some IdPs don't support client id and secret in form params,
                                         ;;      and require us to use HTTP basic auth
                                         {:basic-auth [(str (getx env :oidc-client-id))
@@ -177,6 +203,7 @@
 
             (-> (redirect "/redirect")
                 (assoc :session (:session request))
+                (dissoc-in [:session :oidc-csrf-token]) ; cleanup, not used again
                 (assoc-in [:session :access-token] access-token)
                 (assoc-in [:session :identity] user))))))
 
@@ -198,21 +225,29 @@
           (when-not (= 200 (:status response))
             (log/error "received HTTP status" (:status response) "from" endpoint)))))))
 
-(defroutes routes
-  (GET "/oidc-login" _req (redirect (login-url)))
-  (GET "/oidc-logout" req
-    (let [session (get req :session)
-          redirect-url (:oidc-logout-redirect-url env)]
-      (when (:log-authentication-details env)
-        (log/info "logging out" (:identity session)))
+(defn oidc-login [request]
+  (let [oidc-csrf-token (maybe-create-oidc-csrf-token)]
+    (-> (redirect (login-url {:oidc-csrf-token oidc-csrf-token}))
+        (update :session assoc :oidc-csrf-token oidc-csrf-token))))
 
-      (when (:oidc-perform-revoke-in-logout env)
-        (oidc-revoke (:access-token session)))
+(defn oidc-logout [request]
+  (let [session (get request :session)
+        redirect-url (:oidc-logout-redirect-url env)]
+    (when (:log-authentication-details env)
+      (log/info "logging out" (:identity session)))
 
-      (when (:log-authentication-details env)
-        (log/info "redirecting to" redirect-url))
+    (when (:oidc-perform-revoke-in-logout env)
+      (oidc-revoke (:access-token session)))
 
-      (assoc (redirect redirect-url)
-             :session (dissoc session :identity :access-token))))
-  (GET "/oidc-callback" req (oidc-callback req)))
+    (when (:log-authentication-details env)
+      (log/info "redirecting to" redirect-url))
+
+    (assoc (redirect redirect-url)
+           :session (dissoc session :identity :access-token))))
 
+(defroutes routes
+  (GET "/oidc-login" req (oidc-login req))
+
+  (GET "/oidc-logout" req (oidc-logout req))
+
+  (GET "/oidc-callback" req (oidc-callback req)))


Test patch:
diff --git a/test/clj/rems/auth/test_oidc.clj b/test/clj/rems/auth/test_oidc.clj
index bd215a876..99fa3672f 100644
--- a/test/clj/rems/auth/test_oidc.clj
+++ b/test/clj/rems/auth/test_oidc.clj
@@ -12,7 +12,8 @@
 (defn- with-special-setup [params f]
   (let [id-data (:id-data params)
         config (:config params)
-        user-info {:unrelated 42}]
+        user-info {:unrelated 42}
+        oidc-csrf-token (:oidc-csrf-token params)]
 
     (with-redefs [rems.config/env (merge (assoc rems.config/env
                                                 :oidc-userid-attributes [{:attribute "sub" :rename "elixirId"}
@@ -21,7 +22,9 @@
                                                 :public-url "http://special:3000/"
                                                 :oidc-client-id "special.client-id"
                                                 :oidc-client-secret "special.client-secret")
+                                         (when oidc-csrf-token {:oidc-use-state :csrf-token})
                                          config)
+                  rems.auth.oidc/maybe-create-oidc-csrf-token (constantly oidc-csrf-token)
                   rems.config/oidc-configuration {:token_endpoint "https://special.case/token"
                                                   :issuer "https://special.case/issuer"
                                                   :userinfo_endpoint "https://special.case/user-info"}
@@ -125,3 +128,57 @@
                 :body ""}
                response)
             "can't log in when an error happens")))))
+
+(deftest test-state
+  (let [token "oidc-csrf-token-for-test"]
+    (with-special-setup {:id-data {:sub "user" :name "User" :email "user@example.com"}
+                         :config {:oidc-use-state :csrf-token}
+                         :oidc-csrf-token token}
+      (fn []
+
+        (testing "state is sent to provider"
+          (let [request {}
+                response (oidc/oidc-login request)]
+
+            (is (= {:status 302
+                    :headers {"Location" (str "?response_type=code&client_id=special.client-id&redirect_uri=http://special:3000/oidc-callback&scope=openid profile email&state=" token)}
+                    :body ""
+                    :session {:oidc-csrf-token token}}
+                   response))))
+
+        (testing "state is checked in callback"
+
+          (testing "state is configured but not in session"
+            (let [request {:params {:code "special-case-code"
+                                    :state token}
+                           :session {}}]
+              (is (thrown-with-msg? IllegalArgumentException
+                                    #"\{:original-csrf-token nil\}"
+                                    (oidc/oidc-callback request)))))
+
+          (testing "state is not passed to callback"
+            (let [request {:params {:code "special-case-code"}
+                           :session {:oidc-csrf-token token}}]
+              (is (thrown-with-msg? IllegalArgumentException
+                                    #"\{:received-csrf-token nil\}"
+                                    (oidc/oidc-callback request)))))
+
+          (testing "state does not match"
+            (let [request {:params {:code "special-case-code"
+                                    :state "wrong-token"}
+                           :session {:oidc-csrf-token token}}]
+              (is (thrown-with-msg? IllegalArgumentException
+                                    #"\{:original-csrf-token \"oidc-csrf-token-for-test\", :received-csrf-token \"wrong-token\"\}"
+                                    (oidc/oidc-callback request)))))
+
+          (testing "success"
+            (let [request {:params {:code "special-case-code"
+                                    :state token}
+                           :session {:oidc-csrf-token token}}
+                  response (oidc/oidc-callback request)]
+              (is (= {:status 302
+                      :headers {"Location" "/redirect"}
+                      :body ""
+                      :session {:access-token "special.access-token"
+                                :identity {:userid "user" :name "User" :email "user@example.com"}}}
+                     response)))))))))