fix a flaky test in advanced TLS (#8474)

* fix a flaky test in advanced tls

Author: ZhenLian

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

@@ -23,12 +23,11 @@
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.net.Socket;
-import java.security.NoSuchAlgorithmException;
+import java.security.GeneralSecurityException;
 import java.security.Principal;
 import java.security.PrivateKey;
 import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
-import java.security.spec.InvalidKeySpecException;
 import java.util.Arrays;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
@@ -107,8 +106,7 @@ 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
    */
-  public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs)
-      throws CertificateException {
+  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"));
   }
@@ -126,10 +124,16 @@ public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs)
    * @return an object that caller should close when the file refreshes are not needed
    */
   public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile,
-      long period, TimeUnit unit, ScheduledExecutorService executor) {
+      long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException,
+      GeneralSecurityException {
+    UpdateResult newResult = readAndUpdate(keyFile, certFile, 0, 0);
+    if (!newResult.success) {
+      throw new GeneralSecurityException(
+          "Files were unmodified before their initial update. Probably a bug.");
+    }
     final ScheduledFuture<?> future =
         executor.scheduleWithFixedDelay(
-            new LoadFilePathExecution(keyFile, certFile), 0, period, unit);
+            new LoadFilePathExecution(keyFile, certFile), period, period, unit);
     return new Closeable() {
       @Override public void close() {
         future.cancel(false);
@@ -170,8 +174,7 @@ public void run() {
           this.currentKeyTime = newResult.keyTime;
           this.currentCertTime = newResult.certTime;
         }
-      } catch (CertificateException | IOException | NoSuchAlgorithmException
-          | InvalidKeySpecException e) {
+      } catch (IOException | GeneralSecurityException e) {
         log.log(Level.SEVERE, "Failed refreshing private key and certificate chain from files. "
             + "Using previous ones", e);
       }
@@ -201,7 +204,7 @@ public UpdateResult(boolean success, long keyTime, long certTime) {
    * @return the result of this update execution
    */
   private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long oldCertTime)
-      throws IOException, CertificateException, NoSuchAlgorithmException, InvalidKeySpecException {
+      throws IOException, GeneralSecurityException {
     long newKeyTime = keyFile.lastModified();
     long newCertTime = certFile.lastModified();
     // We only update when both the key and the certs are updated.

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

@@ -21,6 +21,7 @@
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.net.Socket;
+import java.security.GeneralSecurityException;
 import java.security.KeyStore;
 import java.security.KeyStoreException;
 import java.security.NoSuchAlgorithmException;
@@ -124,8 +125,8 @@ public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreEx
    *
    * @param trustCerts the trust certificates that are going to be used
    */
-  public void updateTrustCredentials(X509Certificate[] trustCerts) throws CertificateException,
-      KeyStoreException, NoSuchAlgorithmException, IOException {
+  public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOException,
+      GeneralSecurityException {
     KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
     keyStore.load(null, null);
     int i = 1;
@@ -219,10 +220,15 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
    * @return an object that caller should close when the file refreshes are not needed
    */
   public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
-      ScheduledExecutorService executor) {
+      ScheduledExecutorService executor) throws IOException, GeneralSecurityException {
+    long updatedTime = readAndUpdate(trustCertFile, 0);
+    if (updatedTime == 0) {
+      throw new GeneralSecurityException(
+          "Files were unmodified before their initial update. Probably a bug.");
+    }
     final ScheduledFuture<?> future =
         executor.scheduleWithFixedDelay(
-            new LoadFilePathExecution(trustCertFile), 0, period, unit);
+            new LoadFilePathExecution(trustCertFile), period, period, unit);
     return new Closeable() {
       @Override public void close() {
         future.cancel(false);
@@ -243,8 +249,7 @@ public LoadFilePathExecution(File file) {
     public void run() {
       try {
         this.currentTime = readAndUpdate(this.file, this.currentTime);
-      } catch (CertificateException | IOException | KeyStoreException
-          | NoSuchAlgorithmException e) {
+      } catch (IOException | GeneralSecurityException e) {
         log.log(Level.SEVERE, "Failed refreshing trust CAs from file. Using previous CAs", e);
       }
     }
@@ -259,7 +264,7 @@ public void run() {
    * @return oldTime if failed or the modified time is not changed, otherwise the new modified time
    */
   private long readAndUpdate(File trustCertFile, long oldTime)
-      throws CertificateException, IOException, KeyStoreException, NoSuchAlgorithmException {
+      throws IOException, GeneralSecurityException {
     long newTime = trustCertFile.lastModified();
     if (newTime == oldTime) {
       return oldTime;

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

@@ -45,6 +45,7 @@
 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;
@@ -169,7 +170,6 @@ public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception {
         .clientAuth(ClientAuth.REQUIRE).build();
     server = Grpc.newServerBuilderForPort(0, serverCredentials).addService(
         new SimpleServiceImpl()).build().start();
-    TimeUnit.SECONDS.sleep(5);
     // Create a client with the key manager and trust manager.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
     clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
@@ -232,7 +232,6 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy
         .clientAuth(ClientAuth.REQUIRE).build();
     server = Grpc.newServerBuilderForPort(0, serverCredentials).addService(
         new SimpleServiceImpl()).build().start();
-    TimeUnit.SECONDS.sleep(5);
 
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
     clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
@@ -307,7 +306,6 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy
         .clientAuth(ClientAuth.REQUIRE).build();
     server = Grpc.newServerBuilderForPort(0, serverCredentials).addService(
         new SimpleServiceImpl()).build().start();
-    TimeUnit.SECONDS.sleep(5);
 
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
     clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0);
@@ -359,7 +357,6 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
         .clientAuth(ClientAuth.REQUIRE).build();
     server = Grpc.newServerBuilderForPort(0, serverCredentials).addService(
         new SimpleServiceImpl()).build().start();
-    TimeUnit.SECONDS.sleep(5);
     // Create a client to connect.
     AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager();
     Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File,
@@ -391,6 +388,28 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
     clientTrustShutdown.close();
   }
 
+  @Test
+  public void onFileReloadingKeyManagerBadInitialContentTest() throws Exception {
+    exceptionRule.expect(GeneralSecurityException.class);
+    AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager();
+    // We swap the order of key and certificates to intentionally create an exception.
+    Closeable keyShutdown = keyManager.updateIdentityCredentialsFromFile(serverCert0File,
+        serverKey0File, 100, TimeUnit.MILLISECONDS, executor);
+    keyShutdown.close();
+  }
+
+  @Test
+  public void onFileReloadingTrustManagerBadInitialContentTest() throws Exception {
+    exceptionRule.expect(GeneralSecurityException.class);
+    AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder()
+        .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
+        .build();
+    // We pass in a key as the trust certificates to intentionally create an exception.
+    Closeable trustShutdown = trustManager.updateTrustCredentialsFromFile(serverKey0File,
+        100, TimeUnit.MILLISECONDS, executor);
+    trustShutdown.close();
+  }
+
   @Test
   public void keyManagerAliasesTest() throws Exception {
     AdvancedTlsX509KeyManager km = new AdvancedTlsX509KeyManager();