Bug 1123778 - [ACL] Python client demands unnecessary permission / performs unnecessary actions
Summary: [ACL] Python client demands unnecessary permission / performs unnecessary act...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise MRG
Classification: Red Hat
Component: python-qpid
Version: Development
Hardware: Unspecified
OS: Unspecified
medium
high
Target Milestone: 3.2
: ---
Assignee: Ernie
QA Contact: Zdenek Kraus
URL:
Whiteboard:
Depends On:
Blocks: 1152090
TreeView+ depends on / blocked
 
Reported: 2014-07-28 08:59 UTC by Zdenek Kraus
Modified: 2015-10-08 13:08 UTC (History)
5 users (show)

Fixed In Version: python-qpid-0.34-1
Doc Type: Bug Fix
Doc Text:
Clone Of:
: 1152090 (view as bug list)
Environment:
Last Closed: 2015-10-08 13:08:57 UTC


Attachments (Terms of Use)
Patch proposal (3.66 KB, patch)
2014-08-14 07:26 UTC, Pavel Moravec
no flags Details | Diff
Patch proposal (not 100% ideal) (3.66 KB, patch)
2014-08-17 12:26 UTC, Pavel Moravec
no flags Details | Diff
Patch proposal for 0.22 downstream (2.71 KB, patch)
2014-09-16 12:48 UTC, Pavel Moravec
no flags Details | Diff
Updated patch to pass all python tests (4.08 KB, patch)
2015-06-12 15:57 UTC, Ernie
no flags Details | Diff


Links
System ID Priority Status Summary Last Updated
Apache JIRA QPID-6326 None None None Never
Red Hat Product Errata RHEA-2015:1879 normal SHIPPED_LIVE Red Hat Enterprise MRG Messaging 3.2 Release 2015-10-08 17:07:53 UTC

Description Zdenek Kraus 2014-07-28 08:59:20 UTC
Description of problem:
Python clients accesses both exchange and queue objects, even when the object types is specified. Thus demanding unnecessary ACL rules to be allowed.



Version-Release number of selected component (if applicable):
python-qpid-0.22-15

How reproducible:
100%

Steps to Reproduce:
Scenario A (access)
1. create acl:
acl allow-log all access exchange
acl deny-log all all
2. send message to an amq.fanout
/usr/share/doc/python-qpid-0.22/examples/api/spout -c 1 -b UserA/UserA@localhost:5672 "amq.fanout;{node:{type:topic}}"
3. check qpidd log

Scenario B (create)
1. create acl:
acl allow-log access all
acl allow-log create queue
acl deny-log all all
2. create a queue using spout
/usr/share/doc/python-qpid-0.22/examples/api/spout -c 1 -b UserA/UserA@localhost:5672 "q;{create:always, node:{type:queue}}"
3. check qpidd log


Scenario A
Actual results:
2014-07-28 10:45:07 [Security] info ACL Allow id:UserA@QPID action:access ObjectType:exchange Name:amq.fanout
2014-07-28 10:45:07 [Security] info ACL Deny id:UserA@QPID action:access ObjectType:queue Name:amq.fanout

Expected results:
2014-07-28 10:45:07 [Security] info ACL Allow id:UserA@QPID action:access ObjectType:exchange Name:amq.fanout
2014-07-28 10:45:07 [Security] info ACL Deny id:UserA@QPID action:publish ObjectType:exchange Name:amq.fanout


Scenario B
Actual results:
2014-07-28 10:57:31 [Security] info ACL Allow id:UserA@QPID action:access ObjectType:exchange Name:q
2014-07-28 10:57:31 [Security] info ACL Allow id:UserA@QPID action:access ObjectType:queue Name:q
2014-07-28 10:57:31 [Security] info ACL Allow id:UserA@QPID action:create ObjectType:queue Name:q

Expected results:
2014-07-28 10:57:31 [Security] info ACL Allow id:UserA@QPID action:create ObjectType:queue Name:q


Additional info:
[A] it behaves the same for node/type:queue, querying the exchanges then queue.
[B] when creating, client should request only 'create' action, same as C++

