[SSSD] [PATCH][PRELIMINARY] sudo: send username and uid while requesting default options

Pavel Březina pbrezina at redhat.com
Fri May 11 10:23:34 UTC 2012


On 05/07/2012 07:17 PM, Pavel Březina wrote:
> On 05/03/2012 10:30 PM, Pavel Březina wrote:
>> On 3.5.2012 13:48, Jakub Hrozek wrote:
>>> On Fri, Apr 27, 2012 at 01:21:30PM +0200, Jakub Hrozek wrote:
>>>> On Wed, Apr 25, 2012 at 05:05:36PM +0200, Pavel Březina wrote:
>>>>> The subdomains patches messed with one line which was causing
>>>>> troubles.
>>>>> Here are the rebased patches.
>>>>>
>>>>> On 04/23/2012 12:51 PM, Pavel Březina wrote:
>>>>>> On 04/23/2012 10:03 AM, Jakub Hrozek wrote:
>>>>>>> On Fri, Apr 13, 2012 at 12:46:01PM +0200, Pavel Březina wrote:
>>>>>>>> On 03/26/2012 09:47 PM, Jakub Hrozek wrote:
>>>>>>>>> On Mon, Mar 19, 2012 at 02:46:02PM +0100, Pavel Březina wrote:
>>>>>>>>>> https://fedorahosted.org/sssd/ticket/1239
>>>>>>>>>>
>>>>>>>>>> [PATCH 1/2]
>>>>>>>>>> Finally removes EOK constant from sudo api header. It is not
>>>>>>>>>> used in
>>>>>>>>>> the SUDO code so it does not require their changes.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Looks good to me, it'w weird to use EOK in an external header
>>>>>>>>> (even
>>>>>>>>> though it is correctly defined).
>>>>>>>>>
>>>>>>>>> (Note: this patch shouldn't be pushed even though I don't have any
>>>>>>>>> comments. It is an ABI break and we need to coordinate with
>>>>>>>>> Daniel K.)
>>>>>>>>>
>>>>>>>>>> [PATCH 2/2]
>>>>>>>>>> This does what is requested in the ticket. It seems to be very
>>>>>>>>>> huge
>>>>>>>>>> but
>>>>>>>>>> in fact it is mainly changing the variable. Basically I tried
>>>>>>>>>> to get
>>>>>>>>>> rid of domain ctx where possible, leave it only in initgroups
>>>>>>>>>> part and
>>>>>>>>>> use command ctx elsewhere.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Still, it is hard to review the huge patch. Can you split it into
>>>>>>>>> smaller ones? What about creating one that removes the duplication
>>>>>>>>> between _get_sudorules and _get_defaults, one that converts to
>>>>>>>>> using
>>>>>>>>> cmdctx and one that adds the uid support?
>>>>>>>>
>>>>>>>> The patches are attached.
>>>>>>>>
>>>>>>>> I want to get those patches acked because I need to deliver the
>>>>>>>> updated
>>>>>>>> interface to Dan so he can update the sudo binary.
>>>>>>>>
>>>>>>>> It does not implement the in-memory cache (yet? :-)) due to the
>>>>>>>> discussion with simo.
>>>>>>>>
>>>>>>>> The patches expects "sudo api: check sss_status instead of
>>>>>>>> errnop in
>>>>>>>> sss_sudo_send_recv_generic()" (already acked on the list and
>>>>>>>> waiting
>>>>>>>> to be pushed).
>>>>>>>
>>>>>>> Patch 0001: sudo api: remove EOK
>>>>>>> Ack
>>>>>>>
>>>>>>> Patch 0002: sudo responder: remove code duplication in commands
>>>>>>> I think it would be a good idea to also check if
>>>>>>> rawname[rawname_len] ==
>>>>>>> '\0' when parsing username, this would ensure that all the string
>>>>>>> checks
>>>>>>> work OK.
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>>
>>>>>>> Now that several sudosrv_response_append_* functions were removed
>>>>>>> from
>>>>>>> header, can you make them static? In particular
>>>>>>> sudosrv_response_append_string(), sudosrv_response_append_uint32(),
>>>>>>> sudosrv_response_append_rule(), sudosrv_response_append_attr().
>>>>>>
>>>>>> Done.
>>>>>>
>>>>>>> Patch 0003: sudo sysdb: make sysdb_get_sudo_user_info more
>>>>>>> configurable
>>>>>>> Ack
>>>>>>>
>>>>>>> Patch 0004: sudo api: send uid, username and domainname
>>>>>>> Please get rid of the C++ comments in sudosrv_cmd(). If you need to
>>>>>>> leave code in but disabled, please use #if 0/#endif
>>>>>>
>>>>>> Thanks. I'm using keybord shortcut to comment out whole block with
>>>>>> these slashes and I fortgot that we have this rule :)
>>>>>>
>>>>>> The whole hunk that
>>>>>>> changes sudosrv_cmd() seems like it belongs to patch #2.
>>>>>>
>>>>>> Not really. It requires the new protocol which is implemented in this
>>>>>> patch. It makes the two command more similar than they was before.
>>>>>>
>>>>>>> There's another // comment in sudosrv_get_sudorules_from_cache.
>>>>>>>
>>>>>>> The logic looks good to me, though.
>>>>>>
>>>>>> Thank you for the review.
>>>>>>
>>>>>> I'm attaching a 6th patch that removes all code related to the
>>>>>> in-memory cache as it will not be used anymore.
>>>>>>
>>>>
>>>> Patch 0001: Ack
>>>> Patch 0002: Ack
>>>> Patch 0003: Ack
>>>> Patch 0004: Ack
>>>> Patch 0005: Ack
>>>> Patch 0006: Nack, does to apply on master:
>>>>
>>>> $ git reset --hard origin/master
>>>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>>>> $ git am *.patch
>>>> Applying: sudo api: remove EOK
>>>> Applying: sudo responder: remove code duplication in commands
>>>> Applying: sudo responder: get rid of dctx where possible
>>>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>>>> Applying: sudo api: send uid, username and domainname
>>>> Applying: sudo responder: discard in-memory cache
>>>> error: patch failed: src/responder/sudo/sudosrv_cache.c:1
>>
>> I'm sorry, but it works for me. Are you sure you have the correct
>> patches? What exactly fails to merge?
>>
>> $ git reset --hard origin/master
>> HEAD is now at 24ba5b8 NSS: Only return data from initgroups once
>> $ git am *.patch
>> Applying: sudo api: remove EOK
>> Applying: sudo responder: remove code duplication in commands
>> Applying: sudo responder: get rid of dctx where possible
>> Applying: sudo sysdb: make sysdb_get_sudo_user_info more configurable
>> Applying: sudo api: send uid, username and domainname
>> Applying: sudo responder: discard in-memory cache
>> $
>
> I'm sending it again formatted via git format-patch -M -C --patience
> --full-index and tarred.

There was one big mistake in the fifth patch.

I was passing wrong uid information into query_cache function.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tar
Type: application/x-tar
Size: 112640 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120511/93357a4b/attachment.tar>


More information about the sssd-devel mailing list