Bug 1066372 - ACL to take default broker values instead of 0 for queue create parameters
Summary: ACL to take default broker values instead of 0 for queue create parameters
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise MRG
Classification: Red Hat
Component: qpid-cpp
Version: 3.0
Hardware: All
OS: All
medium
medium
Target Milestone: 3.1
: ---
Assignee: Pavel Moravec
QA Contact: Petra Svobodová
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-02-18 09:58 UTC by Pavel Moravec
Modified: 2019-06-13 07:57 UTC (History)
5 users (show)

Fixed In Version: qpid-cpp-0.30-2
Doc Type: Bug Fix
Doc Text:
ACL rules check that queue creation sizes are within bounds. When a user request was received and that request did not specify one of the checked sizes, the ACL proceeded checking the ACL rules as if the user had specified zero. For the same user request the broker substituted a default value which was not zero. This caused the ACL rule checks to not render the correct allow or deny decisions. The ACL rule checking behavior is now improved. When the user does not specify a value, the ACL rules are processed using the broker default values and not zero. ACL rule checks are processed using the same values that the broker uses to create the queue. ACL rule checks no longer produce false allow or deny decisions based on queue size limits.
Clone Of:
Environment:
Last Closed: 2015-04-14 13:47:30 UTC
Target Upstream Version:


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Apache JIRA QPID-5565 None None None Never
Red Hat Product Errata RHEA-2015:0805 normal SHIPPED_LIVE Red Hat Enterprise MRG Messaging 3.1 Release 2015-04-14 17:45:54 UTC

Description Pavel Moravec 2014-02-18 09:58:03 UTC
Description of problem:
If a queue is created without specifying any parameters (like paging queue without page_factor or pages_loaded set, or durable queue without filecount / filesize set), the default values from broker configuration are used. However, the ACL rules are in such case tested against _zero_ values. That brings problems when e.g. ACLs require pages_loaded > 10.


Version-Release number of selected component (if applicable):
qpid-cpp-broker (any version incl. 0.22-35)


How reproducible:
100%


Steps to Reproduce:
1. Having ACL file like:
acl allow all create queue paging=true pageslowerlimit=1 pagesupperlimit=100 pagefactorlowerlimit=1 pagefactorlowerlimit=100
acl allow all consume
acl allow all access
acl allow all bind
acl deny all all

2. qpid-receive -a "q1; {create:always, node:{type:queue, x-declare:{arguments:{ 'qpid.paging':true }}}}"

3. qpid-receive -a "q2; {create:always, node:{type:queue, x-declare:{arguments:{ 'qpid.paging':true, 'qpid.max_pages_loaded':4, 'qpid.page_factor':1 }}}}"
(note, qpid.max_pages_loaded has default value 4 and qpid.page_factor has default value 1)


Actual results:
q1 is not created with "ACL denied queue create request from anonymous@QPID"
q2 is created (though with the same parameters)


Expected results:
Both q1 and q2 to be created


Additional info:

Comment 1 Pavel Moravec 2014-02-18 15:08:38 UTC
Preliminary patch being tested:

$ svn diff .
Index: src/qpid/broker/Broker.cpp
===================================================================
--- src/qpid/broker/Broker.cpp	(revision 1569293)
+++ src/qpid/broker/Broker.cpp	(working copy)
@@ -1306,13 +1306,15 @@
         params.insert(make_pair(acl::PROP_AUTODELETE, settings.autodelete ? _TRUE : _FALSE));
         params.insert(make_pair(acl::PROP_POLICYTYPE, settings.getLimitPolicy()));
         params.insert(make_pair(acl::PROP_PAGING, settings.paging ? _TRUE : _FALSE));
-        params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages)));
-        params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor)));
-        params.insert(make_pair(acl::PROP_MAXQUEUECOUNT, boost::lexical_cast<string>(settings.maxDepth.getCount())));
-        params.insert(make_pair(acl::PROP_MAXQUEUESIZE, boost::lexical_cast<string>(settings.maxDepth.getSize())));
-        params.insert(make_pair(acl::PROP_MAXFILECOUNT, boost::lexical_cast<string>(settings.maxFileCount)));
-        params.insert(make_pair(acl::PROP_MAXFILESIZE, boost::lexical_cast<string>(settings.maxFileSize)));
-
+        params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES)));
+        params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR)));
+        if (settings.maxDepth.hasCount())
+            params.insert(make_pair(acl::PROP_MAXQUEUECOUNT, boost::lexical_cast<string>(settings.maxDepth.getCount())));
+        params.insert(make_pair(acl::PROP_MAXQUEUESIZE, boost::lexical_cast<string>(settings.maxDepth.hasSize() ? settings.maxDepth.getSize() : config.queueLimit)));
+        if (settings.durable) {
+            params.insert(make_pair(acl::PROP_MAXFILECOUNT, boost::lexical_cast<string>(settings.maxFileCount ? settings.maxFileCount : 8)));
+            params.insert(make_pair(acl::PROP_MAXFILESIZE, boost::lexical_cast<string>(settings.maxFileSize ? settings.maxFileSize : 24)));
+        }
         if (!acl->authorise(userId,acl::ACT_CREATE,acl::OBJ_QUEUE,name,&params) )
             throw framing::UnauthorizedAccessException(QPID_MSG("ACL denied queue create request from " << userId));
 
Index: src/qpid/broker/QueueFactory.cpp
===================================================================
--- src/qpid/broker/QueueFactory.cpp	(revision 1569293)
+++ src/qpid/broker/QueueFactory.cpp	(working copy)
@@ -80,8 +80,8 @@
             QPID_LOG(warning, "Cannot create paged queue; no paging directory enabled");
         } else {
             queue->messages = std::auto_ptr<Messages>(new PagedQueue(name, broker->getPagingDir().getPath(),
-                                                                     settings.maxPages ? settings.maxPages : 4,
-                                                                     settings.pageFactor ? settings.pageFactor : 1,
+                                                                     settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES,
+                                                                     settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR,
                                                                      broker->getProtocolRegistry(), broker->getExpiryPolicy()));
         }
     } else if (settings.lvqKey.empty()) {//LVQ already handled above
Index: src/qpid/broker/QueueFactory.h
===================================================================
--- src/qpid/broker/QueueFactory.h	(revision 1569293)
+++ src/qpid/broker/QueueFactory.h	(working copy)
@@ -25,6 +25,9 @@
 #include "qpid/types/Variant.h"
 #include <boost/shared_ptr.hpp>
 
+#define DEFAULT_MAX_PAGES 4
+#define DEFAULT_PAGE_FACTOR 1
+
 namespace qpid {
 namespace management {
 class Manageable;
$

Comment 2 Chuck Rolke 2014-02-18 21:20:36 UTC
Patch in Comment 1 is changing the broker logic during the creation of queues. The current (failing) method sends a zero to ACL for testing when the user does not specify a value. The patched method sends the default value (that will be used when the user specifies none) to ACL for testing.

+1 Ship It!

Comment 3 Pavel Moravec 2014-02-19 08:47:15 UTC
(In reply to Chuck Rolke from comment #2)
> Patch in Comment 1 is changing the broker logic during the creation of
> queues. The current (failing) method sends a zero to ACL for testing when
> the user does not specify a value. The patched method sends the default
> value (that will be used when the user specifies none) to ACL for testing.
> 
> +1 Ship It!

Thanks for review, I originally planned to ask for in in upstream, but why not here (as I suppose you will be the only one interested atm). Still I have two doubts here, as even my patch is ideal:

1) (minor issue) "if (settings.durable) {" test: 
this would "break" ACL rule like:

acl allow all create queue filemaxcountupperlimit=16

where obviously "durable=true" misses there. So the ACL line above prevents creating transient queues at all (it allows queues with maxFileSize<=16 but that is taken into account only for durable queues). On the other side, the above rule can be seen as invalid, as filemaxcountupperlimit pre-assumes durable=true that misses there.

Also, I don't like copy&pasting constants (like "static const u_int32_t defJrnlFileSizePgs = 24;" -> "settings.maxFileSize ? settings.maxFileSize : 24" but dont see an easy way of getting the constant value from legacystore (on the other hand, I suppose no change in the values so I am fine with it)


2) (bigger issue) "if (settings.maxDepth.hasCount())" test:
Assume ACL:

acl allow all create queue queuemaxcountlowerlimit=10

that should pass default queue creation with no limits to queue depth imposed. But ACL does not match:

2014-02-19 09:38:15 [Security] debug ACL: checking rule [rule 1 ruleMode = allow props{ queuemaxcountlowerlimit=10 }]
2014-02-19 09:38:15 [Security] debug ACL: lookup parameter map doesn't contain the rule property 'queuemaxcountlowerlimit'
2014-02-19 09:38:15 [Security] debug ACL: No successful match, defaulting to the decision mode deny

(This is very related to 0 standing for unlimited value)


3) (modification): add equivalent test to durability: 

+        if (settings.paging) {
+            params.insert(make_pair(acl::PROP_MAXPAGES, boost::lexical_cast<string>(settings.maxPages ? settings.maxPages : DEFAULT_MAX_PAGES)));
+            params.insert(make_pair(acl::PROP_MAXPAGEFACTOR, boost::lexical_cast<string>(settings.pageFactor ? settings.pageFactor : DEFAULT_PAGE_FACTOR)));
+        }


I am about to post it to upsetram(*) and file bug "ACL should treat zero as unlimitted value".

(*) once automated tests finish, as in upstream they fail even without mine patch

Comment 4 Chuck Rolke 2014-02-19 16:02:01 UTC
These rules are complicated and figuring out a rule's implication is difficult: My code does X -> the client library sends Y on the wire to the broker -> the broker sends Z to ACL for lookup.

Ok, don't ship it yet.

---
In issue 1) the rule
  acl allow all create queue filemaxcountupperlimit=16

