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

erm-g · web-flow · commit ecae9b797d35 · 2024-07-10T12:30:27.000+05:30
Add new 'updateIdentityCredentials' methods with swapped (chain, key) signatures.
diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java
@@ -147,7 +147,7 @@ public void basicMutualTlsTest() throws Exception {
   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 void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception {
         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 void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception {
   @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 void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy
         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 void trustManagerInsecurelySkipAllTest() throws Exception {
     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 void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy
         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 void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy
   public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
     // Create & start a server.
     AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
-    Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File,
-        serverCert0File, 100, TimeUnit.MILLISECONDS, executor);
+    Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentials(serverCert0File,
+        serverKey0File, 100, TimeUnit.MILLISECONDS, executor);
     AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
         .build();
@@ -348,8 +348,8 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
         new SimpleServiceImpl()).build().start();
     // Create a client to connect.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
-    Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File,
-        clientCert0File,100, TimeUnit.MILLISECONDS, executor);
+    Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentials(clientCert0File,
+        clientKey0File,100, TimeUnit.MILLISECONDS, executor);
     AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
         .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
         .build();
@@ -381,7 +381,7 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
   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 void onFileLoadingKeyManagerTrustManagerTest() throws Exception {
         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 void onFileReloadingKeyManagerBadInitialContentTest() {
     AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
     // We swap the order of key and certificates to intentionally create an exception.
     assertThrows(GeneralSecurityException.class,
-        () -> keyManager.updateIdentityCredentialsFromFile(serverCert0File,
-          serverKey0File, 100, TimeUnit.MILLISECONDS, executor));
+        () -> keyManager.updateIdentityCredentials(serverKey0File, serverCert0File,
+          100, TimeUnit.MILLISECONDS, executor));
   }
 
   @Test
diff --git a/util/BUILD.bazel b/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"),
diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java
@@ -18,7 +18,7 @@
 
 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 @@
  * 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 String chooseEngineServerAlias(String keyType, Principal[] issuers,
    *
    * @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,25 +150,66 @@ public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile,
     }
     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 the private key and certificate chains from the local file paths.
+   * Updates certificate chains and the private key 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
+   * @param keyFile  the file on disk holding the private key
    */
-  public void updateIdentityCredentialsFromFile(File keyFile, File certFile) throws IOException,
+  public void updateIdentityCredentials(File certFile, File keyFile) 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.");
     }
   }
 
+  /**
+   * 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 {
+    updateIdentityCredentials(certFile, keyFile);
+  }
+
+  /**
+   * 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 {
     // The private key and the cert chain we will use to send to peers to prove our identity.
     final PrivateKey key;
@@ -174,26 +227,26 @@ private class LoadFilePathExecution implements Runnable {
     long currentKeyTime;
     long currentCertTime;
 
-    public LoadFilePathExecution(File keyFile, File certFile) {
-      this.keyFile = keyFile;
+    public LoadFilePathExecution(File certFile, File keyFile) {
       this.certFile = certFile;
+      this.keyFile = keyFile;
       this.currentKeyTime = 0;
       this.currentCertTime = 0;
     }
 
     @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;
           this.currentCertTime = newResult.certTime;
         }
       } 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 "
-                + "lastModified = %s)", keyFile.lastModified(), certFile.lastModified()), e);
+                + " chain from files. Using previous ones (certFile lastModified = %s, keyFile "
+                + "lastModified = %s)", certFile.lastModified(), keyFile.lastModified()), e);
       }
     }
   }
@@ -214,13 +267,13 @@ public UpdateResult(boolean success, long keyTime, long certTime) {
    * 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 @@ private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime,
         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();
diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java
