Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces DynamicKeyManager and DynamicTrustManager to support dynamic loading and automatic reloading of client certificates, private keys, and server root CA certificates for Spanner Omni. It also updates SpannerOptions, ConnectionOptions, and related classes to support these new configurations. The review feedback highlights a potential performance issue where blocking disk I/O and CPU-heavy cryptographic operations are performed on the calling thread (typically a Netty EventLoop thread) during TLS handshakes. Additionally, the feedback points out a potential mismatch bug in DynamicKeyManager's fallback logic when an alias is evicted, suggesting safer alias resolution and returning empty arrays instead of nulls to minimize null pointer risks.
4383710 to
9ad0f6d
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces dynamic certificate and key reloading for Spanner Omni instances by adding DynamicKeyManager and DynamicTrustManager to automatically reload client certificates, private keys, and server root CA certificates from disk when modified. It also updates configuration classes, connection properties, and connection pools to support these new options, alongside comprehensive unit tests. The review feedback highlights a critical issue in both new managers where using ThreadPoolExecutor.DiscardPolicy silently discards rejected tasks, permanently blocking future reloads because the isReloading flag is never reset. To resolve this, it is recommended to use ThreadPoolExecutor.AbortPolicy so that RejectedExecutionException is correctly thrown and handled.
9ad0f6d to
77f2b1a
Compare
styee
left a comment
There was a problem hiding this comment.
Code Review & Assessment for b/562755231
Thank you for working on this! This PR directly targets the issue described in b/562755231 ("PGAdapter: Rotated certificates are not picked up until restart"). Dynamically reloading mTLS client key material and server root CA certificates in Spanner Omni without requiring application or process restarts is a significant improvement.
The overall approach of using custom X509ExtendedKeyManager and X509ExtendedTrustManager implementations integrated into Netty's SslContext is the right design.
There are two critical issues in the current implementation, along with a few minor observations, that should be addressed before merging:
Critical Issues
1. Asynchronous reload causes the first connection after rotation to fail (Race Condition)
In both DynamicKeyManager.java and DynamicTrustManager.java:
void checkAndReload() {
...
if (checkIntervalNs == 0) {
try {
doReloadCheck(now);
} finally {
isReloading.set(false);
}
} else {
try {
ASYNC_RELOAD_EXECUTOR.execute(() -> {
try {
doReloadCheck(System.nanoTime());
} finally {
isReloading.set(false);
}
});
} catch (RejectedExecutionException e) {
isReloading.set(false);
}
}
}In production, checkIntervalMs defaults to 5000L (checkIntervalNs > 0). When a handshake occurs:
checkAndReload()is called fromchooseEngineClientAlias()orcheckServerTrusted().- It sees
now - lastCheckedNs >= checkIntervalNsand queuesdoReloadCheck()ontoASYNC_RELOAD_EXECUTOR. checkAndReload()returns immediately, before the background thread has read or parsed the updated files on disk.- The calling thread immediately proceeds with the handshake:
- In
DynamicKeyManager: it returnscurrentMaterial.alias(the old certificate/key). If the previous certificate is already expired, the client presents expired credentials and the server rejects the handshake. - In
DynamicTrustManager: Netty callscheckServerTrusted(), which validates the server's certificate againstcurrentMaterial.delegate(the old CA). If the server is presenting a certificate signed by the newly rotated CA, this throws aCertificateExceptionand immediately terminates the TLS handshake.
- In
Note: Unit tests did not catch this because all rotation tests instantiate the managers with checkIntervalMs = 0L, which takes the synchronous execution path.
Suggestions:
- Calling
File.lastModified()andFile.length()takes microseconds (< 5 µs). If timestamps and lengths have not changed, it returns immediately without any disk read or cryptographic parsing. - If a modification is detected, reload synchronously on that handshake so that the incoming connection uses the updated material.
- Alternatively: Run a proactive background timer (
ScheduledExecutorService) that checks and reloads the files periodically every 5–30s independently of handshakes. That way,currentMaterialis already fresh before any handshake arrives, avoiding both event-loop I/O and handshake failures.
2. JSSE Specification Violation in DynamicKeyManager.getCertificateChain(String alias)
In DynamicKeyManager.java:
@Override
public X509Certificate[] getCertificateChain(String alias) {
KeyMaterial mat = (alias != null) ? materials.get(alias) : this.currentMaterial;
return mat != null ? mat.certificateChain.clone() : new X509Certificate[0];
}The standard specification for X509KeyManager.getCertificateChain(String alias) states:
"Returns: the certificate chain associated with the given alias, or
nullif the alias can't be found."
Returning new X509Certificate[0] instead of null causes standard JSSE and TLS engine implementations that do:
X509Certificate[] certs = keyManager.getCertificateChain(alias);
if (certs != null) {
PublicKey pubKey = certs[0].getPublicKey();
}to fail with an ArrayIndexOutOfBoundsException: Index 0 out of bounds for length 0.
Suggestion:
Return null when mat == null (or when mat.certificateChain is empty):
@Override
public X509Certificate[] getCertificateChain(String alias) {
KeyMaterial mat = (alias != null) ? materials.get(alias) : this.currentMaterial;
return (mat != null && mat.certificateChain != null && mat.certificateChain.length > 0)
? mat.certificateChain.clone()
: null;
}Observations & Suggestions
-
SpannerOptions.toBuilder().build()configurator chaining:
InSpannerOptions.java, each call totoBuilder().build()wraps the existingchannelConfiguratorwith an additional lambda layer that re-appliessslContext. Consider ensuring that repeatedtoBuilder().build()calls do not accumulate redundant configurator layers. -
Documentation on CA certificate rotation:
InSpannerOptions.Builder,DynamicTrustManageris initialized only whenhasCaCertis true (builder.caCertificateis set). If a user only setsuseClientCert(...)withoutsetCaCertificate(...), the server root still relies on the static JVM trust store. Please highlight in the PGAdapter documentation that to enable dynamic CA rotation, users must explicitly supply the CA certificate path (e.g., via-r "caCertificate=/path/to/ca.crt;..."orsetCaCertificate(...)). -
Test coverage for non-zero check interval:
Adding a test forDynamicKeyManagerandDynamicTrustManagerwherecheckIntervalMs > 0and verifying that rotation works seamlessly would prevent regressions.
…panner Omni
Add support for zero-downtime dynamic reloading of client certificates/keys (mTLS)
and server root CA certificates in Spanner Omni without requiring application or connection pool restarts.
Changes:
- DynamicKeyManager (com.google.cloud.spanner.omni): An X509ExtendedKeyManager that
monitors certificate/key file modification times and lengths on disk. Uses non-blocking
tryLock() and atomic versioned alias mapping to reload rotated client certificates and
RSA/EC private keys during active TLS handshakes without blocking Netty event loop threads.
- DynamicTrustManager (com.google.cloud.spanner.omni): An X509ExtendedTrustManager that
dynamically reloads rotated server root CA certificates into an in-memory keystore/trust
manager upon file changes.
- SpannerOptions & Connection API:
- Added Builder.setCaCertificate(String) and getCaCertificate() across SpannerOptions,
ConnectionProperties, ConnectionOptions, and SpannerPool.
- Updated Builder.useClientCert(String, String) to use dynamic key management.
- Preserved raw certificate paths in SpannerOptions and built transient Netty SslContext
in prepareBuilder to prevent transport leaks.
- SpannerOmniHelper: Added support for spanner.ca_cert_path and updated mTLS setup detection.
- Tests: Added comprehensive unit tests covering dynamic certificate/key rotation, CA rotation,
multi-CA bundles, fallback on corruption/mismatch, throttling, and options configuration.
Fixes b/562755231
c197d47 to
9efa817
Compare
…ation - Preserve instanceType, usePlainText, and host in SpannerOptions and its Builder copy constructor so options.toBuilder().build() retains Spanner Omni settings. - Sample file timestamps and lengths before Files.readAllBytes() to prevent TOCTOU races during file rotation. - Use lock.lock() with double-checked locking in DynamicKeyManager and DynamicTrustManager to prevent handshake failure race conditions during concurrent requests. - Improve binary DER vs headerless Base64 key detection by checking for multi-byte sequence length tags. - Eliminate background thread pool executors to prevent thread and resource leaks. - Return null instead of empty array in DynamicKeyManager.getCertificateChain for unknown aliases according to JSSE specification. - Use unwrappable OmniSslChannelConfigurator in SpannerOptions to avoid nested lambda wrapping on repeated toBuilder().build() calls without modifying GapicSpannerRpc. - Unwrap OmniSslChannelConfigurator even when omniSslContext is null. - Fix SpannerPool to validate clientCertificate and clientCertificateKey independently so IllegalArgumentException is properly surfaced. - Add comprehensive unit tests covering concurrent handshakes during rotation, headerless Base64 keys, SpannerOptions toBuilder() preservation, and SpannerPool error handling.
9efa817 to
f0a17f3
Compare
Overview
In Spanner Omni deployments (e.g., on-premises, AWS EKS, GKE), client mTLS certificates/keys and server root CA certificates can be rotated on disk periodically by automated systems (such as Kubernetes
cert-manageror HashiCorp Vault).Previously, Java TLS components cached credentials in memory at initialization. When certificates were rotated on disk, long-running applications and proxies (such as PGAdapter) would eventually fail handshakes until the process or connection pool was restarted.
This PR adds zero-downtime dynamic TLS certificate and private key rotation for Spanner Omni, allowing clients to detect and pick up rotated credentials on disk with zero application restarts, zero downtime, and zero background thread leaks.
How It Works
lastModifiedandlength) are piggybacked synchronously onto connection attempts (< 5µs check), throttled to a default 5-second interval.ReentrantLockwith double-checked locking so concurrent handshakes during rotation wait for the reload rather than failing with stale or mismatched credentials.Key Changes
DynamicKeyManager(com.google.cloud.spanner.omni):X509ExtendedKeyManagerthat dynamically reloads client certificate chains and RSA/EC PKCS#8 private keys (PEM, Base64, and binary DER formats).DynamicTrustManager(com.google.cloud.spanner.omni):X509ExtendedTrustManagerthat dynamically reloads server root CA certificate bundles into an in-memory keystore.SpannerOptions:setCaCertificate(String)/getCaCertificate().useClientCert(String, String)to configure dynamic key management.OmniSslChannelConfiguratorto avoid nested lambda wrapping across.toBuilder().build()calls.instanceType,usePlainText, andhostacross.toBuilder().build().SpannerPool:caCertificateURI parameter and surfaced validation for partial certificate/key configurations.Issue Link
Fixes b/562755231