could be fixed with
  acl allow all create queue filemaxcountupperlimit=16 durable=true

That doesn't seem like such a problem as long as some examples and guidance are provided.
---
In issue 1) yes, copying and pasting constants is bad. 

I suggest that we may be able to avoid using the constants at all by introducing the concept of the ACL limit rule value"default". Then you can specify a rule that says what to do when the user didn't specify the value. Back in the broker the code would not send the default value to ACL (as proposed in your patch) but send the zero if the value was not specified (or if the user actually specified zero). Essentially this means don't change broker lookup code but add new code to ACL lookups.

Applied to issue 2) you could have ACL

  acl allow all create queue queuemaxcountlowerlimit=default
  acl allow all create queue queuemaxcountlowerlimit=10

The rule using 'default' would match if the item (queuemaxcountlowerlimit) is absent in the lookup or present in the lookup but with a value of zero. Together these rules specify that if no queuemaxcountlowerlimit was specified by the user then allow; failing that if the user specifies a queuemaxcountlower limit then the value must be >= 10.

The appeal to the 'default' rule value is that you don't have to discover or make up the default values in broker code. It gives ACL a way to have explicit rules governing the default case.

---
Issue 3) is also solved by the 'default' rule addition and by the requirement to have "paging=true" in the rule before the rule applies.

Comment 5 Pavel Moravec 2014-02-20 08:53:24 UTC
(In reply to Chuck Rolke from comment #4)
> My code does X -> the client library sends Y on the wire to the broker -> the
> broker sends Z to ACL for lookup.
The corret flow is rather "my code does X that is sent over wire -> broker interprets it as Y -> ACL understands it as Z (0=0 but not unlimited as now)", no?

> I suggest that we may be able to avoid using the constants at all by
> introducing the concept of the ACL limit rule value"default". Then you can
> specify a rule that says what to do when the user didn't specify the value.
> Back in the broker the code would not send the default value to ACL (as
> proposed in your patch) but send the zero if the value was not specified (or
> if the user actually specified zero). Essentially this means don't change
> broker lookup code but add new code to ACL lookups.
So, if a user does not specify a property, then the broker will not set the ACL property and ACL will assume it is "default" value? Assume ACL:

acl allow all queue create queuemaxsizeupperlimit=104857600
(the number is the value of --default-queue-limit broker parameter)

Will that ACL rule be met in these situations?
a) user does not specify qpid.max_size queue declare option (such that broker will set 104857600 there)
b) user sets qpid.max_size=104857600

Is it ok if a) is not matched while b) does? Would not it be confusing?

Note the only default constant values (that would have to be specified on 2nd place in source code) are only related to legacy store. I will think about the way passing the legacy store constants there..


> 
> Applied to issue 2) you could have ACL
> 
>   acl allow all create queue queuemaxcountlowerlimit=default
>   acl allow all create queue queuemaxcountlowerlimit=10

Then explicitly set zero value (that is interpreted by broker as infinity) would be interpreted by ACL as "default", what is wrong. Esp. for qpid.max_size with default value 100MB.

Moreover even approach with "default" ACL value does not solve the problem how to distinguish cases when user did not specify a property (and default 0 is set in QueueSettings) and when user set it to zero explicitly. In the first case, real default value should be used, in the second, zero/infinity value should be used. But that is QueueSettings / QueueDepth class limitation.

Comment 6 Pavel Moravec 2014-02-20 19:21:03 UTC
FYI I have a solution for the "zero means infinite / no limit" issue. It is already resolved for max_size / max_count (see struct Optional in src/qpid/broker/QueueDepth.h).

Comment 7 Pavel Moravec 2014-02-24 14:20:19 UTC
(In reply to Pavel Moravec from comment #6)
> FYI I have a solution for the "zero means infinite / no limit" issue. It is
> already resolved for max_size / max_count (see struct Optional in
> src/qpid/broker/QueueDepth.h).

https://reviews.apache.org/r/18423/

Comment 8 Pavel Moravec 2014-02-26 14:14:28 UTC
Committed to upstream as r1572059.

Comment 12 Petra Svobodová 2015-03-18 13:20:06 UTC
Retested on qpid-cpp-

Comment 13 Petra Svobodová 2015-03-18 13:27:00 UTC
The ACL rules are tested against the directly specified parameters by the user or the default broker values.

Verified on qpid-cpp-0.30-7 on Rhel6-i686 and x86_64 and Rhel7-x86_64.

--> VERIFIED

Comment 15 errata-xmlrpc 2015-04-14 13:47:30 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-0805.html


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