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

Pavel Březina pbrezina at redhat.com
Thu Jun 21 15:29:56 UTC 2012


On 17.5.2012 13:03, Pavel Březina wrote:
> 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.

Rebased on the current master.

The sudo client is finally ready:
http://koji.fedoraproject.org/koji/rpminfo?rpmID=3115173

I will test it tomorrow.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 19088 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120621/1f8a4d9d/attachment.bin>


More information about the sssd-devel mailing list