frankgh commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1893289300
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -400,4 +419,104 @@ else if (e instanceof
PasswordGuardrail.PasswordGuardrailException)
return PasswordObfuscator.obfuscate(e.getMessage());
}
+
+ private static class JmxFormatter
+ {
+ private static String user(Subject subject)
+ {
+ return String.format("%s", subject == null ? null :
subject.getPrincipals().stream().map(Objects::toString).collect(Collectors.joining(",
")));
+ }
+
+ private static String method(Method method, Object[] args)
+ {
+ String argsFmt = "";
+ if (args != null)
+ argsFmt =
Arrays.stream(args).map(Objects::toString).collect(Collectors.joining(", "));
+ return String.format("%s#%s(%s)",
method.getDeclaringClass().getCanonicalName(), method.getName(), argsFmt);
+ }
+ }
+
+ @Override
+ public void onInvocation(Subject subject, Method method, Object[] args)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX INVOCATION:
%s", JmxFormatter.method(method, args)))
+ .setUser(JmxFormatter.user(subject))
+ .build();
+ log(entry);
+ }
+
+ @Override
+ public void onFailure(Subject subject, Method method, Object[] args,
String reason)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX FAILURE: %s due
to %s", JmxFormatter.method(method, args), reason))
+ .setUser(JmxFormatter.user(subject))
+ .build();
+ log(entry);
+ }
+
+
+ private class JmxHandler implements InvocationHandler
+ {
+ private MBeanServer mbs = null;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable
+ {
+ // See AuthorizationProxy.invoke
+ if (("setMBeanServer").equals(method.getName()))
Review Comment:
nit: remove redundant unnecessary parentheses
```suggestion
if ("setMBeanServer".equals(method.getName()))
```
##########
test/unit/org/apache/cassandra/audit/AuditLoggerTest.java:
##########
@@ -758,6 +790,108 @@ public void testJMXArchiveCommand() throws IOException
assertEquals("/xyz/not/null",
AuditLogManager.instance.getAuditLogOptions().archive_command);
}
+ @Test
+ public void testJMXAuditLogs() throws Throwable
+ {
+ // Need to use distinct ports, otherwise would get RMI registry object
ID collision, even with server shutdown between
+ testJMXAuditLogs(false,
getAutomaticallyAllocatedPort(InetAddress.getLoopbackAddress()));
Review Comment:
I was going to suggest using `SocketUtils.findAvailablePort` but it looks
like it always requires a fall back port number. Maybe we can change the
signature of that method to take an Integer object instead of the primitive,
and reuse the same code?
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -400,4 +419,104 @@ else if (e instanceof
PasswordGuardrail.PasswordGuardrailException)
return PasswordObfuscator.obfuscate(e.getMessage());
}
+
+ private static class JmxFormatter
+ {
+ private static String user(Subject subject)
+ {
+ return String.format("%s", subject == null ? null :
subject.getPrincipals().stream().map(Objects::toString).collect(Collectors.joining(",
")));
+ }
+
+ private static String method(Method method, Object[] args)
+ {
+ String argsFmt = "";
+ if (args != null)
+ argsFmt =
Arrays.stream(args).map(Objects::toString).collect(Collectors.joining(", "));
+ return String.format("%s#%s(%s)",
method.getDeclaringClass().getCanonicalName(), method.getName(), argsFmt);
+ }
+ }
+
+ @Override
+ public void onInvocation(Subject subject, Method method, Object[] args)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX INVOCATION:
%s", JmxFormatter.method(method, args)))
+ .setUser(JmxFormatter.user(subject))
+ .build();
+ log(entry);
+ }
+
+ @Override
+ public void onFailure(Subject subject, Method method, Object[] args,
String reason)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX FAILURE: %s due
to %s", JmxFormatter.method(method, args), reason))
+ .setUser(JmxFormatter.user(subject))
+ .build();
+ log(entry);
+ }
+
+
+ private class JmxHandler implements InvocationHandler
+ {
+ private MBeanServer mbs = null;
+
+ @Override
+ public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable
+ {
+ // See AuthorizationProxy.invoke
+ if (("setMBeanServer").equals(method.getName()))
+ {
+ if (args[0] == null)
+ throw new IllegalArgumentException("Null MBeanServer");
+
+ if (mbs != null)
+ throw new IllegalArgumentException("MBeanServer already
initialized");
+
+ mbs = (MBeanServer) args[0];
+ return null;
+ }
+
+ AccessControlContext acc = AccessController.getContext();
+ Subject subject = Subject.getSubject(acc);
+
+ try
+ {
+ Object invoke = method.invoke(mbs, args);
+ AuditLogManager.this.onInvocation(subject, method, args);
+ return invoke;
+ }
+ catch (InvocationTargetException e)
+ {
+ Throwable cause = e.getCause();
+ AuditLogManager.instance.onFailure(subject, method, args,
cause.getMessage());
+ throw cause;
+ }
+ }
+ }
+
+ private MBeanServerForwarder createMBeanServerForwarder()
+ {
+ final InvocationHandler handler = new JmxHandler();
+ final Class<?>[] interfaces = { MBeanServerForwarder.class };
Review Comment:
Avoid using final for variables
```suggestion
InvocationHandler handler = new JmxHandler();
Class<?>[] interfaces = { MBeanServerForwarder.class };
```
##########
src/java/org/apache/cassandra/utils/JmxInvocationListener.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.cassandra.utils;
+
+import java.lang.reflect.Method;
+import javax.security.auth.Subject;
+
+public interface JmxInvocationListener
+{
+ default void onInvocation(Subject subject, Method method, Object[] args)
+ {}
+
+ default void onFailure(Subject subject, Method method, Object[] args,
String reason)
Review Comment:
should we pass the cause here instead of the string reason? The idea is so
we can add the cause to the log entry during failure , i.e. `log(entry, cause);`
##########
src/java/org/apache/cassandra/utils/JmxInvocationListener.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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 org.apache.cassandra.utils;
+
+import java.lang.reflect.Method;
+import javax.security.auth.Subject;
+
+public interface JmxInvocationListener
Review Comment:
can we add javadocs on public interfaces and methods ?
##########
src/java/org/apache/cassandra/auth/jmx/AuthorizationProxy.java:
##########
@@ -140,43 +142,57 @@ public class AuthorizationProxy implements
InvocationHandler
*/
protected BooleanSupplier isAuthSetupComplete = () ->
StorageService.instance.isAuthSetupComplete();
+ protected JmxInvocationListener listener = AuditLogManager.instance;
+
@Override
public Object invoke(Object proxy, Method method, Object[] args)
throws Throwable
{
String methodName = method.getName();
- if ("getMBeanServer".equals(methodName))
- throw new SecurityException("Access denied");
-
- // Corresponds to MBeanServer.invoke
- if (methodName.equals("invoke") && args.length == 4)
- checkVulnerableMethods(args);
-
// Retrieve Subject from current AccessControlContext
AccessControlContext acc = AccessController.getContext();
Subject subject = Subject.getSubject(acc);
- // Allow setMBeanServer iff performed on behalf of the connector
server itself
- if (("setMBeanServer").equals(methodName))
+ try
{
- if (subject != null)
+ if ("getMBeanServer".equals(methodName))
throw new SecurityException("Access denied");
- if (args[0] == null)
- throw new IllegalArgumentException("Null MBeanServer");
+ // Corresponds to MBeanServer.invoke
+ if (methodName.equals("invoke") && args.length == 4)
Review Comment:
I think the only real change here is that we wrap the existing code in a
try/catch block so we can propagate the success/failure event . The code is
exactly the same as before, so I think it's fine to keep. The only actual
behavior change is that we are doing
```
AccessControlContext acc = AccessController.getContext();
Subject subject = Subject.getSubject(acc);
```
before we do the first two checks. I think that's fine
##########
src/java/org/apache/cassandra/audit/AuditLogEntryType.java:
##########
@@ -71,7 +71,8 @@ public enum AuditLogEntryType
LOGIN_ERROR(AuditLogEntryCategory.AUTH),
UNAUTHORIZED_ATTEMPT(AuditLogEntryCategory.AUTH),
LOGIN_SUCCESS(AuditLogEntryCategory.AUTH),
- LIST_SUPERUSERS(AuditLogEntryCategory.DCL);
+ LIST_SUPERUSERS(AuditLogEntryCategory.DCL),
+ JMX(AuditLogEntryCategory.JMX);
Review Comment:
I'm thinking we should split this into JMX_SUCCESS and JMX_ERROR. I can see
use cases where you might only be interested in audit logs for failed JMX
operations. What do you think?
--
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]
