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/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);
+    }
 }

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

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
+{
+}

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;
     }
-}
+
+   /**
+    * 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;
+    }
+}

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