From 03cc1395d34ebd2f387252bd9e6dc7c57b381a1e Mon Sep 17 00:00:00 2001 From: "Dr. Christian Betz" Date: Fri, 26 Jan 2018 10:26:01 +0100 Subject: [PATCH] add :strategy option and protocol to middleware This allows other CSRF-protection strategies to be employed. --- README.md | 19 ++- project.clj | 1 + src/ring/middleware/oauth2.clj | 140 +++++++++++------- .../middleware/oauth2/default_handlers.clj | 11 ++ src/ring/middleware/oauth2/strategy.clj | 41 +++++ .../middleware/oauth2/strategy/session.clj | 48 ++++++ test/ring/middleware/oauth2_test.clj | 139 +++++++++-------- 7 files changed, 277 insertions(+), 122 deletions(-) create mode 100644 src/ring/middleware/oauth2/default_handlers.clj create mode 100644 src/ring/middleware/oauth2/strategy.clj create mode 100644 src/ring/middleware/oauth2/strategy/session.clj diff --git a/README.md b/README.md index 60634bf..df17d99 100644 --- a/README.md +++ b/README.md @@ -18,9 +18,10 @@ To install, add the following to your project `:dependencies`: ## Usage The middleware function to use is `ring.middleware.oauth2/wrap-oauth2`. -This takes a Ring handler, and a map of profiles as arguments. Each -profile has a key to identify it, and a map of options that define how -to authorize against a third-party service. +This takes a Ring handler, and a map of profiles as arguments and an +optional map of options. Each profile has a key to identify it, and +a map of properties that define how to authorize against a third-party +service. Here's an example that provides authentication with GitHub: @@ -104,6 +105,18 @@ in order to make the callback request handling work correctly, eg: [the specification]: https://tools.ietf.org/html/rfc6749#section-2.3.1 +## Options + +Using the optional options paramter, you can configure the behaviour +of wrap-oauth2. + +Using the `:strategy` option, you are able to configure a `state` +management strategy. The `state` is a CSRF-protection mechanism. The +default strategy relies on a server-side session to store the token +used for `state` in order to compare `state` received to the +session-token. To use another strategy, implement the +`ring.middleware.oauth2.strategy/Strategy`-protocol. + ## Workflow diagram The following image is a workflow diagram that describes the OAuth2 diff --git a/project.clj b/project.clj index 5a1bbaf..baef830 100644 --- a/project.clj +++ b/project.clj @@ -4,6 +4,7 @@ :license {:name "The MIT License" :url "http://opensource.org/licenses/MIT"} :dependencies [[org.clojure/clojure "1.7.0"] + [org.clojure/tools.logging "0.4.0"] [cheshire "5.8.0"] [clj-http "3.7.0"] [clj-time "0.14.0"] diff --git a/src/ring/middleware/oauth2.clj b/src/ring/middleware/oauth2.clj index 0d6635c..6b38b16 100644 --- a/src/ring/middleware/oauth2.clj +++ b/src/ring/middleware/oauth2.clj @@ -1,11 +1,16 @@ (ns ring.middleware.oauth2 - (:require [clj-http.client :as http] + (:require [ring.middleware.oauth2.strategy :as csrf] + [ring.middleware.oauth2.strategy.session :as session] + [ring.middleware.oauth2.default-handlers :as default-handlers] + [clj-http.client :as http] [clj-time.core :as time] [clojure.string :as str] [crypto.random :as random] [ring.util.codec :as codec] [ring.util.request :as req] - [ring.util.response :as resp])) + [ring.util.response :as resp] + [clojure.tools.logging :as log])) + (defn- redirect-uri [profile request] (-> (req/request-url request) @@ -25,78 +30,103 @@ :scope (scopes profile) :state state}))) -(defn- random-state [] - (-> (random/base64 9) (str/replace "+" "-") (str/replace "/" "_"))) - -(defn- make-launch-handler [profile] - (fn [{:keys [session] :or {session {}} :as request}] - (let [state (random-state)] - (-> (resp/redirect (authorize-uri profile request state)) - (assoc :session (assoc session ::state state)))))) +(defn- make-launch-handler [strategy profile] + (fn [request] + (let [token (csrf/get-token strategy request) + response (resp/redirect (authorize-uri profile request token))] + (csrf/write-token strategy profile request response token)))) -(defn- state-matches? [request] - (= (get-in request [:session ::state]) - (get-in request [:query-params "state"]))) (defn- format-access-token - [{{:keys [access_token expires_in refresh_token id_token]} :body :as r}] - (-> {:token access_token} - (cond-> expires_in (assoc :expires (-> expires_in time/seconds time/from-now)) - refresh_token (assoc :refresh-token refresh_token) - id_token (assoc :id-token id_token)))) + [{{:keys [access_token expires_in refresh_token id_token error error_description error_uri]} :body :as r}] + (when error + (log/warn (str error ": " error_description ", " error_uri) {:request r})) + (cond-> {:token access_token} + expires_in (assoc :expires (-> expires_in time/seconds time/from-now)) + refresh_token (assoc :refresh-token refresh_token) + id_token (assoc :id-token id_token))) (defn- request-params [profile request] - {:grant_type "authorization_code" - :code (get-in request [:query-params "code"]) - :redirect_uri (redirect-uri profile request)}) + {:grant_type "authorization_code" + :code (get-in request [:query-params "code"]) + :redirect_uri (redirect-uri profile request)}) (defn- add-header-credentials [opts id secret] (assoc opts :basic-auth [id secret])) (defn- add-form-credentials [opts id secret] - (assoc opts :form-params (-> (:form-params opts) - (merge {:client_id id - :client_secret secret})))) + (update-in + opts + [:form-params] + merge + {:client_id id, :client_secret secret})) (defn- get-access-token [{:keys [access-token-uri client-id client-secret basic-auth?] - :or {basic-auth? false} :as profile} request] + :or {basic-auth? false} :as profile} request] (format-access-token - (http/post access-token-uri - (cond-> {:accept :json, :as :json, - :form-params (request-params profile request)} - basic-auth? (add-header-credentials client-id client-secret) - (not basic-auth?) (add-form-credentials client-id client-secret))))) - -(defn state-mismatch-handler [_] - {:status 400, :headers {}, :body "State mismatch"}) - -(defn- make-redirect-handler [{:keys [id landing-uri] :as profile}] - (let [error-handler (:state-mismatch-handler profile state-mismatch-handler)] - (fn [{:keys [session] :or {session {}} :as request}] - (if (state-matches? request) - (let [access-token (get-access-token profile request)] - (-> (resp/redirect landing-uri) - (assoc :session (-> session - (assoc-in [::access-tokens id] access-token) - (dissoc ::state))))) - (error-handler request))))) - -(defn- assoc-access-tokens [request] - (if-let [tokens (-> request :session ::access-tokens)] - (assoc request :oauth2/access-tokens tokens) - request)) + (http/post access-token-uri + (cond-> {:accept :json, :as :json, + :form-params (request-params profile request)} + basic-auth? (add-header-credentials client-id client-secret) + (not basic-auth?) (add-form-credentials client-id client-secret))))) (defn- parse-redirect-url [{:keys [redirect-uri]}] (.getPath (java.net.URI. redirect-uri))) -(defn wrap-oauth2 [handler profiles] - (let [profiles (for [[k v] profiles] (assoc v :id k)) - launches (into {} (map (juxt :launch-uri identity)) profiles) +(defn wrap-access-tokens [handler] + (fn [request] + (handler + (if-let [tokens (-> request :session ::access-tokens)] + (assoc request :ring.middleware.oauth2/access-tokens tokens) + request)))) + + +(defn wrap-access-token-response + "if access-token-to-session? is true adds the access-token to the session for + response" + [{:keys [id landing-uri] :as profile} + {:keys [session] :or {session {}} :as request} + response + access-token + access-token-to-session?] + (if access-token-to-session? + (let [session (assoc-in session [:ring.middleware.oauth2/access-tokens id] access-token)] + (assoc response :session session)) + response)) + +(defn- read-token [request] + (get-in request [:query-params "state"])) + +(defn- make-redirect-handler [strategy {:keys [id landing-uri] :as profile} access-token-to-session?] + (let [error-handler (:state-mismatch-handler profile default-handlers/default-state-mismatch-handler) + success-handler (:success-handler profile default-handlers/default-success-handler)] + (fn [request] + (if (csrf/valid-token? strategy profile request (read-token request)) + (let [access-token (get-access-token profile request) + response (wrap-access-token-response profile + request + (success-handler profile access-token request) + access-token + access-token-to-session?)] + (csrf/remove-token strategy profile response)) + (error-handler profile request))))) + +(defn wrap-oauth2-flow [handler profiles & {:keys [strategy access-token-to-session?] + :or {strategy (session/session-strategy) + access-token-to-session? true}}] + (let [profiles (for [[k v] profiles] (assoc v :id k)) + launches (into {} (map (juxt :launch-uri identity)) profiles) redirects (into {} (map (juxt parse-redirect-url identity)) profiles)] (fn [{:keys [uri] :as request}] (if-let [profile (launches uri)] - ((make-launch-handler profile) request) + ((make-launch-handler strategy profile) request) (if-let [profile (redirects uri)] - ((make-redirect-handler profile) request) - (handler (assoc-access-tokens request))))))) + ((make-redirect-handler strategy profile access-token-to-session?) request) + (handler request)))))) + + +(defn wrap-oauth2 [handler profiles & options] + (-> + (apply wrap-oauth2-flow handler profiles options) + (wrap-access-tokens))) diff --git a/src/ring/middleware/oauth2/default_handlers.clj b/src/ring/middleware/oauth2/default_handlers.clj new file mode 100644 index 0000000..98636af --- /dev/null +++ b/src/ring/middleware/oauth2/default_handlers.clj @@ -0,0 +1,11 @@ +(ns ring.middleware.oauth2.default-handlers + (:require [ring.util.response :as resp])) + +(defn default-success-handler + [{:keys [id landing-uri] :as profile} access-token request] + (resp/redirect landing-uri)) + + + +(defn default-state-mismatch-handler [_ _] + {:status 400, :headers {}, :body "State mismatch"}) \ No newline at end of file diff --git a/src/ring/middleware/oauth2/strategy.clj b/src/ring/middleware/oauth2/strategy.clj new file mode 100644 index 0000000..0c9d8ef --- /dev/null +++ b/src/ring/middleware/oauth2/strategy.clj @@ -0,0 +1,41 @@ +(ns ring.middleware.oauth2.strategy + "Helper functions to implement strategies." + (:require [clj-http.client :as http] + [clj-time.core :as time] + [clojure.string :as str] + [ring.util.request :as req] + [ring.util.codec :as codec] + [clojure.tools.logging :as log])) + + + +(defprotocol Strategy + "CSRF protection is based on the fact, that some state is embedded + in the client webpage (e.g. as hidden form field) + and the server is able to validate that state. + + OWASP documents a number of patterns how to create and validate that state + in the form of a 'token', each with its own advantages and disadvantages. + + Strategy is the protocol to abstract the process + of token creation and validation." + (get-token [strategy request] + "Returns a token to be used. Users of ring.middleware.anti-forgery should + use the appropriate utility functions from `ring.util.anti-forgery` + namespace.") + + (valid-token? [strategy profile request token] + "Given the `request` and the `token` from that request, `valid-token?` + returns true if the token is valid. Returns false otherwise.") + + (write-token [strategy profile request response token] + "Some state management strategies do need to remember state (e.g., by + storing it to some storage accessible in different requests). `write-token` + is the method to handle state persistence, if necessary.") + + + (remove-token [strategy profile response] + ) + + ) + diff --git a/src/ring/middleware/oauth2/strategy/session.clj b/src/ring/middleware/oauth2/strategy/session.clj new file mode 100644 index 0000000..1d28934 --- /dev/null +++ b/src/ring/middleware/oauth2/strategy/session.clj @@ -0,0 +1,48 @@ +(ns ring.middleware.oauth2.strategy.session + (:require [ring.middleware.oauth2.strategy :as strategy] + [ring.util.response :as resp] + [clojure.string :as str] + [crypto.random :as random])) + +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; +;; Managing state using sessions +;; +;; This requires a session (or a shared session, can easily be distributed horizontally + + + + + + + + +#_(defn access-token-to-session + [{:keys [id landing-uri] :as profile} + access-token + {:keys [session] :or {session {}} :as request}] + (-> (default-success-handler profile access-token request) + (assoc :session (-> session + (assoc-in [:ring.middleware.oauth2/access-tokens id] access-token))))) + + + + +(deftype SessionStrategy [] + strategy/Strategy + + (get-token [_ _] + (-> (random/base64 9) (str/replace "+" "-") (str/replace "/" "_"))) + + (write-token [strategy profile {:keys [session] :or {session {}} :as request} response token] + (assoc response :session (assoc session :ring.middleware.oauth2/state token))) + + (remove-token [strategy profile response] + (update-in response [:session] dissoc :ring.middleware.oauth2/state)) + + (valid-token? [strategy profile request token] + (= (get-in request [:session :ring.middleware.oauth2/state]) + token))) + + +(defn session-strategy [] + (->SessionStrategy)) \ No newline at end of file diff --git a/test/ring/middleware/oauth2_test.clj b/test/ring/middleware/oauth2_test.clj index e7019f6..b7dceae 100644 --- a/test/ring/middleware/oauth2_test.clj +++ b/test/ring/middleware/oauth2_test.clj @@ -8,6 +8,7 @@ [ring.middleware.params :refer [wrap-params]] [ring.util.codec :as codec])) + (def test-profile {:authorize-uri "https://example.com/oauth2/authorize" :access-token-uri "https://example.com/oauth2/access-token" @@ -18,17 +19,20 @@ :client-id "abcdef" :client-secret "01234567890abcdef"}) -(defn token-handler [{:keys [oauth2/access-tokens]}] - {:status 200, :headers {}, :body access-tokens}) -(def test-handler + +(defn token-handler [req] + {:status 200, :headers {}, :body {:test {:expires 3600 + :token "defdef"}}}) + +(def test-handler-session (wrap-oauth2 token-handler {:test test-profile})) -(deftest test-launch-uri - (let [response (test-handler (mock/request :get "/oauth2/test")) - location (get-in response [:headers "Location"]) +(deftest test-launch-uri-session + (let [response (test-handler-session (mock/request :get "/oauth2/test")) + location (get-in response [:headers "Location"]) [_ query] (str/split location #"\?" 2) - params (codec/form-decode query)] + params (codec/form-decode query)] (is (= 302 (:status response))) (is (.startsWith ^String location "https://example.com/oauth2/authorize?")) (is (= {"response_type" "code" @@ -41,12 +45,12 @@ (:session response))))) (deftest test-location-uri-with-query - (let [profile (assoc test-profile - :authorize-uri - "https://example.com/oauth2/authorize?business_partner_id=XXXX") - handler (wrap-oauth2 token-handler {:test profile}) - response (handler (mock/request :get "/oauth2/test")) - location (get-in response [:headers "Location"])] + (let [profile (assoc test-profile + :authorize-uri + "https://example.com/oauth2/authorize?business_partner_id=XXXX") + handler (wrap-oauth2 token-handler {:test profile}) + response (handler (mock/request :get "/oauth2/test")) + location (get-in response [:headers "Location"])] (is (.startsWith ^String location "https://example.com/oauth2/authorize?business_partner_id=XXXX&")))) (def token-response @@ -56,53 +60,56 @@ (defn approx-eq [a b] (time/within? - (time/interval (time/minus a (time/seconds 1)) (time/plus a (time/seconds 1))) - b)) + (time/interval (time/minus a (time/seconds 1)) (time/plus a (time/seconds 1))) + b)) + +(defn callback [state & [cookie-state]] + (-> (mock/request :get "/oauth2/test/callback") + (assoc :query-params {"code" "abcabc", "state" state}) + (update-in [:cookies] assoc "state_test" {:value (or cookie-state state)}))) + +(defn callback-session [state session-state] + (-> (callback state) + (assoc :session {::oauth2/state session-state}))) + + -(deftest test-redirect-uri +(deftest test-redirect-uri-session (fake/with-fake-routes {"https://example.com/oauth2/access-token" (constantly token-response)} (testing "valid state" - (let [request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxyz"})) - response (test-handler request) - expires (-> 3600 time/seconds time/from-now)] + (let [request (callback-session "xyzxyz" "xyzxyz") + response (test-handler-session request) + expires (-> 3600 time/seconds time/from-now)] (is (= 302 (:status response))) (is (= "/" (get-in response [:headers "Location"]))) - (is (map? (-> response :session ::oauth2/access-tokens))) + (is (map? (-> response :session ::oauth2/access-tokens))) ;; default success handler is writing access-token to session. (is (= "defdef" (-> response :session ::oauth2/access-tokens :test :token))) (is (approx-eq (-> 3600 time/seconds time/from-now) (-> response :session ::oauth2/access-tokens :test :expires))))) (testing "invalid state" - (let [request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxya"})) - response (test-handler request)] + (let [request (callback-session "xyzxya" "xyzxyz") + response (test-handler-session request)] (is (= {:status 400, :headers {}, :body "State mismatch"} response)))) (testing "custom error" - (let [error {:status 400, :headers {}, :body "Error!"} - profile (assoc test-profile :state-mismatch-handler (constantly error)) - handler (wrap-oauth2 token-handler {:test profile}) - request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxya"})) + (let [error {:status 400, :headers {}, :body "Error!"} + profile (assoc test-profile :state-mismatch-handler (constantly error)) + handler (wrap-oauth2 token-handler {:test profile}) + request (callback-session "xyzxya" "xyzxyz") response (handler request)] (is (= {:status 400, :headers {}, :body "Error!"} response)))) (testing "absolute redirect uri" - (let [profile (assoc test-profile - :redirect-uri - "https://example.com/oauth2/test/callback?query") - handler (wrap-oauth2 token-handler {:test profile}) - request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxyz"})) + (let [profile (assoc test-profile + :redirect-uri + "https://example.com/oauth2/test/callback?query") + handler (wrap-oauth2 token-handler {:test profile}) + request (callback-session "xyzxyz" "xyzxyz") response (handler request)] (is (= 302 (:status response))) (is (= "/" (get-in response [:headers "Location"]))) @@ -111,28 +118,26 @@ (is (approx-eq (-> 3600 time/seconds time/from-now) (-> response :session ::oauth2/access-tokens :test :expires))))))) + (deftest test-access-tokens-key (let [tokens {:test {:token "defdef", :expires 3600}}] (is (= {:status 200, :headers {}, :body tokens} - (test-handler (-> (mock/request :get "/") - (assoc :session {::oauth2/access-tokens tokens}))))))) + (test-handler-session (-> (mock/request :get "/") + (assoc :session {::oauth2/access-tokens tokens}))))))) (deftest test-true-basic-auth-param (fake/with-fake-routes {"https://example.com/oauth2/access-token" - (fn [req] - (let [auth (get-in req [:headers "authorization"])] - (is (and (not (str/blank? auth)) - (.startsWith auth "Basic"))) - token-response))} + (fn [req] + (let [auth (get-in req [:headers "authorization"])] + (is (and (not (str/blank? auth)) + (.startsWith auth "Basic"))) + token-response))} (testing "valid state" (let [profile (assoc test-profile :basic-auth? true) handler (wrap-oauth2 token-handler {:test profile}) - request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc" - "state" "xyzxyz"})) + request (callback-session "xyzxyz" "xyzxyz") response (handler request)])))) (defn contains-many? [m & ks] @@ -142,16 +147,14 @@ (fake/with-fake-routes {"https://example.com/oauth2/access-token" (wrap-params (fn [req] - (let [params (get-in req [:params])] - (is (contains-many? params "client_id" "client_secret")) - token-response)))} + (let [params (get-in req [:params])] + (is (contains-many? params "client_id" "client_secret")) + token-response)))} (testing "valid state" (let [profile (assoc test-profile :basic-auth? false) handler (wrap-oauth2 token-handler {:test profile}) - request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxyz"})) + request (callback-session "xyzxyz" "xyzxyz") response (handler request)])))) @@ -161,23 +164,31 @@ :body "{\"access_token\":\"defdef\",\"expires_in\":3600, \"refresh_token\":\"ghighi\",\"id_token\":\"abc.def.ghi\"}"}) -(deftest test-openid-response +(deftest test-openid-response-session (fake/with-fake-routes {"https://example.com/oauth2/access-token" (constantly openid-response)} (testing "valid state" - (let [request (-> (mock/request :get "/oauth2/test/callback") - (assoc :session {::oauth2/state "xyzxyz"}) - (assoc :query-params {"code" "abcabc", "state" "xyzxyz"})) - response (test-handler request) - expires (-> 3600 time/seconds time/from-now)] + (let [request (callback-session "xyzxyz" "xyzxyz") + response (test-handler-session request) + expires (-> 3600 time/seconds time/from-now)] (is (= 302 (:status response))) (is (= "/" (get-in response [:headers "Location"]))) (is (map? (-> response :session ::oauth2/access-tokens))) (is (= "defdef" (-> response :session ::oauth2/access-tokens :test :token))) (is (= "ghighi" (-> response :session ::oauth2/access-tokens - :test :refresh-token))) + :test :refresh-token))) (is (= "abc.def.ghi" (-> response :session ::oauth2/access-tokens - :test :id-token))) + :test :id-token))) (is (approx-eq (-> 3600 time/seconds time/from-now) (-> response :session ::oauth2/access-tokens :test :expires))))))) + + +(deftest test-with-access-token + (let [test-map {:anything :really} + request (-> (mock/request :get "/anything-not-oauth-related") + (assoc-in [:session :ring.middleware.oauth2/access-tokens :test] test-map)) + identity-handler (wrap-oauth2 identity {:test test-profile}) + request-after-middleware (identity-handler request)] + (is (= {:test test-map} + (:ring.middleware.oauth2/access-tokens request-after-middleware))))) \ No newline at end of file