frankgh commented on code in PR #2693:
URL: https://github.com/apache/cassandra/pull/2693#discussion_r1332175038
##########
src/java/org/apache/cassandra/transport/PipelineConfigurator.java:
##########
@@ -198,7 +201,9 @@ protected void decode(ChannelHandlerContext
channelHandlerContext, ByteBuf byteB
return channel -> {
SslContext sslContext =
SSLFactory.getOrCreateSslContext(encryptionOptions,
encryptionOptions.require_client_auth,
-
ISslContextFactory.SocketType.SERVER);
+
ISslContextFactory.SocketType.SERVER,
+
SSL_FACTORY_CONTEXT_DESCRIPTION);
+ InetSocketAddress peer =
encryptionOptions.require_endpoint_verification ? (InetSocketAddress)
channel.remoteAddress() : null;
Review Comment:
is this needed for something?
##########
src/java/org/apache/cassandra/net/MessagingServiceMBeanImpl.java:
##########
@@ -278,10 +276,7 @@ public boolean isBackPressureEnabled()
@Override
public void reloadSslCertificates() throws IOException
Review Comment:
`IOException` is no longer thrown, so it can be removed.
##########
test/unit/org/apache/cassandra/security/SSLFactoryTest.java:
##########
@@ -296,9 +311,38 @@ public void
testCacheKeyInequalityForCustomSslContextFactory() {
.withSslContextFactory(new
ParameterizedClass(DummySslContextFactoryImpl.class.getName(), parameters2))
.withProtocol("TLSv1.1");
- SSLFactory.CacheKey cacheKey2 = new
SSLFactory.CacheKey(encryptionOptions2, ISslContextFactory.SocketType.SERVER
+ SSLFactory.CacheKey cacheKey2 = new
SSLFactory.CacheKey(encryptionOptions2, ISslContextFactory.SocketType.SERVER,
"test"
);
Assert.assertNotEquals(cacheKey1, cacheKey2);
}
+
+ public static class TestFileBasedSSLContextFactory extends
FileBasedSslContextFactory {
Review Comment:
unused?
##########
src/java/org/apache/cassandra/security/SSLFactory.java:
##########
@@ -175,42 +177,52 @@ static SslContext createNettySslContext(EncryptionOptions
options, boolean verif
* @throws IllegalStateException if {@link
#initHotReloading(EncryptionOptions.ServerEncryptionOptions, EncryptionOptions,
boolean)}
* is not called first
*/
- public static void
checkCertFilesForHotReloading(EncryptionOptions.ServerEncryptionOptions
serverOpts,
- EncryptionOptions
clientOpts)
+ public static void checkCertFilesForHotReloading()
{
if (!isHotReloadingInitialized)
throw new IllegalStateException("Hot reloading functionality has
not been initialized.");
+ checkCachedContextsForReload(false);
+ }
- logger.debug("Checking whether certificates have been updated for
server {} and client {}",
-
serverOpts.sslContextFactoryInstance.getClass().getName(),
clientOpts.sslContextFactoryInstance.getClass().getName());
-
- if (serverOpts != null)
- {
- checkCertFilesForHotReloading(serverOpts,
"server_encryption_options", true);
- }
- if (clientOpts != null)
- {
- checkCertFilesForHotReloading(clientOpts,
"client_encryption_options", clientOpts.require_client_auth);
- }
+ /**
+ * Forces revalidation and loading of SSL certifcates if valid
+ */
+ public static void forceCheckCertFiles()
+ {
+ checkCachedContextsForReload(true);
}
- private static void checkCertFilesForHotReloading(EncryptionOptions
options, String contextDescription,
- boolean
verifyPeerCertificate)
+ private static void checkCachedContextsForReload(boolean forceReload)
{
- try
- {
- if (options.sslContextFactoryInstance.shouldReload())
+ List<CacheKey> keys = new
ArrayList<>(Collections.list(cachedSslContexts.keys()));
+ Set<EncryptionOptions> checked = new HashSet<>();
+
+ keys.forEach(key -> {
+ final EncryptionOptions opts = key.encryptionOptions;
+ if (checked.contains(opts)) // only check each encryption option
once as shared for different types
+ return;
+ checked.add(key.encryptionOptions);
+
+ logger.debug("Checking whether certificates have been updated for
{}", key.contextDescription);
+ if (forceReload || opts.sslContextFactoryInstance.shouldReload())
{
- logger.info("SSL certificates have been updated for {}.
Resetting the ssl contexts for new " +
- "connections.", options.getClass().getName());
- validateSslContext(contextDescription, options,
verifyPeerCertificate, false);
- clearSslContextCache(options);
+ try
+ {
+ validateSslContext(key.contextDescription, opts,
+ opts instanceof
EncryptionOptions.ServerEncryptionOptions ? true
+
: opts.require_client_auth, false);
Review Comment:
This should simplify a bit:
```suggestion
opts instanceof
EncryptionOptions.ServerEncryptionOptions || opts.require_client_auth, false);
```
##########
src/java/org/apache/cassandra/security/SSLFactory.java:
##########
@@ -175,42 +177,52 @@ static SslContext createNettySslContext(EncryptionOptions
options, boolean verif
* @throws IllegalStateException if {@link
#initHotReloading(EncryptionOptions.ServerEncryptionOptions, EncryptionOptions,
boolean)}
* is not called first
*/
- public static void
checkCertFilesForHotReloading(EncryptionOptions.ServerEncryptionOptions
serverOpts,
- EncryptionOptions
clientOpts)
+ public static void checkCertFilesForHotReloading()
{
if (!isHotReloadingInitialized)
throw new IllegalStateException("Hot reloading functionality has
not been initialized.");
+ checkCachedContextsForReload(false);
+ }
- logger.debug("Checking whether certificates have been updated for
server {} and client {}",
-
serverOpts.sslContextFactoryInstance.getClass().getName(),
clientOpts.sslContextFactoryInstance.getClass().getName());
-
- if (serverOpts != null)
- {
- checkCertFilesForHotReloading(serverOpts,
"server_encryption_options", true);
- }
- if (clientOpts != null)
- {
- checkCertFilesForHotReloading(clientOpts,
"client_encryption_options", clientOpts.require_client_auth);
- }
+ /**
+ * Forces revalidation and loading of SSL certifcates if valid
+ */
+ public static void forceCheckCertFiles()
+ {
+ checkCachedContextsForReload(true);
}
- private static void checkCertFilesForHotReloading(EncryptionOptions
options, String contextDescription,
- boolean
verifyPeerCertificate)
+ private static void checkCachedContextsForReload(boolean forceReload)
{
- try
- {
- if (options.sslContextFactoryInstance.shouldReload())
+ List<CacheKey> keys = new
ArrayList<>(Collections.list(cachedSslContexts.keys()));
+ Set<EncryptionOptions> checked = new HashSet<>();
+
+ keys.forEach(key -> {
+ final EncryptionOptions opts = key.encryptionOptions;
+ if (checked.contains(opts)) // only check each encryption option
once as shared for different types
+ return;
+ checked.add(key.encryptionOptions);
Review Comment:
we can simplify this a bit:
```suggestion
if (!checked.add(key.encryptionOptions)) // only check each
encryption option once as shared for different types
return;
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
