Support dynamic client certificate resolution (ResolvesClientCert) - #1340
Support dynamic client certificate resolution (ResolvesClientCert)#1340brucearctor wants to merge 5 commits into
Conversation
…ClientCert) Add a new client_cert_resolver field to TlsOptions that accepts an Arc<dyn ResolvesClientCert> for dynamic per-handshake client certificate resolution. This enables transparent mTLS certificate rotation without requiring a process restart -- useful for short-lived certificates managed by Vault agents, sidecars, or HSMs. Key changes: - Add client_cert_resolver: Option<Arc<dyn ResolvesClientCert>> to TlsOptions (mutually exclusive with static client_tls_options) - Re-export ResolvesClientCert from the crate root for ergonomic use - When a resolver is set, bypass tonic's static Identity path and build a rustls::ClientConfig manually with with_client_cert_resolver() - Introduce DynamicTlsConnector (tower::Service<Uri>) that performs TLS via tokio_rustls::TlsConnector with the custom config - Use Endpoint::connect_with_connector() to wire it into tonic's channel - Add validation: error when both static and dynamic client certs are set - DNS load balancing returns a clear error for now (not yet supported with dynamic cert resolution) - Update C bridge and envconfig with client_cert_resolver: None Tests (13 new): - Mutual exclusion validation (static + dynamic = error) - CustomConnector result with explicit and inferred domain - Resolver + custom ServerCertVerifier combination - Resolver + custom CA certificate - build_custom_rustls_config with/without resolver/verifier - Debug output for TlsOptions with resolver - DynamicTlsConnector Clone requirement - Default TlsOptions has no resolver - No-TLS passthrough still returns Standard Closes: temporalio#1338
Fixes from 4-reviewer deep code review (Temporal, Rust, Systems, Security): Must Fix: - Proxy + cert resolver: add validation to prevent silent discard of resolver when http_connect_proxy is also set (was silently ignoring the dynamic cert resolver) - Empty SNI domain: fail early with clear error instead of producing an empty string that causes a confusing late error from ServerName - Localhost fallback: DynamicTlsConnector no longer falls back to "localhost" when URI has no host — returns a clear error instead - Connect timeout: wrap TCP connect in 30s timeout to prevent hanging on unreachable hosts (tonic's built-in connector handles this but custom connectors must do it themselves) - CA cert parsing: check roots.is_empty() after add_parsable_certificates to catch cases where all provided CA certs are malformed - Dead code: remove unused CountingCertResolver test helper Should Fix: - Tracing: add debug! logging to DynamicTlsConnector for TCP connect and TLS handshake completion - Re-exports: add CertifiedKey and SignatureScheme re-exports so users can implement ResolvesClientCert without adding tokio-rustls directly - Debug impl: add manual Debug for DynamicTlsConnector showing domain - Nodelay comment: document why set_nodelay(true) is set
|
Will be giving this a review very soon, thanks for your patience. |
|
i'm in no rush |
chris-olszewski
left a comment
There was a problem hiding this comment.
Overall I think this design makes sense to me. The largest thing for me is making sure we feature gate it.
|
@chris-olszewski - i think addressed? |
chris-olszewski
left a comment
There was a problem hiding this comment.
Looks good to me. Only a few questions on some of the new tests and a suggestion that we mark this variant as experimental. Thanks for your patience on the review process.
| #[test] | ||
| fn dynamic_tls_connector_is_clone_and_debug() { | ||
| // Verify DynamicTlsConnector is Clone (required by tonic) and Debug | ||
| let config = build_custom_rustls_config( | ||
| &TlsOptions::default(), | ||
| Some(Arc::new(MockClientCertResolver)), | ||
| ) | ||
| .unwrap(); | ||
| let server_name = | ||
| tokio_rustls::rustls::pki_types::ServerName::try_from("test.temporal.io") | ||
| .unwrap() | ||
| .to_owned(); | ||
| let connector = DynamicTlsConnector { | ||
| tls: tokio_rustls::TlsConnector::from(Arc::new(config)), | ||
| domain: Arc::new(server_name), | ||
| }; | ||
| let _cloned = connector.clone(); | ||
| let debug_str = format!("{:?}", connector); | ||
| assert!( | ||
| debug_str.contains("DynamicTlsConnector"), | ||
| "Debug should show struct name: {debug_str}" | ||
| ); | ||
| assert!( | ||
| debug_str.contains("test.temporal.io"), | ||
| "Debug should show domain: {debug_str}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Unsure the value of a test asserting that these traits are implemented
| #[test] | |
| fn dynamic_tls_connector_is_clone_and_debug() { | |
| // Verify DynamicTlsConnector is Clone (required by tonic) and Debug | |
| let config = build_custom_rustls_config( | |
| &TlsOptions::default(), | |
| Some(Arc::new(MockClientCertResolver)), | |
| ) | |
| .unwrap(); | |
| let server_name = | |
| tokio_rustls::rustls::pki_types::ServerName::try_from("test.temporal.io") | |
| .unwrap() | |
| .to_owned(); | |
| let connector = DynamicTlsConnector { | |
| tls: tokio_rustls::TlsConnector::from(Arc::new(config)), | |
| domain: Arc::new(server_name), | |
| }; | |
| let _cloned = connector.clone(); | |
| let debug_str = format!("{:?}", connector); | |
| assert!( | |
| debug_str.contains("DynamicTlsConnector"), | |
| "Debug should show struct name: {debug_str}" | |
| ); | |
| assert!( | |
| debug_str.contains("test.temporal.io"), | |
| "Debug should show domain: {debug_str}" | |
| ); | |
| } |
| #[test] | ||
| fn re_exports_are_accessible() { | ||
| // Verify that CertifiedKey and SignatureScheme are re-exported | ||
| // and usable from the crate root. This is a compile-time check. | ||
| fn _assert_types_exist( | ||
| _resolver: &dyn ResolvesClientCert, | ||
| _scheme: SignatureScheme, | ||
| ) { | ||
| // This function just needs to compile | ||
| } | ||
| let _ = SignatureScheme::ECDSA_NISTP256_SHA256; | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't believe the test asserts what it says since use super::* gets parent's imports, not just exports. Directly referencing these via crate:: is probably what is desired, but unsure if testing this is necessary.
| /// Standard tonic TLS was applied, endpoint is ready to connect normally. | ||
| Standard(Endpoint), | ||
| /// A custom rustls::ClientConfig is needed. The endpoint has no TLS configured; | ||
| /// the caller must use `connect_with_connector` with a custom TLS connector. |
There was a problem hiding this comment.
| /// the caller must use `connect_with_connector` with a custom TLS connector. | |
| /// the caller must use `connect_with_connector` with a custom TLS connector. | |
| /// | |
| /// Experimental API subject to change |
Support dynamic client certificate resolution (ResolvesClientCert)
What
Add a
client_cert_resolverfield toTlsOptionsthat accepts anArc<dyn ResolvesClientCert>for dynamic, per-handshake client certificate selection. This enables transparent mTLS certificate rotation without requiring a process restart — useful for short-lived certificates managed by Vault agents, cert-manager sidecars, or HSM-backed signers.Closes #1338
Why
The existing
client_tls_optionsrequires static(cert, key)bytes at connection time. Users with rotating certificates (e.g., Vault-issued certs with 24h TTLs) must restart the entire Temporal client to pick up new material. The Go SDK solves this withtls.Config.GetClientCertificate; this PR brings equivalent functionality to the Rust SDK.How
Since
tonic0.14.6 does not exposerustls::ClientConfig::with_client_cert_resolver(), the implementation bypasses tonic's TLS layer when a resolver is present:build_custom_rustls_config()— manually constructs arustls::ClientConfigwith the user'sResolvesClientCert, replicating tonic's security defaults (protocol versions, ALPN, native/custom CA roots)DynamicTlsConnector— atower::Service<Uri>that wraps TCP with TLS usingtokio_rustls::TlsConnector, including connect timeouts and tracingEndpoint::connect_with_connector()— used instead ofconnect()to wire the custom connector into tonic's channelThe resolver fires on each new TLS handshake (reconnections), not per-RPC over an existing HTTP/2 connection.
Key design decisions
client_tls_optionsandclient_cert_resolverreturnsInvalidConfigResolvesClientCert,CertifiedKey, andSignatureSchemeare re-exported at the crate root so users don't need to depend ontokio-rustlsdirectlybalance_channelAPI constraints)client_cert_resolver: None(dynamic resolution from C callers needs a callback design in a follow-up)Changes
crates/client/src/options_structs.rsclient_cert_resolverfield, updateDebugimplcrates/client/src/lib.rsTlsConfigResultenum,add_tls_to_channelbranching,build_custom_rustls_config,DynamicTlsConnector, proxy/DNS validation, tracingcrates/client/src/dns.rsTlsConfigResult, reject resolver + DNS LBcrates/client/src/envconfig.rsclient_cert_resolver: Nonecrates/client/Cargo.tomlrustls-native-certsdepcrates/sdk-core-c-bridge/src/client.rsclient_cert_resolver: Nonecrates/sdk-core/tests/common/mod.rsCHANGELOG.md[Unreleased] > AddedTests (15 new, 134 total pass)
ServerCertVerifiercombinationbuild_custom_rustls_configwith/without resolver/verifierTlsOptionswith resolverDynamicTlsConnectorisClone+DebugTlsOptionshas no resolverStandardCertifiedKey,SignatureScheme)Future work
FileWatchingCertResolverbatteries-included implementation — Provide batteries-included FileWatchingCertResolver for common mTLS rotation #1345