Skip to content

build(deps-dev): bump @metcoder95/https-pem from 1.0.0 to 1.0.1 - #5800

Open
ViniciusDev26 wants to merge 1 commit into
nodejs:mainfrom
ViniciusDev26:fix/flaky-pem-illegal-padding
Open

ViniciusDev26 wants to merge 1 commit into
nodejs:mainfrom
ViniciusDev26:fix/flaky-pem-illegal-padding

Conversation

@ViniciusDev26

@ViniciusDev26 ViniciusDev26 commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

Fixes #5245.

test/http2-abort.js was not the problem. selfsigned occasionally generates a certificate with a serial number OpenSSL cannot parse, so any test that builds a TLS server from a freshly generated pair can fail the same way.

pem.generate()@metcoder95/https-pemselfsigned@3.0.1node-forge@1.4.0. The serial comes from selfsigned/index.js:

cert.serialNumber = toPositiveHex(forge.util.bytesToHex(forge.random.getBytesSync(9)))

function toPositiveHex (hexString) {
  var msb = parseInt(hexString[0], 16);
  if (msb < 8) return hexString;
  msb -= 8;
  return msb.toString() + hexString.substring(1);
}

That makes the integer positive, but it does not re-minimise it, and DER requires minimal encoding. When the 9 random bytes start with 0x80 (or 0x00) followed by 0x00, the serial keeps two redundant leading zero bytes.

node-forge would normally strip those, except it only strips one, and says so in lib/asn1.js:

// ensure integer is minimally-encoded
// TODO: should all leading bytes be stripped vs just one?
// .. ex '00 00 01' => '01'?

So 00 00 01 … becomes 00 01 …, still non-minimal. A positive INTEGER with a leading 0x00 followed by a byte under 0x80 is illegal padding, and OpenSSL rejects it:

serial = 000001020304050607
DER    = 02080001020304050607

Error: error:068000DD:asn1 encoding routines::illegal padding
  code: 'ERR_OSSL_ASN1_ILLEGAL_PADDING'
  opensslErrorStack: [ 'error:0488000D:PEM routines::ASN1 lib',
                       'error:0688010A:asn1 encoding routines::nested asn1 error',
                       'error:0688010A:asn1 encoding routines::nested asn1 error' ]

It takes two or more zero bytes followed by a byte under 0x80, which needs b0 ∈ {0x00, 0x80}, b1 == 0x00 and b2 < 0x80: roughly 1 in 65536 certificates. selfsigned runs verifyCertificateChain() before returning and it passes, because node-forge's own parser accepts the non-minimal encoding, so the certificate only fails once OpenSSL reads it inside createSecureServer(). There is nothing platform-specific about it, so Windows and http2-abort.js are just where it happened to land.

What this changes

Per review, the fix was landed upstream rather than worked around here: metcoder95/https-pem#2, released as @metcoder95/https-pem@1.0.1. The package now validates each generated certificate with X509Certificate and draws a new serial when OpenSSL refuses it, for both the async generator and the sync path used by the postinstall hook. Each attempt draws an independent serial, so three attempts make it a non-issue, and it throws rather than ever returning a broken pair.

So this PR is now just the bump. No test file changes are needed.

The real fix still belongs in selfsigned, emitting a minimally encoded serial:

const bytes = Buffer.from(forge.random.getBytesSync(9), 'binary')
bytes[0] &= 0x7f
if (bytes[0] === 0) bytes[0] = 0x01
cert.serialNumber = bytes.toString('hex')

Finishing the node-forge TODO so it strips every redundant leading byte would fix the whole class of it, and the retry in https-pem can be dropped once that happens.

Comment thread test/pem-generate.js Outdated
@codecov-commenter

codecov-commenter commented Sep 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.52%. Comparing base (cb7373a) to head (2e9b7ab).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5800      +/-   ##
==========================================
- Coverage   93.52%   93.52%   -0.01%     
==========================================
  Files         110      110              
  Lines       39415    39415              
==========================================
- Hits        36864    36863       -1     
- Misses       2551     2552       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@metcoder95

Copy link
Copy Markdown
Member

released new @metcoder95/https-pem version, can you update the PR so we can merge?

Fixes nodejs#5245.

`selfsigned` builds 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. When those bytes start with
0x80 (or 0x00) followed by 0x00, the serial keeps two redundant leading zero
bytes. node-forge strips only one of them -- the "should all leading bytes be
stripped vs just one?" TODO in its asn1.js -- so a positive INTEGER with a
leading 0x00 followed by a byte under 0x80 reaches OpenSSL, which rejects it
with ERR_OSSL_ASN1_ILLEGAL_PADDING.

It happens for roughly 1 in 65536 certificates, and `selfsigned`'s own
`verifyCertificateChain()` does not catch it because node-forge's parser
accepts the non-minimal encoding. The certificate only fails once OpenSSL
reads it, so every test that builds a TLS/HTTP2 server from a freshly
generated pair was flaky, most visibly test/http2-abort.js.

The fix landed upstream in metcoder95/https-pem#2: the package now validates
each certificate with X509Certificate and generates again when OpenSSL
refuses it, for both the async generator and the pair written by the
postinstall hook. Nothing is needed on undici's side beyond the bump.
@ViniciusDev26
ViniciusDev26 force-pushed the fix/flaky-pem-illegal-padding branch from 2e9b7ab to 00e1c24 Compare September 14, 2026 19:24
@ViniciusDev26 ViniciusDev26 changed the title test: retry certificate generation on non-minimal serial numbers build(deps-dev): bump @metcoder95/https-pem from 1.0.0 to 1.0.1 Sep 14, 2026
@ViniciusDev26

Copy link
Copy Markdown
Contributor Author

released new @metcoder95/https-pem version, can you update the PR so we can merge?

sure, PR updated

@metcoder95 metcoder95 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like a single test is having troubles with it. Pool related one, if not mistaken recent change added

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[flaky] test\http2-abort.js

3 participants