chore: configure TLS trust roots for rustls clients#4667
chore: configure TLS trust roots for rustls clients#4667NathanFlurry wants to merge 1 commit intorecover-engine-npm-packagefrom
Conversation
|
🚅 Deployed to the rivet-pr-4667 environment in rivet-frontend
|
2d05736 to
1bb4288
Compare
Code ReviewOverviewThis PR migrates rustls-based TLS clients ( TLS Strategy (Core Change)The dual-roots approach is well-reasoned and correctly documented in CLAUDE.md:
The CLAUDE.md note that engine-internal clients (ClickHouse, hyper-tls, guard HTTP proxy) intentionally stay on OpenSSL is a good explicit carve-out that prevents future confusion. Security note: Enabling both root stores creates a union trust set -- any CA in either store is trusted. This is the correct trade-off for the stated use case, but it does slightly expand the trust surface compared to webpki-only. This appears intentional and acceptable. Issues / Questions1. Quinn unexpectedly pulled in as a reqwest dependency In 2. The previous reqwest config used default features, which include 3. Large reformatting noise in a generated file
4. Sandbox example: Vercel equivalent not updated Per CLAUDE.md, updating examples requires also updating the Vercel equivalent. The Minor Observations
SummaryThe core TLS migration is solid and the CLAUDE.md documentation is clear and useful. The main items to resolve before merge: verify the unexpected Quinn dependency, confirm whether HTTP compression is still needed, and separate the generated file reformatting from the functional changes. |
PR Review: chore: configure TLS trust roots for rustls clientsOverviewThis PR addresses TLS trust root configuration for rustls-based outbound TLS clients (tokio-tungstenite, reqwest) in the rivetkit-rust SDK. The core change enables both rustls-tls-native-roots and rustls-tls-webpki-roots together as a union trust store, fixing TLS failures in Distroless/minimal container images. Substantive ChangesTLS Configuration (core fix)
remote_manager.rs client reuse Issues and Suggestions1. Missing reqwest default features (potential regression) 2. Removal of eventsource-client without replacement 3. Two webpki-roots versions in rivetkit-rust 4. Generated file formatting churn in engine-runner-protocol 5. Silent removal of examples/global-smoke-test and sqlite-vfs 6. PR description is empty CLAUDE.md Alignment
SummaryThe core TLS fix is correct and well-reasoned. Main items to address before merge:
|
Preview packages published to npmInstall with: npm install rivetkit@pr-4667All packages published as Engine binary is shipped via Docker images: docker pull rivetdev/engine:slim-da01c89
docker pull rivetdev/engine:full-da01c89Individual packagesnpm install rivetkit@pr-4667
npm install @rivetkit/react@pr-4667
npm install @rivetkit/rivetkit-native@pr-4667
npm install @rivetkit/workflow-engine@pr-4667 |
1bb4288 to
01a868f
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: