QPID-4292: add ACL rule to authorise access to the web management UI

* added object name MANAGEMENT to represent both JMX and Web Management layers
* Change both JMX/Web entry points to permission access with an access management check
* Updated examples and docbook
* Made Principals serialised to avoid container warnings when Qpid principals are placed within a HttpSession.

Work of Robbie Gemmell <robbie@apache.org> and myself.

git-svn-id: https://svn.apache.org/repos/asf/qpid/trunk@1382947 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 132 insertions, 29 deletions

diff --git a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
index be4962615c..fa9e96ba1e 100644
--- a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
+++ b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/PlainConfigurationTest.java
@@ -438,6 +438,15 @@ public class PlainConfigurationTest extends TestCase
         }
     }
 
+    public void testManagementRuleParsing() throws Exception
+    {
+        validateRule(writeACLConfig("ACL ALLOW user1 ALL MANAGEMENT"),
+                "user1", Operation.ALL, ObjectType.MANAGEMENT, ObjectProperties.EMPTY);
+
+        validateRule(writeACLConfig("ACL ALLOW user1 ACCESS MANAGEMENT"),
+                "user1", Operation.ACCESS, ObjectType.MANAGEMENT, ObjectProperties.EMPTY);
+    }
+
     private void validateRule(final PlainConfiguration config, String username, Operation operation, ObjectType objectType, ObjectProperties objectProperties)
     {
         final RuleSet rs = config.getConfiguration();
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java
index 2dc819cb90..2d15b8a1d9 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/AbstractServlet.java
@@ -43,6 +43,7 @@ import org.apache.qpid.server.logging.actors.HttpManagementActor;
 import org.apache.qpid.server.management.plugin.session.LoginLogoutReporter;
 import org.apache.qpid.server.model.Broker;
 import org.apache.qpid.server.registry.ApplicationRegistry;
+import org.apache.qpid.server.security.SecurityManager;
 import org.apache.qpid.server.security.SubjectCreator;
 import org.apache.qpid.server.security.auth.AuthenticationResult.AuthenticationStatus;
 import org.apache.qpid.server.security.auth.SubjectAuthenticationResult;
@@ -50,11 +51,10 @@ import org.apache.qpid.server.security.auth.manager.AnonymousAuthenticationManag
 
 public abstract class AbstractServlet extends HttpServlet
 {
-    private static final String ATTR_LOGIN_LOGOUT_REPORTER = "attrLoginLogoutReporter";
-
     private static final Logger LOGGER = Logger.getLogger(AbstractServlet.class);
 
-    protected static final String ATTR_SUBJECT = "subject";
+    private static final String ATTR_LOGIN_LOGOUT_REPORTER = "AbstractServlet.loginLogoutReporter";
+    private static final String ATTR_SUBJECT = "AbstractServlet.subject";
     private static final String ATTR_LOG_ACTOR = "AbstractServlet.logActor";
 
     private final Broker _broker;
@@ -193,9 +193,18 @@ public abstract class AbstractServlet extends HttpServlet
                     final HttpServletRequest request,
                     final HttpServletResponse resp)
     {
-        Subject subject = getAndCacheAuthorizedSubject(request);
-        org.apache.qpid.server.security.SecurityManager.setThreadSubject(subject);
+        Subject subject;
+        try
+        {
+            subject = getAndCacheAuthorizedSubject(request);
+        }
+        catch (AccessControlException e)
+        {
+            sendError(resp, HttpServletResponse.SC_FORBIDDEN);
+            return;
+        }
 
+        SecurityManager.setThreadSubject(subject);
         try
         {
             HttpManagementActor logActor = getLogActorAndCacheInSession(request);
@@ -223,7 +232,7 @@ public abstract class AbstractServlet extends HttpServlet
         {
             try
             {
-                org.apache.qpid.server.security.SecurityManager.setThreadSubject(null);
+                SecurityManager.setThreadSubject(null);
             }
             finally
             {
@@ -247,7 +256,7 @@ public abstract class AbstractServlet extends HttpServlet
     private Subject getAndCacheAuthorizedSubject(HttpServletRequest request)
     {
         HttpSession session = request.getSession();
-        Subject subject = getSubjectFromSession(session);
+        Subject subject = getAuthorisedSubjectFromSession(session);
 
         if(subject != null)
         {
@@ -255,6 +264,65 @@ public abstract class AbstractServlet extends HttpServlet
         }
 
         SubjectCreator subjectCreator = ApplicationRegistry.getInstance().getSubjectCreator(getSocketAddress(request));
+        subject = authenticate(request, subjectCreator);
+        if (subject != null)
+        {
+            authoriseManagement(request, subject);
+            setAuthorisedSubjectInSession(subject, request, session);
+        }
+        else
+        {
+            subject = subjectCreator.createSubjectWithGroups(AnonymousAuthenticationManager.ANONYMOUS_USERNAME);
+        }
+
+        return subject;
+    }
+
+    protected void authoriseManagement(HttpServletRequest request, Subject subject)
+    {
+        // TODO: We should eliminate SecurityManager.setThreadSubject in favour of Subject.doAs
+        SecurityManager.setThreadSubject(subject);  // Required for accessManagement check
+        LogActor actor = createHttpManagementActor(request);
+        CurrentActor.set(actor);
+        try
+        {
+            try
+            {
+                Subject.doAs(subject, new PrivilegedExceptionAction<Void>() // Required for proper logging of Subject
+                {
+                    @Override
+                    public Void run() throws Exception
+                    {
+                        boolean allowed = ApplicationRegistry.getInstance().getSecurityManager().accessManagement();
+                        if (!allowed)
+                        {
+                            throw new AccessControlException("User is not authorised for management");
+                        }
+                        return null;
+                    }
+                });
+            }
+            catch (PrivilegedActionException e)
+            {
+                throw new RuntimeException("Unable to perform access check", e);
+            }
+        }
+        finally
+        {
+            try
+            {
+                CurrentActor.remove();
+            }
+            finally
+            {
+                SecurityManager.setThreadSubject(null);
+            }
+        }
+    }
+
+    private Subject authenticate(HttpServletRequest request, SubjectCreator subjectCreator)
+    {
+        Subject subject = null;
 
         String remoteUser = request.getRemoteUser();
         if(remoteUser != null)
@@ -268,8 +336,7 @@ public abstract class AbstractServlet extends HttpServlet
             if (header != null)
             {
                 String[] tokens = header.split("\\s");
-                if(tokens.length >= 2
-                        && "BASIC".equalsIgnoreCase(tokens[0]))
+                if(tokens.length >= 2 && "BASIC".equalsIgnoreCase(tokens[0]))
                 {
                     if(!isBasicAuthSupported(request))
                     {
@@ -277,30 +344,27 @@ public abstract class AbstractServlet extends HttpServlet
                         throw new IllegalArgumentException("BASIC Authorization is not enabled.");
                     }
 
-                    String[] credentials = (new String(Base64.decodeBase64(tokens[1].getBytes()))).split(":",2);
-                    if(credentials.length == 2)
-                    {
-                        subject = authenticateUserAndGetSubject(subjectCreator, credentials[0], credentials[1]);
-                    }
-                    else
-                    {
-                        //TODO: write a return response indicating failure?
-                        throw new AccessControlException("Invalid number of credentials supplied: "
-                                                        + credentials.length);
-                    }
+                    subject = performBasicAuth(subject, subjectCreator, tokens[1]);
                 }
             }
         }
 
-        if (subject != null)
+        return subject;
+    }
+
+    private Subject performBasicAuth(Subject subject,SubjectCreator subjectCreator, String base64UsernameAndPassword)
+    {
+        String[] credentials = (new String(Base64.decodeBase64(base64UsernameAndPassword.getBytes()))).split(":",2);
+        if(credentials.length == 2)
         {
-            setSubjectInSession(subject, request, session);
+            subject = authenticateUserAndGetSubject(subjectCreator, credentials[0], credentials[1]);
         }
         else
         {
-            subject = subjectCreator.createSubjectWithGroups(AnonymousAuthenticationManager.ANONYMOUS_USERNAME);
+            //TODO: write a return response indicating failure?
+            throw new AccessControlException("Invalid number of credentials supplied: "
+                                            + credentials.length);
         }
-
         return subject;
     }
 
@@ -336,12 +400,12 @@ public abstract class AbstractServlet extends HttpServlet
         return actor;
     }
 
-    protected Subject getSubjectFromSession(HttpSession session)
+    protected Subject getAuthorisedSubjectFromSession(HttpSession session)
     {
         return (Subject)session.getAttribute(ATTR_SUBJECT);
     }
 
-    protected void setSubjectInSession(Subject subject, HttpServletRequest request, final HttpSession session)
+    protected void setAuthorisedSubjectInSession(Subject subject, HttpServletRequest request, final HttpSession session)
     {
         session.setAttribute(ATTR_SUBJECT, subject);
 
@@ -360,9 +424,22 @@ public abstract class AbstractServlet extends HttpServlet
         return InetSocketAddress.createUnresolved(request.getServerName(), request.getServerPort());
     }
 
+    protected void sendError(final HttpServletResponse resp, int errorCode)
+    {
+        try
+        {
+            resp.sendError(errorCode);
+        }
+        catch (IOException e)
+        {
+            throw new RuntimeException("Failed to send error response code " + errorCode, e);
+        }
+    }
+
     private HttpManagementActor createHttpManagementActor(HttpServletRequest request)
     {
         return new HttpManagementActor(_rootLogger, request.getRemoteAddr(), request.getRemotePort());
     }
 
+
 }
diff --git a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
index feedc283a9..0a035006c7 100644
--- a/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
+++ b/qpid/java/broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/SaslServlet.java
@@ -39,6 +39,7 @@ import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.security.AccessControlException;
 import java.security.Principal;
 import java.security.SecureRandom;
 import java.util.LinkedHashMap;
@@ -85,7 +86,7 @@ public class SaslServlet extends AbstractServlet
         String[] mechanisms = subjectCreator.getMechanisms().split(" ");
         Map<String, Object> outputObject = new LinkedHashMap<String, Object>();
 
-        final Subject subject = getSubjectFromSession(session);
+        final Subject subject = getAuthorisedSubjectFromSession(session);
         if(subject != null)
         {
             Principal principal = AuthenticatedPrincipal.getAuthenticatedPrincipalFromSubject(subject);
@@ -236,8 +237,17 @@ public class SaslServlet extends AbstractServlet
         {
             Subject subject = subjectCreator.createSubjectWithGroups(saslServer.getAuthorizationID());
 
-            setSubjectInSession(subject, request, session);
+            try
+            {
+                authoriseManagement(request, subject);
+            }
+            catch (AccessControlException ace)
+            {
+                sendError(response, HttpServletResponse.SC_FORBIDDEN);
+                return;
+            }
 
+            setAuthorisedSubjectInSession(subject, request, session);
             session.removeAttribute(ATTR_ID);
             session.removeAttribute(ATTR_SASL_SERVER);
             session.removeAttribute(ATTR_EXPIRY);
diff --git a/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java b/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
index 3d21f95f0c..f85fd02199 100644
--- a/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
+++ b/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/GroupRestACLTest.java
@@ -89,6 +89,7 @@ public class GroupRestACLTest extends QpidRestTestCase
     public void testCreateGroup() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " CREATE GROUP",
                 "ACL DENY-LOG " + DENIED_GROUP + " CREATE GROUP");
 
@@ -116,6 +117,7 @@ public class GroupRestACLTest extends QpidRestTestCase
     public void testDeleteGroup() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " DELETE GROUP",
                 "ACL DENY-LOG " + DENIED_GROUP + " DELETE GROUP");
 
@@ -143,6 +145,7 @@ public class GroupRestACLTest extends QpidRestTestCase
     public void testUpdateGroupAddMember() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " UPDATE GROUP",
                 "ACL DENY-LOG " + DENIED_GROUP + " UPDATE GROUP");
 
@@ -163,6 +166,7 @@ public class GroupRestACLTest extends QpidRestTestCase
     public void testUpdateGroupDeleteMember() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " UPDATE GROUP",
                 "ACL DENY-LOG " + DENIED_GROUP + " UPDATE GROUP");
 
