diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index 21997cffd4f..c16f15b4894 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -54,12 +54,21 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager private final SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier; // The trust certs we will use to verify peer certificate chain. private volatile X509Certificate[] trustCerts; + // The key store that is used to hold the trust certificates. It will always keep synced with + // trustCerts. + private volatile KeyStore ks; private AdvancedTlsX509TrustManager(Verification verification, PeerVerifier peerVerifier, - SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) { + SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) throws CertificateException { this.verification = verification; this.peerVerifier = peerVerifier; this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier; + try { + ks = KeyStore.getInstance(KeyStore.getDefaultType()); + ks.load(null, null); + } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException e) { + throw new CertificateException("Failed to create KeyStore", e); + } } @Override @@ -108,18 +117,41 @@ public X509Certificate[] getAcceptedIssuers() { // updateTrustCredentialsFromFile()) should not be called. public void useSystemDefaultTrustCerts() { this.trustCerts = new X509Certificate[0]; + this.ks = null; } /** - * Updates the current cached trust certificates. + * Updates the current cached trust certificates as well as the key store. * * @param trustCerts the trust certificates that are going to be used */ - 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? + public void updateTrustCredentials(X509Certificate[] trustCerts) throws KeyStoreException, + CertificateException { this.trustCerts = Arrays.copyOf(trustCerts, trustCerts.length); + // Clear the key store by re-creating it. + try { + ks = KeyStore.getInstance(KeyStore.getDefaultType()); + ks.load(null, null); + } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException e) { + throw new CertificateException("Failed to create KeyStore", e); + } + // Update the ks with the new credential contents. + int i = 1; + // Question: I know we've already made this.trustCerts volatile, but shouldn't we require a + // lock around it as well? Otherwise, while we are reading here, the updating thread might + // change it as well? + // I could be wrong, but my current understanding about "volatile" is that it guarantees we + // store value in memory instead of cache, so there wouldn't be problems like dirty read, + // etc, but it doesn't eliminate race conditions. + for (X509Certificate cert: this.trustCerts) { + String alias = Integer.toString(i); + try { + ks.setCertificateEntry(alias, cert); + } catch (KeyStoreException e) { + throw new CertificateException("Failed to load trust certificate into KeyStore", e); + } + i++; + } } private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine sslEngine, @@ -130,30 +162,6 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss throw new CertificateException( "Want certificate verification but got null or empty certificates"); } - // Set the cached trust certificates into a key store. - KeyStore ks; - try { - ks = KeyStore.getInstance(KeyStore.getDefaultType()); - ks.load(null, null); - } catch (KeyStoreException | IOException | NoSuchAlgorithmException e) { - throw new CertificateException("Failed to create KeyStore", e); - } - int i = 1; - // Question: I know we've already made this.trustCerts volatile, but shouldn't we require a - // lock around it as well? Otherwise, while we are reading here, the updating thread might - // change it as well? - // I could be wrong, but my current understanding about "volatile" is that it guarantees we - // store value in memory instead of cache, so there wouldn't be problems like dirty read, - // etc, but it doesn't eliminate race conditions. - for (X509Certificate cert: this.trustCerts) { - String alias = Integer.toString(i); - try { - ks.setCertificateEntry(alias, cert); - } catch (KeyStoreException e) { - throw new CertificateException("Failed to load trust certificate into KeyStore", e); - } - i++; - } // Use the key store to create a delegated trust manager. X509ExtendedTrustManager delegateManager = null; TrustManagerFactory tmf; @@ -166,7 +174,7 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss TrustManager[] tms = tmf.getTrustManagers(); // Iterate over the returned trust managers, looking for an instance of X509TrustManager. // If found, use that as the delegate trust manager. - for (i = 0; i < tms.length; i++) { + for (int i = 0; i < tms.length; i++) { if (tms[i] instanceof X509ExtendedTrustManager) { delegateManager = (X509ExtendedTrustManager) tms[i]; break; @@ -180,7 +188,8 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss if (checkingServer) { if (this.verification == Verification.CertificateAndHostNameVerification || this.verification == Verification.CertificateOnlyVerification) { - if (sslEngine == null) { + if (this.verification == Verification.CertificateAndHostNameVerification + && sslEngine == null) { throw new CertificateException( "SSLEngine is null. Couldn't check host name"); } @@ -196,10 +205,12 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss } } // Perform the additional peer cert check. - if (sslEngine != null && socket != null && socketAndEnginePeerVerifier != null) { - socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, socket); + if (sslEngine != null && socketAndEnginePeerVerifier != null) { socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine); } + if (socket != null && socketAndEnginePeerVerifier != null) { + socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, socket); + } if (peerVerifier != null) { peerVerifier.verifyPeerCertificate(chain, authType); } @@ -246,7 +257,7 @@ public void run() { if (newUpdateTime != 0) { this.currentTime = newUpdateTime; } - } catch (CertificateException | IOException e) { + } catch (CertificateException | IOException | KeyStoreException e) { log.log(Level.SEVERE, "readAndUpdate() execution thread failed", e); } @@ -262,7 +273,7 @@ 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 CertificateException, IOException { + throws CertificateException, IOException, KeyStoreException { long newTime = trustCertFile.lastModified(); if (newTime != oldTime) { FileInputStream inputStream = new FileInputStream(trustCertFile); @@ -376,7 +387,7 @@ public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier // Question: shall we do some sanity checks here, to make sure all three verifiers are set, to // be more secure? - public AdvancedTlsX509TrustManager build() { + public AdvancedTlsX509TrustManager build() throws CertificateException { return new AdvancedTlsX509TrustManager(this.verification, this.peerVerifier, 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 c134cde6ffe..739065303e2 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -16,6 +16,8 @@ package io.grpc.netty; +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; import com.google.common.util.concurrent.MoreExecutors; @@ -55,7 +57,9 @@ import javax.net.ssl.SSLEngine; import org.junit.After; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.ExpectedException; public class AdvancedTlsTest { public static final String SERVER_0_KEY_FILE = "server0.key"; @@ -79,6 +83,9 @@ public class AdvancedTlsTest { private PrivateKey clientKey0; private X509Certificate[] clientCert0; + @Rule + public ExpectedException exceptionRule = ExpectedException.none(); + @Before public void setUp() throws NoSuchAlgorithmException, IOException, CertificateException, InvalidKeySpecException { @@ -342,6 +349,106 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { clientTrustShutdown.close(); } + @Test + public void keyManagerAliasesTest() throws Exception { + AdvancedTlsX509KeyManager km = new AdvancedTlsX509KeyManager(); + assertArrayEquals( + new String[] {"default"}, km.getClientAliases("", null)); + assertEquals( + "default", km.chooseClientAlias(new String[] {"default"}, null, null)); + assertArrayEquals( + new String[] {"default"}, km.getServerAliases("", null)); + assertEquals( + "default", km.chooseServerAlias("default", null, null)); + } + + @Test + public void trustManagerCheckTrustTest() throws Exception { + AdvancedTlsX509TrustManager tm = AdvancedTlsX509TrustManager.newBuilder() + .setVerification(Verification.InsecurelySkipAllVerification) + .setSslSocketAndEnginePeerVerifier( + new SslSocketAndEnginePeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + Socket socket) throws CertificateException { } + + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + SSLEngine engine) throws CertificateException { } + }) + .setPeerVerifier(new PeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) + throws CertificateException { } + }) + .build(); + tm.updateTrustCredentials(caCert); + tm.checkClientTrusted(serverCert0, "RSA", new Socket()); + tm.checkClientTrusted(serverCert0, "RSA"); + tm.useSystemDefaultTrustCerts(); + tm.checkServerTrusted(clientCert0, "RSA", new Socket()); + tm.checkServerTrusted(clientCert0, "RSA"); + } + + @Test + public void trustManagerEmptyChainTest() throws Exception { + exceptionRule.expect(CertificateException.class); + exceptionRule.expectMessage( + "Want certificate verification but got null or empty certificates"); + AdvancedTlsX509TrustManager tm = AdvancedTlsX509TrustManager.newBuilder() + .setVerification(Verification.CertificateOnlyVerification) + .setSslSocketAndEnginePeerVerifier( + new SslSocketAndEnginePeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + Socket socket) throws CertificateException { } + + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + SSLEngine engine) throws CertificateException { } + }) + .setPeerVerifier(new PeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) + throws CertificateException { } + }) + .build(); + tm.updateTrustCredentials(caCert); + tm.checkClientTrusted(null, "RSA"); + } + + @Test + public void trustManagerBadCustomVerificationTest() throws Exception { + exceptionRule.expect(CertificateException.class); + exceptionRule.expectMessage("Bad Custom Verification"); + AdvancedTlsX509TrustManager tm = AdvancedTlsX509TrustManager.newBuilder() + .setVerification(Verification.CertificateOnlyVerification) + .setSslSocketAndEnginePeerVerifier( + new SslSocketAndEnginePeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + Socket socket) throws CertificateException { + throw new CertificateException("Bad Custom Verification"); + } + + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, + SSLEngine engine) throws CertificateException { + throw new CertificateException("Bad Custom Verification"); + } + }) + .setPeerVerifier(new PeerVerifier() { + @Override + public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType) + throws CertificateException { + throw new CertificateException("Bad Custom Verification"); + } + }) + .build(); + tm.updateTrustCredentials(caCert); + tm.checkClientTrusted(serverCert0, "RSA", new Socket()); + } + private static class SimpleServiceImpl extends SimpleServiceGrpc.SimpleServiceImplBase { @Override public void unaryRpc(SimpleRequest req, StreamObserver respOb) {