On 06/24/2011 02:24 PM, Stephen Gallagher wrote: > On Fri, 2011-06-24 at 11:15 -0400, Simo Sorce wrote: >> On Fri, 2011-06-24 at 09:54 -0400, Stephen Gallagher wrote: >>> New patches attached with some significant changes. After much >>> discussion, we decided that, rather than implement a facility for >>> identifying "unresolved groups" for the user, we would instead perform >>> a >>> group lookup during the initial pull-down of the HBAC rules. This way, >>> we know that all groups that are relevant to HBAC rules are available >>> on >>> the system. So if a user is a member of a group that is not >>> resolvable, >>> we don't have to worry that it might match a DENY rule. >> >> NACK >> >> New patches fail to take in account non-posix groups. > > I've added three new patches to account for the non-POSIX groups. I've > also removed the old patch 0001, as with these new changes it is no > longer necessary. > > Patch 0009 and 0010 make no functional changes, they just reorganize the > code a little bit to make 0012 easier. > > Patch 0012: This makes two changes. > 1) Change the construction of the eval request to use > ipa_get_groupname() and the originalMemberOf attribute to create the > list of groups, rather than calling initgroups. > > 2) In hbac_user_attrs_to_rule(), if the member is not detected as a > POSIX user or group, check to see if it matches with > ipa_get_groupname(), indicating that it is a non-POSIX group. There is > no need to detect non-POSIX users here, as they wouldn't be eligible to > log in anyway. >
0002: ACK 0003: NACK The defattr in "%files -n libipa_hbac-devel" is missing a comma before the last parameter, it says "%defattr(-,root,root-)". hbac_free_error(): I wonder if it makes sense to add an explicit "if (!error) return". I think it is expected that most free functions accept NULL as a noop and it is quite useful for goto-cleanup cases. 0004: ACK 0005: ACK 0006: ACK 0007: ACK, altough the description of ipa_hbac_refresh was added to config/SSSDConfig.py in patch 0008. Feel free to fix or not, I don't think this should be a nack as the patchset will be applied as a whole anyway. For the rest of the patches - the code seems to be OK sans the one little issue below. I will defer the "architectural" review to Simo as the original comments were based on his PAC development experience. 0008: NACK Please free tmp_ctx in the "immediate:" label in ipa_hbac_update_groups_send() The "/* Not found as a user; Check Groups*/" seems to be misleading, the code never checks users. 0009: ACK 0010: ACK 0011: ACK 0012: ACK _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
