fix: retry certificate generation on serial numbers OpenSSL rejects (#2)

Author: ViniciusDev26

generate.js

@@ -0,0 +1,74 @@
+'use strict'
+
+const { X509Certificate } = require('node:crypto')
+const selfsigned = require('selfsigned')
+
+// `selfsigned` derives the certificate serial number from 9 random bytes and
+// runs them through its own `toPositiveHex()`, which clears the sign bit but
+// does not re-minimise the resulting DER INTEGER. Roughly 1 in 65536 draws end
+// up with two redundant leading zero bytes, and node-forge's encoder strips
+// only one of them (see the "should all leading bytes be stripped vs just one?"
+// TODO in its `asn1.js`), so the serial goes out as a positive INTEGER with
+// illegal padding.
+//
+// node-forge's own parser accepts that encoding, so the
+// `verifyCertificateChain()` check `selfsigned` runs before returning passes
+// and the pair looks fine. OpenSSL rejects it, so the certificate only blows
+// up later, as ERR_OSSL_ASN1_ILLEGAL_PADDING from the middle of a TLS
+// handshake. That made every consumer building a server from a freshly
+// generated pair intermittently fail (nodejs/undici#5245).
+//
+// Each serial number is drawn independently, so generating again is enough to
+// get past it: three attempts bring the odds down to about 1 in 2.8e14. All of
+// this can go away once `selfsigned` emits minimally encoded serial numbers.
+const ATTEMPTS = 3
+
+// Returns the error OpenSSL refused the certificate with, or `null` if it
+// loads. node-forge parsing it successfully says nothing about OpenSSL.
+function loadError (cert) {
+  try {
+    new X509Certificate(cert) // eslint-disable-line no-new
+    return null
+  } catch (err) {
+    return err
+  }
+}
+
+function unusable (cause) {
+  return new Error(
+    `could not generate a certificate OpenSSL can load in ${ATTEMPTS} attempts`,
+    { cause }
+  )
+}
+
+function generateSync (attrs, opts) {
+  let lastError
+
+  for (let i = 0; i < ATTEMPTS; i++) {
+    const pems = selfsigned.generate(attrs, opts)
+    const err = loadError(pems.cert)
+
+    if (err === null) return pems
+
+    lastError = err
+  }
+
+  throw unusable(lastError)
+}
+
+function generate (attrs, opts, done) {
+  let remaining = ATTEMPTS
+
+  selfsigned.generate(attrs, opts, function onPems (err, pems) {
+    if (err) return done(err)
+
+    const loadErr = loadError(pems.cert)
+
+    if (loadErr === null) return done(null, pems)
+    if (--remaining > 0) return selfsigned.generate(attrs, opts, onPems)
+
+    done(unusable(loadErr))
+  })
+}
+
+module.exports = { generate, generateSync }

index.js

@@ -3,7 +3,7 @@
 const path = require('node:path')
 const fs = require('node:fs')
 
-let selfsigned
+let generator
 const keyPath = path.join(__dirname, 'key.pem')
 const certPath = path.join(__dirname, 'cert.pem')
 
@@ -25,7 +25,7 @@ function generate ({ attr, opts } = { attr: [], opts: null }, done) {
     return promise
   }
 
-  if (!selfsigned) selfsigned = require('selfsigned')
+  if (!generator) generator = require('./generate')
 
-  selfsigned.generate(attr, opts, done)
+  generator.generate(attr, opts, done)
 }

install.js

@@ -2,15 +2,15 @@
 
 const fs = require('node:fs')
 const path = require('node:path')
-const selfsigned = require('selfsigned')
+const { generateSync } = require('./generate')
 
 const nodeVersion = Number(process.versions.node.split('.')[0])
 const pems =
   // Due to new version of openssl, we need to use a larger key size
   // for node 24 and above, otherwise the default key size is sufficient
   nodeVersion > 22
-    ? selfsigned.generate({}, { keySize: 2048 })
-    : selfsigned.generate()
+    ? generateSync({}, { keySize: 2048 })
+    : generateSync()
 
 fs.writeFileSync(path.join(__dirname, 'key.pem'), pems.private)
 fs.writeFileSync(path.join(__dirname, 'cert.pem'), pems.cert)

package.json

