URL: https://github.com/SSSD/sssd/pull/31 Author: sumit-bose Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME Action: opened
PR body: """ When adding support for UPNs, email addresses and aliases the SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME request were forgotten. This patch adds the missing support because it might be irritating if getpwnam() can resolve the name but the other requests fail. The same logic as for the plain user lookup is used, this add some code duplication which is expected to be removed when the nss responder will be switched to use the new cache_req code.
Resolves https://fedorahosted.org/sssd/ticket/3194 """
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/31/head:pr31 git checkout pr31
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
jhrozek commented: """ I'm sorry the review took so long.
The patch mostly looks good, but see the two questions though the github review tool. I also wrote some tests, so far just in my personal branch https://github.com/SSSD/sssd/compare/master...jhrozek:review
I will submit them atop this PR once we know if the overriden names are also something that should be tested. And if there is some new flavor added to negcache, I need to add tests for that as well. """
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-249519530
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
sumit-bose commented: """ 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?
It is not new, it is used in nss_cmd_getpwnam_search() as well.
commit 62df78512145db94b51c5573d4df1737197e368a Author: Sumit Bose sbose@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 jhrozek@redhat.com
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).
@@ -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.
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/SSSD/sssd/pull/31#pullrequestreview-1498162
"""
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-249525279
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 sbose@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 <jhrozek@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
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/31 Author: sumit-bose Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/31/head:pr31 git checkout pr31
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
jhrozek commented: """ ACK. I will push the patch once Coverity finishes. """
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-252280250
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: +Changes requested
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
jhrozek commented: """ I'm afraid Coverity found something it doesn't like: ``` Error: COMPILER_WARNING: sssd-1.14.2/src/responder/nss/nsssrv_cmd.c: scope_hint: In function 'nss_cmd_getsidby_search' sssd-1.14.2/src/responder/nss/nsssrv_cmd.c:4590:21: warning: 'neg_cache_name' may be used uninitialized in this function [-Wmaybe-uninitialized] # ret = sss_ncache_set_group(nctx->rctx->ncache, false, # ^ # 4588| } # 4589| # 4590|-> ret = sss_ncache_set_group(nctx->rctx->ncache, false, # 4591| dom, neg_cache_name); # 4592| if (ret != EOK) { ``` """
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-252281331
URL: https://github.com/SSSD/sssd/pull/31 Author: sumit-bose Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME Action: synchronized
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/31/head:pr31 git checkout pr31
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: -Changes requested
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
sumit-bose commented: """ On Fri, Oct 07, 2016 at 08:21:21AM -0700, Jakub Hrozek wrote:
I'm afraid Coverity found something it doesn't like:
The latest patch should fix this. In general I think it is a false positive because it is hard for Coverity to follow the different if-blocks. I tried to fix it with some minor code changes but failed, so I just initialized neg_cache_name in the latest version.
bye., Sumit
Error: COMPILER_WARNING: sssd-1.14.2/src/responder/nss/nsssrv_cmd.c: scope_hint: In function 'nss_cmd_getsidby_search' sssd-1.14.2/src/responder/nss/nsssrv_cmd.c:4590:21: warning: 'neg_cache_name' may be used uninitialized in this function [-Wmaybe-uninitialized] # ret = sss_ncache_set_group(nctx->rctx->ncache, false, # ^ # 4588| } # 4589| # 4590|-> ret = sss_ncache_set_group(nctx->rctx->ncache, false, # 4591| dom, neg_cache_name); # 4592| if (ret != EOK) {
-- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: https://github.com/SSSD/sssd/pull/31#issuecomment-252281331
"""
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-252556554
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, Oct 10, 2016 at 01:07:12AM -0700, sumit-bose wrote:
On Fri, Oct 07, 2016 at 08:21:21AM -0700, Jakub Hrozek wrote:
I'm afraid Coverity found something it doesn't like:
The latest patch should fix this. In general I think it is a false positive because it is hard for Coverity to follow the different if-blocks. I tried to fix it with some minor code changes but failed, so I just initialized neg_cache_name in the latest version.
I agree, but I also prefer slight code-bending if it keep our code-scans clean.
Thank you, my tests are still passing and Coverity no longer complains.
"""
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-252577955
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: +Accepted
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: +Pushed
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
Label: -Accepted
URL: https://github.com/SSSD/sssd/pull/31 Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME
jhrozek commented: """ * master: dcdf292567d50e5cc527766c1944dcf6a8ecacc5 """
See the full comment at https://github.com/SSSD/sssd/pull/31#issuecomment-252583002
URL: https://github.com/SSSD/sssd/pull/31 Author: sumit-bose Title: #31: nss: allow UPNs in SSS_NSS_GETSIDBYNAME and SSS_NSS_GETORIGBYNAME Action: closed
To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/31/head:pr31 git checkout pr31
sssd-devel@lists.fedorahosted.org
-
fidencio