Skip to content

Commit

Permalink
resolve some of the comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@ZhenLian
ZhenLian committed May 29, 2021
1 parent e17a2e5 commit 3388548
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 111 deletions.
49 changes: 27 additions & 22 deletions core/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java
View file
Expand Up @@ -27,6 +27,7 @@
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.security.spec.InvalidKeySpecException;
import java.util.Arrays;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
Expand All @@ -35,11 +36,11 @@
import javax.net.ssl.SSLEngine;
import javax.net.ssl.X509ExtendedKeyManager;

@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
/**
* AdvancedTlsX509KeyManager is an {@code X509ExtendedKeyManager} that allows users to configure
* advanced TLS features, such as private key and certificate chain reloading, etc.
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager {
private static final Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName());

Expand All @@ -57,12 +58,12 @@ public PrivateKey getPrivateKey(String alias) {

@Override
public X509Certificate[] getCertificateChain(String alias) {
return this.certs;
return Arrays.copyOf(this.certs, this.certs.length);
}

@Override
public String[] getClientAliases(String keyType, Principal[] issuers) {
return new String[0];
return new String[] {"default"};
}

@Override
Expand All @@ -77,7 +78,7 @@ public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSL

@Override
public String[] getServerAliases(String keyType, Principal[] issuers) {
return new String[0];
return new String[] {"default"};
}

@Override
Expand All @@ -93,38 +94,36 @@ public String chooseEngineServerAlias(String keyType, Principal[] issuers,

/**
* Updates the current cached private key and cert chains.
* Right now, callers should make sure |key| matches the public key on the leaf certificate of
* |certs|.
* TODO(ZhenLian): explore possibilities to do a crypto check here.
*
* @param key the private key that is going to be used
* @param certs the certificate chain that is going to be used
*/
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}.
// TODO(ZhenLian): explore possibilities to do a crypto check here.
this.key = key;
this.certs = certs;
this.certs = Arrays.copyOf(certs, certs.length);
}

/**
* starts a {@code ScheduledExecutorService} to read private key and certificate chains from the
* local file paths periodically, and update the cached identity credentials if they are both
* Schedules a {@code ScheduledExecutorService} to read private key and certificate chains from
* the local file paths periodically, and update the cached identity credentials if they are both
* updated.
*
* @param keyFile the file on disk holding the private key
* @param certFile the file on disk holding the certificate chain
* @param initialDelay the time to delay first read-and-update execution
* @param delay the period between successive read-and-update executions
* @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 start the read-and-update thread
* @return an object that caller should close when the scheduled execution is not needed
* @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 initialDelay, long delay, TimeUnit unit, ScheduledExecutorService executor) {
long period, TimeUnit unit, ScheduledExecutorService executor) {
final ScheduledFuture<?> future =
executor.scheduleWithFixedDelay(
new LoadFilePathExecution(keyFile, certFile),
initialDelay, delay, unit);
new LoadFilePathExecution(keyFile, certFile), 0, period, unit);
return new Closeable() {
@Override public void close() {
future.cancel(false);
Expand Down Expand Up @@ -174,8 +173,8 @@ public UpdateResult(boolean success, long keyTime, long certTime) {
}

/**
* reads the private key and certificates specified in the path locations. Updates |key| and
* |cert| if both of their modified time changed since last read.
* Reads the private key and certificates specified in the path locations. Updates {@code key} and
* {@code cert} if both of their modified time changed since last read.
*
* @param keyFile the file on disk holding the private key
* @param certFile the file on disk holding the certificate chain
Expand All @@ -189,15 +188,21 @@ 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) {
PrivateKey key = CertificateUtils.getPrivateKey(new FileInputStream(keyFile));
X509Certificate[] certs = CertificateUtils.getX509Certificates(new FileInputStream(certFile));
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);
}
return new UpdateResult(false, -1, -1);
}

// Mainly used to avoid throwing IO Exceptions in java.io.Closeable.
/**
* Mainly used to avoid throwing IO Exceptions in java.io.Closeable.
*/
public interface Closeable extends java.io.Closeable {
@Override public void close();
}
Expand Down
88 changes: 37 additions & 51 deletions core/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
View file
Expand Up @@ -19,14 +19,14 @@
import io.grpc.ExperimentalApi;
import java.io.File;
import java.io.FileInputStream;
import java.io.FileNotFoundException;
import java.io.IOException;
import java.net.Socket;
import java.security.KeyStore;
import java.security.KeyStoreException;
import java.security.NoSuchAlgorithmException;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
import java.util.Arrays;
import java.util.concurrent.ScheduledExecutorService;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
Expand All @@ -39,29 +39,27 @@
import javax.net.ssl.X509ExtendedTrustManager;
import org.codehaus.mojo.animal_sniffer.IgnoreJRERequirement;

@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
/**
* For Android users: this class is only supported in API level 24 and above.
* AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure
* advanced TLS features, such as root certificate reloading, peer cert custom verification, etc.
* For Android users: this class is only supported in API level 24 and above.
*/
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
@IgnoreJRERequirement
public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager {
private static final Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName());

private Verification verification;
private PeerVerifier peerVerifier;
private SslSocketPeerVerifier sslSocketPeerVerifier;
private SslEnginePeerVerifier sslEnginePeerVerifier;
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;

