Skip to content

Commit c14a488

Browse files
kannanjgithubkannanjgithub
committed
More fixes for system root certs.
1 parent d6acdfc commit c14a488

File tree

6 files changed

+104
-93
lines changed

6 files changed

+104
-93
lines changed

‎xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java‎

Lines changed: 5 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,10 @@
1616

1717
package io.grpc.xds.internal.security.certprovider;
1818

19-
import static java.util.Objects.requireNonNull;
20-
2119
import io.envoyproxy.envoy.config.core.v3.Node;
2220
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext;
2321
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext;
2422
import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProviderInstance;
25-
import io.grpc.Status;
2623
import io.grpc.xds.EnvoyServerProtoData.BaseTlsContext;
2724
import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo;
2825
import io.grpc.xds.internal.security.CommonTlsContextUtil;
@@ -69,7 +66,7 @@ protected CertProviderSslContextProvider(
6966
CertificateProviderInfo certProviderInstanceConfig =
7067
getCertProviderConfig(certProviders, certInstance.getInstanceName());
7168
CertificateProvider.Watcher watcher = this;
72-
if (!sharedCertInstance) {
69+
if (!sharedCertInstance && !isUsingSystemRootCerts) {
7370
watcher = new IgnoreUpdatesWatcher(watcher, /* ignoreRootCertUpdates= */ true);
7471
}
7572
// TODO: Previously we'd hang if certProviderInstanceConfig were null or
@@ -156,9 +153,6 @@ public final void updateCertificate(PrivateKey key, List<X509Certificate> certCh
156153

157154
@Override
158155
public final void updateTrustedRoots(List<X509Certificate> trustedRoots) {
159-
if (isUsingSystemRootCerts) {
160-
return;
161-
}
162156
savedTrustedRoots = trustedRoots;
163157
updateSslContextWhenReady();
164158
}
@@ -190,7 +184,9 @@ private void updateSslContextWhenReady() {
190184

191185
private void clearKeysAndCerts() {
192186
savedKey = null;
193-
savedTrustedRoots = null;
187+
if (!isUsingSystemRootCerts) {
188+
savedTrustedRoots = null;
189+
}
194190
savedSpiffeTrustMap = null;
195191
savedCertChain = null;
196192
}
@@ -200,10 +196,7 @@ protected final boolean isMtls() {
200196
}
201197

202198
protected final boolean isRegularTlsAndClientSide() {
203-
// We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of how this
204-
// method is used. With the rootCertInstance being null when using system root certs, there
205-
// is nothing to update in the SslContext
206-
return rootCertInstance != null && certInstance == null;
199+
return (rootCertInstance != null || isUsingSystemRootCerts) && certInstance == null;
207200
}
208201

209202
protected final boolean isRegularTlsAndServerSide() {
@@ -229,41 +222,4 @@ interface NoExceptionCloseable extends Closeable {
229222
@Override
230223
void close();
231224
}
232-
233-
static final class IgnoreUpdatesWatcher implements CertificateProvider.Watcher {
234-
private final CertificateProvider.Watcher delegate;
235-
private final boolean ignoreRootCertUpdates;
236-
237-
public IgnoreUpdatesWatcher(
238-
CertificateProvider.Watcher delegate, boolean ignoreRootCertUpdates) {
239-
this.delegate = requireNonNull(delegate, "delegate");
240-
this.ignoreRootCertUpdates = ignoreRootCertUpdates;
241-
}
242-
243-
@Override
244-
public void updateCertificate(PrivateKey key, List<X509Certificate> certChain) {
245-
if (ignoreRootCertUpdates) {
246-
delegate.updateCertificate(key, certChain);
247-
}
248-
}
249-
250-
@Override
251-
public void updateTrustedRoots(List<X509Certificate> trustedRoots) {
252-
if (!ignoreRootCertUpdates) {
253-
delegate.updateTrustedRoots(trustedRoots);
254-
}
255-
}
256-
257-
@Override
258-
public void updateSpiffeTrustMap(Map<String, List<X509Certificate>> spiffeTrustMap) {
259-
if (!ignoreRootCertUpdates) {
260-
delegate.updateSpiffeTrustMap(spiffeTrustMap);
261-
}
262-
}
263-
264-
@Override
265-
public void onError(Status errorStatus) {
266-
delegate.onError(errorStatus);
267-
}
268-
}
269225
}

‎xds/src/main/java/io/grpc/xds/internal/security/certprovider/IgnoreUpdatesWatcher.java‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
/*
2+
* Copyright 2025 The gRPC Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.grpc.xds.internal.security.certprovider;
18+
19+
import static java.util.Objects.requireNonNull;
20+
21+
import com.google.common.annotations.VisibleForTesting;
22+
import io.grpc.Status;
23+
import java.security.PrivateKey;
24+
import java.security.cert.X509Certificate;
25+
import java.util.List;
26+
import java.util.Map;
27+
28+
public final class IgnoreUpdatesWatcher implements CertificateProvider.Watcher {
29+
private final CertificateProvider.Watcher delegate;
30+
private final boolean ignoreRootCertUpdates;
31+
32+
public IgnoreUpdatesWatcher(
33+
CertificateProvider.Watcher delegate, boolean ignoreRootCertUpdates) {
34+
this.delegate = requireNonNull(delegate, "delegate");
35+
this.ignoreRootCertUpdates = ignoreRootCertUpdates;
36+
}
37+
38+
@Override
39+
public void updateCertificate(PrivateKey key, List<X509Certificate> certChain) {
40+
if (ignoreRootCertUpdates) {
41+
delegate.updateCertificate(key, certChain);
42+
}
43+
}
44+
45+
@Override
46+
public void updateTrustedRoots(List<X509Certificate> trustedRoots) {
47+
if (!ignoreRootCertUpdates) {
48+
delegate.updateTrustedRoots(trustedRoots);
49+
}
50+
}
51+
52+
@Override
53+
public void updateSpiffeTrustMap(Map<String, List<X509Certificate>> spiffeTrustMap) {
54+
if (!ignoreRootCertUpdates) {
55+
delegate.updateSpiffeTrustMap(spiffeTrustMap);
56+
}
57+
}
58+
59+
@Override
60+
public void onError(Status errorStatus) {
61+
delegate.onError(errorStatus);
62+
}
63+
64+
@VisibleForTesting
65+
public CertificateProvider.Watcher getDelegate() {
66+
return delegate;
67+
}
68+
}

‎xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -282,10 +282,6 @@ public void tlsClientServer_useSystemRootCerts_mtls() throws Exception {
282282
}
283283
}
284284

285-
/**
286-
* Use system root ca cert for TLS channel - no mTLS.
287-
* Subj Alt Names to match are specified in the validaton context.
288-
*/
289285
@Test
290286
public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() throws Exception {
291287
Path trustStoreFilePath = getCacertFilePathForTestCa();
@@ -317,7 +313,6 @@ public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() thro
317313

318314
/**
319315
* Use system root ca cert for TLS channel - mTLS.
320-
* Uses common_tls_context.combined_validation_context in upstream_tls_context.
321316
*/
322317
@Test
323318
public void tlsClientServer_useSystemRootCerts_requireClientAuth() throws Exception {

‎xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java‎

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
import io.grpc.xds.internal.security.certprovider.CertificateProviderProvider;
3838
import io.grpc.xds.internal.security.certprovider.CertificateProviderRegistry;
3939
import io.grpc.xds.internal.security.certprovider.CertificateProviderStore;
40+
import io.grpc.xds.internal.security.certprovider.IgnoreUpdatesWatcher;
4041
import io.grpc.xds.internal.security.certprovider.TestCertificateProvider;
4142
import org.junit.Before;
4243
import org.junit.Test;
@@ -84,7 +85,7 @@ public void createCertProviderClientSslContextProvider() throws XdsInitializatio
8485
clientSslContextProviderFactory.create(upstreamTlsContext);
8586
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
8687
"CertProviderClientSslContextProvider");
87-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
88+
verifyWatcher(sslContextProvider, watcherCaptor[0], false);
8889
// verify that bootstrapInfo is cached...
8990
sslContextProvider =
9091
clientSslContextProviderFactory.create(upstreamTlsContext);
@@ -119,7 +120,7 @@ public void bothPresent_expectCertProviderClientSslContextProvider()
119120
clientSslContextProviderFactory.create(upstreamTlsContext);
120121
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
121122
"CertProviderClientSslContextProvider");
122-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
123+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
123124
}
124125

125126
@Test
@@ -145,7 +146,7 @@ public void createCertProviderClientSslContextProvider_onlyRootCert()
145146
clientSslContextProviderFactory.create(upstreamTlsContext);
146147
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
147148
"CertProviderClientSslContextProvider");
148-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
149+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
149150
}
150151

151152
@Test
@@ -179,7 +180,7 @@ public void createCertProviderClientSslContextProvider_withStaticContext()
179180
clientSslContextProviderFactory.create(upstreamTlsContext);
180181
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
181182
"CertProviderClientSslContextProvider");
182-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
183+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
183184
}
184185

