From d52f2b18a5362096269fa63b4c2e7f1dce16af50 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 10:13:55 +0200 Subject: [PATCH 1/6] :sparkles: Add context to OIDC errors --- backend/src/app/auth/oidc.clj | 32 ++++++++++++++++++++++---------- 1 file changed, 22 insertions(+), 10 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 5b34ca10a..1bcbbda32 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -19,6 +19,7 @@ [app.email.blacklist :as email.blacklist] [app.email.whitelist :as email.whitelist] [app.http.client :as http] + [app.http.errors :as errors] [app.http.session :as session] [app.loggers.audit :as audit] [app.rpc.commands.profile :as profile] @@ -208,8 +209,9 @@ (ex/raise :type :internal :code :unable-to-retrieve-github-emails :hint "unable to retrieve github emails" - :http-status status - :http-body body)) + :request-uri (:uri params) + :response-status status + :response-body body)) (->> body json/decode (filter :primary) first :email)))) @@ -324,7 +326,7 @@ :uri (:token-uri provider) :body (u/map->query-string params)}] - (l/trace :hint "request access token" + (l/trace :hint "fetch access token" :provider (:name provider) :client-id (:client-id provider) :client-secret (obfuscate-string (:client-secret provider)) @@ -332,7 +334,7 @@ :redirect-uri (:redirect_uri params)) (let [{:keys [status body]} (http/req! cfg req {:sync? true})] - (l/trace :hint "access token response" :status status :body body) + (l/trace :hint "access token fetched" :status status :body body) (if (= status 200) (let [data (json/decode body)] {:token/access (get data :access_token) @@ -340,10 +342,11 @@ :token/type (get data :token_type)}) (ex/raise :type :internal - :code :unable-to-retrieve-token - :hint "unable to retrieve token" - :http-status status - :http-body body))))) + :code :unable-to-fetch-access-token + :hint "unable to fetch access token" + :request-uri (:uri req) + :response-status status + :response-body body))))) (defn- process-user-info [provider tdata info] @@ -601,7 +604,7 @@ ::rres/body {:redirect-uri uri}})) (defn- callback-handler - [cfg request] + [{:keys [::provider] :as cfg} request] (try (if-let [error (dm/get-in request [:params :error])] (redirect-with-error "unable-to-auth" error) @@ -609,7 +612,16 @@ profile (get-profile cfg info)] (process-callback cfg request info profile))) (catch Throwable cause - (l/err :hint "error on oauth process" :cause cause) + (binding [l/*context* (-> (errors/request->context request) + (assoc :auth/provider (:name provider)))] + (let [edata (ex-data cause)] + (cond + (= :validation (:type edata)) + (l/wrn :hint "invalid token received" :cause cause) + + :else + (l/err :hint "error on oauth process" :cause cause)))) + (redirect-with-error "unable-to-auth" (ex-message cause))))) (def provider-lookup From f4b59cc5a0fbb721c8aff9bfaccfe97338f19835 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 10:20:24 +0200 Subject: [PATCH 2/6] :sparkles: Normalize external-session-id parsing from request --- backend/src/app/auth/oidc.clj | 30 +++++++++++++++++++++--------- backend/src/app/rpc.clj | 18 ++++++++++++++++-- 2 files changed, 37 insertions(+), 11 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 1bcbbda32..64f9f534f 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -22,6 +22,7 @@ [app.http.errors :as errors] [app.http.session :as session] [app.loggers.audit :as audit] + [app.rpc :as rpc] [app.rpc.commands.profile :as profile] [app.setup :as-alias setup] [app.tokens :as tokens] @@ -589,17 +590,28 @@ (redirect-to-register cfg info request) (redirect-with-error "registration-disabled"))))) +(defn- get-external-session-id + [request] + (let [session-id (rreq/get-header request "x-external-session-id")] + (when (string? session-id) + (if (or (> (count session-id) 256) + (= session-id "null") + (str/blank? session-id)) + nil + session-id)))) + (defn- auth-handler [cfg {:keys [params] :as request}] - (let [props (audit/extract-utm-params params) - esid (rreq/get-header request "x-external-session-id") - state (tokens/generate (::setup/props cfg) - {:iss :oauth - :invitation-token (:invitation-token params) - :external-session-id esid - :props props - :exp (dt/in-future "4h")}) - uri (build-auth-uri cfg state)] + (let [props (audit/extract-utm-params params) + esid (rpc/get-external-session-id request) + params {:iss :oauth + :invitation-token (:invitation-token params) + :external-session-id esid + :props props + :exp (dt/in-future "4h")} + state (tokens/generate (::setup/props cfg) + (d/without-nils params)) + uri (build-auth-uri cfg state)] {::rres/status 200 ::rres/body {:redirect-uri uri}})) diff --git a/backend/src/app/rpc.clj b/backend/src/app/rpc.clj index fb6807651..9ee6a0abb 100644 --- a/backend/src/app/rpc.clj +++ b/backend/src/app/rpc.clj @@ -70,6 +70,20 @@ (handle-response-transformation request mdata) (handle-before-comple-hook mdata)))) +(defn get-external-session-id + [request] + (when-let [session-id (rreq/get-header request "x-external-session-id")] + (when-not (or (> (count session-id) 256) + (= session-id "null") + (str/blank? session-id)) + session-id))) + +(defn- get-external-event-origin + [request] + (when-let [origin (rreq/get-header request "x-event-origin")] + (when-not (> (count origin) 256) + origin))) + (defn- rpc-handler "Ring handler that dispatches cmd requests and convert between internal async flow into ring async flow." @@ -79,8 +93,8 @@ profile-id (or (::session/profile-id request) (::actoken/profile-id request)) - session-id (rreq/get-header request "x-external-session-id") - event-origin (rreq/get-header request "x-event-origin") + session-id (get-external-session-id request) + event-origin (get-external-event-origin request) data (-> params (assoc ::handler-name handler-name) From a54160965da8d2e39397cfef5e98d8e66046b613 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 10:42:59 +0200 Subject: [PATCH 3/6] :bug: Fix ip-addr parsing issue when it comes with port --- backend/src/app/loggers/audit.clj | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/backend/src/app/loggers/audit.clj b/backend/src/app/loggers/audit.clj index 3ef9e94e8..17a044459 100644 --- a/backend/src/app/loggers/audit.clj +++ b/backend/src/app/loggers/audit.clj @@ -35,9 +35,13 @@ (defn parse-client-ip [request] - (or (some-> (rreq/get-header request "x-forwarded-for") (str/split ",") first) - (rreq/get-header request "x-real-ip") - (some-> (rreq/remote-addr request) str))) + (let [ip-addr (or (some-> (rreq/get-header request "x-forwarded-for") (str/split ",") first) + (rreq/get-header request "x-real-ip") + (some-> (rreq/remote-addr request) str)) + ip-addr (-> ip-addr + (str/split ":" 2) + (first))] + ip-addr)) (defn extract-utm-params "Extracts additional data from params and namespace them under From 3b48be808c97d7a2e14408cccba688a7b45c275c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 13:39:32 +0200 Subject: [PATCH 4/6] :lipstick: Add minor naming change on calling logging on oidc ns --- backend/src/app/auth/oidc.clj | 78 +++++++++++++++++------------------ 1 file changed, 39 insertions(+), 39 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index 64f9f534f..ab82352fa 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -147,18 +147,18 @@ (when (contains? cf/flags :login-with-oidc) (if-let [opts (prepare-oidc-opts cfg)] (let [jwks (fetch-oidc-jwks cfg opts)] - (l/info :hint "provider initialized" - :provider "oidc" - :method (if (:discover? opts) "discover" "manual") - :client-id (:client-id opts) - :client-secret (obfuscate-string (:client-secret opts)) - :scopes (str/join "," (:scopes opts)) - :auth-uri (:auth-uri opts) - :user-uri (:user-uri opts) - :token-uri (:token-uri opts) - :roles-attr (:roles-attr opts) - :roles (:roles opts) - :keys (str/join "," (map str (keys jwks)))) + (l/inf :hint "provider initialized" + :provider "oidc" + :method (if (:discover? opts) "discover" "manual") + :client-id (:client-id opts) + :client-secret (obfuscate-string (:client-secret opts)) + :scopes (str/join "," (:scopes opts)) + :auth-uri (:auth-uri opts) + :user-uri (:user-uri opts) + :token-uri (:token-uri opts) + :roles-attr (:roles-attr opts) + :roles (:roles opts) + :keys (str/join "," (map str (keys jwks)))) (assoc opts :jwks jwks)) (do (l/warn :hint "unable to initialize auth provider, missing configuration" :provider "oidc") @@ -182,10 +182,10 @@ (if (and (string? (:client-id opts)) (string? (:client-secret opts))) (do - (l/info :hint "provider initialized" - :provider "google" - :client-id (:client-id opts) - :client-secret (obfuscate-string (:client-secret opts))) + (l/inf :hint "provider initialized" + :provider "google" + :client-id (:client-id opts) + :client-secret (obfuscate-string (:client-secret opts))) opts) (do @@ -237,10 +237,10 @@ (if (and (string? (:client-id opts)) (string? (:client-secret opts))) (do - (l/info :hint "provider initialized" - :provider "github" - :client-id (:client-id opts) - :client-secret (obfuscate-string (:client-secret opts))) + (l/inf :hint "provider initialized" + :provider "github" + :client-id (:client-id opts) + :client-secret (obfuscate-string (:client-secret opts))) opts) (do @@ -266,11 +266,11 @@ (if (and (string? (:client-id opts)) (string? (:client-secret opts))) (do - (l/info :hint "provider initialized" - :provider "gitlab" - :base-uri base - :client-id (:client-id opts) - :client-secret (obfuscate-string (:client-secret opts))) + (l/inf :hint "provider initialized" + :provider "gitlab" + :base-uri base + :client-id (:client-id opts) + :client-secret (obfuscate-string (:client-secret opts))) opts) (do @@ -327,15 +327,15 @@ :uri (:token-uri provider) :body (u/map->query-string params)}] - (l/trace :hint "fetch access token" - :provider (:name provider) - :client-id (:client-id provider) - :client-secret (obfuscate-string (:client-secret provider)) - :grant-type (:grant_type params) - :redirect-uri (:redirect_uri params)) + (l/trc :hint "fetch access token" + :provider (:name provider) + :client-id (:client-id provider) + :client-secret (obfuscate-string (:client-secret provider)) + :grant-type (:grant_type params) + :redirect-uri (:redirect_uri params)) (let [{:keys [status body]} (http/req! cfg req {:sync? true})] - (l/trace :hint "access token fetched" :status status :body body) + (l/trc :hint "access token fetched" :status status :body body) (if (= status 200) (let [data (json/decode body)] {:token/access (get data :access_token) @@ -374,9 +374,9 @@ (defn- fetch-user-info [{:keys [::provider] :as cfg} tdata] - (l/trace :hint "fetch user info" - :uri (:user-uri provider) - :token (obfuscate-string (:token/access tdata))) + (l/trc :hint "fetch user info" + :uri (:user-uri provider) + :token (obfuscate-string (:token/access tdata))) (let [params {:uri (:user-uri provider) :headers {"Authorization" (str (:token/type tdata) " " (:token/access tdata))} @@ -384,9 +384,9 @@ :method :get} response (http/req! cfg params {:sync? true})] - (l/trace :hint "user info response" - :status (:status response) - :body (:body response)) + (l/trc :hint "user info response" + :status (:status response) + :body (:body response)) (when-not (s/int-in-range? 200 300 (:status response)) (ex/raise :type :internal @@ -436,7 +436,7 @@ info (process-user-info provider tdata info)] - (l/trace :hint "user info" :info info) + (l/trc :hint "user info" :info info) (when-not (s/valid? ::info info) (l/warn :hint "received incomplete profile info object (please set correct scopes)" :info info) From 8dfc97d875b01582e8cc76318d6196096eb1846c Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 14:03:48 +0200 Subject: [PATCH 5/6] :sparkles: Add jwks loading on gitlab oidc provider --- backend/src/app/auth/oidc.clj | 25 +++++++++++++++---------- backend/src/app/main.clj | 2 +- 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/backend/src/app/auth/oidc.clj b/backend/src/app/auth/oidc.clj index ab82352fa..b48bdc9ac 100644 --- a/backend/src/app/auth/oidc.clj +++ b/backend/src/app/auth/oidc.clj @@ -132,8 +132,8 @@ (-> body json/decode :keys process-oidc-jwks) (do (l/warn :hint "unable to retrieve JWKs (unexpected response status code)" - :http-status status - :http-body body) + :response-status status + :response-body body) nil))) (catch Throwable cause (l/warn :hint "unable to retrieve JWKs (unexpected exception)" @@ -252,7 +252,7 @@ ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; (defmethod ig/init-key ::providers/gitlab - [_ _] + [_ cfg] (let [base (cf/get :gitlab-base-uri "https://gitlab.com") opts {:base-uri base :client-id (cf/get :gitlab-client-id) @@ -261,17 +261,18 @@ :auth-uri (str base "/oauth/authorize") :token-uri (str base "/oauth/token") :user-uri (str base "/oauth/userinfo") + :jwks-uri (str base "/oauth/discovery/keys") :name "gitlab"}] (when (contains? cf/flags :login-with-gitlab) (if (and (string? (:client-id opts)) (string? (:client-secret opts))) - (do + (let [jwks (fetch-oidc-jwks cfg opts)] (l/inf :hint "provider initialized" :provider "gitlab" :base-uri base :client-id (:client-id opts) :client-secret (obfuscate-string (:client-secret opts))) - opts) + (assoc opts :jwks jwks)) (do (l/warn :hint "unable to initialize auth provider, missing configuration" :provider "gitlab") @@ -337,11 +338,15 @@ (let [{:keys [status body]} (http/req! cfg req {:sync? true})] (l/trc :hint "access token fetched" :status status :body body) (if (= status 200) - (let [data (json/decode body)] - {:token/access (get data :access_token) - :token/id (get data :id_token) - :token/type (get data :token_type)}) - + (let [data (json/decode body) + data {:token/access (get data :access_token) + :token/id (get data :id_token) + :token/type (get data :token_type)}] + (l/trc :hint "access token fetched" + :token-id (:token/id data) + :token-type (:token/type data) + :token (:token/access data)) + data) (ex/raise :type :internal :code :unable-to-fetch-access-token :hint "unable to fetch access token" diff --git a/backend/src/app/main.clj b/backend/src/app/main.clj index f9797521d..a6a2bcebe 100644 --- a/backend/src/app/main.clj +++ b/backend/src/app/main.clj @@ -254,7 +254,7 @@ {::http.client/client (ig/ref ::http.client/client)} ::oidc.providers/gitlab - {} + {::http.client/client (ig/ref ::http.client/client)} ::oidc.providers/generic {::http.client/client (ig/ref ::http.client/client)} From 085b93379694157154302f8a0997d8dda62686e5 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 12 Jul 2024 14:26:21 +0200 Subject: [PATCH 6/6] :sparkles: Update default buffers and resolver on devenv nginx config --- docker/devenv/files/nginx.conf | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/docker/devenv/files/nginx.conf b/docker/devenv/files/nginx.conf index 60d025284..05df881bd 100644 --- a/docker/devenv/files/nginx.conf +++ b/docker/devenv/files/nginx.conf @@ -68,7 +68,10 @@ http { proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; - resolver 127.0.0.11; + proxy_buffer_size 16k; + proxy_busy_buffers_size 24k; # essentially, proxy_buffer_size + 2 small buffers of 4k + proxy_buffers 32 4k; + resolver 127.0.0.11 ipv6=off; etag off;