From 741aac49fb2e414a53de26e07b67a5584246c11a Mon Sep 17 00:00:00 2001 From: "Charles E. Rolke" Date: Wed, 5 Mar 2014 02:33:46 +0000 Subject: 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 --- qpid/cpp/src/qpid/acl/Acl.cpp | 4 +++- qpid/cpp/src/qpid/acl/Acl.h | 5 +++++ qpid/cpp/src/qpid/acl/AclPlugin.cpp | 25 +++++++++++-------------- qpid/cpp/src/qpid/broker/AclModule.h | 2 ++ qpid/cpp/src/qpid/broker/ConnectionHandler.cpp | 2 +- qpid/cpp/src/qpid/broker/amqp/Authorise.cpp | 2 +- 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; boost::shared_ptr 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 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, 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")); } -- cgit v1.2.1