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?
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?)
Sorry, I don't get this, can you explain?
This patch contains a modified version of sysdb_get_sudo_user_info()
where the uid is not mandatory. I want to replace this function with
sysdb_sudo_get_user_groups() (or make it generic and place it in
sysdb_ops?) because the groupnames are the only thing we don't know.
However this requires a modification of the data provider protocol
as well so I'm keeping it for later.