util: Stabilize AdvancedTlsX509TrustManager

Author: erm-g

util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java

@@ -16,7 +16,8 @@
 
 package io.grpc.util;
 
-import io.grpc.ExperimentalApi;
+import static com.google.common.base.Preconditions.checkNotNull;
+
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -42,31 +43,36 @@
 
 /**
  * AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure
- * advanced TLS features, such as root certificate reloading, peer cert custom verification, etc.
- * For Android users: this class is only supported in API level 24 and above.
+ * advanced TLS features, such as root certificate reloading and peer cert custom verification.
+ * The basic instantiation pattern is
+ * <code>new Builder().build().useSystemDefaultTrustCerts();</code>
+ *
+ * <p>For Android users: this class is only supported in API level 24 and above.
  */
-@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
 @IgnoreJRERequirement
 public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager {
   private static final Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName());
 
+  // Minimum allowed period for refreshing files with credential information.
+  private static final int MINIMUM_REFRESH_PERIOD_IN_MINUTES = 1;
+  private static final String NOT_ENOUGH_INFO_MESSAGE =
+      "Not enough information to validate peer. SSLEngine or Socket required.";
   private final Verification verification;
   private final SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier;
 
   // The delegated trust manager used to perform traditional certificate verification.
   private volatile X509ExtendedTrustManager delegateManager = null;
 
   private AdvancedTlsX509TrustManager(Verification verification,
-      SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) throws CertificateException {
+      SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) {
     this.verification = verification;
     this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier;
   }
 
   @Override
   public void checkClientTrusted(X509Certificate[] chain, String authType)
       throws CertificateException {
-    throw new CertificateException(
-        "Not enough information to validate peer. SSLEngine or Socket required.");
+    throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE);
   }
 
   @Override
@@ -90,8 +96,7 @@ public void checkServerTrusted(X509Certificate[] chain,  String authType, SSLEng
   @Override
   public void checkServerTrusted(X509Certificate[] chain, String authType)
       throws CertificateException {
-    throw new CertificateException(
-        "Not enough information to validate peer. SSLEngine or Socket required.");
+    throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE);
   }
 
   @Override
@@ -127,6 +132,7 @@ public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreEx
    */
   public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOException,
       GeneralSecurityException {
+    checkNotNull(trustCerts, "trustCerts");
     KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
     keyStore.load(null, null);
     int i = 1;
@@ -135,8 +141,7 @@ public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOExcept
       keyStore.setCertificateEntry(alias, cert);
       i++;
     }
-    X509ExtendedTrustManager newDelegateManager = createDelegateTrustManager(keyStore);
-    this.delegateManager = newDelegateManager;
+    this.delegateManager = createDelegateTrustManager(keyStore);
   }
 
   private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyStore)
@@ -148,9 +153,9 @@ private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyS
     TrustManager[] tms = tmf.getTrustManagers();
     // Iterate over the returned trust managers, looking for an instance of X509TrustManager.
     // If found, use that as the delegate trust manager.
