QPID-5567 : Further changes to SecurityMangager

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

6 files changed, 79 insertions, 121 deletions

diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
index b0ef11bc1b..d380996da2 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/model/adapter/BrokerAdapter.java
@@ -24,6 +24,8 @@ import java.lang.reflect.Type;
 import java.net.InetSocketAddress;
 import java.net.SocketAddress;
 import java.security.AccessControlException;
+import java.security.PrivilegedAction;
+import java.security.PrivilegedExceptionAction;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
@@ -73,6 +75,8 @@ import org.apache.qpid.server.util.MapValueConverter;
 import org.apache.qpid.server.virtualhost.VirtualHostRegistry;
 import org.apache.qpid.util.SystemUtils;
 
+import javax.security.auth.Subject;
+
 public class BrokerAdapter extends AbstractAdapter implements Broker, ConfigurationChangeListener
 {
     private static final Logger LOGGER = Logger.getLogger(BrokerAdapter.class);
@@ -297,15 +301,15 @@ public class BrokerAdapter extends AbstractAdapter implements Broker, Configurat
 
         // permission has already been granted to create the virtual host
         // disable further access check on other operations, e.g. create exchange
-        SecurityManager.setAccessChecksDisabled(true);
-        try
-        {
-            virtualHostAdapter.setDesiredState(State.INITIALISING, State.ACTIVE);
-        }
-        finally
-        {
-            SecurityManager.setAccessChecksDisabled(false);
-        }
+        Subject.doAs(SecurityManager.SYSTEM, new PrivilegedAction<Object>()
+                            {
+                                @Override
+                                public Object run()
+                                {
+                                    virtualHostAdapter.setDesiredState(State.INITIALISING, State.ACTIVE);
+                                    return null;
+                                }
+                            });
         return virtualHostAdapter;
     }
 
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/AccessControl.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/AccessControl.java
index 87946590ec..63dcb9a3d6 100644
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/AccessControl.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/AccessControl.java
@@ -23,22 +23,17 @@ import org.apache.qpid.server.security.access.ObjectType;
 import org.apache.qpid.server.security.access.Operation;
 
 /**
- * The two methods, {@link #access(org.apache.qpid.server.security.access.ObjectType)} and {@link #authorise(Operation, ObjectType, ObjectProperties)},
- * return the {@link Result} of the security decision, which may be to {@link Result#ABSTAIN} if no decision is made.
+ * The method {@link #authorise(Operation, ObjectType, ObjectProperties)},
+ * returns the {@link Result} of the security decision, which may be to {@link Result#ABSTAIN} if no decision is made.
  */
 public interface AccessControl
 {
 	/**
-	 * Default result for {@link #access(org.apache.qpid.server.security.access.ObjectType)} or {@link #authorise(Operation, ObjectType, ObjectProperties)}.
+	 * Default result for {@link #authorise(Operation, ObjectType, ObjectProperties)}.
 	 */
 	Result getDefault();
 
     /**
-     * Authorise access granted to an object instance.
-     */
-    Result access(ObjectType objectType);
-
-    /**
      * Authorise an operation on an object defined by a set of properties.
      */
     Result authorise(Operation operation, ObjectType objectType, ObjectProperties properties);
diff --git a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
index e5911d268b..85be4c6a3d 100755
--- a/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
+++ b/qpid/java/broker-core/src/main/java/org/apache/qpid/server/security/SecurityManager.java
@@ -36,6 +36,8 @@ import org.apache.qpid.server.security.access.ObjectType;
 import org.apache.qpid.server.security.access.Operation;
 import org.apache.qpid.server.security.access.OperationLoggingDetails;
 
+import javax.security.auth.Subject;
+
 import static org.apache.qpid.server.security.access.ObjectType.BROKER;
 import static org.apache.qpid.server.security.access.ObjectType.EXCHANGE;
 import static org.apache.qpid.server.security.access.ObjectType.GROUP;
@@ -54,8 +56,9 @@ import static org.apache.qpid.server.security.access.Operation.PURGE;
 import static org.apache.qpid.server.security.access.Operation.UNBIND;
 import static org.apache.qpid.server.security.access.Operation.UPDATE;
 
-import java.net.SocketAddress;
 import java.security.AccessControlException;
+import java.security.AccessController;
+import java.security.Principal;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.HashMap;
@@ -67,7 +70,11 @@ public class SecurityManager implements ConfigurationChangeListener
 {
     private static final Logger _logger = Logger.getLogger(SecurityManager.class);
 
-    public static final ThreadLocal<Boolean> _accessChecksDisabled = new ClearingThreadLocal(false);
+    public static final Subject SYSTEM = new Subject(true,
+                                                     Collections.singleton(new SystemPrincipal()),
+                                                     Collections.emptySet(),
+                                                     Collections.emptySet());
+
 
     private ConcurrentHashMap<String, AccessControl> _globalPlugins = new ConcurrentHashMap<String, AccessControl>();
     private ConcurrentHashMap<String, AccessControl> _hostPlugins = new ConcurrentHashMap<String, AccessControl>();
@@ -76,51 +83,6 @@ public class SecurityManager implements ConfigurationChangeListener
 
     private Broker _broker;
 
-    /**
-     * A special ThreadLocal, which calls remove() on itself whenever the value is
-     * the default, to avoid leaving a default value set after its use has passed.
-     */
-    private static final class ClearingThreadLocal extends ThreadLocal<Boolean>
-    {
-        private Boolean _defaultValue;
-
-        public ClearingThreadLocal(Boolean defaultValue)
-        {
-            super();
-            _defaultValue = defaultValue;
-        }
-
-        @Override
-        protected Boolean initialValue()
-        {
-            return _defaultValue;
-        }
-
-        @Override
-        public void set(Boolean value)
-        {
-            if (value == _defaultValue)
-            {
-                super.remove();
-            }
-            else
-            {
-                super.set(value);
-            }
-        }
-
-        @Override
-        public Boolean get()
-        {
-            Boolean value = super.get();
-            if (value == _defaultValue)
-            {
-                super.remove();
-            }
-            return value;
-        }
-    }
-
     /*
      * Used by the Broker.
      */
@@ -190,6 +152,19 @@ public class SecurityManager implements ConfigurationChangeListener
         return _logger;
     }
 
+    private static final class SystemPrincipal implements Principal
+    {
+        private SystemPrincipal()
+        {
+        }
+
+        @Override
+        public String getName()
+        {
+            return "SYSTEM";
+        }
+    }
+
     private abstract class AccessCheck
     {
         abstract Result allowed(AccessControl plugin);
@@ -197,7 +172,9 @@ public class SecurityManager implements ConfigurationChangeListener
 
     private boolean checkAllPlugins(AccessCheck checker)
     {
-        if(_accessChecksDisabled.get())
+        // If we are running as SYSTEM then no ACL checking
+        final Subject subject = Subject.getSubject(AccessController.getContext());
+        if(subject != null && !subject.getPrincipals(SystemPrincipal.class).isEmpty())
         {
             return true;
         }
@@ -321,7 +298,7 @@ public class SecurityManager implements ConfigurationChangeListener
         {
             Result allowed(AccessControl plugin)
             {
-                return plugin.access(ObjectType.MANAGEMENT);
+                return plugin.authorise(Operation.ACCESS, ObjectType.MANAGEMENT, ObjectProperties.EMPTY);
             }
         }))
         {
@@ -335,7 +312,7 @@ public class SecurityManager implements ConfigurationChangeListener
         {
             Result allowed(AccessControl plugin)
             {
-                return plugin.access(VIRTUALHOST);
+                return plugin.authorise(Operation.ACCESS, VIRTUALHOST, ObjectProperties.EMPTY);
             }
         }))
         {
@@ -548,15 +525,6 @@ public class SecurityManager implements ConfigurationChangeListener
         }
     }
 
