On Tue, Jun 28, 2011 at 08:14:57AM -0400, Stephen Gallagher wrote:
On Mon, 2011-06-27 at 18:31 -0400, Jakub Hrozek wrote:
> 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-)".
>
Whoops, I could have sworn I fixed that...
> 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.
>
Yeah, that was an oversight. I converted it from talloc_free() (while
eliminating the talloc requirement) and didn't think about the NULL
case.
> 0004: ACK
NACK
+ /* Get the source host */
+ if (pd->rhost == NULL || pd->rhost[0] == '\0') {
+ /* If we haven't been passed an rhost, we
+ * have to assume it's coming from the
+ * target host
+ */
this assumption is wrong, if rhost is missing we cannot assume anything
about the remote host. See
http://www.kernel.org/pub/linux/libs/pam/Linux-PAM-html/adg-security-user...
for details.
bye,
Sumit
>
> 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.
>
Oops. Looks like I squashed that into the wrong patch. Fixed.
>
> 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()
>
Thanks, I missed that.
> The "/* Not found as a user; Check Groups*/" seems to be misleading, the
> code never checks users.
>
That was a relic from before I decided to live with IPA-isms. Originally
I was doing a sysdb_search_user() to eliminate users from consideration.
I've removed this comment.
> 0009: ACK
>
> 0010: ACK
>
> 0011: ACK
>
> 0012: ACK
Updated patches attached.