From 658cbf6cfe36bcf8364730ce3adbe7f22cbdad36 Mon Sep 17 00:00:00 2001 From: erm-g <110920239+erm-g@users.noreply.github.com> Date: Thu, 11 Jul 2024 12:11:48 -0400 Subject: [PATCH] util: Stabilize AdvancedTlsX509TrustManager --- .../util/AdvancedTlsX509TrustManager.java | 132 +++++++++----- .../util/AdvancedTlsX509TrustManagerTest.java | 162 ++++++++++++++++++ 2 files changed, 249 insertions(+), 45 deletions(-) create mode 100644 util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java diff --git a/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java b/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java index 07375edd9b8..d9a78224a4d 100644 --- a/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java +++ b/util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java @@ -16,7 +16,8 @@ package io.grpc.util; -import io.grpc.ExperimentalApi; +import static com.google.common.base.Preconditions.checkNotNull; + import java.io.File; import java.io.FileInputStream; import java.io.IOException; @@ -42,14 +43,20 @@ /** * 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. + * advanced TLS features, such as root certificate reloading and peer cert custom verification. + * The basic instantiation pattern is + * new Builder().build().useSystemDefaultTrustCerts(); + * + *

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()); + // Minimum allowed period for refreshing files with credential information. + private static final int MINIMUM_REFRESH_PERIOD_IN_MINUTES = 1; + private static final String NOT_ENOUGH_INFO_MESSAGE = + "Not enough information to validate peer. SSLEngine or Socket required."; private final Verification verification; private final SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier; @@ -57,7 +64,7 @@ public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager private volatile X509ExtendedTrustManager delegateManager = null; private AdvancedTlsX509TrustManager(Verification verification, - SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) throws CertificateException { + SslSocketAndEnginePeerVerifier socketAndEnginePeerVerifier) { this.verification = verification; this.socketAndEnginePeerVerifier = socketAndEnginePeerVerifier; } @@ -65,8 +72,7 @@ private AdvancedTlsX509TrustManager(Verification verification, @Override public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException { - throw new CertificateException( - "Not enough information to validate peer. SSLEngine or Socket required."); + throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE); } @Override @@ -90,8 +96,7 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEng @Override public void checkServerTrusted(X509Certificate[] chain, String authType) throws CertificateException { - throw new CertificateException( - "Not enough information to validate peer. SSLEngine or Socket required."); + throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE); } @Override @@ -127,6 +132,7 @@ public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreEx */ public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOException, GeneralSecurityException { + checkNotNull(trustCerts, "trustCerts"); KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); keyStore.load(null, null); int i = 1; @@ -135,8 +141,7 @@ public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOExcept keyStore.setCertificateEntry(alias, cert); i++; } - X509ExtendedTrustManager newDelegateManager = createDelegateTrustManager(keyStore); - this.delegateManager = newDelegateManager; + this.delegateManager = createDelegateTrustManager(keyStore); } private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyStore) @@ -148,9 +153,9 @@ private static X509ExtendedTrustManager createDelegateTrustManager(KeyStore keyS TrustManager[] tms = tmf.getTrustManagers(); // Iterate over the returned trust managers, looking for an instance of X509TrustManager. // If found, use that as the delegate trust manager. - for (int j = 0; j < tms.length; j++) { - if (tms[j] instanceof X509ExtendedTrustManager) { - delegateManager = (X509ExtendedTrustManager) tms[j]; + for (TrustManager tm : tms) { + if (tm instanceof X509ExtendedTrustManager) { + delegateManager = (X509ExtendedTrustManager) tm; break; } } @@ -169,8 +174,7 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss "Want certificate verification but got null or empty certificates"); } if (sslEngine == null && socket == null) { - throw new CertificateException( - "Not enough information to validate peer. SSLEngine or Socket required."); + throw new CertificateException(NOT_ENOUGH_INFO_MESSAGE); } if (this.verification != Verification.INSECURELY_SKIP_ALL_VERIFICATION) { X509ExtendedTrustManager currentDelegateManager = this.delegateManager; @@ -211,12 +215,18 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss /** * 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. + * 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 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 updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit, @@ -226,14 +236,17 @@ public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, 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(trustCertFile), period, period, unit); - return new Closeable() { - @Override public void close() { - future.cancel(false); - } - }; + return () -> future.cancel(false); } /** @@ -264,13 +277,14 @@ public void run() { try { this.currentTime = readAndUpdate(this.file, this.currentTime); } catch (IOException | GeneralSecurityException e) { - log.log(Level.SEVERE, "Failed refreshing trust CAs from file. Using previous CAs", e); + log.log(Level.SEVERE, String.format("Failed refreshing trust CAs from file. Using " + + "previous CAs (file lastModified = %s)", file.lastModified()), e); } } } /** - * Reads the trust certificates specified in the path location, and update the key store if the + * Reads the trust certificates specified in the path location, and updates the key store if the * modified time has changed since last read. * * @param trustCertFile the file on disk holding the trust certificates @@ -279,7 +293,7 @@ public void run() { */ private long readAndUpdate(File trustCertFile, long oldTime) throws IOException, GeneralSecurityException { - long newTime = trustCertFile.lastModified(); + long newTime = checkNotNull(trustCertFile, "trustCertFile").lastModified(); if (newTime == oldTime) { return oldTime; } @@ -303,27 +317,32 @@ public static Builder newBuilder() { return new Builder(); } - // The verification mode when authenticating the peer certificate. + /** + * The verification mode when authenticating the peer certificate. + */ public enum Verification { - // This is the DEFAULT and RECOMMENDED mode for most applications. - // Setting this on the client side will do the certificate and hostname verification, while - // setting this on the server side will only do the certificate verification. + /** + * This is the DEFAULT and RECOMMENDED mode for most applications. + * Setting this on the client side performs both certificate and hostname verification, while + * setting it on the server side only performs certificate verification. + */ CERTIFICATE_AND_HOST_NAME_VERIFICATION, - // This SHOULD be chosen only when you know what the implication this will bring, and have a - // basic understanding about TLS. - // It SHOULD be accompanied with proper additional peer identity checks set through - // {@code PeerVerifier}(nit: why this @code not working?). Failing to do so will leave - // applications to MITM attack. - // Also note that this will only take effect if the underlying SDK implementation invokes - // checkClientTrusted/checkServerTrusted with the {@code SSLEngine} parameter while doing - // verification. - // Setting this on either side will only do the certificate verification. + /** + * DANGEROUS: Use trusted credentials to verify the certificate, but clients will not verify the + * certificate is for the expected host. This setting is only appropriate when accompanied by + * proper additional peer identity checks set through SslSocketAndEnginePeerVerifier. Failing to + * do so will leave your applications vulnerable to MITM attacks. + * This setting has the same behavior on server-side as CERTIFICATE_AND_HOST_NAME_VERIFICATION. + */ CERTIFICATE_ONLY_VERIFICATION, - // Setting is very DANGEROUS. Please try to avoid this in a real production environment, unless - // you are a super advanced user intended to re-implement the whole verification logic on your - // own. A secure verification might include: - // 1. proper verification on the peer certificate chain - // 2. proper checks on the identity of the peer certificate + /** + * DANGEROUS: This SHOULD be used by advanced user intended to implement the entire verification + * logic themselves {@link SslSocketAndEnginePeerVerifier}) themselves. This includes:
+ * 1. Proper verification of the peer certificate chain
+ * 2. Proper checks of the identity of the peer certificate
+ * Failing to do so will leave your application without any TLS-related protection. Keep in mind + * that any loaded trust certificates will be ignored when using this mode. + */ INSECURELY_SKIP_ALL_VERIFICATION, } @@ -356,6 +375,14 @@ void verifyPeerCertificate(X509Certificate[] peerCertChain, String authType, SSL throws CertificateException; } + /** + * 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 + * (File, long, TimeUnit, ScheduledExecutorService)}. + */ public static final class Builder { private Verification verification = Verification.CERTIFICATE_AND_HOST_NAME_VERIFICATION; @@ -363,11 +390,26 @@ public static final class Builder { private Builder() {} + /** + * Sets {@link Verification}, mode when authenticating the peer certificate. By default, {@link + * Verification#CERTIFICATE_AND_HOST_NAME_VERIFICATION} value is used. + * + * @param verification Verification mode used for the current AdvancedTlsX509TrustManager + * @return Builder with set verification + */ public Builder setVerification(Verification verification) { this.verification = verification; return this; } + /** + * Sets {@link SslSocketAndEnginePeerVerifier}, which methods will be called in addition to + * verifying certificates. + * + * @param verifier SslSocketAndEnginePeerVerifier used for the current + * AdvancedTlsX509TrustManager + * @return Builder with set verifier + */ public Builder setSslSocketAndEnginePeerVerifier(SslSocketAndEnginePeerVerifier verifier) { this.socketAndEnginePeerVerifier = verifier; return this; diff --git a/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java b/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java new file mode 100644 index 00000000000..c8ca0adb5c0 --- /dev/null +++ b/util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java @@ -0,0 +1,162 @@ +/* + * 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.assertNotNull; +import static org.junit.Assert.assertThrows; + +import com.google.common.collect.Iterables; +import io.grpc.internal.FakeClock; +import io.grpc.internal.testing.TestUtils; +import io.grpc.testing.TlsTesting; +import java.io.File; +import java.io.IOException; +import java.net.Socket; +import java.security.GeneralSecurityException; +import java.security.cert.CertificateException; +import java.security.cert.X509Certificate; +import java.util.ArrayList; +import java.util.List; +import java.util.NoSuchElementException; +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 AdvancedTlsX509TrustManager}. */ +@RunWith(JUnit4.class) +public class AdvancedTlsX509TrustManagerTest { + + private static final String CA_PEM_FILE = "ca.pem"; + private static final String SERVER_0_PEM_FILE = "server0.pem"; + private File caCertFile; + private File serverCert0File; + + private X509Certificate[] caCert; + private X509Certificate[] serverCert0; + + private ScheduledExecutorService executor; + + @Before + public void setUp() throws IOException, GeneralSecurityException { + executor = new FakeClock().getScheduledExecutorService(); + caCertFile = TestUtils.loadCert(CA_PEM_FILE); + caCert = CertificateUtils.getX509Certificates(TlsTesting.loadCert(CA_PEM_FILE)); + serverCert0File = TestUtils.loadCert(SERVER_0_PEM_FILE); + serverCert0 = CertificateUtils.getX509Certificates(TlsTesting.loadCert(SERVER_0_PEM_FILE)); + } + + @Test + public void updateTrustCredentials_replacesIssuers() throws Exception { + // Overall happy path checking of public API. + AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build(); + trustManager.updateTrustCredentialsFromFile(serverCert0File); + assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers()); + + trustManager.updateTrustCredentials(caCert); + assertArrayEquals(caCert, trustManager.getAcceptedIssuers()); + + trustManager.updateTrustCredentialsFromFile(serverCert0File, 1, TimeUnit.MINUTES, + executor); + assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers()); + + trustManager.updateTrustCredentialsFromFile(serverCert0File); + assertArrayEquals(serverCert0, trustManager.getAcceptedIssuers()); + } + + @Test + public void systemDefaultDelegateManagerInstantiation() throws Exception { + AdvancedTlsX509TrustManager trustManager = AdvancedTlsX509TrustManager.newBuilder().build(); + trustManager.useSystemDefaultTrustCerts(); + CertificateException ce = assertThrows(CertificateException.class, () -> trustManager + .checkServerTrusted(serverCert0, "RSA", new Socket())); + assertEquals("socket is not a type of SSLSocket", ce.getMessage()); + } + + @Test + public void credentialSettingParameterValidity() throws Exception { + // Checking edge cases of public API parameter setting. + 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)); + assertEquals("trustCertFile", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> trustManager + .updateTrustCredentialsFromFile(caCertFile, 1, null, null)); + assertEquals("unit", npe.getMessage()); + + npe = assertThrows(NullPointerException.class, () -> trustManager + .updateTrustCredentialsFromFile(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); + log.removeHandler(handler); + try { + LogRecord logRecord = Iterables.find(handler.getRecords(), + record -> record.getMessage().contains("Default value of ")); + assertNotNull(logRecord); + } catch (NoSuchElementException e) { + throw new AssertionError("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; + } + } + +}