[SSSD] [PATCHES] Add SID related lookups to the NSS responder

Jakub Hrozek jhrozek at redhat.com
Wed May 1 19:08:47 UTC 2013


On Mon, Apr 29, 2013 at 09:06:39PM +0200, Sumit Bose wrote:
> Hi,
> 
> this is the first part of patches to add SID lookups, i.e. lookups by
> SID and lookups for SIDs which is tracked by ticket
> https://fedorahosted.org/sssd/ticket/1559 and described in
> https://fedorahosted.org/sssd/wiki/DesignDocs/NSSResponderIDMappingCalls.
> 
> I split the patches into different parts to make review eaiser
> (hopefully :-). This first part contains patches with minor code changes
> and some new help functions. Details can be found in the commit
> messages.
> 
> bye,
> Sumit

[PATCH 01/12] Remove unused TALLOC_CTX from responder_get_domain()
Ack

[PATCH 02/12] responder_get_domain: do not return disabled domains
Ack, we already do the same in find_subdomain_by_name, so it makes sense
to skip the disabled domains in responder_get_domain, too.

[PATCH 03/12] responder_get_domain(): remove timeout calculation
Wasn't the purpose of the code to assign "ret_dom = dom"; only after
establishing that the domain is not expired? That way the expired
domains would be skipped.
But I agree the current responder code can't really hit this situation
because sss_dp_get_domains_send() would check using check_last_request()
and the subdomains would be refreshed before reaching responder_get_domain()

[PATCH 04/12] LDAP: always store SID if available
Ack

[PATCH 05/12] Add secid filter to responder-dp protocol
Ack but the nested if-else structure in sss_dp_get_account_msg() is
starting to look unreadable :-) Maybe we could split it into smaller
functions in a follow up patch.

[PATCH 06/12] Add two new request types to the data-provider interface
The functionality is good, but the names are not clear to me. What about
BE_REQ_BY_SECID and BE_REQ_GET_SECID ?

[PATCH 07/12] Add idmap context to nss context
Ack

[PATCH 08/12] Add responder_get_domain_by_id()
Why does responder_get_domain_by_id() calculate the timeout but
responder_get_domain() does not?
The code itself looks good to me.

[PATCH 09/12] sysdb: add sysdb_search_object_by_sid()
Ack

[PATCH 10/12] Add sss_ncache_set_sid() and sss_ncache_check_sid()
Ack

[PATCH 11/12] Remove unused attribute list
Ack

[PATCH 12/12] Use struct to hold different types of request
I like the idea. One question though:

>      switch (entry_type) {
>          case BE_REQ_USER:
> -            if (name != NULL) {
> +            if (req_input->type == REQ_INP_NAME) {
>                  ret = ber_printf(ber, "{ee{ss}}", INP_NAME, REQ_FULL,
> -                                                  domain_name, name);
> +                                                  domain_name,
> +                                                  req_input->inp.name);
>              } else {
>                  ret = ber_printf(ber, "{ee{si}}", INP_POSIX_UID, REQ_FULL,
> -                                                  domain_name, id);
> +                                                  domain_name,
> +                                                  req_input->inp.id);

Here REQ_INP_SECID would match, too, but I don't think secid would be
set. I think it would be safer to:

    if (req_input->type == REQ_INP_NAME) {
        /* Use req_input->inp.name */
    } else if (req_input->type == REQ_INP_NAME) {
        /* Use req_input->inp.id */
    } else {
        /* Error */
    }

But maybe it would be handled in a follow up patch?



More information about the sssd-devel mailing list