fix(ext/node): accept X.509v1 server certs in tls.createServer#33505
Conversation
Enables tests/node_compat/runner/suite/test/parallel/test-http2-server-startup.js Co-authored-by: Divy Srivastava <[email protected]>
533f222 to
2b32775
Compare
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM. The fix is targeted at the right thing — with_single_cert runs CertifiedKey::keys_match() which calls webpki's parser, and webpki rejects v1 certs with UnsupportedCertVersion. Constructing CertifiedKey directly via provider.key_provider.load_private_key(...) and registering through SingleCertAndKey resolver bypasses just that check. SingleCertAndKey is a stable rustls 0.23 API. The comment correctly identifies the upstream Node fixtures (agent2, agent3) that triggered the test failure.
One non-blocking note inline about what else keys_match was doing.
| let provider = builder.crypto_provider().clone(); | ||
| let signing_key = provider.key_provider.load_private_key(private_key).ok()?; | ||
| let certified_key = rustls::sign::CertifiedKey::new(certs, signing_key); | ||
| let resolver = rustls::sign::SingleCertAndKey::from(certified_key); |
There was a problem hiding this comment.
Heads up that skipping keys_match doesn't only suppress the v1-cert rejection — it also drops the cert/key pairing check and the empty-chain check. Concretely:
- An empty
certs: Vec<CertificateDer>previously failed atwith_single_certand now silently succeeds at config-build, then fails at handshake with a less-helpful error. - A cert that doesn't match the private key previously failed at config-build, now fails at the first handshake's signing operation.
Node (OpenSSL) actually has the same loose-at-load / strict-at-handshake behavior, so this matches Node, but worth documenting in the comment so the next person reading this doesn't think keys_match was just the v1 check. If you wanted to preserve the cert/key pairing check, you could call certified_key.keys_match() and translate the UnsupportedCertVersion error to Ok(()) while propagating others.
Co-authored-by: Divy Srivastava <[email protected]>
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
LGTM after 9ce9b1c3. The keys_match() is preserved with just the UnsupportedCertVersion arm translated to success, which keeps the cert/key pairing check and empty-chain check working — exactly the recovery I was asking for. The updated comment correctly enumerates all three things keys_match was doing.
One inline note about the error-matching mechanism — non-blocking but worth thinking about.
| Err(rustls::Error::InvalidCertificate( | ||
| rustls::CertificateError::Other(ref other), | ||
| )) if other.to_string().contains("UnsupportedCertVersion") => {} | ||
| Err(e) => { |
There was a problem hiding this comment.
Non-blocking: matching on other.to_string().contains("UnsupportedCertVersion") is fragile against rustls-webpki Display format changes (the variant name is auto-generated from Debug/Display derive on webpki::Error and could change between minor versions or be reformatted). A downcast_ref::<webpki::Error>() on the Arc<dyn StdError> inside Other would let you match the variant directly:
Err(rustls::Error::InvalidCertificate(
rustls::CertificateError::Other(other),
)) if other.downcast_ref::<webpki::Error>()
.is_some_and(|e| matches!(e, webpki::Error::UnsupportedCertVersion)) => {}That needs a webpki dep entry (or rustls_webpki) but survives display-format churn. Pinning rustls to =0.23.28 mitigates but doesn't eliminate the risk for future bumps.
Co-authored-by: Divy Srivastava <[email protected]>
fibibot
left a comment
fibibot
left a comment
There was a problem hiding this comment.
Still LGTM after 66fe51b9. The downcast is wired correctly:
- rustls 0.23.28 internally depends on
rustls-webpki 0.103.3(verified inCargo.lock), and the PR pinsrustls-webpki = "0.103"inext/node/Cargo.toml, so both arms of the type identity match. TheArc<dyn StdError>insideOtherErrorwas constructed by rustls with awebpki::Errorfrom 0.103, and thedowncast_ref::<webpki::Error>()call resolves through the same version — no silent miss from version-skew. - The
is_some_and(|e| matches!(e, webpki::Error::UnsupportedCertVersion))closure pattern keeps the original failure-mode (cert/key pairing, empty-chain) intact via theErr(e) => log + return Nonefallthrough. - The comment correctly flags the pinning requirement so a future rustls major bump knows to update this dep too.
Nice.
- Move BadEncoding check outside the if-let Other(other) arm so it can actually match (was dead code). - Replace synchronous ssl.destroySsl() in TLSWrap.close() with the underlying onStreamRead fix: silently drop bytes that arrive on a stream the consumer already destroyed instead of forwarding a positive nread to errnoException. Restores writes queued before destroy() (fixes test-tls-invoke-queued regression). - Switch substring matching of webpki errors in tls_wrap to a proper webpki::Error downcast, mirroring the approach in denoland#33505. build_server_config now keeps CertifiedKey::keys_match() so the cert/key pairing and empty-chain checks still run. Co-authored-by: Divy Srivastava <[email protected]>
- Move BadEncoding check outside the if-let Other(other) arm so it can actually match (was dead code). - Replace synchronous ssl.destroySsl() in TLSWrap.close() with the underlying onStreamRead fix: silently drop bytes that arrive on a stream the consumer already destroyed instead of forwarding a positive nread to errnoException. Restores writes queued before destroy() (fixes test-tls-invoke-queued regression). - Switch substring matching of webpki errors in tls_wrap to a proper webpki::Error downcast, mirroring the approach in denoland#33505. build_server_config now keeps CertifiedKey::keys_match() so the cert/key pairing and empty-chain checks still run. Co-authored-by: Divy Srivastava <[email protected]>
3 participants
Summary
Enables
test-http2-server-startupin node_compat suite.Test plan
cargo test --test node_compat -- test-http2-server-startup