Comment 1 Pavel Moravec 2014-08-13 10:35:21 UTC
FYI the problem is bigger:

./python/examples/api/spout -c 1 "amq.fanout;{node:{type:queue}}"

should send a message to _queue_, but if there is no such queue, it sends the message to the _exchange_ :-/

Comment 2 Pavel Moravec 2014-08-13 12:06:01 UTC
Scenario A: expected results should be just with "action:access" and no "action:publish", as publish is for queue object (and in our case, the message is discarded by the exchange due to no route/binding).

Scenario B: proper ACL file content should be:

acl allow-log all access all
acl allow-log all create queue
acl deny-log all all

(two typos were there)

Btw. it is questonable if queue query in Scenario B can't be done (/me to think about it and ask gsim).

Apart of that, I have a patch:

Index: python/qpid/messaging/driver.py
===================================================================
--- python/qpid/messaging/driver.py	(revision 1617701)
+++ python/qpid/messaging/driver.py	(working copy)
@@ -122,9 +122,9 @@
 
     self.destinations = {}
 
-  def write_query(self, query, handler):
+  def write_query(self, query, handler, obj):
     id = self.sent
-    self.write_cmd(query, lambda: handler(self.results.pop(id)))
+    self.write_cmd(query, lambda: handler(self.results.pop(id), obj))
 
   def apply_overrides(self, cmd, overrides):
     for k, v in overrides.items():
@@ -1012,36 +1012,46 @@
       except KeyError:
         pass
 
-    args = []
-    def do_result(r):
-      args.append(r)
-    def do_action(r):
-      do_result(r)
-      er, qr = args
-      if node_type == "topic" and not er.not_found:
+    args = {"topic":None, "queue":None}
+    def do_result(r, obj):
+      args[obj] = r
+    def do_action(r, obj):
+      do_result(r, obj)
+      er = args["topic"]
+      qr = args["queue"]
+
+      if node_type == "topic" and er and not er.not_found:
         type, subtype = "topic", er.type
-      elif node_type == "queue" and qr.queue:
+      elif node_type == "queue" and qr and qr.queue:
         type, subtype = "queue", None
-      elif er.not_found and not qr.queue:
+      elif (er and er.not_found) and qr and not qr.queue:
         type, subtype = None, None
-      elif qr.queue:
+      elif (qr and qr.queue):
         if node_type == "topic" and force:
           type, subtype = None, None
         else:
           type, subtype = "queue", None
-      elif not er.not_found:
+      elif (er and not er.not_found):
         if node_type == "queue" and force:
           type, subtype = None, None
         else:
           type, subtype = "topic", er.type
+      elif er:
+        type, subtype = "topic", er.type
       else:
-        type, subtype = "topic", er.type
+        type, subtype = None, None
       if type is not None:
         self.address_cache[name] = (type, subtype)
       action(type, subtype)
-    sst.write_query(ExchangeQuery(name), do_result)
-    sst.write_query(QueueQuery(name), do_action)
 
+    if (node_type is None):
+      sst.write_query(ExchangeQuery(name), do_result, "topic")
+      sst.write_query(QueueQuery(name), do_action, "queue")
+    elif (node_type == "topic"):
+      sst.write_query(ExchangeQuery(name), do_action, "topic")
+    else:
+      sst.write_query(QueueQuery(name), do_action, "queue")
+
   def declare(self, sst, lnk, action, create_node):
     name = lnk.name
     props = lnk.options.get("node", {})

Comment 3 Pavel Moravec 2014-08-14 07:26:56 UTC
Created attachment 926687 [details]
Patch proposal

Patch proposal (before running automated tests)

Comment 4 Pavel Moravec 2014-08-17 12:26:44 UTC
Created attachment 927455 [details]
Patch proposal (not 100% ideal)

Better patch that fixes almost everything.

What it fixes:
- scenario A
- scenario B subcase "access:topic"
- spout -c 1 "amq.fanout;{node:{type:queue}}" (raises "qpid.messaging.exceptions.NotFound: no such queue: amq.fanout")

What it does not fix:
- scenario B subcase "access:queue"

