On Wed, 2014-10-01 at 22:50 -0400, Stephen Gallagher wrote: > > > On Thu, 2014-09-11 at 23:51 -0400, Yassir Elley wrote: > > > > ----- Original Message ----- > > > > > > > > > ----- Original Message ----- > > > > Hi, > > > > > > > > The attached patch fixes ticket #2437 (conflicting gpo policy settings > > > > not > > > > being resolved correctly) > > > > > > > > https://fedorahosted.org/sssd/ticket/2437 > > > > > > > > Regards, > > > > Yassir. > > > > > > > > _______________________________________________ > > > > sssd-devel mailing list > > > > [email protected] > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > > > > > > I discovered some more issues while doing further testing on this ticket > > > relating to "defined but empty" policy settings (see additional comments > > > on > > > ticket #2347). I have also made some minor changes to this patch, b/c they > > > didn't warrant their own ticket. Namely, I have removed some attributes > > > that > > > we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we > > > handle machine_ext_names by allowing for the possibility of a "present but > > > empty" value for AD_AT_MACHINE_EXT_NAMES. > > > > > > I am attaching a revised path to address these issues. > > > > > > Regards, > > > Yassir. > > > _______________________________________________ > > > sssd-devel mailing list > > > [email protected] > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > > > > > I have implemented the new design (described in ticket #2347) in which > > policy settings are written (and potentially over-written) to a "GPO > > Enforcement" object (which I ended up calling "GPO Result" object) in the > > sysdb cache. This allows policy settings to be resolved correctly for both > > the online and offline cases. It is a reasonably large patch, but I think > > the final implementation is cleaner and simpler. Revised patch is attached. > > > > Sorry it took me so long to finish this review. The code is mostly > right, but I found three issues that needed to be addressed before we > could commit it. > > 1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which > was resulting in the PAM conversation ultimately returning > PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed > in the 0001 patch attached. Can be applied separately. > > 2) There was an 'attrs' variable being passed in the > sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator, > causing a crash when accessing LDB. > > 3) The code could not properly handle when a GPO had an explicitly empty > value (such as one might use to expressly disable a setting from a > lower-priority GPO). > > Issues 2) and 3) are both addressed by the 0002 patch. For an easy view > of the changes I made, see > https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked > my review there as I went through, so I wouldn't miss any corrections > and as a sort of proof-of-concept) > > > I (believe) I have done a very thorough review of this code, so I'd ask > that someone double-check my updates and then we should be good to go > with committing this. It's a pretty important set of fixes. One additional note. This incorrect resolution order does *NOT* cause a security issue. I've carefully tested that the way processing was happening would only cause unintentional denials. (Specifically, we were too zealous about returning a denial as soon as we saw one, even though AD's GPOs permit the deny list to be overruled by a higher-priority GPO.) However, because of the way we were doing the lookups, the reverse was not true. A grant of permission overridden by an exclusion of that user/group from the permitted list would still not grant permission, because we were re-validating access permission for each GPO, so it had to be positively accepted at all levels to succeed.
signature.asc
Description: This is a digitally signed message part
_______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
