Skip to content

Commit 08bcf60

Browse files
Nicolas-EMNicolas-EM
authored
xds: Keep saved certs and roots after SslContext update (grpc#13085)
Fixes grpc#13058. `CertProviderSslContextProvider` cleared the saved key, cert chain and trusted roots after every `SslContext` build (`clearKeysAndCerts()`). When the identity cert and the CA roots come from separate certificate provider instances (for example, two `file_watcher` instances with different refresh intervals), the roots provider usually does not send another update. The next identity cert rotation then found no roots and did not rebuild the `SslContext`, so new connections kept the old, possibly expired, identity cert. The same problem occurred with a single shared provider instance when only the identity cert changed. This change removes `clearKeysAndCerts()`. The saved identity credentials and trust roots are now kept as the latest known values. An update from either provider rebuilds the `SslContext` with the latest values from both, as discussed in the issue. This generalizes grpc#12340, which kept the roots only when using system root certs. Behavior changes: - A root-only update now rebuilds the `SslContext`. Before, it did not, because the key had been cleared. - When a shared provider instance updates the cert and the roots in the same refresh, the `SslContext` is built twice. The first build briefly uses the new identity cert with the old roots. The next update immediately replaces it. Tests: - Updated the existing client and server tests for the new behavior, and removed assertions on the internal `saved*` fields after a build. - Added client and server tests for separate cert and root instances: cert-only updates, root-only updates, and updates that the other instance must ignore. - Added client and server tests for a shared instance with cert-only updates. - The new `*UpdateOnly` tests fail without the change in `CertProviderSslContextProvider`.
1 parent b731158 commit 08bcf60

3 files changed

Lines changed: 284 additions & 62 deletions

File tree

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

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -167,34 +167,24 @@ public final void updateSpiffeTrustMap(Map<String, List<X509Certificate>> spiffe
167167
updateSslContextWhenReady();
168168
}
169169

170+
// Saved credentials and roots persist across builds, so an update from either provider
171+
// rebuilds using the latest values from both.
170172
private void updateSslContextWhenReady() {
171173
if (isMtls()) {
172174
if (savedKey != null && (savedTrustedRoots != null || savedSpiffeTrustMap != null)) {
173175
updateSslContext();
174-
clearKeysAndCerts();
175176
}
176177
} else if (isRegularTlsAndClientSide()) {
177178
if (savedTrustedRoots != null || savedSpiffeTrustMap != null) {
178179
updateSslContext();
179-
clearKeysAndCerts();
180180
}
181181
} else if (isRegularTlsAndServerSide()) {
182182
if (savedKey != null) {
183183
updateSslContext();
184-
clearKeysAndCerts();
185184
}
186185
}
187186
}
188187

189-
private void clearKeysAndCerts() {
190-
savedKey = null;
191-
if (!isUsingSystemRootCerts) {
192-
savedTrustedRoots = null;
193-
savedSpiffeTrustMap = null;
194-
}
195-
savedCertChain = null;
196-
}
197-
198188
protected final boolean isMtls() {
199189
return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts);
200190
}

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

Lines changed: 140 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -154,9 +154,6 @@ public void testProviderForClient_mtls() throws Exception {
154154
// now generate root cert update
155155
watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
156156
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
157-
assertThat(provider.savedKey).isNull();
158-
assertThat(provider.savedCertChain).isNull();
159-
assertThat(provider.savedTrustedRoots).isNull();
160157

161158
TestCallback testCallback =
162159
CommonTlsContextTestsUtil.getValueThruCallback(provider);
@@ -166,27 +163,157 @@ public void testProviderForClient_mtls() throws Exception {
166163
CommonTlsContextTestsUtil.getValueThruCallback(provider);
167164
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
168165

169-
// just do root cert update: sslContext should still be the same
166+
// just do root cert update: sslContext should be updated
170167
watcherCaptor[0].updateTrustedRoots(
171168
ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE)));
172-
assertThat(provider.savedKey).isNull();
173-
assertThat(provider.savedCertChain).isNull();
174169
assertThat(provider.savedTrustedRoots).isNotNull();
175170
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
176-
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
171+
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
172+
testCallback = testCallback1;
177173

178174
// now update id cert: sslContext should be updated i.e.different from the previous one
179175
watcherCaptor[0].updateCertificate(
180176
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
181177
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
182-
assertThat(provider.savedKey).isNull();
183-
assertThat(provider.savedCertChain).isNull();
184-
assertThat(provider.savedTrustedRoots).isNull();
185178
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
186179
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
187180
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
188181
}
189182

