From 321b2c4948923ac71bdf80e14574c3fc5d961c0c Mon Sep 17 00:00:00 2001 From: ZhenLian Date: Fri, 13 Aug 2021 14:49:50 -0700 Subject: [PATCH] change to use key store only; fix the latest comments --- .../grpc/util/AdvancedTlsX509KeyManager.java | 132 ++++++++++++++---- .../util/AdvancedTlsX509TrustManager.java | 93 ++++++------ .../java/io/grpc/netty/AdvancedTlsTest.java | 12 +- 3 files changed, 167 insertions(+), 70 deletions(-) diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index facbe13e514..0c01b75417b 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -21,13 +21,19 @@ import java.io.FileInputStream; import java.io.IOException; import java.net.Socket; +import java.security.KeyStore; +import java.security.KeyStore.PrivateKeyEntry; +import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; import java.security.Principal; import java.security.PrivateKey; +import java.security.UnrecoverableEntryException; +import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; import java.security.spec.InvalidKeySpecException; -import java.util.Arrays; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -44,21 +50,64 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { private static final Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName()); - // The private key and the cert chain we will use to send to peers to prove our identity. - private volatile PrivateKey key; - private volatile X509Certificate[] certs; + // The key store that is used to hold the private key and the certificate chain. + private volatile KeyStore ks; + // The password associated with the private key. + private volatile String password = null; - public AdvancedTlsX509KeyManager() { } + /** + * Constructs an AdvancedTlsX509KeyManager. + */ + public AdvancedTlsX509KeyManager() throws CertificateException { + 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 public PrivateKey getPrivateKey(String alias) { - // Same question as the trust manager: is it really thread-safe to do this? - return this.key; + KeyStore.Entry entry = null; + try { + entry = ks.getEntry(alias, new KeyStore.PasswordProtection(this.password.toCharArray())); + } catch (NoSuchAlgorithmException | UnrecoverableEntryException | KeyStoreException e) { + log.log(Level.SEVERE, "Unable to get the key entry from the key store", e); + return null; + } + if (entry == null || !(entry instanceof PrivateKeyEntry)) { + log.log(Level.SEVERE, "Failed to get the actual entry"); + return null; + } + PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) entry; + return privateKeyEntry.getPrivateKey(); } @Override public X509Certificate[] getCertificateChain(String alias) { - return Arrays.copyOf(this.certs, this.certs.length); + KeyStore.Entry entry = null; + try { + entry = ks.getEntry(alias, new KeyStore.PasswordProtection(this.password.toCharArray())); + } catch (NoSuchAlgorithmException | UnrecoverableEntryException | KeyStoreException e) { + log.log(Level.SEVERE, "Unable to get the key entry from the key store", e); + return null; + } + if (entry == null || !(entry instanceof PrivateKeyEntry)) { + log.log(Level.SEVERE, "Failed to get the actual entry"); + return null; + } + PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) entry; + Certificate[] certs = privateKeyEntry.getCertificateChain(); + List returnedCerts = new ArrayList<>(); + for (int i = 0; i < certs.length; ++i) { + if (certs[i] instanceof X509Certificate) { + returnedCerts.add((X509Certificate) certs[i]); + } else { + log.log(Level.SEVERE, "The certificate is not type of X509Certificate. Skip it"); + } + } + return returnedCerts.toArray(new X509Certificate[0]); } @Override @@ -97,14 +146,32 @@ public String chooseEngineServerAlias(String keyType, Principal[] issuers, * * @param key the private key that is going to be used * @param certs the certificate chain that is going to be used + * @param password the password associated with the key */ - 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}. + public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs, String password) + throws CertificateException { // TODO(ZhenLian): explore possibilities to do a crypto check here. - this.key = key; - this.certs = Arrays.copyOf(certs, certs.length); + // Clear the key store by re-creating it. + KeyStore newKeyStore = null; + try { + newKeyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + newKeyStore.load(null, null); + } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException e) { + throw new CertificateException("Failed to create KeyStore", e); + } + if (newKeyStore != null) { + this.ks = newKeyStore; + } + this.password = password; + // Update the ks with the new credential contents. + try { + PrivateKeyEntry privateKeyEntry = new PrivateKeyEntry(key, certs); + ks.setEntry("default", privateKeyEntry, new KeyStore.PasswordProtection( + this.password.toCharArray())); + } catch (KeyStoreException e) { + throw new CertificateException( + "Failed to load private key and certificates into KeyStore", e); + } } /** @@ -114,13 +181,15 @@ public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) { * * @param keyFile the file on disk holding the private key * @param certFile the file on disk holding the certificate chain + * @param password the password associated with the key * @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 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 period, TimeUnit unit, ScheduledExecutorService executor) { + String password, long period, TimeUnit unit, ScheduledExecutorService executor) { + this.password = password; final ScheduledFuture future = executor.scheduleWithFixedDelay( new LoadFilePathExecution(keyFile, certFile), 0, period, unit); @@ -155,7 +224,8 @@ public void run() { } } catch (CertificateException | IOException | NoSuchAlgorithmException | InvalidKeySpecException e) { - log.log(Level.SEVERE, "readAndUpdate() execution thread failed", e); + log.log(Level.SEVERE, "Failed refreshing private key and certificate chain from files. " + + "Using previous ones", e); } } } @@ -188,16 +258,28 @@ 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) { - 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); + FileInputStream keyInputStream = null; + try { + keyInputStream = new FileInputStream(keyFile); + PrivateKey key = CertificateUtils.getPrivateKey(keyInputStream); + FileInputStream certInputStream = null; + try { + certInputStream = new FileInputStream(certFile); + X509Certificate[] certs = CertificateUtils.getX509Certificates(certInputStream); + updateIdentityCredentials(key, certs, this.password); + return new UpdateResult(true, newKeyTime, newCertTime); + } finally { + if (certInputStream != null) { + certInputStream.close(); + } + } + } finally { + if (keyInputStream != null) { + keyInputStream.close(); + } + } } - return new UpdateResult(false, -1, -1); + return new UpdateResult(false, oldKeyTime, oldCertTime); } /** diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index c16f15b4894..1a1da50c9d6 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -24,9 +24,11 @@ import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; +import java.security.cert.Certificate; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; -import java.util.Arrays; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -52,14 +54,15 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager 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; + // The aliases we use in the key store to keep track of all the trust certificates. + private volatile List aliases; // The key store that is used to hold the trust certificates. It will always keep synced with // trustCerts. - private volatile KeyStore ks; + private volatile KeyStore ks; private AdvancedTlsX509TrustManager(Verification verification, PeerVerifier peerVerifier, SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) throws CertificateException { + this.aliases = new ArrayList<>(); this.verification = verification; this.peerVerifier = peerVerifier; this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier; @@ -109,42 +112,53 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket @Override public X509Certificate[] getAcceptedIssuers() { - return trustCerts; + List trustCerts = new ArrayList<>(); + for (String alias : this.aliases) { + try { + Certificate cert = this.ks.getCertificate(alias); + if (cert instanceof X509Certificate) { + trustCerts.add((X509Certificate) cert); + } else { + log.log(Level.SEVERE, "The certificate is not type of X509Certificate. Skip it"); + } + } catch (KeyStoreException e) { + log.log(Level.SEVERE, "Failed to get the certificate from the key store", e); + } + } + return trustCerts.toArray(new X509Certificate[0]); + } // If this is set, we will use the default trust certificates stored on user's local system. - // After this is used, functions that will update this.trustCerts(e.g. updateTrustCredentials(), + // After this is used, functions that will update this.ks(e.g. updateTrustCredentials(), // updateTrustCredentialsFromFile()) should not be called. public void useSystemDefaultTrustCerts() { - this.trustCerts = new X509Certificate[0]; this.ks = null; } /** * Updates the current cached trust certificates as well as the key store. * - * @param trustCerts the trust certificates that are going to be used + * @param trustCerts the trust certificates that are going to be used */ - public void updateTrustCredentials(X509Certificate[] trustCerts) throws KeyStoreException, - CertificateException { - this.trustCerts = Arrays.copyOf(trustCerts, trustCerts.length); + public void updateTrustCredentials(X509Certificate[] trustCerts) throws CertificateException { // Clear the key store by re-creating it. + KeyStore newKeyStore = null; try { - ks = KeyStore.getInstance(KeyStore.getDefaultType()); - ks.load(null, null); + newKeyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + newKeyStore.load(null, null); } catch (KeyStoreException | IOException | NoSuchAlgorithmException | CertificateException e) { throw new CertificateException("Failed to create KeyStore", e); } + if (newKeyStore != null) { + this.ks = newKeyStore; + } + this.aliases.clear(); // 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) { + for (X509Certificate cert: trustCerts) { String alias = Integer.toString(i); + this.aliases.add(alias); try { ks.setCertificateEntry(alias, cert); } catch (KeyStoreException e) { @@ -207,14 +221,11 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss // Perform the additional peer cert check. if (sslEngine != null && socketAndEnginePeerVerifier != null) { socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine); - } - if (socket != null && socketAndEnginePeerVerifier != null) { + } else if (socket != null && socketAndEnginePeerVerifier != null) { socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, socket); - } - if (peerVerifier != null) { + } else if (peerVerifier != null) { peerVerifier.verifyPeerCertificate(chain, authType); - } - if (socketAndEnginePeerVerifier == null && peerVerifier == null) { + } else { log.log(Level.INFO, "No peer verifier is specified"); } } @@ -254,35 +265,41 @@ public LoadFilePathExecution(File file) { public void run() { try { long newUpdateTime = readAndUpdate(this.file, this.currentTime); - if (newUpdateTime != 0) { + if (this.currentTime != newUpdateTime) { this.currentTime = newUpdateTime; } } catch (CertificateException | IOException | KeyStoreException e) { - log.log(Level.SEVERE, "readAndUpdate() execution thread failed", e); + log.log(Level.SEVERE, "Failed refreshing trust CAs from file. Using previous CAs", e); } } } /** - * reads the trust certificates specified in the path location, and update |trustCerts| if the + * Reads the trust certificates specified in the path location, and update the key store if the * modified time has changed since last read. * * @param trustCertFile the file on disk holding the trust certificates * @param oldTime the time when the trust file is modified during last execution - * @return 0 if failed or the modified time is not changed, otherwise the new modified time + * @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 { long newTime = trustCertFile.lastModified(); if (newTime != oldTime) { - FileInputStream inputStream = new FileInputStream(trustCertFile); - X509Certificate[] certificates = CertificateUtils.getX509Certificates(inputStream); - inputStream.close(); - updateTrustCredentials(certificates); - return newTime; + FileInputStream inputStream = null; + try { + inputStream = new FileInputStream(trustCertFile); + X509Certificate[] certificates = CertificateUtils.getX509Certificates(inputStream); + updateTrustCredentials(certificates); + return newTime; + } finally { + if (inputStream != null) { + inputStream.close(); + } + } } - return 0; + return oldTime; } // Mainly used to avoid throwing IO Exceptions in java.io.Closeable. @@ -291,7 +308,7 @@ public interface Closeable extends java.io.Closeable { } public static Builder newBuilder() { - return new Builder().setVerification(Verification.CertificateAndHostNameVerification); + return new Builder(); } // The verification mode when authenticating the peer certificate. @@ -385,8 +402,6 @@ public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier return this; } - // Question: shall we do some sanity checks here, to make sure all three verifiers are set, to - // be more secure? 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 739065303e2..7fd52abde14 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -149,7 +149,7 @@ public void basicMutualTlsTest() throws Exception { public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0, ""); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) .build(); @@ -162,7 +162,7 @@ public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception { TimeUnit.SECONDS.sleep(5); // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); + clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0, ""); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateAndHostNameVerification) .build(); @@ -188,7 +188,7 @@ public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception { public void trustManagerCustomVerifierMutualTlsTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0, ""); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) .setSslSocketAndEnginePeerVerifier( @@ -240,7 +240,7 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy TimeUnit.SECONDS.sleep(5); // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); + clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0, ""); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) .setSslSocketAndEnginePeerVerifier( @@ -306,7 +306,7 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, - serverCert0File, 100, TimeUnit.MILLISECONDS, executor); + serverCert0File, "", 100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateOnlyVerification) .build(); @@ -321,7 +321,7 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File, - clientCert0File, 100, TimeUnit.MILLISECONDS, executor); + clientCert0File, "",100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CertificateAndHostNameVerification) .build();