Skip to content

Commit c197d47

Browse files
sagnghossagnghos
committed
fix(spanner): Address review feedback for dynamic TLS certificate rotation
- Use synchronous check and reload on connection attempt under ReentrantLock to prevent first-connection handshake failure race condition. - Eliminate background thread pool executor and avoid any thread or resource leaks. - Return null instead of empty array in DynamicKeyManager.getCertificateChain for unknown alias according to JSSE specification. - Use unwrappable OmniSslChannelConfigurator in SpannerOptions to avoid nested lambda wrapping on repeated toBuilder().build() calls without modifying GapicSpannerRpc. - Add unit tests for non-zero interval rotation and SpannerOptions toBuilder() configurator preservation.
1 parent f987d02 commit c197d47

6 files changed

Lines changed: 169 additions & 118 deletions

File tree

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/SpannerOptions.java‎

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -951,22 +951,16 @@ protected SpannerOptions(Builder builder) {
951951
clientCertificateKey = builder.clientCertificateKey;
952952
caCertificate = builder.caCertificate;
953953
if (builder.omniSslContext != null) {
954-
final SslContext sslContext = builder.omniSslContext;
955954
@SuppressWarnings("rawtypes")
956-
final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> parentConfigurator =
955+
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> baseConfigurator =
957956
builder.channelConfigurator;
958-
channelConfigurator =
959-
channelBuilder -> {
960-
if (parentConfigurator != null) {
961-
channelBuilder = parentConfigurator.apply(channelBuilder);
962-
}
963-
if (channelBuilder instanceof NettyChannelBuilder) {
964-
((NettyChannelBuilder) channelBuilder).sslContext(sslContext);
965-
}
966-
return channelBuilder;
967-
};
957+
while (baseConfigurator instanceof OmniSslChannelConfigurator) {
958+
baseConfigurator = ((OmniSslChannelConfigurator) baseConfigurator).getUserConfigurator();
959+
}
960+
this.channelConfigurator =
961+
new OmniSslChannelConfigurator(baseConfigurator, builder.omniSslContext);
968962
} else {
969-
channelConfigurator = builder.channelConfigurator;
963+
this.channelConfigurator = builder.channelConfigurator;
970964
}
971965
interceptorProvider = builder.interceptorProvider;
972966
sessionPoolOptions =
@@ -2729,6 +2723,35 @@ public ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> getChannelConfi
27292723
return channelConfigurator;
27302724
}
27312725

2726+
@SuppressWarnings("rawtypes")
2727+
private static class OmniSslChannelConfigurator
2728+
implements ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> {
2729+
private final ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> userConfigurator;
2730+
private final SslContext sslContext;
2731+
2732+
OmniSslChannelConfigurator(
2733+
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> userConfigurator,
2734+
SslContext sslContext) {
2735+
this.userConfigurator = userConfigurator;
2736+
this.sslContext = sslContext;
2737+
}
2738+
2739+
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> getUserConfigurator() {
2740+
return userConfigurator;
2741+
}
2742+
2743+
@Override
2744+
public ManagedChannelBuilder apply(ManagedChannelBuilder channelBuilder) {
2745+
if (userConfigurator != null) {
2746+
channelBuilder = userConfigurator.apply(channelBuilder);
2747+
}
2748+
if (channelBuilder instanceof NettyChannelBuilder) {
2749+
((NettyChannelBuilder) channelBuilder).sslContext(sslContext);
2750+
}
2751+
return channelBuilder;
2752+
}
2753+
}
2754+
27322755
public GrpcInterceptorProvider getInterceptorProvider() {
27332756
return interceptorProvider;
27342757
}

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/DynamicKeyManager.java‎

