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