On Thu, Jan 26, 2012 at 05:12:01PM +0100, Pavel Březina wrote:
> Dne 25.1.2012 18:55, Jakub Hrozek napsal(a):
> >On Tue, Jan 24, 2012 at 10:42:14AM +0100, Pavel Březina wrote:
> >>Dne 19.1.2012 12:31, Jakub Hrozek napsal(a):
> >>>On Tue, Jan 17, 2012 at 05:55:49PM +0100, Pavel Březina wrote:
> >>>>https://fedorahosted.org/sssd/ticket/1143
> >>>>
> >>>>Add a new responder command that will return to the caller just the
> >>>>rule named cn=defaults.
> >>>
> >>>The patches look fine conceptually but I would like to have them rebased
on
> >>>top of the upcoming patches for
https://fedorahosted.org/sssd/ticket/1115
> >>>not the other way around, it would be much easier. I'd like to send
patches
> >>>for #1115 quite soon, probably today.
> >>
> >>Rebased patches attached.
> >
> >0001: Ack
> >
> >0002: Nack, please use "uid_t" for uid in "struct
be_sudo_req"
>
> Done.
>
> >I'm also not very fond of using BE_REQ_* inside the sdap_* request and
> >especially in the periodic update.
>
> I though of this as well but I chose it as a lesser evil. We have
> already BE_REQ_* and SSS_DP_*, I don't want to create another name
> for sdap and eventually for IPA that would have literally the same
> meaning.
>
> I think we should rather try to
> >construct the filters on one place, maybe even before the _send
> >function. The "ALL" case could then be handled by
"username=*" similarly
> >to what we do in the LDAP provider.
>
> This is a good idea. Unfortunately as you have found that the way we
> are clearing the cache is entirely wrong*, this would be possible
> only for the LDAP filter. I suggest to leave it as it is for the
> moment and deal with it after the cache purging is reworked. Than we
> will know what options do we have.
>
Yeah, I can't believe this design bug made it through my review during
development and then the peer-review on the list..
> *
> - DP downloads rules for the user, searching for the keyword ALL,
> username, #uid, %group, +* (all netgroups)
> - before it stores these rules into the sysdb, it deletes the
> entries using the same filter
>
> This can cause problems in following situation:
> - we have users A and B
> - rule R will match for both users by their name or uid (there is no
> problem for the groups or netgroups)
> - the rule is stored in the sysdb
> - A is removed from R
> - A runs sudo which causes deletion of R
> - DP goes offline
> - B is unable to run sudo beacuse R is deleted
>
> Jakub suggested than instead of deleting entire rule we will just
> delete the username/uid/group from the sudoUser attribute and delete
> the rule only in case the sudoUser is empty.
>
Because the update step would also download all the rules that contain
any netgroups, we can purge rules that contain "sudoUser=+*" as well.
> >0003: Ack
> >
> >0004: Nack, you are leaking reply_buf in sss_sudo_send_recv_generic()
>
> Fixed.
>
> >0005: Ack
>
> Thank you for the review.
>
This round of patches looks good to me.
Ack to all five of them.