Remove special-casing for System root certs in SslContextProviderSupplier and handle it in

kannanjgithub · kannanjgithub · commit 7f48afa77823 · 2025-09-11T06:28:12.000Z
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
@@ -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
@@ -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,15 +49,24 @@ 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
   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
@@ -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
@@ -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<SslContextProvider.Callback> 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<SslContextProvider.Callback> 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<Runnable> 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));
   }
-}
+}
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
@@ -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();
   }

Original file line number	Diff line number	Diff line change
`@@ -158,12 +158,12 @@ private void updateSslContextWhenReady() {`
`158`	`158`	`updateSslContext();`
`159`	`159`	`clearKeysAndCerts();`
`160`	`160`	`}`
`161`		`- } else if (isNormalTlsAndClientSide()) {`
	`161`	`+ } else if (isRegularTlsAndClientSide()) {`
`162`	`162`	`if (savedTrustedRoots != null \|\| savedSpiffeTrustMap != null) {`
`163`	`163`	`updateSslContext();`
`164`	`164`	`clearKeysAndCerts();`
`165`	`165`	`}`
`166`		`- } else if (isNormalTlsAndServerSide()) {`
	`166`	`+ } else if (isRegularTlsAndServerSide()) {`
`167`	`167`	`if (savedKey != null) {`
`168`	`168`	`updateSslContext();`
`169`	`169`	`clearKeysAndCerts();`
`@@ -182,14 +182,14 @@ protected final boolean isMtls() {`
`182`	`182`	`return certInstance != null && (rootCertInstance != null \|\| isUsingSystemRootCerts);`
`183`	`183`	`}`
`184`	`184`
`185`		`- protected final boolean isNormalTlsAndClientSide() {`
	`185`	`+ protected final boolean isRegularTlsAndClientSide() {`
`186`	`186`	`// We don't do (rootCertInstance != null \|\| isUsingSystemRootCerts) here because of how this`
`187`	`187`	`// method is used. With the rootCertInstance being null when using system root certs, there`
`188`	`188`	`// is nothing to update in the SslContext`
`189`	`189`	`return rootCertInstance != null && certInstance == null;`
`190`	`190`	`}`
`191`	`191`
`192`		`- protected final boolean isNormalTlsAndServerSide() {`
	`192`	`+ protected final boolean isRegularTlsAndServerSide() {`
`193`	`193`	`return certInstance != null && rootCertInstance == null;`
`194`	`194`	`}`
`195`	`195`