URL: https://github.com/SSSD/sssd/pull/31
Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
jhrozek commented:
"""
On Mon, Sep 26, 2016 at 02:43:03AM -0700, sumit-bose wrote:
> On Mon, Sep 26, 2016 at 02:13:08AM -0700, Jakub Hrozek wrote:
> > jhrozek commented on this pull request.
> >
> >
> >
> > > if (name == NULL) {
> > DEBUG(SSSDBG_OP_FAILURE, "sss_get_cased_name failed.\n");
> > ret = ENOMEM;
> > goto done;
> > }
> >
> > + if (cmdctx->name_is_upn) {
> > + neg_cache_name = talloc_asprintf(name, "@%s", name);
> >
> > Hmm this is new, why is there the @-sign?
(Summing up a discussion from IRC)
>
> It is not new, it is used in nss_cmd_getpwnam_search() as well.
>
> commit 62df78512145db94b51c5573d4df1737197e368a
> Author: Sumit Bose <sb...@redhat.com>
> Date: Fri Jul 22 16:01:38 2016 +0200
>
> NSS: use different neg cache name for UPN searches
>
> If Kerberos principals or email address have the same domain suffix as
> the domain itself the first user lookup by name might have already added
> the name to the negative cache and the second lookup by UPN/email will
> skip the domain because of the neg cache entry. To avoid this a special
> name with a '@' prefix is used here.
>
> Reviewed-by: Jakub Hrozek <jhro...@redhat.com>
Oops, I forgot it was even me who reviewed this change :)
>
>
> But I agree that it would be better to make this a bit more obvious by
> e.g. adding something like sss_ncache_name(const char *name, bool
> is_upn).
Yes, but I think it would be more systematic to do this change in the
cache_req code. I filed https://fedorahosted.org/sssd/ticket/3204 to
track this.
>
> >
> > > @@ -4573,8 +4623,19 @@ static errno_t nss_cmd_getsidby_search(struct
> > > nss_dom_ctx *dctx)
> > req_type = SSS_DP_USER_AND_GROUP;
> > }
> >
> > + if (cmdctx->name_is_upn) {
> > + extra_flag = EXTRA_NAME_IS_UPN;
> > + } else if (DOM_HAS_VIEWS(dom) && (dctx->res->count == 0
> > + || ldb_msg_find_attr_as_string(dctx->res->msgs[0],
> > + OVERRIDE_PREFIX
> > SYSDB_NAME,
> > + NULL) != NULL)) {
> >
> > I started writing tests for this new functionality and I wonder if this is
> > another a bit unrelated thing this patch fixes -- looking up SID by
> > overriden name?
>
> Currently it is more a copy-and-paste error because the name based sysdb
> searches in nss_cmd_getsidby_search() do not use the *_with_views()
> variant of the searches.
>
> There was some discussion when adding the override feature what name a
> by-SID search should return and we agreed that in this case always the
> original name from AD should be returned. I guess as a result the whole
> SID lookup related code path was skipped when adding overrides. But I
> think it would be ok to allow lookup by overriden names here as well.
OK, as discussed on IRC, it would be better to remove this change,
resubmit this patch so only related changes are included and file a
ticket to track this separate bug.
"""
See the full comment at
https://github.com/SSSD/sssd/pull/31#issuecomment-250107905
_______________________________________________
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
