diff --git a/apps/docs/content/self-hosting/manage/login-client.mdx b/apps/docs/content/self-hosting/manage/login-client.mdx index bcd02bdf79..3adc1f4ee6 100644 --- a/apps/docs/content/self-hosting/manage/login-client.mdx +++ b/apps/docs/content/self-hosting/manage/login-client.mdx @@ -42,6 +42,8 @@ The Login UI logs a warning at startup while it uses this fallback. The fallback has one consequence to be aware of: **rotating the value of that credential (for example creating a new PAT) invalidates all existing session cookies**, and users have to sign in to the Login UI again. Their application sessions and tokens are not affected. +The same secret also signs the short-lived (5 minutes) proof that a user just verified their email or invite code, so a user who is in the middle of that step during the rotation has to request a new code. +Likewise, it signs the short-lived (15 minutes) proof that ties linking an identity provider to the signed-in session, so a user linking an identity provider during the rotation has to start the linking again. Switching from one credential type to another (for example from a PAT to a login client key) does not invalidate cookies as long as both credentials are configured during the migration. Set a dedicated secret with `ZITADEL_SESSION_COOKIE_SECRET` (at least 32 characters, for example `openssl rand -base64 32`). This is the recommended configuration and will become required in a future major release: it decouples the cookie lifetime from credential rotation. diff --git a/apps/login/next-env-vars.d.ts b/apps/login/next-env-vars.d.ts index 4d69754372..270277af7a 100644 --- a/apps/login/next-env-vars.d.ts +++ b/apps/login/next-env-vars.d.ts @@ -25,14 +25,16 @@ declare namespace NodeJS { ZITADEL_SERVICE_USER_TOKEN: string; /** - * Secret used to sign LoginV2 `sessions` cookie entries (a signing key is derived via HKDF). + * Secret used to sign LoginV2 `sessions` cookie entries, the `verificationCheck` cookie and the + * `linkFingerprint` of IdP linking flows (purpose-bound signing keys are derived via HKDF). * Recommended configuration; will become required in a future major release. * Every value must be at least 32 characters long (e.g. `openssl rand -base64 32`); a shorter value * disables signing and makes /ready fail instead of falling back to the API credential. * Deprecated fallback if unset: the configured API credential is used (ZITADEL_SERVICE_USER_TOKEN, * SYSTEM_USER_PRIVATE_KEY(_FILE) or ZITADEL_LOGINCLIENT_KEYFILE) and a warning is logged at startup. * A comma-separated list is supported for rotation: the first value signs, all values verify. - * Unsigned or tampered cookie entries are ignored so a client cannot patch an arbitrary session. + * Unsigned or tampered cookie entries are ignored so a client cannot patch an arbitrary session + * or claim a user verification (email / invite code) that never happened. * All replicas of a deployment must resolve the same secret. */ ZITADEL_SESSION_COOKIE_SECRET?: string; diff --git a/apps/login/src/lib/idp-link-proof.test.ts b/apps/login/src/lib/idp-link-proof.test.ts new file mode 100644 index 0000000000..4dba6ea7c9 --- /dev/null +++ b/apps/login/src/lib/idp-link-proof.test.ts @@ -0,0 +1,60 @@ +import { createHash } from "crypto"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { createIdpLinkProof, IDP_LINK_PROOF_MAX_AGE_SECONDS, isIdpLinkProofValid } from "./idp-link-proof"; +import { createVerificationCheck } from "./verification-check"; + +vi.mock("next/headers", () => ({ cookies: vi.fn() })); +vi.mock("./fingerprint", () => ({ getFingerprintIdCookie: vi.fn(), getOrSetFingerprintId: vi.fn() })); + +describe("idp-link-proof", () => { + beforeEach(() => { + vi.stubEnv("ZITADEL_SESSION_COOKIE_SECRET", "test-session-cookie-secret-at-least-32-chars"); + }); + + afterEach(() => { + vi.unstubAllEnvs(); + }); + + it("accepts a proof issued for the same session and fingerprint", () => { + const value = createIdpLinkProof("session-1", "fp-1"); + expect(isIdpLinkProofValid(value, "session-1", "fp-1")).toBe(true); + }); + + it("rejects a proof for another session or another fingerprint", () => { + const value = createIdpLinkProof("session-1", "fp-1"); + expect(isIdpLinkProofValid(value, "session-2", "fp-1")).toBe(false); + expect(isIdpLinkProofValid(value, "session-1", "fp-2")).toBe(false); + expect(isIdpLinkProofValid(value, undefined, "fp-1")).toBe(false); + }); + + it("rejects the legacy, client computable sha256(sessionId + fingerprintId) value", () => { + const forged = createHash("sha256") + .update("session-1" + "fp-1") + .digest("hex"); + expect(isIdpLinkProofValid(forged, "session-1", "fp-1")).toBe(false); + }); + + it("rejects a value signed with a secret the server does not know", () => { + const value = createIdpLinkProof("session-1", "fp-1"); + vi.stubEnv("ZITADEL_SESSION_COOKIE_SECRET", "another-secret-".padEnd(32, "x")); + expect(isIdpLinkProofValid(value, "session-1", "fp-1")).toBe(false); + }); + + it("enforces the expiry server side", () => { + const now = Date.now(); + const value = createIdpLinkProof("session-1", "fp-1", now); + expect(isIdpLinkProofValid(value, "session-1", "fp-1", now + (IDP_LINK_PROOF_MAX_AGE_SECONDS - 1) * 1000)).toBe(true); + expect(isIdpLinkProofValid(value, "session-1", "fp-1", now + (IDP_LINK_PROOF_MAX_AGE_SECONDS + 1) * 1000)).toBe(false); + }); + + it("is not interchangeable with a verification check for the same values", () => { + const now = Date.now(); + // a verification check with the same fields and the same expiry must not be accepted as link proof + const verificationCheck = createVerificationCheck( + "session-1", + "fp-1", + now + (IDP_LINK_PROOF_MAX_AGE_SECONDS - 300) * 1000, + ); + expect(isIdpLinkProofValid(verificationCheck, "session-1", "fp-1", now)).toBe(false); + }); +}); diff --git a/apps/login/src/lib/idp-link-proof.ts b/apps/login/src/lib/idp-link-proof.ts new file mode 100644 index 0000000000..489af3db76 --- /dev/null +++ b/apps/login/src/lib/idp-link-proof.ts @@ -0,0 +1,22 @@ +import { createSignedProof, isSignedProofValid } from "./signed-proof"; + +// covers the round trip to the external IdP, including a login (and MFA) there +export const IDP_LINK_PROOF_MAX_AGE_SECONDS = 900; // 15 minutes + +/** + * Creates the `linkFingerprint` passed through the IdP flow when a signed-in user links an IdP to + * their account. It binds the session to link to the browser that started the flow and is signed + * with a server-only secret, so it can't be computed by a client for another session. + */ +export function createIdpLinkProof(sessionId: string, fingerprintId: string, now: number = Date.now()): string { + return createSignedProof("idp-link", [sessionId, fingerprintId], IDP_LINK_PROOF_MAX_AGE_SECONDS, now); +} + +export function isIdpLinkProofValid( + value: string | undefined, + sessionId: string | undefined, + fingerprintId: string | undefined, + now: number = Date.now(), +): boolean { + return isSignedProofValid("idp-link", value, [sessionId, fingerprintId], now); +} diff --git a/apps/login/src/lib/server/idp-intent.test.ts b/apps/login/src/lib/server/idp-intent.test.ts index a8efa53791..6660a237e5 100644 --- a/apps/login/src/lib/server/idp-intent.test.ts +++ b/apps/login/src/lib/server/idp-intent.test.ts @@ -2,6 +2,7 @@ import { AutoLinkingOption } from "@zitadel/proto/zitadel/idp/v2/idp_pb"; import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import crypto from "crypto"; import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; +import { createIdpLinkProof } from "../idp-link-proof"; import { processIDPCallback } from "./idp-intent"; // Mock all the dependencies @@ -523,18 +524,16 @@ describe("processIDPCallback", () => { describe("CASE 2: Link IDP to existing user", () => { const sessionId = "session123"; const fingerprintValue = "fp123"; - const linkFingerprint = crypto - .createHash("sha256") - .update(sessionId + fingerprintValue) - .digest("hex"); - - const linkParams = { - ...defaultParams, - sessionId, - linkFingerprint, - }; + let linkParams: typeof defaultParams & { sessionId: string; linkFingerprint: string }; beforeEach(() => { + vi.stubEnv("ZITADEL_SESSION_COOKIE_SECRET", "test-session-cookie-secret-at-least-32-chars"); + linkParams = { + ...defaultParams, + sessionId, + linkFingerprint: createIdpLinkProof(sessionId, fingerprintValue), + }; + mockGetFingerprintIdCookie.mockResolvedValue({ name: "fingerprintId", value: fingerprintValue }); mockRetrieveIDPIntent.mockResolvedValue({ ...defaultIntent, @@ -552,6 +551,31 @@ describe("processIDPCallback", () => { }); }); + afterEach(() => { + vi.unstubAllEnvs(); + }); + + test.each([ + [ + "the legacy, client computable sha256(sessionId + fingerprint)", + () => + crypto + .createHash("sha256") + .update(sessionId + fingerprintValue) + .digest("hex"), + ], + ["a proof for another session", () => createIdpLinkProof("other-session", fingerprintValue)], + ["a proof for another browser", () => createIdpLinkProof(sessionId, "other-fp")], + ])("should reject %s as linkFingerprint", async (_name, makeLinkFingerprint) => { + mockGetIDPByID.mockResolvedValue(defaultIdp); + + const result = await processIDPCallback({ ...linkParams, linkFingerprint: makeLinkFingerprint() }); + + expect(result.redirect).toContain("/idp/google/linking-failed"); + expect(result.redirect).toContain("error=session_mismatch"); + expect(mockAddIDPLink).not.toHaveBeenCalled(); + }); + test("should link IDP and create session when linking is allowed", async () => { mockGetIDPByID.mockResolvedValue({ ...defaultIdp, diff --git a/apps/login/src/lib/server/idp-intent.ts b/apps/login/src/lib/server/idp-intent.ts index f9c4bb46cb..1e57672b1f 100644 --- a/apps/login/src/lib/server/idp-intent.ts +++ b/apps/login/src/lib/server/idp-intent.ts @@ -29,10 +29,10 @@ import { UpdateUserRequest, UpdateUserRequestSchema, } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; -import crypto from "crypto"; import { getTranslations } from "next-intl/server"; import { headers } from "next/headers"; import { getFingerprintIdCookie } from "../fingerprint"; +import { isIdpLinkProofValid } from "../idp-link-proof"; import { getEnrollmentAuthorizationError } from "./enrollment-guard"; import { createNewSessionFromIdpIntent } from "./idp"; import { syncInstanceRolesFromIdpIntent } from "./instance-roles"; @@ -358,12 +358,7 @@ async function handleExplicitLinking(ctx: IDPHandlerContext): Promise { beforeEach(async () => { vi.clearAllMocks(); + vi.stubEnv("ZITADEL_SESSION_COOKIE_SECRET", "test-session-cookie-secret-at-least-32-chars"); // Import mocked modules const { headers } = await import("next/headers"); @@ -76,6 +78,7 @@ describe("redirectToIdp", () => { afterEach(() => { vi.restoreAllMocks(); + vi.unstubAllEnvs(); }); describe("postErrorRedirectUrl parameter handling", () => { @@ -239,9 +242,10 @@ describe("redirectToIdp", () => { // Both parameters should be present // Both parameters should be present expect(successUrl).toContain("linkToSessionId=session123"); - // We can't easily predict the exact fingerprint hash without mocking crypto in this test, - // but we can check if the parameter key exists - expect(successUrl).toContain("linkFingerprint="); + // The linkFingerprint is a signed proof bound to the session and this browser's fingerprint + const linkFingerprint = new URL(successUrl).searchParams.get("linkFingerprint") ?? undefined; + expect(isIdpLinkProofValid(linkFingerprint, "session123", "fp123")).toBe(true); + expect(isIdpLinkProofValid(linkFingerprint, "other-session", "fp123")).toBe(false); expect(successUrl).toContain("postErrorRedirectUrl=%2Fcustom-error"); expect(failureUrl).toContain("linkToSessionId=session123"); expect(failureUrl).toContain("linkFingerprint="); diff --git a/apps/login/src/lib/server/idp.ts b/apps/login/src/lib/server/idp.ts index a043e6bcf6..e17daea817 100644 --- a/apps/login/src/lib/server/idp.ts +++ b/apps/login/src/lib/server/idp.ts @@ -8,12 +8,12 @@ import { startIdentityProviderFlow, startLDAPIdentityProviderFlow, } from "@/lib/zitadel"; -import crypto from "crypto"; import { headers } from "next/headers"; import { redirect } from "next/navigation"; import { completeFlowOrGetUrl } from "../client"; import { getSessionCookieById } from "../cookies"; import { getOrSetFingerprintId } from "../fingerprint"; +import { createIdpLinkProof } from "../idp-link-proof"; import { getServiceConfig } from "../service-url"; import { checkEmailVerification, checkMFAFactors } from "../verify-helper"; import { createSessionForIdpAndUpdateCookie } from "./cookie"; @@ -45,11 +45,8 @@ export async function redirectToIdp(prevState: RedirectToIdpState, formData: For // Get fingerprint (ensure it exists) const fingerprintId = await getOrSetFingerprintId(); - // Create hash to verify intent upon return - const linkFingerprint = crypto - .createHash("sha256") - .update(sessionId + fingerprintId) - .digest("hex"); + // Signed proof (server-only secret) to verify the intent upon return + const linkFingerprint = createIdpLinkProof(sessionId, fingerprintId); params.set("linkToSessionId", sessionId); params.set("linkFingerprint", linkFingerprint); diff --git a/apps/login/src/lib/server/register.ts b/apps/login/src/lib/server/register.ts index 9b927c0a9e..2ad5d190d1 100644 --- a/apps/login/src/lib/server/register.ts +++ b/apps/login/src/lib/server/register.ts @@ -5,13 +5,12 @@ import { addHumanUser, addIDPLink, getLoginSettings, getUserByID, listAuthentica import { Code, ConnectError, Duration, create } from "@zitadel/client"; import { Factors } from "@zitadel/proto/zitadel/session/v2/session_pb"; import { Checks, ChecksJson, ChecksSchema } from "@zitadel/proto/zitadel/session/v2/session_service_pb"; -import crypto from "crypto"; import { getTranslations } from "next-intl/server"; -import { cookies, headers } from "next/headers"; +import { headers } from "next/headers"; import { completeFlowOrGetUrl } from "../client"; -import { getOrSetFingerprintId } from "../fingerprint"; import { createLogger } from "../logger"; import { getServiceConfig } from "../service-url"; +import { setUserVerificationCheckCookie } from "../verification-check"; import { checkEmailVerification, checkMFAFactors } from "../verify-helper"; const logger = createLogger("register"); @@ -136,18 +135,7 @@ export async function registerUser( // Set verification cookie for users registering with passkey (no password) // This allows them to proceed with passkey registration without additional verification - const cookiesList = await cookies(); - const userAgentId = await getOrSetFingerprintId(); - - const verificationCheck = crypto.createHash("sha256").update(`${session.factors.user.id}:${userAgentId}`).digest("hex"); - - await cookiesList.set({ - name: "verificationCheck", - value: verificationCheck, - httpOnly: true, - path: "/", - maxAge: 300, // 5 minutes - }); + await setUserVerificationCheckCookie(session.factors.user.id); return { redirect: "/passkey/set?" + params }; } else { diff --git a/apps/login/src/lib/server/verify.test.ts b/apps/login/src/lib/server/verify.test.ts index 6a6c766f60..f24795f365 100644 --- a/apps/login/src/lib/server/verify.test.ts +++ b/apps/login/src/lib/server/verify.test.ts @@ -51,8 +51,8 @@ vi.mock("../service-url", () => ({ getServiceConfig: vi.fn(() => ({ serviceConfig: {} })), })); -vi.mock("../fingerprint", () => ({ - getOrSetFingerprintId: vi.fn(), +vi.mock("../verification-check", () => ({ + setUserVerificationCheckCookie: vi.fn(), })); vi.mock("./host", () => ({ @@ -131,6 +131,9 @@ describe("sendVerification", () => { expect(mockGetSession).toHaveBeenCalled(); // Verify creation was attempted expect(mockCreateSessionAndUpdateCookie).toHaveBeenCalled(); + // The signed verification check is issued for the verified user + const { setUserVerificationCheckCookie } = await import("../verification-check"); + expect(setUserVerificationCheckCookie).toHaveBeenCalledWith("user-1"); }); test("should include requestId in /authenticator/set redirect when provided", async () => { diff --git a/apps/login/src/lib/server/verify.ts b/apps/login/src/lib/server/verify.ts index 382c82a6e7..9175f97538 100644 --- a/apps/login/src/lib/server/verify.ts +++ b/apps/login/src/lib/server/verify.ts @@ -12,20 +12,19 @@ import { verifyTOTPRegistration, sendEmailCode as zitadelSendEmailCode, } from "@/lib/zitadel"; -import crypto from "crypto"; import { create } from "@zitadel/client"; import { Session } from "@zitadel/proto/zitadel/session/v2/session_pb"; import { ChecksSchema } from "@zitadel/proto/zitadel/session/v2/session_service_pb"; import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import { getTranslations } from "next-intl/server"; -import { cookies, headers } from "next/headers"; +import { headers } from "next/headers"; import { completeFlowOrGetUrl } from "../client"; import { getSessionCookieByLoginName } from "../cookies"; -import { getOrSetFingerprintId } from "../fingerprint"; import { checkMFAFactors } from "../mfa-helper"; import { getServiceConfig } from "../service-url"; import { loadMostRecentSession } from "../session"; +import { setUserVerificationCheckCookie } from "../verification-check"; import { createSessionAndUpdateCookie } from "./cookie"; import { getEnrollmentAuthorizationError } from "./enrollment-guard"; import { getPublicHostWithProtocol } from "./host"; @@ -172,19 +171,9 @@ export async function sendVerification(command: VerifyUserByEmailCommand) { params.set("requestId", command.requestId); } - // set hash of userId and userAgentId to prevent attacks, checks are done for users with invalid sessions and invalid userAgentId - const cookiesList = await cookies(); - const userAgentId = await getOrSetFingerprintId(); - - const verificationCheck = crypto.createHash("sha256").update(`${user.userId}:${userAgentId}`).digest("hex"); - - await cookiesList.set({ - name: "verificationCheck", - value: verificationCheck, - httpOnly: true, - path: "/", - maxAge: 300, // 5 minutes - }); + // remember the successful verification for this browser (signed with a server-only secret), + // checks are done for users with invalid sessions and invalid userAgentId + await setUserVerificationCheckCookie(user.userId); return { redirect: `/authenticator/set?${params}` }; } diff --git a/apps/login/src/lib/session-cookie-signature.ts b/apps/login/src/lib/session-cookie-signature.ts index 5e97ced7f1..9ae9d5e4b8 100644 --- a/apps/login/src/lib/session-cookie-signature.ts +++ b/apps/login/src/lib/session-cookie-signature.ts @@ -28,11 +28,17 @@ function readSecretFile(path: string): string | undefined { return fileCache.get(path); } -function deriveKey(secret: string): Buffer { - let key = derivedKeyCache.get(secret); +/** + * Derives a purpose-bound signing key from a secret returned by `getSessionCookieSecrets`. + * `info` provides the domain separation, so keys for different purposes (session cookie entries, + * user verification check, ...) are independent even though they share the same secret. + */ +export function deriveSigningKey(secret: string, info: string): Buffer { + const cacheKey = JSON.stringify([info, secret]); + let key = derivedKeyCache.get(cacheKey); if (!key) { - key = Buffer.from(hkdfSync("sha256", secret, "", HKDF_INFO, DERIVED_KEY_LENGTH)); - derivedKeyCache.set(secret, key); + key = Buffer.from(hkdfSync("sha256", secret, "", info, DERIVED_KEY_LENGTH)); + derivedKeyCache.set(cacheKey, key); } return key; } @@ -154,10 +160,25 @@ function canonicalPayload(session: SignableSession): string { } function computeSignature(session: SignableSession, secret: string): string { - return createHmac("sha256", deriveKey(secret)).update(canonicalPayload(session)).digest("base64url"); + return createHmac("sha256", deriveSigningKey(secret, HKDF_INFO)).update(canonicalPayload(session)).digest("base64url"); } -function signaturesEqual(left: string, right: string): boolean { +/** + * Returns the secret new signatures are created with (the first of `getSessionCookieSecrets`). + * Throws if no usable secret is configured. + */ +export function getPrimarySigningSecret(): string { + const [secret] = getSessionCookieSecrets(); + if (!secret) { + throw new Error( + getSessionCookieSecretConfigError() ?? + `Session cookie signing secret is not configured. Set ZITADEL_SESSION_COOKIE_SECRET (at least ${MIN_SESSION_COOKIE_SECRET_LENGTH} characters, e.g. \`openssl rand -base64 32\`).`, + ); + } + return secret; +} + +export function signaturesEqual(left: string, right: string): boolean { const leftBytes = Buffer.from(left); const rightBytes = Buffer.from(right); if (leftBytes.length !== rightBytes.length) { @@ -171,13 +192,7 @@ export function signSession(session: T): T & { sig: s throw new Error("Session cookie entries require a non-empty id and token to be signed."); } - const [secret] = getSessionCookieSecrets(); - if (!secret) { - throw new Error( - getSessionCookieSecretConfigError() ?? - "Session cookie signing secret is not configured. Set ZITADEL_SESSION_COOKIE_SECRET (at least 32 characters, e.g. `openssl rand -base64 32`).", - ); - } + const secret = getPrimarySigningSecret(); const { sig: _ignored, ...unsigned } = session; return { diff --git a/apps/login/src/lib/signed-proof.ts b/apps/login/src/lib/signed-proof.ts new file mode 100644 index 0000000000..561d73c3f0 --- /dev/null +++ b/apps/login/src/lib/signed-proof.ts @@ -0,0 +1,80 @@ +import { createHmac } from "crypto"; +import { + deriveSigningKey, + getPrimarySigningSecret, + getSessionCookieSecrets, + signaturesEqual, +} from "./session-cookie-signature"; + +const VERSION = "v1"; + +/** + * Purposes a signed proof can be issued for. Each purpose gets its own HKDF-derived key, so a proof + * issued for one purpose can never be replayed as a proof for another one. + */ +export type SignedProofPurpose = "verification-check" | "idp-link"; + +function computeSignature(secret: string, purpose: SignedProofPurpose, fields: string[], expiresAt: number): string { + // JSON array encoding keeps the fields unambiguous (no delimiter injection through any field) + const payload = JSON.stringify([VERSION, ...fields, expiresAt]); + // Domain separation: all proofs share the secret of the session cookie signature + // (ZITADEL_SESSION_COOKIE_SECRET or the API credential) but use their own derived key per purpose. + return createHmac("sha256", deriveSigningKey(secret, `zitadel-login-${purpose}-${VERSION}`)) + .update(payload) + .digest("base64url"); +} + +/** + * Creates a proof `v1..` binding `fields` together until `ttlSeconds` from now. + * The signature is an HMAC keyed with a server-only secret, so the proof can neither be forged by a + * client nor be reused for other field values or after expiry. The value is cookie and URL safe. + */ +export function createSignedProof( + purpose: SignedProofPurpose, + fields: string[], + ttlSeconds: number, + now: number = Date.now(), +): string { + if (fields.length === 0 || fields.some((field) => !field)) { + throw new Error(`All fields are required to create a ${purpose} proof.`); + } + + const secret = getPrimarySigningSecret(); + + const expiresAt = Math.floor(now / 1000) + ttlSeconds; + return `${VERSION}.${expiresAt}.${computeSignature(secret, purpose, fields, expiresAt)}`; +} + +export function isSignedProofValid( + purpose: SignedProofPurpose, + value: string | undefined, + fields: (string | undefined)[], + now: number = Date.now(), +): boolean { + if (!value || fields.length === 0 || fields.some((field) => !field)) { + return false; + } + + const parts = value.split("."); + if (parts.length !== 3) { + return false; + } + + const [version, expiresAtRaw, signature] = parts; + if (version !== VERSION || !/^\d{1,12}$/.test(expiresAtRaw) || !signature) { + return false; + } + + const expiresAt = Number(expiresAtRaw); + + // check every candidate secret (no early return) to keep timing independent of which one matches + let valid = false; + for (const secret of getSessionCookieSecrets()) { + if (signaturesEqual(signature, computeSignature(secret, purpose, fields as string[], expiresAt))) { + valid = true; + } + } + + // expiry is enforced server side, a cookie maxAge is only a hint for the browser + return valid && expiresAt > Math.floor(now / 1000); +} diff --git a/apps/login/src/lib/verification-check.test.ts b/apps/login/src/lib/verification-check.test.ts new file mode 100644 index 0000000000..5e584d9c43 --- /dev/null +++ b/apps/login/src/lib/verification-check.test.ts @@ -0,0 +1,141 @@ +import { createHash } from "crypto"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +const mockCookies = { get: vi.fn(), set: vi.fn() }; + +vi.mock("next/headers", () => ({ + cookies: vi.fn(async () => mockCookies), +})); + +vi.mock("./fingerprint", () => ({ + getFingerprintIdCookie: vi.fn(), + getOrSetFingerprintId: vi.fn(), +})); + +import { getFingerprintIdCookie, getOrSetFingerprintId } from "./fingerprint"; +import { MIN_SESSION_COOKIE_SECRET_LENGTH, signSession } from "./session-cookie-signature"; +import { + createVerificationCheck, + hasValidUserVerificationCheckCookie, + isVerificationCheckValid, + setUserVerificationCheckCookie, + VERIFICATION_CHECK_MAX_AGE_SECONDS, +} from "./verification-check"; + +const ENV_KEYS = [ + "ZITADEL_SESSION_COOKIE_SECRET", + "ZITADEL_SERVICE_USER_TOKEN", + "SYSTEM_USER_PRIVATE_KEY", + "SYSTEM_USER_PRIVATE_KEY_FILE", + "ZITADEL_LOGINCLIENT_KEYFILE", +] as const; + +describe("verification-check", () => { + const originalEnv: Record = {}; + + beforeEach(() => { + vi.clearAllMocks(); + for (const key of ENV_KEYS) { + originalEnv[key] = process.env[key]; + delete process.env[key]; + } + process.env.ZITADEL_SERVICE_USER_TOKEN = "service-user-token"; + }); + + afterEach(() => { + for (const key of ENV_KEYS) { + if (originalEnv[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = originalEnv[key]; + } + } + }); + + it("accepts a check issued for the same user and fingerprint", () => { + const value = createVerificationCheck("user-1", "fp-1"); + expect(isVerificationCheckValid(value, "user-1", "fp-1")).toBe(true); + }); + + it("rejects a check for another user or another fingerprint", () => { + const value = createVerificationCheck("user-1", "fp-1"); + expect(isVerificationCheckValid(value, "user-2", "fp-1")).toBe(false); + expect(isVerificationCheckValid(value, "user-1", "fp-2")).toBe(false); + }); + + it("rejects the legacy, client computable sha256(userId:fingerprintId) value", () => { + const forged = createHash("sha256").update("victim:attacker-fp").digest("hex"); + expect(isVerificationCheckValid(forged, "victim", "attacker-fp")).toBe(false); + }); + + it("rejects a value signed with a secret the server does not know", () => { + const value = createVerificationCheck("user-1", "fp-1"); + process.env.ZITADEL_SERVICE_USER_TOKEN = "another-token"; + expect(isVerificationCheckValid(value, "user-1", "fp-1")).toBe(false); + }); + + it("enforces the expiry server side and rejects a tampered expiry", () => { + const now = Date.now(); + const value = createVerificationCheck("user-1", "fp-1", now); + + expect(isVerificationCheckValid(value, "user-1", "fp-1", now + (VERIFICATION_CHECK_MAX_AGE_SECONDS - 1) * 1000)).toBe( + true, + ); + expect(isVerificationCheckValid(value, "user-1", "fp-1", now + (VERIFICATION_CHECK_MAX_AGE_SECONDS + 1) * 1000)).toBe( + false, + ); + + const [version, expiresAt, signature] = value.split("."); + const extended = `${version}.${Number(expiresAt) + 3600}.${signature}`; + expect(isVerificationCheckValid(extended, "user-1", "fp-1", now)).toBe(false); + }); + + it("rejects malformed values", () => { + for (const value of [undefined, "", "v1", "v1..", "v2.123.abc", "v1.abc.def", "a.b.c.d"]) { + expect(isVerificationCheckValid(value, "user-1", "fp-1")).toBe(false); + } + }); + + it("does not let field boundaries be shifted between userId and fingerprintId", () => { + const value = createVerificationCheck("user:1", "fp"); + expect(isVerificationCheckValid(value, "user", "1:fp")).toBe(false); + }); + + it("prefers the dedicated secret and supports rotation", () => { + const oldSecret = "old-secret-".padEnd(MIN_SESSION_COOKIE_SECRET_LENGTH, "o"); + const newSecret = "new-secret-".padEnd(MIN_SESSION_COOKIE_SECRET_LENGTH, "n"); + process.env.ZITADEL_SESSION_COOKIE_SECRET = oldSecret; + const value = createVerificationCheck("user-1", "fp-1"); + + process.env.ZITADEL_SESSION_COOKIE_SECRET = `${newSecret}, ${oldSecret}`; + expect(isVerificationCheckValid(value, "user-1", "fp-1")).toBe(true); + }); + + it("fails closed without any secret", () => { + delete (process.env as Record).ZITADEL_SERVICE_USER_TOKEN; + expect(() => createVerificationCheck("user-1", "fp-1")).toThrow(); + expect(isVerificationCheckValid("v1.9999999999.abc", "user-1", "fp-1")).toBe(false); + }); + + it("uses a key that is independent of the session cookie signature", () => { + const session = signSession({ id: "user-1", token: "fp-1" }); + const [, , signature] = createVerificationCheck("user-1", "fp-1").split("."); + expect(signature).not.toBe(session.sig); + }); + + it("round trips through the cookie helpers", async () => { + vi.mocked(getOrSetFingerprintId).mockResolvedValue("fp-1"); + await setUserVerificationCheckCookie("user-1"); + + expect(mockCookies.set).toHaveBeenCalledWith( + expect.objectContaining({ name: "verificationCheck", httpOnly: true, maxAge: VERIFICATION_CHECK_MAX_AGE_SECONDS }), + ); + + const { value } = mockCookies.set.mock.calls[0][0]; + mockCookies.get.mockReturnValue({ value }); + vi.mocked(getFingerprintIdCookie).mockResolvedValue({ name: "fingerprintId", value: "fp-1" } as any); + + expect(await hasValidUserVerificationCheckCookie("user-1")).toBe(true); + expect(await hasValidUserVerificationCheckCookie("user-2")).toBe(false); + }); +}); diff --git a/apps/login/src/lib/verification-check.ts b/apps/login/src/lib/verification-check.ts new file mode 100644 index 0000000000..ff7c7cc58f --- /dev/null +++ b/apps/login/src/lib/verification-check.ts @@ -0,0 +1,50 @@ +import { cookies } from "next/headers"; +import { getFingerprintIdCookie, getOrSetFingerprintId } from "./fingerprint"; +import { createSignedProof, isSignedProofValid } from "./signed-proof"; + +export const VERIFICATION_CHECK_COOKIE = "verificationCheck"; +export const VERIFICATION_CHECK_MAX_AGE_SECONDS = 300; // 5 minutes + +/** + * Creates the value of the verification check cookie: `v1..`. + * The signature is an HMAC over userId, fingerprintId and expiry, keyed with a server-only secret, + * so it can neither be forged by a client nor be reused for another user, browser or after expiry. + */ +export function createVerificationCheck(userId: string, fingerprintId: string, now: number = Date.now()): string { + return createSignedProof("verification-check", [userId, fingerprintId], VERIFICATION_CHECK_MAX_AGE_SECONDS, now); +} + +export function isVerificationCheckValid( + value: string | undefined, + userId: string, + fingerprintId: string | undefined, + now: number = Date.now(), +): boolean { + return isSignedProofValid("verification-check", value, [userId, fingerprintId], now); +} + +/** + * Marks the user as verified (email / invite code, or fresh registration) for this browser. + * Must only be called after a genuine, server-side validated verification. + */ +export async function setUserVerificationCheckCookie(userId: string): Promise { + const cookiesList = await cookies(); + const fingerprintId = await getOrSetFingerprintId(); + + await cookiesList.set({ + name: VERIFICATION_CHECK_COOKIE, + value: createVerificationCheck(userId, fingerprintId), + httpOnly: true, + path: "/", + maxAge: VERIFICATION_CHECK_MAX_AGE_SECONDS, + }); +} + +export async function hasValidUserVerificationCheckCookie(userId: string): Promise { + const cookiesList = await cookies(); + + // only read cookie to prevent issues on page.tsx + const fingerprintCookie = await getFingerprintIdCookie(); + + return isVerificationCheckValid(cookiesList.get(VERIFICATION_CHECK_COOKIE)?.value, userId, fingerprintCookie?.value); +} diff --git a/apps/login/src/lib/verify-helper.test.ts b/apps/login/src/lib/verify-helper.test.ts index a2939a3ae8..46e3601172 100644 --- a/apps/login/src/lib/verify-helper.test.ts +++ b/apps/login/src/lib/verify-helper.test.ts @@ -2,6 +2,7 @@ import crypto from "crypto"; import { cookies } from "next/headers"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { getFingerprintIdCookie } from "./fingerprint"; +import { createVerificationCheck } from "./verification-check"; import { AuthenticationMethodType } from "@zitadel/proto/zitadel/user/v2/user_service_pb"; import { @@ -349,21 +350,13 @@ vi.mock("next/headers", () => ({ vi.mock("./fingerprint", () => ({ getFingerprintIdCookie: vi.fn(), + getOrSetFingerprintId: vi.fn(), })); vi.mock("./zitadel", () => ({ getUserByID: vi.fn(), })); -vi.mock("crypto", () => ({ - default: { - createHash: vi.fn(() => ({ - update: vi.fn().mockReturnThis(), - digest: vi.fn(() => "mockedhash123"), - })), - }, -})); - describe("checkPasswordChangeRequired", () => { const mockSession: any = { id: "session-123", @@ -640,23 +633,30 @@ describe("checkEmailVerification", () => { describe("checkUserVerification", () => { let mockCookies: any; + const originalToken = process.env.ZITADEL_SERVICE_USER_TOKEN; beforeEach(() => { vi.clearAllMocks(); + vi.spyOn(console, "warn").mockImplementation(() => {}); + process.env.ZITADEL_SERVICE_USER_TOKEN = "server-only-test-secret"; mockCookies = { get: vi.fn(), }; vi.mocked(cookies).mockResolvedValue(mockCookies); }); - it("should return true if verification hash matches", async () => { + afterEach(() => { + process.env.ZITADEL_SERVICE_USER_TOKEN = originalToken; + }); + + it("should return true for a server issued verification check", async () => { vi.mocked(getFingerprintIdCookie).mockResolvedValue({ name: "fingerprintId", value: "fingerprint-123", } as any); mockCookies.get.mockReturnValue({ - value: "mockedhash123", + value: createVerificationCheck("user-123", "fingerprint-123"), }); const result = await checkUserVerification("user-123"); @@ -664,9 +664,43 @@ describe("checkUserVerification", () => { expect(result).toBe(true); }); + it("should return false for a verification check issued for another user", async () => { + vi.mocked(getFingerprintIdCookie).mockResolvedValue({ + name: "fingerprintId", + value: "fingerprint-123", + } as any); + + mockCookies.get.mockReturnValue({ + value: createVerificationCheck("attacker-user", "fingerprint-123"), + }); + + const result = await checkUserVerification("user-123"); + + expect(result).toBe(false); + }); + + it("should return false for a client computed sha256(userId:fingerprintId) hash", async () => { + vi.mocked(getFingerprintIdCookie).mockResolvedValue({ + name: "fingerprintId", + value: "attacker-chosen", + } as any); + + mockCookies.get.mockReturnValue({ + value: crypto.createHash("sha256").update("user-123:attacker-chosen").digest("hex"), + }); + + const result = await checkUserVerification("user-123"); + + expect(result).toBe(false); + }); + it("should return false if fingerprint cookie not found", async () => { vi.mocked(getFingerprintIdCookie).mockResolvedValue(undefined); + mockCookies.get.mockReturnValue({ + value: createVerificationCheck("user-123", "fingerprint-123"), + }); + const result = await checkUserVerification("user-123"); expect(result).toBe(false); @@ -696,34 +730,19 @@ describe("checkUserVerification", () => { expect(result).toBe(false); }); - it("should return false if verification hash does not match", async () => { + it("should not log the expected value on mismatch", async () => { + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); vi.mocked(getFingerprintIdCookie).mockResolvedValue({ name: "fingerprintId", value: "fingerprint-123", } as any); - mockCookies.get.mockReturnValue({ - value: "wronghash", - }); + mockCookies.get.mockReturnValue({ value: "wronghash" }); - const result = await checkUserVerification("user-123"); + expect(await checkUserVerification("user-123")).toBe(false); - expect(result).toBe(false); - }); - - it("should create hash from userId and fingerprint", async () => { - vi.mocked(getFingerprintIdCookie).mockResolvedValue({ - name: "fingerprintId", - value: "fingerprint-456", - } as any); - - mockCookies.get.mockReturnValue({ - value: "mockedhash123", - }); - - await checkUserVerification("user-456"); - - expect(crypto.createHash).toHaveBeenCalledWith("sha256"); + const expected = createVerificationCheck("user-123", "fingerprint-123").split(".")[2]; + expect(JSON.stringify(warn.mock.calls)).not.toContain(expected); }); }); diff --git a/apps/login/src/lib/verify-helper.ts b/apps/login/src/lib/verify-helper.ts index c8bc14c051..6ce8238072 100644 --- a/apps/login/src/lib/verify-helper.ts +++ b/apps/login/src/lib/verify-helper.ts @@ -2,11 +2,9 @@ import { timestampDate } from "@zitadel/client"; import { Session } from "@zitadel/proto/zitadel/session/v2/session_pb"; import { PasswordExpirySettings } from "@zitadel/proto/zitadel/settings/v2/password_settings_pb"; import { HumanUser } from "@zitadel/proto/zitadel/user/v2/user_pb"; -import crypto from "crypto"; import moment from "moment"; -import { cookies } from "next/headers"; -import { getFingerprintIdCookie } from "./fingerprint"; import { trySendVerification } from "./server/verify"; +import { hasValidUserVerificationCheckCookie } from "./verification-check"; export function checkPasswordChangeRequired( expirySettings: PasswordExpirySettings | undefined, @@ -111,29 +109,13 @@ export async function checkEmailVerification( export { checkMFAFactors, shouldEnforceMFA } from "./mfa-helper"; export async function checkUserVerification(userId: string): Promise { - // check if a verification was done earlier - const cookiesList = await cookies(); + // check if a verification was done earlier in this browser. The cookie value is an HMAC keyed with a + // server-only secret (see verification-check.ts), so it can't be computed by the client. + const valid = await hasValidUserVerificationCheckCookie(userId); - // only read cookie to prevent issues on page.tsx - const fingerPrintCookie = await getFingerprintIdCookie(); - - if (!fingerPrintCookie || !fingerPrintCookie.value) { - return false; + if (!valid) { + console.warn("User verification check failed."); } - const verificationCheck = crypto.createHash("sha256").update(`${userId}:${fingerPrintCookie.value}`).digest("hex"); - - const cookieValue = await cookiesList.get("verificationCheck")?.value; - - if (!cookieValue) { - console.warn("User verification check cookie not found. User verification check failed."); - return false; - } - - if (cookieValue !== verificationCheck) { - console.warn(`User verification check failed. Expected ${verificationCheck} but got ${cookieValue}`); - return false; - } - - return true; + return valid; }