GROOVY-12270: Map connector authentication properties onto the keys a connector consumes

JmxBuilder's connectorServer documents properties:[authenticate:true,
passwordFile:..., accessFile:...], and wrote them into the connector
environment under com.sun.management.jmxremote.* names. Those names belong
to the JDK's out-of-the-box management agent, not to a connector server.
The agent reads them and translates them into the jmx.remote.x.* names the
connector actually consumes, then installs the authenticator itself; see
sun.management.jmxremote.ConnectorBootstrap. Nothing performed that
translation here, so an operator following the documented syntax started a
connector with no authenticator at all.

Verified rather than reasoned: with the environment this class built, the
connector reported no authenticator and a credential-less client connected
and read the MBean count; with jmx.remote.x.password.file the same client
is rejected with "Authentication failed! Credentials required".

Translate the aliases, and only when authentication was requested. Add
loginConfig for JAAS, mapping to jmx.remote.x.login.config. Reject
authenticate:true with no source of credentials at all, since that asks for
authentication and would otherwise start open, which is the failure being
fixed; a caller-supplied jmx.remote.authenticator counts as such a source,
since passing a JMXAuthenticator through is the standard JSR-160 route for
custom authentication.

The com.sun.management.jmxremote.* names remain accepted as input spellings
but are no longer copied into the environment, where they mean nothing;
the ssl alias is likewise consumed to select the socket factories rather
than passed through. Three GROOVY-12119 tests asserted the presence of
those inert keys as a witness that the environment map was not discarded;
they now assert the effective configuration instead, which is what their
comments describe.

Note on urgency rather than severity: no released version has ever passed
the property map to the connector, because the building method returned
null until GROOVY-12119, which is in no GA release. There is therefore no
installed base of connectors that believe they are authenticated. What
makes this worth fixing before GA is that GROOVY-12119 leaves SSL working
while authentication silently does not, which is a quieter failure than the
wholly broken configuration it replaced.

The default remains an unauthenticated connector when no authentication is
requested. Warning on that is a separate question from this one.

Author: paulk-asert

subprojects/groovy-jmx/src/main/groovy/groovy/jmx/builder/JmxServerConnectorFactory.groovy

@@ -19,6 +19,7 @@
 package groovy.jmx.builder
 
 import javax.management.MBeanServer
+import javax.management.remote.JMXAuthenticator
 import javax.management.remote.JMXConnectorServer
 import javax.management.remote.JMXConnectorServerFactory
 import javax.management.remote.JMXServiceURL
@@ -42,11 +43,19 @@ import javax.rmi.ssl.SslRMIServerSocketFactory
  *            "authenticate":true|false,
  *            "passwordFile":"...",
  *            "accessFile":"...",
+ *            "loginConfig":"...",
  *            "sslEnabled" : true | false
  *         ...
  *        ]
  *     )
  * </pre>
+ * <p>
+ * When {@code authenticate} is true a source of credentials must be supplied, being one of
+ * {@code passwordFile}, {@code loginConfig}, or a {@code jmx.remote.authenticator} entry
+ * holding a {@link javax.management.remote.JMXAuthenticator}. A connector which was asked to
+ * authenticate but has none of these would start open, so that combination is rejected rather
+ * than accepted silently. Any other entry in {@code properties} is passed to the connector
+ * environment unaltered.
  *
  * @see javax.management.remote.JMXConnectorServer
  */
