Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

util: Align AdvancedTlsX509{Key and Trust}Manager #11385

Merged
merged 7 commits into from
Jul 16, 2024
Merged

util: Align AdvancedTlsX509{Key and Trust}Manager #11385

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
60e4679
Swap internal key/cert args
erm-g Jul 11, 2024
601ec35
Deprecate *FromFile methods
erm-g Jul 11, 2024
bec6f9e
Deprecate *FromFile methods
erm-g Jul 12, 2024
d8a4f01
Style fix
erm-g Jul 12, 2024
a1f6af7
Style fix
erm-g Jul 12, 2024
917e48f
Test for client trusted socket
erm-g Jul 16, 2024
f289c90
Style fix
erm-g Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,8 +339,8 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
.setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
.build();
Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentialsFromFile(caCertFile,
100, TimeUnit.MILLISECONDS, executor);
Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentials(caCertFile,100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space before 100. Ditto elsewhere.

FWIW, I'll commonly linewrap before the 100, to keep it together with its unit. Although it seems you are purposefully doing the opposite, as late in the file you could have TimeUnit on the same line and you chose not to.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, missed the spaces because of IDE param name hints:
Image 7-16-24 at 11 17 AM
Will keep it disabled.
I like the idea of having value + unit on the same line, also fixed it.

TimeUnit.MILLISECONDS, executor);
ServerCredentials serverCredentials = TlsServerCredentials.newBuilder()
.keyManager(serverKeyManager).trustManager(serverTrustManager)
.clientAuth(ClientAuth.REQUIRE).build();
Expand All @@ -353,8 +353,8 @@ public void onFileReloadingKeyManagerTrustManagerTest() throws Exception {
AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
.setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
.build();
Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentialsFromFile(caCertFile,
100, TimeUnit.MILLISECONDS, executor);
Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentials(caCertFile,100,
TimeUnit.MILLISECONDS, executor);
ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder()
.keyManager(clientKeyManager).trustManager(clientTrustManager).build();
channel = Grpc.newChannelBuilderForAddress("localhost", server.getPort(), channelCredentials)
Expand Down Expand Up @@ -385,7 +385,7 @@ public void onFileLoadingKeyManagerTrustManagerTest() throws Exception {
AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder()
.setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION)
.build();
serverTrustManager.updateTrustCredentialsFromFile(caCertFile);
serverTrustManager.updateTrustCredentials(caCertFile);
ServerCredentials serverCredentials = TlsServerCredentials.newBuilder()
.keyManager(serverKeyManager).trustManager(serverTrustManager)
.clientAuth(ClientAuth.REQUIRE).build();
Expand All @@ -397,7 +397,7 @@ public void onFileLoadingKeyManagerTrustManagerTest() throws Exception {
AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder()
.setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION)
.build();
clientTrustManager.updateTrustCredentialsFromFile(caCertFile);
clientTrustManager.updateTrustCredentials(caCertFile);
ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder()
.keyManager(clientKeyManager).trustManager(clientTrustManager).build();
channel = Grpc.newChannelBuilderForAddress("localhost", server.getPort(), channelCredentials)
Expand Down Expand Up @@ -431,8 +431,8 @@ public void onFileReloadingTrustManagerBadInitialContentTest() throws Exception
.build();
// We pass in a key as the trust certificates to intentionally create an exception.
assertThrows(GeneralSecurityException.class,
() -> trustManager.updateTrustCredentialsFromFile(serverKey0File,
100, TimeUnit.MILLISECONDS, executor));
() -> trustManager.updateTrustCredentials(serverKey0File,100,
TimeUnit.MILLISECONDS, executor));
}

