From 97413ed8fbf4570b7982d7facd28a9a4ed3aa55b Mon Sep 17 00:00:00 2001 From: Anshika Pandey Date: Wed, 16 Sep 2026 12:59:23 +0530 Subject: [PATCH] xds: fix identity cert rotation with separate CA provider --- .../CertProviderSslContextProvider.java | 11 ++- ...tProviderClientSslContextProviderTest.java | 77 +++++++++++++++++-- ...tProviderServerSslContextProviderTest.java | 14 +++- 3 files changed, 88 insertions(+), 14 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java index 948b6ceeb9e..bb08c7864db 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderSslContextProvider.java @@ -187,11 +187,14 @@ private void updateSslContextWhenReady() { } private void clearKeysAndCerts() { + // savedTrustedRoots/savedSpiffeTrustMap are deliberately not cleared here: the root/CA + // provider instance is independent of the identity cert provider instance (they may be two + // separate file_watcher instances polling on different schedules) and may not push another + // update for a long time, or ever again. Clearing them after a rebuild triggered solely by an + // identity-cert update would make updateSslContextWhenReady() get stuck waiting forever for a + // root update that never comes, silently freezing the SslContext on the original identity + // cert. The last known-good trust roots remain valid until the root provider pushes new ones. savedKey = null; - if (!isUsingSystemRootCerts) { - savedTrustedRoots = null; - savedSpiffeTrustMap = null; - } savedCertChain = null; } diff --git a/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java b/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java index 91f02863ca4..dcbb5c69295 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProviderTest.java @@ -30,6 +30,7 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; import com.google.common.util.concurrent.MoreExecutors; import io.envoyproxy.envoy.config.core.v3.DataSource; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; @@ -155,7 +156,10 @@ public void testProviderForClient_mtls() throws Exception { assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + // Trust roots are not cleared: the root provider is independent of the identity provider and + // may not push another update for a long time (or ever), so the last known-good value must + // be retained to allow future identity-only rotations to still trigger a rebuild. + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); @@ -180,12 +184,70 @@ public void testProviderForClient_mtls() throws Exception { ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContextAndTrustManager()).isNotNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); } + /** + * Regression test for https://github.com/grpc/grpc-java/issues/13058: the identity cert and + * the CA trust bundle are served by two *separate* certificate provider instances (as happens + * with two file_watcher instances on independent refresh schedules). Once the root/CA + * instance stops sending updates (e.g. its backing file never changes again), a later + * identity-cert-only rotation on the other, independent instance must still trigger a rebuild, + * reusing the last known-good trust roots rather than getting stuck forever. + */ + @Test + public void testProviderForClient_mtls_separateRootInstance_identityRotationRebuildsContext() + throws Exception { + final CertificateProvider.DistributorWatcher[] watcherCaptor = + new CertificateProvider.DistributorWatcher[2]; + TestCertificateProvider.createAndRegisterProviderProvider( + certificateProviderRegistry, watcherCaptor, "testca_identity", 0); + TestCertificateProvider.createAndRegisterProviderProvider( + certificateProviderRegistry, watcherCaptor, "testca_root", 1); + + Bootstrapper.BootstrapInfo bootstrapInfo = + Bootstrapper.BootstrapInfo.builder() + .servers(ImmutableList.of()) + .node(CommonBootstrapperTestUtils.getTestBootstrapInfo().node()) + .certProviders(ImmutableMap.of( + "identity_instance", + Bootstrapper.CertificateProviderInfo.create("testca_identity", ImmutableMap.of()), + "ca_instance", + Bootstrapper.CertificateProviderInfo.create("testca_root", ImmutableMap.of()))) + .build(); + + CertProviderClientSslContextProvider provider = + getSslContextProvider( + "identity_instance", + "ca_instance", + bootstrapInfo, + /* alpnProtocols= */ null, + /* staticCertValidationContext= */ null, false); + + // Initial mTLS build: the identity provider and the independent root provider each fire once. + watcherCaptor[0].updateCertificate( + CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE), + ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE))); + watcherCaptor[1].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE))); + assertThat(provider.getSslContextAndTrustManager()).isNotNull(); + + TestCallback initialCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(initialCallback.updatedSslContext).isNotNull(); + + // The CA/root provider never fires again (its file never changes), but the identity cert + // rotates on its own, independent schedule. + watcherCaptor[0].updateCertificate( + CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE), + ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); + + TestCallback afterRotationCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(afterRotationCallback.updatedSslContext) + .isNotSameInstanceAs(initialCallback.updatedSslContext); + } + @Test public void testProviderForClient_systemRootCerts_mtls() throws Exception { final CertificateProvider.DistributorWatcher[] watcherCaptor = @@ -299,7 +361,10 @@ public void testProviderForClient_mtls_newXds() throws Exception { assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + // Trust roots are not cleared: the root provider is independent of the identity provider and + // may not push another update for a long time (or ever), so the last known-good value must + // be retained to allow future identity-only rotations to still trigger a rebuild. + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); @@ -324,7 +389,7 @@ public void testProviderForClient_mtls_newXds() throws Exception { ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContextAndTrustManager()).isNotNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); @@ -387,7 +452,7 @@ public void testProviderForClient_tls() throws Exception { assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); @@ -514,7 +579,7 @@ public void testProviderForClient_deprecatedCertProviderField() throws Exception assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); diff --git a/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderServerSslContextProviderTest.java b/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderServerSslContextProviderTest.java index 93559f47245..5f74fc9e295 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderServerSslContextProviderTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/certprovider/CertProviderServerSslContextProviderTest.java @@ -142,7 +142,10 @@ public void testProviderForServer_mtls() throws Exception { assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + // Trust roots are not cleared: the root provider is independent of the identity provider and + // may not push another update for a long time (or ever), so the last known-good value must + // be retained to allow future identity-only rotations to still trigger a rebuild. + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); @@ -167,7 +170,7 @@ public void testProviderForServer_mtls() throws Exception { ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContextAndTrustManager()).isNotNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); @@ -211,7 +214,10 @@ public void testProviderForServer_mtls_newXds() throws Exception { assertThat(provider.getSslContextAndTrustManager()).isNotNull(); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + // Trust roots are not cleared: the root provider is independent of the identity provider and + // may not push another update for a long time (or ever), so the last known-good value must + // be retained to allow future identity-only rotations to still trigger a rebuild. + assertThat(provider.savedTrustedRoots).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); @@ -236,7 +242,7 @@ public void testProviderForServer_mtls_newXds() throws Exception { ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContextAndTrustManager()).isNotNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext);