Fix OCSP stapling on public certs

Author: pimterry

src/endpoints/tls/cert-modes.ts

@@ -25,7 +25,7 @@ export const revoked: TlsEndpoint = {
     },
     meta: {
         path: 'revoked',
-        description: 'Serves a revoked TLS certificate (reported via OCSP).',
+        description: 'Serves a revoked TLS certificate (by stapled OCSP for local root, or CRL for public root).',
         examples: ['https://revoked.testserver.host/'],
         group: tlsCertificateModes
     }

src/tls-certificates/local-ca.ts

@@ -260,6 +260,10 @@ export class LocalCA {
 
     private certInMemoryCache: { [domain: string]: LocallyGeneratedCertificate | undefined } = {};
 
+    // A self-signed cert is its own issuer, so it can answer OCSP for itself - but only while we
+    // still hold its key. Kept by serial, and dropped alongside the cached cert it belongs to.
+    private selfSignedCerts = new Map<string, { cert: x509.X509Certificate, key: CryptoKey }>();
+
     private intermediateCA?: Promise<CaMaterial>;
 
     private clientAuthCA?: Promise<CaMaterial>;
@@ -640,26 +644,48 @@ export class LocalCA {
 
         this.certInMemoryCache[cacheKey] = generatedCertificate;
 
+        if (options.selfSigned) {
+            this.selfSignedCerts.set(certificate.serialNumber, {
+                cert: certificate,
+                key: leafKeyPair.privateKey as CryptoKey
+            });
+        }
+
         setTimeout(() => {
             delete this.certInMemoryCache[cacheKey];
+            this.selfSignedCerts.delete(certificate.serialNumber);
         }, 1000 * 60 * 60 * 24).unref();
 
         return generatedCertificate;
     }
 
-    // An OCSP response must be signed by (and its CertID derived from) the cert's actual
-    // issuer. Non-self-signed leaves are issued by the intermediate, so resolve that;
-    // fall back to the root for anything else.
-    private async resolveOcspIssuer(
+    // An OCSP response must be signed by (and its CertID derived from) the cert's actual issuer,
+    // so we can only answer for certificates issued by a key we hold: one of our own CAs, or a
+    // self-signed cert we generated, which issued itself. Anything else (notably real ACME certs)
+    // gets no answer, rather than one naming an issuer unrelated to the served chain.
+    private async findOcspIssuer(
         cert: x509.X509Certificate
-    ): Promise<{ cert: x509.X509Certificate, key: CryptoKey }> {
-        if (this.intermediateCA) {
-            const intermediate = await this.intermediateCA;
-            if (cert.issuer === intermediate.cert.subject) {
-                return { cert: intermediate.cert, key: intermediate.key };
-            }
+    ): Promise<{ cert: x509.X509Certificate, key: CryptoKey } | undefined> {
+        const selfSigned = this.selfSignedCerts.get(cert.serialNumber);
+        if (selfSigned?.cert.equal(cert)) return selfSigned;
+
+        const candidates = [
+            ...(this.intermediateCA ? [await this.intermediateCA] : []),
+            { cert: this.caCert, key: this.caKey }
+        ];
+
+        for (const candidate of candidates) {
+            if (cert.issuer !== candidate.cert.subject) continue;
+
+            const signedByCandidate = await cert.verify({
+                publicKey: candidate.cert.publicKey,
+                signatureOnly: true
+            }).catch(() => false);
+
+            if (signedByCandidate) return { cert: candidate.cert, key: candidate.key };
         }
-        return { cert: this.caCert, key: this.caKey };
+
+        return undefined;
     }
 
     async getOcspResponse(certDer: Buffer): Promise<Buffer | null> {
@@ -672,7 +698,10 @@ export class LocalCA {
             return null;
         }
 
-        const { cert: issuerCert, key: issuerKey } = await this.resolveOcspIssuer(cert);
+        const issuer = await this.findOcspIssuer(cert);
+        if (!issuer) return null;
+
+        const { cert: issuerCert, key: issuerKey } = issuer;
 
         if (isRevokedCert(cert)) {
             // Certificate is revoked - return revoked OCSP response

src/tls-certificates/ocsp.ts

@@ -38,19 +38,29 @@ async function sha1Hash(data: BufferSource): Promise<ArrayBuffer> {
     return await crypto.subtle.digest('SHA-1', data);
 }
 
+// The CertID serial is a DER INTEGER, so a serial whose leading byte has the high bit set
+// needs a zero pad - without it the response identifies a negative, i.e. different, serial.
+function serialNumberBytes(cert: x509.X509Certificate): Uint8Array {
+    const serialHex = cert.serialNumber.replace(/\s/g, '');
+    const paddedHex = serialHex.length % 2 === 0 ? serialHex : `0${serialHex}`;
+
+    const bytes = Uint8Array.from(
+        paddedHex.match(/.{2}/g)!.map(byte => parseInt(byte, 16))
+    );
+
+    if ((bytes[0]! & 0x80) === 0) return bytes;
+
+    const signedBytes = new Uint8Array(bytes.length + 1);
+    signedBytes.set(bytes, 1);
+    return signedBytes;
+}
+
 function createCertId(
     cert: x509.X509Certificate,
-    issuerCert: x509.X509Certificate,
     issuerNameHash: ArrayBuffer,
     issuerKeyHash: ArrayBuffer
 ): asn1Ocsp.CertID {
-    // Get serial number as hex string and convert to bytes
-    const serialHex = cert.serialNumber;
-    // Remove any spaces and ensure even length
-    const cleanSerial = serialHex.replace(/\s/g, '');
-    const serialBytes = new Uint8Array(
-        cleanSerial.match(/.{1,2}/g)!.map(byte => parseInt(byte, 16))
-    );
+    const serialBytes = serialNumberBytes(cert);
 
     return new asn1Ocsp.CertID({
         hashAlgorithm: new asn1X509.AlgorithmIdentifier({
@@ -90,7 +100,7 @@ async function createSingleResponse(
     const thisUpdate = options.thisUpdate || new Date();
 
     const singleResponse = new asn1Ocsp.SingleResponse({
-        certID: createCertId(options.cert, options.issuerCert, issuerNameHash, issuerKeyHash),
+        certID: createCertId(options.cert, issuerNameHash, issuerKeyHash),
         certStatus: createCertStatus(options.status, options.revocationTime, options.revocationReason),
         thisUpdate
     });

src/tls-handler.ts

@@ -16,8 +16,6 @@ import { PROXY_PROTOCOL } from './proxy-protocol.js';
 import { TLS_CLIENT_HELLO } from './tls-client-hello.js';
 import { tlsConnectionsTotal } from './metrics.js';
 
-const secureContextCache = new SecureContextCache();
-
 function calculateContextCacheKey(
     domain: string,
     certOptions: CertOptions,
@@ -78,7 +76,11 @@ function proactivelyRefreshDomains(rootDomain: string, domains: string[], certGe
 }
 
 class TlsConnectionHandler {
-    
+
+    // Cached contexts are built from this handler's own CA, so they can't be shared with
+    // another handler - it would serve certs whose issuer it knows nothing about.
+    private secureContextCache = new SecureContextCache();
+
     // To keep Node happy, we need a TLS server attached to our sockets in some cases
     // to enable some features (like OCSP). This'll do:
     private ocspServer = new EventEmitter();
@@ -90,18 +92,14 @@ class TlsConnectionHandler {
         this.ocspServer.on('OCSPRequest', async (
             certificate: Buffer,
             _issuer: Buffer,
-            callback: (err: Error | null, response: Buffer) => void
+            callback: (err: Error | null, response?: Buffer) => void
         ) => {
             try {
                 const ocspResponse = await this.tlsConfig.localCA!.getOcspResponse(certificate);
-                if (ocspResponse) {
-                    callback(null, ocspResponse);
-                } else {
-                    callback(null, Buffer.alloc(0));
-                }
+                callback(null, ocspResponse ?? undefined);
             } catch (e) {
                 console.error('OCSP response generation error', e);
-                callback(null, Buffer.alloc(0));
+                callback(null, undefined);
             }
         });
     }
@@ -132,7 +130,7 @@ class TlsConnectionHandler {
             const cacheKey = calculateContextCacheKey(certDomain, certOptions, tlsOptions)
                 + (requireClientCert ? '|client-cert' : '');
 
-            const secureContext = await secureContextCache.getOrCreate(cacheKey, async () => {
+            const secureContext = await this.secureContextCache.getOrCreate(cacheKey, async () => {
                 const cert = await this.tlsConfig.generateCertificate(certDomain, certOptions);
 
                 const servedCert = certOptions.incompleteChain

test/ocsp-helpers.ts

@@ -0,0 +1,50 @@
+import * as x509 from '@peculiar/x509';
+import * as asn1Ocsp from '@peculiar/asn1-ocsp';
+import * as asn1X509 from '@peculiar/asn1-x509';
+import * as asn1Schema from '@peculiar/asn1-schema';
+
+const crypto = globalThis.crypto;
+
+// Read a CertID serial the way a client does: as a signed DER INTEGER, so that a missing
+// sign pad shows up as the negative (i.e. wrong) serial that it is.
+export function readSignedInteger(bytes: BufferSource): bigint {
+    const buffer = Buffer.from(ArrayBuffer.isView(bytes)
+        ? bytes.buffer.slice(bytes.byteOffset, bytes.byteOffset + bytes.byteLength)
+        : bytes
+    );
+
+    const magnitude = BigInt(`0x${buffer.toString('hex')}`);
+    return (buffer[0]! & 0x80)
+        ? magnitude - (1n << BigInt(buffer.length * 8))
+        : magnitude;
+}
+
+export function parseSingleResponse(response: Buffer) {
+    const parsed = asn1Schema.AsnConvert.parse(response, asn1Ocsp.OCSPResponse);
+    const basicResponse = asn1Schema.AsnConvert.parse(
+        parsed.responseBytes!.response.buffer,
+        asn1Ocsp.BasicOCSPResponse
+    );
+    return basicResponse.tbsResponseData.responses[0]!;
+}
+
+// The CertID hashes a client would calculate for certs issued by this issuer
+export async function certIdHashes(issuerCert: x509.X509Certificate) {
+    const asn1 = asn1Schema.AsnConvert.parse(issuerCert.rawData, asn1X509.Certificate);
+    const nameHash = await crypto.subtle.digest(
+        'SHA-1',
+        asn1Schema.AsnConvert.serialize(asn1.tbsCertificate.subject)
+    );
+    const keyHash = await crypto.subtle.digest(
+        'SHA-1',
+        new Uint8Array(asn1.tbsCertificate.subjectPublicKeyInfo.subjectPublicKey)
+    );
+    return {
+        nameHash: Buffer.from(nameHash).toString('hex'),
+        keyHash: Buffer.from(keyHash).toString('hex')
+    };
+}
+
+export function toDer(certPem: string) {
+    return Buffer.from(new x509.X509Certificate(certPem).rawData);
+}

test/ocsp.spec.ts

@@ -4,7 +4,9 @@ import * as asn1Ocsp from '@peculiar/asn1-ocsp';
 import * as asn1X509 from '@peculiar/asn1-x509';
 import * as asn1Schema from '@peculiar/asn1-schema';
 import { createOcspResponse, parseOcspRequest, RevocationReason } from '../src/tls-certificates/ocsp.js';
-import { generateCACertificate } from '../src/tls-certificates/local-ca.js';
+import { generateCACertificate, LocalCA } from '../src/tls-certificates/local-ca.js';
+import { extractLeafCertificate } from '../src/tls-certificates/cert-definitions.js';
+import { certIdHashes, parseSingleResponse, readSignedInteger, toDer } from './ocsp-helpers.js';
 
 const crypto = globalThis.crypto;
 
@@ -179,7 +181,26 @@ describe("OCSP response generation", () => {
 
         // Convert the serial number to hex for comparison
         const serialHex = Buffer.from(certId.serialNumber).toString('hex');
-        expect(serialHex.toLowerCase()).to.equal(cert.serialNumber.toLowerCase().replace(/\s/g, ''));
+        expect(serialHex.toLowerCase().replace(/^00/, ''))
+            .to.equal(cert.serialNumber.toLowerCase().replace(/\s/g, ''));
+    });
+
+    it("encodes a high-bit serial number as a positive integer", async () => {
+        // 0xA1... has its top bit set, so without a sign pad this reads back as a negative
+        // number - i.e. a CertID for a certificate other than the one being served.
+        expect(cert.serialNumber.toLowerCase()).to.equal('a123456789abcdef');
+
+        const response = await createOcspResponse({
+            cert,
+            issuerCert: caCert,
+            issuerKey: caKey,
+            status: 'good'
+        });
+
+        const { certID } = parseSingleResponse(response);
+
+        expect(readSignedInteger(certID.serialNumber))
+            .to.equal(BigInt(`0x${cert.serialNumber}`));
     });
 
     it("includes thisUpdate timestamp", async () => {
@@ -290,3 +311,86 @@ describe("OCSP response generation", () => {
         expect(parsed!.serialNumber).to.equal('010203');
     });
 });
+
+describe("OCSP responses from the local CA", () => {
+
+    let localCA: LocalCA;
+    let otherCA: LocalCA;
+
+    before(async () => {
+        localCA = await LocalCA.create(await generateCACertificate());
+        otherCA = await LocalCA.create(await generateCACertificate({ commonName: 'Other CA' }));
+    });
+
+    it("answers for a certificate it issued, naming the issuing intermediate", async () => {
+        const generated = await localCA.generateCertificate('example.testserver.host', {});
+        const leafPem = extractLeafCertificate(generated.cert);
+
+        const response = await localCA.getOcspResponse(toDer(leafPem));
+        expect(response).to.not.be.null;
+
+        const { certID } = parseSingleResponse(response!);
+        const leaf = new x509.X509Certificate(leafPem);
+        const intermediate = new x509.X509Certificate(await localCA.getIntermediateCertificatePem());
+        const expectedHashes = await certIdHashes(intermediate);
+
+        expect(leaf.issuer).to.equal(intermediate.subject);
+        expect(Buffer.from(certID.issuerNameHash.buffer).toString('hex')).to.equal(expectedHashes.nameHash);
+        expect(Buffer.from(certID.issuerKeyHash.buffer).toString('hex')).to.equal(expectedHashes.keyHash);
+        expect(readSignedInteger(certID.serialNumber)).to.equal(BigInt(`0x${leaf.serialNumber}`));
+    });
+
+    it("refuses to answer for a certificate issued by another CA", async () => {
+        // This is what a real ACME-issued cert looks like to us: we have no standing to say
+        // anything about it, so we must not staple a response naming one of our own CAs.
+        const foreignCert = await otherCA.generateCertificate('example.testserver.host', {});
+
+        const response = await localCA.getOcspResponse(toDer(extractLeafCertificate(foreignCert.cert)));
+        expect(response).to.be.null;
+    });
+
+    it("answers for a self-signed certificate it generated, naming the cert itself", async () => {
+        // A self-signed cert is its own issuer, so signing with its own key is exactly what
+        // RFC 6960 asks for - and we still hold that key.
+        const generated = await localCA.generateCertificate('example.testserver.host', {
+            selfSigned: true
+        });
+        const leafPem = extractLeafCertificate(generated.cert);
+
+        const response = await localCA.getOcspResponse(toDer(leafPem));
+        expect(response).to.not.be.null;
+
+        const { certID } = parseSingleResponse(response!);
+        const leaf = new x509.X509Certificate(leafPem);
+        const expectedHashes = await certIdHashes(leaf);
+
+        expect(leaf.issuer).to.equal(leaf.subject);
+        expect(Buffer.from(certID.issuerNameHash.buffer).toString('hex')).to.equal(expectedHashes.nameHash);
+        expect(Buffer.from(certID.issuerKeyHash.buffer).toString('hex')).to.equal(expectedHashes.keyHash);
+        expect(readSignedInteger(certID.serialNumber)).to.equal(BigInt(`0x${leaf.serialNumber}`));
+    });
+
+    it("reports a self-signed revoked certificate as revoked", async () => {
+        const generated = await localCA.generateCertificate('revoked.testserver.host', {
+            selfSigned: true
+        });
+
+        const response = await localCA.getOcspResponse(toDer(extractLeafCertificate(generated.cert)));
+        expect(response).to.not.be.null;
+
+        expect(parseSingleResponse(response!).certStatus.revoked).to.exist;
+    });
+
+    it("refuses to answer for a self-signed certificate generated elsewhere", async () => {
+        const foreignCert = await otherCA.generateCertificate('example.testserver.host', {
+            selfSigned: true
+        });
+
+        const response = await localCA.getOcspResponse(toDer(extractLeafCertificate(foreignCert.cert)));
+        expect(response).to.be.null;
+    });
+
+    it("refuses to answer for data that isn't a certificate", async () => {
+        expect(await localCA.getOcspResponse(Buffer.from('not a certificate'))).to.be.null;
+    });
+});