Skip to content

Commit

Permalink
fix the latest comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ZhenLian
ZhenLian committed Aug 13, 2021
1 parent b196a12 commit 6930cac
Show file tree
Hide file tree
Showing 3 changed files with 166 additions and 70 deletions.
132 changes: 107 additions & 25 deletions core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java
View file
Expand Up @@ -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;
Expand All @@ -44,21 +50,65 @@
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. It will always
// keep synced with key and certs.
private volatile KeyStore ks;
// The password associated with the 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 private key entry from the key store");
return null;
}
if (entry == null || !(entry instanceof PrivateKeyEntry)) {
log.log(Level.SEVERE, "Failed to get the actual entry. Cannot get the private key");
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 private key entry from the key store");
return null;
}
if (entry == null || !(entry instanceof PrivateKeyEntry)) {
log.log(Level.SEVERE, "Failed to get the actual entry. Cannot get the private key");
return null;
}
PrivateKeyEntry privateKeyEntry = (PrivateKeyEntry) entry;
Certificate[] certs = privateKeyEntry.getCertificateChain();
List<X509Certificate> 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
Expand Down Expand Up @@ -97,14 +147,31 @@ 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 trust certificate into KeyStore", e);
}
}

/**
Expand All @@ -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);
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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);
}

/**
Expand Down
92 changes: 53 additions & 39 deletions core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
View file
Expand Up @@ -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;
Expand All @@ -52,11 +54,11 @@ 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<String> aliases = new ArrayList<>();
// 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 {
Expand Down Expand Up @@ -109,42 +111,53 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, Socket

@Override
public X509Certificate[] getAcceptedIssuers() {
return trustCerts;
List<X509Certificate> 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) {
Expand Down Expand Up @@ -207,14 +220,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");
}
}
Expand Down Expand Up @@ -254,35 +264,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.
Expand All @@ -291,7 +307,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.
Expand Down Expand Up @@ -385,8 +401,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);
Expand Down

0 comments on commit 6930cac

Please sign in to comment.