Return-Path: X-Original-To: apmail-activemq-commits-archive@www.apache.org Delivered-To: apmail-activemq-commits-archive@www.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id CCD7717B1A for ; Mon, 20 Apr 2015 17:08:35 +0000 (UTC) Received: (qmail 27503 invoked by uid 500); 20 Apr 2015 17:08:35 -0000 Delivered-To: apmail-activemq-commits-archive@activemq.apache.org Received: (qmail 27462 invoked by uid 500); 20 Apr 2015 17:08:35 -0000 Mailing-List: contact commits-help@activemq.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@activemq.apache.org Delivered-To: mailing list commits@activemq.apache.org Received: (qmail 27453 invoked by uid 99); 20 Apr 2015 17:08:35 -0000 Received: from git1-us-west.apache.org (HELO git1-us-west.apache.org) (140.211.11.23) by apache.org (qpsmtpd/0.29) with ESMTP; Mon, 20 Apr 2015 17:08:35 +0000 Received: by git1-us-west.apache.org (ASF Mail Server at git1-us-west.apache.org, from userid 33) id 897E0E0663; Mon, 20 Apr 2015 17:08:35 +0000 (UTC) Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit From: martyntaylor@apache.org To: commits@activemq.apache.org Message-Id: X-Mailer: ASF-Git Admin Mailer Subject: activemq git commit: https://issues.apache.org/jira/browse/AMQ-5729 - Do not log passwords on MBean method calls. Date: Mon, 20 Apr 2015 17:08:35 +0000 (UTC) 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 Authored: Tue Apr 14 17:26:49 2015 +0100 Committer: Martyn Taylor 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 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(); 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 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[] 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); + } +}