snleee commented on code in PR #12325:
URL: https://github.com/apache/pinot/pull/12325#discussion_r1468937073
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java:
##########
@@ -322,6 +351,119 @@ public static SslContext buildServerContext(TlsConfig
tlsConfig) {
}
}
+ /**
+ * check if the key store or trust store path is null or has file scheme.
+ *
+ * @param keyOrTrustStorePath key store or trust store path in String format.
+ */
+ public static boolean isKeyOrTrustStorePathNullOrHasFileScheme(String
keyOrTrustStorePath) {
+ try {
+ return keyOrTrustStorePath == null
+ ||
makeKeyOrTrustStoreUrl(keyOrTrustStorePath).toURI().getScheme().startsWith(FILE_SCHEME);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Enables auto renewal of SSLFactory when
+ * 1. the {@link SSLFactory} is created with a key manager and trust manager
swappable
+ * 2. the key store is null or a local file
+ * 3. the trust store is null or a local file
+ * 4. the key store or trust store file changes.
+ * @param sslFactory the {@link SSLFactory} to enable key manager and trust
manager auto renewal
+ * @param tlsConfig the {@link TlsConfig} to get the key store and trust
store information
+ */
+ public static void enableAutoRenewalFromFileStoreForSSLFactory(SSLFactory
sslFactory, TlsConfig tlsConfig) {
+ enableAutoRenewalFromFileStoreForSSLFactory(sslFactory,
+ tlsConfig.getKeyStoreType(), tlsConfig.getKeyStorePath(),
tlsConfig.getKeyStorePassword(),
+ tlsConfig.getTrustStoreType(), tlsConfig.getTrustStorePath(),
tlsConfig.getTrustStorePassword(),
+ null, null);
+ }
+
+ private static void enableAutoRenewalFromFileStoreForSSLFactory(
+ SSLFactory sslFactory,
+ String keyStoreType, String keyStorePath, String keyStorePassword,
+ String trustStoreType, String trustStorePath, String trustStorePassword,
+ String sslContextProtocol, SecureRandom secureRandom) {
+ try {
+ URL keyStoreURL = keyStorePath == null ? null :
makeKeyOrTrustStoreUrl(keyStorePath);
+ URL trustStoreURL = trustStorePath == null ? null :
makeKeyOrTrustStoreUrl(trustStorePath);
+ if (keyStoreURL != null) {
+ Preconditions.checkArgument(
+ keyStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "key store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getKeyManager().isPresent()
+ && sslFactory.getKeyManager().get() instanceof
HotSwappableX509ExtendedKeyManager,
+ "key manager of the existing SSLFactory must be swappable"
+ );
+ }
+ if (trustStoreURL != null) {
+ Preconditions.checkArgument(
+ trustStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "trust store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getTrustManager().isPresent()
+ && sslFactory.getTrustManager().get() instanceof
HotSwappableX509ExtendedTrustManager,
+ "trust manager of the existing SSLFactory must be swappable"
+ );
+ }
+ // The reloadSslFactoryWhenFileStoreChanges is a blocking call, so we
need to create a new thread to run it.
+ // Creating a new thread to run the reloadSslFactoryWhenFileStoreChanges
is costly; however, unless we
+ // invoke the createAutoRenewedSSLFactoryFromFileStore method crazily,
this should not be a problem.
+ Executors.newSingleThreadExecutor().execute(() -> {
+ try {
+ reloadSslFactoryWhenFileStoreChanges(sslFactory,
+ keyStoreType, keyStorePath, keyStorePassword,
+ trustStoreType, trustStorePath, trustStorePassword,
+ sslContextProtocol, secureRandom);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ @VisibleForTesting
+ static void reloadSslFactoryWhenFileStoreChanges(SSLFactory baseSslFactory,
+ String keyStoreType, String keyStorePath, String keyStorePassword,
+ String trustStoreType, String trustStorePath, String trustStorePassword,
+ String sslContextProtocol, SecureRandom secureRandom)
+ throws IOException, URISyntaxException, InterruptedException {
+ WatchService watchService = FileSystems.getDefault().newWatchService();
Review Comment:
Interesting approach :) I had a question on how we detect the change for the
local files and found this code. It's a clever approach 👍 (I thought of keeping
modified time or crc)
##########
pinot-common/src/test/java/org/apache/pinot/common/utils/TlsUtilsTest.java:
##########
@@ -140,4 +167,119 @@ private static void
assertSSLTrustManagersEqual(TrustManager tm1, TrustManager t
assertEquals(x509TrustManager1.getAcceptedIssuers().length, 1);
assertEquals(x509TrustManager1.getAcceptedIssuers()[0],
x509TrustManager2.getAcceptedIssuers()[0]);
}
+
+ @Test
+ public void reloadSslFactoryWhenFileStoreChanges()
+ throws IOException, URISyntaxException, InterruptedException {
+ SecureRandom secureRandom = new SecureRandom();
+ SSLFactory sslFactory = TlsUtils.createSSLFactory(KEYSTORE_TYPE,
TLS_KEYSTORE_FILE_PATH, PASSWORD, TRUSTSTORE_TYPE,
+ TLS_TRUSTSTORE_FILE_PATH, PASSWORD, "TLS", secureRandom, true);
+ X509ExtendedKeyManager x509ExtendedKeyManager =
sslFactory.getKeyManager().get();
+ X509ExtendedTrustManager x509ExtendedTrustManager =
sslFactory.getTrustManager().get();
+ SSLContext sslContext = sslFactory.getSslContext();
+
+ PrivateKey privateKey =
x509ExtendedKeyManager.getPrivateKey(KEY_NAME_ALIAS);
+ Certificate certForPrivateKey =
x509ExtendedKeyManager.getCertificateChain(KEY_NAME_ALIAS)[0];
+ X509Certificate acceptedIssuerForCert =
x509ExtendedTrustManager.getAcceptedIssuers()[0];
+
+ // start a new thread to reload the ssl factory when the tls files change
+ ExecutorService executorService = Executors.newSingleThreadExecutor();
+ executorService.execute(
+ () -> {
+ try {
+ TlsUtils.reloadSslFactoryWhenFileStoreChanges(sslFactory,
KEYSTORE_TYPE, TLS_KEYSTORE_FILE_PATH, PASSWORD,
+ TRUSTSTORE_TYPE, TLS_TRUSTSTORE_FILE_PATH, PASSWORD, "TLS",
secureRandom);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ });
+
+ WatchService watchService = FileSystems.getDefault().newWatchService();
+ Map<WatchKey, Set<Path>> watchKeyPathMap = new HashMap<>();
+ TlsUtils.registerFile(watchService, watchKeyPathMap,
TLS_KEYSTORE_FILE_PATH);
+ TlsUtils.registerFile(watchService, watchKeyPathMap,
TLS_TRUSTSTORE_FILE_PATH);
+
+ // wait for the thread to start
+ Thread.sleep(1000);
Review Comment:
1 sec may be too long?
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java:
##########
@@ -322,6 +351,119 @@ public static SslContext buildServerContext(TlsConfig
tlsConfig) {
}
}
+ /**
+ * check if the key store or trust store path is null or has file scheme.
+ *
+ * @param keyOrTrustStorePath key store or trust store path in String format.
+ */
+ public static boolean isKeyOrTrustStorePathNullOrHasFileScheme(String
keyOrTrustStorePath) {
+ try {
+ return keyOrTrustStorePath == null
+ ||
makeKeyOrTrustStoreUrl(keyOrTrustStorePath).toURI().getScheme().startsWith(FILE_SCHEME);
+ } catch (Exception e) {
+ throw new RuntimeException(e);
+ }
+ }
+
+ /**
+ * Enables auto renewal of SSLFactory when
+ * 1. the {@link SSLFactory} is created with a key manager and trust manager
swappable
+ * 2. the key store is null or a local file
+ * 3. the trust store is null or a local file
+ * 4. the key store or trust store file changes.
+ * @param sslFactory the {@link SSLFactory} to enable key manager and trust
manager auto renewal
+ * @param tlsConfig the {@link TlsConfig} to get the key store and trust
store information
+ */
+ public static void enableAutoRenewalFromFileStoreForSSLFactory(SSLFactory
sslFactory, TlsConfig tlsConfig) {
+ enableAutoRenewalFromFileStoreForSSLFactory(sslFactory,
+ tlsConfig.getKeyStoreType(), tlsConfig.getKeyStorePath(),
tlsConfig.getKeyStorePassword(),
+ tlsConfig.getTrustStoreType(), tlsConfig.getTrustStorePath(),
tlsConfig.getTrustStorePassword(),
+ null, null);
+ }
+
+ private static void enableAutoRenewalFromFileStoreForSSLFactory(
+ SSLFactory sslFactory,
+ String keyStoreType, String keyStorePath, String keyStorePassword,
+ String trustStoreType, String trustStorePath, String trustStorePassword,
+ String sslContextProtocol, SecureRandom secureRandom) {
+ try {
+ URL keyStoreURL = keyStorePath == null ? null :
makeKeyOrTrustStoreUrl(keyStorePath);
+ URL trustStoreURL = trustStorePath == null ? null :
makeKeyOrTrustStoreUrl(trustStorePath);
+ if (keyStoreURL != null) {
+ Preconditions.checkArgument(
+ keyStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "key store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getKeyManager().isPresent()
+ && sslFactory.getKeyManager().get() instanceof
HotSwappableX509ExtendedKeyManager,
+ "key manager of the existing SSLFactory must be swappable"
+ );
+ }
+ if (trustStoreURL != null) {
+ Preconditions.checkArgument(
+ trustStoreURL.toURI().getScheme().startsWith(FILE_SCHEME),
+ "trust store path must be a local file path or null when SSL auto
renew is enabled");
+ Preconditions.checkArgument(
+ sslFactory.getTrustManager().isPresent()
+ && sslFactory.getTrustManager().get() instanceof
HotSwappableX509ExtendedTrustManager,
+ "trust manager of the existing SSLFactory must be swappable"
+ );
+ }
+ // The reloadSslFactoryWhenFileStoreChanges is a blocking call, so we
need to create a new thread to run it.
+ // Creating a new thread to run the reloadSslFactoryWhenFileStoreChanges
is costly; however, unless we
+ // invoke the createAutoRenewedSSLFactoryFromFileStore method crazily,
this should not be a problem.
+ Executors.newSingleThreadExecutor().execute(() -> {
Review Comment:
I checked the usage of `createAutoRenewedSSLFactoryFromFileStore()` and we
may hit this for every single query request? (Since we call it when we
initialize `GrpcQueryClient`. It looks that we will create `GrpcQueryClient`
every time we hit broker query api)
If that's the case, this can hurt the broker (imagine that we get 1000s of
queries per sec). Should we consider to add a background periodic task that
monitors the ssl files and make the change if needed?
Let's double check if that's the case and think about the better solution.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@pinot.apache.org
For additional commands, e-mail: commits-h...@pinot.apache.org