private AdvancedTlsX509TrustManager(Verification verification, PeerVerifier peerVerifier,
SslSocketPeerVerifier socketPeerVerifier, SslEnginePeerVerifier enginePeerVerifier) {
SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) {
this.verification = verification;
this.peerVerifier = peerVerifier;
this.sslSocketPeerVerifier = socketPeerVerifier;
this.sslEnginePeerVerifier = enginePeerVerifier;
this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier;
}

@Override
Expand Down Expand Up @@ -121,7 +119,7 @@ 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?
this.trustCerts = trustCerts;
this.trustCerts = Arrays.copyOf(trustCerts, trustCerts.length);
}

private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine sslEngine,
Expand Down Expand Up @@ -198,37 +196,33 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
}
}
// Perform the additional peer cert check.
if (sslEngine != null && sslEnginePeerVerifier != null) {
sslEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine);
}
if (socket != null && sslSocketPeerVerifier != null) {
sslSocketPeerVerifier.verifyPeerCertificate(chain, authType, socket);
if (sslEngine != null && socket != null && socketAndEnginePeerVerifier != null) {
socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, socket);
socketAndEnginePeerVerifier.verifyPeerCertificate(chain, authType, sslEngine);
}
if (peerVerifier != null) {
peerVerifier.verifyPeerCertificate(chain, authType);
}
if (sslEnginePeerVerifier == null && sslSocketPeerVerifier == null && peerVerifier == null) {
if (socketAndEnginePeerVerifier == null && peerVerifier == null) {
log.log(Level.INFO, "No peer verifier is specified");
}
}

/**
* starts a {@code ScheduledExecutorService} to read trust certificates from a local file path
* Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
* periodically, and update the cached trust certs if there is an update.
*
* @param trustCertFile the file on disk holding the trust certificates
* @param initialDelay the time to delay first read-and-update execution
* @param delay the period between successive read-and-update executions
* @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 start the read-and-update thread
* @return an object that caller should close when the scheduled execution is not needed
* @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 updateTrustCredentialsFromFile(File trustCertFile, long initialDelay,
long delay, TimeUnit unit, ScheduledExecutorService executor) {
public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
ScheduledExecutorService executor) {
final ScheduledFuture<?> future =
executor.scheduleWithFixedDelay(
new LoadFilePathExecution(trustCertFile),
initialDelay, delay, unit);
new LoadFilePathExecution(trustCertFile), 0, period, unit);
return new Closeable() {
@Override public void close() {
future.cancel(false);
Expand All @@ -252,7 +246,7 @@ public void run() {
if (newUpdateTime != 0) {
this.currentTime = newUpdateTime;
}
} catch (FileNotFoundException | CertificateException e) {
} catch (CertificateException | IOException e) {
log.log(Level.SEVERE, "readAndUpdate() execution thread failed", e);

}
Expand All @@ -268,11 +262,12 @@ 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 FileNotFoundException, CertificateException {
throws CertificateException, IOException {
long newTime = trustCertFile.lastModified();
if (newTime != oldTime) {
X509Certificate[] certificates = CertificateUtils.getX509Certificates(
new FileInputStream(trustCertFile));
FileInputStream inputStream = new FileInputStream(trustCertFile);
X509Certificate[] certificates = CertificateUtils.getX509Certificates(inputStream);
inputStream.close();
updateTrustCredentials(certificates);
return newTime;
}
Expand Down Expand Up @@ -328,9 +323,9 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType)
}

// Additional custom peer verification check.
// It will be used when checkClientTrusted/checkServerTrusted is called with the {@code Socket}
// parameter.
public interface SslSocketPeerVerifier {
// It will be used when checkClientTrusted/checkServerTrusted is called with the {@code Socket} or
// the {@code SSLEngine} parameter.
public interface SslSocketAndEnginePeerVerifier {
/**
* Verifies the peer certificate chain. For more information, please refer to
* {@code X509ExtendedTrustManager}.
Expand All @@ -342,12 +337,7 @@ public interface SslSocketPeerVerifier {
*/
void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, Socket socket)
throws CertificateException;
}

// Additional custom peer verification check.
// It will be used when checkClientTrusted/checkServerTrusted is called with the {@code SSLEngine}
// parameter.
public interface SslEnginePeerVerifier {
/**
* Verifies the peer certificate chain. For more information, please refer to
* {@code X509ExtendedTrustManager}.
Expand All @@ -363,36 +353,32 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSL

public static final class Builder {

private Verification verification;
private Verification verification = Verification.CertificateAndHostNameVerification;
private PeerVerifier peerVerifier;
private SslSocketPeerVerifier socketPeerVerifier;
private SslEnginePeerVerifier enginePeerVerifier;
private SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier;

private Builder() {}

public Builder setVerification(Verification verification) {
this.verification = verification;
return this;
}

public Builder setPeerVerifier(PeerVerifier peerVerifier) {
this.peerVerifier = peerVerifier;
return this;
}

public Builder setSslSocketPeerVerifier(SslSocketPeerVerifier peerVerifier) {
this.socketPeerVerifier = peerVerifier;
public Builder setPeerVerifier(PeerVerifier verifier) {
this.peerVerifier = verifier;
return this;
}

public Builder setSslEnginePeerVerifier(SslEnginePeerVerifier peerVerifier) {
this.enginePeerVerifier = peerVerifier;
public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier verifier) {
this.socketAndEnginePeerVerifier = verifier;
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() {
return new AdvancedTlsX509TrustManager(this.verification, this.peerVerifier,
this.socketPeerVerifier, this.enginePeerVerifier);
this.socketAndEnginePeerVerifier);
}
}
}
Expand Down

0 comments on commit 3388548

Please sign in to comment.