Qpid
  1. Qpid
  2. QPID-4032

Broker does not accept sub-groups in group declaration, contrary to wiki documentation

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.14, 0.16
    • Fix Version/s: 0.17
    • Component/s: C++ Broker
    • Labels:
    • Environment:

      Linux 2.6.32-5-amd64 #1 SMP Thu Mar 22 17:26:33 UTC 2012 x86_64 GNU/Linux
      gcc (Debian 4.4.5-8) 4.4.5

      Description

      The ACL wiki page (https://cwiki.apache.org/qpid/acl.html) indicates by at least two examples, that group declarations can contain sub-groups. However, Qpid 0.16 does not (seem to) allow this.

      For example, given the following ACL file (cut down from the example at https://cwiki.apache.org/qpid/acl.html#ACL-Examplefile%253A):

      # Some groups
      group user-consume martin@QPID ted@QPID
      group group2 kim@QPID user-consume rob@QPID
      

      Qpid 0.16 gives the following error:

      ACL format error: /home/paul/.qpidd/qpidd.acl:3: Line : 3, Username 'user-consume' must contain a realm

      It appears that the broker is requiring user-consume to be a qualified user name such as user-consume@QPID and not realizing that it is a group name instead.

        Activity

        Hide
        Paul Colby added a comment -

        Problem exists in 0.14 too.

        Show
        Paul Colby added a comment - Problem exists in 0.14 too.
        Hide
        Paul Colby added a comment -

        Also looks (not tested) to be broken in 0.8 (and presumably 0.10).

        I have a patch. Just tracing the history in svn now, and will attach a patch soon...

        Show
        Paul Colby added a comment - Also looks (not tested) to be broken in 0.8 (and presumably 0.10). I have a patch. Just tracing the history in svn now, and will attach a patch soon...
        Hide
        Paul Colby added a comment -

        Ok, it looks to me that this was broken way back before 0.6 was branched.

        Specifically, it was broken in two separate commits (either of which were enough to break it) - the two commits are:

        • r728880 "This is a fix for QPID-1545"
          • it seems that this commit tried to fix an issue where an ACL group contained a recursive sub-group, eg: group admin admin however, that solution was incorrect. It actually turned an "if" condition into one that can never evaluate to true. It incorrectly compares the name of a found group, with the very same name used to find that group, and insists that they don't match. It seems that what they meant to do, was compare the name of the found group with name of the group being added to, however the latter group name is not available within the AclReader::addName function.
        • r732466 "This is related to QPID-1558. The test case test_group_and_user_with_same_name covers the error condition in QPID-1545"
          • this commit was intended to "Reject ACL file if user names does not contain a realm". However, it failed to allow for sub-group names (as documented as valid in the wiki), and so now sub-groups result in the current error message (Username '<group>' must contain a realm).

        The patch I'll attach in a minute does two things:

        1. Revert r728880 - the AclReader::addName can't do what that commit wanted without extended the function signature to either include the name that the groupNameSet parameter came from, or upgrade the groupNameSet argument from a nameSetPtr to the gmCitr from which it was derived. My patch (in step 2 below) performs equivalent work elsewhere.
        2. Add checks to the AclReader::processGroupLine for valid sub-group names, and if found, defends against:
          1. recursive sub-groups (eg: group admin admin) in which case a debug message is included, but otherwise the recursion is safely ignored.
          2. non-existent subgroups (eg: group admin this-group-does-not-exist-yet) in which case an error is returned, and the ACL file rejected.

        I believe that my patch restores the original sub-group capability as described at https://cwiki.apache.org/qpid/acl.html, while protecting against the issues raised in both QPID-1545 and QPID-1558.

        Let me know what you think

        Show
        Paul Colby added a comment - Ok, it looks to me that this was broken way back before 0.6 was branched. Specifically, it was broken in two separate commits (either of which were enough to break it) - the two commits are: r728880 "This is a fix for QPID-1545 " it seems that this commit tried to fix an issue where an ACL group contained a recursive sub-group, eg: group admin admin however, that solution was incorrect. It actually turned an "if" condition into one that can never evaluate to true. It incorrectly compares the name of a found group, with the very same name used to find that group, and insists that they don't match. It seems that what they meant to do, was compare the name of the found group with name of the group being added to, however the latter group name is not available within the AclReader::addName function. r732466 "This is related to QPID-1558 . The test case test_group_and_user_with_same_name covers the error condition in QPID-1545 " this commit was intended to "Reject ACL file if user names does not contain a realm". However, it failed to allow for sub-group names (as documented as valid in the wiki), and so now sub-groups result in the current error message ( Username '<group>' must contain a realm ). The patch I'll attach in a minute does two things: Revert r728880 - the AclReader::addName can't do what that commit wanted without extended the function signature to either include the name that the groupNameSet parameter came from, or upgrade the groupNameSet argument from a nameSetPtr to the gmCitr from which it was derived. My patch (in step 2 below) performs equivalent work elsewhere. Add checks to the AclReader::processGroupLine for valid sub-group names, and if found, defends against: recursive sub-groups (eg: group admin admin ) in which case a debug message is included, but otherwise the recursion is safely ignored. non-existent subgroups (eg: group admin this-group-does-not-exist-yet ) in which case an error is returned, and the ACL file rejected. I believe that my patch restores the original sub-group capability as described at https://cwiki.apache.org/qpid/acl.html , while protecting against the issues raised in both QPID-1545 and QPID-1558 . Let me know what you think
        Hide
        Paul Colby added a comment -

        Patch - as described above.

        Show
        Paul Colby added a comment - Patch - as described above.

          People

          • Assignee:
            Chuck Rolke
            Reporter:
            Paul Colby
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development