[Date Prev][Date Next] [Chronological] [Thread] [Top]

Re: ACL processing: additive privs (using control continue)



Howard Chu wrote:
Kurt Zeilenga wrote:
On Aug 4, 2012, at 9:08 AM, Howard Chu<hyc@symas.com>  wrote:
I haven't looked at the code yet but it's possible this is a bug.
Not a bug.  As documented, every access statement ends implicitly with a "by * none" clause.
Ah right. The "continue" control is only useful if a following "by" clause
matches the subject *and* specifies incremental privileges.

Any following "by" clause specifying a group pattern obviously can also specify incremental privileges *and* can (unforeseeable) match the subject.

Group entries and their members tend to be independent. Neither the existence of a distinct group's entry nor its members are known in advance (for example during slapd acl configuration time). Furthermore existing group entries as well as memberships disappear without prior notice. - In general groups are managed by people that aren't and usually don't need to be aware of slapd's internal acl engine dependencies. - Modifying a group (e.g. removing members or the entry) that is referenced within an acl probably results in the need to modify the belonging "by" clauses. - Statically pre-initializing all group entries referenced in the acls is misleading especially regarding the idea behind variable groups and memberships.
 - ...

Thus, resetting previously given privileges just in case a group pattern temporarily does not match seems to overshoot or even miss the target, especially in case the group entry or the member exists/disappears/exists/... during runtime.

My suggestion regarding a basis to start a hopefully fruitful discussion is:
Within "by" clauses using group patterns and specifying incremental privileges the final implicit "by * none" should be ignored (or at least be made ignoreable) for the current access-statement, while the controls stay untouched (attached you'll find a prototype of a patch). If the idea is acceptable, stable's behavior can be changed in general or smoothly for example by introducing some kind of groupDn evaluation specifier to make the evaluation of group pattern specific incremental privileges customizable (ignoreable), e.g. "group[/<objectclass>[/<attrname>]][.<groupstyle>][,<evalspecifier>]=<group>".

<evalspecifier> ::= "ignoreNoSuchObject" | "ignoreCompareFalse" | "ignoreBoth" ...

The location of such an <evalspecifier>, e.g. at the groupDn specifier, within the by clause, within the access clause or even globally is intentionally left open here.

Another (at least to me much better) solution seems to be to get rid of the implicit "by * none" code: It represents neither a reasonable security functionality, nor a valueable convenience feature - the opposite seems true: Implicit (hidden) "security" hides mission critical settings and functionallity, causes headache that leads to misinterpretation that can result in wrong asumptions (not only for beginners, even for experts, see above). The (need for) discussion represents just one sign for the inconvenience caused by such a kind of "convenience feature". With cn=config a missing "by * none" (as any other) acl statement can be added during runtime without the past time (slapd.conf) need to restart slapd. Although the implicit "by * none" seems outdated, specifying adequate frontend acls "implicit (but documented in the configuration) by * none" has been already achievable for some time in the past until today.

In case of an "unwilling to perform situation", lets just try to get rid of the two useless ("silly" and the "even more silly") continue control examples described in slapd.access(5), either by - Describing a appropriate "clever" continue example or if this should be generally impossible, by
- Removing the obviously silly continue control code, or eventually by
- Starting a (openldap-devel?) discussion whether and if so, how to integrate the above described (or similar) generally useful feature.

Thank you very much.
diff --git a/servers/slapd/acl.c b/servers/slapd/acl.c
index 92b4c43..35c3fb7 100644
--- a/servers/slapd/acl.c
+++ b/servers/slapd/acl.c
@@ -1121,7 +1121,7 @@ slap_acl_mask(
 	AccessControlState	*state,
 	slap_access_t	access )
 {
-	int		i;
+	int		i, implNone;
 	Access		*b;
 #ifdef LDAP_DEBUG
 	char		accessmaskbuf[ACCESSMASK_MAXLEN];
@@ -1152,6 +1152,7 @@ slap_acl_mask(
 
 	b = a->acl_access;
 	i = 1;
+	implNone = 1;
 
 	for ( ; b != NULL; b = b->a_next, i++ ) {
 		slap_mask_t oldmask, modmask;
@@ -1648,6 +1649,21 @@ slap_acl_mask(
 			}
 
 			if ( rc != 0 ) {
+				Debug( LDAP_DEBUG_ACL, "<= check a_group_pat: membership evaluation failed: \"%s\" (rc: %d)\n",
+					ldap_err2string(rc), rc, 0 );
+
+				if( ACL_IS_ADDITIVE(b->a_access_mask) || ACL_IS_SUBTRACTIVE(b->a_access_mask) ) {
+					Debug( LDAP_DEBUG_ACL,
+						"<= acl_mask: [%d] ignoring incremental privileges %s (%s)\n",
+						i, accessmask2str( b->a_access_mask, accessmaskbuf, 1 ), 
+						b->a_type == ACL_CONTINUE
+							? "continue"
+							: b->a_type == ACL_BREAK
+								? "break"
+								: "stop" );
+					implNone = 0;
+					goto byClauseDone;
+				}
 				continue;
 			}
 		}
@@ -1872,6 +1888,7 @@ slap_acl_mask(
 			"<= acl_mask: [%d] mask: %s\n",
 			i, accessmask2str(*mask, accessmaskbuf, 1), 0 );
 
+byClauseDone:
 		if( b->a_type == ACL_CONTINUE ) {
 			continue;
 
@@ -1884,7 +1901,8 @@ slap_acl_mask(
 	}
 
 	/* implicit "by * none" clause */
-	ACL_INIT(*mask);
+	if ( implNone ) 
+		ACL_INIT(*mask);
 
 	Debug( LDAP_DEBUG_ACL,
 		"<= acl_mask: no more <who> clauses, returning %s (stop)\n",