From 6776bab9d5dd9621c1240c59239b34317c01c88b Mon Sep 17 00:00:00 2001 From: Brian DeRocher Date: Wed, 1 May 2024 15:59:25 -0400 Subject: [PATCH 1/4] improve: Also pass along a state (nonce) paramater. bug: #1134 It's not needed for CSRF protection because code_challenge does this, but the Authentik IDP will return &state= in the query string. This will trigger node-openid-client to fail a check. --- lib/resources/oidc.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/lib/resources/oidc.js b/lib/resources/oidc.js index 22a11cf21..96d2a806d 100644 --- a/lib/resources/oidc.js +++ b/lib/resources/oidc.js @@ -43,6 +43,7 @@ const ONE_HOUR = 60 * 60 * 1000; // * https://bugs.chromium.org/p/chromium/issues/detail?id=1056543 const CODE_VERIFIER_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'ocv'; const NEXT_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'next'; // eslint-disable-line no-multi-spaces +const STATE_COOKIE = (HTTPS_ENABLED ? '__Secure-' : '') + 'oidcstate'; const callbackCookieProps = { httpOnly: true, secure: HTTPS_ENABLED, @@ -102,15 +103,19 @@ module.exports = (service, endpoint) => { const client = await getClient(); const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase + // State is not strictly required because PKCS (code_challenge) protects us from CSRF attacks. + const state = generators.state(16); const code_challenge = generators.codeChallenge(code_verifier); // eslint-disable-line camelcase const authUrl = client.authorizationUrl({ scope: SCOPES.join(' '), resource: `${envDomain}/v1`, + state, code_challenge, code_challenge_method: CODE_CHALLENGE_METHOD, }); + res.cookie(STATE_COOKIE, state, { ...callbackCookieProps, maxAge: ONE_HOUR }); res.cookie(CODE_VERIFIER_COOKIE, code_verifier, { ...callbackCookieProps, maxAge: ONE_HOUR }); const { next } = req.query; @@ -129,15 +134,17 @@ module.exports = (service, endpoint) => { service.get('/oidc/callback', endpoint.html(async (container, _, req, res) => { try { + const state = req.cookies[STATE_COOKIE]; const code_verifier = req.cookies[CODE_VERIFIER_COOKIE]; // eslint-disable-line camelcase const next = req.cookies[NEXT_COOKIE]; // eslint-disable-line no-multi-spaces + res.clearCookie(STATE_COOKIE, callbackCookieProps); res.clearCookie(CODE_VERIFIER_COOKIE, callbackCookieProps); res.clearCookie(NEXT_COOKIE, callbackCookieProps); // eslint-disable-line no-multi-spaces const client = await getClient(); const params = client.callbackParams(req); - const tokenSet = await client.callback(getRedirectUri(), params, { code_verifier }); + const tokenSet = await client.callback(getRedirectUri(), params, { state, code_verifier }); const { access_token } = tokenSet; From 1e416c1b078ceaf28a32ce1430945853c488a197 Mon Sep 17 00:00:00 2001 From: Brian DeRocher Date: Thu, 2 May 2024 10:46:47 -0400 Subject: [PATCH 2/4] Explain why the nonce (state) is sent. --- lib/resources/oidc.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/resources/oidc.js b/lib/resources/oidc.js index 96d2a806d..cb6098148 100644 --- a/lib/resources/oidc.js +++ b/lib/resources/oidc.js @@ -104,6 +104,9 @@ module.exports = (service, endpoint) => { const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase // State is not strictly required because PKCS (code_challenge) protects us from CSRF attacks. + // See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-countermeasures-4 + // We create a nonce (state) anyway because some IDPs (e.g. Authentik) will send the nonce + // back to us. If it doesn't explicitly get one, it sends back an empty string. const state = generators.state(16); const code_challenge = generators.codeChallenge(code_verifier); // eslint-disable-line camelcase From 4cfd3ecb7ed74af8c0e6124ac5b0326fa39db647 Mon Sep 17 00:00:00 2001 From: Brian DeRocher Date: Sat, 4 May 2024 11:40:37 -0400 Subject: [PATCH 3/4] PKCS -> PKCE --- lib/resources/oidc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/resources/oidc.js b/lib/resources/oidc.js index cb6098148..b188a80ed 100644 --- a/lib/resources/oidc.js +++ b/lib/resources/oidc.js @@ -103,7 +103,7 @@ module.exports = (service, endpoint) => { const client = await getClient(); const code_verifier = generators.codeVerifier(); // eslint-disable-line camelcase - // State is not strictly required because PKCS (code_challenge) protects us from CSRF attacks. + // State is not strictly required because PKCE (code_challenge) protects us from CSRF attacks. // See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-countermeasures-4 // We create a nonce (state) anyway because some IDPs (e.g. Authentik) will send the nonce // back to us. If it doesn't explicitly get one, it sends back an empty string. From 5ecc86de2965175674721cf917ee8ba163420fea Mon Sep 17 00:00:00 2001 From: Brian DeRocher Date: Sat, 4 May 2024 11:44:20 -0400 Subject: [PATCH 4/4] Use default generated nonce length of 32. --- lib/resources/oidc.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/resources/oidc.js b/lib/resources/oidc.js index b188a80ed..ad598ac61 100644 --- a/lib/resources/oidc.js +++ b/lib/resources/oidc.js @@ -107,7 +107,7 @@ module.exports = (service, endpoint) => { // See https://datatracker.ietf.org/doc/html/draft-ietf-oauth-security-topics#name-countermeasures-4 // We create a nonce (state) anyway because some IDPs (e.g. Authentik) will send the nonce // back to us. If it doesn't explicitly get one, it sends back an empty string. - const state = generators.state(16); + const state = generators.state(); const code_challenge = generators.codeChallenge(code_verifier); // eslint-disable-line camelcase const authUrl = client.authorizationUrl({