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