QPID-5599: C++ Broker silently ignores --max-connections option when no ACL file is loaded

Simply installing a null and permissive rule file trips up the 'create link'
security check. The security check from 
https://issues.apache.org/jira/browse/QPID-4631 reasons that if authentication
is enabled and no ACL rule file is specified then interbroker links are 
denied. The check for 'ACL rule file is loaded' is simply the existence of
the ACL object. That check is voided by always having an ACL object regardless
of whether the ACL rule file was specified or not.

One fix considered was adding an ACL rule "acl deny-log all create link" to
the formerly null rule set when no ACL file is specified. This solution has
too much complexity in several places and is too hard.

The fix implemented here is a boolean flag indicating if the ACL rule set 
in force is specified by the user or not. Then the security check tests
that the acl exists (always true) and that the rule set is specified by the
user. 



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

6 files changed, 23 insertions, 17 deletions

diff --git a/qpid/cpp/src/qpid/acl/Acl.cpp b/qpid/cpp/src/qpid/acl/Acl.cpp
index e75573b623..3d83cb0b2b 100644
--- a/qpid/cpp/src/qpid/acl/Acl.cpp
+++ b/qpid/cpp/src/qpid/acl/Acl.cpp
@@ -55,7 +55,8 @@ namespace _qmf = qmf::org::apache::qpid::acl;
 
 Acl::Acl (AclValues& av, Broker& b): aclValues(av), broker(&b), transferAcl(false),
     connectionCounter(new ConnectionCounter(*this, aclValues.aclMaxConnectPerUser, aclValues.aclMaxConnectPerIp, aclValues.aclMaxConnectTotal)),
-    resourceCounter(new ResourceCounter(*this, aclValues.aclMaxQueuesPerUser)){
+    resourceCounter(new ResourceCounter(*this, aclValues.aclMaxQueuesPerUser)),userRules(true)
+{
 
     if (aclValues.aclMaxConnectPerUser > AclData::getConnectMaxSpec())
         throw Exception("--connection-limit-per-user switch cannot be larger than " + AclData::getMaxConnectSpecStr());
@@ -86,6 +87,7 @@ Acl::Acl (AclValues& av, Broker& b): aclValues(av), broker(&b), transferAcl(fals
         }
     } else {
         loadEmptyAclRuleset();
+        userRules = false;
         QPID_LOG(debug, "ACL loaded empty rule set");
     }
     broker->getConnectionObservers().add(connectionCounter);
diff --git a/qpid/cpp/src/qpid/acl/Acl.h b/qpid/cpp/src/qpid/acl/Acl.h
index d1d7033fe3..655e1554f6 100644
--- a/qpid/cpp/src/qpid/acl/Acl.h
+++ b/qpid/cpp/src/qpid/acl/Acl.h
@@ -67,6 +67,7 @@ private:
     mutable qpid::sys::Mutex             dataLock;
     boost::shared_ptr<ConnectionCounter> connectionCounter;
     boost::shared_ptr<ResourceCounter>   resourceCounter;
+    bool                                 userRules;
 
 public:
     Acl (AclValues& av, broker::Broker& b);
@@ -85,6 +86,10 @@ public:
         return aclValues.aclMaxConnectTotal;
     };
 
+    inline virtual bool userAclRules() {
+      return userRules;
+    };
+
 // create specilied authorise methods for cases that need faster matching as needed.
     virtual bool authorise(
         const std::string&               id,
diff --git a/qpid/cpp/src/qpid/acl/AclPlugin.cpp b/qpid/cpp/src/qpid/acl/AclPlugin.cpp
index 04044867ec..77580ba531 100644
--- a/qpid/cpp/src/qpid/acl/AclPlugin.cpp
+++ b/qpid/cpp/src/qpid/acl/AclPlugin.cpp
@@ -62,20 +62,17 @@ struct AclPlugin : public Plugin {
     Options* getOptions() { return &options; }
 
     void init(broker::Broker& b) {
-    	if (acl) throw Exception("ACL plugin cannot be initialized twice in one process.");
-
-        if (values.aclFile.empty()){
-            QPID_LOG(info, "ACL Policy file not specified.");
-        } else {
-	  sys::Path aclFile(values.aclFile);
-	  sys::Path dataDir(b.getDataDir().getPath());
-	  if (!aclFile.isAbsolute() && !dataDir.empty())
-            values.aclFile =  (dataDir + aclFile).str();
-
-          acl = new Acl(values, b);
-	  b.setAcl(acl.get());
-	  b.addFinalizer(boost::bind(&AclPlugin::shutdown, this));
-	}
+        if (acl) throw Exception("ACL plugin cannot be initialized twice in one process.");
+
+        if (!values.aclFile.empty()){
+            sys::Path aclFile(values.aclFile);
+            sys::Path dataDir(b.getDataDir().getPath());
+            if (!aclFile.isAbsolute() && !dataDir.empty())
+                values.aclFile =  (dataDir + aclFile).str();
+        }
+        acl = new Acl(values, b);
+        b.setAcl(acl.get());
+        b.addFinalizer(boost::bind(&AclPlugin::shutdown, this));
     }
 
     template <class T> bool init(Plugin::Target& target) {
diff --git a/qpid/cpp/src/qpid/broker/AclModule.h b/qpid/cpp/src/qpid/broker/AclModule.h
index aa0ea0c6b0..f97c932e27 100644
--- a/qpid/cpp/src/qpid/broker/AclModule.h
+++ b/qpid/cpp/src/qpid/broker/AclModule.h
@@ -142,6 +142,8 @@ namespace broker {
 
         virtual uint16_t getMaxConnectTotal()=0;
 
+        virtual bool userAclRules()=0;
+
         virtual bool authorise(
             const std::string&                    id,
             const acl::Action&                    action,
diff --git a/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp b/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp
index 09f774da62..fe8a84dcce 100644
--- a/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp
+++ b/qpid/cpp/src/qpid/broker/ConnectionHandler.cpp
@@ -198,7 +198,7 @@ void ConnectionHandler::Handler::startOk(const ConnectionStartOkBody& body)
     }
     if (connection.isFederationLink()) {
         AclModule* acl =  connection.getBroker().getAcl();
-        if (acl) {
+        if (acl && acl->userAclRules()) {
             if (!acl->authorise(connection.getUserId(),acl::ACT_CREATE,acl::OBJ_LINK,"")){
                 proxy.close(framing::connection::CLOSE_CODE_CONNECTION_FORCED,
                             QPID_MSG("ACL denied " << connection.getUserId()
diff --git a/qpid/cpp/src/qpid/broker/amqp/Authorise.cpp b/qpid/cpp/src/qpid/broker/amqp/Authorise.cpp
index 28dc3b25ac..3c88414567 100644
--- a/qpid/cpp/src/qpid/broker/amqp/Authorise.cpp
+++ b/qpid/cpp/src/qpid/broker/amqp/Authorise.cpp
@@ -121,7 +121,7 @@ void Authorise::route(boost::shared_ptr<Exchange> exchange, const Message& msg)
 
 void Authorise::interlink()
 {
-    if (acl) {
+    if (acl && acl->userAclRules()) {
         if (!acl->authorise(user, acl::ACT_CREATE, acl::OBJ_LINK, "")){
             throw Exception(qpid::amqp::error_conditions::UNAUTHORIZED_ACCESS, QPID_MSG("ACL denied " << user << "  a AMQP 1.0 link"));
         }