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.