diff --git a/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java b/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
index 31286fb70b..09c82b9205 100644
--- a/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
+++ b/qpid/java/broker-plugins/management-http/src/test/java/org/apache/qpid/systest/rest/acl/UserRestACLTest.java
@@ -91,6 +91,7 @@ public class UserRestACLTest extends QpidRestTestCase
     public void testAddUser() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " CREATE USER",
                 "ACL DENY-LOG " + DENIED_GROUP + " CREATE USER");
 
@@ -115,6 +116,7 @@ public class UserRestACLTest extends QpidRestTestCase
     public void testDeleteUser() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " DELETE USER",
                 "ACL DENY-LOG " + DENIED_GROUP + " DELETE USER");
 
@@ -135,6 +137,7 @@ public class UserRestACLTest extends QpidRestTestCase
     public void testUpdateUser() throws Exception
     {
         AbstractACLTestCase.writeACLFileUtil(this, null,
+                "ACL ALLOW-LOG ALL ACCESS MANAGEMENT",
                 "ACL ALLOW-LOG " + ALLOWED_GROUP + " UPDATE USER",
                 "ACL DENY-LOG " + DENIED_GROUP + " UPDATE USER");
 
diff --git a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java
index cb2e9f5e54..9df6a181f2 100644
--- a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java
+++ b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/JMXManagedObjectRegistry.java
@@ -199,7 +199,7 @@ public class JMXManagedObjectRegistry implements ManagedObjectRegistry
         }
 
         //add a JMXAuthenticator implementation the env map to authenticate the RMI based JMX connector server
-        RMIPasswordAuthenticator rmipa = new RMIPasswordAuthenticator(new InetSocketAddress(_jmxPortRegistryServer));
+        RMIPasswordAuthenticator rmipa = new RMIPasswordAuthenticator(ApplicationRegistry.getInstance(), new InetSocketAddress(_jmxPortRegistryServer));
         HashMap<String,Object> env = new HashMap<String,Object>();
         env.put(JMXConnectorServer.AUTHENTICATOR, rmipa);