From ecae9b797d35f0c3acc256edcf4e723b9792222c Mon Sep 17 00:00:00 2001 From: erm-g <110920239+erm-g@users.noreply.github.com> Date: Wed, 10 Jul 2024 03:00:27 -0400 Subject: [PATCH] security: Add updateIdentityCredentials methods to AdvancedTlsX509KeyManager. (#11358) Add new 'updateIdentityCredentials' methods with swapped (chain, key) signatures. --- .../java/io/grpc/netty/AdvancedTlsTest.java | 28 +++--- util/BUILD.bazel | 1 + .../grpc/util/AdvancedTlsX509KeyManager.java | 97 ++++++++++++++----- .../util/AdvancedTlsX509KeyManagerTest.java | 26 ++--- 4 files changed, 103 insertions(+), 49 deletions(-) diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java index da3e20e9ff..d34eecc5ab 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -147,7 +147,7 @@ public class AdvancedTlsTest { public void advancedTlsKeyManagerTrustManagerMutualTlsTest() throws Exception { // Create a server with the key manager and trust manager. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) .build(); @@ -159,7 +159,7 @@ public class AdvancedTlsTest { new SimpleServiceImpl()).build().start(); // Create a client with the key manager and trust manager. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); + clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION) .build(); @@ -182,7 +182,7 @@ public class AdvancedTlsTest { @Test public void trustManagerCustomVerifierMutualTlsTest() throws Exception { AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0); // Set server's custom verification based on the information of clientCert0. AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) @@ -221,7 +221,7 @@ public class AdvancedTlsTest { new SimpleServiceImpl()).build().start(); AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); + clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0); // Set client's custom verification based on the information of serverCert0. AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) @@ -275,7 +275,7 @@ public class AdvancedTlsTest { AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); // Even if we provide bad credentials for the server, the test should still pass, because we // will configure the client to skip all checks later. - serverKeyManager.updateIdentityCredentials(serverKeyBad, serverCertBad); + serverKeyManager.updateIdentityCredentials(serverCertBad, serverKeyBad); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) .setSslSocketAndEnginePeerVerifier( @@ -297,7 +297,7 @@ public class AdvancedTlsTest { new SimpleServiceImpl()).build().start(); AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentials(clientKey0, clientCert0); + clientKeyManager.updateIdentityCredentials(clientCert0, clientKey0); // Set the client to skip all checks, including traditional certificate verification. // Note this is very dangerous in production environment - only do so if you are confident on // what you are doing! @@ -334,8 +334,8 @@ public class AdvancedTlsTest { public void onFileReloadingKeyManagerTrustManagerTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, - serverCert0File, 100, TimeUnit.MILLISECONDS, executor); + Closeable serverKeyShutdown = serverKeyManager.updateIdentityCredentials(serverCert0File, + serverKey0File, 100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) .build(); @@ -348,8 +348,8 @@ public class AdvancedTlsTest { new SimpleServiceImpl()).build().start(); // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File, - clientCert0File,100, TimeUnit.MILLISECONDS, executor); + Closeable clientKeyShutdown = clientKeyManager.updateIdentityCredentials(clientCert0File, + clientKey0File,100, TimeUnit.MILLISECONDS, executor); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION) .build(); @@ -381,7 +381,7 @@ public class AdvancedTlsTest { public void onFileLoadingKeyManagerTrustManagerTest() throws Exception { // Create & start a server. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File); + serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File); AdvancedTlsX509TrustManager serverTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_ONLY_VERIFICATION) .build(); @@ -393,7 +393,7 @@ public class AdvancedTlsTest { new SimpleServiceImpl()).build().start(); // Create a client to connect. AdvancedTlsX509KeyManager clientKeyManager = new AdvancedTlsX509KeyManager(); - clientKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File); + clientKeyManager.updateIdentityCredentials(clientCert0File, clientKey0File); AdvancedTlsX509TrustManager clientTrustManager = AdvancedTlsX509TrustManager.newBuilder() .setVerification(Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION) .build(); @@ -420,8 +420,8 @@ public class AdvancedTlsTest { AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager(); // We swap the order of key and certificates to intentionally create an exception. assertThrows(GeneralSecurityException.class, - () -> keyManager.updateIdentityCredentialsFromFile(serverCert0File, - serverKey0File, 100, TimeUnit.MILLISECONDS, executor)); + () -> keyManager.updateIdentityCredentials(serverKey0File, serverCert0File, + 100, TimeUnit.MILLISECONDS, executor)); } @Test diff --git a/util/BUILD.bazel b/util/BUILD.bazel index 4c88d850d8..7a38063a98 100644 --- a/util/BUILD.bazel +++ b/util/BUILD.bazel @@ -13,6 +13,7 @@ java_library( "//api", "//core:internal", artifact("com.google.code.findbugs:jsr305"), + artifact("com.google.errorprone:error_prone_annotations"), artifact("com.google.guava:guava"), artifact("com.google.j2objc:j2objc-annotations"), artifact("org.codehaus.mojo:animal-sniffer-annotations"), diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index 3014004dda..ded7a6aed1 100644 --- a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -18,7 +18,7 @@ package io.grpc.util; import static com.google.common.base.Preconditions.checkNotNull; -import io.grpc.ExperimentalApi; +import com.google.errorprone.annotations.InlineMe; import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -40,7 +40,6 @@ import javax.net.ssl.X509ExtendedKeyManager; * AdvancedTlsX509KeyManager is an {@code X509ExtendedKeyManager} that allows users to configure * advanced TLS features, such as private key and certificate chain reloading. */ -@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()); // Minimum allowed period for refreshing files with credential information. @@ -100,31 +99,44 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { * * @param key the private key that is going to be used * @param certs the certificate chain that is going to be used + * @deprecated Use {@link #updateIdentityCredentials(X509Certificate[], PrivateKey)} */ + @Deprecated + @InlineMe(replacement = "this.updateIdentityCredentials(certs, key)") public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) { + updateIdentityCredentials(certs, key); + } + + /** + * Updates the current cached private key and cert chains. + * + * @param certs the certificate chain that is going to be used + * @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")); } /** - * Schedules a {@code ScheduledExecutorService} to read private key and certificate chains from + * Schedules a {@code ScheduledExecutorService} to read certificate chains and private key from * the local file paths periodically, and update the cached identity credentials if they are both * updated. You must close the returned Closeable before calling this method again or other update * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link - * AdvancedTlsX509KeyManager#updateIdentityCredentialsFromFile(File, File)}). + * AdvancedTlsX509KeyManager#updateIdentityCredentials(File, File)}). * Before scheduling the task, the method synchronously executes {@code readAndUpdate} once. The * minimum refresh period of 1 minute is enforced. * - * @param keyFile the file on disk holding the private key * @param certFile the file on disk holding the certificate chain + * @param keyFile the file on disk holding the private 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 + * @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 updateIdentityCredentialsFromFile(File keyFile, File certFile, + public Closeable updateIdentityCredentials(File certFile, File keyFile, long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException, GeneralSecurityException { - UpdateResult newResult = readAndUpdate(keyFile, certFile, 0, 0); + UpdateResult newResult = readAndUpdate(certFile, keyFile, 0, 0); if (!newResult.success) { throw new GeneralSecurityException( "Files were unmodified before their initial update. Probably a bug."); @@ -138,23 +150,64 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { } final ScheduledFuture future = checkNotNull(executor, "executor").scheduleWithFixedDelay( - new LoadFilePathExecution(keyFile, certFile), period, period, unit); + new LoadFilePathExecution(certFile, keyFile), period, period, unit); return () -> future.cancel(false); } + /** + * Updates certificate chains and the private key from the local file paths. + * + * @param certFile the file on disk holding the certificate chain + * @param keyFile the file on disk holding the private key + */ + public void updateIdentityCredentials(File certFile, File keyFile) throws IOException, + GeneralSecurityException { + UpdateResult newResult = readAndUpdate(certFile, keyFile, 0, 0); + if (!newResult.success) { + throw new GeneralSecurityException( + "Files were unmodified before their initial update. Probably a bug."); + } + } + /** * Updates the private key and certificate chains from the local file paths. * * @param keyFile the file on disk holding the private key * @param certFile the file on disk holding the certificate chain + * @deprecated Use {@link #updateIdentityCredentials(File, File)} instead. */ + @Deprecated + @InlineMe(replacement = "this.updateIdentityCredentials(certFile, keyFile)") public void updateIdentityCredentialsFromFile(File keyFile, File certFile) throws IOException, GeneralSecurityException { - UpdateResult newResult = readAndUpdate(keyFile, certFile, 0, 0); - if (!newResult.success) { - throw new GeneralSecurityException( - "Files were unmodified before their initial update. Probably a bug."); - } + updateIdentityCredentials(certFile, keyFile); + } + + /** + * 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. You must close the returned Closeable before calling this method again or other update + * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link + * AdvancedTlsX509KeyManager#updateIdentityCredentials(File, File)}). + * Before scheduling the task, the method synchronously executes {@code readAndUpdate} once. The + * minimum refresh period of 1 minute is enforced. + * + * @param keyFile the file on disk holding the private key + * @param certFile the file on disk holding the certificate chain + * @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 + * #updateIdentityCredentials(File, File, long, TimeUnit, ScheduledExecutorService)} instead. + */ + @Deprecated + @InlineMe(replacement = + "this.updateIdentityCredentials(certFile, keyFile, period, unit, executor)") + public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile, + long period, TimeUnit unit, ScheduledExecutorService executor) throws IOException, + GeneralSecurityException { + return updateIdentityCredentials(certFile, keyFile, period, unit, executor); } private static class KeyInfo { @@ -174,9 +227,9 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { long currentKeyTime; long currentCertTime; - public LoadFilePathExecution(File keyFile, File certFile) { - this.keyFile = keyFile; + public LoadFilePathExecution(File certFile, File keyFile) { this.certFile = certFile; + this.keyFile = keyFile; this.currentKeyTime = 0; this.currentCertTime = 0; } @@ -184,7 +237,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { @Override public void run() { try { - UpdateResult newResult = readAndUpdate(this.keyFile, this.certFile, this.currentKeyTime, + UpdateResult newResult = readAndUpdate(this.certFile, this.keyFile, this.currentKeyTime, this.currentCertTime); if (newResult.success) { this.currentKeyTime = newResult.keyTime; @@ -192,8 +245,8 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { } } catch (IOException | GeneralSecurityException e) { log.log(Level.SEVERE, String.format("Failed refreshing private key and certificate" - + " chain from files. Using previous ones (keyFile lastModified = %s, certFile " - + "lastModified = %s)", keyFile.lastModified(), certFile.lastModified()), e); + + " chain from files. Using previous ones (certFile lastModified = %s, keyFile " + + "lastModified = %s)", certFile.lastModified(), keyFile.lastModified()), e); } } } @@ -214,13 +267,13 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { * 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 + * @param keyFile the file on disk holding the private key * @param oldKeyTime the time when the private key file is modified during last execution * @param oldCertTime the time when the certificate chain file is modified during last execution * @return the result of this update execution */ - private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long oldCertTime) + private UpdateResult readAndUpdate(File certFile, File keyFile, long oldKeyTime, long oldCertTime) throws IOException, GeneralSecurityException { long newKeyTime = checkNotNull(keyFile, "keyFile").lastModified(); long newCertTime = checkNotNull(certFile, "certFile").lastModified(); @@ -232,7 +285,7 @@ public final class AdvancedTlsX509KeyManager extends X509ExtendedKeyManager { FileInputStream certInputStream = new FileInputStream(certFile); try { X509Certificate[] certs = CertificateUtils.getX509Certificates(certInputStream); - updateIdentityCredentials(key, certs); + updateIdentityCredentials(certs, key); return new UpdateResult(true, newKeyTime, newCertTime); } finally { certInputStream.close(); diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java index 02813e44fe..ea9a2234f1 100644 --- a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java +++ b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java @@ -79,15 +79,15 @@ public class AdvancedTlsX509KeyManagerTest { public void credentialSetting() throws Exception { // Overall happy path checking of public API. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); - serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + serverKeyManager.updateIdentityCredentials(serverCert0, serverKey0); assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS)); assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS)); - serverKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File); + serverKeyManager.updateIdentityCredentials(clientCert0File, clientKey0File); assertEquals(clientKey0, serverKeyManager.getPrivateKey(ALIAS)); assertArrayEquals(clientCert0, serverKeyManager.getCertificateChain(ALIAS)); - serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, + serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File,1, TimeUnit.MINUTES, executor); assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS)); assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS)); @@ -98,28 +98,28 @@ public class AdvancedTlsX509KeyManagerTest { // Checking edge cases of public API parameter setting. AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); NullPointerException npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentials(null, serverCert0)); + .updateIdentityCredentials(serverCert0, null)); assertEquals("key", npe.getMessage()); npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentials(serverKey0, null)); + .updateIdentityCredentials(null, serverKey0)); assertEquals("certs", npe.getMessage()); npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentialsFromFile(null, serverCert0File)); - assertEquals("keyFile", npe.getMessage()); - - npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentialsFromFile(serverKey0File, null)); + .updateIdentityCredentials(null, serverKey0File)); assertEquals("certFile", npe.getMessage()); npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, null, + .updateIdentityCredentials(serverCert0File, null)); + assertEquals("keyFile", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentials(serverCert0File, serverKey0File, 1, null, executor)); assertEquals("unit", npe.getMessage()); npe = assertThrows(NullPointerException.class, () -> serverKeyManager - .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, + .updateIdentityCredentials(serverCert0File, serverKey0File, 1, TimeUnit.MINUTES, null)); assertEquals("executor", npe.getMessage()); @@ -128,7 +128,7 @@ public class AdvancedTlsX509KeyManagerTest { log.addHandler(handler); log.setUseParentHandlers(false); log.setLevel(Level.FINE); - serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, -1, + serverKeyManager.updateIdentityCredentials(serverCert0File, serverKey0File, -1, TimeUnit.SECONDS, executor); log.removeHandler(handler); for (LogRecord record : handler.getRecords()) {