183+
@Test
184+
public void testProviderForClient_mtls_sharedInstance_certUpdateOnly() throws Exception {
185+
final CertificateProvider.DistributorWatcher[] watcherCaptor =
186+
new CertificateProvider.DistributorWatcher[1];
187+
TestCertificateProvider.createAndRegisterProviderProvider(
188+
certificateProviderRegistry, watcherCaptor, "testca", 0);
189+
CertProviderClientSslContextProvider provider =
190+
getSslContextProvider(
191+
"gcp_id",
192+
"gcp_id",
193+
CommonBootstrapperTestUtils.getTestBootstrapInfo(),
194+
/* alpnProtocols= */ null,
195+
/* staticCertValidationContext= */ null, false);
196+
watcherCaptor[0].updateCertificate(
197+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
198+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
199+
watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
200+
TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider);
201+
assertThat(testCallback.updatedSslContext).isNotNull();
202+
203+
// just do id cert update: sslContext should be updated
204+
watcherCaptor[0].updateCertificate(
205+
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
206+
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
207+
TestCallback testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
208+
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
209+
210+
// another id cert update: sslContext should be updated again
211+
watcherCaptor[0].updateCertificate(
212+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
213+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
214+
TestCallback testCallback2 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
215+
assertThat(testCallback2.updatedSslContext)
216+
.isNotSameInstanceAs(testCallback1.updatedSslContext);
217+
}
218+
219+
/**
220+
* Helper method to build CertProviderClientSslContextProvider with separate cert and root
221+
* instances. watcherCaptor[0] is the cert watcher and watcherCaptor[1] is the root watcher.
222+
*/
223+
private CertProviderClientSslContextProvider getSslContextProviderWithSeparateInstances(
224+
CertificateProvider.DistributorWatcher[] watcherCaptor) {
225+
TestCertificateProvider.createAndRegisterProviderProvider(
226+
certificateProviderRegistry, watcherCaptor, "testca", 0);
227+
TestCertificateProvider.createAndRegisterProviderProvider(
228+
certificateProviderRegistry, watcherCaptor, "file_watcher", 1);
229+
return getSslContextProvider(
230+
"gcp_id",
231+
"file_provider",
232+
CommonBootstrapperTestUtils.getTestBootstrapInfo(),
233+
/* alpnProtocols= */ null,
234+
/* staticCertValidationContext= */ null, false);
235+
}
236+
237+
@Test
238+
public void testProviderForClient_mtls_separateInstances_certUpdateOnly() throws Exception {
239+
final CertificateProvider.DistributorWatcher[] watcherCaptor =
240+
new CertificateProvider.DistributorWatcher[2];
241+
CertProviderClientSslContextProvider provider =
242+
getSslContextProviderWithSeparateInstances(watcherCaptor);
243+
watcherCaptor[0].updateCertificate(
244+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
245+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
246+
watcherCaptor[1].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
247+
TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider);
248+
assertThat(testCallback.updatedSslContext).isNotNull();
249+
250+
// just do id cert update: sslContext should be updated
251+
watcherCaptor[0].updateCertificate(
252+
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
253+
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
254+
TestCallback testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
255+
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
256+
257+
// another id cert update: sslContext should be updated again
258+
watcherCaptor[0].updateCertificate(
259+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
260+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
261+
TestCallback testCallback2 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
262+
assertThat(testCallback2.updatedSslContext)
263+
.isNotSameInstanceAs(testCallback1.updatedSslContext);
264+
}
265+
266+
@Test
267+
public void testProviderForClient_mtls_separateInstances_rootUpdateOnly() throws Exception {
268+
final CertificateProvider.DistributorWatcher[] watcherCaptor =
269+
new CertificateProvider.DistributorWatcher[2];
270+
CertProviderClientSslContextProvider provider =
271+
getSslContextProviderWithSeparateInstances(watcherCaptor);
272+
watcherCaptor[0].updateCertificate(
273+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
274+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
275+
watcherCaptor[1].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
276+
TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider);
277+
assertThat(testCallback.updatedSslContext).isNotNull();
278+
279+
// just do root cert update: sslContext should be updated
280+
watcherCaptor[1].updateTrustedRoots(
281+
ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE)));
282+
TestCallback testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
283+
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
284+
285+
// another root cert update: sslContext should be updated again
286+
watcherCaptor[1].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
287+
TestCallback testCallback2 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
288+
assertThat(testCallback2.updatedSslContext)
289+
.isNotSameInstanceAs(testCallback1.updatedSslContext);
290+
}
291+
292+
@Test
293+
public void testProviderForClient_mtls_separateInstances_ignoresOtherInstanceUpdates()
294+
throws Exception {
295+
final CertificateProvider.DistributorWatcher[] watcherCaptor =
296+
new CertificateProvider.DistributorWatcher[2];
297+
CertProviderClientSslContextProvider provider =
298+
getSslContextProviderWithSeparateInstances(watcherCaptor);
299+
watcherCaptor[0].updateCertificate(
300+
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
301+
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
302+
watcherCaptor[1].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
303+
TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider);
304+
assertThat(testCallback.updatedSslContext).isNotNull();
305+
306+
// root cert update from the cert instance: sslContext should still be the same
307+
watcherCaptor[0].updateTrustedRoots(
308+
ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE)));
309+
// id cert update from the root instance: sslContext should still be the same
310+
watcherCaptor[1].updateCertificate(
311+
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
312+
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
313+
TestCallback testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
314+
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
315+
}
316+
190317
@Test
191318
public void testProviderForClient_systemRootCerts_mtls() throws Exception {
192319
final CertificateProvider.DistributorWatcher[] watcherCaptor =
@@ -214,8 +341,6 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception {
214341
watcherCaptor[0].updateCertificate(
215342
CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE),
216343
ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE)));
217-
assertThat(provider.savedKey).isNull();
218-
assertThat(provider.savedCertChain).isNull();
219344
assertThat(provider.savedTrustedRoots).isNotNull();
220345
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
221346

