From 9fa57bcf2882554974800afe09d2a8d1c15e8bf6 Mon Sep 17 00:00:00 2001 From: fuomag9 <1580624+fuomag9@users.noreply.github.com> Date: Fri, 6 Mar 2026 18:21:48 +0100 Subject: [PATCH] fix mTLS: use trusted_leaf_certs for issued certs, surface CA delete errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs fixed: 1. buildClientAuthentication was placing issued leaf cert PEMs into trusted_ca_certs. Caddy uses that field for CA chain validation, not leaf pinning — putting leaf certs there made chain verification fail for every presented client cert, causing the browser to be asked repeatedly. Fixed by using trusted_leaf_certs for managed CAs. 2. If all issued certs for a CA were revoked, the active cert map would be empty and the code fell back to trusting the CA cert directly, effectively un-revoking everything. Fixed by tracking which CAs have ever had issued certs (including revoked) and keeping them in trusted_leaf_certs mode permanently (empty list = no one trusted). Also fix CA certificate delete action not surfacing the error message to the user in production (Next.js strips thrown error messages in server actions). Changed to return { success, error } and updated the client dialog to check the result instead of using try/catch. Co-Authored-By: Claude Sonnet 4.6 --- app/(dashboard)/certificates/ca-actions.ts | 11 ++- .../ca-certificates/CaCertDialogs.tsx | 8 +- src/lib/caddy.ts | 75 ++++++++++++------- 3 files changed, 59 insertions(+), 35 deletions(-) diff --git a/app/(dashboard)/certificates/ca-actions.ts b/app/(dashboard)/certificates/ca-actions.ts index 6f92e137..e00b3255 100644 --- a/app/(dashboard)/certificates/ca-actions.ts +++ b/app/(dashboard)/certificates/ca-actions.ts @@ -46,11 +46,16 @@ export async function updateCaCertificateAction(id: number, formData: FormData) revalidatePath("/certificates"); } -export async function deleteCaCertificateAction(id: number) { +export async function deleteCaCertificateAction(id: number): Promise<{ success: boolean; error?: string }> { const session = await requireAdmin(); const userId = Number(session.user.id); - await deleteCaCertificate(id, userId); - revalidatePath("/certificates"); + try { + await deleteCaCertificate(id, userId); + revalidatePath("/certificates"); + return { success: true }; + } catch (e) { + return { success: false, error: e instanceof Error ? e.message : "Failed to delete CA certificate" }; + } } export async function generateCaCertificateAction(formData: FormData): Promise<{ id: number }> { diff --git a/src/components/ca-certificates/CaCertDialogs.tsx b/src/components/ca-certificates/CaCertDialogs.tsx index 049c9a6a..270dfb3a 100644 --- a/src/components/ca-certificates/CaCertDialogs.tsx +++ b/src/components/ca-certificates/CaCertDialogs.tsx @@ -518,11 +518,11 @@ export function DeleteCaCertDialog({ function handleDelete() { setError(null); startTransition(async () => { - try { - await deleteCaCertificateAction(cert.id); + const result = await deleteCaCertificateAction(cert.id); + if (result.success) { onClose(); - } catch (e) { - setError(e instanceof Error ? e.message : "Failed to delete"); + } else { + setError(result.error ?? "Failed to delete"); } }); } diff --git a/src/lib/caddy.ts b/src/lib/caddy.ts index 26b5f5bf..54fe2ceb 100644 --- a/src/lib/caddy.ts +++ b/src/lib/caddy.ts @@ -6,7 +6,7 @@ import crypto from "node:crypto"; import http from "node:http"; import https from "node:https"; import db, { nowIso } from "./db"; -import { isNull } from "drizzle-orm"; +import { isNull, isNotNull } from "drizzle-orm"; import { config } from "./config"; import { getCloudflareSettings, @@ -1320,11 +1320,10 @@ function buildClientAuthentication( domains: string[], mTlsDomainMap: Map, caCertMap: Map, - issuedClientCertMap: Map + issuedClientCertMap: Map, + cAsWithAnyIssuedCerts: Set ): Record | null { // Collect all CA cert IDs for any domain in this policy that has mTLS. - // If a CA has managed issued client certs, trust only the active leaf certs - // for that CA so revocation can be enforced by removing them from the pool. const caCertIds = new Set(); for (const domain of domains) { const ids = mTlsDomainMap.get(domain.toLowerCase()); @@ -1334,26 +1333,35 @@ function buildClientAuthentication( } if (caCertIds.size === 0) return null; - const derCerts: string[] = []; + // For CAs that have never issued tracked certs: trust any cert signed by that CA. + // For CAs that have issued tracked certs: pin to only the active (non-revoked) leaf + // certs so revocation is enforced. trusted_leaf_certs bypasses chain validation and + // accepts only those exact certificates — do NOT also put the CA in trusted_ca_certs + // or that would allow any cert signed by it (defeating revocation). + const trustedCaCerts: string[] = []; + const trustedLeafCerts: string[] = []; + for (const id of caCertIds) { - const issuedLeafCerts = issuedClientCertMap.get(id) ?? []; - if (issuedLeafCerts.length > 0) { - for (const certPem of issuedLeafCerts) { - derCerts.push(pemToBase64Der(certPem)); + if (cAsWithAnyIssuedCerts.has(id)) { + // Managed CA: only trust active leaf certs (revoked ones are absent from the map) + const activeLeafCerts = issuedClientCertMap.get(id) ?? []; + for (const certPem of activeLeafCerts) { + trustedLeafCerts.push(pemToBase64Der(certPem)); } - continue; + } else { + // Unmanaged CA: trust any cert in the chain + const ca = caCertMap.get(id); + if (!ca) continue; + trustedCaCerts.push(pemToBase64Der(ca.certificatePem)); } - - const ca = caCertMap.get(id); - if (!ca) continue; - derCerts.push(pemToBase64Der(ca.certificatePem)); } - if (derCerts.length === 0) return null; - return { - trusted_ca_certs: derCerts, - mode: "require_and_verify" - }; + if (trustedCaCerts.length === 0 && trustedLeafCerts.length === 0) return null; + + const result: Record = { mode: "require_and_verify" }; + if (trustedCaCerts.length > 0) result.trusted_ca_certs = trustedCaCerts; + if (trustedLeafCerts.length > 0) result.trusted_leaf_certs = trustedLeafCerts; + return result; } function buildTlsConnectionPolicies( @@ -1362,16 +1370,20 @@ function buildTlsConnectionPolicies( autoManagedDomains: Set, mTlsDomainMap: Map, caCertMap: Map, - issuedClientCertMap: Map + issuedClientCertMap: Map, + cAsWithAnyIssuedCerts: Set ) { const policies: Record[] = []; const readyCertificates = new Set(); const importedCertPems: { certificate: string; key: string }[] = []; + const buildAuth = (domains: string[]) => + buildClientAuthentication(domains, mTlsDomainMap, caCertMap, issuedClientCertMap, cAsWithAnyIssuedCerts); + // Add policy for auto-managed domains (certificate_id = null) if (autoManagedDomains.size > 0) { const domains = Array.from(autoManagedDomains); - const clientAuth = buildClientAuthentication(domains, mTlsDomainMap, caCertMap, issuedClientCertMap); + const clientAuth = buildAuth(domains); if (clientAuth) { // Split: mTLS domains get their own policy, non-mTLS get another @@ -1379,10 +1391,9 @@ function buildTlsConnectionPolicies( const nonMTlsDomains = domains.filter(d => !mTlsDomainMap.has(d)); if (mTlsDomains.length > 0) { - const mTlsAuth = buildClientAuthentication(mTlsDomains, mTlsDomainMap, caCertMap, issuedClientCertMap); policies.push({ match: { sni: mTlsDomains }, - client_authentication: mTlsAuth + client_authentication: buildAuth(mTlsDomains) }); } if (nonMTlsDomains.length > 0) { @@ -1414,7 +1425,7 @@ function buildTlsConnectionPolicies( const nonMTlsDomains = domains.filter(d => !mTlsDomainMap.has(d)); if (mTlsDomains.length > 0) { - const mTlsAuth = buildClientAuthentication(mTlsDomains, mTlsDomainMap, caCertMap, issuedClientCertMap); + const mTlsAuth = buildAuth(mTlsDomains); policies.push({ match: { sni: mTlsDomains }, ...(mTlsAuth ? { client_authentication: mTlsAuth } : {}) @@ -1437,7 +1448,7 @@ function buildTlsConnectionPolicies( const nonMTlsDomains = domains.filter(d => !mTlsDomainMap.has(d)); if (mTlsDomains.length > 0) { - const mTlsAuth = buildClientAuthentication(mTlsDomains, mTlsDomainMap, caCertMap, issuedClientCertMap); + const mTlsAuth = buildAuth(mTlsDomains); policies.push({ match: { sni: mTlsDomains }, ...(mTlsAuth ? { client_authentication: mTlsAuth } : {}) @@ -1595,7 +1606,7 @@ async function buildTlsAutomation( } async function buildCaddyDocument() { - const [proxyHostRecords, certRows, accessListEntryRecords, caCertRows, issuedClientCertRows] = await Promise.all([ + const [proxyHostRecords, certRows, accessListEntryRecords, caCertRows, issuedClientCertRows, allIssuedCaCertIds] = await Promise.all([ db .select({ id: proxyHosts.id, @@ -1645,7 +1656,13 @@ async function buildCaddyDocument() { certificatePem: issuedClientCertificates.certificatePem }) .from(issuedClientCertificates) - .where(isNull(issuedClientCertificates.revokedAt)) + .where(isNull(issuedClientCertificates.revokedAt)), + // Distinct CA IDs that have ever had a tracked issued cert (including revoked). + // Used to distinguish "managed" CAs (pin to leaf certs) from "unmanaged" CAs + // (trust any cert signed by that CA). + db + .selectDistinct({ caCertificateId: issuedClientCertificates.caCertificateId }) + .from(issuedClientCertificates) ]); const proxyHostRows: ProxyHostRow[] = proxyHostRecords.map((h) => ({ @@ -1690,6 +1707,7 @@ async function buildCaddyDocument() { map.set(record.caCertificateId, current); return map; }, new Map()); + const cAsWithAnyIssuedCerts = new Set(allIssuedCaCertIds.map(r => r.caCertificateId)); const accessMap = accessListEntryRows.reduce>((map, entry) => { if (!map.has(entry.access_list_id)) { map.set(entry.access_list_id, []); @@ -1728,7 +1746,8 @@ async function buildCaddyDocument() { autoManagedDomains, mTlsDomainMap, caCertMap, - issuedClientCertMap + issuedClientCertMap, + cAsWithAnyIssuedCerts ); const httpRoutes: CaddyHttpRoute[] = await buildProxyRoutes(