From 8f4a297ead427293a8829336e950c3657e88a4d3 Mon Sep 17 00:00:00 2001 From: ZhenLian Date: Sat, 14 Aug 2021 10:43:40 -0700 Subject: [PATCH] resolve comments based on the discussion offline --- .../grpc/util/AdvancedTlsX509KeyManager.java | 116 +++---------- .../util/AdvancedTlsX509TrustManager.java | 156 +++++++----------- .../java/io/grpc/netty/AdvancedTlsTest.java | 12 +- 3 files changed, 93 insertions(+), 191 deletions(-) diff --git a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index 0c01b75417b4..ec6a5a9beae1 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -21,19 +21,13 @@ 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.ArrayList; -import java.util.List; +import java.util.Arrays; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -50,64 +44,22 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { private static final Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName()); - // 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; + // The credential information sent to peers to prove our identity. + private volatile KeyInfo keyInfo; /** * 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); - } - } + public AdvancedTlsX509KeyManager() throws CertificateException { } @Override public PrivateKey getPrivateKey(String alias) { - 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(); + return this.keyInfo.key; } @Override public X509Certificate[] getCertificateChain(String alias) { - 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]); + return Arrays.copyOf(this.keyInfo.certs, this.keyInfo.certs.length); } @Override @@ -146,32 +98,11 @@ 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, String password) + public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) throws CertificateException { // TODO(ZhenLian): explore possibilities to do a crypto check here. - // 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); - } + this.keyInfo = new KeyInfo(key, certs); } /** @@ -181,15 +112,13 @@ public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs, S * * @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, - String password, long period, TimeUnit unit, ScheduledExecutorService executor) { - this.password = password; + long period, TimeUnit unit, ScheduledExecutorService executor) { final ScheduledFuture future = executor.scheduleWithFixedDelay( new LoadFilePathExecution(keyFile, certFile), 0, period, unit); @@ -200,6 +129,17 @@ public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile, }; } + private static class KeyInfo { + // The private key and the cert chain we will use to send to peers to prove our identity. + final PrivateKey key; + final X509Certificate[] certs; + + public KeyInfo(PrivateKey key, X509Certificate[] certs) { + this.key = key; + this.certs = certs; + } + } + private class LoadFilePathExecution implements Runnable { File keyFile; File certFile; @@ -258,25 +198,19 @@ 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 = null; + FileInputStream keyInputStream = new FileInputStream(keyFile); try { - keyInputStream = new FileInputStream(keyFile); PrivateKey key = CertificateUtils.getPrivateKey(keyInputStream); - FileInputStream certInputStream = null; + FileInputStream certInputStream = new FileInputStream(certFile); try { - certInputStream = new FileInputStream(certFile); X509Certificate[] certs = CertificateUtils.getX509Certificates(certInputStream); - updateIdentityCredentials(key, certs, this.password); + updateIdentityCredentials(key, certs); return new UpdateResult(true, newKeyTime, newCertTime); } finally { - if (certInputStream != null) { - certInputStream.close(); - } + certInputStream.close(); } } finally { - if (keyInputStream != null) { - keyInputStream.close(); - } + keyInputStream.close(); } } 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 1a1da50c9d6d..136b02bb766d 100644 --- a/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -24,11 +24,8 @@ 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.ArrayList; -import java.util.List; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -54,24 +51,15 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager private final Verification verification; private final PeerVerifier peerVerifier; private final SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier; - // 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; + + // The delegated trust manager used to perform traditional certificate verification. + private volatile X509ExtendedTrustManager delegateManager = null; private AdvancedTlsX509TrustManager(Verification verification, PeerVerifier peerVerifier, SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) throws CertificateException { - this.aliases = new ArrayList<>(); 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 @@ -112,28 +100,24 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket @Override public X509Certificate[] getAcceptedIssuers() { - 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); - } + if (this.delegateManager == null) { + log.log(Level.SEVERE, "The certificate is not type of X509Certificate. Skip it"); + return new X509Certificate[0]; } - return trustCerts.toArray(new X509Certificate[0]); - + return this.delegateManager.getAcceptedIssuers(); } - // 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.ks(e.g. updateTrustCredentials(), - // updateTrustCredentialsFromFile()) should not be called. - public void useSystemDefaultTrustCerts() { - this.ks = null; + /** + * Uses the default trust certificates stored on user's local system. + * After this is used, functions that will provide new credential + * data(e.g. updateTrustCredentials(), updateTrustCredentialsFromFile()) should not be called. + */ + public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreException, + NoSuchAlgorithmException { + // Passing a null value of KeyStore would make {@code TrustManagerFactory} attempt to use + // system-default trust CA certs. + X509ExtendedTrustManager newDelegateManager = createDelegateTrustManager(null); + this.delegateManager = newDelegateManager; } /** @@ -141,31 +125,40 @@ public void useSystemDefaultTrustCerts() { * * @param trustCerts the trust certificates that are going to be used */ - public void updateTrustCredentials(X509Certificate[] trustCerts) throws CertificateException { - // 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.aliases.clear(); - // Update the ks with the new credential contents. + public void updateTrustCredentials(X509Certificate[] trustCerts) throws CertificateException, + KeyStoreException, NoSuchAlgorithmException, IOException { + KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); + keyStore.load(null, null); int i = 1; for (X509Certificate cert: trustCerts) { String alias = Integer.toString(i); - this.aliases.add(alias); - try { - ks.setCertificateEntry(alias, cert); - } catch (KeyStoreException e) { - throw new CertificateException("Failed to load trust certificate into KeyStore", e); - } + keyStore.setCertificateEntry(alias, cert); i++; } + X509ExtendedTrustManager newDelegateManager = createDelegateTrustManager(keyStore); + this.delegateManager = newDelegateManager; + } + + private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyStore) + throws CertificateException, KeyStoreException, NoSuchAlgorithmException { + TrustManagerFactory tmf = TrustManagerFactory.getInstance( + TrustManagerFactory.getDefaultAlgorithm()); + tmf.init(keyStore); + X509ExtendedTrustManager delegateManager = null; + 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 (int j = 0; j < tms.length; j++) { + if (tms[j] instanceof X509ExtendedTrustManager) { + delegateManager = (X509ExtendedTrustManager) tms[j]; + break; + } + } + if (delegateManager == null) { + throw new CertificateException( + "Instance delegateX509TrustManager is null. Failed to initialize"); + } + return delegateManager; } private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine sslEngine, @@ -176,43 +169,21 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss throw new CertificateException( "Want certificate verification but got null or empty certificates"); } - // Use the key store to create a delegated trust manager. - X509ExtendedTrustManager delegateManager = null; - TrustManagerFactory tmf; - try { - tmf = TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); - tmf.init(ks); - } catch (KeyStoreException | NoSuchAlgorithmException e) { - throw new CertificateException("Failed to create delegate TrustManagerFactory", e); - } - 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 (int i = 0; i < tms.length; i++) { - if (tms[i] instanceof X509ExtendedTrustManager) { - delegateManager = (X509ExtendedTrustManager) tms[i]; - break; - } - } - if (delegateManager == null) { + if (this.delegateManager == null) { throw new CertificateException( - "Instance delegateX509TrustManager is null. Failed to initialize"); + "Credential hasn't been provided yet. Failed to build the trust manager"); } - // Configure the delegated trust manager based on users' input. if (checkingServer) { if (this.verification == Verification.CertificateAndHostNameVerification - || this.verification == Verification.CertificateOnlyVerification) { - if (this.verification == Verification.CertificateAndHostNameVerification - && sslEngine == null) { - throw new CertificateException( - "SSLEngine is null. Couldn't check host name"); - } - String algorithm = this.verification == Verification.CertificateAndHostNameVerification - ? "HTTPS" : ""; - SSLParameters sslParams = sslEngine.getSSLParameters(); - sslParams.setEndpointIdentificationAlgorithm(algorithm); - sslEngine.setSSLParameters(sslParams); + && sslEngine == null) { + throw new CertificateException( + "SSLEngine is null. Couldn't check host name"); } + String algorithm = this.verification == Verification.CertificateAndHostNameVerification + ? "HTTPS" : ""; + SSLParameters sslParams = sslEngine.getSSLParameters(); + sslParams.setEndpointIdentificationAlgorithm(algorithm); + sslEngine.setSSLParameters(sslParams); delegateManager.checkServerTrusted(chain, authType, sslEngine); } else { delegateManager.checkClientTrusted(chain, authType, sslEngine); @@ -268,9 +239,9 @@ public void run() { if (this.currentTime != newUpdateTime) { this.currentTime = newUpdateTime; } - } catch (CertificateException | IOException | KeyStoreException e) { + } catch (CertificateException | IOException | KeyStoreException + | NoSuchAlgorithmException e) { log.log(Level.SEVERE, "Failed refreshing trust CAs from file. Using previous CAs", e); - } } } @@ -284,19 +255,16 @@ public void run() { * @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 { + throws CertificateException, IOException, KeyStoreException, NoSuchAlgorithmException { long newTime = trustCertFile.lastModified(); if (newTime != oldTime) { - FileInputStream inputStream = null; + FileInputStream inputStream = new FileInputStream(trustCertFile); try { - inputStream = new FileInputStream(trustCertFile); X509Certificate[] certificates = CertificateUtils.getX509Certificates(inputStream); updateTrustCredentials(certificates); return newTime; } finally { - if (inputStream != null) { - inputStream.close(); - } + inputStream.close(); } } 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 7fd52abde149..3007e4f7640a 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();