-    public static boolean setAccessChecksDisabled(final boolean status)
-    {
-        //remember current value
-        boolean current = _accessChecksDisabled.get();
-
-        _accessChecksDisabled.set(status);
-
-        return current;
-    }
 
     private class PublishAccessCheck extends AccessCheck
     {
diff --git a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControl.java b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControl.java
index 75006ae697..f579ea0ec5 100644
--- a/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControl.java
+++ b/qpid/java/broker-plugins/access-control/src/main/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControl.java
@@ -113,49 +113,31 @@ public class DefaultAccessControl implements AccessControl
     }
 
     /**
-     * Object instance access authorisation.
-     *
-	 * Delegate to the {@link #authorise(Operation, ObjectType, ObjectProperties)} method, with
-     * the operation set to ACCESS and no object properties.
-	 */
-    public Result access(ObjectType objectType)
-    {
-        InetAddress addressOfClient = null;
-        final Subject subject = Subject.getSubject(AccessController.getContext());
-        if(subject != null)
-        {
-            Set<ConnectionPrincipal> principals = subject.getPrincipals(ConnectionPrincipal.class);
-            if(!principals.isEmpty())
-            {
-                SocketAddress address = principals.iterator().next().getConnection().getRemoteAddress();
-                if(address instanceof InetSocketAddress)
-                {
-                    addressOfClient = ((InetSocketAddress) address).getAddress();
-                }
-            }
-        }
-        return authoriseFromAddress(Operation.ACCESS, objectType, ObjectProperties.EMPTY, addressOfClient);
-    }
-
-    /**
      * Check if an operation is authorised by asking the  configuration object about the access
      * control rules granted to the current thread's {@link Subject}. If there is no current
      * user the plugin will abstain.
      */
     public Result authorise(Operation operation, ObjectType objectType, ObjectProperties properties)
     {
-        return authoriseFromAddress(operation, objectType, properties, null);
-    }
-
-    public Result authoriseFromAddress(Operation operation, ObjectType objectType, ObjectProperties properties, InetAddress addressOfClient)
-    {
+        InetAddress addressOfClient = null;
         final Subject subject = Subject.getSubject(AccessController.getContext());
+
         // Abstain if there is no subject/principal associated with this thread
         if (subject == null  || subject.getPrincipals().size() == 0)
         {
             return Result.ABSTAIN;
         }
 
+        Set<ConnectionPrincipal> principals = subject.getPrincipals(ConnectionPrincipal.class);
+        if(!principals.isEmpty())
+        {
+            SocketAddress address = principals.iterator().next().getConnection().getRemoteAddress();
+            if(address instanceof InetSocketAddress)
+            {
+                addressOfClient = ((InetSocketAddress) address).getAddress();
+            }
+        }
+
         if(_logger.isDebugEnabled())
         {
             _logger.debug("Checking " + operation + " " + objectType + " " + ObjectUtils.defaultIfNull(addressOfClient, ""));
diff --git a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControlTest.java b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControlTest.java
index 8ac4e0c424..e907a88001 100644
--- a/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControlTest.java
+++ b/qpid/java/broker-plugins/access-control/src/test/java/org/apache/qpid/server/security/access/plugins/DefaultAccessControlTest.java
@@ -246,7 +246,7 @@ public class DefaultAccessControlTest extends TestCase
 
                 DefaultAccessControl accessControl = new DefaultAccessControl(mockRuleSet);
 
-                accessControl.access(ObjectType.VIRTUALHOST);
+                accessControl.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
 
                 verify(mockRuleSet).check(subject, Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY, inetAddress);
                 return null;
@@ -282,7 +282,7 @@ public class DefaultAccessControlTest extends TestCase
                         inetAddress)).thenThrow(new RuntimeException());
 
                 DefaultAccessControl accessControl = new DefaultAccessControl(mockRuleSet);