@@ -231,8 +356,6 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception {
231356
watcherCaptor[0].updateCertificate(
232357
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
233358
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
234-
assertThat(provider.savedKey).isNull();
235-
assertThat(provider.savedCertChain).isNull();
236359
assertThat(provider.savedTrustedRoots).isNotNull();
237360
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
238361
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
@@ -298,9 +421,6 @@ public void testProviderForClient_mtls_newXds() throws Exception {
298421
// now generate root cert update
299422
watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
300423
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
301-
assertThat(provider.savedKey).isNull();
302-
assertThat(provider.savedCertChain).isNull();
303-
assertThat(provider.savedTrustedRoots).isNull();
304424

305425
TestCallback testCallback =
306426
CommonTlsContextTestsUtil.getValueThruCallback(provider);
@@ -310,22 +430,18 @@ public void testProviderForClient_mtls_newXds() throws Exception {
310430
CommonTlsContextTestsUtil.getValueThruCallback(provider);
311431
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
312432

313-
// just do root cert update: sslContext should still be the same
433+
// just do root cert update: sslContext should be updated
314434
watcherCaptor[0].updateTrustedRoots(
315435
ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE)));
316-
assertThat(provider.savedKey).isNull();
317-
assertThat(provider.savedCertChain).isNull();
318436
assertThat(provider.savedTrustedRoots).isNotNull();
319437
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
320-
assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext);
438+
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
439+
testCallback = testCallback1;
321440

322441
// now update id cert: sslContext should be updated i.e.different from the previous one
323442
watcherCaptor[0].updateCertificate(
324443
CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE),
325444
ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE)));
326-
assertThat(provider.savedKey).isNull();
327-
assertThat(provider.savedCertChain).isNull();
328-
assertThat(provider.savedTrustedRoots).isNull();
329445
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
330446
testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider);
331447
assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);
@@ -388,7 +504,6 @@ public void testProviderForClient_tls() throws Exception {
388504
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
389505
assertThat(provider.savedKey).isNull();
390506
assertThat(provider.savedCertChain).isNull();
391-
assertThat(provider.savedTrustedRoots).isNull();
392507

393508
TestCallback testCallback =
394509
CommonTlsContextTestsUtil.getValueThruCallback(provider);
@@ -517,9 +632,6 @@ public void testProviderForClient_deprecatedCertProviderField() throws Exception
517632
// Generate root cert update
518633
watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE)));
519634
assertThat(provider.getSslContextAndTrustManager()).isNotNull();
520-
assertThat(provider.savedKey).isNull();
521-
assertThat(provider.savedCertChain).isNull();
522-
assertThat(provider.savedTrustedRoots).isNull();
523635

524636
TestCallback testCallback =
525637
CommonTlsContextTestsUtil.getValueThruCallback(provider);

0 commit comments

Comments
 (0)