The Python client is quite tighten to get in advance the knowledge of queue/topic objects on the broker, hence requiring the access ACLs checks. It can be limited only to accessing the type mentioned in address string as I did. Fixing the remaining scenario would require changing a lot of code I am not courage enough to do (dont want to infest a regression there).

Zdenek, could you please run your tests if there is no regression? Upstream automated tests went fine.

Comment 5 Zdenek Kraus 2014-08-18 13:23:14 UTC
I've launched the tests, but I'm afraid that your patch is tailored for the upstream python only. Firstly it don't apply same as previous one: 
#patch:
78:  def declare(self, sst, lnk, action, create_node):
#python-qpid-0.22-17 driver.py:
1021:  def declare(self, sst, lnk, action)

secondly because of the runtime error:

  Traceback (most recent call last):
    File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line 660, in write
      op.dispatch(self)
    File "/usr/lib/python2.6/site-packages/qpid/ops.py", line 84, in dispatch
      getattr(target, handler)(self, *args)
    File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line 780, in do_session_completed
      sst.actions.pop(sst.min_completion)()
    File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line 126, in <lambda>
      self.write_cmd(query, lambda: handler(self.results.pop(id), obj))
  TypeError: do_action() takes no arguments (2 given)

Comment 6 Pavel Moravec 2014-09-16 12:48:30 UTC
Created attachment 937991 [details]
Patch proposal for 0.22 downstream

Patch proposal for python-qpid-0.22-18.el6.noarch.

Zdenek, could you pls. run the tests?

Comment 7 Pavel Moravec 2014-09-16 12:49:01 UTC
(In reply to Zdenek Kraus from comment #5)
> I've launched the tests, but I'm afraid that your patch is tailored for the
> upstream python only. Firstly it don't apply same as previous one: 
> #patch:
> 78:  def declare(self, sst, lnk, action, create_node):
> #python-qpid-0.22-17 driver.py:
> 1021:  def declare(self, sst, lnk, action)
> 
> secondly because of the runtime error:
> 
>   Traceback (most recent call last):
>     File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line
> 660, in write
>       op.dispatch(self)
>     File "/usr/lib/python2.6/site-packages/qpid/ops.py", line 84, in dispatch
>       getattr(target, handler)(self, *args)
>     File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line
> 780, in do_session_completed
>       sst.actions.pop(sst.min_completion)()
>     File "/usr/lib/python2.6/site-packages/qpid/messaging/driver.py", line
> 126, in <lambda>
>       self.write_cmd(query, lambda: handler(self.results.pop(id), obj))
>   TypeError: do_action() takes no arguments (2 given)

That was a typo in the patch, attached is now correct version.

Comment 8 Zdenek Kraus 2014-09-17 18:54:59 UTC
I can confirm that the patch fixes the issue described.

Comment 15 Ernie 2015-06-12 15:57:38 UTC
Created attachment 1038103 [details]
Updated patch to pass all python tests

Several python tests were failing with the original patch.

Comment 18 Zdenek Kraus 2015-07-30 14:16:30 UTC
This was tested on RHEL 6 i686 & x86_64, RHEL 7 x86_64 with following packages:

python-qpid-0.34-1
python-qpid-qmf-0.34-1
qpid-cpp-client-0.34-1
qpid-cpp-client-devel-0.34-1
qpid-cpp-debuginfo-0.34-1
qpid-cpp-server-0.34-1
qpid-java-client-0.32-2
qpid-java-common-0.32-2
qpid-java-example-0.32-2
qpid-jms-client-0.2.0-2
qpid-jms-client-docs-0.2.0-2
qpid-jms-client-examples-0.2.0-2
qpid-jms-client-maven-repo-0.2.0-2
qpid-proton-c-0.9-4
qpid-proton-java-0.9.1-2
qpid-proton-java-maven-repo-0.9.1-2
qpid-qmf-0.34-1
qpid-tools-0.34-1


fix works as expected.

->VERIFIED

Comment 20 errata-xmlrpc 2015-10-08 13:08:57 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHEA-2015-1879.html


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