-                Result result = accessControl.access(ObjectType.VIRTUALHOST);
+                Result result = accessControl.authorise(Operation.ACCESS, ObjectType.VIRTUALHOST, ObjectProperties.EMPTY);
 
                 assertEquals(Result.DENIED, result);
                 return null;
diff --git a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
index e78e59b7fc..f959977d1f 100644
--- a/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
+++ b/qpid/java/broker-plugins/management-jmx/src/main/java/org/apache/qpid/server/jmx/MBeanInvocationHandlerImpl.java
@@ -46,6 +46,9 @@ import java.lang.reflect.Method;
 import java.lang.reflect.Proxy;
 import java.security.AccessControlContext;
 import java.security.AccessController;
+import java.security.PrivilegedAction;
+import java.security.PrivilegedActionException;
+import java.security.PrivilegedExceptionAction;
 import java.util.Arrays;
 
 /**
@@ -201,7 +204,7 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler
         }
     }
 
-    private Object authoriseAndInvoke(Method method, Object[] args) throws IllegalAccessException, InvocationTargetException
+    private Object authoriseAndInvoke(final Method method, final Object[] args) throws Throwable
     {
         String methodName;
         // Get the component, type and impact, which may be null
@@ -229,23 +232,29 @@ public class MBeanInvocationHandlerImpl implements InvocationHandler
         Operation operation = (isAccessMethod(methodName) || impact == MBeanOperationInfo.INFO) ? Operation.ACCESS : Operation.UPDATE;
         security.authoriseMethod(operation, type, methodName);
 
-        boolean oldAccessChecksDisabled = false;
-        if(_managementRightsInferAllAccess)
+        if (_managementRightsInferAllAccess)
         {
-            oldAccessChecksDisabled = SecurityManager.setAccessChecksDisabled(true);
+            try
+            {
+                return Subject.doAs(SecurityManager.SYSTEM, new PrivilegedExceptionAction<Object>()
+                {
+                    @Override
+                    public Object run() throws IllegalAccessException, InvocationTargetException
+                    {
+                        return method.invoke(_mbs, args);
+                    }
+                });
+            }
+            catch (PrivilegedActionException e)
+            {
+                throw e.getCause();
+            }
         }
-
-        try
+        else
         {
             return method.invoke(_mbs, args);
         }
-        finally
-        {
-            if(_managementRightsInferAllAccess)
-            {
-                SecurityManager.setAccessChecksDisabled(oldAccessChecksDisabled);
-            }
-        }
+
     }
 
     private String getType(Method method, Object[] args)