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

Pavel Březina pbrezina at redhat.com
Thu May 17 11:03:56 UTC 2012


On 14.5.2012 18:37, Jakub Hrozek wrote:
> On Mon, May 14, 2012 at 05:55:09PM +0200, Pavel Březina wrote:
>> On 11.5.2012 12:23, Pavel Březina wrote:
>>> 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.
>>>
>
> Thank you, I'm not sure which one of the changes did it, but I'm able to
> apply the patches now.
>
>>> There was one big mistake in the fifth patch.
>>>
>>> I was passing wrong uid information into query_cache function.
>>
>> I have added a patch that increases protocol number. If client uses
>> old protocol, responder will return error immediately.
>>
>
> Sorry, one last change..can you change --version to --version-info and
> bump the "current" number in the version string to 1?

Patches are attached.

>> I know the sudo part is not yet ready. But I would like to get these
>> patches pushed - I don't want to rebase them again :) It does not
>> break sudo until sss is present in nsswitch.conf.
>
> FWIW, I don't like breaking running systems. Yes, there is probably very
> few people who use sudo and sssd and would be affected by this, but
> still..
>
> I don't have any issues with the code, I'll ack it when the version is
> bumped.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tar.gz
Type: application/x-gzip
Size: 19274 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120517/02923549/attachment.gz>


More information about the sssd-devel mailing list