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

Jakub Hrozek jhrozek at redhat.com
Tue Jul 24 09:11:54 UTC 2012


On Tue, Jun 26, 2012 at 10:02:32AM +0200, Pavel Březina wrote:
> 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.
> 

For the record, these patches were included in another thread, which
superseded this one.



More information about the sssd-devel mailing list