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

Pavel Březina pbrezina at redhat.com
Tue Jun 26 08:02:32 UTC 2012


On 06/21/2012 05:29 PM, Pavel Březina wrote:
> 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.

The sudo client has some problems with undefined symbols while
dlopen(sudoers.so). Daniel should look at it today.

I'm sending rebased patches one more time.


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


More information about the sssd-devel mailing list