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

Simo Sorce simo at redhat.com
Mon Mar 26 22:35:08 UTC 2012


On Mon, 2012-03-26 at 15:47 -0400, 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.)

I do not see any ABI change in the first patch, what are you referring
to ?

> > [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?
> 
> What is the rationale for removing the dctx? It would seem logical to me
> to continue using it, the user is found in a particular domain anyway..
> 
> sudosrv_cmd: we should free cmd_ctx if retreiving cmd_ctx->sudo_ctx
> fails (I know the problem was there before)
> 
> sudosrv_get_user: can you add a DEBUG() message when the UID number does
> not match?
> 
> You shouldn't add add _GNU_SOURCE to sss_sudo.c, it is part of config.h
> in SSSD (see AC_GNU_SOURCE)
> 
> While we are touching sss_sudo_create_query, can you use
> safealign_memcpy? It does virtually the same as your code, but would be
> more error prone when/if we extend the protocol again.
> 
> > The in-memory cache is not yet implemented, I want to discuss the
> > possible ways of doing it.
> > 
> > The basic problem is that we need to get the domain during the request
> > for default options. How will we do it? I think there are two options:
> > 
> > 1. always try to perform the initgroups - find the domain and the
> > check the in-memory cache (which may be slow if the user is in the
> > last domain, but that will be probably handled as part of
> > https://fedorahosted.org/sssd/ticket/1126
> > 
> 
> I'm not sure if this is what are you proposing in 1), but when I look
> at sudo_sssd_display_defaults() in the sudo tree, I see that it knows
> the user we are performing sudo on behalf on already. So can we extend
> it to send username/uid and perform initgroups during the "get default
> options operation?". I don't think there is much slowness here, as you
> said, ticket #1226 would bring some improvement and with the current
> model we need to search all the domains anyway. Also, only the first
> request would perform searches, the other would be caught by the
> in-memory cache, right?
> 
> > 2. store uid:username = domain in the in-memory cache (same cache as
> > results or a new one?)
> > 

May I ask what 'in-memory' cache are we talking about ?
I would like to understand why looking up into LDB is not considered a
cache look-up.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list