Skip to content
This repository has been archived by the owner on Mar 22, 2023. It is now read-only.

Commit

Permalink
reports 403 error on OIDC too many cookies (#1660)
Browse files Browse the repository at this point in the history
  • Loading branch information
@shamsimam
shamsimam authored Sep 21, 2022
1 parent 7721b50 commit d9c4fb7
Show file tree
Hide file tree
Showing 4 changed files with 106 additions and 23 deletions.
86 changes: 67 additions & 19 deletions waiter/src/waiter/auth/oidc.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
[clojure.string :as str]
[clojure.tools.logging :as log]
[digest]
[ring.middleware.cookies :as cookies]
[waiter.auth.authentication :as auth]
[waiter.auth.jwt :as jwt]
[waiter.cookie-support :as cookie-support]
Expand Down Expand Up @@ -308,18 +309,55 @@
(= "." accept-redirect-auth)
(some #(= oidc-authority %) (str/split accept-redirect-auth #" ")))))))

(defn count-challenge-cookies
"Returns the number of challenge cookies."
[request]
(let [cookie-header (get-in request [:headers "cookie"])
request-cookies (cond->> cookie-header
(not (string? cookie-header)) (str/join ";"))]
(count
(filter #(str/starts-with? (str/trim %) oidc-challenge-cookie-prefix)
(str/split (str request-cookies) #";")))))

(defn too-many-oidc-challenge-cookies?
"Returns true if the request already contains too many OIDC challenge cookies."
[request num-allowed]
(let [cookie-header (get-in request [:headers "cookie"])
request-cookies (cond->> cookie-header
(not (string? cookie-header)) (str/join ";"))
num-challenge-cookies (count
(filter #(str/starts-with? (str/trim %) oidc-challenge-cookie-prefix)
(str/split (str request-cookies) #";")))]
(let [num-challenge-cookies (count-challenge-cookies request)]
(log/info "request has" num-challenge-cookies "oidc challenge cookies")
(> num-challenge-cookies num-allowed)))

(defn- promote-401-to-403?
"Determines whether the response represents a 401 from a downstream handler which attempted no authentication."
[response]
;; when downstream auth is disabled, the 401 response indicates OIDC auth handler is authoritative in
;; terms of the 4XX authentication response that is generated. We can rewrite the 401 response to a
;; 403 response to indicate an authentication error due to too many cookies.
(and (map? response)
(let [{:keys [waiter/auth-disabled? status]} response]
(and auth-disabled?
(= status http-401-unauthorized)
(utils/waiter-generated-response? response)))))

(defn make-oidc-auth-too-many-cookies-response-updater
"Returns a response updater that rewrites 401 waiter responses to 302 redirects."
[request num-allowed]
(fn update-oidc-auth-too-many-cookies-response
[response]
(cond-> response
(promote-401-to-403? response)
(merge (-> {:message (str "Too many OIDC challenge cookies (allowed: "
num-allowed ", provided: " (count-challenge-cookies request) ")")
:status http-403-forbidden}
(utils/data->error-response request)
(cookies/cookies-response))))))

(defn dissoc-auth-disabled?
"Dissociates the waiter/auth-disabled? entry on the response."
[response]
(cond-> response
(map? response)
(dissoc :waiter/auth-disabled?)))

(defn wrap-auth-handler
"Wraps the request handler with a handler to trigger OIDC+PKCE authentication."
[{:keys [allow-oidc-auth-api? allow-oidc-auth-services? jwt-auth-server oidc-authorize-uri oidc-default-mode
Expand All @@ -328,20 +366,30 @@
request-handler]
(let [oidc-authority (utils/uri-string->host oidc-authorize-uri)]
(fn oidc-auth-handler [request]
(let [oidc-mode-delay (delay (request->oidc-mode allow-oidc-auth-api? allow-oidc-auth-services? oidc-default-mode request))]
(cond
(or (auth/request-authenticated? request)
(= :disabled @oidc-mode-delay)
;; OIDC auth is no-op when request cannot be redirected
(not (supports-redirect? oidc-authority oidc-redirect-user-agent-products request))
;; OIDC auth is avoided if client already has too many challenge cookies
(too-many-oidc-challenge-cookies? request oidc-num-challenge-cookies-allowed-in-request))
(request-handler request)
(-> (let [oidc-mode-delay (delay (request->oidc-mode allow-oidc-auth-api? allow-oidc-auth-services? oidc-default-mode request))
skip-oidc? (or (auth/request-authenticated? request)
(= :disabled @oidc-mode-delay)
;; OIDC auth is no-op when request cannot be redirected
(not (supports-redirect? oidc-authority oidc-redirect-user-agent-products request)))]
(cond
(or skip-oidc?
;; OIDC auth is avoided if client already has too many challenge cookies
(too-many-oidc-challenge-cookies? request oidc-num-challenge-cookies-allowed-in-request))
(cond-> (request-handler request)
(not skip-oidc?)
;; update a 401 response to 403 if there were too many cookies and downstream auth options are disabled:
;; 401 indicates that the client request lacks valid authentication credentials whereas
;; 403 indicates that the server understands the request but refuses to authorize it.
(ru/update-response
(make-oidc-auth-too-many-cookies-response-updater request oidc-num-challenge-cookies-allowed-in-request)))

:else
(ru/update-response
(request-handler request)
(make-oidc-auth-response-updater jwt-auth-server @oidc-mode-delay oidc-same-site-attribute password request)))))))
:else
(ru/update-response
(request-handler request)
(make-oidc-auth-response-updater jwt-auth-server @oidc-mode-delay oidc-same-site-attribute password request))))
;; do not let the downstream waiter/auth-disabled? on the response flow out of the OIDC auth handler to avoid confusion
;; in upstream handlers who may think authentication was disabled overall while processing the request.
(ru/update-response dissoc-auth-disabled?)))))

(defrecord OidcAuthenticator [allow-oidc-auth-api? allow-oidc-auth-services? oidc-authorize-uri oidc-default-mode
jwt-auth-server jwt-validator oidc-num-challenge-cookies-allowed-in-request
Expand Down
8 changes: 5 additions & 3 deletions waiter/src/waiter/auth/spnego.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
(encode-output-token output-token))))

(defn- response-http-401-unauthorized
[request cause]
[request cause spnego-disabled?]
(log/info "triggering 401 response for spnego authentication" cause)
(counters/inc! (metrics/waiter-counter "core" "response-status" "401"))
(meters/mark! (metrics/waiter-meter "core" "response-status-rate" "401"))
Expand All @@ -68,21 +68,23 @@
:status http-401-unauthorized}
(utils/data->error-response request)
(cookies/cookies-response))
;; communicate upstream when spnego auth has been disabled.
true (assoc :waiter/auth-disabled? spnego-disabled?)
waiter-token (assoc :waiter/token waiter-token))))

(defn response-http-401-unauthorized-negotiate
"Tell the client you'd like them to use kerberos"
[request]
(log/info "triggering 401 negotiate for spnego authentication")
(-> (response-http-401-unauthorized request "for negotiation")
(-> (response-http-401-unauthorized request "for negotiation" false)
(assoc :error-class error-class-kerberos-negotiate)
(assoc-in [:headers "www-authenticate"] (str/trim negotiate-prefix))))

(defn response-http-401-unauthorized-spnego-disabled
"Tell the client you'd like them to not use kerberos.
Does not send the www-authenticate header, relies on upstream handlers to handle the missing negotiate header."
[request]
(response-http-401-unauthorized request "as it is disabled"))
(response-http-401-unauthorized request "as it is disabled" true))

(defn response-http-503-service-unavailable-temporarily-unavailable
"Tell the client you're overloaded and would like them to try later"
Expand Down
33 changes: 32 additions & 1 deletion waiter/test/waiter/auth/oidc_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,10 @@
(assoc response :processed-by :oidc-updater))]
(let [request-handler (fn [request] (assoc request
:processed-by :request-handler
:waiter/response-source :waiter))]
:waiter/response-source :waiter))
auth-disabled-request-handler (fn [request]
(-> (request-handler request)
(assoc :processed-by :oidc-auth-disabled-updater :waiter/auth-disabled? true)))]

(testing "allow oidc authentication combinations"
(doseq [allow-oidc-auth-api? [false true]]
Expand Down Expand Up @@ -531,6 +534,34 @@
"host" "www.test.com:1234"}
:status http-401-unauthorized
:waiter-api-call? false}))))
(let [oidc-auth-handler (wrap-auth-handler oidc-authenticator auth-disabled-request-handler)]
(let [cookie-header (str/join ";" (take (inc num-challenge-cookies-allowed-in-request) challenge-cookies))
response (oidc-auth-handler {:headers {"cookie" cookie-header
"host" "www.test.com:1234"}
:status http-401-unauthorized
:waiter-api-call? false})]
(is (str/includes? (get response :body)
(str "Too many OIDC challenge cookies "
"(allowed: " num-challenge-cookies-allowed-in-request
", provided: " (inc num-challenge-cookies-allowed-in-request) ")")))
(is (= {:headers {"content-type" "text/plain"}
:processed-by :oidc-auth-disabled-updater
:status http-403-forbidden
:waiter-api-call? false
:waiter/response-source :waiter}
(select-keys response [:headers :processed-by :status :waiter-api-call? :waiter/response-source]))))
(let [cookie-header (str/join ";" (take num-challenge-cookies-allowed-in-request challenge-cookies))
response (oidc-auth-handler {:headers {"cookie" cookie-header
"host" "www.test.com:1234"}
:status http-401-unauthorized
:waiter-api-call? false})]
(is (= {:headers {"cookie" cookie-header
"host" "www.test.com:1234"}
:processed-by :oidc-updater
:status http-401-unauthorized
:waiter-api-call? false
:waiter/response-source :waiter}
response))))
(let [cookie-header (str/join ";" (take (inc num-challenge-cookies-allowed-in-request) challenge-cookies))]
(is (= {:headers {"cookie" cookie-header
"host" "www.test.com:1234"}
Expand Down
2 changes: 2 additions & 0 deletions waiter/test/waiter/auth/spnego_test.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
:headers {"content-type" "text/plain"
"www-authenticate" "Negotiate"}
:status http-401-unauthorized
:waiter/auth-disabled? false
:waiter/response-source :waiter}]

(with-redefs [utils/error-context->text-body (fn mocked-error-context->text-body [data-map _] (-> data-map :message str))]
Expand All @@ -53,6 +54,7 @@
(is (= {:body "Unauthorized"
:headers {"content-type" "text/plain"}
:status http-401-unauthorized
:waiter/auth-disabled? true
:waiter/response-source :waiter}
(handler request))))))

Expand Down

0 comments on commit d9c4fb7

Please sign in to comment.