You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@activemq.apache.org by ma...@apache.org on 2015/04/20 19:08:35 UTC
activemq git commit: https://issues.apache.org/jira/browse/AMQ-5729 -
Do not log passwords on MBean method calls.
Repository: activemq
Updated Branches:
refs/heads/master 514496eba -> a65ac586c
https://issues.apache.org/jira/browse/AMQ-5729 - Do not log passwords on MBean method calls.
Previous to this patch the AnnotatedMBean class would simply dump
any arguments passed in via JMX call to the log (when audit is enabled).
Method parameters can sometimes contain sensitive information such as
the password field on QueueView.sendTextMessage.
This patch adds a @Sensitive annotation to the JMX module allowing
implementations of MBean interfaces to mark method parameters as sensitive
preventing values from being logged.
Project: http://git-wip-us.apache.org/repos/asf/activemq/repo
Commit: http://git-wip-us.apache.org/repos/asf/activemq/commit/a65ac586
Tree: http://git-wip-us.apache.org/repos/asf/activemq/tree/a65ac586
Diff: http://git-wip-us.apache.org/repos/asf/activemq/diff/a65ac586
Branch: refs/heads/master
Commit: a65ac586c203d08b6a68b07eedd7ae28a63b58a6
Parents: 514496e
Author: Martyn Taylor <mt...@redhat.com>
Authored: Tue Apr 14 17:26:49 2015 +0100
Committer: Martyn Taylor <mt...@redhat.com>
Committed: Mon Apr 20 18:06:45 2015 +0100
----------------------------------------------------------------------
.../activemq/broker/jmx/AnnotatedMBean.java | 30 +++-
.../activemq/broker/jmx/DestinationView.java | 4 +-
.../apache/activemq/broker/jmx/Sensitive.java | 34 +++++
.../activemq/broker/util/AuditLogEntry.java | 32 ++++-
.../apache/activemq/jmx/JmxAuditLogTest.java | 138 +++++++++++++++++++
5 files changed, 234 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/activemq/blob/a65ac586/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/AnnotatedMBean.java
----------------------------------------------------------------------
diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/AnnotatedMBean.java b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/AnnotatedMBean.java
index 8207627..a2bdd62 100644
--- a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/AnnotatedMBean.java
+++ b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/AnnotatedMBean.java
@@ -191,10 +191,38 @@ public class AnnotatedMBean extends StandardMBean {
entry.setUser(caller);
entry.setTimestamp(System.currentTimeMillis());
entry.setOperation(this.getMBeanInfo().getClassName() + "." + s);
- entry.getParameters().put("arguments", objects);
+
+ try
+ {
+ if (objects.length == strings.length)
+ {
+ Method m = getMBeanMethod(this.getImplementationClass(), s, strings);
+ entry.getParameters().put("arguments", AuditLogEntry.sanitizeArguments(objects, m));
+ }
+ else
+ {
+ // Supplied Method Signature and Arguments do not match. Set all supplied Arguments in Log Entry. To diagnose user error.
+ entry.getParameters().put("arguments", objects);
+ }
+ }
+ catch (ReflectiveOperationException e)
+ {
+ // Method or Class not found, set all supplied arguments. Set all supplied Arguments in Log Entry. To diagnose user error.
+ entry.getParameters().put("arguments", objects);
+ }
auditLog.log(entry);
}
return super.invoke(s, objects, strings);
}
+
+ private Method getMBeanMethod(Class clazz, String methodName, String[] signature) throws ReflectiveOperationException
+ {
+ Class[] parameterTypes = new Class[signature.length];
+ for (int i = 0; i < signature.length; i++)
+ {
+ parameterTypes[i] = Class.forName(signature[i]);
+ }
+ return clazz.getMethod(methodName, parameterTypes);
+ }
}
http://git-wip-us.apache.org/repos/asf/activemq/blob/a65ac586/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java
----------------------------------------------------------------------
diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java
index bf9d0d5..b3bf869 100644
--- a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java
+++ b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/DestinationView.java
@@ -335,12 +335,12 @@ public class DestinationView implements DestinationViewMBean {
}
@Override
- public String sendTextMessage(String body, String user, String password) throws Exception {
+ public String sendTextMessage(String body, String user, @Sensitive String password) throws Exception {
return sendTextMessage(Collections.EMPTY_MAP, body, user, password);
}
@Override
- public String sendTextMessage(Map<String, String> headers, String body, String userName, String password) throws Exception {
+ public String sendTextMessage(Map<String, String> headers, String body, String userName, @Sensitive String password) throws Exception {
String brokerUrl = "vm://" + broker.getBrokerName();
ActiveMQDestination dest = destination.getActiveMQDestination();
http://git-wip-us.apache.org/repos/asf/activemq/blob/a65ac586/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/Sensitive.java
----------------------------------------------------------------------
diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/Sensitive.java b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/Sensitive.java
new file mode 100644
index 0000000..83cae8d
--- /dev/null
+++ b/activemq-broker/src/main/java/org/apache/activemq/broker/jmx/Sensitive.java
@@ -0,0 +1,34 @@
+/**
+ * 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.activemq.broker.jmx;
+
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Retention(RetentionPolicy.RUNTIME)
+@Target(ElementType.PARAMETER)
+
+/**
+ * Sensitive annotation, allows a method parameter to be marked as sensitive. This will prevent this method being
+ * logged during audit. For example a user password sent via JMX call on sendTextMessage.
+ */
+public @interface Sensitive
+{
+}
http://git-wip-us.apache.org/repos/asf/activemq/blob/a65ac586/activemq-broker/src/main/java/org/apache/activemq/broker/util/AuditLogEntry.java
----------------------------------------------------------------------
diff --git a/activemq-broker/src/main/java/org/apache/activemq/broker/util/AuditLogEntry.java b/activemq-broker/src/main/java/org/apache/activemq/broker/util/AuditLogEntry.java
index 6644310..9f4df32 100644
--- a/activemq-broker/src/main/java/org/apache/activemq/broker/util/AuditLogEntry.java
+++ b/activemq-broker/src/main/java/org/apache/activemq/broker/util/AuditLogEntry.java
@@ -17,11 +17,15 @@
package org.apache.activemq.broker.util;
+import java.lang.annotation.Annotation;
+import java.lang.reflect.Method;
import java.text.SimpleDateFormat;
import java.util.Date;
import java.util.HashMap;
import java.util.Map;
+import org.apache.activemq.broker.jmx.Sensitive;
+
public class AuditLogEntry {
protected String user = "anonymous";
@@ -76,4 +80,30 @@ public class AuditLogEntry {
public void setParameters(Map<String, Object> parameters) {
this.parameters = parameters;
}
-}
\ No newline at end of file
+
+ /**
+ * Method to remove any sensitive parameters before logging. Replaces any sensitive value with ****. Sensitive
+ * values are defined on MBean interface implementation method parameters using the @Sensitive annotation.
+ *
+ * @param arguments A array of arguments to test against method signature
+ * @param method The method to test the arguments against.
+ */
+ public static Object[] sanitizeArguments(Object[] arguments, Method method)
+ {
+ Object[] sanitizedArguments = arguments.clone();
+ Annotation[][] parameterAnnotations = method.getParameterAnnotations();
+
+ for (int i = 0; i < arguments.length; i++)
+ {
+ for (Annotation annotation : parameterAnnotations[i])
+ {
+ if (annotation instanceof Sensitive)
+ {
+ sanitizedArguments[i] = "****";
+ break;
+ }
+ }
+ }
+ return sanitizedArguments;
+ }
+}
http://git-wip-us.apache.org/repos/asf/activemq/blob/a65ac586/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxAuditLogTest.java
----------------------------------------------------------------------
diff --git a/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxAuditLogTest.java b/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxAuditLogTest.java
new file mode 100644
index 0000000..6ad570a
--- /dev/null
+++ b/activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxAuditLogTest.java
@@ -0,0 +1,138 @@
+/**
+ * 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.activemq.jmx;
+
+import javax.management.MBeanServerConnection;
+import javax.management.ObjectName;
+import javax.management.remote.JMXConnector;
+import javax.management.remote.JMXConnectorFactory;
+import javax.management.remote.JMXServiceURL;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+import org.apache.activemq.TestSupport;
+import org.apache.activemq.broker.BrokerService;
+import org.apache.activemq.broker.jmx.ManagementContext;
+import org.apache.activemq.command.ActiveMQDestination;
+import org.apache.activemq.command.ActiveMQQueue;
+import org.apache.activemq.util.DefaultTestAppender;
+import org.apache.log4j.Appender;
+import org.apache.log4j.Level;
+import org.apache.log4j.Logger;
+import org.apache.log4j.spi.LoggingEvent;
+import org.junit.Test;
+
+public class JmxAuditLogTest extends TestSupport
+{
+ protected BrokerService broker;
+
+ protected ActiveMQQueue queue;
+
+ @Override
+ protected void setUp() throws Exception
+ {
+ super.setUp();
+ setMaxTestTime(TimeUnit.MINUTES.toMillis(10));
+ setAutoFail(true);
+
+ System.setProperty("org.apache.activemq.audit", "true");
+
+ broker = new BrokerService();
+ broker.setUseJmx(true);
+ broker.setManagementContext(createManagementContext("broker", 1099));
+ broker.setPopulateUserNameInMBeans(true);
+ broker.setDestinations(createDestinations());
+ broker.start();
+ }
+
+ @Override
+ protected void tearDown() throws Exception
+ {
+ System.clearProperty("org.apache.activemq.audit");
+ broker.stop();
+ super.tearDown();
+ }
+
+ protected ActiveMQDestination[] createDestinations()
+ {
+ queue = new ActiveMQQueue("myTestQueue");
+ return new ActiveMQDestination[] {queue};
+ }
+
+ private MBeanServerConnection createJMXConnector(int port) throws Exception
+ {
+ String url = "service:jmx:rmi:///jndi/rmi://localhost:" + port + "/jmxrmi";
+
+ Map env = new HashMap<String, String>();
+ String[] creds = {"admin", "activemq"};
+ env.put(JMXConnector.CREDENTIALS, creds);
+
+ JMXConnector connector = JMXConnectorFactory.connect(new JMXServiceURL(url), env);
+ connector.connect();
+ return connector.getMBeanServerConnection();
+ }
+
+ private ManagementContext createManagementContext(String name, int port)
+ {
+ ManagementContext managementContext = new ManagementContext();
+ managementContext.setBrokerName(name);
+ managementContext.setConnectorPort(port);
+ managementContext.setConnectorHost("localhost");
+ managementContext.setCreateConnector(true);
+
+ Map<String, String> env = new HashMap<String, String>();
+ env.put("jmx.remote.x.password.file", basedir + "/src/test/resources/jmx.password");
+ env.put("jmx.remote.x.access.file", basedir + "/src/test/resources/jmx.access");
+ managementContext.setEnvironment(env);
+ return managementContext;
+ }
+
+ @Test
+ public void testPasswordsAreNotLoggedWhenAuditIsTurnedOn() throws Exception
+ {
+ Logger log4jLogger = Logger.getLogger("org.apache.activemq.audit");
+ log4jLogger.setLevel(Level.INFO);
+
+ Appender appender = new DefaultTestAppender()
+ {
+ @Override
+ public void doAppend(LoggingEvent event)
+ {
+ if (event.getMessage() instanceof String)
+ {
+ String message = (String) event.getMessage();
+ if (message.contains("testPassword"))
+ {
+ fail("Password should not appear in log file");
+ }
+ }
+ }
+ };
+ log4jLogger.addAppender(appender);
+
+ MBeanServerConnection conn = createJMXConnector(1099);
+ ObjectName queueObjName = new ObjectName(broker.getBrokerObjectName() + ",destinationType=Queue,destinationName=" + queue.getQueueName());
+
+ Object[] params = {"body", "testUser", "testPassword"};
+ String[] signature = {"java.lang.String", "java.lang.String", "java.lang.String"};
+
+ conn.invoke(queueObjName, "sendTextMessage", params, signature);
+ log4jLogger.removeAppender(appender);
+ }
+}