security: Stabilize AdvancedTlsX509KeyManager. (#11139)

* Clean up and de-experimentalization of KeyManager

* Unit tests for API validity.

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

@@ -45,14 +45,11 @@ import io.grpc.util.AdvancedTlsX509TrustManager.Verification;
 import io.grpc.util.CertificateUtils;
 import java.io.Closeable;
 import java.io.File;
-import java.io.IOException;
 import java.net.Socket;
 import java.security.GeneralSecurityException;
-import java.security.NoSuchAlgorithmException;
 import java.security.PrivateKey;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import java.security.spec.InvalidKeySpecException;
 import java.util.concurrent.Executors;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
@@ -65,13 +62,13 @@ import org.junit.runners.JUnit4;
 
 @RunWith(JUnit4.class)
 public class AdvancedTlsTest {
-  public static final String SERVER_0_KEY_FILE = "server0.key";
+  private static final String SERVER_0_KEY_FILE = "server0.key";
-  public static final String SERVER_0_PEM_FILE = "server0.pem";
+  private static final String SERVER_0_PEM_FILE = "server0.pem";
-  public static final String CLIENT_0_KEY_FILE = "client.key";
+  private static final String CLIENT_0_KEY_FILE = "client.key";
-  public static final String CLIENT_0_PEM_FILE = "client.pem";
+  private static final String CLIENT_0_PEM_FILE = "client.pem";
-  public static final String CA_PEM_FILE = "ca.pem";
+  private static final String CA_PEM_FILE = "ca.pem";
-  public static final String SERVER_BAD_KEY_FILE = "badserver.key";
+  private static final String SERVER_BAD_KEY_FILE = "badserver.key";
-  public static final String SERVER_BAD_PEM_FILE = "badserver.pem";
+  private static final String SERVER_BAD_PEM_FILE = "badserver.pem";
 
   private ScheduledExecutorService executor;
   private Server server;
@@ -92,7 +89,7 @@ public class AdvancedTlsTest {
 
   @Before
   public void setUp()
-      throws NoSuchAlgorithmException, IOException, CertificateException, InvalidKeySpecException {
+      throws Exception {
     executor = Executors.newSingleThreadScheduledExecutor();
     caCertFile = TestUtils.loadCert(CA_PEM_FILE);
     serverKey0File = TestUtils.loadCert(SERVER_0_KEY_FILE);
@@ -285,11 +282,11 @@ public class AdvancedTlsTest {
             new SslSocketAndEnginePeerVerifier() {
               @Override
               public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
-                  Socket socket) throws CertificateException { }
+                  Socket socket) { }
 
               @Override
               public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
-                  SSLEngine engine) throws CertificateException { }
+                  SSLEngine engine) { }
             })
         .build();
     serverTrustManager.updateTrustCredentials(caCert);
@@ -310,11 +307,11 @@ public class AdvancedTlsTest {
             new SslSocketAndEnginePeerVerifier() {
               @Override
               public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
-                  Socket socket) throws CertificateException { }
+                  Socket socket) { }
 
               @Override
               public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType,
-                  SSLEngine engine) throws CertificateException { }
+                  SSLEngine engine) { }
             })
         .build();
     clientTrustManager.updateTrustCredentials(caCert);
@@ -419,7 +416,7 @@ public class AdvancedTlsTest {
   }
 
   @Test
