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

Jakub Hrozek jhrozek at redhat.com
Mon May 14 16:37:53 UTC 2012


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?

> 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.



More information about the sssd-devel mailing list