From 516dec989add854a2d458737b7efc3b810b0ede9 Mon Sep 17 00:00:00 2001 From: erm-g <110920239+erm-g@users.noreply.github.com> Date: Tue, 16 Jul 2024 12:33:19 -0400 Subject: [PATCH] util: Align AdvancedTlsX509{Key and Trust}Manager (#11385) * Swap internal key/cert args * Deprecate *FromFile methods * Test for client trusted socket --- .../java/io/grpc/netty/AdvancedTlsTest.java | 14 +-- .../grpc/util/AdvancedTlsX509KeyManager.java | 22 ++--- .../util/AdvancedTlsX509TrustManager.java | 97 +++++++++++++------ .../util/AdvancedTlsX509KeyManagerTest.java | 6 +- .../util/AdvancedTlsX509TrustManagerTest.java | 39 +++++--- 5 files changed, 115 insertions(+), 63 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java index d34eecc5ab..f34e336553 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -339,7 +339,7 @@ public class AdvancedTlsTest { AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) .build(); - Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentialsFromFile(caCertFile, + Closeable serverTrustShutdown = serverTrustManager.updateTrustCredentials(caCertFile, 100, TimeUnit.MILLISECONDS, executor); ServerCredentials serverCredentials = TlsServerCredentials.newBuilder() .keyManager(serverKeyManager).trustManager(serverTrustManager) @@ -349,11 +349,11 @@ public class AdvancedTlsTest { // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentials(clientCert0File, - clientKey0File,100, TimeUnit.MILLISECONDS, executor); + clientKey0File, 100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION) .build(); - Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentialsFromFile(caCertFile, + Closeable clientTrustShutdown = clientTrustManager.updateTrustCredentials(caCertFile, 100, TimeUnit.MILLISECONDS, executor); ChannelCredentials channelCredentials = TlsChannelCredentials.newBuilder() .keyManager(clientKeyManager).trustManager(clientTrustManager).build(); @@ -385,7 +385,7 @@ public class AdvancedTlsTest { 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(); @@ -397,7 +397,7 @@ public class AdvancedTlsTest { 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) @@ -431,8 +431,8 @@ public class AdvancedTlsTest { .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 diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index ccec3ab342..1f807cd405 100644 --- a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -116,7 +116,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { * @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")); } /** @@ -216,26 +216,26 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { 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 @@ -244,11 +244,11 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { 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); } @@ -257,13 +257,13 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { 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; } } diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index bfa0a04007..088f4caa00 100644 --- a/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -18,6 +18,8 @@ package io.grpc.util; 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; @@ -116,7 +118,7 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager /** * 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 { @@ -125,25 +127,6 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager 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( @@ -217,6 +200,39 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager } } + /** + * 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 @@ -233,7 +249,7 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager * @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) { @@ -253,18 +269,43 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager 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 { @@ -383,8 +424,8 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager * 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 { diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java index ea9a2234f1..f96c85e4f4 100644 --- a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java +++ b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java @@ -76,7 +76,7 @@ public class AdvancedTlsX509KeyManagerTest { } @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); @@ -91,6 +91,10 @@ public class AdvancedTlsX509KeyManagerTest { 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 diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java index c8ca0adb5c..36ef75abea 100644 --- a/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java +++ b/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java @@ -20,11 +20,14 @@ import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; import com.google.common.collect.Iterables; import io.grpc.internal.FakeClock; import io.grpc.internal.testing.TestUtils; import io.grpc.testing.TlsTesting; +import io.grpc.util.AdvancedTlsX509TrustManager.Verification; import java.io.File; import java.io.IOException; import java.net.Socket; @@ -40,6 +43,7 @@ import java.util.logging.Handler; import java.util.logging.Level; import java.util.logging.LogRecord; import java.util.logging.Logger; +import javax.net.ssl.SSLSocket; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; @@ -72,17 +76,17 @@ public class AdvancedTlsX509TrustManagerTest { 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()); } @@ -101,23 +105,15 @@ public class AdvancedTlsX509TrustManagerTest { AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build(); NullPointerException npe = assertThrows(NullPointerException.class, () -> trustManager - .updateTrustCredentials(null)); - assertEquals("trustCerts", npe.getMessage()); - - npe = assertThrows(NullPointerException.class, () -> trustManager - .updateTrustCredentialsFromFile(null)); + .updateTrustCredentials(null, 1, null, null)); assertEquals("trustCertFile", npe.getMessage()); npe = assertThrows(NullPointerException.class, () -> trustManager - .updateTrustCredentialsFromFile(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()); @@ -125,8 +121,7 @@ public class AdvancedTlsX509TrustManagerTest { 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(), @@ -137,6 +132,18 @@ public class AdvancedTlsX509TrustManagerTest { } } + @Test + public void clientTrustedWithSocketTest() throws Exception { + AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder() + .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION).build(); + trustManager.updateTrustCredentials(caCert); + SSLSocket sslSocket = mock(SSLSocket.class); + when(sslSocket.isConnected()).thenReturn(true); + when(sslSocket.getHandshakeSession()).thenReturn(null); + CertificateException ce = assertThrows(CertificateException.class, () -> trustManager + .checkClientTrusted(serverCert0, "RSA", sslSocket)); + assertEquals("No handshake session", ce.getMessage()); + } private static class TestHandler extends Handler { private final List records = new ArrayList<>();