[SSSD] [sssd PR#16] TOOLS: sss_groupshow did not work (comment)
mzidek-rh commented on a pull request """ The patches from PR are included in PR #18 . Labeling this one as rejected. """ See the full comment at https://github.com/SSSD/sssd/pull/16#issuecomment-245437017 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (+Pushed)
mzidek-rh's pull request #18: "[PATCHES] sss_user/groupmod fixes" label *Pushed* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/18 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#16] TOOLS: sss_groupshow did not work (+Rejected)
mzidek-rh's pull request #16: "TOOLS: sss_groupshow did not work" label *Rejected* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/16 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#16] TOOLS: sss_groupshow did not work (-Changes requested)
mzidek-rh's pull request #16: "TOOLS: sss_groupshow did not work" label *Changes requested* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/16 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (comment)
lslebodn commented on a pull request """ On (07/09/16 09:51), fidencio wrote: >A few minors in the commit messages that should be fixed before pushing. > >Patch0001: TOOLS: Fix a typo in groupadd() >Remove the four spaces before the link of the trac ticket. > >Patch0002: TOOLS: sss_groupshow did not work >Patch0003: TESTS: sss_groupadd/groupshow regressions >Patch0004: TOOLS: use internal fqdn for DN >Patch0006: TOOLS: sss_mc_refresh_nested_group short/fqname usage >Patch0007: TESTS: Add FQDN variants for some tests >Use (but do not exceed) 72 characters in the commit (body) message. I have the >feeling you're breaking the line with 52. > Changed together with adding review-by ACK http://sssd-ci.duckdns.org/logs/job/53/01/summary.html master: * f2d1d90a14267c01155eab7bb95b8eb34128acc9 * cb54dbad6be907d277ce6aa39524338643e2f5a4 * 7fa4964d84f41bd80a6d971ffaeef87a7c2f19be * 5e2142b66589e5e50cb404fc972ed5418bbaa772 * 20c2d76d9430a1fc069531ff537df046a74c8f61 * 5210c5d3a5a83b5d08396ee23d88f6ba0994097d * 6be723a089a1e07a1cd19b4fa53fd142c13f0c69 LS """ See the full comment at https://github.com/SSSD/sssd/pull/18#issuecomment-245377110 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH] failover: proceed normally when no new server is found
On Thu, Sep 01, 2016 at 02:02:40PM +0200, Pavel Březina wrote: > https://fedorahosted.org/sssd/ticket/3131 > > I couldn't reproduce manually so I used the second patch as a by-code > reproducer. If you apply the patch then sssd will try to resolve meta server > twice simultaneously and triggering the problematic code path. You can > search for RESOLV SRV DONE in logs. Superseded by: https://github.com/SSSD/sssd/pull/12 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: MONITOR: Add disable_netlink sssd.conf option
Hi, sorry to come late, but I have one more request (last one, I promise..) On Thu, Sep 01, 2016 at 09:36:32AM -0400, Justin Stephenson wrote: > From f647e732c2a5b8727376dded962766fb03bb5ea8 Mon Sep 17 00:00:00 2001 > From: Justin Stephenson > Date: Fri, 26 Aug 2016 15:15:32 -0400 > Subject: [PATCH 1/3] MONITOR: Remove --disable-netlink command-line option > > Removing monitor command-line option, to be superceded by > sssd.conf option [...] > +if (opt_netlinkoff) { > +fprintf(stderr, "Option --disable-netlink has been removed and " > +"replaced as a Monitor option in sssd.conf\n"); > +poptPrintUsage(pc, stderr, 0); > +return 1; I would prefer if we were a little more graceful here and only called DEBUG and sss_log() but did not abort the startup. I think it's better to start SSSD in a degraded mode than not at all.. > +} > + > if (!opt_daemon && !opt_interactive && !opt_genconf) { > opt_daemon = 1; > } ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#20] sss_override fails to export (opened)
mzidek-rh's pull request #20: "sss_override fails to export" was opened PR body: """ Here is a fix + CI test for https://fedorahosted.org/sssd/ticket/3179. Michal """ See the full pull-request at https://github.com/SSSD/sssd/pull/20 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/20/head:pr20 git checkout pr20 From 06d41f825c3a485659c4db1a273b0d881a017a78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Wed, 7 Sep 2016 17:09:53 +0200 Subject: [PATCH 1/2] TOOLS: sss_override without name override sss_override failed to export user/group overrides if user had no overrides for name. Resolves: https://fedorahosted.org/sssd/ticket/3179 --- src/tools/sss_override.c | 24 ++-- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/tools/sss_override.c b/src/tools/sss_override.c index d41da52..212bf9a 100644 --- a/src/tools/sss_override.c +++ b/src/tools/sss_override.c @@ -1159,12 +1159,14 @@ list_user_overrides(TALLOC_CTX *mem_ctx, } fqname = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL); -ret = sss_parse_internal_fqname(tmp_ctx, fqname, &name, NULL); -if (ret != EOK) { -ret = ERR_WRONG_NAME_FORMAT; -goto done; +if (fqname != NULL) { +ret = sss_parse_internal_fqname(tmp_ctx, fqname, &name, NULL); +if (ret != EOK) { +ret = ERR_WRONG_NAME_FORMAT; +goto done; +} +objs[i].name = talloc_steal(objs, name); } -objs[i].name = talloc_steal(objs, name); objs[i].uid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_UIDNUM, 0); objs[i].gid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0); @@ -1248,12 +1250,14 @@ list_group_overrides(TALLOC_CTX *mem_ctx, talloc_steal(objs, objs[i].orig_name); fqname = ldb_msg_find_attr_as_string(msgs[i], SYSDB_NAME, NULL); -ret = sss_parse_internal_fqname(tmp_ctx, fqname, &name, NULL); -if (ret != EOK) { -ret = ERR_WRONG_NAME_FORMAT; -goto done; +if (fqname != NULL) { +ret = sss_parse_internal_fqname(tmp_ctx, fqname, &name, NULL); +if (ret != EOK) { +ret = ERR_WRONG_NAME_FORMAT; +goto done; +} +objs[i].name = talloc_steal(objs, name); } -objs[i].name = talloc_steal(objs, name); objs[i].gid = ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0); } From e21e78231e21c7959e4ce132297fcb31c67681ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Wed, 7 Sep 2016 18:23:16 +0200 Subject: [PATCH 2/2] TEST: Add regression test for ticket #3179 Resolves: https://fedorahosted.org/sssd/ticket/3179 --- src/tests/intg/ldap_local_override_test.py | 37 ++ 1 file changed, 37 insertions(+) diff --git a/src/tests/intg/ldap_local_override_test.py b/src/tests/intg/ldap_local_override_test.py index 63de836..74d0462 100644 --- a/src/tests/intg/ldap_local_override_test.py +++ b/src/tests/intg/ldap_local_override_test.py @@ -902,6 +902,43 @@ def test_regr_2757_override(ldap_conn, env_regr_2757_override): pwd.getpwnam('alias1') +# Regression test for bug 3179 +# sss_override fails to export user/group data if name is not overriden + + +@pytest.fixture +def env_regr_3179_override(request, ldap_conn): + +prepare_sssd(request, ldap_conn) + +# Add entries +ent_list = ldap_ent.List(ldap_conn.ds_inst.base_dn) +ent_list.add_user("user1", 10001, 20001) +ent_list.add_group("empty_group", 2002, []) + +create_ldap_fixture(request, ldap_conn, ent_list) + +# Assert entries are not overridden +ent.assert_passwd_by_name( +'user1', +dict(name='user1', passwd='*', uid=10001, gid=20001)) + +# Override something something, but not name +subprocess.check_call(["sss_override", "user-add", "user1", + "-s", "/bin/myshell"]) +subprocess.check_call(["sss_override", "group-add", "empty_group", + "-g", "3000"]) + +restart_sssd() + + +def test_regr_3179_override(ldap_conn, env_regr_3179_override): + +# exporting should return 0 +subprocess.check_call(["sss_override", "user-export", "test_file"]) +subprocess.check_call(["sss_override", "group-export", "test_file"]) + + # Regression test for bug #2790 # sss_override --name doesn't work with RFC2307 and ghost users ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#15] Avoid returning System Error on clock skew (comment)
jhrozek commented on a pull request """ On Tue, Sep 06, 2016 at 06:09:58AM -0700, Jakub Hrozek wrote: > good idea ah, only when I started to implement this I realized it's already done :) See: https://github.com/SSSD/sssd/blob/master/src/providers/krb5/krb5_child.c#L1364 in the current master, KRB5_CHILD_DEBUG() expands into sss_log() as well unconditionally. """ See the full comment at https://github.com/SSSD/sssd/pull/15#issuecomment-245351855 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#19] KRB5: Send the output username, not internal fqname to krb5_child (+Accepted)
jhrozek's pull request #19: "KRB5: Send the output username, not internal fqname to krb5_child" label *Accepted* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/19 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (comment)
fidencio commented on a pull request """ A few minors in the commit messages that should be fixed before pushing. Patch0001: TOOLS: Fix a typo in groupadd() Remove the four spaces before the link of the trac ticket. Patch0002: TOOLS: sss_groupshow did not work Patch0003: TESTS: sss_groupadd/groupshow regressions Patch0004: TOOLS: use internal fqdn for DN Patch0006: TOOLS: sss_mc_refresh_nested_group short/fqname usage Patch0007: TESTS: Add FQDN variants for some tests Use (but do not exceed) 72 characters in the commit (body) message. I have the feeling you're breaking the line with 52. """ See the full comment at https://github.com/SSSD/sssd/pull/18#issuecomment-245345440 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (synchronize)
mzidek-rh's pull request #18: "[PATCHES] sss_user/groupmod fixes" was synchronize See the full pull-request at https://github.com/SSSD/sssd/pull/18 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/18/head:pr18 git checkout pr18 From 0bcb9ce46d91129516ab0b1d4862856e0e2af88f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 6 Sep 2016 12:13:08 +0200 Subject: [PATCH 1/7] TOOLS: Fix a typo in groupadd() Resolves: https://fedorahosted.org/sssd/ticket/3173 --- src/tools/sss_sync_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index a23a0b8..39ef5be 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c @@ -657,7 +657,7 @@ int groupadd(struct ops_ctx *data) int ret; data->sysdb_fqname = sss_create_internal_fqname(data, -data->sysdb_fqname, +data->name, data->domain->name); if (data->sysdb_fqname == NULL) { return ENOMEM; From 48449165abbbfb807dc928085cce4d6f50af96df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 13:46:53 +0200 Subject: [PATCH 2/7] TOOLS: sss_groupshow did not work sss_groupshow used shortname to search in sysdb database. We have to u e sysdb_fqname (aka internal_fqname) format for all sysdb oprations. Resolves: https://fedorahosted.org/sssd/ticket/3175 --- src/tools/sss_groupshow.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/tools/sss_groupshow.c b/src/tools/sss_groupshow.c index 41d7475..5870cc8 100644 --- a/src/tools/sss_groupshow.c +++ b/src/tools/sss_groupshow.c @@ -318,7 +318,7 @@ int group_show(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb, struct sss_domain_info *domain, bool recursive, - const char *name, + const char *shortname, struct group_info **res) { struct group_info *root; @@ -326,11 +326,20 @@ int group_show(TALLOC_CTX *mem_ctx, struct ldb_message *msg = NULL; const char **group_members = NULL; int nmembers = 0; +char *sysdb_fqname = NULL; int ret; int i; +sysdb_fqname = sss_create_internal_fqname(mem_ctx, + shortname, + domain->name); +if (sysdb_fqname == NULL) { +return ENOMEM; +} + /* First, search for the root group */ -ret = sysdb_search_group_by_name(mem_ctx, domain, name, attrs, &msg); +ret = sysdb_search_group_by_name(mem_ctx, domain, sysdb_fqname, attrs, + &msg); if (ret) { DEBUG(SSSDBG_OP_FAILURE, "Search failed: %s (%d)\n", strerror(ret), ret); From a71e0c521d3a1d8dee3dcfd41fa5f5c3decc26af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 17:37:14 +0200 Subject: [PATCH 3/7] TESTS: sss_groupadd/groupshow regressions Adds regression CI test for ticket #3173 and #3175. Resolves: https://fedorahosted.org/sssd/ticket/3173 https://fedorahosted.org/sssd/ticket/3175 --- src/tests/intg/test_local_domain.py | 26 ++ 1 file changed, 26 insertions(+) diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py index b83e56d..56e3812 100644 --- a/src/tests/intg/test_local_domain.py +++ b/src/tests/intg/test_local_domain.py @@ -19,11 +19,13 @@ import os import stat import pwd +import grp import time import config import signal import subprocess import pytest +import ent from util import unindent @@ -90,6 +92,11 @@ def assert_nonexistent_user(name): pwd.getpwnam(name) +def assert_nonexistent_group(name): +with pytest.raises(KeyError): +grp.getgrnam(name) + + def test_wrong_LC_ALL(local_domain_only): """ Regression test for ticket @@ -107,3 +114,22 @@ def test_wrong_LC_ALL(local_domain_only): subprocess.check_call(["sss_userdel", "foo", "-R"]) assert_nonexistent_user("foo") os.environ["LC_ALL"] = oldvalue + + +def test_sss_group_add_show_del(local_domain_only): +""" +Regression test for tickets +https://fedorahosted.org/sssd/ticket/3173 +https://fedorahosted.org/sssd/ticket/3175 +""" + +subprocess.check_call(["sss_groupadd", "foo", "-g", "10001"]) + +"This should not raise KeyError" +ent.assert_group_by_name("foo", dict(name="foo", gid=10001)) + +"sss_grupshow should return 0 with existing group name" +subprocess.check_call(["sss_groupshow", "foo"]) + +subprocess.check_call(["sss_groupdel", "foo"]) +assert_nonexistent_group("foo") From a065fbfbb7de34f06981c5dade3da123433503dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: We
[SSSD] [sssd PR#19] KRB5: Send the output username, not internal fqname to krb5_child (comment)
lslebodn commented on a pull request """ On (07/09/16 08:49), Jakub Hrozek wrote: >btw feel free to ping me on RH IRC for a link that shows the patch fixes the >RH tests.. > When I was looking into this bug https://bugzilla.redhat.com/show_bug.cgi?id=1372753#c7 I verified in gdb that setting kr->pd->user to shorname (after krb5_setup) fixed the issue. And you fixed it in right way :-) ACK Local CI passed. Feel free to push LS """ See the full comment at https://github.com/SSSD/sssd/pull/19#issuecomment-245340476 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (comment)
lslebodn commented on a pull request """ On (07/09/16 07:18), mzidek-rh wrote: >Hi, > >This PR is blocked by PR#16 (TOOLS: sss_groupshow did not work). It includes >fix for the ticket https://fedorahosted.org/sssd/ticket/3178. > >Michal >You can view, comment on, or merge this pull request online at: > > https://github.com/SSSD/sssd/pull/18 > 6th patch src/tools/tools_mc_util.c if (ret != EOK) { DEBUG(SSSDBG_MINOR_FAILURE, "Malformed DN [%s]? Skipping\n", (const char *) el->values[i].data); -talloc_free(parent_name); +talloc_free(parent_internal_name); continue; } -ret = sss_mc_refresh_group(parent_name); -talloc_free(parent_name); +ret = sss_parse_internal_fqname(tmpctx, parent_internal_name, +&parent_shortname, NULL); we should use sss_output_name here +if (ret != EOK) { +DEBUG(SSSDBG_MINOR_FAILURE, "Could not parse name %s\n", + parent_internal_name); +goto done; +} + +ret = sss_mc_refresh_group(parent_shortname); +talloc_free(parent_internal_name); +talloc_free(parent_shortname); otherwise LGTM LS """ See the full comment at https://github.com/SSSD/sssd/pull/18#issuecomment-245334928 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#19] KRB5: Send the output username, not internal fqname to krb5_child (comment)
jhrozek commented on a pull request """ btw feel free to ping me on RH IRC for a link that shows the patch fixes the RH tests.. """ See the full comment at https://github.com/SSSD/sssd/pull/19#issuecomment-245325705 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#19] KRB5: Send the output username, not internal fqname to krb5_child (opened)
jhrozek's pull request #19: "KRB5: Send the output username, not internal fqname to krb5_child" was opened PR body: """ Resolves: https://fedorahosted.org/sssd/ticket/3172 krb5_child calls krb5_kuserok() during the access phase which checks if a particular user is allowed to authenticate as a particular principal. We used to pass the internal fqname to krb5_kuserok() which broke the functionality and all users were denied access. This patch changes that to send the 'output' username to krb5_child, because that's the username the system receives through getpwnam() or getpwuid() anyway. The patch also adds a new structure member fo the krb5child_req structure to avoid reusing the pd->user variable but have an explicit one that serves as the input for the child process. """ See the full pull-request at https://github.com/SSSD/sssd/pull/19 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/19/head:pr19 git checkout pr19 From 439b3f0585993f4c9161f958ebd34498b73b2377 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Wed, 7 Sep 2016 12:07:36 +0200 Subject: [PATCH] KRB5: Send the output username, not internal fqname to krb5_child Resolves: https://fedorahosted.org/sssd/ticket/3172 krb5_child calls krb5_kuserok() during the access phase which checks if a particular user is allowed to authenticate as a particular principal. We used to pass the internal fqname to krb5_kuserok() which broke the functionality and all users were denied access. This patch changes that to send the 'output' username to krb5_child, because that's the username the system receives through getpwnam() or getpwuid() anyway. The patch also adds a new structure member fo the krb5child_req structure to avoid reusing the pd->user variable but have an explicit one that serves as the input for the child process. --- src/providers/krb5/krb5_access.c| 10 -- src/providers/krb5/krb5_auth.c | 18 ++ src/providers/krb5/krb5_auth.h | 9 ++--- src/providers/krb5/krb5_child_handler.c | 4 ++-- 4 files changed, 30 insertions(+), 11 deletions(-) diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c index 3afb901..be9068c 100644 --- a/src/providers/krb5/krb5_access.c +++ b/src/providers/krb5/krb5_access.c @@ -51,6 +51,7 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, int ret; const char **attrs; struct ldb_result *res; +struct sss_domain_info *dom; req = tevent_req_create(mem_ctx, &state, struct krb5_access_state); if (req == NULL) { @@ -64,8 +65,13 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, state->krb5_ctx = krb5_ctx; state->access_allowed = false; -ret = krb5_setup(state, pd, krb5_ctx, be_ctx->domain->case_sensitive, - &state->kr); +ret = get_domain_or_subdomain(be_ctx, pd->domain, &dom); +if (ret != EOK) { +DEBUG(SSSDBG_OP_FAILURE, "get_domain_or_subdomain failed.\n"); +goto done; +} + +ret = krb5_setup(state, pd, dom, krb5_ctx, &state->kr); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "krb5_setup failed.\n"); goto done; diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index dabf55c..f0f2280 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -174,8 +174,10 @@ get_krb_primary(struct map_id_name_to_krb_primary *name_to_primary, return ret; } -errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, - struct krb5_ctx *krb5_ctx, bool cs, +errno_t krb5_setup(TALLOC_CTX *mem_ctx, + struct pam_data *pd, + struct sss_domain_info *dom, + struct krb5_ctx *krb5_ctx, struct krb5child_req **_krb5_req) { struct krb5child_req *kr; @@ -201,13 +203,21 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd, kr->krb5_ctx = krb5_ctx; ret = get_krb_primary(krb5_ctx->name_to_primary, - pd->user, cs, &mapped_name); + pd->user, dom->case_sensitive, &mapped_name); if (ret == EOK) { DEBUG(SSSDBG_TRACE_FUNC, "Setting mapped name to: %s\n", mapped_name); kr->user = mapped_name; +kr->kuserok_user = mapped_name; } else if (ret == ENOENT) { DEBUG(SSSDBG_TRACE_ALL, "No mapping for: %s\n", pd->user); kr->user = pd->user; + +kr->kuserok_user = sss_output_name(kr, kr->user, + dom->case_sensitive, 0); +if (kr->kuserok_user == NULL) { +ret = ENOMEM; +goto done; +} } else { DEBUG(SSSDBG_CRIT_FAILURE, "get_krb_primary failed - %s:[%d]\n", sss_strerror(ret), ret); @@ -534,7 +544,7 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx, attrs[6] = SYSDB_AUTH_TYPE; attrs[7] = NU
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (closed)
jhrozek's pull request #14: "Attempting to create a qualified name from a NULL pointer broke sss_groupadd" was closed See the full pull-request at https://github.com/SSSD/sssd/pull/14 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/14/head:pr14 git checkout pr14 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (comment)
mzidek-rh commented on a pull request """ Thanks for the tip Nick. """ See the full comment at https://github.com/SSSD/sssd/pull/18#issuecomment-245304406 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (comment)
spbnick commented on a pull request """ Just a hint: if you avoid putting "PR" in front of the PR#16, then you'll get a link to the actual pull request on the GitHub page, plus the target pull request will have a link back. Like this: #16. """ See the full comment at https://github.com/SSSD/sssd/pull/18#issuecomment-245302411 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#18] [PATCHES] sss_user/groupmod fixes (opened)
mzidek-rh's pull request #18: "[PATCHES] sss_user/groupmod fixes" was opened PR body: """ Hi, This PR is blocked by PR#16 (TOOLS: sss_groupshow did not work). It includes fix for the ticket https://fedorahosted.org/sssd/ticket/3178. Michal """ See the full pull-request at https://github.com/SSSD/sssd/pull/18 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/18/head:pr18 git checkout pr18 From 0bcb9ce46d91129516ab0b1d4862856e0e2af88f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 6 Sep 2016 12:13:08 +0200 Subject: [PATCH 1/7] TOOLS: Fix a typo in groupadd() Resolves: https://fedorahosted.org/sssd/ticket/3173 --- src/tools/sss_sync_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index a23a0b8..39ef5be 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c @@ -657,7 +657,7 @@ int groupadd(struct ops_ctx *data) int ret; data->sysdb_fqname = sss_create_internal_fqname(data, -data->sysdb_fqname, +data->name, data->domain->name); if (data->sysdb_fqname == NULL) { return ENOMEM; From 48449165abbbfb807dc928085cce4d6f50af96df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 13:46:53 +0200 Subject: [PATCH 2/7] TOOLS: sss_groupshow did not work sss_groupshow used shortname to search in sysdb database. We have to u e sysdb_fqname (aka internal_fqname) format for all sysdb oprations. Resolves: https://fedorahosted.org/sssd/ticket/3175 --- src/tools/sss_groupshow.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/tools/sss_groupshow.c b/src/tools/sss_groupshow.c index 41d7475..5870cc8 100644 --- a/src/tools/sss_groupshow.c +++ b/src/tools/sss_groupshow.c @@ -318,7 +318,7 @@ int group_show(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb, struct sss_domain_info *domain, bool recursive, - const char *name, + const char *shortname, struct group_info **res) { struct group_info *root; @@ -326,11 +326,20 @@ int group_show(TALLOC_CTX *mem_ctx, struct ldb_message *msg = NULL; const char **group_members = NULL; int nmembers = 0; +char *sysdb_fqname = NULL; int ret; int i; +sysdb_fqname = sss_create_internal_fqname(mem_ctx, + shortname, + domain->name); +if (sysdb_fqname == NULL) { +return ENOMEM; +} + /* First, search for the root group */ -ret = sysdb_search_group_by_name(mem_ctx, domain, name, attrs, &msg); +ret = sysdb_search_group_by_name(mem_ctx, domain, sysdb_fqname, attrs, + &msg); if (ret) { DEBUG(SSSDBG_OP_FAILURE, "Search failed: %s (%d)\n", strerror(ret), ret); From a71e0c521d3a1d8dee3dcfd41fa5f5c3decc26af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 17:37:14 +0200 Subject: [PATCH 3/7] TESTS: sss_groupadd/groupshow regressions Adds regression CI test for ticket #3173 and #3175. Resolves: https://fedorahosted.org/sssd/ticket/3173 https://fedorahosted.org/sssd/ticket/3175 --- src/tests/intg/test_local_domain.py | 26 ++ 1 file changed, 26 insertions(+) diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py index b83e56d..56e3812 100644 --- a/src/tests/intg/test_local_domain.py +++ b/src/tests/intg/test_local_domain.py @@ -19,11 +19,13 @@ import os import stat import pwd +import grp import time import config import signal import subprocess import pytest +import ent from util import unindent @@ -90,6 +92,11 @@ def assert_nonexistent_user(name): pwd.getpwnam(name) +def assert_nonexistent_group(name): +with pytest.raises(KeyError): +grp.getgrnam(name) + + def test_wrong_LC_ALL(local_domain_only): """ Regression test for ticket @@ -107,3 +114,22 @@ def test_wrong_LC_ALL(local_domain_only): subprocess.check_call(["sss_userdel", "foo", "-R"]) assert_nonexistent_user("foo") os.environ["LC_ALL"] = oldvalue + + +def test_sss_group_add_show_del(local_domain_only): +""" +Regression test for tickets +https://fedorahosted.org/sssd/ticket/3173 +https://fedorahosted.org/sssd/ticket/3175 +""" + +subprocess.check_call(["sss_groupadd", "foo", "-g", "10001"]) + +"This should not raise KeyError" +ent.assert_group_by_name("foo", dict(name="foo", gid=10001)) + +"sss_grupshow should return 0 with existing group name" +subprocess.check_call(["sss_groupshow", "foo"]) + +subprocess.check_call(["sss_groupdel",
[SSSD] [sssd PR#17] Improve support for gdm Smartcard support (opened)
sumit-bose's pull request #17: "Improve support for gdm Smartcard support" was opened PR body: """ Those two patches try to fix two issues related to the Smartcard handling feature of gdm. The first fixes https://fedorahosted.org/sssd/ticket/3165 and the second fixes an issue which was introduced by the sysdb refactoring. """ See the full pull-request at https://github.com/SSSD/sssd/pull/17 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/17/head:pr17 git checkout pr17 From 905e3ef9a3fcd4b75e87435a3d37303d100739f5 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Wed, 31 Aug 2016 14:32:31 +0200 Subject: [PATCH 1/2] p11: only set PKCS11_LOGIN_TOKEN_NAME if gdm-smartcard is used Resolves https://fedorahosted.org/sssd/ticket/3165 --- src/responder/pam/pamsrv_p11.c | 33 +-- src/tests/cmocka/test_pam_srv.c | 89 +++-- 2 files changed, 97 insertions(+), 25 deletions(-) diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c index a2514f6..22da330 100644 --- a/src/responder/pam/pamsrv_p11.c +++ b/src/responder/pam/pamsrv_p11.c @@ -505,7 +505,11 @@ errno_t pam_check_cert_recv(struct tevent_req *req, TALLOC_CTX *mem_ctx, } /* The PKCS11_LOGIN_TOKEN_NAME environment variable is e.g. used by the Gnome - * Settings Daemon to determine the name of the token used for login */ + * Settings Daemon to determine the name of the token used for login but it + * should be only set if SSSD is called by gdm-smartcard. Otherwise desktop + * components might assume that gdm-smartcard PAM stack is configured + * correctly which might not be the case e.g. if Smartcard authentication was + * used when running gdm-password. */ #define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME" errno_t add_pam_cert_response(struct pam_data *pd, const char *sysdb_username, @@ -553,19 +557,22 @@ errno_t add_pam_cert_response(struct pam_data *pd, const char *sysdb_username, return ret; } -env = talloc_asprintf(pd, "%s=%s", PKCS11_LOGIN_TOKEN_ENV_NAME, token_name); -if (env == NULL) { -DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); -return ENOMEM; -} +if (strcmp(pd->service, "gdm-smartcard") == 0) { +env = talloc_asprintf(pd, "%s=%s", PKCS11_LOGIN_TOKEN_ENV_NAME, + token_name); +if (env == NULL) { +DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); +return ENOMEM; +} -ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(env) + 1, - (uint8_t *)env); -talloc_free(env); -if (ret != EOK) { -DEBUG(SSSDBG_OP_FAILURE, - "pam_add_response failed to add environment variable.\n"); -return ret; +ret = pam_add_response(pd, SSS_PAM_ENV_ITEM, strlen(env) + 1, + (uint8_t *)env); +talloc_free(env); +if (ret != EOK) { +DEBUG(SSSDBG_OP_FAILURE, + "pam_add_response failed to add environment variable.\n"); +return ret; +} } return ret; diff --git a/src/tests/cmocka/test_pam_srv.c b/src/tests/cmocka/test_pam_srv.c index 5de092d..02199e6 100644 --- a/src/tests/cmocka/test_pam_srv.c +++ b/src/tests/cmocka/test_pam_srv.c @@ -554,7 +554,7 @@ static void mock_input_pam(TALLOC_CTX *mem_ctx, const char *name, } static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, -const char *pin) +const char *pin, const char *service) { size_t buf_size; uint8_t *m_buf; @@ -576,7 +576,7 @@ static void mock_input_pam_cert(TALLOC_CTX *mem_ctx, const char *name, pi.pam_authtok_type = SSS_AUTHTOK_TYPE_SC_PIN; } -pi.pam_service = "login"; +pi.pam_service = service == NULL ? "login" : service; pi.pam_service_size = strlen(pi.pam_service) + 1; pi.pam_tty = "/dev/tty"; pi.pam_tty_size = strlen(pi.pam_tty) + 1; @@ -626,7 +626,8 @@ static int test_pam_simple_check(uint32_t status, uint8_t *body, size_t blen) #define PKCS11_LOGIN_TOKEN_ENV_NAME "PKCS11_LOGIN_TOKEN_NAME" -static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) +static int test_pam_cert_check_gdm_smartcard(uint32_t status, uint8_t *body, + size_t blen) { size_t rp = 0; uint32_t val; @@ -675,6 +676,44 @@ static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) return EOK; } +static int test_pam_cert_check(uint32_t status, uint8_t *body, size_t blen) +{ +size_t rp = 0; +uint32_t val; + +assert_int_equal(status, 0); + +SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); +assert_int_equal(val, pam_test_ctx->exp_pam_status); + +SAFEALIGN_COPY_UINT32(&val, body + rp, &rp); +assert_int_equal(val, 2); + +SAFEALIGN_COPY_UINT32(
[SSSD] Re: Getting overridden shell in pam_sss
On Wed, Sep 07, 2016 at 02:34:19PM +0300, Nikolai Kondrashov wrote: > On 09/07/2016 02:18 PM, Sumit Bose wrote: > > On Wed, Sep 07, 2016 at 01:28:12PM +0300, Nikolai Kondrashov wrote: > > > Hi Sumit, > > > > > > Just wanted to tell you I still need an answer to the below. > > > > ah, sorry, I think I missed this question while discussing the group > > lookups with Simo in the other thread. > > No problem, happens to everyone, thanks for answering :) > > > > Thanks! > > > > > > Nick > > > > > > On 08/19/2016 07:39 PM, Nikolai Kondrashov wrote: > > > > Now I'm again approaching the implementation of tlog integration in > > > > pam_sss, > > > > and as planned, I need to get the actual user shell to put it into > > > > TLOG_REC_SHELL environment variable upon opening of the session. > > > > > > > > However, the get_shell_override, which does all the hops and tricks to > > > > get it, > > > > requires nss_ctx, which belongs to NSS responder, specifically various > > > > shell-related configuration settings > > > > (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. > > > > essentially the > > > > PAM responder needs to be an NSS responder to get it. > > > > > > > > To me it seems that there is no exit but to finally put that override > > > > machinery into a library, instead of having it directly in the NSS > > > > responder. > > > > > > > > Am I wrong? Is there perhaps another way? > > > > Do you have any suggestion how to best do it? > > > > I would move get_shell_override() to > > src/responder/common/responder_utils.c and replace the nss responder > > context in the parameter list by a new struct which contains all the > > shell related elements currently kept directly in the nss context. This > > struct should be defined in the common responder code as well so that > > the PAM responder can use it as well and add it to its own context. > > Finally, to avoid code duplication I would put the code which reads all > > the shell related options in nss_get_config() for a new common all with > > initializes the new struct with the values from the configuration and > > add this call to pam_process_init() so that the PAM responder knows > > about the options as well. Now you should be able to call > > get_shell_override from the PAM responder as well. > > This is similar to what I had in mind. Although, I'd like to extract all the > *_override functions together in this manner, if you don't mind. sure, please go ahead. This would it also make it easier to write unit-tests for those calls (hint, hint, hint :-). bye, Sumit > > Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (+rejected)
jhrozek's pull request #14: "Attempting to create a qualified name from a NULL pointer broke sss_groupadd" label *rejected* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/14 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (-rejected)
jhrozek's pull request #14: "Attempting to create a qualified name from a NULL pointer broke sss_groupadd" label *rejected* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/14 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (-Changes requested)
jhrozek's pull request #14: "Attempting to create a qualified name from a NULL pointer broke sss_groupadd" label *Changes requested* has been removed See the full pull-request at https://github.com/SSSD/sssd/pull/14 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (+rejected)
jhrozek's pull request #14: "Attempting to create a qualified name from a NULL pointer broke sss_groupadd" label *rejected* has been added See the full pull-request at https://github.com/SSSD/sssd/pull/14 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (comment)
jhrozek commented on a pull request """ On Tue, Sep 06, 2016 at 10:23:13AM -0700, mzidek-rh wrote: > The PR#16 includes jhrozek's patch from this PR as well as CI test + fix for > sss_groupshow. OK, thank you, I will close this one as rejected (just so that we don't track the PR any longer..) """ See the full comment at https://github.com/SSSD/sssd/pull/14#issuecomment-245260472 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [RFCv3] NSS tlog integration
On 08/29/2016 11:47 PM, Sumit Bose wrote: Finally you call sysdb_initgroups_with_views() to get the list of groups the user is a member of and compare them with the groups form the session_recording configuration. Since you compare the DNs I think this can be improved a bit. sysdb_initgroups_with_views() does a dereference search based on the memberOf attribute of the user which holds the DNs of all the groups the user is a member of. But since you only need the DNs of the groups for the comparison you can just add the memberOf attribute to the attribute list in nss_cmd_getpwnam_search() and friends to make the DNs of the groups available in session_recording_is_enabled(). And one more question: the above basically means that we need to make sysdb_getpwnam retrieve "memberOf" as well. Do we want to do that? If yes, do we want to do that unconditionally, or change the interface to have that optional (ugh)? Do we want to change sysdb_enumpwent_filter (used in setpwent) to do that as well? We went through some of this in a chat a while ago, but I just want to be sure. Thanks! Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Getting overridden shell in pam_sss
On 09/07/2016 02:18 PM, Sumit Bose wrote: On Wed, Sep 07, 2016 at 01:28:12PM +0300, Nikolai Kondrashov wrote: Hi Sumit, Just wanted to tell you I still need an answer to the below. ah, sorry, I think I missed this question while discussing the group lookups with Simo in the other thread. No problem, happens to everyone, thanks for answering :) Thanks! Nick On 08/19/2016 07:39 PM, Nikolai Kondrashov wrote: Now I'm again approaching the implementation of tlog integration in pam_sss, and as planned, I need to get the actual user shell to put it into TLOG_REC_SHELL environment variable upon opening of the session. However, the get_shell_override, which does all the hops and tricks to get it, requires nss_ctx, which belongs to NSS responder, specifically various shell-related configuration settings (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. essentially the PAM responder needs to be an NSS responder to get it. To me it seems that there is no exit but to finally put that override machinery into a library, instead of having it directly in the NSS responder. Am I wrong? Is there perhaps another way? Do you have any suggestion how to best do it? I would move get_shell_override() to src/responder/common/responder_utils.c and replace the nss responder context in the parameter list by a new struct which contains all the shell related elements currently kept directly in the nss context. This struct should be defined in the common responder code as well so that the PAM responder can use it as well and add it to its own context. Finally, to avoid code duplication I would put the code which reads all the shell related options in nss_get_config() for a new common all with initializes the new struct with the values from the configuration and add this call to pam_process_init() so that the PAM responder knows about the options as well. Now you should be able to call get_shell_override from the PAM responder as well. This is similar to what I had in mind. Although, I'd like to extract all the *_override functions together in this manner, if you don't mind. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Getting overridden shell in pam_sss
On 09/07/2016 01:48 PM, Pavel Březina wrote: On 08/19/2016 06:39 PM, Nikolai Kondrashov wrote: Hi Sumit, Now I'm again approaching the implementation of tlog integration in pam_sss, and as planned, I need to get the actual user shell to put it into TLOG_REC_SHELL environment variable upon opening of the session. However, the get_shell_override, which does all the hops and tricks to get it, requires nss_ctx, which belongs to NSS responder, specifically various shell-related configuration settings (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. essentially the PAM responder needs to be an NSS responder to get it. All of these seems to be just simple sssd.conf options, feel free to get them with confdb api. See nss_get_config(). Well, these are not only options, but also logic that interprets them, and I don't want to essentially copy the corresponding code from NSS responder to PAM responder. To me it seems that there is no exit but to finally put that override machinery into a library, instead of having it directly in the NSS responder. This would be awesome though :-) Yes, I would like that too, but I'd like to wait for Sumit's response :) Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Getting overridden shell in pam_sss
On Wed, Sep 07, 2016 at 01:28:12PM +0300, Nikolai Kondrashov wrote: > Hi Sumit, > > Just wanted to tell you I still need an answer to the below. ah, sorry, I think I missed this question while discussing the group lookups with Simo in the other thread. > > Thanks! > > Nick > > On 08/19/2016 07:39 PM, Nikolai Kondrashov wrote: > > Now I'm again approaching the implementation of tlog integration in pam_sss, > > and as planned, I need to get the actual user shell to put it into > > TLOG_REC_SHELL environment variable upon opening of the session. > > > > However, the get_shell_override, which does all the hops and tricks to get > > it, > > requires nss_ctx, which belongs to NSS responder, specifically various > > shell-related configuration settings > > (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. essentially > > the > > PAM responder needs to be an NSS responder to get it. > > > > To me it seems that there is no exit but to finally put that override > > machinery into a library, instead of having it directly in the NSS > > responder. > > > > Am I wrong? Is there perhaps another way? > > Do you have any suggestion how to best do it? I would move get_shell_override() to src/responder/common/responder_utils.c and replace the nss responder context in the parameter list by a new struct which contains all the shell related elements currently kept directly in the nss context. This struct should be defined in the common responder code as well so that the PAM responder can use it as well and add it to its own context. Finally, to avoid code duplication I would put the code which reads all the shell related options in nss_get_config() for a new common all with initializes the new struct with the values from the configuration and add this call to pam_process_init() so that the PAM responder knows about the options as well. Now you should be able to call get_shell_override from the PAM responder as well. HTH bye, Sumit ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Getting overridden shell in pam_sss
On 08/19/2016 06:39 PM, Nikolai Kondrashov wrote: Hi Sumit, Now I'm again approaching the implementation of tlog integration in pam_sss, and as planned, I need to get the actual user shell to put it into TLOG_REC_SHELL environment variable upon opening of the session. However, the get_shell_override, which does all the hops and tricks to get it, requires nss_ctx, which belongs to NSS responder, specifically various shell-related configuration settings (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. essentially the PAM responder needs to be an NSS responder to get it. All of these seems to be just simple sssd.conf options, feel free to get them with confdb api. See nss_get_config(). To me it seems that there is no exit but to finally put that override machinery into a library, instead of having it directly in the NSS responder. This would be awesome though :-) ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [RFCv3] NSS tlog integration
Hi Simo, On 08/29/2016 11:47 PM, Sumit Bose wrote: Then you do a normal user lookup but if there are groups configured in the session_recording section you always do a SSS_DP_INITGROUPS instead of a SSS_DP_USER if the entry is expired. I think here you can add some improvement to reduce the number of SSS_DP_INITGROUPS calls which are more expensive than SSS_DP_USER. User entries in the cache have two timeout values SYSDB_CACHE_EXPIRE and SYSDB_INITGR_EXPIRE. The first is the expiration time for the general user data (name, home directory, shell etc) and is checked during the normal user lookup. The second is the expiration time of the group-membership data. To make sure that SSS_DP_INITGROUPS is only used when needed I would recommend to additionally check if SYSDB_INITGR_EXPIRE is expired and only do a SSS_DP_INITGROUPS in this case and a SSS_DP_USER in all other cases. Currently both timestamps might not differ often but future enhancements to the backends might change this. I'm looking at this and am getting confused (old news, I know :). Two cases where I use SSS_DP_INITGROUPS are invocations of check_cache, which has this code: /* if we have any reply let's check cache validity */ if (res->count > 0) { /* ... */ if (req_type == SSS_DP_INITGROUPS) { cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_INITGR_EXPIRE, 0); } else { cacheExpire = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_CACHE_EXPIRE, 0); } /* ... */ } It seems to me that it already checks the cache as you suggested. Is that so? The only other case is in nss_cmd_setpwent_step, which fetches users for getpwent using sss_dp_get_account_send and it doesn't seem that I could/should do anything there. Is that right? Thank you. Nick ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: Getting overridden shell in pam_sss
Hi Sumit, Just wanted to tell you I still need an answer to the below. Thanks! Nick On 08/19/2016 07:39 PM, Nikolai Kondrashov wrote: Now I'm again approaching the implementation of tlog integration in pam_sss, and as planned, I need to get the actual user shell to put it into TLOG_REC_SHELL environment variable upon opening of the session. However, the get_shell_override, which does all the hops and tricks to get it, requires nss_ctx, which belongs to NSS responder, specifically various shell-related configuration settings (override_shell/allowed_shells/vetoed_shells/etc_shells). I.e. essentially the PAM responder needs to be an NSS responder to get it. To me it seems that there is no exit but to finally put that override machinery into a library, instead of having it directly in the NSS responder. Am I wrong? Is there perhaps another way? Do you have any suggestion how to best do it? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] My development scripts
Hi folks, while converting my workflow scripts to github [1] I decided to polish them, give them some help and publish them on github. Feel free to use them and improve them as you wish. I'll gladly learn about your workflow and simplification. [1] https://github.com/pbrezina/sssd-dev-utils ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache
On 09/07/2016 09:53 AM, Jakub Hrozek wrote: On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote: On (05/09/16 16:07), Jakub Hrozek wrote: On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote: On (05/09/16 15:24), Jakub Hrozek wrote: On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote: On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio wrote: Petr, I went through your patches and in general they look good to me. However, I haven't done any tests yet with your patches (and I'll do it after lunch). I've done some tests and I've been able to see the ldif changes in the domain log. So, I assume it's working. For sure it's a good improvement! Would be worth to link some documentation about ldiff as it may be confusing for someone who is not used to it. I'll wait for a new version of the patches and go through them again. I really would like to have someone's else opinion on this series. I quickly scrolled through the patches and the primary thing I don't understand is why are the wrappers used only in sysdb? I think we should just use them everywhere.. I do not like wrappers. We should not log ldif by default. That's why there is a separate log level, you need to turn these on separately (yes, logging LDIFs by default would be too much..) Even though it is a separate debug level users still might enable them by a chance. How, except reading the code? This new debug level is not documented anywhere and even starts off at a different base so neither debug_level=10 nor debug_level=0xFFF0 will trigger this. IMH0 it will be confusing for them. There are many users which are confused when try to analyze sudo logs. They can see some "LDAP like" filters which are used for internal searching. Users think we use wrong attribute due to sudoRule -> sudoRole. really? Someone who will discover a magic constant on SSSD will then be confused by more logging? btw what if this extra debugging was controlled by an environment variable, do you think then it would be safer? Or if we prepend the LDIF with something like "SSSD cache modification message" ? These wrappers should not be used in sssd upstream. They can be prepared for debugging purposes in development process. ...which means we will have to rebase the patches, build the correct version, pass it on to the person and care about correct versioning... Sorry, I just don't agree with any of the arguments, but I'm curious to hear more. Thanks, Jakub. Lukas, I am afraid I am not able to imagine that we have some wrappers which is not used in code but software engineers use them for their daily job. It sounds like obstacle for me. I understand that we have to be care of logs... but this high debug level isn't mentioned in man page. So if user will use it it will be accident anyway. Regards -- Petr^4 Čech ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] [sssd PR#16] TOOLS: sss_groupshow did not work (synchronize)
mzidek-rh's pull request #16: "TOOLS: sss_groupshow did not work" was synchronize See the full pull-request at https://github.com/SSSD/sssd/pull/16 ... or pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/16/head:pr16 git checkout pr16 From 0bcb9ce46d91129516ab0b1d4862856e0e2af88f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Tue, 6 Sep 2016 12:13:08 +0200 Subject: [PATCH 1/3] TOOLS: Fix a typo in groupadd() Resolves: https://fedorahosted.org/sssd/ticket/3173 --- src/tools/sss_sync_ops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c index a23a0b8..39ef5be 100644 --- a/src/tools/sss_sync_ops.c +++ b/src/tools/sss_sync_ops.c @@ -657,7 +657,7 @@ int groupadd(struct ops_ctx *data) int ret; data->sysdb_fqname = sss_create_internal_fqname(data, -data->sysdb_fqname, +data->name, data->domain->name); if (data->sysdb_fqname == NULL) { return ENOMEM; From 48449165abbbfb807dc928085cce4d6f50af96df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 13:46:53 +0200 Subject: [PATCH 2/3] TOOLS: sss_groupshow did not work sss_groupshow used shortname to search in sysdb database. We have to u e sysdb_fqname (aka internal_fqname) format for all sysdb oprations. Resolves: https://fedorahosted.org/sssd/ticket/3175 --- src/tools/sss_groupshow.c | 13 +++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/src/tools/sss_groupshow.c b/src/tools/sss_groupshow.c index 41d7475..5870cc8 100644 --- a/src/tools/sss_groupshow.c +++ b/src/tools/sss_groupshow.c @@ -318,7 +318,7 @@ int group_show(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb, struct sss_domain_info *domain, bool recursive, - const char *name, + const char *shortname, struct group_info **res) { struct group_info *root; @@ -326,11 +326,20 @@ int group_show(TALLOC_CTX *mem_ctx, struct ldb_message *msg = NULL; const char **group_members = NULL; int nmembers = 0; +char *sysdb_fqname = NULL; int ret; int i; +sysdb_fqname = sss_create_internal_fqname(mem_ctx, + shortname, + domain->name); +if (sysdb_fqname == NULL) { +return ENOMEM; +} + /* First, search for the root group */ -ret = sysdb_search_group_by_name(mem_ctx, domain, name, attrs, &msg); +ret = sysdb_search_group_by_name(mem_ctx, domain, sysdb_fqname, attrs, + &msg); if (ret) { DEBUG(SSSDBG_OP_FAILURE, "Search failed: %s (%d)\n", strerror(ret), ret); From a71e0c521d3a1d8dee3dcfd41fa5f5c3decc26af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= Date: Tue, 6 Sep 2016 17:37:14 +0200 Subject: [PATCH 3/3] TESTS: sss_groupadd/groupshow regressions Adds regression CI test for ticket #3173 and #3175. Resolves: https://fedorahosted.org/sssd/ticket/3173 https://fedorahosted.org/sssd/ticket/3175 --- src/tests/intg/test_local_domain.py | 26 ++ 1 file changed, 26 insertions(+) diff --git a/src/tests/intg/test_local_domain.py b/src/tests/intg/test_local_domain.py index b83e56d..56e3812 100644 --- a/src/tests/intg/test_local_domain.py +++ b/src/tests/intg/test_local_domain.py @@ -19,11 +19,13 @@ import os import stat import pwd +import grp import time import config import signal import subprocess import pytest +import ent from util import unindent @@ -90,6 +92,11 @@ def assert_nonexistent_user(name): pwd.getpwnam(name) +def assert_nonexistent_group(name): +with pytest.raises(KeyError): +grp.getgrnam(name) + + def test_wrong_LC_ALL(local_domain_only): """ Regression test for ticket @@ -107,3 +114,22 @@ def test_wrong_LC_ALL(local_domain_only): subprocess.check_call(["sss_userdel", "foo", "-R"]) assert_nonexistent_user("foo") os.environ["LC_ALL"] = oldvalue + + +def test_sss_group_add_show_del(local_domain_only): +""" +Regression test for tickets +https://fedorahosted.org/sssd/ticket/3173 +https://fedorahosted.org/sssd/ticket/3175 +""" + +subprocess.check_call(["sss_groupadd", "foo", "-g", "10001"]) + +"This should not raise KeyError" +ent.assert_group_by_name("foo", dict(name="foo", gid=10001)) + +"sss_grupshow should return 0 with existing group name" +subprocess.check_call(["sss_groupshow", "foo"]) + +subprocess.check_call(["sss_groupdel", "foo"]) +assert_nonexistent_group("foo") ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.
[SSSD] [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
jhrozek commented on a pull request """ On Tue, Sep 06, 2016 at 11:49:09AM -0700, lslebodn wrote: > IMHO, it might be better to close this PR. > If we decide to dor support for libini_config < 1.1 or 1.2 > then it will be a different patch anyway. @see my previous comment The only reason I suggest deferred over closing is that if we close this PR, we will never remember to ressurect it. If it's going to be deferred, it will keep coming up in the PR list. """ See the full comment at https://github.com/SSSD/sssd/pull/10#issuecomment-245204278 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [PATCH SET] SYSDB: Adding message to inform about cache
On Wed, Sep 07, 2016 at 08:45:18AM +0200, Lukas Slebodnik wrote: > On (05/09/16 16:07), Jakub Hrozek wrote: > >On Mon, Sep 05, 2016 at 03:32:48PM +0200, Lukas Slebodnik wrote: > >> On (05/09/16 15:24), Jakub Hrozek wrote: > >> >On Mon, Sep 05, 2016 at 02:31:31PM +0200, Fabiano Fidêncio wrote: > >> >> On Mon, Sep 5, 2016 at 11:59 AM, Fabiano Fidêncio > >> >> wrote: > >> >> > Petr, > >> >> > > >> >> > I went through your patches and in general they look good to me. > >> >> > However, I haven't done any tests yet with your patches (and I'll do > >> >> > it after lunch). > >> >> > >> >> I've done some tests and I've been able to see the ldif changes in the > >> >> domain log. So, I assume it's working. > >> >> For sure it's a good improvement! Would be worth to link some > >> >> documentation about ldiff as it may be confusing for someone who is > >> >> not used to it. > >> >> > >> >> I'll wait for a new version of the patches and go through them again. > >> >> > >> >> I really would like to have someone's else opinion on this series. > >> > > >> >I quickly scrolled through the patches and the primary thing I don't > >> >understand is why are the wrappers used only in sysdb? I think we should > >> >just use them everywhere.. > >> I do not like wrappers. > >> We should not log ldif by default. > > > >That's why there is a separate log level, you need to turn these on > >separately (yes, logging LDIFs by default would be too much..) > > > Even though it is a separate debug level users still might > enable them by a chance. How, except reading the code? This new debug level is not documented anywhere and even starts off at a different base so neither debug_level=10 nor debug_level=0xFFF0 will trigger this. > IMH0 it will be confusing for them. > There are many users which are confused when try to analyze > sudo logs. They can see some "LDAP like" filters which > are used for internal searching. Users think we use wrong attribute > due to sudoRule -> sudoRole. really? Someone who will discover a magic constant on SSSD will then be confused by more logging? btw what if this extra debugging was controlled by an environment variable, do you think then it would be safer? Or if we prepend the LDIF with something like "SSSD cache modification message" ? > > These wrappers should not be used in sssd upstream. > They can be prepared for debugging purposes in development process. ...which means we will have to rebase the patches, build the correct version, pass it on to the person and care about correct versioning... Sorry, I just don't agree with any of the arguments, but I'm curious to hear more. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On Wed, Sep 7, 2016 at 9:03 AM, Lukas Slebodnik wrote: > On (07/09/16 08:46), Fabiano Fidêncio wrote: >>On Wed, Sep 7, 2016 at 8:34 AM, Lukas Slebodnik wrote: >>> On (06/09/16 21:38), Fabiano Fidêncio wrote: On Tue, Sep 6, 2016 at 8:49 PM, lslebodn wrote: > lslebodn commented on a pull request > > """ > IMHO, it might be better to close this PR. > If we decide to dor support for libini_config < 1.1 or 1.2 > then it will be a different patch anyway. @see my previous comment > """ Please, take a look on Jakub's comment and see what he proposed. >>> Do you mean: jhrozek commented on a pull request """ > - Ubuntu 12.04 LTS and older 12.04 is often used (for example travis-ci still offers only this distro) and ends its life in 2017. On one hand, it's unlikely that users of LTS distributions will run master, they will probably only run sssd-1-13 or some PPAs, on the other hand, I don't see too much value except for cleaner code. So all in all I suggest we nack this patchset for now and push it when Ubuntu 12.04 goes EOL. Does that sound like a reasonable compromise? """ >>> If we decide to drop support for older versions of libini_config >>> then we should also remove unnecessary wrappers in src/util/sss_ini.c. >>> and replace sss_ini* functions with direct usage of ini_*. >>> >>> Therefore this patchset will need to be changed. >>> IMHO, it will be much simpler to create new patch-set then. >>> >>> Anyways, I'm fine with closing this PR and opening a new one when we can drop support to libini_config < 1.3. Just for the record, from this whole discussion I could see that dropping support to augeas in order to use libini is something that shouldn't and is not going to happen any time soon >>> I do not understand how is this patch-set related to replacing augeas >>> with libini_config. >> >>Well, this patch set is _not_ related to replacing augeas with >>libini_config, we just will fall under the same issue in the future >>when doing that. >>If we introduce a new code that will deal with configuration files the >>least I would expect is that old systems could have some benefit from it >>in the same way they can have benefit from the current code. > Maybe you did not get my point and therefore you get wrong expectations. > I do not expect that old systems will benefit from new features. > e.g. > Currently rhel6 cannot: > * validate configuration file because it has old version of libini_config > * be integrated with "CIFS Client"[1] because it does not have necessary > dependencies > ... > >>So, if I'm the one writing or reviewing the code with this discussion in mind >>I'd opt for something that is present in the major distros and for >>sure libini > 1.3 is not. So, why not use augeas instead of it? :-) >> > Augeas is optional dependency libini_config-1.2 is optional dependency. > I cannot see a conflict for replacing it :-) > But it also does not make a sense > because we should drop manipulation with sssd.conf Okay, okay, > >>> >>> a) augeas is an optional dependency >>>if BUILD_IFP >>>if BUILD_CONFIG_LIB >>>pkglib_LTLIBRARIES += libsss_config.la >>> b) replacing augeas with libini_config requires minimal version 1.2 >>>which is already optionally used in master. >>> c) IIRC discussion from devel meeting. Some developers prefer >>>to drop feature for manipulation of sssd.conf and write from >>>scratch if there will be new requirements. Current version is very >>>limited. >>> as well and for the very same reasons that this patch wasn't accepted. We could, in the best (or worst?) case scenario support/depend on both due to compatibility with elder systems, but I can't see any advantage on that. >>> I do not suggest to support all features in old systems. >>> But I would like to support at least minimal feature set (ldap+krb5). >>> And parsing configuration file is required for minimal feature set :-) >>> >> >>Sure, sure. >> >>We wasted a lot of time on this patch set that could have been used >>reviewing more useful patches on the queue. >>Please, close the PR if you want to. >> > I'm sorry if you think we wasted a lot of time. There's no reason to be sorry for. > I did not want to close PR without proper explanation why I would > prefer to keep this optional code in sssd. It would be impolite > to close PR without general agreement. I don't think we are going to agree here. So, please, just close the PR. Best Regards, -- Fabiano Fidêncio ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)
On (07/09/16 08:46), Fabiano Fidêncio wrote: >On Wed, Sep 7, 2016 at 8:34 AM, Lukas Slebodnik wrote: >> On (06/09/16 21:38), Fabiano Fidêncio wrote: >>>On Tue, Sep 6, 2016 at 8:49 PM, lslebodn >>> wrote: lslebodn commented on a pull request """ IMHO, it might be better to close this PR. If we decide to dor support for libini_config < 1.1 or 1.2 then it will be a different patch anyway. @see my previous comment """ >>> >>>Please, take a look on Jakub's comment and see what he proposed. >>> >> Do you mean: >>> >>>jhrozek commented on a pull request >>> >>>""" - Ubuntu 12.04 LTS and older >>> >>>12.04 is often used (for example travis-ci still offers only this >>>distro) and ends its life in 2017. >>> >>>On one hand, it's unlikely that users of LTS distributions will run >>>master, they will probably only run sssd-1-13 or some PPAs, on the other >>>hand, I don't see too much value except for cleaner code. >>> >>>So all in all I suggest we nack this patchset for now and push it when >>>Ubuntu 12.04 goes EOL. >>> >>>Does that sound like a reasonable compromise? >>> >>>""" >> If we decide to drop support for older versions of libini_config >> then we should also remove unnecessary wrappers in src/util/sss_ini.c. >> and replace sss_ini* functions with direct usage of ini_*. >> >> Therefore this patchset will need to be changed. >> IMHO, it will be much simpler to create new patch-set then. >> >> >>>Anyways, I'm fine with closing this PR and opening a new one when we >>>can drop support to libini_config < 1.3. >>> >>>Just for the record, from this whole discussion I could see that >>>dropping support to augeas in order to use libini is something that >>>shouldn't and is not going to happen any time soon >> I do not understand how is this patch-set related to replacing augeas >> with libini_config. > >Well, this patch set is _not_ related to replacing augeas with >libini_config, we just will fall under the same issue in the future >when doing that. >If we introduce a new code that will deal with configuration files the >least I would expect is that old systems could have some benefit from it >in the same way they can have benefit from the current code. Maybe you did not get my point and therefore you get wrong expectations. I do not expect that old systems will benefit from new features. e.g. Currently rhel6 cannot: * validate configuration file because it has old version of libini_config * be integrated with "CIFS Client"[1] because it does not have necessary dependencies ... >So, if I'm the one writing or reviewing the code with this discussion in mind >I'd opt for something that is present in the major distros and for >sure libini > 1.3 is not. So, why not use augeas instead of it? :-) > Augeas is optional dependency libini_config-1.2 is optional dependency. I cannot see a conflict for replacing it :-) But it also does not make a sense because we should drop manipulation with sssd.conf >> >> a) augeas is an optional dependency >>if BUILD_IFP >>if BUILD_CONFIG_LIB >>pkglib_LTLIBRARIES += libsss_config.la >> b) replacing augeas with libini_config requires minimal version 1.2 >>which is already optionally used in master. >> c) IIRC discussion from devel meeting. Some developers prefer >>to drop feature for manipulation of sssd.conf and write from >>scratch if there will be new requirements. Current version is very >>limited. >> >>>as well and for the >>>very same reasons that this patch wasn't accepted. We could, in the >>>best (or worst?) case scenario support/depend on both due to >>>compatibility with elder systems, but I can't see any advantage on >>>that. >>> >> I do not suggest to support all features in old systems. >> But I would like to support at least minimal feature set (ldap+krb5). >> And parsing configuration file is required for minimal feature set :-) >> > >Sure, sure. > >We wasted a lot of time on this patch set that could have been used >reviewing more useful patches on the queue. >Please, close the PR if you want to. > I'm sorry if you think we wasted a lot of time. I did not want to close PR without proper explanation why I would prefer to keep this optional code in sssd. It would be impolite to close PR without general agreement. LS ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org