You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "aratno (via GitHub)" <gi...@apache.org> on 2024/12/06 18:49:56 UTC
[PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
aratno opened a new pull request, #3728:
URL: https://github.com/apache/cassandra/pull/3728
https://issues.apache.org/jira/browse/CASSANDRA-20128
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "bbotella (via GitHub)" <gi...@apache.org>.
bbotella commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1892384865
##########
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(", ")));
Review Comment:
I understand this is not a hot path, but following the decision on [this](https://lists.apache.org/thread/65glsjzkmpktzmns6j9wvr4nczvskx36) thread, we may want to stay away from streams in non test code?
##########
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:
Nit: `"invoke".equals(methodName)` for consistency with others?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1896150290
##########
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:
Can do, this was copied from JMXServerUtils
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1901030339
##########
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 think that's reasonable. We can enhance it later as needed.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh closed pull request #3728: CASSANDRA-20128: Support audit logging for JMX operations
URL: https://github.com/apache/cassandra/pull/3728
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#issuecomment-2568109689
Closed via https://github.com/apache/cassandra/commit/c853efffa8b173a3afe1b966456bb77db5a68883
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1890671957
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -398,4 +418,103 @@ 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)
+ {
+ Function<Object, String> fmt = obj -> {
+ if (obj == null)
+ return "null";
+ return obj.toString();
+ };
+ String argsFmt = "";
+ if (args != null)
+ argsFmt = Arrays.stream(args).map(fmt).collect(Collectors.joining(", "));
+ return String.format("%s#%s(%s)", method.getDeclaringClass().getCanonicalName(), method.getName(), argsFmt);
+ }
Review Comment:
This will reuse the same allocation, but create a new reference on the stack to the anonymous class that the lambda compiles into. Fully agree with the stylistic comment here though, updated.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1896154107
##########
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:
What's the benefit of findAvailablePort? Looks like getAutomaticallyAllocatedPort has more similar usage within test code.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1890674517
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -400,4 +418,103 @@ 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)
+ {
+ Function<Object, String> fmt = obj -> {
+ if (obj == null)
+ return "null";
+ return obj.toString();
+ };
+ String argsFmt = "";
+ if (args != null)
+ argsFmt = Arrays.stream(args).map(fmt).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 onRejection(Subject subject, Method method, Object[] args, String reason)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX REJECTION: %s due to %s", JmxFormatter.method(method, args), reason))
Review Comment:
Good point - REJECTION was from a prior version of this patch that only applied for AuthorizationProxy. Now it's much closer to a generic failure audit log. I've updated the event naming to clarify that.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1890662241
##########
test/unit/org/apache/cassandra/audit/InMemoryAuditLogger.java:
##########
@@ -25,7 +25,8 @@
public class InMemoryAuditLogger implements IAuditLogger
{
- final Queue<AuditLogEntry> inMemQueue = new LinkedList<>();
+ @VisibleForTesting
+ public final Queue<AuditLogEntry> inMemQueue = new LinkedList<>();
Review Comment:
`internalQueue()` didn't exist when I initially wrote this patch, thanks for pointing it out. Switched to use that.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1900972752
##########
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(", ")));
Review Comment:
Good catch - especially given that I wrote myself on that thread "+1 to forbidding Stream usage entirely".
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1901000437
##########
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:
I'll use the existing `org.apache.cassandra.audit.AuditLogManager#log(org.apache.cassandra.audit.AuditLogEntry, java.lang.Exception)` entrypoint
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1901031526
##########
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 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.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "JeetKunDoug (via GitHub)" <gi...@apache.org>.
JeetKunDoug commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1890431190
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -398,4 +418,103 @@ 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)
+ {
+ Function<Object, String> fmt = obj -> {
+ if (obj == null)
+ return "null";
+ return obj.toString();
+ };
+ String argsFmt = "";
+ if (args != null)
+ argsFmt = Arrays.stream(args).map(fmt).collect(Collectors.joining(", "));
+ return String.format("%s#%s(%s)", method.getDeclaringClass().getCanonicalName(), method.getName(), argsFmt);
+ }
Review Comment:
Rather than embedding `fmt` in the `method` method as a function object like this, I'd move it into its own function and just use a method reference in stead. It avoids creating the function instance each time you format a method for logging:
```suggestion
private static String method(Method method, Object[] args)
{
String argsFmt = "";
if (args != null)
argsFmt = Arrays.stream(args).map(JmxFormatter::nullOrToString).collect(Collectors.joining(", "));
return String.format("%s#%s(%s)", method.getDeclaringClass().getCanonicalName(), method.getName(), argsFmt);
}
private static String nullOrToString(Object obj)
{
if (obj == null)
return "null";
return obj.toString();
}
```
##########
src/java/org/apache/cassandra/audit/AuditLogManager.java:
##########
@@ -400,4 +418,103 @@ 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)
+ {
+ Function<Object, String> fmt = obj -> {
+ if (obj == null)
+ return "null";
+ return obj.toString();
+ };
+ String argsFmt = "";
+ if (args != null)
+ argsFmt = Arrays.stream(args).map(fmt).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 onRejection(Subject subject, Method method, Object[] args, String reason)
+ {
+ if (filter.isFiltered(AuditLogEntryCategory.JMX))
+ return;
+
+ AuditLogEntry entry = new AuditLogEntry.Builder(JMX)
+ .setOperation(String.format("JMX REJECTION: %s due to %s", JmxFormatter.method(method, args), reason))
Review Comment:
Question - is this "rejection" or just more generic "failure"? Rejection makes it sound like the only reason for a failure is because we have the AuthorizationProxy _also_ installed and it rejected the request, but I think there are lots of other reasons why this may fail. If it's more generic, I'd rename the method and update the string here.
##########
src/java/org/apache/cassandra/audit/AuditLogFilter.java:
##########
@@ -126,6 +126,20 @@ private IncludeExcludeHolder(ImmutableSet<String> includedSet, ImmutableSet<Stri
}
}
+ /**
+ * Checks whether a give AuditLogEntryCategory is filtered or not.
Review Comment:
NIT (spelling):
```suggestion
* Checks whether a given AuditLogEntryCategory is filtered or not.
```
Actually applies to the pre-existing comment below as well.
##########
test/unit/org/apache/cassandra/audit/InMemoryAuditLogger.java:
##########
@@ -25,7 +25,8 @@
public class InMemoryAuditLogger implements IAuditLogger
{
- final Queue<AuditLogEntry> inMemQueue = new LinkedList<>();
+ @VisibleForTesting
+ public final Queue<AuditLogEntry> inMemQueue = new LinkedList<>();
Review Comment:
Is there a reason you can't use the `internalQueue()` method below to do this? Although I suppose our code style does say to prefer public final members to getters, so maybe we should get rid of the method and just inline it everywhere? Regardless, let's pick one way or the other and be consistent.
##########
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 onRejection(Subject subject, Method method, Object[] args, String reason)
Review Comment:
Same comment about rejection vs. failure from AuditLogManager - I think this is just `onFailure`?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1896149923
##########
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:
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`)?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1896149923
##########
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:
How do you want the AuditLogEntry for failures to be encoded? Full stack trace, or just a string reason (or something in the middle, like `class: message`)?
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org
Re: [PR] CASSANDRA-20128: Support audit logging for JMX operations [cassandra]
Posted by "aratno (via GitHub)" <gi...@apache.org>.
aratno commented on code in PR #3728:
URL: https://github.com/apache/cassandra/pull/3728#discussion_r1896152446
##########
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'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.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org