-    for (int j = 0; j < tms.length; j++) {
-      if (tms[j] instanceof X509ExtendedTrustManager) {
-        delegateManager = (X509ExtendedTrustManager) tms[j];
+    for (TrustManager tm : tms) {
+      if (tm instanceof X509ExtendedTrustManager) {
+        delegateManager = (X509ExtendedTrustManager) tm;
         break;
       }
     }
@@ -169,8 +174,7 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
           "Want certificate verification but got null or empty certificates");
     }
     if (sslEngine == null && socket == null) {
-      throw new CertificateException(
-          "Not enough information to validate peer. SSLEngine or Socket required.");
+      throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE);
     }
     if (this.verification != Verification.INSECURELY_SKIP_ALL_VERIFICATION) {
       X509ExtendedTrustManager currentDelegateManager = this.delegateManager;
@@ -211,12 +215,18 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
 
   /**
    * Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
-   * periodically, and update the cached trust certs if there is an update.
+   * periodically, and updates the cached trust certs if there is an update. You must close the
+   * returned Closeable before calling this method again or other update methods
+   * ({@link AdvancedTlsX509TrustManager#useSystemDefaultTrustCerts()},
+   * {@link AdvancedTlsX509TrustManager#updateTrustCredentials(X509Certificate[])},
+   * {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File)}).
+   * Before scheduling the task, the method synchronously reads and updates trust certificates once.
+   * If the provided period is less than 1 minute, it is automatically adjusted to 1 minute.
    *
    * @param trustCertFile  the file on disk holding the trust certificates
    * @param period the period between successive read-and-update executions
    * @param unit the time unit of the initialDelay and period parameters
-   * @param executor the execute service we use to read and update the credentials
+   * @param executor the executor service we use to read and update the credentials
    * @return an object that caller should close when the file refreshes are not needed
    */
   public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
@@ -226,14 +236,17 @@ public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period,
       throw new GeneralSecurityException(
           "Files were unmodified before their initial update. Probably a bug.");
     }
+    if (checkNotNull(unit, "unit").toMinutes(period) < MINIMUM_REFRESH_PERIOD_IN_MINUTES) {
+      log.log(Level.FINE,
+          "Provided refresh period of {0} {1} is too small. Default value of {2} minute(s) "
+          + "will be used.", new Object[] {period, unit.name(), MINIMUM_REFRESH_PERIOD_IN_MINUTES});
+      period = MINIMUM_REFRESH_PERIOD_IN_MINUTES;
+      unit = TimeUnit.MINUTES;
+    }
     final ScheduledFuture<?> future =
-        executor.scheduleWithFixedDelay(
+        checkNotNull(executor, "executor").scheduleWithFixedDelay(
             new LoadFilePathExecution(trustCertFile), period, period, unit);
-    return new Closeable() {
-      @Override public void close() {
-        future.cancel(false);
-      }
-    };
+    return () -> future.cancel(false);
   }
 
   /**
@@ -264,13 +277,14 @@ public void run() {
       try {
         this.currentTime = readAndUpdate(this.file, this.currentTime);
       } catch (IOException | GeneralSecurityException e) {
-        log.log(Level.SEVERE, "Failed refreshing trust CAs from file. Using previous CAs", e);
+        log.log(Level.SEVERE, String.format("Failed refreshing trust CAs from file. Using "
+            + "previous CAs (file lastModified = %s)", file.lastModified()), e);
       }
     }
   }
 
   /**
-   * Reads the trust certificates specified in the path location, and update the key store if the
+   * Reads the trust certificates specified in the path location, and updates the key store if the
    * modified time has changed since last read.
    *
    * @param trustCertFile  the file on disk holding the trust certificates
@@ -279,7 +293,7 @@ public void run() {
    */
   private long readAndUpdate(File trustCertFile, long oldTime)
       throws IOException, GeneralSecurityException {
-    long newTime = trustCertFile.lastModified();
+    long newTime = checkNotNull(trustCertFile, "trustCertFile").lastModified();
     if (newTime == oldTime) {
       return oldTime;
     }
@@ -303,27 +317,32 @@ public static Builder newBuilder() {
     return new Builder();
   }
 
-  // The verification mode when authenticating the peer certificate.
+  /**
+   * The verification mode when authenticating the peer certificate.
+   */
   public enum Verification {
-    // This is the DEFAULT and RECOMMENDED mode for most applications.
-    // Setting this on the client side will do the certificate and hostname verification, while
-    // setting this on the server side will only do the certificate verification.
+    /**
+     * This is the DEFAULT and RECOMMENDED mode for most applications.
+     * Setting this on the client side performs both certificate and hostname verification, while
+     * setting it on the server side only performs certificate verification.
+     */
     CERTIFICATE_AND_HOST_NAME_VERIFICATION,
-    // This SHOULD be chosen only when you know what the implication this will bring, and have a
-    // basic understanding about TLS.
-    // It SHOULD be accompanied with proper additional peer identity checks set through
-    // {@code PeerVerifier}(nit: why this @code not working?). Failing to do so will leave
-    // applications to MITM attack.
-    // Also note that this will only take effect if the underlying SDK implementation invokes
-    // checkClientTrusted/checkServerTrusted with the {@code SSLEngine} parameter while doing
-    // verification.
-    // Setting this on either side will only do the certificate verification.
+    /**
+     * DANGEROUS: Use trusted credentials to verify the certificate, but clients will not verify the
+     * certificate is for the expected host. This setting is only appropriate when accompanied by
+     * proper additional peer identity checks set through SslSocketAndEnginePeerVerifier. Failing to
+     * do so will leave your applications vulnerable to MITM attacks.
+     * This setting has the same behavior on server-side as CERTIFICATE_AND_HOST_NAME_VERIFICATION.
+     */
     CERTIFICATE_ONLY_VERIFICATION,
-    // Setting is very DANGEROUS. Please try to avoid this in a real production environment, unless
-    // you are a super advanced user intended to re-implement the whole verification logic on your
-    // own. A secure verification might include:
-    // 1. proper verification on the peer certificate chain
-    // 2. proper checks on the identity of the peer certificate
+    /**
+     * DANGEROUS: This SHOULD be used by advanced user intended to implement the entire verification
+     * logic themselves {@link SslSocketAndEnginePeerVerifier}) themselves. This includes: <br>
+     * 1. Proper verification of the peer certificate chain <br>
+     * 2. Proper checks of the identity of the peer certificate <br>
+     * Failing to do so will leave your application without any TLS-related protection. Keep in mind
+     * that any loaded trust certificates will be ignored when using this mode.
+     */
     INSECURELY_SKIP_ALL_VERIFICATION,
   }
 
@@ -356,18 +375,41 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSL
         throws CertificateException;
   }
 
