util: Align AdvancedTlsX509{Key and Trust}Manager (#11385)

* Swap internal key/cert args

* Deprecate *FromFile methods

* Test for client trusted socket

netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java

@@ -339,7 +339,7 @@ public class AdvancedTlsTest {
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
-    Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentialsFromFile(caCertFile,
+    Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentials(caCertFile,
         100, TimeUnit.MILLISECONDS, executor);
     ServerCredentials serverCredentials = TlsServerCredentials.newBuilder()
         .keyManager(serverKeyManager).trustManager(serverTrustManager)
@@ -349,11 +349,11 @@ public class AdvancedTlsTest {
     // Create a client to connect.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
     Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentials(clientCert0File,
-        clientKey0File,100, TimeUnit.MILLISECONDS, executor);
+        clientKey0File, 100, TimeUnit.MILLISECONDS, executor);
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
-    Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentialsFromFile(caCertFile,
+    Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentials(caCertFile,
         100, TimeUnit.MILLISECONDS, executor);
     ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder()
         .keyManager(clientKeyManager).trustManager(clientTrustManager).build();
@@ -385,7 +385,7 @@ public class AdvancedTlsTest {
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
-    serverTrustManager.updateTrustCredentialsFromFile(caCertFile);
+    serverTrustManager.updateTrustCredentials(caCertFile);
     ServerCredentials serverCredentials = TlsServerCredentials.newBuilder()
         .keyManager(serverKeyManager).trustManager(serverTrustManager)
         .clientAuth(ClientAuth.REQUIRE).build();
@@ -397,7 +397,7 @@ public class AdvancedTlsTest {
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
-    clientTrustManager.updateTrustCredentialsFromFile(caCertFile);
+    clientTrustManager.updateTrustCredentials(caCertFile);
     ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder()
         .keyManager(clientKeyManager).trustManager(clientTrustManager).build();
     channel = Grpc.newChannelBuilderForAddress("localhost", server.getPort(), channelCredentials)
@@ -431,8 +431,8 @@ public class AdvancedTlsTest {
         .build();
     // We pass in a key as the trust certificates to intentionally create an exception.
     assertThrows(GeneralSecurityException.class,
-        () -> trustManager.updateTrustCredentialsFromFile(serverKey0File,
+        () -> trustManager.updateTrustCredentials(serverKey0File, 100, TimeUnit.MILLISECONDS,
-          100, TimeUnit.MILLISECONDS, executor));
+            executor));
   }
 
   @Test

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

@@ -116,7 +116,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
    * @param key  the private key that is going to be used
    */
   public void updateIdentityCredentials(X509Certificate[] certs, PrivateKey key) {
-    this.keyInfo = new KeyInfo(checkNotNull(key, "key"), checkNotNull(certs, "certs"));
+    this.keyInfo = new KeyInfo(checkNotNull(certs, "certs"), checkNotNull(key, "key"));
   }
 
   /**
@@ -216,26 +216,26 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
 
   private static class KeyInfo {
     // The private key and the cert chain we will use to send to peers to prove our identity.
-    final PrivateKey key;
     final X509Certificate[] certs;
+    final PrivateKey key;
 
-    public KeyInfo(PrivateKey key, X509Certificate[] certs) {
+    public KeyInfo(X509Certificate[] certs, PrivateKey key) {
-      this.key = key;
       this.certs = certs;
+      this.key = key;
     }
   }
 
   private class LoadFilePathExecution implements Runnable {
     File keyFile;
     File certFile;
-    long currentKeyTime;
     long currentCertTime;
+    long currentKeyTime;
 
     public LoadFilePathExecution(File certFile, File keyFile) {
       this.certFile = certFile;
       this.keyFile = keyFile;
-      this.currentKeyTime = 0;
       this.currentCertTime = 0;
+      this.currentKeyTime = 0;
     }
 
     @Override
@@ -244,11 +244,11 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
         UpdateResult newResult = readAndUpdate(this.certFile, this.keyFile, this.currentKeyTime,
             this.currentCertTime);
         if (newResult.success) {
-          this.currentKeyTime = newResult.keyTime;
           this.currentCertTime = newResult.certTime;
+          this.currentKeyTime = newResult.keyTime;
         }
       } catch (IOException | GeneralSecurityException e) {
-        log.log(Level.SEVERE, String.format("Failed refreshing private key and certificate"
+        log.log(Level.SEVERE, String.format("Failed refreshing certificate and private key"
                 + " chain from files. Using previous ones (certFile lastModified = %s, keyFile "
                 + "lastModified = %s)", certFile.lastModified(), keyFile.lastModified()), e);
       }
@@ -257,13 +257,13 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
 
   private static class UpdateResult {
     boolean success;
-    long keyTime;
     long certTime;
+    long keyTime;
 
-    public UpdateResult(boolean success, long keyTime, long certTime) {
+    public UpdateResult(boolean success, long certTime, long keyTime) {
       this.success = success;
-      this.keyTime = keyTime;
       this.certTime = certTime;
+      this.keyTime = keyTime;
     }
   }
 

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

@@ -18,6 +18,8 @@ package io.grpc.util;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.errorprone.annotations.InlineMe;
+import io.grpc.ExperimentalApi;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -116,7 +118,7 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
   /**
    * Uses the default trust certificates stored on user's local system.
    * After this is used, functions that will provide new credential
-   * data(e.g. updateTrustCredentials(), updateTrustCredentialsFromFile()) should not be called.
+   * data(e.g. updateTrustCredentials) should not be called.
    */
   public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreException,
       NoSuchAlgorithmException {
@@ -125,25 +127,6 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
     this.delegateManager = createDelegateTrustManager(null);
   }
 
-  /**
-   * Updates the current cached trust certificates as well as the key store.
-   *
-   * @param trustCerts the trust certificates that are going to be used
-   */
-  public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOException,
-      GeneralSecurityException {
-    checkNotNull(trustCerts, "trustCerts");
-    KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
-    keyStore.load(null, null);
-    int i = 1;
-    for (X509Certificate cert: trustCerts) {
-      String alias = Integer.toString(i);
-      keyStore.setCertificateEntry(alias, cert);
-      i++;
-    }
-    this.delegateManager = createDelegateTrustManager(keyStore);
-  }
-
   private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyStore)
       throws CertificateException, KeyStoreException, NoSuchAlgorithmException {
     TrustManagerFactory tmf = TrustManagerFactory.getInstance(
@@ -217,6 +200,39 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
     }
   }
 
+  /**
+   * Updates the current cached trust certificates as well as the key store.
+   *
+   * @param trustCerts the trust certificates that are going to be used
+   */
+  public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOException,
+      GeneralSecurityException {
+    checkNotNull(trustCerts, "trustCerts");
+    KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
+    keyStore.load(null, null);
+    int i = 1;
+    for (X509Certificate cert: trustCerts) {
+      String alias = Integer.toString(i);
+      keyStore.setCertificateEntry(alias, cert);
+      i++;
+    }
+    this.delegateManager = createDelegateTrustManager(keyStore);
+  }
+
+  /**
+   * Updates the trust certificates from a local file path.
+   *
+   * @param trustCertFile  the file on disk holding the trust certificates
+   */
+  public void updateTrustCredentials(File trustCertFile) throws IOException,
+      GeneralSecurityException {
+    long updatedTime = readAndUpdate(trustCertFile, 0);
+    if (updatedTime == 0) {
+      throw new GeneralSecurityException(
+          "Files were unmodified before their initial update. Probably a bug.");
+    }
+  }
+
   /**
    * Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
    * periodically, and updates the cached trust certs if there is an update. You must close the
@@ -233,7 +249,7 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
    * @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,
+  public Closeable updateTrustCredentials(File trustCertFile, long period, TimeUnit unit,
       ScheduledExecutorService executor) throws IOException, GeneralSecurityException {
     long updatedTime = readAndUpdate(trustCertFile, 0);
     if (updatedTime == 0) {
@@ -253,18 +269,43 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
     return () -> future.cancel(false);
   }
 
+  /**
+   * Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
+   * 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 executor service we use to read and update the credentials
+   * @return an object that caller should close when the file refreshes are not needed
+   * @deprecated Use {@link #updateTrustCredentials(File, long ,TimeUnit, ScheduledExecutorService)}
+   */
+  @Deprecated
+  @InlineMe(replacement = "this.updateTrustCredentials(trustCertFile, period, unit, executor)")
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
+  public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
+      ScheduledExecutorService executor) throws IOException, GeneralSecurityException {
+    return updateTrustCredentials(trustCertFile, period, unit, executor);
+  }
+
   /**
    * Updates the trust certificates from a local file path.
    *
    * @param trustCertFile  the file on disk holding the trust certificates
+   * @deprecated Use {@link #updateTrustCredentials(File)}
    */
+  @Deprecated
+  @InlineMe(replacement = "this.updateTrustCredentials(trustCertFile)")
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
   public void updateTrustCredentialsFromFile(File trustCertFile) throws IOException,
       GeneralSecurityException {
-    long updatedTime = readAndUpdate(trustCertFile, 0);
+    updateTrustCredentials(trustCertFile);
-    if (updatedTime == 0) {
-      throw new GeneralSecurityException(
-          "Files were unmodified before their initial update. Probably a bug.");
-    }
   }
 
   private class LoadFilePathExecution implements Runnable {
@@ -383,8 +424,8 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager
    * 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,
+   * AdvancedTlsX509TrustManager#updateTrustCredentials(File, long, TimeUnit,
-   * ScheduledExecutorService)}, {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile
+   * ScheduledExecutorService)}, {@link AdvancedTlsX509TrustManager#updateTrustCredentials
    * (File, long, TimeUnit, ScheduledExecutorService)}.
    */
   public static final class Builder {

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

@@ -76,7 +76,7 @@ public class AdvancedTlsX509KeyManagerTest {
   }
 
   @Test
-  public void credentialSetting() throws Exception {
+  public void updateTrustCredentials_replacesIssuers() throws Exception {
     // Overall happy path checking of public API.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
     serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
@@ -91,6 +91,10 @@ public class AdvancedTlsX509KeyManagerTest {
         TimeUnit.MINUTES, executor);
     assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
     assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
+
+    serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
+    assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
+    assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
   }
 
   @Test

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

@@ -20,11 +20,14 @@ 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 static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 import com.google.common.collect.Iterables;
 import io.grpc.internal.FakeClock;
 import io.grpc.internal.testing.TestUtils;
 import io.grpc.testing.TlsTesting;
+import io.grpc.util.AdvancedTlsX509TrustManager.Verification;
 import java.io.File;
 import java.io.IOException;
 import java.net.Socket;
@@ -40,6 +43,7 @@ import java.util.logging.Handler;
 import java.util.logging.Level;
 import java.util.logging.LogRecord;
 import java.util.logging.Logger;
+import javax.net.ssl.SSLSocket;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -72,17 +76,17 @@ public class AdvancedTlsX509TrustManagerTest {
   public void updateTrustCredentials_replacesIssuers() throws Exception {
     // Overall happy path checking of public API.
     AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
-    trustManager.updateTrustCredentialsFromFile(serverCert0File);
+    trustManager.updateTrustCredentials(serverCert0File);
     assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
 
     trustManager.updateTrustCredentials(caCert);
     assertArrayEquals(caCert, trustManager.getAcceptedIssuers());
 
-    trustManager.updateTrustCredentialsFromFile(serverCert0File, 1, TimeUnit.MINUTES,
+    trustManager.updateTrustCredentials(serverCert0File, 1, TimeUnit.MINUTES,
         executor);
     assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
 
-    trustManager.updateTrustCredentialsFromFile(serverCert0File);
+    trustManager.updateTrustCredentials(serverCert0File);
     assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
   }
 
@@ -101,23 +105,15 @@ public class AdvancedTlsX509TrustManagerTest {
     AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
 
     NullPointerException npe = assertThrows(NullPointerException.class, () -> trustManager
-        .updateTrustCredentials(null));
+        .updateTrustCredentials(null, 1, null, 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));
+        .updateTrustCredentials(caCertFile, 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));
+        .updateTrustCredentials(caCertFile, 1, TimeUnit.MINUTES, null));
     assertEquals("executor", npe.getMessage());
 
     Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName());
@@ -125,8 +121,7 @@ public class AdvancedTlsX509TrustManagerTest {
     log.addHandler(handler);
     log.setUseParentHandlers(false);
     log.setLevel(Level.FINE);
-    trustManager.updateTrustCredentialsFromFile(serverCert0File, -1, TimeUnit.SECONDS,
+    trustManager.updateTrustCredentials(serverCert0File, -1, TimeUnit.SECONDS, executor);
-        executor);
     log.removeHandler(handler);
     try {
       LogRecord logRecord = Iterables.find(handler.getRecords(),
@@ -137,6 +132,18 @@ public class AdvancedTlsX509TrustManagerTest {
     }
   }
 
+  @Test
+  public void clientTrustedWithSocketTest() throws Exception {
+    AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder()
+        .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION).build();
+    trustManager.updateTrustCredentials(caCert);
+    SSLSocket sslSocket = mock(SSLSocket.class);
+    when(sslSocket.isConnected()).thenReturn(true);
+    when(sslSocket.getHandshakeSession()).thenReturn(null);
+    CertificateException ce = assertThrows(CertificateException.class, () -> trustManager
+        .checkClientTrusted(serverCert0, "RSA", sslSocket));
+    assertEquals("No handshake session", ce.getMessage());
+  }
 
   private static class TestHandler extends Handler {
     private final List<LogRecord> records = new ArrayList<>();