CASSANDRA-20128: Support audit logging for JMX operations #3728

aratno · 2024-12-06T18:49:53Z

https://issues.apache.org/jira/browse/CASSANDRA-20128

JeetKunDoug

Some questions/suggestions, but glad to see the audit logging applied to JMX as well. Thanks!

src/java/org/apache/cassandra/audit/AuditLogFilter.java

src/java/org/apache/cassandra/audit/AuditLogManager.java

src/java/org/apache/cassandra/utils/JmxInvocationListener.java

test/unit/org/apache/cassandra/audit/InMemoryAuditLogger.java

bbotella · 2024-12-19T14:53:52Z

src/java/org/apache/cassandra/audit/AuditLogManager.java

+    {
+        private static String user(Subject subject)
+        {
+            return String.format("%s", subject == null ? null : subject.getPrincipals().stream().map(Objects::toString).collect(Collectors.joining(", ")));


I understand this is not a hot path, but following the decision on this thread, we may want to stay away from streams in non test code?

Good catch - especially given that I wrote myself on that thread "+1 to forbidding Stream usage entirely".

bbotella · 2024-12-19T14:54:56Z

src/java/org/apache/cassandra/auth/jmx/AuthorizationProxy.java

-            if (args[0] == null)
-                throw new IllegalArgumentException("Null MBeanServer");
+            // Corresponds to MBeanServer.invoke
+            if (methodName.equals("invoke") && args.length == 4)


Nit: "invoke".equals(methodName) for consistency with others?

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

frankgh

Looks good overall, I have some general comments. Let me know what you think

frankgh · 2024-12-20T00:16:12Z

src/java/org/apache/cassandra/utils/JmxInvocationListener.java

+import java.lang.reflect.Method;
+import javax.security.auth.Subject;
+
+public interface JmxInvocationListener


can we add javadocs on public interfaces and methods ?

frankgh · 2024-12-20T00:20:09Z

src/java/org/apache/cassandra/audit/AuditLogManager.java

+        public Object invoke(Object proxy, Method method, Object[] args) throws Throwable
+        {
+            // See AuthorizationProxy.invoke
+            if (("setMBeanServer").equals(method.getName()))


nit: remove redundant unnecessary parentheses

Suggested change

if (("setMBeanServer").equals(method.getName()))

if ("setMBeanServer".equals(method.getName()))

frankgh · 2024-12-20T00:24:42Z

src/java/org/apache/cassandra/utils/JmxInvocationListener.java

+    default void onInvocation(Subject subject, Method method, Object[] args)
+    {}
+
+    default void onFailure(Subject subject, Method method, Object[] args, String reason)


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);

How do you want failures to be presented in AuditLogEntry? Full stack trace, or just a string reason (or something in the middle, like class: message)?

I'll use the existing org.apache.cassandra.audit.AuditLogManager#log(org.apache.cassandra.audit.AuditLogEntry, java.lang.Exception) entrypoint

frankgh · 2024-12-20T00:27:48Z

src/java/org/apache/cassandra/audit/AuditLogManager.java

+        final InvocationHandler handler = new JmxHandler();
+        final Class<?>[] interfaces = { MBeanServerForwarder.class };


Avoid using final for variables

Suggested change

final InvocationHandler handler = new JmxHandler();

final Class<?>[] interfaces = { MBeanServerForwarder.class };

InvocationHandler handler = new JmxHandler();

Class<?>[] interfaces = { MBeanServerForwarder.class };

Can do, this was copied from JMXServerUtils

frankgh · 2024-12-20T00:33:26Z

src/java/org/apache/cassandra/auth/jmx/AuthorizationProxy.java

-            if (args[0] == null)
-                throw new IllegalArgumentException("Null MBeanServer");
+            // Corresponds to MBeanServer.invoke
+            if (methodName.equals("invoke") && args.length == 4)


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

frankgh · 2024-12-20T00:36:30Z

test/unit/org/apache/cassandra/audit/AuditLoggerTest.java

+    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()));


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?

What's the benefit of findAvailablePort? Looks like getAutomaticallyAllocatedPort has more similar usage within test code.

I think the benefit is that the code already exists under the test code, and that we avoid duplication. However since the existing method doesn't fit exactly the requirement, I think it's fine to keep the new method.

frankgh · 2024-12-20T00:41:07Z

src/java/org/apache/cassandra/audit/AuditLogEntryType.java

    LOGIN_SUCCESS(AuditLogEntryCategory.AUTH),
-    LIST_SUPERUSERS(AuditLogEntryCategory.DCL);
+    LIST_SUPERUSERS(AuditLogEntryCategory.DCL),
+    JMX(AuditLogEntryCategory.JMX);


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?

I'd rather start with a basic on / off flag for now, and we can make this more refined once it gets used. There's room for more specific inclusion / exclusion criteria, like excluding "boilerplate" GetClassLoader calls made by the JMX client, etc.

I think that's reasonable. We can enhance it later as needed.

frankgh

+1

bbotella

+1! (nb)

frankgh · 2025-01-02T17:18:35Z

Closed via c853eff

aratno force-pushed the CASSANDRA-20128-trunk branch 3 times, most recently from 8b8d068 to f8ac711 Compare December 18, 2024 15:30

JeetKunDoug reviewed Dec 18, 2024

View reviewed changes

CASSANDRA-20128: Support audit logging for JMX operations

86f6015

aratno force-pushed the CASSANDRA-20128-trunk branch from f8ac711 to 86f6015 Compare December 18, 2024 17:58

aratno added 2 commits December 18, 2024 13:35

Address PR feedback

405cb68

Update changelog

1b6dd5e

bbotella reviewed Dec 19, 2024

View reviewed changes

frankgh reviewed Dec 20, 2024

View reviewed changes

aratno added 2 commits January 2, 2025 10:10

Remove usage of streams

bb75fb2

Address PR feedback

bfcabb0

frankgh approved these changes Jan 2, 2025

View reviewed changes

bbotella approved these changes Jan 2, 2025

View reviewed changes

frankgh closed this Jan 2, 2025

	if (("setMBeanServer").equals(method.getName()))
	if ("setMBeanServer".equals(method.getName()))

		final InvocationHandler handler = new JmxHandler();
		final Class<?>[] interfaces = { MBeanServerForwarder.class };

CASSANDRA-20128: Support audit logging for JMX operations #3728

CASSANDRA-20128: Support audit logging for JMX operations #3728

Uh oh!

Conversation

aratno commented Dec 6, 2024

Uh oh!

JeetKunDoug left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aratno Dec 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frankgh left a comment

Choose a reason for hiding this comment

Uh oh!

bbotella left a comment

Choose a reason for hiding this comment

Uh oh!

frankgh commented Jan 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aratno Dec 23, 2024 •

edited

Loading