Skip to content

Use rustls for TLS - #2

Merged
CritasWang merged 4 commits into
mainfrom
codex/use-rustls
Aug 12, 2026
Merged

Use rustls for TLS#2
CritasWang merged 4 commits into
mainfrom
codex/use-rustls

Conversation

@HTHou

@HTHou HTHou commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • replace the optional native-tls client transport with rustls 0.23 using the ring crypto provider
  • preserve the existing TLS configuration API, including native roots, additional PEM CA roots, SNI/hostname override, unsafe certificate bypass, and PKCS#8 mutual TLS identities
  • load native trust roots with rustls-native-certs, then use rustls/WebPKI for consistent certificate-chain and hostname verification on every platform
  • keep thrift = "0.23"; TLS continues to wrap the TCP stream below Thrift's framed transport
  • replace the loopback test server with rustls and make the mutual-TLS test actually require and verify the client certificate
  • document trust-store and verification behavior in English and Chinese
  • add macOS/Windows TLS jobs and a Rust 1.75 MSRV job

The explicit uuid, zeroize, and Apple security-framework compatibility constraints keep this lockfile-free library resolvable with the declared Rust 1.75 MSRV.

Closes #1.

User impact

The existing tls feature and public configuration fields remain unchanged. TLS-enabled builds now use rustls for TLS 1.2/1.3. rustls-native-certs supplies the native trust roots and ca_cert_path augments them, while rustls/WebPKI applies the same verification rules across macOS, Windows, and Linux.

accept_invalid_certs skips certificate-chain and hostname verification, while TLS handshake signatures remain cryptographically verified.

Validation

  • ./tools/check-license.sh
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --no-default-features -- -D warnings
  • cargo clippy --all-targets --features tls -- -D warnings
  • cargo test --features tls (124 unit tests + doc test)
  • cargo check --features tls with Rust 1.75.0
  • cargo package --allow-dirty

@HTHou
HTHou marked this pull request as ready for review August 12, 2026 10:06

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Migrates optional TLS support from native-tls to rustls while preserving existing configuration behavior.

Changes:

  • Adds rustls transport, native roots, custom CAs, hostname override, and mutual TLS.
  • Reworks TLS fixtures and tests.
  • Adds cross-platform TLS and Rust 1.75 CI coverage.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
Cargo.toml Replaces native-tls dependencies with rustls.
src/connection/mod.rs Implements rustls transport and tests.
src/error.rs Updates TLS error representation.
README.md Documents rustls behavior.
README_ZH.md Adds Chinese rustls documentation.
.github/workflows/ci.yml Adds platform and MSRV jobs.
tests/fixtures/tls/README.md Updates certificate-generation guidance.
tests/fixtures/tls/cert.pem Regenerates the server certificate.
tests/fixtures/tls/client-cert.pem Regenerates the client certificate.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/connection/mod.rs Outdated
Comment thread src/connection/mod.rs Outdated
@HTHou
HTHou requested a balanced review from Copilot August 12, 2026 10:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/connection/mod.rs:288

  • The rustls migration leaves hostname verification and domain_override untested. The loopback certificate contains SANs for both the endpoint (127.0.0.1) and the override (localhost), so the current successful handshake would also pass if this line ignored the override, and no test checks that a mismatched name is rejected. Add coverage using a name absent from the certificate (and a valid override) so this acceptance criterion cannot regress.
    let domain = tls.domain_override.as_deref().unwrap_or(&endpoint.host);
    let server_name = ServerName::try_from(domain.to_owned())
        .map_err(|e| Error::Client(format!("invalid TLS server name '{domain}': {e}")))?;

@CritasWang
CritasWang merged commit 8046bd3 into main Aug 12, 2026
7 checks passed
@CritasWang
CritasWang deleted the codex/use-rustls branch August 12, 2026 11:11
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.

Change TLS provider to rustls?

3 participants