From f04feec273f3444343fdb6d530726d54894f1133 Mon Sep 17 00:00:00 2001 From: Juergen Kunz Date: Wed, 8 Apr 2026 00:56:02 +0000 Subject: [PATCH] fix(certificate-handler): preserve wildcard coverage during forced certificate renewals and propagate renewed certs to sibling domains --- changelog.md | 8 + test/test.cert-renewal.ts | 196 +++++++++++++++++++ ts/00_commitinfo_data.ts | 2 +- ts/opsserver/handlers/certificate.handler.ts | 126 +++++++++++- ts_web/00_commitinfo_data.ts | 2 +- 5 files changed, 331 insertions(+), 3 deletions(-) create mode 100644 test/test.cert-renewal.ts diff --git a/changelog.md b/changelog.md index 660843a..257bf7b 100644 --- a/changelog.md +++ b/changelog.md @@ -1,5 +1,13 @@ # Changelog +## 2026-04-08 - 13.1.3 - fix(certificate-handler) +preserve wildcard coverage during forced certificate renewals and propagate renewed certs to sibling domains + +- add deriveCertDomainName helper to match shared ACME certificate identities across wildcard and subdomain routes +- pass includeWildcard when force-renewing certificates so renewed certs keep wildcard SAN coverage for sibling subdomains +- persist renewed certificate data to all sibling route domains that share the same cert identity and clear cached certificate status entries +- add regression tests for certificate domain derivation and force-renew wildcard handling + ## 2026-04-07 - 13.1.2 - fix(deps) bump @serve.zone/catalog to ^2.12.3 diff --git a/test/test.cert-renewal.ts b/test/test.cert-renewal.ts new file mode 100644 index 0000000..60a6906 --- /dev/null +++ b/test/test.cert-renewal.ts @@ -0,0 +1,196 @@ +import { tap, expect } from '@git.zone/tstest/tapbundle'; +import { deriveCertDomainName } from '../ts/opsserver/handlers/certificate.handler.js'; + +// ────────────────────────────────────────────────────────────────────────────── +// deriveCertDomainName — pure helper that mirrors smartacme's certmatcher. +// Used by the force-renew sibling-propagation logic to identify which routes +// share a single underlying ACME certificate. +// ────────────────────────────────────────────────────────────────────────────── + +tap.test('deriveCertDomainName collapses 3-level subdomain to base', async () => { + expect(deriveCertDomainName('outline.task.vc')).toEqual('task.vc'); + expect(deriveCertDomainName('pr.task.vc')).toEqual('task.vc'); + expect(deriveCertDomainName('mtd.task.vc')).toEqual('task.vc'); +}); + +tap.test('deriveCertDomainName returns base domain unchanged for 2-level domain', async () => { + expect(deriveCertDomainName('task.vc')).toEqual('task.vc'); + expect(deriveCertDomainName('example.com')).toEqual('example.com'); +}); + +tap.test('deriveCertDomainName strips wildcard prefix', async () => { + expect(deriveCertDomainName('*.task.vc')).toEqual('task.vc'); + expect(deriveCertDomainName('*.example.com')).toEqual('example.com'); +}); + +tap.test('deriveCertDomainName collapses subdomain and wildcard to same identity', async () => { + // This is the core property: outline.task.vc and *.task.vc must yield + // the same cert identity, otherwise sibling propagation cannot work. + const subdomain = deriveCertDomainName('outline.task.vc'); + const wildcard = deriveCertDomainName('*.task.vc'); + expect(subdomain).toEqual(wildcard); +}); + +tap.test('deriveCertDomainName returns undefined for 4+ level domains', async () => { + // Matches smartacme's "deeper domains not supported" behavior. + expect(deriveCertDomainName('a.b.task.vc')).toBeUndefined(); + expect(deriveCertDomainName('one.two.three.example.com')).toBeUndefined(); +}); + +tap.test('deriveCertDomainName returns undefined for malformed inputs', async () => { + expect(deriveCertDomainName('vc')).toBeUndefined(); + expect(deriveCertDomainName('')).toBeUndefined(); +}); + +// ────────────────────────────────────────────────────────────────────────────── +// CertificateHandler.reprovisionCertificateDomain — verify the includeWildcard +// option is forwarded to smartAcme.getCertificateForDomain on force renew. +// +// This is the regression test for Bug 1: previously the call passed only +// `{ forceRenew: true }`, causing the re-issued cert to drop the wildcard SAN +// and break every sibling subdomain. +// ────────────────────────────────────────────────────────────────────────────── + +import { CertificateHandler } from '../ts/opsserver/handlers/certificate.handler.js'; + +// Build a minimal stub of OpsServer + DcRouter that satisfies CertificateHandler. +// We only need: viewRouter.addTypedHandler / adminRouter.addTypedHandler (no-op), +// dcRouterRef.smartProxy.routeManager.getRoutes(), dcRouterRef.smartAcme, +// dcRouterRef.findRouteNamesForDomain, dcRouterRef.certificateStatusMap. +function makeStubOpsServer(opts: { + routes: Array<{ name: string; domains: string[] }>; + smartAcmeStub: { getCertificateForDomain: (domain: string, options: any) => Promise }; +}) { + const captured: { typedHandlers: any[] } = { typedHandlers: [] }; + const router = { + addTypedHandler(handler: any) { captured.typedHandlers.push(handler); }, + }; + + const routes = opts.routes.map((r) => ({ + name: r.name, + match: { domains: r.domains, ports: 443 }, + action: { type: 'forward', tls: { certificate: 'auto' } }, + })); + + const dcRouterRef: any = { + smartProxy: { + routeManager: { getRoutes: () => routes }, + }, + smartAcme: opts.smartAcmeStub, + findRouteNamesForDomain: (domain: string) => + routes.filter((r) => r.match.domains.includes(domain)).map((r) => r.name), + certificateStatusMap: new Map(), + certProvisionScheduler: null, + routeConfigManager: null, + }; + + const opsServerRef: any = { + viewRouter: router, + adminRouter: router, + dcRouterRef, + }; + + return { opsServerRef, dcRouterRef, captured }; +} + +tap.test('reprovisionCertificateDomain passes includeWildcard=true for non-wildcard domain', async () => { + const calls: Array<{ domain: string; options: any }> = []; + + const { opsServerRef, dcRouterRef } = makeStubOpsServer({ + routes: [ + { name: 'outline-route', domains: ['outline.task.vc'] }, + { name: 'pr-route', domains: ['pr.task.vc'] }, + { name: 'mtd-route', domains: ['mtd.task.vc'] }, + ], + smartAcmeStub: { + getCertificateForDomain: async (domain: string, options: any) => { + calls.push({ domain, options }); + // Return a cert object shaped like SmartacmeCert + return { + id: 'test-id', + domainName: 'task.vc', + created: Date.now(), + validUntil: Date.now() + 90 * 24 * 60 * 60 * 1000, + privateKey: '-----BEGIN PRIVATE KEY-----\nfake\n-----END PRIVATE KEY-----', + publicKey: '-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----', + csr: '', + }; + }, + }, + }); + + // Override updateRoutes/applyRoutes to no-op so the test doesn't try to talk to a real proxy + dcRouterRef.smartProxy.updateRoutes = async () => {}; + + // Construct handler — registerHandlers will run and register typed handlers on our stub router. + const handler = new CertificateHandler(opsServerRef); + + // Invoke the private reprovision method directly. The Bug 1 fix is verified + // by inspecting the captured smartAcme call options regardless of whether + // sibling propagation succeeds (it relies on a real DB for ProxyCertDoc). + await (handler as any).reprovisionCertificateDomain('outline.task.vc', true); + + // Sibling propagation may fail because ProxyCertDoc.findByDomain needs a real DB. + // The Bug 1 fix is verified by the captured smartAcme call regardless. + expect(calls.length).toBeGreaterThanOrEqual(1); + expect(calls[0].domain).toEqual('outline.task.vc'); + expect(calls[0].options).toEqual({ forceRenew: true, includeWildcard: true }); +}); + +tap.test('reprovisionCertificateDomain passes includeWildcard=false for wildcard domain', async () => { + const calls: Array<{ domain: string; options: any }> = []; + + const { opsServerRef, dcRouterRef } = makeStubOpsServer({ + routes: [ + { name: 'wildcard-route', domains: ['*.task.vc'] }, + ], + smartAcmeStub: { + getCertificateForDomain: async (domain: string, options: any) => { + calls.push({ domain, options }); + return { + id: 'test-id', + domainName: 'task.vc', + created: Date.now(), + validUntil: Date.now() + 90 * 24 * 60 * 60 * 1000, + privateKey: '-----BEGIN PRIVATE KEY-----\nfake\n-----END PRIVATE KEY-----', + publicKey: '-----BEGIN CERTIFICATE-----\nfake\n-----END CERTIFICATE-----', + csr: '', + }; + }, + }, + }); + + dcRouterRef.smartProxy.updateRoutes = async () => {}; + + const handler = new CertificateHandler(opsServerRef); + await (handler as any).reprovisionCertificateDomain('*.task.vc', true); + + expect(calls.length).toBeGreaterThanOrEqual(1); + expect(calls[0].domain).toEqual('*.task.vc'); + expect(calls[0].options).toEqual({ forceRenew: true, includeWildcard: false }); +}); + +tap.test('reprovisionCertificateDomain does not call smartAcme when forceRenew is false', async () => { + const calls: Array<{ domain: string; options: any }> = []; + + const { opsServerRef, dcRouterRef } = makeStubOpsServer({ + routes: [{ name: 'outline-route', domains: ['outline.task.vc'] }], + smartAcmeStub: { + getCertificateForDomain: async (domain: string, options: any) => { + calls.push({ domain, options }); + return {} as any; + }, + }, + }); + + dcRouterRef.smartProxy.updateRoutes = async () => {}; + + const handler = new CertificateHandler(opsServerRef); + await (handler as any).reprovisionCertificateDomain('outline.task.vc', false); + + // forceRenew=false should NOT call getCertificateForDomain — it just triggers + // applyRoutes and lets the cert provisioning pipeline handle it. + expect(calls.length).toEqual(0); +}); + +export default tap.start(); diff --git a/ts/00_commitinfo_data.ts b/ts/00_commitinfo_data.ts index a1a9d59..da1b32c 100644 --- a/ts/00_commitinfo_data.ts +++ b/ts/00_commitinfo_data.ts @@ -3,6 +3,6 @@ */ export const commitinfo = { name: '@serve.zone/dcrouter', - version: '13.1.2', + version: '13.1.3', description: 'A multifaceted routing service handling mail and SMS delivery functions.' } diff --git a/ts/opsserver/handlers/certificate.handler.ts b/ts/opsserver/handlers/certificate.handler.ts index 351d898..04e8d01 100644 --- a/ts/opsserver/handlers/certificate.handler.ts +++ b/ts/opsserver/handlers/certificate.handler.ts @@ -2,6 +2,28 @@ import * as plugins from '../../plugins.js'; import type { OpsServer } from '../classes.opsserver.js'; import * as interfaces from '../../../ts_interfaces/index.js'; import { AcmeCertDoc, ProxyCertDoc } from '../../db/index.js'; +import { logger } from '../../logger.js'; + +/** + * Mirrors `SmartacmeCertMatcher.getCertificateDomainNameByDomainName` from + * @push.rocks/smartacme. Inlined here because the original is `private` on + * SmartAcme. The cert identity ('task.vc' for both 'outline.task.vc' and + * '*.task.vc') is what AcmeCertDoc is keyed by, so two route domains with + * the same identity share the same underlying ACME cert. + * + * Returns undefined for domains with 4+ levels (matching smartacme's + * "deeper domains not supported" behavior) and for malformed inputs. + * + * Exported for unit testing. + */ +export function deriveCertDomainName(domain: string): string | undefined { + if (domain.startsWith('*.')) { + return domain.slice(2); + } + const parts = domain.split('.'); + if (parts.length < 2 || parts.length > 3) return undefined; + return parts.slice(-2).join('.'); +} export class CertificateHandler { constructor(private opsServerRef: OpsServer) { @@ -363,12 +385,34 @@ export class CertificateHandler { // If forceRenew, order a fresh cert from ACME now so it's already in // AcmeCertDoc by the time certProvisionFunction is invoked below. + // + // includeWildcard: when forcing a non-wildcard subdomain renewal, we still + // want the wildcard SAN in the order so the new cert keeps covering every + // sibling. Without this, smartacme defaults to includeWildcard: false and + // the re-issued cert would have only the base domain as SAN, breaking every + // sibling subdomain that was previously covered by the same wildcard cert. if (forceRenew && dcRouter.smartAcme) { + let newCert: plugins.smartacme.Cert; try { - await dcRouter.smartAcme.getCertificateForDomain(domain, { forceRenew: true }); + newCert = await dcRouter.smartAcme.getCertificateForDomain(domain, { + forceRenew: true, + includeWildcard: !domain.startsWith('*.'), + }); } catch (err: unknown) { return { success: false, message: `Failed to renew certificate for ${domain}: ${(err as Error).message}` }; } + + // Propagate the freshly-issued cert PEM to every sibling route domain that + // shares the same cert identity. Without this, the rust hot-swap (keyed by + // exact domain in `loaded_certs`) only fires for the clicked route via the + // fire-and-forget cert provisioning path, leaving siblings serving the + // stale in-memory cert until the next background reload completes. + try { + await this.propagateCertToSiblings(domain, newCert); + } catch (err: unknown) { + // Best-effort: failure here doesn't undo the cert issuance, just log. + logger.log('warn', `Failed to propagate force-renewed cert to siblings of ${domain}: ${(err as Error).message}`); + } } // Clear status map entry so it gets refreshed by the certificate-issued event @@ -392,6 +436,86 @@ export class CertificateHandler { } } + /** + * After a force-renew, walk every route in the smartproxy that resolves to + * the same cert identity as `forcedDomain` and write the freshly-issued cert + * PEM into ProxyCertDoc for each. This guarantees that the next applyRoutes + * → provisionCertificatesViaCallback iteration will hot-swap every sibling's + * rust loaded_certs entry with the new (correct) PEM, rather than relying on + * the in-memory cert returned by smartacme's per-domain cache. + * + * Why this is necessary: + * Rust's `loaded_certs` is a HashMap. Each + * bridge.loadCertificate(domain, ...) only swaps that one entry. The + * fire-and-forget cert provisioning path triggered by updateRoutes does + * eventually iterate every auto-cert route, but it returns the cached + * (broken pre-fix) cert from smartacme's per-domain mutex. With this + * helper, ProxyCertDoc is updated synchronously to the correct PEM before + * applyRoutes runs, so even the transient window stays consistent. + */ + private async propagateCertToSiblings( + forcedDomain: string, + newCert: plugins.smartacme.Cert, + ): Promise { + const dcRouter = this.opsServerRef.dcRouterRef; + const smartProxy = dcRouter.smartProxy; + if (!smartProxy) return; + + const certIdentity = deriveCertDomainName(forcedDomain); + if (!certIdentity) return; + + // Collect every route domain whose cert identity matches. + const affected = new Set(); + for (const route of smartProxy.routeManager.getRoutes()) { + if (!route.match.domains) continue; + const routeDomains = Array.isArray(route.match.domains) + ? route.match.domains + : [route.match.domains]; + for (const routeDomain of routeDomains) { + if (deriveCertDomainName(routeDomain) === certIdentity) { + affected.add(routeDomain); + } + } + } + + if (affected.size === 0) return; + + // Parse expiry from PEM (defense-in-depth — same pattern as + // ts/classes.dcrouter.ts:988-995 and the existing certStore.save callback). + let validUntil = newCert.validUntil; + let validFrom: number | undefined; + if (newCert.publicKey) { + try { + const x509 = new plugins.crypto.X509Certificate(newCert.publicKey); + validUntil = new Date(x509.validTo).getTime(); + validFrom = new Date(x509.validFrom).getTime(); + } catch { /* fall back to smartacme's value */ } + } + + // Persist new cert PEM under each affected route domain + for (const routeDomain of affected) { + let doc = await ProxyCertDoc.findByDomain(routeDomain); + if (!doc) { + doc = new ProxyCertDoc(); + doc.domain = routeDomain; + } + doc.publicKey = newCert.publicKey; + doc.privateKey = newCert.privateKey; + doc.ca = ''; + doc.validUntil = validUntil || 0; + doc.validFrom = validFrom || 0; + await doc.save(); + + // Clear status so the next event refresh shows the new cert + dcRouter.certificateStatusMap.delete(routeDomain); + } + + logger.log( + 'info', + `Propagated force-renewed cert for ${forcedDomain} (cert identity '${certIdentity}') to ${affected.size} sibling route domain(s): ${[...affected].join(', ')}`, + ); + } + /** * Delete certificate data for a domain from storage */ diff --git a/ts_web/00_commitinfo_data.ts b/ts_web/00_commitinfo_data.ts index a1a9d59..da1b32c 100644 --- a/ts_web/00_commitinfo_data.ts +++ b/ts_web/00_commitinfo_data.ts @@ -3,6 +3,6 @@ */ export const commitinfo = { name: '@serve.zone/dcrouter', - version: '13.1.2', + version: '13.1.3', description: 'A multifaceted routing service handling mail and SMS delivery functions.' }