@@ -53,6 +53,7 @@
   },
   "devDependencies": {
     "@types/node": "^24.2.0",
+    "node-forge": "^1.3.1",
     "snazzy": "^9.0.0",
     "standard": "^17.0.0",
     "standard-version": "^9.5.0",

tests/index.js

@@ -3,8 +3,11 @@
 const https = require('node:https')
 const { once } = require('node:events')
 const { test } = require('node:test')
+const { X509Certificate } = require('node:crypto')
 
 const { Agent } = require('undici')
+const selfsigned = require('selfsigned')
+const forge = require('node-forge')
 
 const client = new Agent({
   connect: {
@@ -58,3 +61,132 @@ test('https-pem (generate)', async t => {
   t.assert.strictEqual(response.statusCode, 200)
   t.assert.strictEqual(await response.body.text(), 'foo')
 })
+
+// `selfsigned` clears the sign bit of the 9 random bytes it draws for the
+// serial number without re-minimising the DER INTEGER, and node-forge strips
+// only one of the redundant leading zero bytes. These 9 bytes are one of the
+// roughly 1 in 65536 draws that come out as `00 00 01 ...` and leave a
+// positive INTEGER with illegal padding, which OpenSSL refuses to load.
+const ILLEGAL_PADDING_SEED = '\x80\x00\x01\x02\x03\x04\x05\x06\x07'
+
+// Turns the next `attempts` serial numbers into the pathological one above.
+// Returns a getter for how many of them were actually drawn, so a test can
+// tell whether it exercised the bad path at all.
+function forceIllegalSerialNumber (t, attempts = 1) {
+  const getBytesSync = forge.random.getBytesSync
+  let remaining = attempts
+
+  forge.random.getBytesSync = function (count) {
+    // 9 bytes are only ever drawn for the serial number
+    if (count === 9 && remaining > 0) {
+      remaining--
+      return ILLEGAL_PADDING_SEED
+    }
+
+    return getBytesSync.call(this, count)
+  }
+  t.after(() => { forge.random.getBytesSync = getBytesSync })
+
+  return () => attempts - remaining
+}
+
+test('selfsigned on its own produces a certificate OpenSSL rejects', async t => {
+  const drawn = forceIllegalSerialNumber(t)
+
+  const { cert } = await new Promise((resolve, reject) => {
+    selfsigned.generate(undefined, { keySize: 1024 }, (err, pems) => {
+      if (err) return reject(err)
+      resolve(pems)
+    })
+  })
+  t.assert.strictEqual(drawn(), 1, 'the serial number seed was not drawn')
+
+  let err
+  try {
+    new X509Certificate(cert) // eslint-disable-line no-new
+  } catch (e) {
+    err = e
+  }
+
+  if (err === undefined) {
+    // Nothing left to work around: `generate.js` can go away and `index.js`
+    // can call `selfsigned.generate()` directly again.
+    t.diagnostic('selfsigned no longer emits non-minimal serial numbers')
+    return
+  }
+
+  t.assert.strictEqual(err.code, 'ERR_OSSL_ASN1_ILLEGAL_PADDING')
+})
+
+test('https-pem (generate) draws a new serial number when OpenSSL rejects one', async t => {
+  const pem = require('..')
+  const drawn = forceIllegalSerialNumber(t)
+
+  const { key, cert } = await pem.generate({ opts: { keySize: 1024 } })
+  t.assert.strictEqual(drawn(), 1, 'the serial number seed was not drawn')
+
+  t.assert.ok(key)
+  // A minimally encoded positive INTEGER never starts with a zero byte
+  const parsed = new X509Certificate(cert)
+  t.assert.doesNotMatch(parsed.serialNumber, /^00/)
+})
+
+test('https-pem (generate) survives consecutive rejected serial numbers', async t => {
+  const pem = require('..')
+  const drawn = forceIllegalSerialNumber(t, 2)
+
+  const { cert } = await pem.generate({ opts: { keySize: 1024 } })
+  t.assert.strictEqual(drawn(), 2, 'not every serial number seed was drawn')
+
+  new X509Certificate(cert) // eslint-disable-line no-new
+})
+
+test('https-pem (generate) gives up instead of returning an unusable pair', async t => {
+  const pem = require('..')
+  forceIllegalSerialNumber(t, Infinity)
+
+  await t.assert.rejects(
+    pem.generate({ opts: { keySize: 1024 } }),
+    err => {
+      t.assert.match(err.message, /could not generate a certificate OpenSSL can load/)
+      t.assert.strictEqual(err.cause.code, 'ERR_OSSL_ASN1_ILLEGAL_PADDING')
+      return true
+    }
+  )
+})
+
+test('https-pem (default) ships a certificate OpenSSL can load', t => {
+  const pem = require('..')
+
+  t.assert.ok(pem.key)
+  const cert = new X509Certificate(pem.cert)
+  t.assert.doesNotMatch(cert.serialNumber, /^00/)
+})
+
+test('https-pem (generate) serves over TLS past a rejected serial number', async t => {
+  const pem = require('..')
+  forceIllegalSerialNumber(t)
+
+  const pems = await pem.generate({
+    attr: [{ name: 'commonName', value: 'localhost' }],
+    opts: { keySize: 2048 }
+  })
+
+  const server = https.createServer(pems, function (req, res) {
+    res.end('foo')
+  })
+
+  server.listen()
+  await once(server, 'listening')
+  t.after(() => server.close())
+
+  const response = await client.request({
+    origin: `https://localhost:${server.address().port}`,
+    path: '/',
+    method: 'GET'
+  })
+
+  t.plan(2)
+  t.assert.strictEqual(response.statusCode, 200)
+  t.assert.strictEqual(await response.body.text(), 'foo')
+})