Address PR comments

Author: andyrzhao

oauth2_http/java/com/google/auth/mtls/DefaultMtlsProviderFactory.java

@@ -46,20 +46,15 @@ public class DefaultMtlsProviderFactory {
    * @throws IOException if an I/O error occurs during provider creation.
    */
   public static MtlsProvider create() throws IOException {
-    MtlsProvider mtlsProvider;
-    try {
-      mtlsProvider = new X509Provider();
-      mtlsProvider.getKeyStore();
+    MtlsProvider mtlsProvider = new X509Provider();
+    if (mtlsProvider.isCertificateSourceAvailable()) {
       return mtlsProvider;
-    } catch (CertificateSourceUnavailableException e) {
-      try {
-        mtlsProvider = new SecureConnectProvider();
-        mtlsProvider.getKeyStore();
-        return mtlsProvider;
-      } catch (CertificateSourceUnavailableException ex) {
-        throw new CertificateSourceUnavailableException(
-            "No MtlsSource is available on this device.");
-      }
     }
+    mtlsProvider = new SecureConnectProvider();
+    if (mtlsProvider.isCertificateSourceAvailable()) {
+      return mtlsProvider;
+    }
+    throw new CertificateSourceUnavailableException(
+        "No Certificate Source is available on this device.");
   }
 }

oauth2_http/java/com/google/auth/mtls/MtlsProvider.java

@@ -43,4 +43,7 @@
 public interface MtlsProvider {
   /** Returns the mutual TLS key store. */
   KeyStore getKeyStore() throws IOException;
+
+  /** Returns true if the underlying certificate source is available. */
+  boolean isCertificateSourceAvailable() throws IOException;
 }

oauth2_http/java/com/google/auth/mtls/SecureConnectProvider.java

@@ -68,7 +68,7 @@ static class DefaultProcessProvider implements ProcessProvider {
     @Override
     public Process createProcess(InputStream metadata) throws IOException {
       if (metadata == null) {
-        return null;
+        throw new IOException("Error creating Process: metadata is null");
       }
       List<String> command = extractCertificateProviderCommand(metadata);
       return new ProcessBuilder(command).start();
@@ -108,12 +108,24 @@ public KeyStore getKeyStore() throws IOException {
     }
   }
 
+  @Override
+  public boolean isCertificateSourceAvailable() throws IOException {
+    try {
+      this.getKeyStore();
+    } catch (CertificateSourceUnavailableException e) {
+      return false;
+    }
+    return true;
+  }
+
   @VisibleForTesting
   static KeyStore getKeyStore(InputStream metadata, ProcessProvider processProvider)
       throws IOException, InterruptedException, GeneralSecurityException {
     Process process = processProvider.createProcess(metadata);
 
     // Run the command and timeout after 1000 milliseconds.
+    // The cert provider command usually finishes instantly (if it doesn't hang),
+    // so 1000 milliseconds is plenty of time.
     int exitCode = runCertificateProviderCommand(process, 1000);
     if (exitCode != 0) {
       throw new IOException("Cert provider command failed with exit code: " + exitCode);
@@ -134,28 +146,11 @@ static ImmutableList<String> extractCertificateProviderCommand(InputStream conte
   @VisibleForTesting
   static int runCertificateProviderCommand(Process commandProcess, long timeoutMilliseconds)
       throws IOException, InterruptedException {
-    long startTime = System.currentTimeMillis();
-    long remainTime = timeoutMilliseconds;
-
-    // In the while loop, keep checking if the process is terminated every 100 milliseconds
-    // until timeout is reached or process is terminated. In getKeyStore we set timeout to
-    // 1000 milliseconds, so 100 millisecond is a good number for the sleep.
-    while (remainTime > 0) {
-      Thread.sleep(Math.min(remainTime + 1, 100));
-      remainTime -= System.currentTimeMillis() - startTime;
-
-      try {
-        return commandProcess.exitValue();
-      } catch (IllegalThreadStateException ignored) {
-        // exitValue throws IllegalThreadStateException if process has not yet terminated.
-        // Once the process is terminated, exitValue no longer throws exception. Therefore
-        // in the while loop, we use exitValue to check if process is terminated. See
-        // https://docs.oracle.com/javase/7/docs/api/java/lang/Process.html#exitValue()
-        // for more details.
-      }
+    boolean terminated = commandProcess.waitFor(timeoutMilliseconds, TimeUnit.MILLISECONDS);
+    if (!terminated) {
+      commandProcess.destroy();
+      throw new IOException("Cert provider command timed out");
     }
-
-    commandProcess.destroy();
-    throw new IOException("cert provider command timed out");
+    return commandProcess.exitValue();
   }
 }

oauth2_http/java/com/google/auth/mtls/X509Provider.java

@@ -93,18 +93,12 @@ public KeyStore getKeyStore() throws IOException {
 
     WorkloadCertificateConfiguration workloadCertConfig = getWorkloadCertificateConfiguration();
 
-    InputStream certStream = null;
-    InputStream privateKeyStream = null;
-    SequenceInputStream certAndPrivateKeyStream = null;
-    try {
-      // Read the certificate and private key file paths into separate streams.
-      File certFile = new File(workloadCertConfig.getCertPath());
-      File privateKeyFile = new File(workloadCertConfig.getPrivateKeyPath());
-      certStream = createInputStream(certFile);
-      privateKeyStream = createInputStream(privateKeyFile);
-
-      // Merge the two streams into a single stream.
-      certAndPrivateKeyStream = new SequenceInputStream(certStream, privateKeyStream);
+    // Read the certificate and private key file paths into streams.
+    try (InputStream certStream = createInputStream(new File(workloadCertConfig.getCertPath()));
+        InputStream privateKeyStream =
+            createInputStream(new File(workloadCertConfig.getPrivateKeyPath()));
+        SequenceInputStream certAndPrivateKeyStream =
+            new SequenceInputStream(certStream, privateKeyStream)) {
 
       // Build a key store using the combined stream.
       return SecurityUtils.createMtlsKeyStore(certAndPrivateKeyStream);
@@ -114,19 +108,19 @@ public KeyStore getKeyStore() throws IOException {
     } catch (Exception e) {
       // Wrap all other exception types to an IOException.
       throw new IOException(e);
-    } finally {
-      if (certStream != null) {
-        certStream.close();
-      }
-      if (privateKeyStream != null) {
-        privateKeyStream.close();
-      }
-      if (certAndPrivateKeyStream != null) {
-        certAndPrivateKeyStream.close();
-      }
     }
   }
 
+  @Override
+  public boolean isCertificateSourceAvailable() throws IOException {
+    try {
+      this.getKeyStore();
+    } catch (CertificateSourceUnavailableException e) {
+      return false;
+    }
+    return true;
+  }
+
   private WorkloadCertificateConfiguration getWorkloadCertificateConfiguration()
       throws IOException {
     File certConfig;