From 40769980a0236f097a9613d30e082964a0ca57a2 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 8 Sep 2025 06:14:52 +0000 Subject: [PATCH 01/23] Save changes. --- .../CertProviderSslContextProvider.java | 3 + .../security/CommonTlsContextTestsUtil.java | 7 ++- ...tProviderClientSslContextProviderTest.java | 57 +++++++++++++++++++ 3 files changed, 64 insertions(+), 3 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 801dabeecb7..639fe92eab6 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 @@ -138,6 +138,9 @@ public final void updateCertificate(PrivateKey key, List certCh @Override public final void updateTrustedRoots(List trustedRoots) { + if (isUsingSystemRootCerts) { + return; + } savedTrustedRoots = trustedRoots; updateSslContextWhenReady(); } diff --git a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java index 48814dece1d..5ad1d757114 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java @@ -229,9 +229,6 @@ private static CommonTlsContext.Builder addCertificateValidationContext( String rootInstanceName, String rootCertName, CertificateValidationContext staticCertValidationContext) { - if (staticCertValidationContext == null && rootInstanceName == null) { - return builder; - } CertificateValidationContext.Builder contextBuilder; if (staticCertValidationContext == null) { contextBuilder = CertificateValidationContext.newBuilder(); @@ -243,6 +240,10 @@ private static CommonTlsContext.Builder addCertificateValidationContext( .setInstanceName(rootInstanceName) .setCertificateName(rootCertName)); builder.setValidationContext(contextBuilder.build()); + } else { + builder.setValidationContext(contextBuilder.setSystemRootCerts( + CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build()); } return builder.setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder() .setDefaultValidationContext(contextBuilder)); 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 b0800458d66..fe8b1cb9b4c 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 @@ -173,6 +173,63 @@ public void testProviderForClient_mtls() throws Exception { assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); } + @Test + public void testProviderForClient_systemRootCerts() throws Exception { + final CertificateProvider.DistributorWatcher[] watcherCaptor = + new CertificateProvider.DistributorWatcher[1]; + TestCertificateProvider.createAndRegisterProviderProvider( + certificateProviderRegistry, watcherCaptor, "testca", 0); + CertProviderClientSslContextProvider provider = + getSslContextProvider( + "gcp_id", + null, + CommonBootstrapperTestUtils.getTestBootstrapInfo(), + /* alpnProtocols= */ null, + /* staticCertValidationContext= */ null); + + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.getSslContext()).isNull(); + + // now generate cert update, updates SslContext + watcherCaptor[0].updateCertificate( + CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE), + ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE))); + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.getSslContext()).isNotNull(); + + TestCallback testCallback = + CommonTlsContextTestsUtil.getValueThruCallback(provider); + + doChecksOnSslContext(false, testCallback.updatedSslContext, /* expectedApnProtos= */ null); + TestCallback testCallback1 = + CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); + + // just do root cert update: trusted roots is not updated (because of system root certs config) + // and sslContext should still be the same + watcherCaptor[0].updateTrustedRoots( + ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE))); + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNull(); + testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); + + // now update id cert: sslContext should be updated i.e.different from the previous one + watcherCaptor[0].updateCertificate( + CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE), + ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.getSslContext()).isNotNull(); + testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); + } + @Test public void testProviderForClient_mtls_newXds() throws Exception { final CertificateProvider.DistributorWatcher[] watcherCaptor = From 968d56414900c6cebcce92996a3e4e45f14f2c9e Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 8 Sep 2025 11:57:07 +0000 Subject: [PATCH 02/23] Save changes. --- .../grpc/xds/XdsSecurityClientServerTest.java | 2 +- .../ClientSslContextProviderFactoryTest.java | 10 +++---- .../security/CommonTlsContextTestsUtil.java | 22 +++++++++----- ...tProviderClientSslContextProviderTest.java | 29 ++++++++++--------- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 23068d665bf..a92b2d868cf 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -563,7 +563,7 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSys CertificateValidationContext.newBuilder() .setSystemRootCerts( CertificateValidationContext.SystemRootCerts.newBuilder().build()) - .build()); + .build(), false); } return CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( "google_cloud_private_spiffe-client", "ROOT", null, diff --git a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java index 397fe01e0f5..e11efc44b49 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java @@ -74,7 +74,7 @@ public void createCertProviderClientSslContextProvider() throws XdsInitializatio "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -105,7 +105,7 @@ public void bothPresent_expectCertProviderClientSslContextProvider() "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); CommonTlsContext.Builder builder = upstreamTlsContext.getCommonTlsContext().toBuilder(); builder = addFilenames(builder, "foo.pem", "foo.key", "root.pem"); @@ -135,7 +135,7 @@ public void createCertProviderClientSslContextProvider_onlyRootCert() "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -169,7 +169,7 @@ public void createCertProviderClientSslContextProvider_withStaticContext() "gcp_id", "root-default", /* alpnProtocols= */ null, - staticCertValidationContext); + staticCertValidationContext, false); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -199,7 +199,7 @@ public void createCertProviderClientSslContextProvider_2providers() "file_provider", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = diff --git a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java index 5ad1d757114..3171a43e05f 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java @@ -165,7 +165,7 @@ public static EnvoyServerProtoData.UpstreamTlsContext buildUpstreamTlsContext( commonInstanceName, "ROOT", null, - null); + null, false); } /** Gets a cert from contents of a resource. */ @@ -182,7 +182,8 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance( String rootInstanceName, String rootCertName, Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext) { + CertificateValidationContext staticCertValidationContext, + boolean useSystemRootCerts) { CommonTlsContext.Builder builder = CommonTlsContext.newBuilder(); if (certInstanceName != null) { builder = @@ -193,7 +194,8 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance( } builder = addCertificateValidationContext( - builder, rootInstanceName, rootCertName, staticCertValidationContext); + builder, rootInstanceName, rootCertName, staticCertValidationContext, + useSystemRootCerts); if (alpnProtocols != null) { builder.addAllAlpnProtocols(alpnProtocols); } @@ -228,7 +230,8 @@ private static CommonTlsContext.Builder addCertificateValidationContext( CommonTlsContext.Builder builder, String rootInstanceName, String rootCertName, - CertificateValidationContext staticCertValidationContext) { + CertificateValidationContext staticCertValidationContext, + boolean useSystemRootCerts) { CertificateValidationContext.Builder contextBuilder; if (staticCertValidationContext == null) { contextBuilder = CertificateValidationContext.newBuilder(); @@ -240,7 +243,7 @@ private static CommonTlsContext.Builder addCertificateValidationContext( .setInstanceName(rootInstanceName) .setCertificateName(rootCertName)); builder.setValidationContext(contextBuilder.build()); - } else { + } else if (useSystemRootCerts) { builder.setValidationContext(contextBuilder.setSystemRootCerts( CertificateValidationContext.SystemRootCerts.getDefaultInstance()) .build()); @@ -277,7 +280,8 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( @Nullable String rootInstanceName, @Nullable String rootCertName, Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext) { + CertificateValidationContext staticCertValidationContext, + boolean useSystemRootCerts) { return buildUpstreamTlsContext( buildCommonTlsContextForCertProviderInstance( certInstanceName, @@ -285,7 +289,8 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( rootInstanceName, rootCertName, alpnProtocols, - staticCertValidationContext)); + staticCertValidationContext, + useSystemRootCerts)); } /** Helper method to build UpstreamTlsContext for CertProvider tests. */ @@ -324,7 +329,8 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( rootInstanceName, rootCertName, alpnProtocols, - staticCertValidationContext), requireClientCert); + staticCertValidationContext, +false), requireClientCert); } /** Helper method to build DownstreamTlsContext for CertProvider tests. */ 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 fe8b1cb9b4c..d3e00f5b59d 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 @@ -68,11 +68,12 @@ public void setUp() throws Exception { /** Helper method to build CertProviderClientSslContextProvider. */ private CertProviderClientSslContextProvider getSslContextProvider( - String certInstanceName, - String rootInstanceName, - Bootstrapper.BootstrapInfo bootstrapInfo, - Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext) { + String certInstanceName, + String rootInstanceName, + Bootstrapper.BootstrapInfo bootstrapInfo, + Iterable alpnProtocols, + CertificateValidationContext staticCertValidationContext, + boolean useSystemRootCerts) { EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext = CommonTlsContextTestsUtil.buildUpstreamTlsContextForCertProviderInstance( certInstanceName, @@ -80,7 +81,8 @@ private CertProviderClientSslContextProvider getSslContextProvider( rootInstanceName, "root-default", alpnProtocols, - staticCertValidationContext); + staticCertValidationContext, + useSystemRootCerts); return (CertProviderClientSslContextProvider) certProviderClientSslContextProviderFactory.getProvider( upstreamTlsContext, @@ -122,7 +124,7 @@ public void testProviderForClient_mtls() throws Exception { "gcp_id", CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); @@ -185,7 +187,8 @@ public void testProviderForClient_systemRootCerts() throws Exception { null, CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, + true); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); @@ -305,7 +308,7 @@ public void testProviderForClient_queueExecutor() throws Exception { "gcp_id", CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); QueuedExecutor queuedExecutor = new QueuedExecutor(); TestCallback testCallback = @@ -338,7 +341,7 @@ public void testProviderForClient_tls() throws Exception { "gcp_id", CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); @@ -375,7 +378,7 @@ public void testProviderForClient_sslContextException_onError() throws Exception "gcp_id", CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */null, - staticCertValidationContext); + staticCertValidationContext, false); TestCallback testCallback = new TestCallback(MoreExecutors.directExecutor()); provider.addCallback(testCallback); @@ -407,7 +410,7 @@ public void testProviderForClient_rootInstanceNull_and_notUsingSystemRootCerts_e /* rootInstanceName= */ null, CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null); + /* staticCertValidationContext= */ null, false); fail("exception expected"); } catch (UnsupportedOperationException expected) { assertThat(expected).hasMessageThat().contains("Unsupported configurations in " @@ -430,7 +433,7 @@ public void testProviderForClient_rootInstanceNull_but_isUsingSystemRootCerts_va CertificateValidationContext.newBuilder() .setSystemRootCerts( CertificateValidationContext.SystemRootCerts.newBuilder().build()) - .build()); + .build(), false); } static class QueuedExecutor implements Executor { From 6c1898a7d90a29a33afcba5ff7b4040d7f02d382 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 8 Sep 2025 12:29:14 +0000 Subject: [PATCH 03/23] Save changes. --- .../grpc/xds/XdsSecurityClientServerTest.java | 2 +- .../ClientSslContextProviderFactoryTest.java | 10 +++--- .../security/CommonTlsContextTestsUtil.java | 25 +++++--------- ...tProviderClientSslContextProviderTest.java | 33 +++++++++++++------ 4 files changed, 37 insertions(+), 33 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index a92b2d868cf..23068d665bf 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -563,7 +563,7 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSys CertificateValidationContext.newBuilder() .setSystemRootCerts( CertificateValidationContext.SystemRootCerts.newBuilder().build()) - .build(), false); + .build()); } return CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( "google_cloud_private_spiffe-client", "ROOT", null, diff --git a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java index e11efc44b49..397fe01e0f5 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java @@ -74,7 +74,7 @@ public void createCertProviderClientSslContextProvider() throws XdsInitializatio "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null, false); + /* staticCertValidationContext= */ null); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -105,7 +105,7 @@ public void bothPresent_expectCertProviderClientSslContextProvider() "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null, false); + /* staticCertValidationContext= */ null); CommonTlsContext.Builder builder = upstreamTlsContext.getCommonTlsContext().toBuilder(); builder = addFilenames(builder, "foo.pem", "foo.key", "root.pem"); @@ -135,7 +135,7 @@ public void createCertProviderClientSslContextProvider_onlyRootCert() "gcp_id", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null, false); + /* staticCertValidationContext= */ null); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -169,7 +169,7 @@ public void createCertProviderClientSslContextProvider_withStaticContext() "gcp_id", "root-default", /* alpnProtocols= */ null, - staticCertValidationContext, false); + staticCertValidationContext); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = @@ -199,7 +199,7 @@ public void createCertProviderClientSslContextProvider_2providers() "file_provider", "root-default", /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null, false); + /* staticCertValidationContext= */ null); Bootstrapper.BootstrapInfo bootstrapInfo = CommonBootstrapperTestUtils.getTestBootstrapInfo(); clientSslContextProviderFactory = diff --git a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java index 3171a43e05f..e39aae56a69 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java @@ -165,7 +165,7 @@ public static EnvoyServerProtoData.UpstreamTlsContext buildUpstreamTlsContext( commonInstanceName, "ROOT", null, - null, false); + null); } /** Gets a cert from contents of a resource. */ @@ -182,8 +182,7 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance( String rootInstanceName, String rootCertName, Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext, - boolean useSystemRootCerts) { + CertificateValidationContext staticCertValidationContext) { CommonTlsContext.Builder builder = CommonTlsContext.newBuilder(); if (certInstanceName != null) { builder = @@ -194,8 +193,7 @@ private static CommonTlsContext buildCommonTlsContextForCertProviderInstance( } builder = addCertificateValidationContext( - builder, rootInstanceName, rootCertName, staticCertValidationContext, - useSystemRootCerts); + builder, rootInstanceName, rootCertName, staticCertValidationContext); if (alpnProtocols != null) { builder.addAllAlpnProtocols(alpnProtocols); } @@ -230,8 +228,7 @@ private static CommonTlsContext.Builder addCertificateValidationContext( CommonTlsContext.Builder builder, String rootInstanceName, String rootCertName, - CertificateValidationContext staticCertValidationContext, - boolean useSystemRootCerts) { + CertificateValidationContext staticCertValidationContext) { CertificateValidationContext.Builder contextBuilder; if (staticCertValidationContext == null) { contextBuilder = CertificateValidationContext.newBuilder(); @@ -243,10 +240,6 @@ private static CommonTlsContext.Builder addCertificateValidationContext( .setInstanceName(rootInstanceName) .setCertificateName(rootCertName)); builder.setValidationContext(contextBuilder.build()); - } else if (useSystemRootCerts) { - builder.setValidationContext(contextBuilder.setSystemRootCerts( - CertificateValidationContext.SystemRootCerts.getDefaultInstance()) - .build()); } return builder.setCombinedValidationContext(CombinedCertificateValidationContext.newBuilder() .setDefaultValidationContext(contextBuilder)); @@ -280,8 +273,7 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( @Nullable String rootInstanceName, @Nullable String rootCertName, Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext, - boolean useSystemRootCerts) { + CertificateValidationContext staticCertValidationContext) { return buildUpstreamTlsContext( buildCommonTlsContextForCertProviderInstance( certInstanceName, @@ -289,8 +281,7 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( rootInstanceName, rootCertName, alpnProtocols, - staticCertValidationContext, - useSystemRootCerts)); + staticCertValidationContext)); } /** Helper method to build UpstreamTlsContext for CertProvider tests. */ @@ -329,8 +320,8 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( rootInstanceName, rootCertName, alpnProtocols, - staticCertValidationContext, -false), requireClientCert); + staticCertValidationContext), + requireClientCert); } /** Helper method to build DownstreamTlsContext for CertProvider tests. */ 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 d3e00f5b59d..ab12d2f32d9 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 @@ -74,15 +74,26 @@ private CertProviderClientSslContextProvider getSslContextProvider( Iterable alpnProtocols, CertificateValidationContext staticCertValidationContext, boolean useSystemRootCerts) { - EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext = - CommonTlsContextTestsUtil.buildUpstreamTlsContextForCertProviderInstance( - certInstanceName, - "cert-default", - rootInstanceName, - "root-default", - alpnProtocols, - staticCertValidationContext, - useSystemRootCerts); + EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext; + if (useSystemRootCerts) { + upstreamTlsContext = + CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( + certInstanceName, + "cert-default", + rootInstanceName, + "root-default", + alpnProtocols, + staticCertValidationContext); + } else { + upstreamTlsContext = + CommonTlsContextTestsUtil.buildUpstreamTlsContextForCertProviderInstance( + certInstanceName, + "cert-default", + rootInstanceName, + "root-default", + alpnProtocols, + staticCertValidationContext); + } return (CertProviderClientSslContextProvider) certProviderClientSslContextProviderFactory.getProvider( upstreamTlsContext, @@ -187,7 +198,9 @@ public void testProviderForClient_systemRootCerts() throws Exception { null, CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, - /* staticCertValidationContext= */ null, + CertificateValidationContext.newBuilder() + .setSystemRootCerts(CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build(), true); assertThat(provider.savedKey).isNull(); From 5be2aa2da33b6c341fb3899f215695ff68ec6b0d Mon Sep 17 00:00:00 2001 From: Kannan J Date: Mon, 8 Sep 2025 12:31:36 +0000 Subject: [PATCH 04/23] Save changes. --- .../internal/security/CommonTlsContextTestsUtil.java | 3 +-- .../CertProviderClientSslContextProviderTest.java | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java index e39aae56a69..718695f3b3f 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java @@ -320,8 +320,7 @@ private static CommonTlsContext.Builder addNewCertificateValidationContext( rootInstanceName, rootCertName, alpnProtocols, - staticCertValidationContext), - requireClientCert); + staticCertValidationContext), requireClientCert); } /** Helper method to build DownstreamTlsContext for CertProvider tests. */ 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 ab12d2f32d9..3d04a1cdf17 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 @@ -68,12 +68,12 @@ public void setUp() throws Exception { /** Helper method to build CertProviderClientSslContextProvider. */ private CertProviderClientSslContextProvider getSslContextProvider( - String certInstanceName, - String rootInstanceName, - Bootstrapper.BootstrapInfo bootstrapInfo, - Iterable alpnProtocols, - CertificateValidationContext staticCertValidationContext, - boolean useSystemRootCerts) { + String certInstanceName, + String rootInstanceName, + Bootstrapper.BootstrapInfo bootstrapInfo, + Iterable alpnProtocols, + CertificateValidationContext staticCertValidationContext, + boolean useSystemRootCerts) { EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext; if (useSystemRootCerts) { upstreamTlsContext = From 90abe55967760f18fe60d285d476eae4c6ea3a0b Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 9 Sep 2025 07:01:16 +0000 Subject: [PATCH 05/23] style --- ...tProviderClientSslContextProviderTest.java | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) 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 3d04a1cdf17..0643295d7e5 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 @@ -193,15 +193,16 @@ public void testProviderForClient_systemRootCerts() throws Exception { TestCertificateProvider.createAndRegisterProviderProvider( certificateProviderRegistry, watcherCaptor, "testca", 0); CertProviderClientSslContextProvider provider = - getSslContextProvider( - "gcp_id", - null, - CommonBootstrapperTestUtils.getTestBootstrapInfo(), - /* alpnProtocols= */ null, - CertificateValidationContext.newBuilder() - .setSystemRootCerts(CertificateValidationContext.SystemRootCerts.getDefaultInstance()) - .build(), - true); + getSslContextProvider( + "gcp_id", + null, + CommonBootstrapperTestUtils.getTestBootstrapInfo(), + /* alpnProtocols= */ null, + CertificateValidationContext.newBuilder() + .setSystemRootCerts( + CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build(), + true); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); From 4c44e4c66806abd40daf11b53c3691f0e889fcac Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 9 Sep 2025 09:07:40 +0000 Subject: [PATCH 06/23] Add comment and rename some confusing method names. --- .../certprovider/CertProviderSslContextProvider.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 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 801dabeecb7..d2bd0d09f93 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 @@ -155,12 +155,12 @@ private void updateSslContextWhenReady() { updateSslContext(); clearKeysAndCerts(); } - } else if (isClientSideTls()) { + } else if (isNormalTlsAndClientSide()) { if (savedTrustedRoots != null || savedSpiffeTrustMap != null) { updateSslContext(); clearKeysAndCerts(); } - } else if (isServerSideTls()) { + } else if (isNormalTlsAndServerSide()) { if (savedKey != null) { updateSslContext(); clearKeysAndCerts(); @@ -179,11 +179,13 @@ protected final boolean isMtls() { return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts); } - protected final boolean isClientSideTls() { + protected final boolean isNormalTlsAndClientSide() { + // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of where this method is called + // from. With the rootCertInstance being null when using system root certs, there is nothing to update. return rootCertInstance != null && certInstance == null; } - protected final boolean isServerSideTls() { + protected final boolean isNormalTlsAndServerSide() { return certInstance != null && rootCertInstance == null; } From 199cc69a359bb8108ea1430e185d1ecc139b3171 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 9 Sep 2025 09:24:20 +0000 Subject: [PATCH 07/23] style. --- .../certprovider/CertProviderSslContextProvider.java | 5 +++-- 1 file changed, 3 insertions(+), 2 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 234d4a06654..b88cfd2b032 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 @@ -183,8 +183,9 @@ protected final boolean isMtls() { } protected final boolean isNormalTlsAndClientSide() { - // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of where this method is called - // from. With the rootCertInstance being null when using system root certs, there is nothing to update. + // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of how this + // method is used. With the rootCertInstance being null when using system root certs, there + // is nothing to update in the SslContext return rootCertInstance != null && certInstance == null; } From 37cd044cfa34b038337e0d4e3b543edd5a7bb7a6 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Wed, 10 Sep 2025 08:27:30 +0000 Subject: [PATCH 08/23] Handle Sslcontext updates for System root certs with and without Mtls. --- .../security/SslContextProviderSupplier.java | 47 ++++++---- .../SslContextProviderSupplierTest.java | 88 ++++++++++++++++--- ...tProviderClientSslContextProviderTest.java | 64 +++++++++++--- 3 files changed, 161 insertions(+), 38 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java b/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java index 5f629273179..b8113bd4752 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java @@ -20,12 +20,14 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import io.grpc.netty.GrpcSslContexts; import io.grpc.xds.EnvoyServerProtoData.BaseTlsContext; import io.grpc.xds.EnvoyServerProtoData.DownstreamTlsContext; import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext; import io.grpc.xds.TlsContextManager; import io.netty.handler.ssl.SslContext; import java.util.Objects; +import javax.net.ssl.SSLException; /** * Enables Client or server side to initialize this object with the received {@link BaseTlsContext} @@ -62,21 +64,36 @@ public synchronized void updateSslContext(final SslContextProvider.Callback call } // we want to increment the ref-count so call findOrCreate again... final SslContextProvider toRelease = getSslContextProvider(); - toRelease.addCallback( - new SslContextProvider.Callback(callback.getExecutor()) { - - @Override - public void updateSslContext(SslContext sslContext) { - callback.updateSslContext(sslContext); - releaseSslContextProvider(toRelease); - } - - @Override - public void onException(Throwable throwable) { - callback.onException(throwable); - releaseSslContextProvider(toRelease); - } - }); + // When using system root certs on client side, SslContext updates via CertificateProvider is + // only required if Mtls is also enabled, i.e. tlsContext has a cert provider instance. + if (tlsContext instanceof UpstreamTlsContext + && !CommonTlsContextUtil.hasCertProviderInstance(tlsContext.getCommonTlsContext()) + && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext())) { + callback.getExecutor().execute(() -> { + try { + callback.updateSslContext(GrpcSslContexts.forClient().build()); + releaseSslContextProvider(toRelease); + } catch (SSLException e) { + callback.onException(e); + } + }); + } else { + toRelease.addCallback( + new SslContextProvider.Callback(callback.getExecutor()) { + + @Override + public void updateSslContext(SslContext sslContext) { + callback.updateSslContext(sslContext); + releaseSslContextProvider(toRelease); + } + + @Override + public void onException(Throwable throwable) { + callback.onException(throwable); + releaseSslContextProvider(toRelease); + } + }); + } } catch (final Throwable throwable) { callback.getExecutor().execute(new Runnable() { @Override diff --git a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java index f476818297d..999b5e2cec5 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java @@ -17,15 +17,18 @@ package io.grpc.xds.internal.security; import static com.google.common.truth.Truth.assertThat; +import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.buildUpstreamTlsContext; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; +import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; import io.grpc.xds.EnvoyServerProtoData; import io.grpc.xds.TlsContextManager; import io.netty.handler.ssl.SslContext; @@ -47,14 +50,17 @@ public class SslContextProviderSupplierTest { @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @Mock private TlsContextManager mockTlsContextManager; + @Mock private Executor mockExecutor; private SslContextProviderSupplier supplier; private SslContextProvider mockSslContextProvider; private EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext; private SslContextProvider.Callback mockCallback; - private void prepareSupplier() { - upstreamTlsContext = - CommonTlsContextTestsUtil.buildUpstreamTlsContext("google_cloud_private_spiffe", true); + private void prepareSupplier(boolean createUpstreamTlsContext) { + if (createUpstreamTlsContext) { + upstreamTlsContext = + buildUpstreamTlsContext("google_cloud_private_spiffe", true); + } mockSslContextProvider = mock(SslContextProvider.class); doReturn(mockSslContextProvider) .when(mockTlsContextManager) @@ -64,14 +70,13 @@ private void prepareSupplier() { private void callUpdateSslContext() { mockCallback = mock(SslContextProvider.Callback.class); - Executor mockExecutor = mock(Executor.class); doReturn(mockExecutor).when(mockCallback).getExecutor(); supplier.updateSslContext(mockCallback); } @Test public void get_updateSecret() { - prepareSupplier(); + prepareSupplier(true); callUpdateSslContext(); verify(mockTlsContextManager, times(2)) .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); @@ -95,11 +100,12 @@ public void get_updateSecret() { @Test public void get_onException() { - prepareSupplier(); + prepareSupplier(true); callUpdateSslContext(); ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass(SslContextProvider.Callback.class); - verify(mockSslContextProvider, times(1)).addCallback(callbackCaptor.capture()); + verify(mockSslContextProvider, times(1)) + .addCallback(callbackCaptor.capture()); SslContextProvider.Callback capturedCallback = callbackCaptor.getValue(); assertThat(capturedCallback).isNotNull(); Exception exception = new Exception("test"); @@ -109,9 +115,71 @@ public void get_onException() { .releaseClientSslContextProvider(eq(mockSslContextProvider)); } + @Test + public void systemRootCertsWithMtls_callbackExecutedFromProvider() { + upstreamTlsContext = + CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( + "gcp_id", + "cert-default", + null, + "root-default", + null, + CertificateValidationContext.newBuilder() + .setSystemRootCerts( + CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build()); + prepareSupplier(false); + + callUpdateSslContext(); + + verify(mockTlsContextManager, times(2)) + .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); + verify(mockTlsContextManager, times(0)) + .releaseClientSslContextProvider(any(SslContextProvider.class)); + ArgumentCaptor callbackCaptor = + ArgumentCaptor.forClass(SslContextProvider.Callback.class); + verify(mockSslContextProvider, times(1)).addCallback(callbackCaptor.capture()); + SslContextProvider.Callback capturedCallback = callbackCaptor.getValue(); + assertThat(capturedCallback).isNotNull(); + SslContext mockSslContext = mock(SslContext.class); + capturedCallback.updateSslContext(mockSslContext); + verify(mockCallback, times(1)).updateSslContext(eq(mockSslContext)); + verify(mockTlsContextManager, times(1)) + .releaseClientSslContextProvider(eq(mockSslContextProvider)); + SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class); + supplier.updateSslContext(mockCallback); + verify(mockTlsContextManager, times(3)) + .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); + } + + @Test + public void systemRootCertsWithRegularTls_callbackExecutedFromSupplier() { + upstreamTlsContext = + CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( + null, + null, + null, + "root-default", + null, + CertificateValidationContext.newBuilder() + .setSystemRootCerts( + CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build()); + supplier = new SslContextProviderSupplier(upstreamTlsContext, mockTlsContextManager); + reset(mockTlsContextManager); + + callUpdateSslContext(); + ArgumentCaptor runnableArgumentCaptor = ArgumentCaptor.forClass(Runnable.class); + verify(mockExecutor).execute(runnableArgumentCaptor.capture()); + runnableArgumentCaptor.getValue().run(); + verify(mockCallback, times(1)).updateSslContext(any(SslContext.class)); + verify(mockTlsContextManager, times(1)) + .releaseClientSslContextProvider(eq(mockSslContextProvider)); + } + @Test public void testClose() { - prepareSupplier(); + prepareSupplier(true); callUpdateSslContext(); supplier.close(); verify(mockTlsContextManager, times(1)) @@ -125,7 +193,7 @@ public void testClose() { @Test public void testClose_nullSslContextProvider() { - prepareSupplier(); + prepareSupplier(true); doThrow(new NullPointerException()).when(mockTlsContextManager) .releaseClientSslContextProvider(null); supplier.close(); 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 0643295d7e5..07d422a97d0 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 @@ -187,14 +187,19 @@ public void testProviderForClient_mtls() throws Exception { } @Test - public void testProviderForClient_systemRootCerts() throws Exception { + /** + * Note this route will not really be invoked since {@link SslContextProviderSupplier} will + * shortcircuit creating the certificate provider and directly invoke the callback with the + * SslContext in this case. + */ + public void testProviderForClient_systemRootCerts_regularTls() throws Exception { final CertificateProvider.DistributorWatcher[] watcherCaptor = new CertificateProvider.DistributorWatcher[1]; TestCertificateProvider.createAndRegisterProviderProvider( certificateProviderRegistry, watcherCaptor, "testca", 0); CertProviderClientSslContextProvider provider = getSslContextProvider( - "gcp_id", + null, null, CommonBootstrapperTestUtils.getTestBootstrapInfo(), /* alpnProtocols= */ null, @@ -209,36 +214,69 @@ public void testProviderForClient_systemRootCerts() throws Exception { assertThat(provider.savedTrustedRoots).isNull(); assertThat(provider.getSslContext()).isNull(); - // now generate cert update, updates SslContext + assertThat(watcherCaptor[0]).isNull(); + } + + @Test + public void testProviderForClient_systemRootCerts_mtls() throws Exception { + final CertificateProvider.DistributorWatcher[] watcherCaptor = + new CertificateProvider.DistributorWatcher[1]; + TestCertificateProvider.createAndRegisterProviderProvider( + certificateProviderRegistry, watcherCaptor, "testca", 0); + CertProviderClientSslContextProvider provider = + getSslContextProvider( + "gcp_id", + null, + CommonBootstrapperTestUtils.getTestBootstrapInfo(), + /* alpnProtocols= */ null, + CertificateValidationContext.newBuilder() + .setSystemRootCerts( + CertificateValidationContext.SystemRootCerts.getDefaultInstance()) + .build(), + true); + + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.getSslContext()).isNull(); + + // now generate root cert update, will get ignored because of systemRootCerts config + watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE))); + assertThat(provider.getSslContext()).isNull(); + assertThat(provider.savedKey).isNull(); + assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNull(); + + // now generate cert update watcherCaptor[0].updateCertificate( - CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE), - ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE))); + CommonCertProviderTestUtils.getPrivateKey(CLIENT_KEY_FILE), + ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); assertThat(provider.getSslContext()).isNotNull(); TestCallback testCallback = - CommonTlsContextTestsUtil.getValueThruCallback(provider); + CommonTlsContextTestsUtil.getValueThruCallback(provider); doChecksOnSslContext(false, testCallback.updatedSslContext, /* expectedApnProtos= */ null); TestCallback testCallback1 = - CommonTlsContextTestsUtil.getValueThruCallback(provider); + CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); - // just do root cert update: trusted roots is not updated (because of system root certs config) - // and sslContext should still be the same + // just do root cert update: sslContext should still be the same, will get ignored because of + // systemRootCerts config watcherCaptor[0].updateTrustedRoots( - ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE))); + ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); assertThat(provider.savedTrustedRoots).isNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); - // now update id cert: sslContext should be updated i.e.different from the previous one + // now update id cert: sslContext should be updated i.e. different from the previous one watcherCaptor[0].updateCertificate( - CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE), - ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); + CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE), + ImmutableList.of(getCertFromResourceName(SERVER_1_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); assertThat(provider.savedTrustedRoots).isNull(); From 139805ec569e2592737d74c436f0c3357b688ea3 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Wed, 10 Sep 2025 09:38:14 +0000 Subject: [PATCH 09/23] Style changes. --- .../CertProviderClientSslContextProviderTest.java | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) 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 07d422a97d0..bfd4bd42211 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 @@ -187,11 +187,9 @@ public void testProviderForClient_mtls() throws Exception { } @Test - /** - * Note this route will not really be invoked since {@link SslContextProviderSupplier} will - * shortcircuit creating the certificate provider and directly invoke the callback with the - * SslContext in this case. - */ + // Note: This code flow will not really be invoked since {@link SslContextProviderSupplier} will + // shortcircuit creating the certificate provider and directly invoke the callback with the + // SslContext in this case. public void testProviderForClient_systemRootCerts_regularTls() throws Exception { final CertificateProvider.DistributorWatcher[] watcherCaptor = new CertificateProvider.DistributorWatcher[1]; From 7f48afa7782339f2646b48d8f3127c0a876ce035 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 06:28:12 +0000 Subject: [PATCH 10/23] Remove special-casing for System root certs in SslContextProviderSupplier and handle it in --- .../security/SslContextProviderSupplier.java | 47 +++------- .../CertProviderClientSslContextProvider.java | 14 ++- .../CertProviderSslContextProvider.java | 8 +- .../SslContextProviderSupplierTest.java | 94 +++---------------- ...tProviderClientSslContextProviderTest.java | 10 +- 5 files changed, 49 insertions(+), 124 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java b/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java index b8113bd4752..5f629273179 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java @@ -20,14 +20,12 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; -import io.grpc.netty.GrpcSslContexts; import io.grpc.xds.EnvoyServerProtoData.BaseTlsContext; import io.grpc.xds.EnvoyServerProtoData.DownstreamTlsContext; import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext; import io.grpc.xds.TlsContextManager; import io.netty.handler.ssl.SslContext; import java.util.Objects; -import javax.net.ssl.SSLException; /** * Enables Client or server side to initialize this object with the received {@link BaseTlsContext} @@ -64,36 +62,21 @@ public synchronized void updateSslContext(final SslContextProvider.Callback call } // we want to increment the ref-count so call findOrCreate again... final SslContextProvider toRelease = getSslContextProvider(); - // When using system root certs on client side, SslContext updates via CertificateProvider is - // only required if Mtls is also enabled, i.e. tlsContext has a cert provider instance. - if (tlsContext instanceof UpstreamTlsContext - && !CommonTlsContextUtil.hasCertProviderInstance(tlsContext.getCommonTlsContext()) - && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext())) { - callback.getExecutor().execute(() -> { - try { - callback.updateSslContext(GrpcSslContexts.forClient().build()); - releaseSslContextProvider(toRelease); - } catch (SSLException e) { - callback.onException(e); - } - }); - } else { - toRelease.addCallback( - new SslContextProvider.Callback(callback.getExecutor()) { - - @Override - public void updateSslContext(SslContext sslContext) { - callback.updateSslContext(sslContext); - releaseSslContextProvider(toRelease); - } - - @Override - public void onException(Throwable throwable) { - callback.onException(throwable); - releaseSslContextProvider(toRelease); - } - }); - } + toRelease.addCallback( + new SslContextProvider.Callback(callback.getExecutor()) { + + @Override + public void updateSslContext(SslContext sslContext) { + callback.updateSslContext(sslContext); + releaseSslContextProvider(toRelease); + } + + @Override + public void onException(Throwable throwable) { + callback.onException(throwable); + releaseSslContextProvider(toRelease); + } + }); } catch (final Throwable throwable) { callback.getExecutor().execute(new Runnable() { @Override diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java index d7c2267c48f..79374ede827 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java @@ -28,6 +28,7 @@ import java.security.cert.X509Certificate; import java.util.Map; import javax.annotation.Nullable; +import javax.net.ssl.SSLException; /** A client SslContext provider using CertificateProviderInstance to fetch secrets. */ final class CertProviderClientSslContextProvider extends CertProviderSslContextProvider { @@ -48,6 +49,17 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP staticCertValidationContext, upstreamTlsContext, certificateProviderStore); + // Null rootCertInstance implies hasSystemRootCerts because of the check in + // CertProviderClientSslContextProviderFactory. + if (rootCertInstance == null && !isMtls()) { + try { + // Instantiate sslContext so that addCallback will immediately update the callback with + // the SslContext. + sslContext = getSslContextBuilder(staticCertificateValidationContext).build(); + } catch (SSLException | CertStoreException e) { + throw new RuntimeException(e); + } + } } @Override @@ -55,8 +67,6 @@ protected final SslContextBuilder getSslContextBuilder( CertificateValidationContext certificateValidationContextdationContext) throws CertStoreException { SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient(); - // Null rootCertInstance implies hasSystemRootCerts because of the check in - // CertProviderClientSslContextProviderFactory. if (rootCertInstance != null) { if (savedSpiffeTrustMap != null) { sslContextBuilder = sslContextBuilder.trustManager( 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 b88cfd2b032..dea60abc35f 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 @@ -158,12 +158,12 @@ private void updateSslContextWhenReady() { updateSslContext(); clearKeysAndCerts(); } - } else if (isNormalTlsAndClientSide()) { + } else if (isRegularTlsAndClientSide()) { if (savedTrustedRoots != null || savedSpiffeTrustMap != null) { updateSslContext(); clearKeysAndCerts(); } - } else if (isNormalTlsAndServerSide()) { + } else if (isRegularTlsAndServerSide()) { if (savedKey != null) { updateSslContext(); clearKeysAndCerts(); @@ -182,14 +182,14 @@ protected final boolean isMtls() { return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts); } - protected final boolean isNormalTlsAndClientSide() { + protected final boolean isRegularTlsAndClientSide() { // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of how this // method is used. With the rootCertInstance being null when using system root certs, there // is nothing to update in the SslContext return rootCertInstance != null && certInstance == null; } - protected final boolean isNormalTlsAndServerSide() { + protected final boolean isRegularTlsAndServerSide() { return certInstance != null && rootCertInstance == null; } diff --git a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java index 999b5e2cec5..f5b462b250d 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java @@ -17,18 +17,15 @@ package io.grpc.xds.internal.security; import static com.google.common.truth.Truth.assertThat; -import static io.grpc.xds.internal.security.CommonTlsContextTestsUtil.buildUpstreamTlsContext; -import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.any; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; -import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; -import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; import io.grpc.xds.EnvoyServerProtoData; import io.grpc.xds.TlsContextManager; import io.netty.handler.ssl.SslContext; @@ -50,33 +47,31 @@ public class SslContextProviderSupplierTest { @Rule public final MockitoRule mocks = MockitoJUnit.rule(); @Mock private TlsContextManager mockTlsContextManager; - @Mock private Executor mockExecutor; private SslContextProviderSupplier supplier; private SslContextProvider mockSslContextProvider; private EnvoyServerProtoData.UpstreamTlsContext upstreamTlsContext; private SslContextProvider.Callback mockCallback; - private void prepareSupplier(boolean createUpstreamTlsContext) { - if (createUpstreamTlsContext) { - upstreamTlsContext = - buildUpstreamTlsContext("google_cloud_private_spiffe", true); - } + private void prepareSupplier() { + upstreamTlsContext = + CommonTlsContextTestsUtil.buildUpstreamTlsContext("google_cloud_private_spiffe", true); mockSslContextProvider = mock(SslContextProvider.class); doReturn(mockSslContextProvider) - .when(mockTlsContextManager) - .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); + .when(mockTlsContextManager) + .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); supplier = new SslContextProviderSupplier(upstreamTlsContext, mockTlsContextManager); } private void callUpdateSslContext() { mockCallback = mock(SslContextProvider.Callback.class); + Executor mockExecutor = mock(Executor.class); doReturn(mockExecutor).when(mockCallback).getExecutor(); supplier.updateSslContext(mockCallback); } @Test public void get_updateSecret() { - prepareSupplier(true); + prepareSupplier(); callUpdateSslContext(); verify(mockTlsContextManager, times(2)) .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); @@ -100,12 +95,11 @@ public void get_updateSecret() { @Test public void get_onException() { - prepareSupplier(true); + prepareSupplier(); callUpdateSslContext(); ArgumentCaptor callbackCaptor = ArgumentCaptor.forClass(SslContextProvider.Callback.class); - verify(mockSslContextProvider, times(1)) - .addCallback(callbackCaptor.capture()); + verify(mockSslContextProvider, times(1)).addCallback(callbackCaptor.capture()); SslContextProvider.Callback capturedCallback = callbackCaptor.getValue(); assertThat(capturedCallback).isNotNull(); Exception exception = new Exception("test"); @@ -115,71 +109,9 @@ public void get_onException() { .releaseClientSslContextProvider(eq(mockSslContextProvider)); } - @Test - public void systemRootCertsWithMtls_callbackExecutedFromProvider() { - upstreamTlsContext = - CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( - "gcp_id", - "cert-default", - null, - "root-default", - null, - CertificateValidationContext.newBuilder() - .setSystemRootCerts( - CertificateValidationContext.SystemRootCerts.getDefaultInstance()) - .build()); - prepareSupplier(false); - - callUpdateSslContext(); - - verify(mockTlsContextManager, times(2)) - .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); - verify(mockTlsContextManager, times(0)) - .releaseClientSslContextProvider(any(SslContextProvider.class)); - ArgumentCaptor callbackCaptor = - ArgumentCaptor.forClass(SslContextProvider.Callback.class); - verify(mockSslContextProvider, times(1)).addCallback(callbackCaptor.capture()); - SslContextProvider.Callback capturedCallback = callbackCaptor.getValue(); - assertThat(capturedCallback).isNotNull(); - SslContext mockSslContext = mock(SslContext.class); - capturedCallback.updateSslContext(mockSslContext); - verify(mockCallback, times(1)).updateSslContext(eq(mockSslContext)); - verify(mockTlsContextManager, times(1)) - .releaseClientSslContextProvider(eq(mockSslContextProvider)); - SslContextProvider.Callback mockCallback = mock(SslContextProvider.Callback.class); - supplier.updateSslContext(mockCallback); - verify(mockTlsContextManager, times(3)) - .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); - } - - @Test - public void systemRootCertsWithRegularTls_callbackExecutedFromSupplier() { - upstreamTlsContext = - CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( - null, - null, - null, - "root-default", - null, - CertificateValidationContext.newBuilder() - .setSystemRootCerts( - CertificateValidationContext.SystemRootCerts.getDefaultInstance()) - .build()); - supplier = new SslContextProviderSupplier(upstreamTlsContext, mockTlsContextManager); - reset(mockTlsContextManager); - - callUpdateSslContext(); - ArgumentCaptor runnableArgumentCaptor = ArgumentCaptor.forClass(Runnable.class); - verify(mockExecutor).execute(runnableArgumentCaptor.capture()); - runnableArgumentCaptor.getValue().run(); - verify(mockCallback, times(1)).updateSslContext(any(SslContext.class)); - verify(mockTlsContextManager, times(1)) - .releaseClientSslContextProvider(eq(mockSslContextProvider)); - } - @Test public void testClose() { - prepareSupplier(true); + prepareSupplier(); callUpdateSslContext(); supplier.close(); verify(mockTlsContextManager, times(1)) @@ -193,7 +125,7 @@ public void testClose() { @Test public void testClose_nullSslContextProvider() { - prepareSupplier(true); + prepareSupplier(); doThrow(new NullPointerException()).when(mockTlsContextManager) .releaseClientSslContextProvider(null); supplier.close(); @@ -203,4 +135,4 @@ public void testClose_nullSslContextProvider() { verify(mockTlsContextManager, times(1)) .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); } -} +} \ No newline at end of file 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 bfd4bd42211..3b2ca05231e 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 @@ -187,10 +187,7 @@ public void testProviderForClient_mtls() throws Exception { } @Test - // Note: This code flow will not really be invoked since {@link SslContextProviderSupplier} will - // shortcircuit creating the certificate provider and directly invoke the callback with the - // SslContext in this case. - public void testProviderForClient_systemRootCerts_regularTls() throws Exception { + public void testProviderForClient_systemRootCerts_regularTls() { final CertificateProvider.DistributorWatcher[] watcherCaptor = new CertificateProvider.DistributorWatcher[1]; TestCertificateProvider.createAndRegisterProviderProvider( @@ -210,7 +207,10 @@ public void testProviderForClient_systemRootCerts_regularTls() throws Exception assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); assertThat(provider.savedTrustedRoots).isNull(); - assertThat(provider.getSslContext()).isNull(); + assertThat(provider.getSslContext()).isNotNull(); + TestCallback testCallback = + CommonTlsContextTestsUtil.getValueThruCallback(provider); + assertThat(testCallback.updatedSslContext).isEqualTo(provider.getSslContext()); assertThat(watcherCaptor[0]).isNull(); } From d2b722a9038fabe57e8b9bc4d6a9455f3dc7a67a Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 06:34:31 +0000 Subject: [PATCH 11/23] Formatting changes. --- .../xds/internal/security/CommonTlsContextTestsUtil.java | 3 +++ .../internal/security/SslContextProviderSupplierTest.java | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java index 718695f3b3f..48814dece1d 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java @@ -229,6 +229,9 @@ private static CommonTlsContext.Builder addCertificateValidationContext( String rootInstanceName, String rootCertName, CertificateValidationContext staticCertValidationContext) { + if (staticCertValidationContext == null && rootInstanceName == null) { + return builder; + } CertificateValidationContext.Builder contextBuilder; if (staticCertValidationContext == null) { contextBuilder = CertificateValidationContext.newBuilder(); diff --git a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java index f5b462b250d..f476818297d 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java @@ -54,11 +54,11 @@ public class SslContextProviderSupplierTest { private void prepareSupplier() { upstreamTlsContext = - CommonTlsContextTestsUtil.buildUpstreamTlsContext("google_cloud_private_spiffe", true); + CommonTlsContextTestsUtil.buildUpstreamTlsContext("google_cloud_private_spiffe", true); mockSslContextProvider = mock(SslContextProvider.class); doReturn(mockSslContextProvider) - .when(mockTlsContextManager) - .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); + .when(mockTlsContextManager) + .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); supplier = new SslContextProviderSupplier(upstreamTlsContext, mockTlsContextManager); } @@ -135,4 +135,4 @@ public void testClose_nullSslContextProvider() { verify(mockTlsContextManager, times(1)) .findOrCreateClientSslContextProvider(eq(upstreamTlsContext)); } -} \ No newline at end of file +} From e95725d9334f23c98882fee954048ebe8427b9d7 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 12:00:50 +0000 Subject: [PATCH 12/23] Trust manager handling for system root certs. --- .../CertProviderClientSslContextProvider.java | 40 +++++++++++++-- .../grpc/xds/XdsSecurityClientServerTest.java | 51 +++++++++++++++++-- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java index 79374ede827..7e6de5f1dd0 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java @@ -24,11 +24,22 @@ import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo; import io.grpc.xds.internal.security.trust.XdsTrustManagerFactory; import io.netty.handler.ssl.SslContextBuilder; + +import java.io.IOException; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; import java.security.cert.CertStoreException; +import java.security.cert.Certificate; +import java.security.cert.CertificateException; import java.security.cert.X509Certificate; -import java.util.Map; +import java.util.*; +import java.util.stream.Collectors; import javax.annotation.Nullable; import javax.net.ssl.SSLException; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; +import javax.net.ssl.X509TrustManager; /** A client SslContext provider using CertificateProviderInstance to fetch secrets. */ final class CertProviderClientSslContextProvider extends CertProviderSslContextProvider { @@ -56,7 +67,7 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP // Instantiate sslContext so that addCallback will immediately update the callback with // the SslContext. sslContext = getSslContextBuilder(staticCertificateValidationContext).build(); - } catch (SSLException | CertStoreException e) { + } catch (CertStoreException | CertificateException | IOException e) { throw new RuntimeException(e); } } @@ -65,7 +76,7 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP @Override protected final SslContextBuilder getSslContextBuilder( CertificateValidationContext certificateValidationContextdationContext) - throws CertStoreException { + throws CertificateException, IOException, CertStoreException { SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient(); if (rootCertInstance != null) { if (savedSpiffeTrustMap != null) { @@ -79,10 +90,33 @@ protected final SslContextBuilder getSslContextBuilder( savedTrustedRoots.toArray(new X509Certificate[0]), certificateValidationContextdationContext)); } + } else { + try { + sslContextBuilder = sslContextBuilder.trustManager( + new XdsTrustManagerFactory( + getX509CertificatesFromSystemTrustStore(), + certificateValidationContextdationContext)); + } catch (KeyStoreException | NoSuchAlgorithmException e) { + throw new CertStoreException(e); + } } if (isMtls()) { sslContextBuilder.keyManager(savedKey, savedCertChain); } return sslContextBuilder; } + + private X509Certificate[] getX509CertificatesFromSystemTrustStore() throws KeyStoreException, CertificateException, IOException, NoSuchAlgorithmException { + TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + trustManagerFactory.init((KeyStore) null); + + List trustManagers = Arrays.asList(trustManagerFactory.getTrustManagers()); + List rootCerts = trustManagers.stream() + .filter(X509TrustManager.class::isInstance) + .map(X509TrustManager.class::cast) + .map(trustManager -> Arrays.asList(trustManager.getAcceptedIssuers())) + .flatMap(Collection::stream) + .collect(Collectors.toList()); + return rootCerts.toArray(new X509Certificate[rootCerts.size()]); + } } diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 23068d665bf..08557a508e2 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -37,6 +37,7 @@ import com.google.common.util.concurrent.SettableFuture; import io.envoyproxy.envoy.config.core.v3.SocketAddress.Protocol; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; +import io.envoyproxy.envoy.type.matcher.v3.StringMatcher; import io.grpc.Attributes; import io.grpc.EquivalentAddressGroup; import io.grpc.Grpc; @@ -117,6 +118,10 @@ @RunWith(Parameterized.class) public class XdsSecurityClientServerTest { + // TODO: Change this is a specific domain after + // https://github.com/grpc/grpc-java/issues/12326 is fixed + private static final String SAN_TO_MATCH = "*.test.google.fr"; + @Parameter public Boolean enableSpiffe; private Boolean originalEnableSpiffe; @@ -217,7 +222,7 @@ public void tlsClientServer_useSystemRootCerts_useCombinedValidationContext() th UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, true); + CLIENT_PEM_FILE, true, SAN_TO_MATCH); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -244,7 +249,7 @@ public void tlsClientServer_useSystemRootCerts_validationContext() throws Except UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, false); + CLIENT_PEM_FILE, false, SAN_TO_MATCH); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -255,6 +260,39 @@ public void tlsClientServer_useSystemRootCerts_validationContext() throws Except } } + /** + * Use system root ca cert for TLS channel - no mTLS. + * Subj Alt Names to match are specified in the validaton context. + */ + @Test + public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() throws Exception { + Path trustStoreFilePath = getCacertFilePathForTestCa(); + try { + setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString()); + DownstreamTlsContext downstreamTlsContext = + setBootstrapInfoAndBuildDownstreamTlsContext(SERVER_1_PEM_FILE, null, null, null, null, + null, false, false); + buildServerWithTlsContext(downstreamTlsContext); + + UpstreamTlsContext upstreamTlsContext = + setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, + CLIENT_PEM_FILE, true, "server1.test.google.in"); + + SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = + getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); + unaryRpc(/* requestMessage= */ "buddy", blockingStub); + fail("Expected handshake failure exception"); + } catch (StatusRuntimeException e) { + assertThat(e.getCause()).isInstanceOf(SSLHandshakeException.class); + assertThat(e.getCause().getCause()).isInstanceOf(CertificateException.class); + assertThat(e.getCause().getCause().getMessage()).isEqualTo( + "Peer certificate SAN check failed"); + } finally { + Files.deleteIfExists(trustStoreFilePath); + clearTrustStoreSystemProperties(); + } + } + /** * Use system root ca cert for TLS channel - mTLS. * Uses common_tls_context.combined_validation_context in upstream_tls_context. @@ -266,12 +304,12 @@ public void tlsClientServer_useSystemRootCerts_requireClientAuth() throws Except setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString()); DownstreamTlsContext downstreamTlsContext = setBootstrapInfoAndBuildDownstreamTlsContext(SERVER_1_PEM_FILE, null, null, null, null, - null, false, false); + null, false, true); buildServerWithTlsContext(downstreamTlsContext); UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, true); + CLIENT_PEM_FILE, true, SAN_TO_MATCH); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -552,7 +590,7 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContext(String cli private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts( String clientKeyFile, String clientPemFile, - boolean useCombinedValidationContext) { + boolean useCombinedValidationContext, String sanToMatch) { bootstrapInfoForClient = CommonBootstrapperTestUtils .buildBootstrapInfo("google_cloud_private_spiffe-client", clientKeyFile, clientPemFile, CA_PEM_FILE, null, null, null, null, null); @@ -563,6 +601,9 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSys CertificateValidationContext.newBuilder() .setSystemRootCerts( CertificateValidationContext.SystemRootCerts.newBuilder().build()) + .addMatchSubjectAltNames( + StringMatcher.newBuilder() + .setExact(sanToMatch)) .build()); } return CommonTlsContextTestsUtil.buildNewUpstreamTlsContextForCertProviderInstance( From 180f373f84299005285131dddab557f469507010 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 12:12:45 +0000 Subject: [PATCH 13/23] Fix style --- .../CertProviderClientSslContextProvider.java | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java index 7e6de5f1dd0..b080417bd53 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java @@ -24,19 +24,19 @@ import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo; import io.grpc.xds.internal.security.trust.XdsTrustManagerFactory; import io.netty.handler.ssl.SslContextBuilder; - import java.io.IOException; import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.security.cert.CertStoreException; -import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; -import java.util.*; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Map; import java.util.stream.Collectors; import javax.annotation.Nullable; -import javax.net.ssl.SSLException; import javax.net.ssl.TrustManager; import javax.net.ssl.TrustManagerFactory; import javax.net.ssl.X509TrustManager; @@ -106,8 +106,10 @@ protected final SslContextBuilder getSslContextBuilder( return sslContextBuilder; } - private X509Certificate[] getX509CertificatesFromSystemTrustStore() throws KeyStoreException, CertificateException, IOException, NoSuchAlgorithmException { - TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); + private X509Certificate[] getX509CertificatesFromSystemTrustStore() + throws KeyStoreException, NoSuchAlgorithmException { + TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( + TrustManagerFactory.getDefaultAlgorithm()); trustManagerFactory.init((KeyStore) null); List trustManagers = Arrays.asList(trustManagerFactory.getTrustManagers()); From 381beb2c53de5702d69241a5396748cd4c32da6e Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 13:09:56 +0000 Subject: [PATCH 14/23] Fixes. --- .../CertProviderClientSslContextProvider.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java index b080417bd53..1acdcad65e9 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java @@ -22,6 +22,7 @@ import io.grpc.netty.GrpcSslContexts; import io.grpc.xds.EnvoyServerProtoData.UpstreamTlsContext; import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo; +import io.grpc.xds.internal.security.CommonTlsContextUtil; import io.grpc.xds.internal.security.trust.XdsTrustManagerFactory; import io.netty.handler.ssl.SslContextBuilder; import java.io.IOException; @@ -60,9 +61,9 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP staticCertValidationContext, upstreamTlsContext, certificateProviderStore); - // Null rootCertInstance implies hasSystemRootCerts because of the check in - // CertProviderClientSslContextProviderFactory. - if (rootCertInstance == null && !isMtls()) { + if (rootCertInstance == null + && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext()) + && !isMtls()) { try { // Instantiate sslContext so that addCallback will immediately update the callback with // the SslContext. @@ -75,7 +76,7 @@ final class CertProviderClientSslContextProvider extends CertProviderSslContextP @Override protected final SslContextBuilder getSslContextBuilder( - CertificateValidationContext certificateValidationContextdationContext) + CertificateValidationContext certificateValidationContext) throws CertificateException, IOException, CertStoreException { SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient(); if (rootCertInstance != null) { @@ -83,19 +84,19 @@ protected final SslContextBuilder getSslContextBuilder( sslContextBuilder = sslContextBuilder.trustManager( new XdsTrustManagerFactory( savedSpiffeTrustMap, - certificateValidationContextdationContext)); + certificateValidationContext)); } else { sslContextBuilder = sslContextBuilder.trustManager( new XdsTrustManagerFactory( savedTrustedRoots.toArray(new X509Certificate[0]), - certificateValidationContextdationContext)); + certificateValidationContext)); } } else { try { sslContextBuilder = sslContextBuilder.trustManager( new XdsTrustManagerFactory( getX509CertificatesFromSystemTrustStore(), - certificateValidationContextdationContext)); + certificateValidationContext)); } catch (KeyStoreException | NoSuchAlgorithmException e) { throw new CertStoreException(e); } From 18f5d5ab0dd7a9abe8f51415818faec8446b098c Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 11 Sep 2025 13:50:52 +0000 Subject: [PATCH 15/23] Fix unit tests to cover both mtls and non-mtls for system root certs. --- .../grpc/xds/XdsSecurityClientServerTest.java | 41 +++++++++++++++---- 1 file changed, 33 insertions(+), 8 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 08557a508e2..6b6c57ba61b 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -211,7 +211,8 @@ public void tlsClientServer_noClientAuthentication() throws Exception { * Uses common_tls_context.combined_validation_context in upstream_tls_context. */ @Test - public void tlsClientServer_useSystemRootCerts_useCombinedValidationContext() throws Exception { + public void tlsClientServer_useSystemRootCerts_noMtls_useCombinedValidationContext() + throws Exception { Path trustStoreFilePath = getCacertFilePathForTestCa(); try { setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString()); @@ -222,7 +223,7 @@ public void tlsClientServer_useSystemRootCerts_useCombinedValidationContext() th UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, true, SAN_TO_MATCH); + CLIENT_PEM_FILE, true, SAN_TO_MATCH, false); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -238,7 +239,7 @@ public void tlsClientServer_useSystemRootCerts_useCombinedValidationContext() th * Uses common_tls_context.validation_context in upstream_tls_context. */ @Test - public void tlsClientServer_useSystemRootCerts_validationContext() throws Exception { + public void tlsClientServer_useSystemRootCerts_noMtls_validationContext() throws Exception { Path trustStoreFilePath = getCacertFilePathForTestCa().toAbsolutePath(); try { setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString()); @@ -249,7 +250,7 @@ public void tlsClientServer_useSystemRootCerts_validationContext() throws Except UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, false, SAN_TO_MATCH); + CLIENT_PEM_FILE, false, SAN_TO_MATCH, false); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -260,6 +261,29 @@ public void tlsClientServer_useSystemRootCerts_validationContext() throws Except } } + @Test + public void tlsClientServer_useSystemRootCerts_mtls() throws Exception { + Path trustStoreFilePath = getCacertFilePathForTestCa(); + try { + setTrustStoreSystemProperties(trustStoreFilePath.toAbsolutePath().toString()); + DownstreamTlsContext downstreamTlsContext = + setBootstrapInfoAndBuildDownstreamTlsContext(SERVER_1_PEM_FILE, null, null, null, null, + null, false, true); + buildServerWithTlsContext(downstreamTlsContext); + + UpstreamTlsContext upstreamTlsContext = + setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, + CLIENT_PEM_FILE, true, SAN_TO_MATCH, true); + + SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = + getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); + assertThat(unaryRpc(/* requestMessage= */ "buddy", blockingStub)).isEqualTo("Hello buddy"); + } finally { + Files.deleteIfExists(trustStoreFilePath); + clearTrustStoreSystemProperties(); + } + } + /** * Use system root ca cert for TLS channel - no mTLS. * Subj Alt Names to match are specified in the validaton context. @@ -276,7 +300,7 @@ public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() thro UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, true, "server1.test.google.in"); + CLIENT_PEM_FILE, true, "server1.test.google.in", false); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -309,7 +333,7 @@ public void tlsClientServer_useSystemRootCerts_requireClientAuth() throws Except UpstreamTlsContext upstreamTlsContext = setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts(CLIENT_KEY_FILE, - CLIENT_PEM_FILE, true, SAN_TO_MATCH); + CLIENT_PEM_FILE, true, SAN_TO_MATCH, false); SimpleServiceGrpc.SimpleServiceBlockingStub blockingStub = getBlockingStub(upstreamTlsContext, /* overrideAuthority= */ OVERRIDE_AUTHORITY); @@ -590,13 +614,14 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContext(String cli private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts( String clientKeyFile, String clientPemFile, - boolean useCombinedValidationContext, String sanToMatch) { + boolean useCombinedValidationContext, String sanToMatch, boolean isMtls) { bootstrapInfoForClient = CommonBootstrapperTestUtils .buildBootstrapInfo("google_cloud_private_spiffe-client", clientKeyFile, clientPemFile, CA_PEM_FILE, null, null, null, null, null); if (useCombinedValidationContext) { return CommonTlsContextTestsUtil.buildUpstreamTlsContextForCertProviderInstance( - "google_cloud_private_spiffe-client", "ROOT", null, + isMtls ? "google_cloud_private_spiffe-client" : null, + isMtls ? "ROOT" : null, null, null, null, CertificateValidationContext.newBuilder() .setSystemRootCerts( From e18d6cdddc9cfca6f5280a38b05a324a9714f105 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Fri, 12 Sep 2025 12:07:56 +0000 Subject: [PATCH 16/23] Suppress warning. --- xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 6b6c57ba61b..9d2f552db1c 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -611,6 +611,7 @@ private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContext(String cli .buildUpstreamTlsContext("google_cloud_private_spiffe-client", hasIdentityCert); } + @SuppressWarnings("deprecation") // gRFC A29 predates match_typed_subject_alt_names private UpstreamTlsContext setBootstrapInfoAndBuildUpstreamTlsContextForUsingSystemRootCerts( String clientKeyFile, String clientPemFile, From 3845e16e9d31d2d87e3eaab53de2760ee7afdd26 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Fri, 12 Sep 2025 13:13:24 +0000 Subject: [PATCH 17/23] Use non wildcard SAN in the SAN matchers in validation context. --- .../test/java/io/grpc/xds/XdsSecurityClientServerTest.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 9d2f552db1c..03f1651f2c7 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -118,9 +118,7 @@ @RunWith(Parameterized.class) public class XdsSecurityClientServerTest { - // TODO: Change this is a specific domain after - // https://github.com/grpc/grpc-java/issues/12326 is fixed - private static final String SAN_TO_MATCH = "*.test.google.fr"; + private static final String SAN_TO_MATCH = "waterzooi.test.google.be"; @Parameter public Boolean enableSpiffe; From f13594373ec4e3107fcc44ac2a256d79472a40fb Mon Sep 17 00:00:00 2001 From: Eric Anderson Date: Mon, 22 Sep 2025 15:26:48 -0700 Subject: [PATCH 18/23] xds: Plumb system root certs similarly to CertProviders --- .../CertProviderClientSslContextProvider.java | 23 ++-- .../CertProviderSslContextProvider.java | 102 +++++++++++++----- .../SystemRootCertificateProvider.java | 71 ++++++++++++ 3 files changed, 159 insertions(+), 37 deletions(-) create mode 100644 xds/src/main/java/io/grpc/xds/internal/security/certprovider/SystemRootCertificateProvider.java diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java index d7c2267c48f..131ae6b6125 100644 --- a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java @@ -55,20 +55,19 @@ protected final SslContextBuilder getSslContextBuilder( CertificateValidationContext certificateValidationContextdationContext) throws CertStoreException { SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient(); - // Null rootCertInstance implies hasSystemRootCerts because of the check in - // CertProviderClientSslContextProviderFactory. - if (rootCertInstance != null) { - if (savedSpiffeTrustMap != null) { - sslContextBuilder = sslContextBuilder.trustManager( + if (savedSpiffeTrustMap != null) { + sslContextBuilder = sslContextBuilder.trustManager( + new XdsTrustManagerFactory( + savedSpiffeTrustMap, + certificateValidationContextdationContext)); + } else if (savedTrustedRoots != null) { + sslContextBuilder = sslContextBuilder.trustManager( new XdsTrustManagerFactory( - savedSpiffeTrustMap, + savedTrustedRoots.toArray(new X509Certificate[0]), certificateValidationContextdationContext)); - } else { - sslContextBuilder = sslContextBuilder.trustManager( - new XdsTrustManagerFactory( - savedTrustedRoots.toArray(new X509Certificate[0]), - certificateValidationContextdationContext)); - } + } else { + // Should be impossible because of the check in CertProviderClientSslContextProviderFactory + throw new IllegalStateException("There must be trusted roots or a SPIFFE trust map"); } if (isMtls()) { sslContextBuilder.keyManager(savedKey, savedCertChain); 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 801dabeecb7..ef9a3cf062c 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 @@ -16,14 +16,18 @@ package io.grpc.xds.internal.security.certprovider; +import static java.util.Objects.requireNonNull; + import io.envoyproxy.envoy.config.core.v3.Node; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProviderInstance; +import io.grpc.Status; import io.grpc.xds.EnvoyServerProtoData.BaseTlsContext; import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo; import io.grpc.xds.internal.security.CommonTlsContextUtil; import io.grpc.xds.internal.security.DynamicSslContextProvider; +import java.io.Closeable; import java.security.PrivateKey; import java.security.cert.X509Certificate; import java.util.List; @@ -34,15 +38,12 @@ abstract class CertProviderSslContextProvider extends DynamicSslContextProvider implements CertificateProvider.Watcher { - @Nullable private final CertificateProviderStore.Handle certHandle; - @Nullable private final CertificateProviderStore.Handle rootCertHandle; - @Nullable private final CertificateProviderInstance certInstance; - @Nullable protected final CertificateProviderInstance rootCertInstance; + @Nullable private final NoExceptionCloseable certHandle; + @Nullable private final NoExceptionCloseable rootCertHandle; @Nullable protected PrivateKey savedKey; @Nullable protected List savedCertChain; @Nullable protected List savedTrustedRoots; @Nullable protected Map> savedSpiffeTrustMap; - private final boolean isUsingSystemRootCerts; protected CertProviderSslContextProvider( Node node, @@ -53,26 +54,33 @@ protected CertProviderSslContextProvider( BaseTlsContext tlsContext, CertificateProviderStore certificateProviderStore) { super(tlsContext, staticCertValidationContext); - this.certInstance = certInstance; - this.rootCertInstance = rootCertInstance; - String certInstanceName = null; - if (certInstance != null && certInstance.isInitialized()) { - certInstanceName = certInstance.getInstanceName(); + boolean createCertInstance = certInstance != null && certInstance.isInitialized(); + boolean createRootCertInstance = rootCertInstance != null && rootCertInstance.isInitialized(); + boolean sharedCertInstance = createCertInstance && createRootCertInstance + && rootCertInstance.getInstanceName().equals(certInstance.getInstanceName()); + if (createCertInstance) { CertificateProviderInfo certProviderInstanceConfig = - getCertProviderConfig(certProviders, certInstanceName); + getCertProviderConfig(certProviders, certInstance.getInstanceName()); + CertificateProvider.Watcher watcher = this; + if (!sharedCertInstance) { + watcher = new IgnoreUpdatesWatcher(watcher, /* ignoreRootCertUpdates= */ true); + } + // TODO: Previously we'd hang if certProviderInstanceConfig were null or + // certInstance.isInitialized() == false. Now we'll proceed. Those should be errors, or are + // they impossible and should be assertions? certHandle = certProviderInstanceConfig == null ? null : certificateProviderStore.createOrGetProvider( certInstance.getCertificateName(), certProviderInstanceConfig.pluginName(), certProviderInstanceConfig.config(), - this, - true); + watcher, + true)::close; } else { certHandle = null; } - if (rootCertInstance != null - && rootCertInstance.isInitialized() - && !rootCertInstance.getInstanceName().equals(certInstanceName)) { + if (createRootCertInstance && sharedCertInstance) { + rootCertHandle = () -> { }; + } else if (createRootCertInstance && !sharedCertInstance) { CertificateProviderInfo certProviderInstanceConfig = getCertProviderConfig(certProviders, rootCertInstance.getInstanceName()); rootCertHandle = certProviderInstanceConfig == null ? null @@ -80,13 +88,16 @@ protected CertProviderSslContextProvider( rootCertInstance.getCertificateName(), certProviderInstanceConfig.pluginName(), certProviderInstanceConfig.config(), - this, - true); + new IgnoreUpdatesWatcher(this, /* ignoreRootCertUpdates= */ false), + false)::close; + } else if (rootCertInstance == null + && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext())) { + SystemRootCertificateProvider systemRootProvider = new SystemRootCertificateProvider(this); + systemRootProvider.start(); + rootCertHandle = systemRootProvider::close; } else { rootCertHandle = null; } - this.isUsingSystemRootCerts = rootCertInstance == null - && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext()); } private static CertificateProviderInfo getCertProviderConfig( @@ -150,8 +161,7 @@ public final void updateSpiffeTrustMap(Map> spiffe private void updateSslContextWhenReady() { if (isMtls()) { - if (savedKey != null - && (savedTrustedRoots != null || isUsingSystemRootCerts || savedSpiffeTrustMap != null)) { + if (savedKey != null && (savedTrustedRoots != null || savedSpiffeTrustMap != null)) { updateSslContext(); clearKeysAndCerts(); } @@ -176,15 +186,15 @@ private void clearKeysAndCerts() { } protected final boolean isMtls() { - return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts); + return certHandle != null && rootCertHandle != null; } protected final boolean isClientSideTls() { - return rootCertInstance != null && certInstance == null; + return rootCertHandle != null && certHandle == null; } protected final boolean isServerSideTls() { - return certInstance != null && rootCertInstance == null; + return certHandle != null && rootCertHandle == null; } @Override @@ -201,4 +211,46 @@ public final void close() { rootCertHandle.close(); } } + + interface NoExceptionCloseable extends Closeable { + @Override + void close(); + } + + static final class IgnoreUpdatesWatcher implements CertificateProvider.Watcher { + private final CertificateProvider.Watcher delegate; + private final boolean ignoreRootCertUpdates; + + public IgnoreUpdatesWatcher( + CertificateProvider.Watcher delegate, boolean ignoreRootCertUpdates) { + this.delegate = requireNonNull(delegate, "delegate"); + this.ignoreRootCertUpdates = ignoreRootCertUpdates; + } + + @Override + public void updateCertificate(PrivateKey key, List certChain) { + if (ignoreRootCertUpdates) { + delegate.updateCertificate(key, certChain); + } + } + + @Override + public void updateTrustedRoots(List trustedRoots) { + if (!ignoreRootCertUpdates) { + delegate.updateTrustedRoots(trustedRoots); + } + } + + @Override + public void updateSpiffeTrustMap(Map> spiffeTrustMap) { + if (!ignoreRootCertUpdates) { + delegate.updateSpiffeTrustMap(spiffeTrustMap); + } + } + + @Override + public void onError(Status errorStatus) { + delegate.onError(errorStatus); + } + } } diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/SystemRootCertificateProvider.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/SystemRootCertificateProvider.java new file mode 100644 index 00000000000..7c60f714e71 --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/SystemRootCertificateProvider.java @@ -0,0 +1,71 @@ +/* + * Copyright 2020 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.security.certprovider; + +import io.grpc.Status; +import java.security.KeyStore; +import java.security.KeyStoreException; +import java.security.NoSuchAlgorithmException; +import java.security.cert.X509Certificate; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.stream.Collectors; +import javax.net.ssl.TrustManager; +import javax.net.ssl.TrustManagerFactory; +import javax.net.ssl.X509TrustManager; + +/** + * An non-registered provider for CertProviderSslContextProvider to use the same code path for + * system root certs as provider-obtained certs. + */ +final class SystemRootCertificateProvider extends CertificateProvider { + public SystemRootCertificateProvider(CertificateProvider.Watcher watcher) { + super(new DistributorWatcher(), false); + getWatcher().addWatcher(watcher); + } + + @Override + public void start() { + try { + TrustManagerFactory trustManagerFactory = TrustManagerFactory.getInstance( + TrustManagerFactory.getDefaultAlgorithm()); + trustManagerFactory.init((KeyStore) null); + + List trustManagers = Arrays.asList(trustManagerFactory.getTrustManagers()); + List rootCerts = trustManagers.stream() + .filter(X509TrustManager.class::isInstance) + .map(X509TrustManager.class::cast) + .map(trustManager -> Arrays.asList(trustManager.getAcceptedIssuers())) + .flatMap(Collection::stream) + .collect(Collectors.toList()); + getWatcher().updateTrustedRoots(rootCerts); + } catch (KeyStoreException | NoSuchAlgorithmException ex) { + getWatcher().onError(Status.UNAVAILABLE + .withDescription("Could not load system root certs") + .withCause(ex)); + } + } + + @Override + public void close() { + // Unnecessary because there's no more callbacks, but do it for good measure + for (Watcher watcher : getWatcher().getDownstreamWatchers()) { + getWatcher().removeWatcher(watcher); + } + } +} From 0d5eb0a7e2ed8fcdac2e5781b053299aaea15929 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 23 Sep 2025 11:28:46 +0000 Subject: [PATCH 19/23] Fix certs not updated for handshake. --- .../CertProviderSslContextProvider.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 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 ef9a3cf062c..05902115a94 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 @@ -40,10 +40,13 @@ abstract class CertProviderSslContextProvider extends DynamicSslContextProvider @Nullable private final NoExceptionCloseable certHandle; @Nullable private final NoExceptionCloseable rootCertHandle; + @Nullable private final CertificateProviderInstance certInstance; + @Nullable protected final CertificateProviderInstance rootCertInstance; @Nullable protected PrivateKey savedKey; @Nullable protected List savedCertChain; @Nullable protected List savedTrustedRoots; @Nullable protected Map> savedSpiffeTrustMap; + private final boolean isUsingSystemRootCerts; protected CertProviderSslContextProvider( Node node, @@ -54,6 +57,10 @@ protected CertProviderSslContextProvider( BaseTlsContext tlsContext, CertificateProviderStore certificateProviderStore) { super(tlsContext, staticCertValidationContext); + this.certInstance = certInstance; + this.rootCertInstance = rootCertInstance; + this.isUsingSystemRootCerts = rootCertInstance == null + && CommonTlsContextUtil.isUsingSystemRootCerts(tlsContext.getCommonTlsContext()); boolean createCertInstance = certInstance != null && certInstance.isInitialized(); boolean createRootCertInstance = rootCertInstance != null && rootCertInstance.isInitialized(); boolean sharedCertInstance = createCertInstance && createRootCertInstance @@ -186,15 +193,15 @@ private void clearKeysAndCerts() { } protected final boolean isMtls() { - return certHandle != null && rootCertHandle != null; + return certInstance != null && (rootCertInstance != null || isUsingSystemRootCerts); } protected final boolean isClientSideTls() { - return rootCertHandle != null && certHandle == null; + return rootCertInstance != null && certInstance == null; } protected final boolean isServerSideTls() { - return certHandle != null && rootCertHandle == null; + return certInstance != null && rootCertInstance == null; } @Override From c14a4884ca2873b2ff1647208c4d51df7a792a26 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 23 Sep 2025 13:54:58 +0000 Subject: [PATCH 20/23] More fixes for system root certs. --- .../CertProviderSslContextProvider.java | 54 ++------------- .../certprovider/IgnoreUpdatesWatcher.java | 68 +++++++++++++++++++ .../grpc/xds/XdsSecurityClientServerTest.java | 5 -- .../ClientSslContextProviderFactoryTest.java | 32 +++++---- .../ServerSslContextProviderFactoryTest.java | 16 ++--- ...tProviderClientSslContextProviderTest.java | 22 +----- 6 files changed, 104 insertions(+), 93 deletions(-) create mode 100644 xds/src/main/java/io/grpc/xds/internal/security/certprovider/IgnoreUpdatesWatcher.java 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 1020932e834..967ed836007 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 @@ -16,13 +16,10 @@ package io.grpc.xds.internal.security.certprovider; -import static java.util.Objects.requireNonNull; - import io.envoyproxy.envoy.config.core.v3.Node; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CertificateValidationContext; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext; import io.envoyproxy.envoy.extensions.transport_sockets.tls.v3.CommonTlsContext.CertificateProviderInstance; -import io.grpc.Status; import io.grpc.xds.EnvoyServerProtoData.BaseTlsContext; import io.grpc.xds.client.Bootstrapper.CertificateProviderInfo; import io.grpc.xds.internal.security.CommonTlsContextUtil; @@ -69,7 +66,7 @@ protected CertProviderSslContextProvider( CertificateProviderInfo certProviderInstanceConfig = getCertProviderConfig(certProviders, certInstance.getInstanceName()); CertificateProvider.Watcher watcher = this; - if (!sharedCertInstance) { + if (!sharedCertInstance && !isUsingSystemRootCerts) { watcher = new IgnoreUpdatesWatcher(watcher, /* ignoreRootCertUpdates= */ true); } // TODO: Previously we'd hang if certProviderInstanceConfig were null or @@ -156,9 +153,6 @@ public final void updateCertificate(PrivateKey key, List certCh @Override public final void updateTrustedRoots(List trustedRoots) { - if (isUsingSystemRootCerts) { - return; - } savedTrustedRoots = trustedRoots; updateSslContextWhenReady(); } @@ -190,7 +184,9 @@ private void updateSslContextWhenReady() { private void clearKeysAndCerts() { savedKey = null; - savedTrustedRoots = null; + if (!isUsingSystemRootCerts) { + savedTrustedRoots = null; + } savedSpiffeTrustMap = null; savedCertChain = null; } @@ -200,10 +196,7 @@ protected final boolean isMtls() { } protected final boolean isRegularTlsAndClientSide() { - // We don't do (rootCertInstance != null || isUsingSystemRootCerts) here because of how this - // method is used. With the rootCertInstance being null when using system root certs, there - // is nothing to update in the SslContext - return rootCertInstance != null && certInstance == null; + return (rootCertInstance != null || isUsingSystemRootCerts) && certInstance == null; } protected final boolean isRegularTlsAndServerSide() { @@ -229,41 +222,4 @@ interface NoExceptionCloseable extends Closeable { @Override void close(); } - - static final class IgnoreUpdatesWatcher implements CertificateProvider.Watcher { - private final CertificateProvider.Watcher delegate; - private final boolean ignoreRootCertUpdates; - - public IgnoreUpdatesWatcher( - CertificateProvider.Watcher delegate, boolean ignoreRootCertUpdates) { - this.delegate = requireNonNull(delegate, "delegate"); - this.ignoreRootCertUpdates = ignoreRootCertUpdates; - } - - @Override - public void updateCertificate(PrivateKey key, List certChain) { - if (ignoreRootCertUpdates) { - delegate.updateCertificate(key, certChain); - } - } - - @Override - public void updateTrustedRoots(List trustedRoots) { - if (!ignoreRootCertUpdates) { - delegate.updateTrustedRoots(trustedRoots); - } - } - - @Override - public void updateSpiffeTrustMap(Map> spiffeTrustMap) { - if (!ignoreRootCertUpdates) { - delegate.updateSpiffeTrustMap(spiffeTrustMap); - } - } - - @Override - public void onError(Status errorStatus) { - delegate.onError(errorStatus); - } - } } diff --git a/xds/src/main/java/io/grpc/xds/internal/security/certprovider/IgnoreUpdatesWatcher.java b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/IgnoreUpdatesWatcher.java new file mode 100644 index 00000000000..cd9d88be41b --- /dev/null +++ b/xds/src/main/java/io/grpc/xds/internal/security/certprovider/IgnoreUpdatesWatcher.java @@ -0,0 +1,68 @@ +/* + * Copyright 2025 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.xds.internal.security.certprovider; + +import static java.util.Objects.requireNonNull; + +import com.google.common.annotations.VisibleForTesting; +import io.grpc.Status; +import java.security.PrivateKey; +import java.security.cert.X509Certificate; +import java.util.List; +import java.util.Map; + +public final class IgnoreUpdatesWatcher implements CertificateProvider.Watcher { + private final CertificateProvider.Watcher delegate; + private final boolean ignoreRootCertUpdates; + + public IgnoreUpdatesWatcher( + CertificateProvider.Watcher delegate, boolean ignoreRootCertUpdates) { + this.delegate = requireNonNull(delegate, "delegate"); + this.ignoreRootCertUpdates = ignoreRootCertUpdates; + } + + @Override + public void updateCertificate(PrivateKey key, List certChain) { + if (ignoreRootCertUpdates) { + delegate.updateCertificate(key, certChain); + } + } + + @Override + public void updateTrustedRoots(List trustedRoots) { + if (!ignoreRootCertUpdates) { + delegate.updateTrustedRoots(trustedRoots); + } + } + + @Override + public void updateSpiffeTrustMap(Map> spiffeTrustMap) { + if (!ignoreRootCertUpdates) { + delegate.updateSpiffeTrustMap(spiffeTrustMap); + } + } + + @Override + public void onError(Status errorStatus) { + delegate.onError(errorStatus); + } + + @VisibleForTesting + public CertificateProvider.Watcher getDelegate() { + return delegate; + } +} diff --git a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java index 03f1651f2c7..e46e440475a 100644 --- a/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java +++ b/xds/src/test/java/io/grpc/xds/XdsSecurityClientServerTest.java @@ -282,10 +282,6 @@ public void tlsClientServer_useSystemRootCerts_mtls() throws Exception { } } - /** - * Use system root ca cert for TLS channel - no mTLS. - * Subj Alt Names to match are specified in the validaton context. - */ @Test public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() throws Exception { Path trustStoreFilePath = getCacertFilePathForTestCa(); @@ -317,7 +313,6 @@ public void tlsClientServer_useSystemRootCerts_failureToMatchSubjAltNames() thro /** * Use system root ca cert for TLS channel - mTLS. - * Uses common_tls_context.combined_validation_context in upstream_tls_context. */ @Test public void tlsClientServer_useSystemRootCerts_requireClientAuth() throws Exception { diff --git a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java index 397fe01e0f5..a0eac581d5c 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/ClientSslContextProviderFactoryTest.java @@ -37,6 +37,7 @@ import io.grpc.xds.internal.security.certprovider.CertificateProviderProvider; import io.grpc.xds.internal.security.certprovider.CertificateProviderRegistry; import io.grpc.xds.internal.security.certprovider.CertificateProviderStore; +import io.grpc.xds.internal.security.certprovider.IgnoreUpdatesWatcher; import io.grpc.xds.internal.security.certprovider.TestCertificateProvider; import org.junit.Before; import org.junit.Test; @@ -84,7 +85,7 @@ public void createCertProviderClientSslContextProvider() throws XdsInitializatio clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], false); // verify that bootstrapInfo is cached... sslContextProvider = clientSslContextProviderFactory.create(upstreamTlsContext); @@ -119,7 +120,7 @@ public void bothPresent_expectCertProviderClientSslContextProvider() clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } @Test @@ -145,7 +146,7 @@ public void createCertProviderClientSslContextProvider_onlyRootCert() clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } @Test @@ -179,7 +180,7 @@ public void createCertProviderClientSslContextProvider_withStaticContext() clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } @Test @@ -209,8 +210,8 @@ public void createCertProviderClientSslContextProvider_2providers() clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); - verifyWatcher(sslContextProvider, watcherCaptor[1]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); + verifyWatcher(sslContextProvider, watcherCaptor[1], true); } @Test @@ -246,8 +247,8 @@ public void createNewCertProviderClientSslContextProvider_withSans() { clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); - verifyWatcher(sslContextProvider, watcherCaptor[1]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); + verifyWatcher(sslContextProvider, watcherCaptor[1], true); } @Test @@ -280,7 +281,7 @@ public void createNewCertProviderClientSslContextProvider_onlyRootCert() { clientSslContextProviderFactory.create(upstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderClientSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } static void createAndRegisterProviderProvider( @@ -310,11 +311,18 @@ public CertificateProvider answer(InvocationOnMock invocation) throws Throwable } static void verifyWatcher( - SslContextProvider sslContextProvider, CertificateProvider.DistributorWatcher watcherCaptor) { + SslContextProvider sslContextProvider, CertificateProvider.DistributorWatcher watcherCaptor, + boolean usesDelegateWatcher) { assertThat(watcherCaptor).isNotNull(); assertThat(watcherCaptor.getDownstreamWatchers()).hasSize(1); - assertThat(watcherCaptor.getDownstreamWatchers().iterator().next()) - .isSameInstanceAs(sslContextProvider); + if (usesDelegateWatcher) { + assertThat(((IgnoreUpdatesWatcher) watcherCaptor.getDownstreamWatchers().iterator().next()) + .getDelegate()) + .isSameInstanceAs(sslContextProvider); + } else { + assertThat(watcherCaptor.getDownstreamWatchers().iterator().next()) + .isSameInstanceAs(sslContextProvider); + } } static CommonTlsContext.Builder addFilenames( diff --git a/xds/src/test/java/io/grpc/xds/internal/security/ServerSslContextProviderFactoryTest.java b/xds/src/test/java/io/grpc/xds/internal/security/ServerSslContextProviderFactoryTest.java index cf86b511f1f..7a5a6c00639 100644 --- a/xds/src/test/java/io/grpc/xds/internal/security/ServerSslContextProviderFactoryTest.java +++ b/xds/src/test/java/io/grpc/xds/internal/security/ServerSslContextProviderFactoryTest.java @@ -78,7 +78,7 @@ public void createCertProviderServerSslContextProvider() throws XdsInitializatio serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], false); // verify that bootstrapInfo is cached... sslContextProvider = serverSslContextProviderFactory.create(downstreamTlsContext); @@ -117,7 +117,7 @@ public void bothPresent_expectCertProviderServerSslContextProvider() serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } @Test @@ -144,7 +144,7 @@ public void createCertProviderServerSslContextProvider_onlyCertInstance() serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); } @Test @@ -179,7 +179,7 @@ public void createCertProviderServerSslContextProvider_withStaticContext() serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); + verifyWatcher(sslContextProvider, watcherCaptor[0], false); } @Test @@ -210,8 +210,8 @@ public void createCertProviderServerSslContextProvider_2providers() serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); - verifyWatcher(sslContextProvider, watcherCaptor[1]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); + verifyWatcher(sslContextProvider, watcherCaptor[1], true); } @Test @@ -249,7 +249,7 @@ public void createNewCertProviderServerSslContextProvider_withSans() serverSslContextProviderFactory.create(downstreamTlsContext); assertThat(sslContextProvider.getClass().getSimpleName()).isEqualTo( "CertProviderServerSslContextProvider"); - verifyWatcher(sslContextProvider, watcherCaptor[0]); - verifyWatcher(sslContextProvider, watcherCaptor[1]); + verifyWatcher(sslContextProvider, watcherCaptor[0], true); + verifyWatcher(sslContextProvider, watcherCaptor[1], true); } } 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 3b2ca05231e..84e858a8ab9 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 @@ -235,15 +235,8 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception { assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); - assertThat(provider.getSslContext()).isNull(); - - // now generate root cert update, will get ignored because of systemRootCerts config - watcherCaptor[0].updateTrustedRoots(ImmutableList.of(getCertFromResourceName(CA_PEM_FILE))); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContext()).isNull(); - assertThat(provider.savedKey).isNull(); - assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); // now generate cert update watcherCaptor[0].updateCertificate( @@ -251,6 +244,7 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception { ImmutableList.of(getCertFromResourceName(CLIENT_PEM_FILE))); assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContext()).isNotNull(); TestCallback testCallback = @@ -261,23 +255,13 @@ public void testProviderForClient_systemRootCerts_mtls() throws Exception { CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); - // just do root cert update: sslContext should still be the same, will get ignored because of - // systemRootCerts config - watcherCaptor[0].updateTrustedRoots( - ImmutableList.of(getCertFromResourceName(SERVER_0_PEM_FILE))); - assertThat(provider.savedKey).isNull(); - assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); - testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); - assertThat(testCallback1.updatedSslContext).isSameInstanceAs(testCallback.updatedSslContext); - // now update id cert: sslContext should be updated i.e. different from the previous one watcherCaptor[0].updateCertificate( CommonCertProviderTestUtils.getPrivateKey(SERVER_1_KEY_FILE), 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.getSslContext()).isNotNull(); testCallback1 = CommonTlsContextTestsUtil.getValueThruCallback(provider); assertThat(testCallback1.updatedSslContext).isNotSameInstanceAs(testCallback.updatedSslContext); From 733f57c9943d6eaf2815c9b1cb4f378322637a5c Mon Sep 17 00:00:00 2001 From: Kannan J Date: Tue, 23 Sep 2025 14:09:01 +0000 Subject: [PATCH 21/23] More fixes for system root certs. --- .../certprovider/CertProviderClientSslContextProviderTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 84e858a8ab9..3c734df3f5a 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 @@ -206,7 +206,7 @@ public void testProviderForClient_systemRootCerts_regularTls() { assertThat(provider.savedKey).isNull(); assertThat(provider.savedCertChain).isNull(); - assertThat(provider.savedTrustedRoots).isNull(); + assertThat(provider.savedTrustedRoots).isNotNull(); assertThat(provider.getSslContext()).isNotNull(); TestCallback testCallback = CommonTlsContextTestsUtil.getValueThruCallback(provider); From 967fe8c268c4ac611f90a7a3c9a5a22942534f05 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Wed, 24 Sep 2025 06:33:54 +0000 Subject: [PATCH 22/23] Address review comment to remove reundant if block --- .../security/certprovider/CertProviderSslContextProvider.java | 4 +--- 1 file changed, 1 insertion(+), 3 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 967ed836007..b4df876dfe3 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 @@ -82,9 +82,7 @@ protected CertProviderSslContextProvider( } else { certHandle = null; } - if (createRootCertInstance && sharedCertInstance) { - rootCertHandle = () -> { }; - } else if (createRootCertInstance && !sharedCertInstance) { + if (createRootCertInstance && !sharedCertInstance) { CertificateProviderInfo certProviderInstanceConfig = getCertProviderConfig(certProviders, rootCertInstance.getInstanceName()); rootCertHandle = certProviderInstanceConfig == null ? null From 0edfb2f1ce752a945b4731876abf1b189e14e9a3 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 25 Sep 2025 05:27:14 +0000 Subject: [PATCH 23/23] Address review comments. --- .../security/certprovider/CertProviderSslContextProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b4df876dfe3..2570dcb731b 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 @@ -184,8 +184,8 @@ private void clearKeysAndCerts() { savedKey = null; if (!isUsingSystemRootCerts) { savedTrustedRoots = null; + savedSpiffeTrustMap = null; } - savedSpiffeTrustMap = null; savedCertChain = null; }