-  public void onFileReloadingKeyManagerBadInitialContentTest() throws Exception {
+  public void onFileReloadingKeyManagerBadInitialContentTest() {
     AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
     // We swap the order of key and certificates to intentionally create an exception.
     assertThrows(GeneralSecurityException.class,
@@ -439,7 +436,7 @@ public class AdvancedTlsTest {
   }
 
   @Test
-  public void keyManagerAliasesTest() throws Exception {
+  public void keyManagerAliasesTest() {
     AdvancedTlsX509KeyManager km = new AdvancedTlsX509KeyManager();
     assertArrayEquals(
         new String[] {"default"}, km.getClientAliases("", null));

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

@@ -18,7 +18,6 @@ package io.grpc.util;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import io.grpc.ExperimentalApi;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -26,7 +25,6 @@ import java.net.Socket;
 import java.security.GeneralSecurityException;
 import java.security.Principal;
 import java.security.PrivateKey;
-import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.Arrays;
 import java.util.concurrent.ScheduledExecutorService;
@@ -39,20 +37,15 @@ 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, etc.
+ * 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.
-  // The credential information sent to peers to prove our identity.
+  private static final int MINIMUM_REFRESH_PERIOD_IN_MINUTES = 1 ;
+  // The credential information to be sent to peers to prove our identity.
   private volatile KeyInfo keyInfo;
 
-  /**
-   * Constructs an AdvancedTlsX509KeyManager.
-   */
-  public AdvancedTlsX509KeyManager() throws CertificateException { }
-
   @Override
   public PrivateKey getPrivateKey(String alias) {
     if (alias.equals("default")) {
@@ -107,14 +100,17 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
    * @param certs  the certificate chain that is going to be used
    */
   public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) {
-    // TODO(ZhenLian): explore possibilities to do a crypto check here.
     this.keyInfo = new KeyInfo(checkNotNull(key, "key"), checkNotNull(certs, "certs"));
   }
 
   /**
    * 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.
+   * updated. You must close the returned Closeable before calling this method again or other update
+   * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link
+   * AdvancedTlsX509KeyManager#updateIdentityCredentialsFromFile(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
@@ -131,14 +127,17 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
       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(keyFile, certFile), period, period, unit);
-    return new Closeable() {
+    return () -> future.cancel(false);
-      @Override public void close() {
-        future.cancel(false);
-      }
-    };
   }
 
   /**
@@ -190,8 +189,9 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
           this.currentCertTime = newResult.certTime;
         }
       } catch (IOException | GeneralSecurityException e) {
-        log.log(Level.SEVERE, "Failed refreshing private key and certificate chain from files. "
+        log.log(Level.SEVERE, e, () -> String.format("Failed refreshing private key and certificate"
-            + "Using previous ones", e);
+                + " chain from files. Using previous ones (keyFile lastModified = %s, certFile "
+                + "lastModified = %s)", keyFile.lastModified(), certFile.lastModified()));
       }
     }
   }
@@ -220,8 +220,8 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
    */
   private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long oldCertTime)
       throws IOException, GeneralSecurityException {
-    long newKeyTime = keyFile.lastModified();
+    long newKeyTime = checkNotNull(keyFile, "keyFile").lastModified();
-    long newCertTime = certFile.lastModified();
+    long newCertTime = checkNotNull(certFile, "certFile").lastModified();
     // We only update when both the key and the certs are updated.
     if (newKeyTime != oldKeyTime && newCertTime != oldCertTime) {
       FileInputStream keyInputStream = new FileInputStream(keyFile);

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

@@ -0,0 +1,165 @@
+/*
+ * 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.assertThrows;
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+
+import io.grpc.internal.FakeClock;
+import io.grpc.internal.testing.TestUtils;
+import io.grpc.testing.TlsTesting;
+import java.io.File;
+import java.security.PrivateKey;
+import java.security.cert.X509Certificate;
+import java.util.ArrayList;
+import java.util.List;
+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 AdvancedTlsX509KeyManager}. */
+@RunWith(JUnit4.class)
+public class AdvancedTlsX509KeyManagerTest {
+  private static final String SERVER_0_KEY_FILE = "server0.key";
+  private static final String SERVER_0_PEM_FILE = "server0.pem";
+  private static final String CLIENT_0_KEY_FILE = "client.key";
+  private static final String CLIENT_0_PEM_FILE = "client.pem";
+  private static final String ALIAS = "default";
+
+  private ScheduledExecutorService executor;
+
+  private File serverKey0File;
+  private File serverCert0File;
+  private File clientKey0File;
+  private File clientCert0File;
+
+  private PrivateKey serverKey0;
+  private X509Certificate[] serverCert0;
+  private PrivateKey clientKey0;
+  private X509Certificate[] clientCert0;
+
+  @Before
+  public void setUp() throws Exception {
+    executor = new FakeClock().getScheduledExecutorService();
+    serverKey0File = TestUtils.loadCert(SERVER_0_KEY_FILE);
+    serverCert0File = TestUtils.loadCert(SERVER_0_PEM_FILE);
+    clientKey0File = TestUtils.loadCert(CLIENT_0_KEY_FILE);
+    clientCert0File = TestUtils.loadCert(CLIENT_0_PEM_FILE);
+    serverKey0 = CertificateUtils.getPrivateKey(TlsTesting.loadCert(SERVER_0_KEY_FILE));
+    serverCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(SERVER_0_PEM_FILE));
+    clientKey0 = CertificateUtils.getPrivateKey(TlsTesting.loadCert(CLIENT_0_KEY_FILE));
+    clientCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(CLIENT_0_PEM_FILE));
+  }
+
+  @Test
+  public void credentialSetting() throws Exception {
+    // Overall happy path checking of public API.
+    AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
+    serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0);
+    assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
+    assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
+
+    serverKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File);
+    assertEquals(clientKey0, serverKeyManager.getPrivateKey(ALIAS));
+    assertArrayEquals(clientCert0, serverKeyManager.getCertificateChain(ALIAS));
+
+    serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1,
+        TimeUnit.MINUTES, executor);
+    assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
+    assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
+  }
+
+  @Test
+  public void credentialSettingParameterValidity() throws Exception {
+    // Checking edge cases of public API parameter setting.
+    AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
+    NullPointerException npe = assertThrows(NullPointerException.class, () -> serverKeyManager
+        .updateIdentityCredentials(null, serverCert0));
+    assertEquals("key", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> serverKeyManager
+        .updateIdentityCredentials(serverKey0, null));
+    assertEquals("certs", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> serverKeyManager
+        .updateIdentityCredentialsFromFile(null, serverCert0File));
+    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,
+            executor));
+    assertEquals("unit", npe.getMessage());
+
+    npe = assertThrows(NullPointerException.class, () -> serverKeyManager
+        .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1,
+            TimeUnit.MINUTES, null));
+    assertEquals("executor", npe.getMessage());
+
+    Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName());
+    TestHandler handler = new TestHandler();
+    log.addHandler(handler);
+    log.setUseParentHandlers(false);
+    log.setLevel(Level.FINE);
+    serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, -1,
+            TimeUnit.SECONDS, executor);
+    log.removeHandler(handler);
+    for (LogRecord record : handler.getRecords()) {
+      if (record.getMessage().contains("Default value of ")) {
+        assertTrue(true);
+        return;
+      }
+    }
+    fail("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;
+    }
+  }
+
+}