refactor(cli): modernize networking #1723
Merged
+315
−54
Conversation
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
Historical context: - PR #798 (May 2022) introduced `default-features = false` to avoid `native-tls` which linked against OpenSSL, breaking cross-compilation - PR #1529 and #1536 (Oct 2024) added `rustls-native-certs` and `webpki-roots` to fix corporate proxy CA certificate issues - Issue #726 tracked users behind SSL-inspecting proxies who couldn't use `nextclade dataset get` because rustls ignored system CA stores What changed in reqwest 0.13.x: - Default TLS backend switched from `native-tls` (OpenSSL) to `rustls` - The `rustls` feature now automatically uses `rustls-platform-verifier` which handles platform CA certificates natively - `rustls-native-certs` and `webpki-roots` are now optional dependencies (not features) that are redundant when using the platform verifier Changes in this commit: - Remove `default-features = false` - no longer needed since defaults now use rustls (pure Rust, cross-compilation safe) - Remove explicit `rustls` feature - included via `default-tls` - Remove `rustls-native-certs` - redundant, platform verifier handles it - Remove `webpki-roots` - redundant, platform verifier has fallback New features gained from defaults: - `system-proxy`: auto-detect OS/system proxy settings - `http2`: HTTP/2 protocol support - `charset`: character encoding support Potential issues to watch: - If certificate verification fails in environments where it previously worked, check that system CA store is properly configured - Corporate proxy users should ensure their proxy CA is in the system trust store, or use NEXTCLADE_EXTRA_CA_CERTS / --extra-ca-certs - The `system-proxy` feature may now auto-detect proxies; users who previously needed --proxy flag might find it works automatically
…_merge Historical context: - PR #1536 (Oct 2024) introduced extra CA certificate support using `add_root_certificate()` to address corporate proxy SSL inspection - The NEXTCLADE_EXTRA_CA_CERTS env var and --extra-ca-certs flag allow users to add custom CA certificates for environments where the system trust store cannot be modified API change in reqwest 0.13.x: - `add_root_certificate(cert)` is deprecated - Replaced by `tls_certs_merge(certs)` which: - Takes a Vec<Certificate> instead of single certificates - Merges extra certs with platform trust store (system CAs preserved) - Uses rustls-platform-verifier for certificate verification - Alternative `tls_certs_only(certs)` exists but would disable platform certs entirely, which is not desired Behavioral difference: - Old: Loop calling add_root_certificate for each cert, always modified builder even when no certs provided - New: Only call tls_certs_merge when extra certs are actually provided, slightly more efficient Potential issues to watch: - If extra CA certs stop working, verify the PEM bundle format is valid - The tls_certs_merge API merges with (not replaces) platform certs, so system CA store issues would still need SSL_CERT_FILE/SSL_CERT_DIR - Error messages may differ slightly on certificate parsing failures
Add retry support for transient network failures and server errors when downloading datasets. This improves reliability on flaky networks. Configuration: - Max 3 retries per request - 20% extra load budget (prevents retry storms) - Scoped per-host (retry budget is not shared across different servers) Retryable conditions: - Network errors (no response received) - 5xx server errors (502, 503, 504, etc.) - 429 Too Many Requests (rate limiting) - 408 Request Timeout Non-retryable: - 4xx client errors (except 408, 429) - Successful responses - Redirects (handled separately by reqwest) The retry budget prevents cascading failures: if too many requests to a host are failing, retries are throttled to avoid overwhelming the server.
Problem: The previous commit (ffcae92) removed `default-features = false` from reqwest, which enabled `rustls-platform-verifier`. This verifier requires system CA certificates to be present, causing failures in minimal Docker containers (ubuntu:22.04, debian:stable, etc.) that don't have the `ca-certificates` package installed: Error: No CA certificates were loaded from the system Location: packages/nextclade-cli/src/io/http_client.rs:99 Root cause analysis: 1. reqwest's `rustls` feature enables `rustls-platform-verifier` 2. On Linux, the platform verifier calls `rustls_native_certs::load_native_certs()` 3. If no system certs are found and no extra_roots provided, it errors 4. The `webpki-roots` feature in reqwest doesn't auto-fallback on Linux Solution: Add `webpki-root-certs` crate as a direct dependency and always pass Mozilla's root certificates via `tls_certs_merge()`. This ensures: - The trust store is never empty (Mozilla roots always present) - System certificates are still loaded when available (via rustls-native-certs) - Enterprise SSL inspection still works (via --extra-ca-certs) - All three sources are combined, not mutually exclusive Certificate loading flow: 1. Mozilla roots + user extras passed to reqwest's tls_certs_merge() 2. reqwest forwards to rustls-platform-verifier::Verifier::new_with_extra_roots() 3. Platform verifier adds our certs to RootCertStore first 4. Platform verifier loads system certs via rustls_native_certs 5. Both are combined - store is never empty due to Mozilla roots Code changes: - Add `webpki-root-certs` dependency (provides CertificateDer, not TrustAnchor) - Create `tls.rs` module with thorough documentation of cert hierarchy - Refactor `http_client.rs` to use new tls module - Keep `default-features = false` + explicit `rustls` + `rustls-native-certs` Historical context: - Issue #726: SSL-inspecting proxy support request - PR #798: Added default-features=false to avoid OpenSSL/native-tls - PR #1529, #1536: Added rustls-native-certs and webpki-roots - PR #1723: This fix - Mozilla CA fallback for minimal containers Tested: - debian:stable (no system certs) - works via Mozilla fallback - ubuntu:22.04 with ca-certificates - works via system + Mozilla certs - Smoke tests pass (67 datasets) - Unit tests pass (187 tests) - Lints pass Co-authored-by: Claude <noreply@anthropic.com>
Change "Failed to parse" to "While parsing" for consistency with existing error message patterns in the codebase.
The count may change with webpki-root-certs updates. Remove specific numbers to avoid outdated documentation.
Previously, invalid certificates were silently skipped. Now a warning is logged with the certificate index and error message. This helps diagnose issues if webpki-root-certs ever ships corrupted data.
Add comprehensive documentation for the retry configuration explaining: - Retry scope is limited to root URL host (not redirect targets) - Which conditions trigger retries vs success - Budget mechanism to prevent retry storms
Document that all network errors are retried, including non-transient ones like DNS resolution failures and TLS handshake errors. These retries are harmless given the low retry count (3 max).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
No description provided.