From 781b4c4575218278614948902a6873304d29b4e4 Mon Sep 17 00:00:00 2001 From: erm-g <110920239+erm-g@users.noreply.github.com> Date: Thu, 30 May 2024 13:54:11 -0400 Subject: [PATCH] security: Stabilize AdvancedTlsX509KeyManager. (#11139) * Clean up and de-experimentalization of KeyManager * Unit tests for API validity. --- .../java/io/grpc/netty/AdvancedTlsTest.java | 31 ++-- .../grpc/util/AdvancedTlsX509KeyManager.java | 46 ++--- .../util/AdvancedTlsX509KeyManagerTest.java | 165 ++++++++++++++++++ 3 files changed, 202 insertions(+), 40 deletions(-) create mode 100644 util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java diff --git a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java index c60cb4824dd..da3e20e9ffe 100644 --- a/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java +++ b/netty/src/test/java/io/grpc/netty/AdvancedTlsTest.java @@ -45,14 +45,11 @@ import io.grpc.util.CertificateUtils; import java.io.Closeable; import java.io.File; -import java.io.IOException; import java.net.Socket; import java.security.GeneralSecurityException; -import java.security.NoSuchAlgorithmException; import java.security.PrivateKey; import java.security.cert.CertificateException; import java.security.cert.X509Certificate; -import java.security.spec.InvalidKeySpecException; import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; @@ -65,13 +62,13 @@ @RunWith(JUnit4.class) public class AdvancedTlsTest { - public static final String SERVER_0_KEY_FILE = "server0.key"; - public static final String SERVER_0_PEM_FILE = "server0.pem"; - public static final String CLIENT_0_KEY_FILE = "client.key"; - public static final String CLIENT_0_PEM_FILE = "client.pem"; - public static final String CA_PEM_FILE = "ca.pem"; - public static final String SERVER_BAD_KEY_FILE = "badserver.key"; - public static final String SERVER_BAD_PEM_FILE = "badserver.pem"; + private static final String SERVER_0_KEY_FILE = "server0.key"; + private static final String SERVER_0_PEM_FILE = "server0.pem"; + private static final String CLIENT_0_KEY_FILE = "client.key"; + private static final String CLIENT_0_PEM_FILE = "client.pem"; + private static final String CA_PEM_FILE = "ca.pem"; + private static final String SERVER_BAD_KEY_FILE = "badserver.key"; + private static final String SERVER_BAD_PEM_FILE = "badserver.pem"; private ScheduledExecutorService executor; private Server server; @@ -92,7 +89,7 @@ public class AdvancedTlsTest { @Before public void setUp() - throws NoSuchAlgorithmException, IOException, CertificateException, InvalidKeySpecException { + throws Exception { executor = Executors.newSingleThreadScheduledExecutor(); caCertFile = TestUtils.loadCert(CA_PEM_FILE); serverKey0File = TestUtils.loadCert(SERVER_0_KEY_FILE); @@ -285,11 +282,11 @@ public void trustManagerInsecurelySkipAllTest() throws Exception { new SslSocketAndEnginePeerVerifier() { @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - Socket socket) throws CertificateException { } + Socket socket) { } @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - SSLEngine engine) throws CertificateException { } + SSLEngine engine) { } }) .build(); serverTrustManager.updateTrustCredentials(caCert); @@ -310,11 +307,11 @@ public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authTy new SslSocketAndEnginePeerVerifier() { @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - Socket socket) throws CertificateException { } + Socket socket) { } @Override public void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, - SSLEngine engine) throws CertificateException { } + SSLEngine engine) { } }) .build(); clientTrustManager.updateTrustCredentials(caCert); @@ -419,7 +416,7 @@ public void onFileLoadingKeyManagerTrustManagerTest() throws Exception { } @Test - public void onFileReloadingKeyManagerBadInitialContentTest() throws Exception { + public void onFileReloadingKeyManagerBadInitialContentTest() { AdvancedTlsX509KeyManager keyManager = new AdvancedTlsX509KeyManager(); // We swap the order of key and certificates to intentionally create an exception. assertThrows(GeneralSecurityException.class, @@ -439,7 +436,7 @@ public void onFileReloadingTrustManagerBadInitialContentTest() throws Exception } @Test - public void keyManagerAliasesTest() throws Exception { + public void keyManagerAliasesTest() { AdvancedTlsX509KeyManager km = new AdvancedTlsX509KeyManager(); assertArrayEquals( new String[] {"default"}, km.getClientAliases("", null)); diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java index 1530834d609..4292107dbe7 100644 --- a/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java +++ b/util/src/main/java/io/grpc/util/AdvancedTlsX509KeyManager.java @@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkNotNull; -import io.grpc.ExperimentalApi; import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -26,7 +25,6 @@ import java.security.GeneralSecurityException; import java.security.Principal; import java.security.PrivateKey; -import java.security.cert.CertificateException; import java.security.cert.X509Certificate; import java.util.Arrays; import java.util.concurrent.ScheduledExecutorService; @@ -39,20 +37,15 @@ /** * AdvancedTlsX509KeyManager is an {@code X509ExtendedKeyManager} that allows users to configure - * advanced TLS features, such as private key and certificate chain reloading, etc. + * 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()); - - // The credential information sent to peers to prove our identity. + // Minimum allowed period for refreshing files with credential information. + private static final int MINIMUM_REFRESH_PERIOD_IN_MINUTES = 1 ; + // The credential information to be sent to peers to prove our identity. private volatile KeyInfo keyInfo; - /** - * Constructs an AdvancedTlsX509KeyManager. - */ - public AdvancedTlsX509KeyManager() throws CertificateException { } - @Override public PrivateKey getPrivateKey(String alias) { if (alias.equals("default")) { @@ -107,14 +100,17 @@ public String chooseEngineServerAlias(String keyType, Principal[] issuers, * @param certs the certificate chain that is going to be used */ public void updateIdentityCredentials(PrivateKey key, X509Certificate[] certs) { - // TODO(ZhenLian): explore possibilities to do a crypto check here. this.keyInfo = new KeyInfo(checkNotNull(key, "key"), checkNotNull(certs, "certs")); } /** * 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. + * updated. You must close the returned Closeable before calling this method again or other update + * methods ({@link AdvancedTlsX509KeyManager#updateIdentityCredentials}, {@link + * AdvancedTlsX509KeyManager#updateIdentityCredentialsFromFile(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 @@ -131,14 +127,17 @@ public Closeable updateIdentityCredentialsFromFile(File keyFile, File certFile, throw new GeneralSecurityException( "Files were unmodified before their initial update. Probably a bug."); } + if (checkNotNull(unit, "unit").toMinutes(period) < MINIMUM_REFRESH_PERIOD_IN_MINUTES) { + log.log(Level.FINE, + "Provided refresh period of {0} {1} is too small. Default value of {2} minute(s) " + + "will be used.", new Object[] {period, unit.name(), MINIMUM_REFRESH_PERIOD_IN_MINUTES}); + period = MINIMUM_REFRESH_PERIOD_IN_MINUTES; + unit = TimeUnit.MINUTES; + } final ScheduledFuture future = - executor.scheduleWithFixedDelay( + checkNotNull(executor, "executor").scheduleWithFixedDelay( new LoadFilePathExecution(keyFile, certFile), period, period, unit); - return new Closeable() { - @Override public void close() { - future.cancel(false); - } - }; + return () -> future.cancel(false); } /** @@ -190,8 +189,9 @@ public void run() { this.currentCertTime = newResult.certTime; } } catch (IOException | GeneralSecurityException e) { - log.log(Level.SEVERE, "Failed refreshing private key and certificate chain from files. " - + "Using previous ones", e); + log.log(Level.SEVERE, e, () -> String.format("Failed refreshing private key and certificate" + + " chain from files. Using previous ones (keyFile lastModified = %s, certFile " + + "lastModified = %s)", keyFile.lastModified(), certFile.lastModified())); } } } @@ -220,8 +220,8 @@ public UpdateResult(boolean success, long keyTime, long certTime) { */ private UpdateResult readAndUpdate(File keyFile, File certFile, long oldKeyTime, long oldCertTime) throws IOException, GeneralSecurityException { - long newKeyTime = keyFile.lastModified(); - long newCertTime = certFile.lastModified(); + long newKeyTime = checkNotNull(keyFile, "keyFile").lastModified(); + long newCertTime = checkNotNull(certFile, "certFile").lastModified(); // We only update when both the key and the certs are updated. if (newKeyTime != oldKeyTime && newCertTime != oldCertTime) { FileInputStream keyInputStream = new FileInputStream(keyFile); diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java new file mode 100644 index 00000000000..02813e44fe7 --- /dev/null +++ b/util/src/test/java/io/grpc/util/AdvancedTlsX509KeyManagerTest.java @@ -0,0 +1,165 @@ +/* + * Copyright 2024 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.util; + +import static org.junit.Assert.assertArrayEquals; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import io.grpc.internal.FakeClock; +import io.grpc.internal.testing.TestUtils; +import io.grpc.testing.TlsTesting; +import java.io.File; +import java.security.PrivateKey; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** Unit tests for {@link AdvancedTlsX509KeyManager}. */ +@RunWith(JUnit4.class) +public class AdvancedTlsX509KeyManagerTest { + private static final String SERVER_0_KEY_FILE = "server0.key"; + private static final String SERVER_0_PEM_FILE = "server0.pem"; + private static final String CLIENT_0_KEY_FILE = "client.key"; + private static final String CLIENT_0_PEM_FILE = "client.pem"; + private static final String ALIAS = "default"; + + private ScheduledExecutorService executor; + + private File serverKey0File; + private File serverCert0File; + private File clientKey0File; + private File clientCert0File; + + private PrivateKey serverKey0; + private X509Certificate[] serverCert0; + private PrivateKey clientKey0; + private X509Certificate[] clientCert0; + + @Before + public void setUp() throws Exception { + executor = new FakeClock().getScheduledExecutorService(); + serverKey0File = TestUtils.loadCert(SERVER_0_KEY_FILE); + serverCert0File = TestUtils.loadCert(SERVER_0_PEM_FILE); + clientKey0File = TestUtils.loadCert(CLIENT_0_KEY_FILE); + clientCert0File = TestUtils.loadCert(CLIENT_0_PEM_FILE); + serverKey0 = CertificateUtils.getPrivateKey(TlsTesting.loadCert(SERVER_0_KEY_FILE)); + serverCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(SERVER_0_PEM_FILE)); + clientKey0 = CertificateUtils.getPrivateKey(TlsTesting.loadCert(CLIENT_0_KEY_FILE)); + clientCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(CLIENT_0_PEM_FILE)); + } + + @Test + public void credentialSetting() throws Exception { + // Overall happy path checking of public API. + AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); + serverKeyManager.updateIdentityCredentials(serverKey0, serverCert0); + assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS)); + assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS)); + + serverKeyManager.updateIdentityCredentialsFromFile(clientKey0File, clientCert0File); + assertEquals(clientKey0, serverKeyManager.getPrivateKey(ALIAS)); + assertArrayEquals(clientCert0, serverKeyManager.getCertificateChain(ALIAS)); + + serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, + TimeUnit.MINUTES, executor); + assertEquals(serverKey0, serverKeyManager.getPrivateKey(ALIAS)); + assertArrayEquals(serverCert0, serverKeyManager.getCertificateChain(ALIAS)); + } + + @Test + public void credentialSettingParameterValidity() throws Exception { + // Checking edge cases of public API parameter setting. + AdvancedTlsX509KeyManager serverKeyManager = new AdvancedTlsX509KeyManager(); + NullPointerException npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentials(null, serverCert0)); + assertEquals("key", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentials(serverKey0, null)); + assertEquals("certs", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentialsFromFile(null, serverCert0File)); + assertEquals("keyFile", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentialsFromFile(serverKey0File, null)); + assertEquals("certFile", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, null, + executor)); + assertEquals("unit", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> serverKeyManager + .updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, 1, + TimeUnit.MINUTES, null)); + assertEquals("executor", npe.getMessage()); + + Logger log = Logger.getLogger(AdvancedTlsX509KeyManager.class.getName()); + TestHandler handler = new TestHandler(); + log.addHandler(handler); + log.setUseParentHandlers(false); + log.setLevel(Level.FINE); + serverKeyManager.updateIdentityCredentialsFromFile(serverKey0File, serverCert0File, -1, + TimeUnit.SECONDS, executor); + log.removeHandler(handler); + for (LogRecord record : handler.getRecords()) { + if (record.getMessage().contains("Default value of ")) { + assertTrue(true); + return; + } + } + fail("Log message related to setting default values not found"); + } + + + private static class TestHandler extends Handler { + private final List records = new ArrayList<>(); + + @Override + public void publish(LogRecord record) { + records.add(record); + } + + @Override + public void flush() { + } + + @Override + public void close() throws SecurityException { + } + + public List getRecords() { + return records; + } + } + +}