185186
@Test
@@ -209,8 +210,8 @@ public void createCertProviderClientSslContextProvider_2providers()
209210
clientSslContextProviderFactory.create(upstreamTlsContext);
210211
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
211212
"CertProviderClientSslContextProvider");
212-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
213-
verifyWatcher(sslContextProvider, watcherCaptor[1]);
213+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
214+
verifyWatcher(sslContextProvider, watcherCaptor[1], true);
214215
}
215216

216217
@Test
@@ -246,8 +247,8 @@ public void createNewCertProviderClientSslContextProvider_withSans() {
246247
clientSslContextProviderFactory.create(upstreamTlsContext);
247248
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
248249
"CertProviderClientSslContextProvider");
249-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
250-
verifyWatcher(sslContextProvider, watcherCaptor[1]);
250+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
251+
verifyWatcher(sslContextProvider, watcherCaptor[1], true);
251252
}
252253

253254
@Test
@@ -280,7 +281,7 @@ public void createNewCertProviderClientSslContextProvider_onlyRootCert() {
280281
clientSslContextProviderFactory.create(upstreamTlsContext);
281282
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
282283
"CertProviderClientSslContextProvider");
283-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
284+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
284285
}
285286

286287
static void createAndRegisterProviderProvider(
@@ -310,11 +311,18 @@ public CertificateProvider answer(InvocationOnMock invocation) throws Throwable
310311
}
311312

