gemmellr commented on code in PR #6024:
URL: https://github.com/apache/activemq-artemis/pull/6024#discussion_r2498732461
##########
artemis-server/src/test/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModuleTest.java:
##########
@@ -76,13 +84,45 @@ public void setLoginConfigSysProperty() {
System.setProperty(loginConfigSysPropName,
"src/test/resources/login.config");
}
+ @Before
Review Comment:
There is already an existing `@Before` method above. The ordering is not
guaranteed so I'd suggest having 1 method that either has it all or calls the
other methods (got bit by order changes during the prior JUnit 5 updates..still
to be done with this class).
EDIT: alternatively, since it is seemingly only stopping in an
`@AfterClass`, should this just be a `@BeforeClass` ? Should the other one also
be class level (same issue with having multiples)?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModuleTest.java:
##########
@@ -373,4 +458,123 @@ private boolean presentIn(Set<LDAPLoginProperty>
ldapProps, String propertyName)
return false;
}
+ @Test
+ public void testLDAPLoginSSLSocketFactoryWithTruststore() throws Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("truststorePath",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.jks")).getFile());
+ extraOptions.put("truststorePassword", "securepass");
+
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, true);
+ }
+
+ @Test
+ public void
testLDAPLoginSSLSocketFactoryWithDefaultPasswordCodecAndTruststore() throws
Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("truststorePath",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.jks")).getFile());
+ extraOptions.put("truststorePassword", PasswordMaskingUtil.wrap(
+ PasswordMaskingUtil.getDefaultCodec().encode("securepass")));
+
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, true);
+ }
+
+ @Test
+ public void
testLDAPLoginSSLSocketFactoryWithCustomPasswordCodecAndTruststore() throws
Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("passwordCodec",
TestSensitiveDataCodec.class.getName());
+ extraOptions.put("truststorePath",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.jks")).getFile());
+ extraOptions.put("truststorePassword", "ENC(ssaperuces)");
+
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, true);
+ }
+
+ @Test
+ public void testLDAPLoginSSLSocketFactoryWithInvalidTruststore() throws
Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("truststorePath", "invalid-ca-truststore.jks");
+ extraOptions.put("truststorePassword", "securepass");
+
+ try {
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, true);
+ fail("Should have thrown CommunicationException");
+ } catch (Exception e) {
+ assertEquals(CommunicationException.class, e.getClass());
+ }
+ }
+
+ @Test
+ public void testLDAPLoginSSLSocketFactoryWithTrustAll() throws Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("trustAll", "true");
+
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, true);
+ }
+
+ @Test
+ public void testLDAPLoginSSLSocketFactoryWithMultipleLDAPLoginModules()
throws Exception {
+ Map<String, Object> extraOptions = new HashMap<>();
+ extraOptions.put("truststorePath",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.jks")).getFile());
+ extraOptions.put("truststorePassword", "securepass");
+
+ try {
+ testLDAPSConnectionWithLDAPLoginSSLSocketFactory(extraOptions, false);
+
+ try {
+
testLDAPSConnectionWithLDAPLoginSSLSocketFactory(Collections.emptyMap(), false);
+ fail("Should have thrown CommunicationException");
+ } catch (Exception e) {
+ assertEquals(CommunicationException.class, e.getClass());
+ }
+
+
testLDAPSConnectionWithLDAPLoginSSLSocketFactory(Collections.singletonMap("trustAll",
"true"), false);
+ } finally {
+ LDAPLoginSSLSocketFactory.getSSLContextFactory().clearSSLContexts();
+ }
+ }
+
+ private void testLDAPSConnectionWithLDAPLoginSSLSocketFactory(Map<String,
Object> extraOptions, boolean clearSSLContexts) throws Exception {
+ Map<String, Object> options = new HashMap<>();
+
+ // Set basic LDAP connection options for LDAPS
+ options.put("initialContextFactory", "com.sun.jndi.ldap.LdapCtxFactory");
+ options.put("connectionURL", "ldaps://localhost:1025");
+ options.put("connectionUsername", PRINCIPAL);
+ options.put("connectionPassword", CREDENTIALS);
+ options.put("authentication", "simple");
+ options.put("connectionProtocol", "ssl");
+
+ // Set SSL configuration options
+ options.put("java.naming.ldap.factory.socket",
LDAPLoginSSLSocketFactory.class.getName());
+ options.putAll(extraOptions);
+
+ LDAPLoginModule loginModule = new LDAPLoginModule();
+ loginModule.initialize(new Subject(), null, null, options);
+
+ try {
+ assertNull("The environment should be cleared before opening
context", LDAPLoginModule.getEnvironment());
+ loginModule.openContext();
+ assertNull("The environment should be cleared after opening context",
LDAPLoginModule.getEnvironment());
+ } finally {
+ loginModule.closeContext();
+ if (clearSSLContexts) {
+
LDAPLoginSSLSocketFactory.getSSLContextFactory().clearSSLContexts();
+ }
Review Comment:
Should the clear go first to ensure it happens if the other ever throws (I
see right now it shouldnt...but..)
##########
docs/user-manual/security.adoc:
##########
@@ -806,6 +814,107 @@ Define a `member` attribute for each member of the
role/group, setting the `memb
If you want to add roles to user entries, you would need to customize the
directory schema, by adding a suitable attribute type to the user entry's
object class.
The chosen attribute type must be capable of handling multiple values.
+===== LDAPLoginSSLSocketFactory
+
+To secure the connection to your LDAP server using SSL/TLS, you can configure
`LDAPLoginModule` to use `LDAPLoginSSLSocketFactory`.
+This socket factory provides comprehensive SSL configuration options including
keystore, truststore, and certificate validation settings.
+
+To use `LDAPLoginSSLSocketFactory`, you need to:
+
+1. Enable SSL/TLS by setting the property `java.naming.security.protocol` to
'ssl' or by using `ldaps://` protocol in your `connectionURL`.
+2. Set `java.naming.ldap.factory.socket` to
`org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginSSLSocketFactory`.
+3. Configure the SSL-related options as described below.
+
+Here's an example configuration in `login.config`:
+
+----
+LDAPLogin {
+ org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginModule required
+ debug=false
+ initialContextFactory=com.sun.jndi.ldap.LdapCtxFactory
+ connectionURL="ldaps://ldapserver:636"
+
java.naming.ldap.factory.socket=org.apache.activemq.artemis.spi.core.security.jaas.LDAPLoginSSLSocketFactory
+ connectionUsername="uid=admin,ou=system"
+ connectionPassword="secret"
+ userBase="ou=User,ou=ActiveMQ,ou=system"
+ userSearchMatching="(uid={0})"
+ userSearchSubtree=false
+ roleBase="ou=Group,ou=ActiveMQ,ou=system"
+ roleName=cn
+ roleSearchMatching="(member=uid={1})"
+ roleSearchSubtree=true
+ truststorePath="/path/to/truststore.jks"
+ truststorePassword="trustpass"
+ keystorePath="/path/to/keystore.jks"
+ keystorePassword="keypass";
+};
+----
+
+The following SSL configuration options are supported:
+
+keyStoreProvider::
+The provider used for the keystore.
+For example, `SUN`, `SunJCE`, etc.
+Default is `null`.
+
+keystoreType::
+The type of keystore being used.
+For example, `JKS`, `JCEKS`, `PKCS12`, `PEM` etc.
+Default is `JKS`.
+
+keystorePath::
+The path to the keystore file containing the client certificate and private
key.
+This is only required if client certificate authentication is needed.
+Default is `null`.
+
+keystorePassword::
+The password for the keystore file.
+Supports xref:masking-passwords.adoc#masking-passwords[password masking].
+Default is `null`.
+
+keystoreAlias::
+The alias of the certificate to use from the keystore if multiple certificates
exist.
+Default is `null`.
+
+truststoreProvider::
+The provider used for the truststore.
+For example, `SUN`, `SunJCE`, etc.
+Default is `null`.
+
+truststoreType::
+The type of truststore being used.
+For example, `JKS`, `JCEKS`, `PKCS12`, `PEM` etc.
+Default is `JKS`.
Review Comment:
May be worth moving PKCS12 towards the start of the list, since although
"JKS" is still widely used as a default setting for historic reasons and
compatibility, PKCS12 has been the effective default since Java 9
(https://openjdk.org/jeps/229 , the loader detects and handles both JKS and
PKCS12 regardless of the setting [unless configured not to])
##########
artemis-server/src/test/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModuleTest.java:
##########
@@ -111,6 +151,44 @@ public void testRunning() throws Exception {
}
+ @Test
+ public void testRunningSSL() throws Exception {
+ Hashtable<String, Object> env = new Hashtable<>();
+ env.put(Context.PROVIDER_URL, "ldaps://localhost:1025");
+ env.put(Context.INITIAL_CONTEXT_FACTORY,
"com.sun.jndi.ldap.LdapCtxFactory");
+ env.put(Context.SECURITY_AUTHENTICATION, "simple");
+ env.put(Context.SECURITY_PRINCIPAL, PRINCIPAL);
+ env.put(Context.SECURITY_CREDENTIALS, CREDENTIALS);
+
+ // Uncomment to enable SSL debugging
+ // System.setProperty("-Djavax.net.debug", "all");
+ System.setProperty("javax.net.ssl.trustStore",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.p12")).getFile());
+ System.setProperty("javax.net.ssl.trustStorePassword", "securepass");
+
+ DirContext ctx = new InitialDirContext(env);
+
+ System.clearProperty("javax.net.ssl.trustStore");
+ System.clearProperty("javax.net.ssl.trustStorePassword");
+
+ Set<String> set = new HashSet<>();
+
+ NamingEnumeration<NameClassPair> list = ctx.list("ou=system");
+
+ while (list.hasMore()) {
+ NameClassPair ncp = list.next();
+ set.add(ncp.getName());
+ }
+
+ assertTrue(set.contains("uid=admin"));
+ assertTrue(set.contains("ou=users"));
+ assertTrue(set.contains("ou=groups"));
+ assertTrue(set.contains("ou=configuration"));
+ assertTrue(set.contains("prefNodeName=sysPrefRoot"));
+
+ ctx.close();
Review Comment:
Maybe a try-finally?
##########
artemis-server/src/test/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModuleTest.java:
##########
@@ -111,6 +151,44 @@ public void testRunning() throws Exception {
}
+ @Test
+ public void testRunningSSL() throws Exception {
+ Hashtable<String, Object> env = new Hashtable<>();
+ env.put(Context.PROVIDER_URL, "ldaps://localhost:1025");
+ env.put(Context.INITIAL_CONTEXT_FACTORY,
"com.sun.jndi.ldap.LdapCtxFactory");
+ env.put(Context.SECURITY_AUTHENTICATION, "simple");
+ env.put(Context.SECURITY_PRINCIPAL, PRINCIPAL);
+ env.put(Context.SECURITY_CREDENTIALS, CREDENTIALS);
+
+ // Uncomment to enable SSL debugging
+ // System.setProperty("-Djavax.net.debug", "all");
+ System.setProperty("javax.net.ssl.trustStore",
Objects.requireNonNull(this.getClass().
+ getClassLoader().getResource("server-ca-truststore.p12")).getFile());
+ System.setProperty("javax.net.ssl.trustStorePassword", "securepass");
+
+ DirContext ctx = new InitialDirContext(env);
+
+ System.clearProperty("javax.net.ssl.trustStore");
+ System.clearProperty("javax.net.ssl.trustStorePassword");
Review Comment:
Maybe a try-finally?
##########
artemis-server/src/main/java/org/apache/activemq/artemis/spi/core/security/jaas/LDAPLoginModule.java:
##########
@@ -71,6 +71,20 @@ public class LDAPLoginModule implements AuditLoginModule {
private static final Logger logger =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+ private static final ThreadLocal<Map<String, String>>
environmentThreadLocal = new ThreadLocal<>();
Review Comment:
Whilst multiple LDAPs seems less likely (or at least, theyd have to all
share the same stores etc like they do via the system properties
currently)...you know once you have per-module config someone is likely going
to try using different config for different ones. So it should probably either
support that as this looks to, or at the minimum enforce they are the same if
not supporting them differing.
--
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]
For further information, visit: https://activemq.apache.org/contact
