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-identity.html
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.
>
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