312313
static void verifyWatcher(
313-
SslContextProvider sslContextProvider, CertificateProvider.DistributorWatcher watcherCaptor) {
314+
SslContextProvider sslContextProvider, CertificateProvider.DistributorWatcher watcherCaptor,
315+
boolean usesDelegateWatcher) {
314316
assertThat(watcherCaptor).isNotNull();
315317
assertThat(watcherCaptor.getDownstreamWatchers()).hasSize(1);
316-
assertThat(watcherCaptor.getDownstreamWatchers().iterator().next())
317-
.isSameInstanceAs(sslContextProvider);
318+
if (usesDelegateWatcher) {
319+
assertThat(((IgnoreUpdatesWatcher) watcherCaptor.getDownstreamWatchers().iterator().next())
320+
.getDelegate())
321+
.isSameInstanceAs(sslContextProvider);
322+
} else {
323+
assertThat(watcherCaptor.getDownstreamWatchers().iterator().next())
324+
.isSameInstanceAs(sslContextProvider);
325+
}
318326
}
319327

320328
static CommonTlsContext.Builder addFilenames(

‎xds/src/test/java/io/grpc/xds/internal/security/ServerSslContextProviderFactoryTest.java‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -78,7 +78,7 @@ public void createCertProviderServerSslContextProvider() throws XdsInitializatio
7878
serverSslContextProviderFactory.create(downstreamTlsContext);
7979
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
8080
"CertProviderServerSslContextProvider");
81-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
81+
verifyWatcher(sslContextProvider, watcherCaptor[0], false);
8282
// verify that bootstrapInfo is cached...
8383
sslContextProvider =
8484
serverSslContextProviderFactory.create(downstreamTlsContext);
@@ -117,7 +117,7 @@ public void bothPresent_expectCertProviderServerSslContextProvider()
117117
serverSslContextProviderFactory.create(downstreamTlsContext);
118118
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
119119
"CertProviderServerSslContextProvider");
120-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
120+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
121121
}
122122

