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

Pavel Březina pbrezina at redhat.com
Tue Mar 27 08:05:04 UTC 2012


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?

I'll try.

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

1. It makes sudosrv_cmd_send_error() and sudosrv_cmd_done() unified and 
error handling is a little bit easier (if dctx is NULL).

2. Some information were taken from cmd_ctx some from dctx and I found 
it confusing when adding new fields to structure - where should I put 
it? It semantically belongs to both? So I decided to use only one of 
them and leave dctx only for initgroups part (which will be removed in 
#1126).

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

Sudo sends us username and uid in sudo_sssd_display_defaults() - yes
(that's what this patch does :-)).

I will try to rephrase the problem first:
Phase 1) sudo requests for default option giving us username and uid
- (search in-memory cache)*
- responder finds the domain user is a member of and performs initgroups
- provider will download the cn=defaults from LDAP and stores them in
   sysdb
- responder fetches the entry
- stores it into in-memory cache with key "$domain"
- and returns it to the sudo with the domain name

*Current master does it the wrong way (more info in the ticket
  description). In this patch it is the problematical part.

Phase 2) sudo requests rules for username at domain with uid
- search in-memory cache, if found then return else continue
- provider will download the rules from LDAP and stores them in sysdb
- responder fetches the entry
- stores it into in-memory cache with key "$domain:$username"
- and returns it to the sudo

All the entries are basically considered expired immediately, that's why 
we are going into LDAP everytime. But it is also slow so we have created 
the in-memory cache.

The problem here is that we have to know the domain before we can start 
searching in the in-memory cache. This is fine with Phase 2 (sudo sends 
us this information), but make problems in Phase 1.

We can do this in two ways in Phase 1 (Phase 2 stays intact):

1.
- find the domain user is a member of and perform initgroups
- search in-memory cache for default options
- if found then return else continue
- provider, ...

Advantage - a little bit lesser memory consumption
Disadvantage - more time spend in initgroups if it is in the last domain

2. store the pair (user,domainname) in the in-memory cache as well
- find domain name in in-memory cache
- if not found then perform initgroups, go into provider, ...

- find default options in in-memory cache
- if not found then go into provider, ...

Advantage - faster, easier to implement
Disadvantage - bigger memory dump if the sudo is used by many users

I hope that this is understandable enough.

>> 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.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list