QPID-445 Added checks to ensure UserManagement MBean is only accessed by Admin users.

Allow Qpid JMX Management console to manage access file.


git-svn-id: https://svn.apache.org/repos/asf/incubator/qpid/branches/M2@530798 13f79535-47bb-0310-9956-ffa450edef68

5 files changed, 135 insertions, 75 deletions

diff --git a/java/broker/src/main/java/org/apache/qpid/server/management/JMXManagedObjectRegistry.java b/java/broker/src/main/java/org/apache/qpid/server/management/JMXManagedObjectRegistry.java
index 38c9e4950a..a14d4214e3 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/management/JMXManagedObjectRegistry.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/management/JMXManagedObjectRegistry.java
@@ -83,7 +83,7 @@ public class JMXManagedObjectRegistry implements ManagedObjectRegistry
     }
 
 
-    public void start()
+    public void start() throws IOException
     {
         // Check if the "QPID_OPTS" is set to use Out of the Box JMXAgent
         if (areOutOfTheBoxJMXOptionsSet())
@@ -97,76 +97,60 @@ public class JMXManagedObjectRegistry implements ManagedObjectRegistry
         boolean security = appRegistry.getConfiguration().getBoolean("management.security-enabled", true);
         int port = appRegistry.getConfiguration().getInt("management.jmxport", 8999);
 
-        try
+        if (security)
         {
-            if (security)
-            {
-                // For SASL using JMXMP
-                _jmxURL = new JMXServiceURL("jmxmp", null, port);
-
-                Map env = new HashMap();
-                Map<String, PrincipalDatabase> map = appRegistry.getDatabaseManager().getDatabases();
-                PrincipalDatabase db = null;
-                
-                for (Map.Entry<String, PrincipalDatabase> entry : map.entrySet())
-                {
-                    if (entry.getValue() instanceof Base64MD5PasswordFilePrincipalDatabase)
-                    {
-                        db = entry.getValue();
-                        break;
-                    }
-                    else if (entry.getValue() instanceof PlainPasswordFilePrincipalDatabase)
-                    {
-                        db = entry.getValue();
-                    }
-                }
-
-                if (db instanceof Base64MD5PasswordFilePrincipalDatabase)
-                {
-                    env.put("jmx.remote.profiles", "SASL/CRAM-MD5");
-                    CRAMMD5HashedInitialiser initialiser = new CRAMMD5HashedInitialiser();
-                    initialiser.initialise(db);
-                    env.put("jmx.remote.sasl.callback.handler", initialiser.getCallbackHandler());
-                }
-                else if (db instanceof PlainPasswordFilePrincipalDatabase)
-                {
-                    env.put("jmx.remote.profiles", "SASL/PLAIN");
-                    env.put("jmx.remote.sasl.callback.handler", new UserCallbackHandler(db));
-                }
+            // For SASL using JMXMP
+            _jmxURL = new JMXServiceURL("jmxmp", null, port);
 
-                // Enable the SSL security and server authentication
-                /*
-                SslRMIClientSocketFactory csf = new SslRMIClientSocketFactory();
-                SslRMIServerSocketFactory ssf = new SslRMIServerSocketFactory();
-                env.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE, csf);
-                env.put(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE, ssf);
-                 */
+            Map env = new HashMap();
+            Map<String, PrincipalDatabase> map = appRegistry.getDatabaseManager().getDatabases();
+            PrincipalDatabase db = null;
 
-                try
+            for (Map.Entry<String, PrincipalDatabase> entry : map.entrySet())
+            {
+                if (entry.getValue() instanceof Base64MD5PasswordFilePrincipalDatabase)
                 {
-                    JMXConnectorServer cs = JMXConnectorServerFactory.newJMXConnectorServer(_jmxURL, env, _mbeanServer);
-                    MBeanServerForwarder mbsf = MBeanInvocationHandlerImpl.newProxyInstance();
-                    cs.setMBeanServerForwarder(mbsf);
-                    cs.start();
-                    _log.info("JMX: Starting JMXConnector server with SASL");
+                    db = entry.getValue();
+                    break;
                 }
-                catch (java.net.MalformedURLException urlException)
+                else if (entry.getValue() instanceof PlainPasswordFilePrincipalDatabase)
                 {
-                    // When JMXMPConnector is not available
-                    // java.net.MalformedURLException: Unsupported protocol: jmxmp
-                    _log.info("JMX: Starting JMXConnector server");
-                    startJMXConnectorServer(port);
+                    db = entry.getValue();
                 }
             }
-            else
+
+            if (db instanceof Base64MD5PasswordFilePrincipalDatabase)
+            {
+                env.put("jmx.remote.profiles", "SASL/CRAM-MD5");
+                CRAMMD5HashedInitialiser initialiser = new CRAMMD5HashedInitialiser();
+                initialiser.initialise(db);
+                env.put("jmx.remote.sasl.callback.handler", initialiser.getCallbackHandler());
+            }
+            else if (db instanceof PlainPasswordFilePrincipalDatabase)
             {
-                startJMXConnectorServer(port);
+                env.put("jmx.remote.profiles", "SASL/PLAIN");
+                env.put("jmx.remote.sasl.callback.handler", new UserCallbackHandler(db));
             }
+
+            // Enable the SSL security and server authentication
+            /*
+           SslRMIClientSocketFactory csf = new SslRMIClientSocketFactory();
+           SslRMIServerSocketFactory ssf = new SslRMIServerSocketFactory();
+           env.put(RMIConnectorServer.RMI_CLIENT_SOCKET_FACTORY_ATTRIBUTE, csf);
+           env.put(RMIConnectorServer.RMI_SERVER_SOCKET_FACTORY_ATTRIBUTE, ssf);
+            */
+
+            JMXConnectorServer cs = JMXConnectorServerFactory.newJMXConnectorServer(_jmxURL, env, _mbeanServer);
+            MBeanServerForwarder mbsf = MBeanInvocationHandlerImpl.newProxyInstance();
+            cs.setMBeanServerForwarder(mbsf);
+            cs.start();
+            _log.warn("JMX: Started JMXConnector server with SASL");
+
         }
-        catch (Exception ex)
+        else
         {
-            _log.error("Error in initialising Managed Object Registry." + ex.getMessage());
-            ex.printStackTrace();
+            startJMXConnectorServer(port);
+            _log.warn("JMX: Started JMXConnector server with security disabled");
         }
     }
 
@@ -280,7 +264,7 @@ public class JMXManagedObjectRegistry implements ManagedObjectRegistry
                 String username = ncb.getDefaultName();
                 try
                 {
-                    authorized = _principalDatabase.verifyPassword(username, new String(pcb.getPassword()));
+                    authorized = _principalDatabase.verifyPassword(username, pcb.getPassword());
                 }
                 catch (AccountNotFoundException e)
                 {
diff --git a/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java b/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
index a79d993afc..7d25fb8c69 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/management/MBeanInvocationHandlerImpl.java
@@ -18,6 +18,7 @@
 package org.apache.qpid.server.management;
 
 import org.apache.qpid.AMQException;
+import org.apache.qpid.server.security.access.AMQUserManagementMBean;
 import org.apache.log4j.Logger;
 
 import javax.management.remote.MBeanServerForwarder;
@@ -122,8 +123,20 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler
         Principal principal = principals.iterator().next();
         String identity = principal.getName();
 
+        if (isAdminMethod(args))
+        {
+            if (isAdmin(identity))
+            {
+                return method.invoke(mbs, args);
+            }
+            else
+            {
+                throw new SecurityException("Access denied");
+            }
+        }
+
         // Following users can perform any operation other than "createMBean" and "unregisterMBean"
-        if (isAdmin(identity) || isAllowedToModify(identity))
+        if (isAllowedToModify(identity))
         {
             return method.invoke(mbs, args);
         }
@@ -138,6 +151,41 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler
         throw new SecurityException("Access denied");
     }
 
+    private boolean isAdminMethod(Object[] args)
+    {
+        if (args[0] instanceof ObjectName)
+        {
+            String mbeanMethod = (args.length > 1) ? (String) args[1] : null;
+            if (mbeanMethod == null)
+            {
+                if (args[0] instanceof ObjectName)
+                {
+                    ObjectName object = (ObjectName) args[0];
+                    return object.getCanonicalName().contains("UserManagement");
+                }
+                else
+                {
+                    return false;
+                }
+            }
+
+            try
+            {
+                MBeanInfo mbeanInfo = mbs.getMBeanInfo((ObjectName) args[0]);
+                if (mbeanInfo != null)
+                {
+                    return mbeanInfo.getClassName().equals("org.apache.qpid.server.security.access.AMQUserManagementMBean");
+                }
+            }
+            catch (JMException ex)
+            {
+                return false;
+            }
+        }
+
+        return false;
+    }
+
     // Initialises the user roles
     public static void setAccessRights(Properties accessRights)
     {
@@ -155,7 +203,8 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler
 
     private boolean isAllowedToModify(String userName)
     {
-        if (READWRITE.equals(_userRoles.getProperty(userName)))
+        if (ADMIN.equals(_userRoles.getProperty(userName))
+            || READWRITE.equals(_userRoles.getProperty(userName)))
         {
             return true;
         }
diff --git a/java/broker/src/main/java/org/apache/qpid/server/management/ManagedObjectRegistry.java b/java/broker/src/main/java/org/apache/qpid/server/management/ManagedObjectRegistry.java
index 5f9bc9ddad..d8d87ef881 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/management/ManagedObjectRegistry.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/management/ManagedObjectRegistry.java
@@ -7,9 +7,9 @@
  * 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
@@ -22,6 +22,7 @@ package org.apache.qpid.server.management;
 
 import javax.management.JMException;
 import java.rmi.RemoteException;
+import java.io.IOException;
 
 /**
  * Handles the registration (and unregistration and so on) of managed objects.
@@ -37,7 +38,7 @@ import java.rmi.RemoteException;
  */
 public interface ManagedObjectRegistry
 {
-    void start();
+    void start() throws IOException;
 
     void registerObject(ManagedObject managedObject) throws JMException;
 
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/access/AMQUserManagementMBean.java b/java/broker/src/main/java/org/apache/qpid/server/security/access/AMQUserManagementMBean.java
index 20f123179f..fbb80494c1 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/access/AMQUserManagementMBean.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/access/AMQUserManagementMBean.java
@@ -30,6 +30,7 @@ import org.apache.log4j.Logger;
 import org.apache.commons.configuration.ConfigurationException;
 
 import javax.management.JMException;
+import javax.management.remote.JMXPrincipal;
 import javax.management.openmbean.TabularData;
 import javax.management.openmbean.TabularDataSupport;
 import javax.management.openmbean.TabularType;
@@ -40,6 +41,7 @@ import javax.management.openmbean.OpenDataException;
 import javax.management.openmbean.CompositeData;
 import javax.management.openmbean.CompositeDataSupport;
 import javax.security.auth.login.AccountNotFoundException;
+import javax.security.auth.Subject;
 import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
@@ -47,8 +49,11 @@ import java.io.FileOutputStream;
 import java.util.Properties;
 import java.util.List;
 import java.util.Enumeration;
+import java.util.Set;
 import java.util.concurrent.locks.ReentrantLock;
 import java.security.Principal;
+import java.security.AccessControlContext;
+import java.security.AccessController;
 
 /** MBean class for AMQUserManagementMBean. It implements all the management features exposed for managing users. */
 @MBeanDescription("User Management Interface")
@@ -250,8 +255,6 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana
         // Table of users
         // Username(string), Access rights Read,Write,Admin(bool,bool,bool)
 
-        reloadData();
-        
         if (_userlistDataType == null)
         {
             _logger.warn("TabluarData not setup correctly");
@@ -411,7 +414,7 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana
                 rights.renameTo(old);
 
                 FileOutputStream output = new FileOutputStream(tmp);
-                _accessRights.store(output, "");
+                _accessRights.store(output, "Last edited by user:" + getCurrentJMXUser());
                 output.close();
 
                 // Rename new file to main file
@@ -434,6 +437,22 @@ public class AMQUserManagementMBean extends AMQManagedObject implements UserMana
         }
     }
 
+    private String getCurrentJMXUser()
+    {
+        AccessControlContext acc = AccessController.getContext();
+        Subject subject = Subject.getSubject(acc);
+
+        // Retrieve JMXPrincipal from Subject
+        Set<JMXPrincipal> principals = subject.getPrincipals(JMXPrincipal.class);
+        if (principals == null || principals.isEmpty())
+        {
+            return "Unknown user principals were null";
+        }
+
+        Principal principal = principals.iterator().next();
+        return principal.getName();
+    }
+
     /**
      * user=read user=write user=readwrite user=admin
      *
diff --git a/java/broker/src/main/java/org/apache/qpid/server/security/access/UserManagement.java b/java/broker/src/main/java/org/apache/qpid/server/security/access/UserManagement.java
index ce5e9fa4a7..1e4e48de16 100644
--- a/java/broker/src/main/java/org/apache/qpid/server/security/access/UserManagement.java
+++ b/java/broker/src/main/java/org/apache/qpid/server/security/access/UserManagement.java
@@ -28,6 +28,7 @@ import org.apache.qpid.AMQException;
 import javax.management.openmbean.TabularData;
 import javax.management.openmbean.CompositeData;
 import javax.management.JMException;
+import javax.management.MBeanOperationInfo;
 import java.io.IOException;
 
 public interface UserManagement
@@ -43,7 +44,8 @@ public interface UserManagement
      *
      * @return The result of the operation
      */
-    @MBeanOperation(name = "setPassword", description = "Set password for user.")              
+    @MBeanOperation(name = "setPassword", description = "Set password for user.",
+                    impact = MBeanOperationInfo.ACTION)
     boolean setPassword(@MBeanOperationParameter(name = "username", description = "Username")String username,
                         @MBeanOperationParameter(name = "password", description = "Password")char[] password);
 
@@ -57,7 +59,8 @@ public interface UserManagement
      *
      * @return The result of the operation
      */
-    @MBeanOperation(name = "setRights", description = "Set access rights for user.")
+    @MBeanOperation(name = "setRights", description = "Set access rights for user.",
+                    impact = MBeanOperationInfo.ACTION)
     boolean setRights(@MBeanOperationParameter(name = "username", description = "Username")String username,
                       @MBeanOperationParameter(name = "read", description = "Administration read")boolean read,
                       @MBeanOperationParameter(name = "write", description = "Administration write")boolean write,
@@ -74,7 +77,8 @@ public interface UserManagement
      *
      * @return The result of the operation
      */
-    @MBeanOperation(name = "createUser", description = "Create new user from system.")
+    @MBeanOperation(name = "createUser", description = "Create new user from system.",
+                    impact = MBeanOperationInfo.ACTION)
     boolean createUser(@MBeanOperationParameter(name = "username", description = "Username")String username,
                        @MBeanOperationParameter(name = "password", description = "Password")char[] password,
                        @MBeanOperationParameter(name = "read", description = "Administration read")boolean read,
@@ -88,7 +92,8 @@ public interface UserManagement
      *
      * @return The result of the operation
      */
-    @MBeanOperation(name = "deleteUser", description = "Delete user from system.")
+    @MBeanOperation(name = "deleteUser", description = "Delete user from system.",
+                    impact = MBeanOperationInfo.ACTION)
     boolean deleteUser(@MBeanOperationParameter(name = "username", description = "Username")String username);
 
 
@@ -97,15 +102,17 @@ public interface UserManagement
      *
      * @return The result of the operation
      */
-//    @MBeanOperation(name = "reloadData", description = "Reload the authentication file from disk.")
-//    boolean reloadData();
+    @MBeanOperation(name = "reloadData", description = "Reload the authentication file from disk.",
+                    impact = MBeanOperationInfo.ACTION)
+    boolean reloadData();
 
     /**
      * View users returns all the users that are currently available to the system.
      *
      * @return a table of users data (Username, read, write, admin)
      */
-    @MBeanOperation(name = "viewUsers", description = "All users with access rights to the system.")
+    @MBeanOperation(name = "viewUsers", description = "All users with access rights to the system.",
+                    impact = MBeanOperationInfo.ACTION)
     TabularData viewUsers();
 
 }