Bug 1269442 - Qpid broker doesn't pass credentials to ACL query when accepting an incoming link
Qpid broker doesn't pass credentials to ACL query when accepting an incoming ...
Status: NEW
Product: Red Hat Enterprise MRG
Classification: Red Hat
Component: qpid-cpp (Show other bugs)
3.1
Unspecified Unspecified
unspecified Severity medium
: ---
: ---
Assigned To: messaging-bugs
Messaging QE
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-07 07:22 EDT by Barbora Vassova
Modified: 2015-12-21 04:43 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed:
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Barbora Vassova 2015-10-07 07:22:29 EDT
Description of problem:
When using PLAIN authentication with ACL, and when creating outgoing link, ACL checks empty user instead of the one supplied in authentication. That causes federation link isn't created. 

Version-Release number of selected component (if applicable):
qpid-cpp-server-0.30-8.el6.x86_64

How reproducible:
Always.

Steps to Reproduce:
1. Set the ACL file to:

acl allow admin@QPID all all
acl deny all all

and via saslpaswd2 create used called "admin" in realm "QPID" with password "admin". 

2. Run this federation script: 

pkill -9 qpidd
rm qpidd.*.log

run_qpidd() {
    port=$1
    shift
    rm -rf _${port}
    mkdir _${port}
    nohup qpidd --port=$port --log-to-file=qpidd.$port.log --data-dir=_${port} --auth=yes --realm=QPID --acl-file /var/lib/qpidd/qpidd.acl --log-to-stderr=no --log-enable=trace+:acl --log-to-syslog=no "$@" > /dev/null 2>&1 &
    sleep 1
}

echo "$(date): starting brokers"
run_qpidd 29701 --trace
run_qpidd 29700 --trace

sleep 2
qpid-config add queue queue1 -b admin/admin@localhost:29701
sleep 1
qpid-config add queue queue2 -b admin/admin@localhost:29700
sleep 1
qpid-config add domain srcBrokerDomain --argument url=$(hostname):29700 --argument username=admin --argument password=admin --argument sasl_mechanisms=PLAIN -b admin/admin@localhost:29701
sleep 1
qpid-config add outgoing my-test-link --argument source=queue1 --argument target=queue2 --argument domain=srcBrokerDomain -b admin/admin@localhost:29701
sleep 1 
qpid-send -a queue1 -m 1000 -b admin/admin@localhost:29701 --content-size=1

Actual results:
Broker fails to access queue1 due to ACL denying access because of missing credentials. 

2015-10-07 12:22:50 [Protocol] trace [qpid.10.34.84.251:29700-10.34.84.251:56006]: 0 <- @close(24) [error=@error(29) [condition=:"amqp:unauthorized-access", description="ACL denied access request to queue1 from  (/builddir/build/BUILD/qpid-cpp-0.30/src/qpid/broker/amqp/Authorise.cpp:144)"]]
2015-10-07 12:22:50 [Broker] error my-test-link@srcBrokerDomain: ACL denied access request to queue1 from  (/builddir/build/BUILD/qpid-cpp-0.30/src/qpid/broker/amqp/Authorise.cpp:144)
2015-10-07 12:22:50 [Protocol] trace [my-test-link@srcBrokerDomain]: 0 -> @close(24) [error=@error(29) [condition=:"amqp:unauthorized-access", description="ACL denied access request to queue1 from  (/builddir/build/BUILD/qpid-cpp-0.30/src/qpid/broker/amqp/Authorise.cpp:144)"]]
2015-10-07 12:22:50 [Security] debug ACL: Lookup for id: action:access objectType:exchange name:queue1 with params { }


Expected results:
Broker creates a federation and delivers 1000 messages

Additional info:
Comment 1 Pavel Moravec 2015-12-19 06:30:20 EST
The broker initiating the federation link connection has qpid::broker::amqp::Connection::userid empty. This causes qpid::broker::amqp::Authorise class to get empty user that is passed to the ACL check.

userid in Connection is set only in qpid::broker::Sasl::respond, i.e. when this broker behaves as server, not as a client for the connection.

For client side connections, a method like

void LinkRegistry::notifyConnection(const std::string& key, amqp_0_10::Connection* c)