Lines changed: 14 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -37,13 +37,8 @@
3737
import java.util.Base64;
3838
import java.util.Collection;
3939
import java.util.concurrent.ConcurrentHashMap;
40-
import java.util.concurrent.ExecutorService;
41-
import java.util.concurrent.LinkedBlockingQueue;
42-
import java.util.concurrent.RejectedExecutionException;
43-
import java.util.concurrent.ThreadPoolExecutor;
44-
import java.util.concurrent.TimeUnit;
45-
import java.util.concurrent.atomic.AtomicBoolean;
4640
import java.util.concurrent.atomic.AtomicLong;
41+
import java.util.concurrent.locks.ReentrantLock;
4742
import java.util.logging.Level;
4843
import java.util.logging.Logger;
4944
import javax.net.ssl.SSLEngine;
@@ -57,34 +52,15 @@
5752
public class DynamicKeyManager extends X509ExtendedKeyManager {
5853
private static final Logger logger = Logger.getLogger(DynamicKeyManager.class.getName());
5954
private static final long DEFAULT_CHECK_INTERVAL_MS = 5000L;
60-
private static final ExecutorService ASYNC_RELOAD_EXECUTOR = createAsyncReloadExecutor();
6155

6256
private final File certFile;
6357
private final File keyFile;
6458
private final long checkIntervalNs;
6559
private final ConcurrentHashMap<String, KeyMaterial> materials = new ConcurrentHashMap<>();
6660
private final AtomicLong versionCounter = new AtomicLong();
67-
private final AtomicBoolean isReloading = new AtomicBoolean(false);
61+
private final ReentrantLock lock = new ReentrantLock();
6862
private volatile long lastCheckedNs;
6963

70-
private static ExecutorService createAsyncReloadExecutor() {
71-
ThreadPoolExecutor executor =
72-
new ThreadPoolExecutor(
73-
1,
74-
2,
75-
60L,
76-
TimeUnit.SECONDS,
77-
new LinkedBlockingQueue<>(10),
78-
runnable -> {
79-
Thread t = new Thread(runnable, "spanner-omni-cert-reloader");
80-
t.setDaemon(true);
81-
return t;
82-
},
83-
new ThreadPoolExecutor.AbortPolicy());
84-
executor.allowCoreThreadTimeOut(true);
85-
return executor;
86-
}
87-
8864
private static class CertificateFactoryHolder {
8965
static final CertificateFactory INSTANCE;
9066

@@ -156,39 +132,21 @@ void checkAndReload() {
156132
if (checkIntervalNs > 0 && now - lastCheckedNs < checkIntervalNs) {
157133
return;
158134
}
159-
if (!isReloading.compareAndSet(false, true)) {
135+
if (!lock.tryLock()) {
160136
return;
161137
}
162-
if (checkIntervalNs == 0) {
163-
try {
164-
doReloadCheck(now);
165-
} finally {
166-
isReloading.set(false);
167-
}
168-
} else {
169-
try {
170-
ASYNC_RELOAD_EXECUTOR.execute(
171-
() -> {
172-
try {
173-
doReloadCheck(System.nanoTime());
174-
} finally {
175-
isReloading.set(false);
176-
}
177-
});
178-
} catch (RejectedExecutionException e) {
179-
isReloading.set(false);
180-
}
181-
}
182-
}
183-
184-
private void doReloadCheck(long now) {
185138
try {
139+
long nowInLock = System.nanoTime();
140+
if (checkIntervalNs > 0 && nowInLock - lastCheckedNs < checkIntervalNs) {
141+
return;
142+
}
186143
KeyMaterial existing = this.currentMaterial;
187144
if (existing != null
188145
&& certFile.lastModified() == existing.certLastModified
189146
&& certFile.length() == existing.certLength
190147
&& keyFile.lastModified() == existing.keyLastModified
191148
&& keyFile.length() == existing.keyLength) {
149+
lastCheckedNs = nowInLock;
192150
return;
193151
}
194152
try {
@@ -198,9 +156,11 @@ private void doReloadCheck(long now) {
198156
Level.WARNING,
199157
"Failed to reload rotated client certificate/key from disk, retaining current material",
200158
e);
159+
} finally {
160+
lastCheckedNs = System.nanoTime();
201161
}
202162
} finally {
203-
lastCheckedNs = now;
163+
lock.unlock();
204164
}
205165
}
206166

@@ -342,7 +302,9 @@ public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSL
342302
@Override
343303
public X509Certificate[] getCertificateChain(String alias) {
344304
KeyMaterial mat = (alias != null) ? materials.get(alias) : this.currentMaterial;
345-
return mat != null ? mat.certificateChain.clone() : new X509Certificate[0];
305+
return (mat != null && mat.certificateChain != null && mat.certificateChain.length > 0)
306+
? mat.certificateChain.clone()
307+
: null;
346308
}
347309

348310
@Override

‎java-spanner/google-cloud-spanner/src/main/java/com/google/cloud/spanner/omni/DynamicTrustManager.java‎

Lines changed: 11 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,7 @@
2828
import java.security.cert.CertificateFactory;
2929
import java.security.cert.X509Certificate;
3030
import java.util.Collection;
31-
import java.util.concurrent.ExecutorService;
32-
import java.util.concurrent.LinkedBlockingQueue;
33-
import java.util.concurrent.RejectedExecutionException;
34-
import java.util.concurrent.ThreadPoolExecutor;
35-
import java.util.concurrent.TimeUnit;
36-
import java.util.concurrent.atomic.AtomicBoolean;
31+
import java.util.concurrent.locks.ReentrantLock;
3732
import java.util.logging.Level;
3833
import java.util.logging.Logger;
3934
import javax.annotation.Nullable;
@@ -51,31 +46,12 @@
5146
public class DynamicTrustManager extends X509ExtendedTrustManager {
5247
private static final Logger logger = Logger.getLogger(DynamicTrustManager.class.getName());
5348
private static final long DEFAULT_CHECK_INTERVAL_MS = 5000L;
54-
private static final ExecutorService ASYNC_RELOAD_EXECUTOR = createAsyncReloadExecutor();
5549

5650
private final File caCertFile;
5751
private final long checkIntervalNs;
58-
private final AtomicBoolean isReloading = new AtomicBoolean(false);
52+
private final ReentrantLock lock = new ReentrantLock();
5953
private volatile long lastCheckedNs;
6054

61-
private static ExecutorService createAsyncReloadExecutor() {
62-
ThreadPoolExecutor executor =
63-
new ThreadPoolExecutor(
64-
1,
65-
2,
66-
60L,
67-
TimeUnit.SECONDS,
68-
new LinkedBlockingQueue<>(10),
69-
runnable -> {
70-
Thread t = new Thread(runnable, "spanner-omni-ca-reloader");
71-
t.setDaemon(true);
72-
return t;
73-
},
74-
new ThreadPoolExecutor.AbortPolicy());
75-
executor.allowCoreThreadTimeOut(true);
76-
return executor;
77-
}
78-
7955
private static class CertificateFactoryHolder {
8056
static final CertificateFactory INSTANCE;
8157

@@ -133,37 +109,19 @@ void checkAndReload() {
133109
if (checkIntervalNs > 0 && now - lastCheckedNs < checkIntervalNs) {
134110
return;
135111
}
136-
if (!isReloading.compareAndSet(false, true)) {
112+
if (!lock.tryLock()) {
137113
return;
138114
}
139-
if (checkIntervalNs == 0) {
140-
try {
141-
doReloadCheck(now);
142-
} finally {
143-
isReloading.set(false);
144-
}
145-
} else {
146-
try {
147-
ASYNC_RELOAD_EXECUTOR.execute(
148-
() -> {
149-
try {
150-
doReloadCheck(System.nanoTime());
151-
} finally {
152-
isReloading.set(false);
153-
}
154-
});
155-
} catch (RejectedExecutionException e) {
156-
isReloading.set(false);
157-
}
158-
}
159-
}
160-
161-
private void doReloadCheck(long now) {
162115
try {
116+
long nowInLock = System.nanoTime();
117+
if (checkIntervalNs > 0 && nowInLock - lastCheckedNs < checkIntervalNs) {
118+
return;
119+
}
163120
TrustMaterial existing = this.currentMaterial;
164121
if (existing != null
165122
&& caCertFile.lastModified() == existing.lastModified
166123
&& caCertFile.length() == existing.length) {
124+
lastCheckedNs = nowInLock;
167125
return;
168126
}
169127
try {
@@ -173,9 +131,11 @@ private void doReloadCheck(long now) {
173131
Level.WARNING,
174132
"Failed to reload rotated CA certificate from disk, retaining previous material",
175133
e);
134+
} finally {
135+
lastCheckedNs = System.nanoTime();
176136
}
177137
} finally {
178-
lastCheckedNs = now;
138+
lock.unlock();
179139
}
180140
}
181141

‎java-spanner/google-cloud-spanner/src/test/java/com/google/cloud/spanner/SpannerOptionsTest.java‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@
3030
import static org.junit.Assert.assertTrue;
3131
import static org.mockito.Mockito.mock;
3232

33+
import com.google.api.core.ApiFunction;
3334
import com.google.api.gax.grpc.GrpcCallContext;
3435
import com.google.api.gax.grpc.InstantiatingGrpcChannelProvider;
3536
import com.google.api.gax.retrying.RetrySettings;
@@ -70,6 +71,7 @@
7071
import com.google.spanner.v1.RollbackRequest;
7172
import com.google.spanner.v1.SpannerGrpc;
7273
import com.google.spanner.v1.TransactionOptions.IsolationLevel;
74+
import io.grpc.ManagedChannelBuilder;
7375
import io.grpc.MethodDescriptor;
7476
import io.grpc.netty.shaded.io.netty.handler.ssl.util.SelfSignedCertificate;
7577
import io.opentelemetry.api.GlobalOpenTelemetry;
@@ -1773,4 +1775,41 @@ public void testUseClientCertAndCaCertificateEmptyValidation() {
17731775
assertThrows(
17741776
IllegalArgumentException.class, () -> SpannerOptions.newBuilder().setCaCertificate(null));
17751777
}
1778+
1779+
@Test
1780+
public void testToBuilderPreservesChannelConfiguratorWithoutChaining() throws Exception {
1781+
SelfSignedCertificate ssc = new SelfSignedCertificate("spanner.test.configurator");
1782+
try {
1783+
String certPath = ssc.certificate().getAbsolutePath();
1784+
String keyPath = ssc.privateKey().getAbsolutePath();
1785+
1786+
final int[] configuratorCallCount = new int[] {0};
1787+
@SuppressWarnings("rawtypes")
1788+
ApiFunction<ManagedChannelBuilder, ManagedChannelBuilder> customConfigurator =
1789+
builder -> {
1790+
configuratorCallCount[0]++;
1791+
return builder;
1792+
};
1793+
1794+
SpannerOptions options =
1795+
SpannerOptions.newBuilder()
1796+
.setProjectId("test-project")
1797+
.setCredentials(NoCredentials.getInstance())
1798+
.setHost("https://localhost:1234")
1799+
.useClientCert(certPath, keyPath)
1800+
.setChannelConfigurator(customConfigurator)
1801+
.build();
1802+
1803+
// Repeated toBuilder().build()
1804+
SpannerOptions options2 = options.toBuilder().build();
1805+
SpannerOptions options3 = options2.toBuilder().build();
1806+
1807+
ManagedChannelBuilder<?> dummyBuilder = mock(ManagedChannelBuilder.class);
1808+
options3.getChannelConfigurator().apply(dummyBuilder);
1809+
// The custom configurator should only be called once, not 3 times due to redundant nesting
1810+
assertEquals(1, configuratorCallCount[0]);
1811+
} finally {
1812+
ssc.delete();
1813+
}
1814+
}
17761815
}

0 commit comments

Comments
 (0)