From a65ac586c203d08b6a68b07eedd7ae28a63b58a6 Mon Sep 17 00:00:00 2001 From: Martyn Taylor Date: Tue, 14 Apr 2015 17:26:49 +0100 Subject: [PATCH] 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. --- .../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(-) create mode 100644 activemq-broker/src/main/java/org/apache/activemq/broker/jmx/Sensitive.java create mode 100644 activemq-unit-tests/src/test/java/org/apache/activemq/jmx/JmxAuditLogTest.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 8207627645..a2bdd6241c 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); + } } 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 bf9d0d528e..b3bf86995d 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 headers, String body, String userName, String password) throws Exception { + public String sendTextMessage(Map headers, String body, String userName, @Sensitive String password) throws Exception { String brokerUrl = "vm://" + broker.getBrokerName(); ActiveMQDestination dest = destination.getActiveMQDestination(); 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 0000000000..83cae8d4b7 --- /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 +{ +} 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 6644310ed2..9f4df32d89 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 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; + } +} 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 0000000000..6ad570a08c --- /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[] 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 env = new HashMap(); + 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); + } +}