@Test
Expand Down
22 changes: 11 additions & 11 deletions util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) {
* @param key the private key that is going to be used
*/
public void updateIdentityCredentials(X509Certificate[] certs, PrivateKey key) {
this.keyInfo = new KeyInfo(checkNotNull(key, "key"), checkNotNull(certs, "certs"));
this.keyInfo = new KeyInfo(checkNotNull(certs, "certs"), checkNotNull(key, "key"));
}

/**
Expand Down Expand Up @@ -216,26 +216,26 @@ 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;
final PrivateKey key;

public KeyInfo(PrivateKey key, X509Certificate[] certs) {
this.key = key;
public KeyInfo(X509Certificate[] certs, PrivateKey key) {
this.certs = certs;
this.key = key;
}
}

private class LoadFilePathExecution implements Runnable {
File keyFile;
File certFile;
long currentKeyTime;
long currentCertTime;
long currentKeyTime;

public LoadFilePathExecution(File certFile, File keyFile) {
this.certFile = certFile;
this.keyFile = keyFile;
this.currentKeyTime = 0;
this.currentCertTime = 0;
this.currentKeyTime = 0;
}

@Override
Expand All @@ -244,11 +244,11 @@ public void run() {
UpdateResult newResult = readAndUpdate(this.certFile, this.keyFile, this.currentKeyTime,
this.currentCertTime);
if (newResult.success) {
this.currentKeyTime = newResult.keyTime;
this.currentCertTime = newResult.certTime;
this.currentKeyTime = newResult.keyTime;
}
} catch (IOException | GeneralSecurityException e) {
log.log(Level.SEVERE, String.format("Failed refreshing private key and certificate"
log.log(Level.SEVERE, String.format("Failed refreshing certificate and private key"
+ " chain from files. Using previous ones (certFile lastModified = %s, keyFile "
+ "lastModified = %s)", certFile.lastModified(), keyFile.lastModified()), e);
}
Expand All @@ -257,13 +257,13 @@ public void run() {

private static class UpdateResult {
boolean success;
long keyTime;
long certTime;
long keyTime;

public UpdateResult(boolean success, long keyTime, long certTime) {
public UpdateResult(boolean success, long certTime, long keyTime) {
this.success = success;
this.keyTime = keyTime;
this.certTime = certTime;
this.keyTime = keyTime;
}
}

Expand Down
97 changes: 69 additions & 28 deletions util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@

import static com.google.common.base.Preconditions.checkNotNull;

import com.google.errorprone.annotations.InlineMe;
import io.grpc.ExperimentalApi;
import java.io.File;
import java.io.FileInputStream;
import java.io.IOException;
Expand Down Expand Up @@ -116,7 +118,7 @@ public X509Certificate[] getAcceptedIssuers() {
/**
* 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.
* data(e.g. updateTrustCredentials) should not be called.
*/
public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreException,
NoSuchAlgorithmException {
Expand All @@ -125,25 +127,6 @@ public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreEx
this.delegateManager = createDelegateTrustManager(null);
}

/**
* 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) throws IOException,
GeneralSecurityException {
checkNotNull(trustCerts, "trustCerts");
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
keyStore.load(null, null);
int i = 1;
for (X509Certificate cert: trustCerts) {
String alias = Integer.toString(i);
keyStore.setCertificateEntry(alias, cert);
i++;
}
this.delegateManager = createDelegateTrustManager(keyStore);
}

private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyStore)
throws CertificateException, KeyStoreException, NoSuchAlgorithmException {
TrustManagerFactory tmf = TrustManagerFactory.getInstance(
Expand Down Expand Up @@ -217,6 +200,39 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
}
}

/**
* 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) throws IOException,
GeneralSecurityException {
checkNotNull(trustCerts, "trustCerts");
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
keyStore.load(null, null);
int i = 1;
for (X509Certificate cert: trustCerts) {
String alias = Integer.toString(i);
keyStore.setCertificateEntry(alias, cert);
i++;
}
this.delegateManager = createDelegateTrustManager(keyStore);
}

/**
* Updates the trust certificates from a local file path.
*
* @param trustCertFile the file on disk holding the trust certificates
*/
public void updateTrustCredentials(File trustCertFile) throws IOException,
GeneralSecurityException {
long updatedTime = readAndUpdate(trustCertFile, 0);
if (updatedTime == 0) {
throw new GeneralSecurityException(
"Files were unmodified before their initial update. Probably a bug.");
}
}

/**
* Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
* periodically, and updates the cached trust certs if there is an update. You must close the
Expand All @@ -233,7 +249,7 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss
* @param executor the executor 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 period, TimeUnit unit,
public Closeable updateTrustCredentials(File trustCertFile, long period, TimeUnit unit,
ScheduledExecutorService executor) throws IOException, GeneralSecurityException {
long updatedTime = readAndUpdate(trustCertFile, 0);
if (updatedTime == 0) {
Expand All @@ -253,18 +269,43 @@ public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period,
return () -> future.cancel(false);
}

/**
* Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path
* periodically, and updates the cached trust certs if there is an update. You must close the
* returned Closeable before calling this method again or other update methods
* ({@link AdvancedTlsX509TrustManager#useSystemDefaultTrustCerts()},
* {@link AdvancedTlsX509TrustManager#updateTrustCredentials(X509Certificate[])},
* {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File)}).
* Before scheduling the task, the method synchronously reads and updates trust certificates once.
* If the provided period is less than 1 minute, it is automatically adjusted to 1 minute.
*
* @param trustCertFile the file on disk holding the trust certificates
* @param period the period between successive read-and-update executions
* @param unit the time unit of the initialDelay and period parameters
* @param executor the executor service we use to read and update the credentials
* @return an object that caller should close when the file refreshes are not needed
* @deprecated Use {@link #updateTrustCredentials(File, long ,TimeUnit, ScheduledExecutorService)}
*/
@Deprecated
@InlineMe(replacement = "this.updateTrustCredentials(trustCertFile, period, unit, executor)")
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
ScheduledExecutorService executor) throws IOException, GeneralSecurityException {
return updateTrustCredentials(trustCertFile, period, unit, executor);
}

