diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index 9b971033870..facbe13e514 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -27,6 +27,7 @@ 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; import java.util.concurrent.TimeUnit; @@ -35,11 +36,11 @@ import javax.net.ssl.SSLEngine; import javax.net.ssl.X509ExtendedKeyManager; -@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024") /** * AdvancedTlsX509KeyManager is an {@code X509ExtendedKeyManager} that allows users to configure * advanced TLS features, such as private key and certificate chain reloading, etc. */ +@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()); @@ -57,12 +58,12 @@ public PrivateKey getPrivateKey(String alias) { @Override public X509Certificate[] getCertificateChain(String alias) { - return this.certs; + return Arrays.copyOf(this.certs, this.certs.length); } @Override public String[] getClientAliases(String keyType, Principal[] issuers) { - return new String[0]; + return new String[] {"default"}; } @Override @@ -77,7 +78,7 @@ public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSL @Override public String[] getServerAliases(String keyType, Principal[] issuers) { - return new String[0]; + return new String[] {"default"}; } @Override @@ -93,38 +94,36 @@ public String chooseEngineServerAlias(String keyType, Principal[] issuers, /** * Updates the current cached private key and cert chains. - * Right now, callers should make sure |key| matches the public key on the leaf certificate of - * |certs|. - * TODO(ZhenLian): explore possibilities to do a crypto check here. * * @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) { // Same question as the trust manager: what to do if copy is not feasible here? + // Right now, callers should make sure {@code key} matches the public key on the leaf + // certificate of {@code certs}. + // TODO(ZhenLian): explore possibilities to do a crypto check here. this.key = key; - this.certs = certs; + this.certs = Arrays.copyOf(certs, certs.length); } /** - * starts 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 + * 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. * * @param keyFile the file on disk holding the private key * @param certFile the file on disk holding the certificate chain - * @param initialDelay the time to delay first read-and-update execution - * @param delay the period between successive read-and-update executions + * @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 start the read-and-update thread - * @return an object that caller should close when the scheduled execution is not needed + * @param executor the execute 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, - long initialDelay, long delay, TimeUnit unit, ScheduledExecutorService executor) { + long period, TimeUnit unit, ScheduledExecutorService executor) { final ScheduledFuture future = executor.scheduleWithFixedDelay( - new LoadFilePathExecution(keyFile, certFile), - initialDelay, delay, unit); + new LoadFilePathExecution(keyFile, certFile), 0, period, unit); return new Closeable() { @Override public void close() { future.cancel(false); @@ -174,8 +173,8 @@ public UpdateResult(boolean success, long keyTime, long certTime) { } /** - * reads the private key and certificates specified in the path locations. Updates |key| and - * |cert| if both of their modified time changed since last read. + * 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 @@ -189,15 +188,21 @@ private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long newCertTime = certFile.lastModified(); // We only update when both the key and the certs are updated. if (newKeyTime != oldKeyTime && newCertTime != oldCertTime) { - PrivateKey key = CertificateUtils.getPrivateKey(new FileInputStream(keyFile)); - X509Certificate[] certs = CertificateUtils.getX509Certificates(new FileInputStream(certFile)); + FileInputStream keyInputStream = new FileInputStream(keyFile); + FileInputStream certInputStream = new FileInputStream(certFile); + PrivateKey key = CertificateUtils.getPrivateKey(keyInputStream); + X509Certificate[] certs = CertificateUtils.getX509Certificates(certInputStream); + keyInputStream.close(); + certInputStream.close(); updateIdentityCredentials(key, certs); return new UpdateResult(true, newKeyTime, newCertTime); } return new UpdateResult(false, -1, -1); } - // Mainly used to avoid throwing IO Exceptions in java.io.Closeable. + /** + * Mainly used to avoid throwing IO Exceptions in java.io.Closeable. + */ public interface Closeable extends java.io.Closeable { @Override public void close(); } diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index 135ca6e6d33..21997cffd4f 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -19,7 +19,6 @@ import io.grpc.ExperimentalApi; import java.io.File; import java.io.FileInputStream; -import java.io.FileNotFoundException; import java.io.IOException; import java.net.Socket; import java.security.KeyStore; @@ -27,6 +26,7 @@ import java.security.NoSuchAlgorithmException; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; +import java.util.Arrays; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -39,29 +39,27 @@ import javax.net.ssl.X509ExtendedTrustManager; import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement; -@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024") /** - * For Android users: this class is only supported in API level 24 and above. * AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure * advanced TLS features, such as root certificate reloading, peer cert custom verification, etc. + * For Android users: this class is only supported in API level 24 and above. */ +@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024") @IgnoreJRERequirement public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager { private static final Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName()); - private Verification verification; - private PeerVerifier peerVerifier; - private SslSocketPeerVerifier sslSocketPeerVerifier; - private SslEnginePeerVerifier sslEnginePeerVerifier; + private final Verification verification; + private final PeerVerifier peerVerifier; + private final SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier; // The trust certs we will use to verify peer certificate chain. private volatile X509Certificate[] trustCerts; private AdvancedTlsX509TrustManager(Verification verification, PeerVerifier peerVerifier, - SslSocketPeerVerifier socketPeerVerifier, SslEnginePeerVerifier enginePeerVerifier) { + SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) { this.verification = verification; this.peerVerifier = peerVerifier; - this.sslSocketPeerVerifier = socketPeerVerifier; - this.sslEnginePeerVerifier = enginePeerVerifier; + this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier; } @Override @@ -121,7 +119,7 @@ public void updateTrustCredentials(X509Certificate[] trustCerts) { // Question: in design, we will do an copy of trustCerts, but that seems not easy, since // X509Certificate is an abstract class without copy constructor. // Can we document in the API that trustCerts shouldn't be modified once set? - this.trustCerts = trustCerts; + this.trustCerts = Arrays.copyOf(trustCerts, trustCerts.length); } private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine sslEngine, @@ -198,37 +196,33 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss } } // Perform the additional peer cert check. - if (sslEngine != null && sslEnginePeerVerifier != null) { - sslEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine); - } - if (socket != null && sslSocketPeerVerifier != null) { - sslSocketPeerVerifier.verifyPeerCertificate(chain, authType, socket); + if (sslEngine != null && socket != null && socketAndEnginePeerVerifier != null) { + socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, socket); + socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine); } if (peerVerifier != null) { peerVerifier.verifyPeerCertificate(chain, authType); } - if (sslEnginePeerVerifier == null && sslSocketPeerVerifier == null && peerVerifier == null) { + if (socketAndEnginePeerVerifier == null && peerVerifier == null) { log.log(Level.INFO, "No peer verifier is specified"); } } /** - * starts a {@code ScheduledExecutorService} to read trust certificates from a local file path + * Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path * periodically, and update the cached trust certs if there is an update. * * @param trustCertFile the file on disk holding the trust certificates - * @param initialDelay the time to delay first read-and-update execution - * @param delay the period between successive read-and-update executions + * @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 start the read-and-update thread - * @return an object that caller should close when the scheduled execution is not needed + * @param executor the execute 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 updateTrustCredentialsFromFile(File trustCertFile, long initialDelay, - long delay, TimeUnit unit, ScheduledExecutorService executor) { + public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit, + ScheduledExecutorService executor) { final ScheduledFuture future = executor.scheduleWithFixedDelay( - new LoadFilePathExecution(trustCertFile), - initialDelay, delay, unit); + new LoadFilePathExecution(trustCertFile), 0, period, unit); return new Closeable() { @Override public void close() { future.cancel(false); @@ -252,7 +246,7 @@ public void run() { if (newUpdateTime != 0) { this.currentTime = newUpdateTime; } - } catch (FileNotFoundException | CertificateException e) { + } catch (CertificateException | IOException e) { log.log(Level.SEVERE, "readAndUpdate() execution thread failed", e); } @@ -268,11 +262,12 @@ public void run() { * @return 0 if failed or the modified time is not changed, otherwise the new modified time */ private long readAndUpdate(File trustCertFile, long oldTime) - throws FileNotFoundException, CertificateException { + throws CertificateException, IOException { long newTime = trustCertFile.lastModified(); if (newTime != oldTime) { - X509Certificate[] certificates = CertificateUtils.getX509Certificates( - new FileInputStream(trustCertFile)); + FileInputStream inputStream = new FileInputStream(trustCertFile); + X509Certificate[] certificates = CertificateUtils.getX509Certificates(inputStream); + inputStream.close(); updateTrustCredentials(certificates); return newTime; } @@ -328,9 +323,9 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) } // Additional custom peer verification check. - // It will be used when checkClientTrusted/checkServerTrusted is called with the {@code Socket} - // parameter. - public interface SslSocketPeerVerifier { + // It will be used when checkClientTrusted/checkServerTrusted is called with the {@code Socket} or + // the {@code SSLEngine} parameter. + public interface SslSocketAndEnginePeerVerifier { /** * Verifies the peer certificate chain. For more information, please refer to * {@code X509ExtendedTrustManager}. @@ -342,12 +337,7 @@ public interface SslSocketPeerVerifier { */ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, Socket socket) throws CertificateException; - } - // Additional custom peer verification check. - // It will be used when checkClientTrusted/checkServerTrusted is called with the {@code SSLEngine} - // parameter. - public interface SslEnginePeerVerifier { /** * Verifies the peer certificate chain. For more information, please refer to * {@code X509ExtendedTrustManager}. @@ -363,28 +353,24 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSL public static final class Builder { - private Verification verification; + private Verification verification = Verification.CertificateAndHostNameVerification; private PeerVerifier peerVerifier; - private SslSocketPeerVerifier socketPeerVerifier; - private SslEnginePeerVerifier enginePeerVerifier; + private SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier; + + private Builder() {} public Builder setVerification(Verification verification) { this.verification = verification; return this; } - public Builder setPeerVerifier(PeerVerifier peerVerifier) { - this.peerVerifier = peerVerifier; - return this; - } - - public Builder setSslSocketPeerVerifier(SslSocketPeerVerifier peerVerifier) { - this.socketPeerVerifier = peerVerifier; + public Builder setPeerVerifier(PeerVerifier verifier) { + this.peerVerifier = verifier; return this; } - public Builder setSslEnginePeerVerifier(SslEnginePeerVerifier peerVerifier) { - this.enginePeerVerifier = peerVerifier; + public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier verifier) { + this.socketAndEnginePeerVerifier = verifier; return this; } @@ -392,7 +378,7 @@ public Builder setSslEnginePeerVerifier(SslEnginePeerVerifier peerVerifier) { // be more secure? public AdvancedTlsX509TrustManager build() { return new AdvancedTlsX509TrustManager(this.verification, this.peerVerifier, - this.socketPeerVerifier, this.enginePeerVerifier); + this.socketAndEnginePeerVerifier); } } } diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java index cc9ebdb7a5d..c134cde6ffe 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -36,8 +36,7 @@ import io.grpc.util.AdvancedTlsX509KeyManager; import io.grpc.util.AdvancedTlsX509TrustManager; import io.grpc.util.AdvancedTlsX509TrustManager.PeerVerifier; -import io.grpc.util.AdvancedTlsX509TrustManager.SslEnginePeerVerifier; -import io.grpc.util.AdvancedTlsX509TrustManager.SslSocketPeerVerifier; +import io.grpc.util.AdvancedTlsX509TrustManager.SslSocketAndEnginePeerVerifier; import io.grpc.util.AdvancedTlsX509TrustManager.Verification; import io.grpc.util.CertificateUtils; @@ -185,8 +184,20 @@ public void trustManagerCustomVerifierMutualTlsTest() throws Exception { serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) - .setSslEnginePeerVerifier( - new SslEnginePeerVerifier() { + .setSslSocketAndEnginePeerVerifier( + new SslSocketAndEnginePeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + Socket socket) throws CertificateException { + if (peerCertChain == null || peerCertChain.length == 0) { + throw new CertificateException("peerCertChain is empty"); + } + X509Certificate leafCert = peerCertChain[0]; + if (!leafCert.getSubjectDN().getName().contains("testclient")) { + throw new CertificateException("SslSocketAndEnginePeerVerifier failed"); + } + } + @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSLEngine engine) throws CertificateException { @@ -195,23 +206,10 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy } X509Certificate leafCert = peerCertChain[0]; if (!leafCert.getSubjectDN().getName().contains("testclient")) { - throw new CertificateException("SSLEnginePeerVerifier failed"); + throw new CertificateException("SslSocketAndEnginePeerVerifier failed"); } } }) - .setSslSocketPeerVerifier(new SslSocketPeerVerifier() { - @Override - public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - Socket socket) throws CertificateException { - if (peerCertChain == null || peerCertChain.length == 0) { - throw new CertificateException("peerCertChain is empty"); - } - X509Certificate leafCert = peerCertChain[0]; - if (!leafCert.getSubjectDN().getName().contains("testclient")) { - throw new CertificateException("SSLSocketPeerVerifier failed"); - } - } - }) .setPeerVerifier(new PeerVerifier() { @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) @@ -238,8 +236,20 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) - .setSslEnginePeerVerifier( - new SslEnginePeerVerifier() { + .setSslSocketAndEnginePeerVerifier( + new SslSocketAndEnginePeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + Socket socket) throws CertificateException { + if (peerCertChain == null || peerCertChain.length == 0) { + throw new CertificateException("peerCertChain is empty"); + } + X509Certificate leafCert = peerCertChain[0]; + if (!leafCert.getSubjectDN().getName().contains("*.test.google.com.au")) { + throw new CertificateException("SslSocketAndEnginePeerVerifier failed"); + } + } + @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSLEngine engine) throws CertificateException { @@ -248,23 +258,10 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy } X509Certificate leafCert = peerCertChain[0]; if (!leafCert.getSubjectDN().getName().contains("*.test.google.com.au")) { - throw new CertificateException("SSLEnginePeerVerifier failed"); + throw new CertificateException("SslSocketAndEnginePeerVerifier failed"); } } }) - .setSslSocketPeerVerifier(new SslSocketPeerVerifier() { - @Override - public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - Socket socket) throws CertificateException { - if (peerCertChain == null || peerCertChain.length == 0) { - throw new CertificateException("peerCertChain is empty"); - } - X509Certificate leafCert = peerCertChain[0]; - if (!leafCert.getSubjectDN().getName().contains("*.test.google.com.au")) { - throw new CertificateException("SSLSocketPeerVerifier failed"); - } - } - }) .setPeerVerifier(new PeerVerifier() { @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) @@ -302,12 +299,12 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, - serverCert0File, 0, 100, TimeUnit.MILLISECONDS, executor); + serverCert0File, 100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) .build(); Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentialsFromFile(caCertFile, - 0, 100, TimeUnit.MILLISECONDS, executor); + 100, TimeUnit.MILLISECONDS, executor); ServerCredentials serverCredentials = TlsServerCredentials.newBuilder() .keyManager(serverKeyManager).trustManager(serverTrustManager) .clientAuth(ClientAuth.REQUIRE).build(); @@ -317,12 +314,12 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File, - clientCert0File, 0, 100, TimeUnit.MILLISECONDS, executor); + clientCert0File, 100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateAndHostNameVerification) .build(); Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentialsFromFile(caCertFile, - 0, 100, TimeUnit.MILLISECONDS, executor); + 100, TimeUnit.MILLISECONDS, executor); ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder() .keyManager(clientKeyManager).trustManager(clientTrustManager).build(); channel = Grpc.newChannelBuilderForAddress("localhost", server.getPort(), channelCredentials)