security: Add updateIdentityCredentials methods to AdvancedTlsX509KeyManager. (#11358)

Add new 'updateIdentityCredentials' methods with swapped (chain, key) signatures.

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

@@ -147,7 +147,7 @@ public class AdvancedTlsTest {
   public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception {
     // Create a server with the key manager and trust manager.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0);
+    serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
@@ -159,7 +159,7 @@ public class AdvancedTlsTest {
         new SimpleServiceImpl()).build().start();
     // Create a client with the key manager and trust manager.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
+    clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0);
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
@@ -182,7 +182,7 @@ public class AdvancedTlsTest {
   @Test
   public void trustManagerCustomVerifierMutualTlsTest() throws Exception {
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0);
+    serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
     // Set server's custom verification based on the information of clientCert0.
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
@@ -221,7 +221,7 @@ public class AdvancedTlsTest {
         new SimpleServiceImpl()).build().start();
 
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
+    clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0);
     // Set client's custom verification based on the information of serverCert0.
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
@@ -275,7 +275,7 @@ public class AdvancedTlsTest {
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
     // Even if we provide bad credentials for the server, the test should still pass, because we
     // will configure the client to skip all checks later.
-    serverKeyManager.updateIdentityCredentials(serverKeyBad, serverCertBad);
+    serverKeyManager.updateIdentityCredentials(serverCertBad, serverKeyBad);
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .setSslSocketAndEnginePeerVerifier(
@@ -297,7 +297,7 @@ public class AdvancedTlsTest {
         new SimpleServiceImpl()).build().start();
 
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
+    clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0);
     // Set the client to skip all checks, including traditional certificate verification.
     // Note this is very dangerous in production environment - only do so if you are confident on
     // what you are doing!
@@ -334,8 +334,8 @@ public class AdvancedTlsTest {
   public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
     // Create & start a server.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File,
+    Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentials(serverCert0File,
-        serverCert0File, 100, TimeUnit.MILLISECONDS, executor);
+        serverKey0File, 100, TimeUnit.MILLISECONDS, executor);
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
@@ -348,8 +348,8 @@ public class AdvancedTlsTest {
         new SimpleServiceImpl()).build().start();
     // Create a client to connect.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File,
+    Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentials(clientCert0File,
-        clientCert0File,100, TimeUnit.MILLISECONDS, executor);
+        clientKey0File,100, TimeUnit.MILLISECONDS, executor);
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
@@ -381,7 +381,7 @@ public class AdvancedTlsTest {
   public void onFileLoadingKeyManagerTrustManagerTest() throws Exception {
     // Create & start a server.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File);
+    serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File);
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
@@ -393,7 +393,7 @@ public class AdvancedTlsTest {
         new SimpleServiceImpl()).build().start();
     // Create a client to connect.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File);
+    clientKeyManager.updateIdentityCredentials(clientCert0File, clientKey0File);
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
@@ -420,8 +420,8 @@ public class AdvancedTlsTest {
     AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
     // We swap the order of key and certificates to intentionally create an exception.
     assertThrows(GeneralSecurityException.class,
-        () -> keyManager.updateIdentityCredentialsFromFile(serverCert0File,
+        () -> keyManager.updateIdentityCredentials(serverKey0File, serverCert0File,
-          serverKey0File, 100, TimeUnit.MILLISECONDS, executor));
+          100, TimeUnit.MILLISECONDS, executor));
   }
 
   @Test

util/BUILD.bazel

@@ -13,6 +13,7 @@ java_library(
         "//api",
         "//core:internal",
         artifact("com.google.code.findbugs:jsr305"),
+        artifact("com.google.errorprone:error_prone_annotations"),
         artifact("com.google.guava:guava"),
         artifact("com.google.j2objc:j2objc-annotations"),
         artifact("org.codehaus.mojo:animal-sniffer-annotations"),

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

@@ -18,7 +18,7 @@ package io.grpc.util;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import io.grpc.ExperimentalApi;
+import com.google.errorprone.annotations.InlineMe;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -40,7 +40,6 @@ import javax.net.ssl.X509ExtendedKeyManager;
  * AdvancedTlsX509KeyManager is an {@code X509ExtendedKeyManager} that allows users to configure
  * advanced TLS features, such as private key and certificate chain reloading.
  */
-@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
 public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
   private static final Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName());
   // Minimum allowed period for refreshing files with credential information.
@@ -100,31 +99,44 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
    *
    * @param key  the private key that is going to be used
    * @param certs  the certificate chain that is going to be used
+   * @deprecated Use {@link #updateIdentityCredentials(X509Certificate[], PrivateKey)}
    */
+  @Deprecated
+  @InlineMe(replacement = "this.updateIdentityCredentials(certs, key)")
   public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) {
+    updateIdentityCredentials(certs, key);
+  }
+
+  /**
+   * Updates the current cached private key and cert chains.
+   *
+   * @param certs  the certificate chain that is going to be used
+   * @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"));
   }
 
   /**
-   * Schedules a {@code ScheduledExecutorService} to read private key and certificate chains from
+   * Schedules a {@code ScheduledExecutorService} to read certificate chains and private key from
    * the local file paths periodically, and update the cached identity credentials if they are both
    * updated. You must close the returned Closeable before calling this method again or other update
    * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link
-   * AdvancedTlsX509KeyManager#updateIdentityCredentialsFromFile(File, File)}).
+   * AdvancedTlsX509KeyManager#updateIdentityCredentials(File, File)}).
    * Before scheduling the task, the method synchronously executes {@code  readAndUpdate} once. The
    * minimum refresh period of 1 minute is enforced.
    *
-   * @param keyFile  the file on disk holding the private key
    * @param certFile  the file on disk holding the certificate chain
+   * @param keyFile  the file on disk holding the private key
    * @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 updateIdentityCredentialsFromFile(File keyFile, File certFile,
+  public Closeable updateIdentityCredentials(File certFile, File keyFile,
       long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException,
       GeneralSecurityException {
-    UpdateResult newResult = readAndUpdate(keyFile, certFile, 0, 0);
+    UpdateResult newResult = readAndUpdate(certFile, keyFile, 0, 0);
     if (!newResult.success) {
       throw new GeneralSecurityException(
           "Files were unmodified before their initial update. Probably a bug.");
@@ -138,23 +150,64 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
     }
     final ScheduledFuture<?> future =
         checkNotNull(executor, "executor").scheduleWithFixedDelay(
-            new LoadFilePathExecution(keyFile, certFile), period, period, unit);
+            new LoadFilePathExecution(certFile, keyFile), period, period, unit);
     return () -> future.cancel(false);
   }
 
+  /**
+   * Updates certificate chains and the private key from the local file paths.
+   *
+   * @param certFile  the file on disk holding the certificate chain
+   * @param keyFile  the file on disk holding the private key
+   */
+  public void updateIdentityCredentials(File certFile, File keyFile) throws IOException,
+      GeneralSecurityException {
+    UpdateResult newResult = readAndUpdate(certFile, keyFile, 0, 0);
+    if (!newResult.success) {
+      throw new GeneralSecurityException(
+          "Files were unmodified before their initial update. Probably a bug.");
+    }
+  }
+
   /**
    * Updates the private key and certificate chains from the local file paths.
    *
    * @param keyFile  the file on disk holding the private key
    * @param certFile  the file on disk holding the certificate chain
+   * @deprecated Use {@link #updateIdentityCredentials(File, File)} instead.
    */
+  @Deprecated
+  @InlineMe(replacement = "this.updateIdentityCredentials(certFile, keyFile)")
   public void updateIdentityCredentialsFromFile(File keyFile, File certFile) throws IOException,
       GeneralSecurityException {
-    UpdateResult newResult = readAndUpdate(keyFile, certFile, 0, 0);
+    updateIdentityCredentials(certFile, keyFile);
-    if (!newResult.success) {
-      throw new GeneralSecurityException(
-          "Files were unmodified before their initial update. Probably a bug.");
   }
+
+  /**
+   * Schedules a {@code ScheduledExecutorService} to read private key and certificate chains from
+   * the local file paths periodically, and update the cached identity credentials if they are both
+   * updated. You must close the returned Closeable before calling this method again or other update
+   * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link
+   * AdvancedTlsX509KeyManager#updateIdentityCredentials(File, File)}).
+   * Before scheduling the task, the method synchronously executes {@code  readAndUpdate} once. The
+   * minimum refresh period of 1 minute is enforced.
+   *
+   * @param keyFile  the file on disk holding the private key
+   * @param certFile  the file on disk holding the certificate chain
+   * @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
+   * #updateIdentityCredentials(File, File, long, TimeUnit, ScheduledExecutorService)} instead.
+   */
+  @Deprecated
+  @InlineMe(replacement =
+      "this.updateIdentityCredentials(certFile, keyFile, period, unit, executor)")
+  public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile,
+      long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException,
+      GeneralSecurityException {
+    return updateIdentityCredentials(certFile, keyFile, period, unit, executor);
   }
 
   private static class KeyInfo {
@@ -174,9 +227,9 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
     long currentKeyTime;
     long currentCertTime;
 
-    public LoadFilePathExecution(File keyFile, File certFile) {
+    public LoadFilePathExecution(File certFile, File keyFile) {
-      this.keyFile = keyFile;
       this.certFile = certFile;
+      this.keyFile = keyFile;
       this.currentKeyTime = 0;
       this.currentCertTime = 0;
     }
@@ -184,7 +237,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
     @Override
     public void run() {
       try {
-        UpdateResult newResult = readAndUpdate(this.keyFile, this.certFile, this.currentKeyTime,
+        UpdateResult newResult = readAndUpdate(this.certFile, this.keyFile, this.currentKeyTime,
             this.currentCertTime);
         if (newResult.success) {
           this.currentKeyTime = newResult.keyTime;
@@ -192,8 +245,8 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
         }
       } catch (IOException | GeneralSecurityException e) {
         log.log(Level.SEVERE, String.format("Failed refreshing private key and certificate"
-                + " chain from files. Using previous ones (keyFile lastModified = %s, certFile "
+                + " chain from files. Using previous ones (certFile lastModified = %s, keyFile "
-                + "lastModified = %s)", keyFile.lastModified(), certFile.lastModified()), e);
+                + "lastModified = %s)", certFile.lastModified(), keyFile.lastModified()), e);
       }
     }
   }
@@ -214,13 +267,13 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
    * Reads the private key and certificates specified in the path locations. Updates {@code key} and
    * {@code cert} if both of their modified time changed since last read.
    *
-   * @param keyFile  the file on disk holding the private key
    * @param certFile  the file on disk holding the certificate chain
+   * @param keyFile  the file on disk holding the private key
    * @param oldKeyTime the time when the private key file is modified during last execution
    * @param oldCertTime the time when the certificate chain file is modified during last execution
    * @return the result of this update execution
    */
-  private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long oldCertTime)
+  private UpdateResult readAndUpdate(File certFile, File keyFile, long oldKeyTime, long oldCertTime)
       throws IOException, GeneralSecurityException {
     long newKeyTime = checkNotNull(keyFile, "keyFile").lastModified();
     long newCertTime = checkNotNull(certFile, "certFile").lastModified();
@@ -232,7 +285,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
         FileInputStream certInputStream = new FileInputStream(certFile);
         try {
           X509Certificate[] certs = CertificateUtils.getX509Certificates(certInputStream);
-          updateIdentityCredentials(key, certs);
+          updateIdentityCredentials(certs, key);
           return new UpdateResult(true, newKeyTime, newCertTime);
         } finally {
           certInputStream.close();

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

@@ -79,15 +79,15 @@ public class AdvancedTlsX509KeyManagerTest {
   public void credentialSetting() throws Exception {
     // Overall happy path checking of public API.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0);
+    serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
     assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
     assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
 
-    serverKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File);
+    serverKeyManager.updateIdentityCredentials(clientCert0File, clientKey0File);
     assertEquals(clientKey0, serverKeyManager.getPrivateKey(ALIAS));
     assertArrayEquals(clientCert0, serverKeyManager.getCertificateChain(ALIAS));
 
-    serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1,
+    serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File,1,
         TimeUnit.MINUTES, executor);
     assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
     assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
@@ -98,28 +98,28 @@ public class AdvancedTlsX509KeyManagerTest {
     // Checking edge cases of public API parameter setting.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
     NullPointerException npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentials(null, serverCert0));
+        .updateIdentityCredentials(serverCert0, null));
     assertEquals("key", npe.getMessage());
 
     npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentials(serverKey0, null));
+        .updateIdentityCredentials(null, serverKey0));
     assertEquals("certs", npe.getMessage());
 
     npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentialsFromFile(null, serverCert0File));
+        .updateIdentityCredentials(null, serverKey0File));
-    assertEquals("keyFile", npe.getMessage());
-
-    npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentialsFromFile(serverKey0File, null));
     assertEquals("certFile", npe.getMessage());
 
     npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, null,
+        .updateIdentityCredentials(serverCert0File, null));
+    assertEquals("keyFile", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> serverKeyManager
+        .updateIdentityCredentials(serverCert0File, serverKey0File, 1, null,
             executor));
     assertEquals("unit", npe.getMessage());
 
     npe = assertThrows(NullPointerException.class, () -> serverKeyManager
-        .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1,
+        .updateIdentityCredentials(serverCert0File, serverKey0File, 1,
             TimeUnit.MINUTES, null));
     assertEquals("executor", npe.getMessage());
 
@@ -128,7 +128,7 @@ public class AdvancedTlsX509KeyManagerTest {
     log.addHandler(handler);
     log.setUseParentHandlers(false);
     log.setLevel(Level.FINE);
-    serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, -1,
+    serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File, -1,
             TimeUnit.SECONDS, executor);
     log.removeHandler(handler);
     for (LogRecord record : handler.getRecords()) {