needs to be implemented - for AMQP 1.0.
Comment 2 Pavel Moravec 2015-12-19 08:26:55 EST
(In reply to Pavel Moravec from comment #1)
> The broker initiating the federation link connection has
> qpid::broker::amqp::Connection::userid empty. This causes
> qpid::broker::amqp::Authorise class to get empty user that is passed to the
> ACL check.
> 
> userid in Connection is set only in qpid::broker::Sasl::respond, i.e. when
> this broker behaves as server, not as a client for the connection.
> 
> For client side connections, a method like
> 
> void LinkRegistry::notifyConnection(const std::string& key,
> amqp_0_10::Connection* c)
> 
> needs to be implemented - for AMQP 1.0.

Patch proposal:

Index: src/qpid/broker/amqp/Domain.h
===================================================================
--- src/qpid/broker/amqp/Domain.h	(revision 1720894)
+++ src/qpid/broker/amqp/Domain.h	(working copy)
@@ -57,6 +57,7 @@
     std::auto_ptr<qpid::Sasl> sasl(const std::string& hostname);
     const std::string& getMechanisms() const;
     qpid::Url getUrl() const;
+    std::string getUsername() const { return username; }
     bool isDurable() const;
     void addPending(boost::shared_ptr<InterconnectFactory>);
     void removePending(boost::shared_ptr<InterconnectFactory>);
Index: src/qpid/broker/amqp/Interconnect.cpp
===================================================================
--- src/qpid/broker/amqp/Interconnect.cpp	(revision 1720894)
+++ src/qpid/broker/amqp/Interconnect.cpp	(working copy)
@@ -18,6 +18,7 @@
  * under the License.
  *
  */
+#include "Domain.h"
 #include "Interconnect.h"
 #include "Interconnects.h"
 #include "Connection.h"
@@ -30,6 +31,7 @@
 #include "qpid/sys/OutputControl.h"
 #include "qpid/sys/SystemInfo.h"
 #include "qpid/log/Statement.h"
+#include <boost/format.hpp>
 #include <boost/shared_ptr.hpp>
 extern "C" {
 #include <proton/engine.h>
@@ -124,6 +126,9 @@
                 setContainerId(std::string(containerid));
             }
             opened();
+	    ManagedConnection::setUserId(boost::str(boost::format("%1%@%2%") 
+		% getInterconnects().findDomain(domain)->getUsername()
+		% getBroker().getRealm()));
             getBroker().getConnectionObservers().opened(*this);
             pn_session_t* s = pn_session(connection);
             pn_session_open(s);


Rationale behind patch: 
- Interconnect::process - when opening outgoing connection - needs to set userId (maintained in relevant Domain)
- Domain::getUsername method added to get that
- ManagedConnection::setUserId called to ignore calling ACL stuff from Connection::setUserId


Gordon, as the author of relevant code, could you please comment the patch before I commit it to upstream? (or do you prefer official upstream commit request?)
Comment 3 Gordon Sim 2015-12-21 04:43:02 EST
(In reply to Pavel Moravec from comment #2)
> (In reply to Pavel Moravec from comment #1)
> > The broker initiating the federation link connection has
> > qpid::broker::amqp::Connection::userid empty. This causes
> > qpid::broker::amqp::Authorise class to get empty user that is passed to the
> > ACL check.
> > 
> > userid in Connection is set only in qpid::broker::Sasl::respond, i.e. when
> > this broker behaves as server, not as a client for the connection.
> > 
> > For client side connections, a method like
> > 
> > void LinkRegistry::notifyConnection(const std::string& key,
> > amqp_0_10::Connection* c)
> > 
> > needs to be implemented - for AMQP 1.0.
> 
> Patch proposal:
> 
> Index: src/qpid/broker/amqp/Domain.h
> ===================================================================
> --- src/qpid/broker/amqp/Domain.h	(revision 1720894)
> +++ src/qpid/broker/amqp/Domain.h	(working copy)
> @@ -57,6 +57,7 @@
>      std::auto_ptr<qpid::Sasl> sasl(const std::string& hostname);
>      const std::string& getMechanisms() const;
>      qpid::Url getUrl() const;
> +    std::string getUsername() const { return username; }
>      bool isDurable() const;
>      void addPending(boost::shared_ptr<InterconnectFactory>);
>      void removePending(boost::shared_ptr<InterconnectFactory>);
> Index: src/qpid/broker/amqp/Interconnect.cpp
> ===================================================================
> --- src/qpid/broker/amqp/Interconnect.cpp	(revision 1720894)
> +++ src/qpid/broker/amqp/Interconnect.cpp	(working copy)
> @@ -18,6 +18,7 @@
>   * under the License.
>   *
>   */
> +#include "Domain.h"
>  #include "Interconnect.h"
>  #include "Interconnects.h"
>  #include "Connection.h"
> @@ -30,6 +31,7 @@
>  #include "qpid/sys/OutputControl.h"
>  #include "qpid/sys/SystemInfo.h"
>  #include "qpid/log/Statement.h"
> +#include <boost/format.hpp>
>  #include <boost/shared_ptr.hpp>
>  extern "C" {
>  #include <proton/engine.h>
> @@ -124,6 +126,9 @@
>                  setContainerId(std::string(containerid));
>              }
>              opened();
> +	    ManagedConnection::setUserId(boost::str(boost::format("%1%@%2%") 
> +		% getInterconnects().findDomain(domain)->getUsername()
> +		% getBroker().getRealm()));
>              getBroker().getConnectionObservers().opened(*this);
>              pn_session_t* s = pn_session(connection);
>              pn_session_open(s);
> 
> 
> Rationale behind patch: 
> - Interconnect::process - when opening outgoing connection - needs to set
> userId (maintained in relevant Domain)
> - Domain::getUsername method added to get that
> - ManagedConnection::setUserId called to ignore calling ACL stuff from
> Connection::setUserId
> 
> 
> Gordon, as the author of relevant code, could you please comment the patch
> before I commit it to upstream? (or do you prefer official upstream commit
> request?)

I believe this patch uses the username specified for connection to the remote domain for the purposes of local authorisation. Ideally I think it would be using the userid of the user who triggered the connect request. Though in the bug description these are the same, in general they might not be, and the remote user credentials are not authenticated locally in anyway.

Note You need to log in before you can comment on or make changes to this bug.