Merge branch 'systemrootcerts-ignore-trusted-root-updates' into system-root-changes-needed

# Conflicts:
#	xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java
#	xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java
#	xds/src/test/java/io/grpc/xds/internal/security/SslContextProviderSupplierTest.java

Author: kannanjgithub

xds/src/main/java/io/grpc/xds/internal/security/SslContextProviderSupplier.java

@@ -16,6 +16,8 @@
 
 package io.grpc.xds.internal.security;
 
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.MoreObjects;
 import io.grpc.netty.GrpcSslContexts;
@@ -30,8 +32,6 @@
 import javax.net.ssl.SSLException;
 import java.util.Set;
 
-import static com.google.common.base.Preconditions.checkNotNull;
-
 /**
  * Enables Client or server side to initialize this object with the received {@link BaseTlsContext}
  * and communicate it to the consumer i.e. {@link SecurityProtocolNegotiators}
@@ -66,8 +66,10 @@ public synchronized void updateSslContext(final SslContextProvider.Callback call
           sslContextProvider = getSslContextProvider(sni);
         }
       }
-
       // we want to increment the ref-count so call findOrCreate again...
+      final SslContextProvider toRelease = getSslContextProvider();
+      toRelease.addCallback(
+          new SslContextProvider.Callback(callback.getExecutor()) {
       final SslContextProvider toRelease = getSslContextProvider(sni);
       // 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.
@@ -86,19 +88,18 @@ public synchronized void updateSslContext(final SslContextProvider.Callback call
         toRelease.addCallback(
             new SslContextProvider.Callback(callback.getExecutor()) {
 
-              @Override
-              public void updateSslContext(SslContext sslContext) {
-                callback.updateSslContext(sslContext);
-                releaseSslContextProvider(toRelease, sni);
-              }
-
-              @Override
-              public void onException(Throwable throwable) {
-                callback.onException(throwable);
-                releaseSslContextProvider(toRelease, sni);
-              }
-            });
-      }
+            @Override
+            public void updateSslContext(SslContext sslContext) {
+              callback.updateSslContext(sslContext);
+              releaseSslContextProvider(toRelease, sni);
+            }
+
+            @Override
+            public void onException(Throwable throwable) {
+              callback.onException(throwable);
+              releaseSslContextProvider(toRelease, sni);
+            }
+          });
     } catch (final Throwable throwable) {
       callback.getExecutor().execute(new Runnable() {
         @Override

xds/src/main/java/io/grpc/xds/internal/security/certprovider/CertProviderClientSslContextProvider.java

@@ -28,11 +28,10 @@
 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. */
-public final class CertProviderClientSslContextProvider extends CertProviderSslContextProvider {
-
-  private final String sniForSanMatching;
+final class CertProviderClientSslContextProvider extends CertProviderSslContextProvider {
 
   CertProviderClientSslContextProvider(
           Node node,
@@ -50,6 +49,17 @@ public final class CertProviderClientSslContextProvider extends CertProviderSslC
         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);
+      }
+    }
     this.sniForSanMatching = upstreamTlsContext.getAutoSniSanValidation()? sniForSanMatching : null;
   }
 
@@ -58,8 +68,6 @@ protected final SslContextBuilder getSslContextBuilder(
           CertificateValidationContext certificateValidationContext)
       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(

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

@@ -162,12 +162,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();
@@ -186,14 +186,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;
   }
 

xds/src/test/java/io/grpc/xds/internal/security/CommonTlsContextTestsUtil.java

@@ -234,6 +234,9 @@ public 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();

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

@@ -189,12 +189,7 @@ 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.
-   */
-  public void testProviderForClient_systemRootCerts_regularTls() throws Exception {
+  public void testProviderForClient_systemRootCerts_regularTls() {
     final CertificateProvider.DistributorWatcher[] watcherCaptor =
             new CertificateProvider.DistributorWatcher[1];
     TestCertificateProvider.createAndRegisterProviderProvider(
@@ -214,7 +209,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();
   }