Qpid
  1. Qpid
  2. QPID-4775

ACL delete action should not ignore object's properties other than name

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 0.18, 0.20
    • Fix Version/s: 0.23
    • Component/s: C++ Broker
    • Labels:

      Description

      Description of problem:
      ACL rule like:

      acl allow all delete queue autodelete=true

      should allow deletion of autodelete queues only. While any queue can be deleted. The same applies to any object's property other than queue's name (see Broker::deleteQueue method and how acl->authorise is called).

      The same applies not only to queues but also to exchanges.

      Version-Release number of selected component (if applicable):
      any

      How reproducible:
      100%

      Steps to Reproduce:
      1. cat <acl-file>

      1. simply allow all except for deleting non-durable queue
        acl allow-log all consume all
        acl allow-log all publish all
        acl allow-log all create all
        acl allow-log all access all
        acl allow-log all bind all
        acl allow-log all unbind all
        acl allow-log all purge all
        acl allow-log all update all
        acl allow-log all delete exchange
        acl allow-log all delete queue durable=true
        acl deny-log all all

      2. Start broker with auth=yes and the ACL file
      3. qpid-config -b admin/admin@localhost:5672 add queue TransientQueue
      4. qpid-config -b admin/admin@localhost:5672 del queue TransientQueue

      Actual results:
      Steps 3 and 4 pass.

      Expected results:
      Deleting queue should fail, as the queue is not durable.

      In fact, even creating the queue that way should raise an exception, as deleting auxiliary queue named like "4135cd9e-04b8-4cef-bcd0-5404444d7a04:0.0" (where the qpid-config gets response) should fail.

      Additional info:
      Same scenarios are applicable for all other queue properties and/or exchange properties. Just queue/exchange name is checked.

      Patch proposed.

      1. bz955674.patch
        4 kB
        Pavel Moravec

        Activity

        Hide
        Justin Ross added a comment -
        Show
        Justin Ross added a comment - Released in Qpid 0.24, http://qpid.apache.org/releases/qpid-0.24/index.html
        Hide
        Chuck Rolke added a comment -

        Fixed with Committed revision 1478418.

        The patch was for 0.18-based code and needed adjustment for trunk.
        The various MAX-SIZE properties, which make sense for queue creation, were removed from the ACL paramenter list for queue deletion.

        Show
        Chuck Rolke added a comment - Fixed with Committed revision 1478418. The patch was for 0.18-based code and needed adjustment for trunk. The various MAX-SIZE properties, which make sense for queue creation, were removed from the ACL paramenter list for queue deletion.
        Hide
        Chuck Rolke added a comment -

        Applying this patch changes one behavior that breaks self tests. Before the patch the delete tests ran the ACL query first and if it fails then it returns a 403 UNAUTHORIZED_ACCESS error. After the patch the delete tests check for the existence of the object to be deleted and if it doesn't exist it return a 404 NOT_FOUND error.

        Show
        Chuck Rolke added a comment - Applying this patch changes one behavior that breaks self tests. Before the patch the delete tests ran the ACL query first and if it fails then it returns a 403 UNAUTHORIZED_ACCESS error. After the patch the delete tests check for the existence of the object to be deleted and if it doesn't exist it return a 404 NOT_FOUND error.
        Hide
        Chuck Rolke added a comment -

        I think this patch makes sense. In a queue/exchange creation the broker receives attribute information over the wire and presents that to ACL for approval. In the delete case only the object names are sent over the wire and not the attributes. This patch simply picks a few attributes from the actual queue and uses them to further qualify the ACL authorization.

        I will commit this patch soon with the exception of the size qualifiers.

        Show
        Chuck Rolke added a comment - I think this patch makes sense. In a queue/exchange creation the broker receives attribute information over the wire and presents that to ACL for approval. In the delete case only the object names are sent over the wire and not the attributes. This patch simply picks a few attributes from the actual queue and uses them to further qualify the ACL authorization. I will commit this patch soon with the exception of the size qualifiers.
        Hide
        Pavel Moravec added a comment -

        Patch proposal (applicable to 0.18 broker but can differ from 0.20 in line positions only).

        Show
        Pavel Moravec added a comment - Patch proposal (applicable to 0.18 broker but can differ from 0.20 in line positions only).

          People

          • Assignee:
            Unassigned
            Reporter:
            Pavel Moravec
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development