@@ -149,17 +158,36 @@ class JmxServerConnectorFactory extends AbstractFactory {
         if (!props) return null
         HashMap<String, Object> env = new HashMap<String, Object>()
 
-        // secure connection
+        // Authentication. The com.sun.management.jmxremote.* names belong to the JDK's
+        // out-of-the-box management agent, which translates them into the jmx.remote.x.*
+        // names a connector server actually consumes (see sun.management.jmxremote.
+        // ConnectorBootstrap). Nothing performs that translation here, so do it: putting the
+        // agent's names into a connector environment leaves the connector with no
+        // authenticator at all, and it accepts credential-less clients.
         def auth = props.remove("com.sun.management.jmxremote.authenticate") ?: props.remove("authenticate")
-        env.put("com.sun.management.jmxremote.authenticate", auth)
         def pFile = props.remove("com.sun.management.jmxremote.password.file") ?: props.remove("passwordFile")
-        env.put("com.sun.management.jmxremote.password.file", pFile)
         def aFile = props.remove("com.sun.management.jmxremote.access.file") ?: props.remove("accessFile")
-        env.put("com.sun.management.jmxremote.access.file", aFile)
+        def loginConfig = props.remove("com.sun.management.jmxremote.login.config") ?: props.remove("loginConfig")
+
+        if (Boolean.valueOf(auth?.toString())) {
+            // A caller may instead pass a JMXAuthenticator straight through, which is the
+            // standard JSR-160 route for custom authentication and is a credential source too.
+            // Validate the value, not just the key: a present-but-null (or wrong-typed) entry is
+            // not a credential source and would leave the connector unauthenticated.
+            boolean suppliedAuthenticator = props.get(JMXConnectorServer.AUTHENTICATOR) instanceof JMXAuthenticator
+            if (!pFile && !loginConfig && !suppliedAuthenticator) {
+                throw new JmxBuilderException("Connector authentication was requested but no source " +
+                        "of credentials was provided; supply 'passwordFile', 'loginConfig' or a " +
+                        "'${JMXConnectorServer.AUTHENTICATOR}' entry, otherwise the connector would " +
+                        "start unauthenticated.")
+            }
+            if (pFile) env.put("jmx.remote.x.password.file", pFile)
+            if (loginConfig) env.put("jmx.remote.x.login.config", loginConfig)
+            if (aFile) env.put("jmx.remote.x.access.file", aFile)
+        }
 
         // SSL connection
         def ssl = props.remove("com.sun.management.jmxremote.ssl") ?: props.remove("sslEnabled")
-        env.put("com.sun.management.jmxremote.ssl", ssl)
 
         // config other rmi props
         if (protocol == "rmi") {

subprojects/groovy-jmx/src/test/groovy/groovy/jmx/builder/JmxServerConnectorFactoryTest.groovy

@@ -23,7 +23,11 @@ import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.extension.ExtendWith
 
+import static groovy.test.GroovyAssert.shouldFail
+
+import javax.management.remote.JMXAuthenticator
 import javax.management.remote.JMXConnector
+import javax.management.remote.JMXConnectorServer
 import javax.management.remote.JMXConnectorFactory
 import javax.management.remote.JMXServiceURL
 import javax.management.remote.rmi.RMIConnectorServer
@@ -48,6 +52,113 @@ class JmxServerConnectorFactoryTest {
         JmxConnectorHelper.destroyRmiRegistry(rmi.registry)
     }
 
+    // GROOVY-12270: authentication properties must land on the keys a connector server
+    // consumes, not on the JDK management agent's names, which it ignores.
+    @Test
+    void testAuthenticationPropertiesMapToConsumedKeys() {
+        def factory = new JmxServerConnectorFactory()
+        def env = factory.confiConnectorProperties('rmi', rmi.port,
+                [authenticate: true, passwordFile: 'pwd.properties', accessFile: 'access.properties'])
+
+        assert env['jmx.remote.x.password.file'] == 'pwd.properties'
+        assert env['jmx.remote.x.access.file'] == 'access.properties'
+        assert !env.containsKey('com.sun.management.jmxremote.password.file')
+        assert !env.containsKey('com.sun.management.jmxremote.access.file')
+    }
+
+    @Test
+    void testLoginConfigMapsToConsumedKey() {
+        def factory = new JmxServerConnectorFactory()
+        def env = factory.confiConnectorProperties('rmi', rmi.port,
+                [authenticate: true, loginConfig: 'MyLoginModule'])
+
+        assert env['jmx.remote.x.login.config'] == 'MyLoginModule'
+    }
+
+    // Credentials are only configured when authentication was actually asked for.
+    @Test
+    void testCredentialsIgnoredWhenAuthenticationNotRequested() {
+        def factory = new JmxServerConnectorFactory()
+        def env = factory.confiConnectorProperties('rmi', rmi.port,
+                [authenticate: false, passwordFile: 'pwd.properties'])
+
+        assert !env.containsKey('jmx.remote.x.password.file')
+    }
+
+    // Asking for authentication without any source of credentials would start an open
+    // connector, which is the failure this ticket is about, so it is rejected.
+    @Test
+    void testAuthenticationWithoutCredentialSourceIsRejected() {
+        def factory = new JmxServerConnectorFactory()
+        def ex = shouldFail(JmxBuilderException) {
+            factory.confiConnectorProperties('rmi', rmi.port, [authenticate: true])
+        }
+        assert ex.message.contains('passwordFile')
+        assert ex.message.contains('loginConfig')
+    }
+
+    // A caller may supply their own JMXAuthenticator instead of a password file; that is the
+    // standard JSR-160 route and must count as a source of credentials.
+    @Test
+    void testCallerSuppliedAuthenticatorSatisfiesAuthenticationRequest() {
+        def factory = new JmxServerConnectorFactory()
+        def authenticator = { env -> new javax.security.auth.Subject() } as JMXAuthenticator
+        def env = factory.confiConnectorProperties('rmi', rmi.port,
+                [authenticate: true, (JMXConnectorServer.AUTHENTICATOR): authenticator])
+
+        assert env[JMXConnectorServer.AUTHENTICATOR].is(authenticator)
+    }
+
+    // A present-but-null (or wrong-typed) authenticator entry is not a credential source; it must
+    // be rejected rather than pass the check on the key's presence alone and leave the connector
+    // unauthenticated.
+    @Test
+    void testNullAuthenticatorIsNotACredentialSource() {
+        def factory = new JmxServerConnectorFactory()
+        def ex = shouldFail(JmxBuilderException) {
+            factory.confiConnectorProperties('rmi', rmi.port,
+                    [authenticate: true, (JMXConnectorServer.AUTHENTICATOR): null])
+        }
+        assert ex.message.contains(JMXConnectorServer.AUTHENTICATOR)
+
+        shouldFail(JmxBuilderException) {
+            factory.confiConnectorProperties('rmi', rmi.port,
+                    [authenticate: true, (JMXConnectorServer.AUTHENTICATOR): 'not-an-authenticator'])
+        }
+    }
+
+    // End-to-end: a connector configured to authenticate must reject a credential-less client.
+    @Test
+    void testAuthenticatedConnectorRejectsAnonymousClient() {
+        File dir = File.createTempDir()
+        try {
+            File password = new File(dir, 'jmxremote.password')
+            password.text = 'probeuser probepass\n'
+            File access = new File(dir, 'jmxremote.access')
+            access.text = 'probeuser readwrite\n'
+            [password, access].each { it.setReadable(false, false); it.setReadable(true, true) }
+
+            def server = builder.serverConnector(port: rmi.port,
+                    properties: [authenticate: true, passwordFile: password.path, accessFile: access.path])
+            server.start()
+            try {
+                JMXServiceURL url = new JMXServiceURL("service:jmx:rmi:///jndi/rmi://localhost:${rmi.port}/jmxrmi")
+                shouldFail(SecurityException) {
+                    JMXConnectorFactory.connect(url, null).withCloseable { it.MBeanServerConnection.MBeanCount }
+                }
+                // ...and accept the configured one.
+                def creds = [(JMXConnector.CREDENTIALS): ['probeuser', 'probepass'] as String[]]
+                JMXConnectorFactory.connect(url, creds).withCloseable {
+                    assert it.MBeanServerConnection.MBeanCount > 0
+                }
+            } finally {
+                server.stop()
+            }
+        } finally {
+            dir.deleteDir()
+        }
+    }
+
     @Test
     void testJmxServerConnectorNode() {
         RMIConnectorServer result = builder.serverConnector(port: rmi.port)
@@ -81,8 +192,11 @@ class JmxServerConnectorFactoryTest {
         def env = factory.confiConnectorProperties('rmi', rmi.port, [authenticate: false])
 
         assert env != null : 'connector environment map must not be discarded'
-        // supplied/derived entries are present
-        assert env.containsKey('com.sun.management.jmxremote.authenticate')
+        // GROOVY-12270: the com.sun.management.jmxremote.* names belong to the JDK management
+        // agent and mean nothing in a connector environment, so they are no longer copied into
+        // it; authentication was not requested here, so nothing is configured for it.
+        assert !env.containsKey('com.sun.management.jmxremote.authenticate')
+        assert !env.containsKey('jmx.remote.x.password.file')
     }
 
     // GROOVY-12119: when SSL is requested the env map must carry the SSL socket factories
@@ -92,7 +206,8 @@ class JmxServerConnectorFactoryTest {
         def env = factory.confiConnectorProperties('rmi', rmi.port, [sslEnabled: true])
 
         assert env != null
-        assert env['com.sun.management.jmxremote.ssl']
+        // The socket factories are what actually enable SSL; see GROOVY-12270 for why the
+        // com.sun.management.jmxremote.ssl key itself is no longer placed in the environment.
         assert env[RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE] instanceof SslRMIServerSocketFactory
         assert env[RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE] instanceof SslRMIClientSocketFactory
     }
@@ -127,7 +242,7 @@ class JmxServerConnectorFactoryTest {
         def env = factory.confiConnectorProperties('rmi', rmi.port, ['com.sun.management.jmxremote.ssl': true])
 
         assert env != null
-        assert env['com.sun.management.jmxremote.ssl']
+        // Recognition is evidenced by the socket factories being configured from it.
         assert env[RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE] instanceof SslRMIServerSocketFactory
     }