Skip to content

Commit

Permalink
resolve comments; add more tests
Browse files Browse the repository at this point in the history
  • Loading branch information
@ZhenLian
ZhenLian committed Jun 23, 2021
1 parent 3388548 commit d1b79a9
Show file tree
Hide file tree
Showing 2 changed files with 154 additions and 37 deletions.
85 changes: 48 additions & 37 deletions core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
View file
Expand Up @@ -27,6 +27,7 @@
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.Enumeration;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
Expand Down Expand Up @@ -54,12 +55,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
Expand Down Expand Up @@ -108,18 +118,40 @@ 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 existing contents stored in ks.
Enumeration<String> aliases = ks.aliases();
while (aliases.hasMoreElements()) {
String alias = aliases.nextElement();
ks.deleteEntry(alias);
}
// 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,
Expand All @@ -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;
Expand All @@ -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;
Expand All @@ -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");
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);

}
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
Expand Down
106 changes: 106 additions & 0 deletions netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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";
Expand All @@ -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 {
Expand Down Expand Up @@ -342,6 +349,105 @@ 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.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<SimpleResponse> respOb) {
Expand Down

0 comments on commit d1b79a9

Please sign in to comment.