diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index adaa1e6e69..8541c6b528 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -23,12 +23,11 @@ import java.io.File; 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 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 */ - 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 final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { * @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 final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { 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 final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { * @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. diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index f6e366d321..ad69fe4abf 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -21,6 +21,7 @@ import java.io.File; 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 final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager * * @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 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager * @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 final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager 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 final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager * @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; diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java index 7dd5ec75e5..9e0d8170c4 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -45,6 +45,7 @@ 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; @@ -169,7 +170,6 @@ public class AdvancedTlsTest { .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 class AdvancedTlsTest { .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 class AdvancedTlsTest { .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 class AdvancedTlsTest { .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 class AdvancedTlsTest { 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();