This repository was archived by the owner on May 14, 2026. It is now read-only.
feat(network): switch TLS backend to rustls for PKCS#1 + EC key support#509
Merged
Conversation
Replaces the `native-tls-vendored` reqwest feature with `rustls`, closing the PKCS#1 parity gap flagged in #499. Native-tls's `Identity::from_pkcs8_pem` accepted only `-----BEGIN PRIVATE KEY-----` (PKCS#8); rustls's `Identity::from_pem` accepts PKCS#1 (`-----BEGIN RSA PRIVATE KEY-----`), PKCS#8, and EC keys — the same surface Node's `tls.createSecureContext` exposes to pnpm via undici. Pacquet now matches pnpm bug-for-bug on the set of client-cert key formats accepted from `.npmrc`'s `key=` / `:key=` / `:keyfile=` entries. PKCS#12 (`.pfx`) stays out of scope — pnpm's `.npmrc` allow-list doesn't expose a `pfx=` option so pacquet doesn't either. ## What changed - `Cargo.toml`: drop `native-tls-vendored`, add `rustls`. Reqwest's `rustls` feature uses `aws-lc-rs` for crypto and `rustls-platform-verifier` for OS trust roots — closest behavioral match to native-tls's "consult the platform trust store" default. - `crates/network/src/lib.rs`: `apply_tls` swaps `Identity::from_pkcs8_pem(cert, key)` for `Identity::from_pem` applied to the concatenated `cert\nkey` PEM buffer. Adds a new `looks_like_pem_cert` syntactic check before `Certificate::from_pem` because rustls's `from_pem` stores the bytes verbatim and validates lazily at `Client::build()` time — a garbage CA entry would otherwise slip through silently and the install would proceed against an unknown trust root. - Updated doc comments on `apply_tls`, `TlsConfig::key`, `RegistryTls::key`, and `TlsError::InvalidClientIdentity` to describe the new surface and drop the PKCS#8-only caveat. - `deny.toml`: allow `CDLA-Permissive-2.0` for `webpki-root-certs` (pulled in by reqwest's `rustls` feature through `rustls-platform-verifier`'s fallback chain). - New fixtures at `crates/network/tests/fixtures/test-client-pkcs1.{crt,key}` loaded via `include_str!`. Regenerable with `openssl genrsa -traditional` + `openssl req -new -x509`. ## Tests `for_installs_with_pkcs1_client_key_builds` pins the contract — if a future change reverts the backend or otherwise narrows the accepted key formats, this build will fail with a clear `InvalidClientIdentity`. All 1175 workspace tests pass. ## Notes for review - **Cert store change.** `rustls-platform-verifier` reads the OS trust store on macOS / Windows / Linux. The lookup is a different syscall path from native-tls's; behavioral parity for "trust roots in the OS store" should hold, but corporate CAs that worked under native-tls and *don't* show up in `rustls-platform-verifier`'s enumeration would now silently fail. Users hitting that should add the CA explicitly via `cafile=` in `.npmrc`. - **Performance.** CI's integrated-benchmark will run on this PR; if it regresses materially on the warm-install path we'd consider falling back to the preprocessing approach (option 2 in #499). - **`hickory-dns` compatibility.** Verified by running the workspace test suite — DNS resolution is independent of the TLS backend. - **`cargo deny` posture.** One new license allowance (`CDLA-Permissive-2.0`) for `webpki-root-certs`. No new advisory surface area beyond what reqwest's `rustls` feature already pulls through `aws-lc-rs` / `rustls` / `rustls-platform-verifier`.
📝 WalkthroughWalkthroughThis PR migrates the network crate from OpenSSL-based native-tls-vendored to rustls TLS backend and refactors client identity handling to accept PKCS#1 and PKCS#8 private keys. The key change replaces separate PKCS#8 identity construction with a concatenated PEM buffer approach, adding early validation of CA certificate armor markers. ChangesTLS rustls migration and PKCS#1 support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/network/src/tests.rs (1)
519-551: ⚡ Quick winAdd an EC-key regression test to pin the full supported-key contract.
This new hunk validates PKCS#1, but the documented/behavioral surface now includes EC keys too. Add one EC cert+key fixture case so that support is locked by tests, not just docs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/network/src/tests.rs` around lines 519 - 551, Add a parallel EC-key fixture and test to lock the documented support for EC client keys: create constants similar to TEST_CLIENT_PKCS1_CERT and TEST_CLIENT_PKCS1_KEY (e.g. TEST_CLIENT_EC_CERT and TEST_CLIENT_EC_KEY) that include an EC cert and EC private key PEM, then add a new #[test] (mirroring for_installs_with_pkcs1_client_key_builds) which constructs a TlsConfig with those EC constants and calls ThrottledClient::for_installs(&ProxyConfig::default(), &tls, &PerRegistryTls::default()).expect(...) to assert EC keys build successfully; this pins support for EC keys alongside the existing PKCS#1 test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/network/src/tests.rs`:
- Around line 519-551: Add a parallel EC-key fixture and test to lock the
documented support for EC client keys: create constants similar to
TEST_CLIENT_PKCS1_CERT and TEST_CLIENT_PKCS1_KEY (e.g. TEST_CLIENT_EC_CERT and
TEST_CLIENT_EC_KEY) that include an EC cert and EC private key PEM, then add a
new #[test] (mirroring for_installs_with_pkcs1_client_key_builds) which
constructs a TlsConfig with those EC constants and calls
ThrottledClient::for_installs(&ProxyConfig::default(), &tls,
&PerRegistryTls::default()).expect(...) to assert EC keys build successfully;
this pins support for EC keys alongside the existing PKCS#1 test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 367697c6-5bed-4d9f-8a66-a6642d4482ba
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlcrates/network/src/lib.rscrates/network/src/tests.rscrates/network/src/tls.rscrates/network/tests/fixtures/test-client-pkcs1.crtcrates/network/tests/fixtures/test-client-pkcs1.keydeny.toml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Dylint
- GitHub Check: Lint and Test (windows-latest)
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Lint and Test (ubuntu-latest)
- GitHub Check: Doc
- GitHub Check: Run benchmark on ubuntu-latest
- GitHub Check: Code Coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Preserve existing method chains andpipe-traitchains; do not break them into intermediateletbindings unless there is a concrete justification such as a compilation failure, borrow checker rejection, meaningful performance improvement, or other technical necessity. Refactoring for style alone is not sufficient justification.
Choose owned vs. borrowed parameters to minimize copies; prefer borrowed types (&Pathover&PathBuf,&strover&String) when it does not force extra copies.
PreferArc::clone(&x)andRc::clone(&x)overx.clone()for reference-counted types to make the cost visible at the call site.
Do not use star imports inside module bodies. Writeuse super::{Foo, bar}instead ofuse super::*;for any glob whose target is a module you control. External-crate preludes (e.g.,use rayon::prelude::*;) and root-of-module re-exports (e.g.,pub use submodule::*;inlib.rs) are exceptions.
Follow Rust API Guidelines for naming, as documented in https://rust-lang.github.io/api-guidelines/naming.html.
Declare a newtype wrapper for any branded string type being ported from TypeScript pnpm. Do not collapse the brand into a plainStringor&str; give the type its own struct so misuse is a type error.
When porting branded string types where upstream TypeScript always validates before construction, validate in the Rust port too. Construct the wrapper only viaTryFrom<String>and/orFromStr; do not provide an infallible public constructor that takes an arbitrary string.
For branded string types where upstream TypeScript never validates (used purely for type-safety to prevent confusion between string slots), expose an infallibleFrom<String>andFrom<&str>constructor in the Rust wrapper.
When upstream TypeScript occasionally constructs a branded type without validation (via bareasassertion), add afrom_str_unchecked(or similarly named) constructor on the Rust side. Keep the validating constructor as well; `from_str_u...
Files:
crates/network/src/tls.rscrates/network/src/lib.rscrates/network/src/tests.rs
🧠 Learnings (4)
📚 Learning: 2026-05-07T23:19:08.272Z
Learnt from: KSXGitHub
Repo: pnpm/pacquet PR: 401
File: tasks/integrated-benchmark/src/work_env.rs:343-344
Timestamp: 2026-05-07T23:19:08.272Z
Learning: When reviewing Rust code in pnpm/pacquet for deprecated API usage, do not automatically treat `serde_saphyr::to_string` as deprecated. In `serde-saphyr` v0.0.25, `serde_saphyr::to_string` has no `#[deprecated]` attribute (the `#[deprecated]` later in `serde-saphyr-0.0.25/src/lib.rs` applies to a different function). Only flag `serde_saphyr::to_string` as deprecated if the resolved dependency version’s source shows `#[deprecated]` on that specific function.
Applied to files:
crates/network/src/tls.rscrates/network/src/lib.rscrates/network/src/tests.rs
📚 Learning: 2026-05-13T22:52:32.579Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 506
File: crates/package-manager/src/link_bins.rs:238-241
Timestamp: 2026-05-13T22:52:32.579Z
Learning: In Rust code using `matches!` (or `match`) on a borrowed value (e.g., `meta: &PackageMetadata` and a field access like `meta.resolution`), it is safe to use wildcard/OR patterns such as `LockfileResolution::Binary(_) | LockfileResolution::Variations(_)` when the pattern does not bind the inner payload by value. `_` wildcard patterns (and similar discriminant-only patterns) should not move non-`Copy` data; they only test the variant and avoid ownership transfer. Do not flag these patterns as move-out-of-borrow/ownership compile errors. Only patterns that bind the inner value by value (e.g., `LockfileResolution::Binary(b)`) would require ownership and may trigger move errors when matching on data behind a borrow.
Applied to files:
crates/network/src/tls.rscrates/network/src/lib.rscrates/network/src/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In Rust test code, follow the repo’s CODE_STYLE_GUIDE test-logging rule: add logging (e.g., `eprintln!`/`eprintln!(...)`) so that useful diagnostic values are printed when a test fails, unless the assertion is `assert_eq!` (where the differing values are already included). Concretely, if you use assertions like `assert!`, `assert_ne!`, etc., ensure the test logs the relevant actual/expected values (or context) before/around the assertion so failures can be diagnosed without rerunning.
Applied to files:
crates/network/src/tests.rs
📚 Learning: 2026-05-01T10:01:33.766Z
Learnt from: zkochan
Repo: pnpm/pacquet PR: 349
File: crates/reporter/src/tests.rs:121-121
Timestamp: 2026-05-01T10:01:33.766Z
Learning: In this repo’s Rust tests (run via `cargo nextest`), stdout/stderr from passing tests is captured and is only shown when a test fails. As a result, `eprintln!` (and similar stderr logging) in these tests should generally not be flagged as CI “noise” on the happy path; CI output should only appear for failing tests.
Applied to files:
crates/network/src/tests.rs
🪛 Betterleaks (1.2.0)
crates/network/tests/fixtures/test-client-pkcs1.key
[high] 1-27: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
crates/network/src/lib.rs
[high] 402-404: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (6)
Cargo.toml (1)
65-71: LGTM!deny.toml (1)
63-63: LGTM!crates/network/src/tls.rs (1)
51-54: LGTM!Also applies to: 85-87, 98-102, 156-159
crates/network/src/lib.rs (1)
189-192: LGTM!Also applies to: 353-366, 373-413
crates/network/tests/fixtures/test-client-pkcs1.key (1)
1-27: LGTM!crates/network/tests/fixtures/test-client-pkcs1.crt (1)
1-19: LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 88.74% 88.61% -0.13%
==========================================
Files 124 125 +1
Lines 13429 13589 +160
==========================================
+ Hits 11917 12042 +125
- Misses 1512 1547 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Micro-Benchmark ResultsLinux |
Integrated-Benchmark Report (Linux)Scenario: Frozen Lockfile
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 2.5806202082,
"stddev": 0.07065578774603276,
"median": 2.5776692602000004,
"user": 2.5991813,
"system": 3.62475382,
"min": 2.4846546132,
"max": 2.6842233232,
"times": [
2.6842233232,
2.6064455162000004,
2.6586850652000003,
2.5868357932,
2.5685027272000003,
2.4846546132,
2.6583040652000003,
2.5201087302,
2.4991648122,
2.5392774362000003
]
},
{
"command": "pacquet@main",
"mean": 2.5175855858,
"stddev": 0.05373506890081997,
"median": 2.5025447487,
"user": 2.5597214999999998,
"system": 3.6027440200000003,
"min": 2.4571449462,
"max": 2.6320824402,
"times": [
2.4957729952000003,
2.6320824402,
2.4673295862,
2.4700351282,
2.4571449462,
2.5471237702000002,
2.5010011882,
2.5040883092,
2.5655932022,
2.5356842922
]
},
{
"command": "pnpm",
"mean": 5.994663356,
"stddev": 0.04788516921613567,
"median": 5.9948375317,
"user": 8.8138824,
"system": 4.44169252,
"min": 5.8989813402,
"max": 6.0791939282,
"times": [
5.9983694342,
6.0791939282,
6.0379419432,
5.991305629199999,
5.9741910082,
6.0128520872,
6.0117730242,
5.959244240199999,
5.9827809252,
5.8989813402
]
}
]
}Scenario: Frozen Lockfile (Hot Cache)
BENCHMARK_REPORT.json{
"results": [
{
"command": "pacquet@HEAD",
"mean": 0.733705106,
"stddev": 0.033090255721194235,
"median": 0.7336583256,
"user": 0.35596908,
"system": 1.5952151,
"min": 0.6864764236,
"max": 0.7982619556,
"times": [
0.7982619556,
0.7455641476,
0.6864764236,
0.7623704256,
0.7448435986,
0.7491618416,
0.6972934666,
0.7143031316,
0.7163030166000001,
0.7224730526
]
},
{
"command": "pacquet@main",
"mean": 0.8110906594000001,
"stddev": 0.08407172561111158,
"median": 0.7807923571000001,
"user": 0.36850958000000006,
"system": 1.6070752,
"min": 0.7334071856000001,
"max": 0.9704252736000001,
"times": [
0.8269118736000001,
0.7642652966000001,
0.9704252736000001,
0.8053658116000001,
0.7851923696,
0.7526558196,
0.9522585546000001,
0.7440320646,
0.7763923446000001,
0.7334071856000001
]
},
{
"command": "pnpm",
"mean": 2.5401923541,
"stddev": 0.08374228169910679,
"median": 2.5190563126,
"user": 3.0961219800000004,
"system": 2.2138629,
"min": 2.4649271115999998,
"max": 2.7549929046,
"times": [
2.5422945585999996,
2.4684011596,
2.5232573406,
2.5723930746,
2.5148552846,
2.5146487356,
2.4846814856,
2.4649271115999998,
2.5614718856,
2.7549929046
]
}
]
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #499.
Summary
Switches reqwest's TLS backend from
native-tls-vendoredtorustls. The single observable contract change: pacquet now accepts PKCS#1, PKCS#8, and EC private keys from.npmrc'skey=/:key=/:keyfile=entries — the same set Node'stls.createSecureContextexposes to pnpm via undici. Native-tls'sIdentity::from_pkcs8_pemaccepted only-----BEGIN PRIVATE KEY-----(PKCS#8); the doc comment in #490 flagged this as a parity gap.PKCS#12 (
.pfx) stays out of scope — pnpm's.npmrcallow-list doesn't expose apfx=option so pacquet doesn't either.What changed
Cargo.toml: dropnative-tls-vendored, addrustls. Reqwest'srustlsfeature usesaws-lc-rsfor crypto andrustls-platform-verifierfor OS trust roots — closest behavioral match to native-tls's "consult the platform trust store" default.crates/network/src/lib.rs:apply_tlsswapsIdentity::from_pkcs8_pem(cert, key)forIdentity::from_pemover the concatenatedcert\nkeyPEM buffer. Adds a newlooks_like_pem_certsyntactic check beforeCertificate::from_pembecause rustls'sfrom_pemstores the bytes verbatim and validates lazily atClient::build()time — a garbage CA entry would otherwise slip through silently and the install would proceed against an unknown trust root. The existingInvalidCa { index, .. }error contract is preserved.crates/network/src/tls.rs: updated doc comments onTlsConfig::key,RegistryTls::key,TlsError::InvalidClientIdentityto describe the new surface and drop the PKCS#8-only caveat.deny.toml: allowCDLA-Permissive-2.0forwebpki-root-certs(pulled in by reqwest'srustlsfeature throughrustls-platform-verifier's fallback chain).crates/network/tests/fixtures/test-client-pkcs1.{crt,key}(PKCS#1 RSA, self-signed, 100-year expiry). Loaded viainclude_str!. Regenerable withopenssl genrsa -traditional+openssl req -new -x509per the constant comment.Reviewer flags
rustls-platform-verifierreads the OS trust store on macOS / Windows / Linux, but it's a different syscall path from native-tls's. Corporate CAs that worked under native-tls and don't show up inrustls-platform-verifier's enumeration would now silently fail. Users hitting this should add the CA explicitly viacafile=in.npmrc.pkcs1/pkcs8RustCrypto crates) as a fallback that keeps native-tls.CDLA-Permissive-2.0) forwebpki-root-certs. No new advisory surface beyond what reqwest'srustlsfeature already pulls throughaws-lc-rs/rustls/rustls-platform-verifier.Test plan
for_installs_with_pkcs1_client_key_buildspins the new PKCS#1-accepted contractInvalidCa/InvalidClientIdentitytests still surface their errors with the right index / reasoncargo nextest run --workspace— 1175 tests passjust readycleantaplo format --checkcleanRUSTDOCFLAGS=-D warnings cargo doc --no-deps --workspace --document-private-itemscleanjust dylintcleancargo deny checkclean (one new license allowance added indeny.toml)mainReferences
Written by an agent (Claude Code, claude-opus-4-7).
Summary by CodeRabbit