[SSSD] [sssd PR#16] TOOLS: sss_groupshow did not work (comment)

[mzidek-rh]

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]

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]

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#18] [PATCHES] sss_user/groupmod fixes (comment)

[lslebodn]

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

[Jakub Hrozek]

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

[Jakub Hrozek]

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]

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, , NULL);
-if (ret != EOK) {
-ret = ERR_WRONG_NAME_FORMAT;
-goto done;
+if (fqname != NULL) {
+ret = sss_parse_internal_fqname(tmp_ctx, fqname, , 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, , NULL);
-if (ret != EOK) {
-ret = ERR_WRONG_NAME_FORMAT;
-goto done;
+if (fqname != NULL) {
+ret = sss_parse_internal_fqname(tmp_ctx, fqname, , 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]

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 (comment)

[lslebodn]

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]

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,
+_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]

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]

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, , 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,
- >kr);
+ret = get_domain_or_subdomain(be_ctx, pd->domain, );
+if (ret != EOK) {
+DEBUG(SSSDBG_OP_FAILURE, "get_domain_or_subdomain failed.\n");
+goto done;
+}
+
+ret = krb5_setup(state, pd, dom, krb5_ctx, >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, _name);
+  pd->user, dom->case_sensitive, _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] = NULL;
 
-ret = 

[SSSD] [sssd PR#14] Attempting to create a qualified name from a NULL pointer broke sss_groupadd (closed)

[jhrozek]

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]

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]

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]

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, );
+ret = sysdb_search_group_by_name(mem_ctx, domain, sysdb_fqname, attrs,
+ );
 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", 

[SSSD] [sssd PR#17] Improve support for gdm Smartcard support (opened)

[sumit-bose]

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(, body + rp, );
+assert_int_equal(val, pam_test_ctx->exp_pam_status);
+
+SAFEALIGN_COPY_UINT32(, body + rp, );
+assert_int_equal(val, 2);
+
+

[SSSD] Re: Getting overridden shell in pam_sss

[Sumit Bose]

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]

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]

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]

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]

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]

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

[Nikolai Kondrashov]

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

[Nikolai Kondrashov]

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

[Nikolai Kondrashov]

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

[Sumit Bose]

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: [RFCv3] NSS tlog integration

[Nikolai Kondrashov]

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

[Nikolai Kondrashov]

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

[Pavel Březina]

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

[Petr Cech]

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#10] UTIL: Remove support to libini older than 1.0.0 (comment)

[jhrozek]

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

[Jakub Hrozek]

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)

[Fabiano Fidêncio]

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)

[Lukas Slebodnik]

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

[SSSD] Re: [sssd PR#10] UTIL: Remove support to libini older than 1.0.0 (comment)

[Fabiano Fidêncio]

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. 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? :-)

>
> 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.

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: [PATCH SET] SYSDB: Adding message to inform about cache

[Lukas Slebodnik]

On (05/09/16 15:35), Petr Cech wrote:
>On 09/05/2016 03:32 PM, 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.
>> I thought they would be used just for development purposes.
>> therefore they should not be used anywhere and not everywhere.
>> 
>> LS
>
>Hello Lukas,
>
>please, are you satisfied with those wrappers at really high debug level?
>
See my other answer.

LS
___
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

[Lukas Slebodnik]

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. 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.

These wrappers should not be used in sssd upstream.
They can be prepared for debugging purposes in development process.

LS
___
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)

[Lukas Slebodnik]

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.

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 :-)

LS
___
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

[Petr Cech]

On 09/06/2016 01:18 PM, Petr Cech wrote:



On 09/06/2016 01:15 PM, Petr Cech wrote:

On 09/05/2016 02:31 PM, 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.



Please, below you can see a few comments. Feel completely free to
ignore the first one if you feel like doing it, it's just a minor :-)
For the other comments, I'd like to understand a few changes you have
done.


Patch 0001: SYSDB: Adding message to inform which cache is used

About the following part of the patch:
+static const char *get_attr_storage(int state_mask)
+{
+const char *storage = "";
+
+if (state_mask == SSS_SYSDB_BOTH_CACHE ) {
+storage = "cache, ts_cache";
+} else if (state_mask == SSS_SYSDB_TS_CACHE) {
+storage = "ts_cache";
+} else if (state_mask == SSS_SYSDB_CACHE) {
+storage = "cache";
+}
+
+return storage;
+}

I personally don't like this kind of comparison done with flags. I'd
go for something like: if ((state_mask & SSS_SYSDB_BOTH_CACHE) != 0)
...
But this is a really minor and feel free to ignore it.


Patch 0002: SYSDB: Adding message about reason why cache changed

LGTM


Patch 0003: SYSDB: Adding wrappers for ldb_* operations

About the following parts of the patch:

On src/db/sysdb_ldb_wrapper.c

+#define ERR_FN_ENOMEM (-1 * ENOMEM)
+#define ERR_FN_ENOENT (-1 * ENOENT)

Why? I failed to understand why you're doing this here.

+if (print_ctx == NULL) {
+return -1;
+return ERR_FN_ENOMEM;
+}

I guess the return -1 is a leftover :-)

+if (print_ctx->ldif == NULL) {
+return -2;
+return ERR_FN_ENOENT;
+}

I guess the return -2 is also a leftover :-)

+if (ret < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE, "ldb_ldif_write() failed with
[%d][%s].\n",
+-1 * ret, sss_strerror(-1 * ret));
+goto done;
+}

And here again this dance multiplying by -1 that I don't understand
the reason :-\

+done:
+if (ldb_print_ctx != NULL && ldb_print_ctx->ldif != NULL) {
+talloc_free(ldb_print_ctx->ldif);
+}
+talloc_free(ldb_print_ctx);

AFAIU talloc_free can gracefully handle NULL. Considering that's the
case I'd just check for (if ldb_print_ctx != NULL)
talloc_free(ldb_print_ctx->ldif);
Considering it doesn't, we may have some issues on trying to free
(ldb_print_ctx)

On src/db/sysdb_ldb_wrapper.h:

+int sss_ldb_rename(struct ldb_context *ldb,
+   struct ldb_dn * olddn,
+   struct ldb_dn *newdn);

Just a really minor codying style change here, remove the extra space
between * and olddn: struct ldb_dn * olddn,  ->  struct ldb_dn *olddn,


Patch0004: SYSDB: ldb_add --> sss_ldb_add in sysdb
Patch0005: SYSDB: ldb_delete --> sss_ldb_delete in sysdb
Patch0006: SYSDB: ldb_modify --> sss_ldb_modify in sysdb
Patch0007: SYSDB: ldb_rename --> sss_ldb_rename in sysdb

LGTM


Best Regards,
--
Fabiano Fidêncio


Hello,


there is new patch set attached.
I replaced all ldb_* to new wrapper in whole code.


I wondered if my VM could push patches to our CI.
I will link the CI results after they finish.


OK, my VM is able to push to CI tests (I was afraid I have issue).
But result is not good:
http://sssd-ci.duckdns.org/logs/job/53/00/summary.html

--
Petr^4 Čech
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org