123123
@Test
@@ -144,7 +144,7 @@ public void createCertProviderServerSslContextProvider_onlyCertInstance()
144144
serverSslContextProviderFactory.create(downstreamTlsContext);
145145
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
146146
"CertProviderServerSslContextProvider");
147-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
147+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
148148
}
149149

150150
@Test
@@ -179,7 +179,7 @@ public void createCertProviderServerSslContextProvider_withStaticContext()
179179
serverSslContextProviderFactory.create(downstreamTlsContext);
180180
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
181181
"CertProviderServerSslContextProvider");
182-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
182+
verifyWatcher(sslContextProvider, watcherCaptor[0], false);
183183
}
184184

185185
@Test
@@ -210,8 +210,8 @@ public void createCertProviderServerSslContextProvider_2providers()
210210
serverSslContextProviderFactory.create(downstreamTlsContext);
211211
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
212212
"CertProviderServerSslContextProvider");
213-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
214-
verifyWatcher(sslContextProvider, watcherCaptor[1]);
213+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
214+
verifyWatcher(sslContextProvider, watcherCaptor[1], true);
215215
}
216216

217217
@Test
@@ -249,7 +249,7 @@ public void createNewCertProviderServerSslContextProvider_withSans()
249249
serverSslContextProviderFactory.create(downstreamTlsContext);
250250
assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo(
251251
"CertProviderServerSslContextProvider");
252-
verifyWatcher(sslContextProvider, watcherCaptor[0]);
253-
verifyWatcher(sslContextProvider, watcherCaptor[1]);
252+
verifyWatcher(sslContextProvider, watcherCaptor[0], true);
253+
verifyWatcher(sslContextProvider, watcherCaptor[1], true);
254254
}
255255
}

‎xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java‎

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -235,22 +235,16 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception {
235235

236236
assertThat(provider.savedKey).isNull();
237237
assertThat(provider.savedCertChain).isNull();
238-
assertThat(provider.savedTrustedRoots).isNull();
239-
assertThat(provider.getSslContext()).isNull();
240-
241-
// now generate root cert update, will get ignored because of systemRootCerts config
242-
watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
238+
assertThat(provider.savedTrustedRoots).isNotNull();
243239
assertThat(provider.getSslContext()).isNull();
244-
assertThat(provider.savedKey).isNull();
245-
assertThat(provider.savedCertChain).isNull();
246-
assertThat(provider.savedTrustedRoots).isNull();
247240

248241
// now generate cert update
249242
watcherCaptor[0].updateCertificate(
250243
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
251244
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
252245
assertThat(provider.savedKey).isNull();
253246
assertThat(provider.savedCertChain).isNull();
247+
assertThat(provider.savedTrustedRoots).isNotNull();
254248
assertThat(provider.getSslContext()).isNotNull();
255249

256250
TestCallback testCallback =
@@ -261,23 +255,13 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception {
261255
CommonTlsContextTestsUtil.getValueThruCallback(provider);
262256
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
263257

264-
// just do root cert update: sslContext should still be the same, will get ignored because of
265-
// systemRootCerts config
266-
watcherCaptor[0].updateTrustedRoots(
267-
ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE)));
268-
assertThat(provider.savedKey).isNull();
269-
assertThat(provider.savedCertChain).isNull();
270-
assertThat(provider.savedTrustedRoots).isNull();
271-
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
272-
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
273-
274258
// now update id cert: sslContext should be updated i.e. different from the previous one
275259
watcherCaptor[0].updateCertificate(
276260
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
277261
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
278262
assertThat(provider.savedKey).isNull();
279263
assertThat(provider.savedCertChain).isNull();
280-
assertThat(provider.savedTrustedRoots).isNull();
264+
assertThat(provider.savedTrustedRoots).isNotNull();
281265
assertThat(provider.getSslContext()).isNotNull();
282266
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
283267
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);

0 commit comments

Comments
 (0)