/**
* Updates the trust certificates from a local file path.
*
* @param trustCertFile the file on disk holding the trust certificates
* @deprecated Use {@link #updateTrustCredentials(File)}
*/
@Deprecated
@InlineMe(replacement = "this.updateTrustCredentials(trustCertFile)")
@ExperimentalApi("https://github.com/grpc/grpc-java/issues/8024")
public void updateTrustCredentialsFromFile(File trustCertFile) throws IOException,
GeneralSecurityException {
long updatedTime = readAndUpdate(trustCertFile, 0);
if (updatedTime == 0) {
throw new GeneralSecurityException(
"Files were unmodified before their initial update. Probably a bug.");
}
updateTrustCredentials(trustCertFile);
}

private class LoadFilePathExecution implements Runnable {
Expand Down Expand Up @@ -383,8 +424,8 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSL
* Builds a new {@link AdvancedTlsX509TrustManager}. By default, no trust certificates are loaded
* after the build. To load them, use one of the following methods: {@link
* AdvancedTlsX509TrustManager#updateTrustCredentials(X509Certificate[])}, {@link
* AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File, long, TimeUnit,
* ScheduledExecutorService)}, {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile
* AdvancedTlsX509TrustManager#updateTrustCredentials(File, long, TimeUnit,
* ScheduledExecutorService)}, {@link AdvancedTlsX509TrustManager#updateTrustCredentials
* (File, long, TimeUnit, ScheduledExecutorService)}.
*/
public static final class Builder {
Expand Down
6 changes: 5 additions & 1 deletion util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public void setUp() throws Exception {
}

@Test
public void credentialSetting() throws Exception {
public void updateTrustCredentials_replacesIssuers() throws Exception {
// Overall happy path checking of public API.
AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager();
serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
Expand All @@ -91,6 +91,10 @@ public void credentialSetting() throws Exception {
TimeUnit.MINUTES, executor);
assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));

serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0);
assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS));
assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS));
}

@Test
Expand Down
23 changes: 7 additions & 16 deletions util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,17 @@ public void setUp() throws IOException, GeneralSecurityException {
public void updateTrustCredentials_replacesIssuers() throws Exception {
// Overall happy path checking of public API.
AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();
trustManager.updateTrustCredentialsFromFile(serverCert0File);
trustManager.updateTrustCredentials(serverCert0File);
assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());

trustManager.updateTrustCredentials(caCert);
assertArrayEquals(caCert, trustManager.getAcceptedIssuers());

trustManager.updateTrustCredentialsFromFile(serverCert0File, 1, TimeUnit.MINUTES,
trustManager.updateTrustCredentials(serverCert0File, 1, TimeUnit.MINUTES,
executor);
assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());

trustManager.updateTrustCredentialsFromFile(serverCert0File);
trustManager.updateTrustCredentials(serverCert0File);
assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers());
}

Expand All @@ -101,32 +101,23 @@ public void credentialSettingParameterValidity() throws Exception {
AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build();

NullPointerException npe = assertThrows(NullPointerException.class, () -> trustManager
.updateTrustCredentials(null));
assertEquals("trustCerts", npe.getMessage());

npe = assertThrows(NullPointerException.class, () -> trustManager
.updateTrustCredentialsFromFile(null));
assertEquals("trustCertFile", npe.getMessage());

npe = assertThrows(NullPointerException.class, () -> trustManager
.updateTrustCredentialsFromFile(null, 1, null, null));
.updateTrustCredentials(null, 1, null, null));
assertEquals("trustCertFile", npe.getMessage());

npe = assertThrows(NullPointerException.class, () -> trustManager
.updateTrustCredentialsFromFile(caCertFile, 1, null, null));
.updateTrustCredentials(caCertFile, 1, null, null));
assertEquals("unit", npe.getMessage());

npe = assertThrows(NullPointerException.class, () -> trustManager
.updateTrustCredentialsFromFile(caCertFile, 1, TimeUnit.MINUTES, null));
.updateTrustCredentials(caCertFile, 1, TimeUnit.MINUTES, null));
assertEquals("executor", npe.getMessage());

Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName());
TestHandler handler = new TestHandler();
log.addHandler(handler);
log.setUseParentHandlers(false);
log.setLevel(Level.FINE);
trustManager.updateTrustCredentialsFromFile(serverCert0File, -1, TimeUnit.SECONDS,
executor);
trustManager.updateTrustCredentials(serverCert0File, -1, TimeUnit.SECONDS, executor);
log.removeHandler(handler);
try {
LogRecord logRecord = Iterables.find(handler.getRecords(),
Expand Down
Loading