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
>> 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
>> the system. So if a user is a member of a group that is not
>> we don't have to worry that it might match a DENY rule.
> 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
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.
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.
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
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.
Please free tmp_ctx in the "immediate:" label in
The "/* Not found as a user; Check Groups*/" seems to be misleading, the
code never checks users.