+  /**
+   * Builds a new {@link AdvancedTlsX509TrustManager}. By default, no trust certificates are loaded
+   * after the build. To load them, use one of the following methods: {@link
+   * AdvancedTlsX509TrustManager#updateTrustCredentials(X509Certificate[])}, {@link
+   * AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File, long, TimeUnit,
+   * ScheduledExecutorService)}, {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile
+   * (File, long, TimeUnit, ScheduledExecutorService)}.
+   */
   public static final class Builder {
 
     private Verification verification = Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION;
     private SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier;
 
     private Builder() {}
 
+    /**
+     * Sets {@link Verification}, mode when authenticating the peer certificate. By default, {@link
+     * Verification#CERTIFICATE_AND_HOST_NAME_VERIFICATION} value is used.
+     *
+     * @param  verification Verification mode used for the current AdvancedTlsX509TrustManager
+     * @return Builder with set verification
+     */
     public Builder setVerification(Verification verification) {
       this.verification = verification;
       return this;
     }
 
+    /**
+     * Sets {@link SslSocketAndEnginePeerVerifier}, which methods will be called in addition to
+     * verifying certificates.
+     *
+     * @param  verifier SslSocketAndEnginePeerVerifier used for the current
+     *         AdvancedTlsX509TrustManager
+     * @return Builder with set verifier
+     */
     public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier verifier) {
       this.socketAndEnginePeerVerifier = verifier;
       return this;

util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java

@@ -0,0 +1,162 @@
+/*
+ * Copyright 2024 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.util;
+
+import static org.junit.Assert.assertArrayEquals;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThrows;
+
+import com.google.common.collect.Iterables;
+import io.grpc.internal.FakeClock;
+import io.grpc.internal.testing.TestUtils;
+import io.grpc.testing.TlsTesting;
+import java.io.File;
+import java.io.IOException;
+import java.net.Socket;
+import java.security.GeneralSecurityException;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.NoSuchElementException;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+/** Unit tests for {@link AdvancedTlsX509TrustManager}. */
+@RunWith(JUnit4.class)
+public class AdvancedTlsX509TrustManagerTest {
+
+  private static final String CA_PEM_FILE = "ca.pem";
+  private static final String SERVER_0_PEM_FILE = "server0.pem";
+  private File caCertFile;
+  private File serverCert0File;
+
+  private X509Certificate[] caCert;
+  private X509Certificate[] serverCert0;
+
+  private ScheduledExecutorService executor;
+
+  @Before
+  public void setUp() throws IOException, GeneralSecurityException {
+    executor = new FakeClock().getScheduledExecutorService();
+    caCertFile = TestUtils.loadCert(CA_PEM_FILE);
+    caCert = CertificateUtils.getX509Certificates(TlsTesting.loadCert(CA_PEM_FILE));
+    serverCert0File = TestUtils.loadCert(SERVER_0_PEM_FILE);
+    serverCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(SERVER_0_PEM_FILE));
+  }
+
+  @Test
+  public void updateTrustCredentials_replacesIssuers() throws Exception {
+    // Overall happy path checking of public API.
+    AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
+    trustManager.updateTrustCredentialsFromFile(serverCert0File);
+    assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
+
+    trustManager.updateTrustCredentials(caCert);
+    assertArrayEquals(caCert, trustManager.getAcceptedIssuers());
+
+    trustManager.updateTrustCredentialsFromFile(serverCert0File, 1, TimeUnit.MINUTES,
+        executor);
+    assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
+
+    trustManager.updateTrustCredentialsFromFile(serverCert0File);
+    assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
+  }
+
+  @Test
+  public void systemDefaultDelegateManagerInstantiation() throws Exception {
+    AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
+    trustManager.useSystemDefaultTrustCerts();
+    CertificateException ce = assertThrows(CertificateException.class, () -> trustManager
+        .checkServerTrusted(serverCert0, "RSA", new Socket()));
+    assertEquals("socket is not a type of SSLSocket", ce.getMessage());
+  }
+
+  @Test
+  public void credentialSettingParameterValidity() throws Exception {
+    // Checking edge cases of public API parameter setting.
+    AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
+
+    NullPointerException npe = assertThrows(NullPointerException.class, () -> trustManager
+        .updateTrustCredentials(null));
+    assertEquals("trustCerts", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> trustManager
+        .updateTrustCredentialsFromFile(null));
+    assertEquals("trustCertFile", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> trustManager
+        .updateTrustCredentialsFromFile(null, 1, null, null));
+    assertEquals("trustCertFile", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> trustManager
+        .updateTrustCredentialsFromFile(caCertFile, 1, null, null));
+    assertEquals("unit", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> trustManager
+        .updateTrustCredentialsFromFile(caCertFile, 1, TimeUnit.MINUTES, null));
+    assertEquals("executor", npe.getMessage());
+
+    Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName());
+    TestHandler handler = new TestHandler();
+    log.addHandler(handler);
+    log.setUseParentHandlers(false);
+    log.setLevel(Level.FINE);
+    trustManager.updateTrustCredentialsFromFile(serverCert0File, -1, TimeUnit.SECONDS,
+        executor);
+    log.removeHandler(handler);
+    try {
+      LogRecord logRecord = Iterables.find(handler.getRecords(),
+          record -> record.getMessage().contains("Default value of "));
+      assertNotNull(logRecord);
+    } catch (NoSuchElementException e) {
+      throw new AssertionError("Log message related to setting default values not found");
+    }
+  }
+
+
+  private static class TestHandler extends Handler {
+    private final List<LogRecord> records = new ArrayList<>();
+
+    @Override
+    public void publish(LogRecord record) {
+      records.add(record);
+    }
+
+    @Override
+    public void flush() {
+    }
+
+    @Override
+    public void close() throws SecurityException {
+    }
+
+    public List<LogRecord> getRecords() {
+      return records;
+    }
+  }
+
+}