diff --git a/generate.js b/generate.js new file mode 100644 index 0000000..b0b5e6b --- /dev/null +++ b/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 } diff --git a/index.js b/index.js index 19c678a..89724b2 100644 --- a/index.js +++ b/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) } diff --git a/install.js b/install.js index 1de2231..fc26bca 100644 --- a/install.js +++ b/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) diff --git a/package.json b/package.json index 55668af..5c1a348 100644 --- a/package.json +++ b/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", diff --git a/tests/index.js b/tests/index.js index 64d2b56..4322131 100644 --- a/tests/index.js +++ b/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') +})