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

Jakub Hrozek jhrozek at redhat.com
Thu May 2 17:32:36 UTC 2013


On Thu, May 02, 2013 at 01:57:29PM +0200, Sumit Bose wrote:
> On Wed, May 01, 2013 at 09:08:47PM +0200, Jakub Hrozek wrote:
> > 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
> 
> thank you for the review, comments a re in-line.
> 
> > 
> > [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.
> 
> yes, but if I read the old code correctly, there is always a domain
> return. And currently the callers do not check for new domains if NULL
> is returned. I think it would be code to clean this up, but I think it
> can wait after the release.
> 

Agreed:
https://fedorahosted.org/sssd/ticket/1902

> > 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.
> 
> yes, I was also thinking if a struct with type and a union would help
> here to make the code more readable, but I thought here as well that it
> can wait after the release.
> 

Sure, it's not needed to do it now:
https://fedorahosted.org/sssd/ticket/1903

> > 
> > [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 ?
> 
> I changed BE_REQ_SECID->BE_REQ_BY_SECID to make the purpose more clear.
> I would like to keep BE_REQ_USER_AND_GROUP because although  it will be
> used when a SID should be lookup up, the general purpose is the check
> the users and the groups for the given name or id.
> 

OK, works for by

> > 
> > [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.
> 
> See above.
> 
> > 
> > [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?
> 
> yes, it handled in the patch which adds support for REQ_INP_SECID to
> the IPA subdomains.
> 

OK, then it's fine.

> New versions attached, but only patch 6 is changed.
> 
> bye,
> Sumit

I don't have any more comments. Ack.



More information about the sssd-devel mailing list