[SSSD] Re: [PATCH] IPA SUDO: download externalUser attribute

[Lukas Slebodnik]

On (08/03/16 18:21), Jakub Hrozek wrote:
>On Fri, Mar 04, 2016 at 02:01:54PM +0100, Pavel Březina wrote:
>> This allows configuration with id_provider = proxy
>> and sudo_provider = ipa when someone needs to fetch
>> rules for local users.
>
>> From a6f23fd5ab16d7903b8388d90eb3bb995c4426d0 Mon Sep 17 00:00:00 2001
>> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
>> Date: Tue, 1 Mar 2016 14:00:26 +0100
>> Subject: [PATCH 1/4] IPA SUDO: download externalUser attribute
>
>ACK
>
>tested with a combination of id_provider=proxy and sudo_provider=ipa
>
>I also think it's fine to not document the option, we didn't document
>the other sudo options either.
Do we have ticket for this?
If not it  would be good to test this use-case.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] LDAP: Do not print "null" in the DEBUG message

[Lukas Slebodnik]

On (08/03/16 18:14), Jakub Hrozek wrote:
>On Tue, Mar 08, 2016 at 09:34:29AM +0100, Lukas Slebodnik wrote:
>> On (25/02/16 11:06), Jakub Hrozek wrote:
>> >On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
>> >> On (24/02/16 16:43), Jakub Hrozek wrote:
>> >> >We don't know the group name at that point yet, so better not print
>> >> >"null" in the debug message..
>> >> 
>> >> >From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001
>> >> >From: Jakub Hrozek 
>> >> >Date: Tue, 26 Jan 2016 16:29:08 +0100
>> >> >Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
>> >> >
>> >> >---
>> >> > src/providers/ldap/sdap_async_groups.c | 3 +--
>> >> > 1 file changed, 1 insertion(+), 2 deletions(-)
>> >> >
>> >> >diff --git a/src/providers/ldap/sdap_async_groups.c 
>> >> >b/src/providers/ldap/sdap_async_groups.c
>> >> >index 
>> >> >5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c
>> >> > 100644
>> >> >--- a/src/providers/ldap/sdap_async_groups.c
>> >> >+++ b/src/providers/ldap/sdap_async_groups.c
>> >> >@@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
>> >> > goto done;
>> >> > }
>> >> > } else if (ret == ENOENT) {
>> >> >-DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group 
>> >> >[%s].\n",
>> >> >- group_name);
>> >> >+DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for 
>> >> >group.\n");
>> >> > sid_str = NULL;
>> >> > } else {
>> >> > DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: 
>> >> > [%s]\n",
>> >> 
>> >> 
>> >> Could we move " sdap_get_group_primary_name(..., &group_name)"
>> >> before "sdap_attrs_get_sid_str"?
>> >
>> >No, because we need to know the object domain first to format the name
>> >properly and in order to find the correct domain, we need to know the
>> >SID. See 970c5afb Maybe we should add a comment to that function, too,
>> >so that someone doesn't try to 'optimize' it in future..
>> Ahh, I missed that we might need different domain for
>> sdap_get_group_primary_name.
>> 
>> But there is a question. Do we really need this trace debug message?
>> IMHO it's confusing debug message with ldap provider.
>
>Dunno, we can remove it as well. I'm fine either way, but printing NULL
>is potentionally dangerous.
Then I vote for removing it. I cannot see a benfit of just logging
message "objectSID: not available for group". It does not say which group
and therefore it's confusing.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

[Simo Sorce]

On Tue, 2016-03-08 at 12:11 -0500, Simo Sorce wrote:
> On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
> > On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
> > > Fixing everything else commented before.
> > > 
> > > On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
> > > > And this is the question. The new code doesn't restore the flags, is
> > > > this an intentional change? Do you know why we restored the flags
> > > > previously?
> > > 
> > > Yes, it is an intentional change as restoring the flags was not needed.
> > > What happens if the function fails is that we are going to close the
> > > socket anyway, so what's the point of restoring flags (which means
> > > removing O_NONBLOCK in the end, somethign we never want to do as all
> > > sockets must be non-blocking in SSSD to avoig hangs.
> > > 
> > > 
> > > Fixed patches attacched.
> > > 
> > > Simo.
> > > 
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
> > 
> > > From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001
> > > From: Simo Sorce 
> > > Date: Wed, 2 Mar 2016 15:49:27 -0500
> > > Subject: [PATCH 2/3] Util: Set socket options and flags separately
> > > 
> > > Reorganize functions to set options and flags, all flags can be set at 
> > > once,
> > > and there is no need to keep old falgs around as nothing ever used that 
> > > for
> > > anything useful.
> > > 
> > > Related:
> > > https://fedorahosted.org/sssd/ticket/2968
> > 
> > This patch breaks failover for me. I can't really figure out why, the
> > code looks OK, though.
> > 
> > What I'm seeing is that the connect() call blocks. If I just add another
> > SETFD with O_NONBLOCK, everything works fine.
> > 
> > I really don't see the error though, can you? I can reliably reproduce
> > the error with:
> > - search for a user to establish connection
> > - pause the VM with the server
> > - search again
> 
> My bad, I see the error, proper patch coming soon.
> 
> Simo.

Please see attached patch, the code path had been compressed .. a lit'l
too much :)

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 6b3c7483793d53dad7b0a629a16d30add21659b5 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Wed, 2 Mar 2016 15:49:27 -0500
Subject: [PATCH 2/2] Util: Set socket options and flags separately

Reorganize functions to set options and flags, all flags can be set at once,
and there is no need to keep old falgs around as nothing ever used that for
anything useful.

Related:
https://fedorahosted.org/sssd/ticket/2968
---
 src/util/sss_sockets.c | 100 +
 1 file changed, 52 insertions(+), 48 deletions(-)

diff --git a/src/util/sss_sockets.c b/src/util/sss_sockets.c
index d0283af2d18ad42a13ec00faf7d6c4934eb696d0..4ca0029df1057107140590e29f2c5d7804f12a69 100644
--- a/src/util/sss_sockets.c
+++ b/src/util/sss_sockets.c
@@ -32,27 +32,52 @@
 #include "util/util.h"
 
 
-static errno_t set_fd_flags_and_opts(int fd)
+static errno_t set_fcntl_flags(int fd, int fd_flags, int fl_flags)
 {
 int ret;
-long flags;
+int cur_flags;
+
+ret = fcntl(fd, F_GETFD, 0);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+cur_flags = ret;
+
+ret = fcntl(fd, F_SETFD, cur_flags | fd_flags);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+
+ret = fcntl(fd, F_GETFL, 0);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+cur_flags = ret;
+
+ret = fcntl(fd, F_SETFL, cur_flags | fl_flags);
+if (ret == -1) {
+ret = errno;
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
+return ret;
+}
+
+return EOK;
+}
+
+static errno_t set_fd_common_opts(int fd)
+{
 int dummy = 1;
-
-flags = fcntl(fd, F_GETFD, 0);
-if (flags == -1) {
-ret = errno;
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "fcntl F_GETFD failed [%d][%s].\n", ret, strerror(ret));
-return ret;
-}
-
-flags = fcntl(fd, F_SETFD, flags| FD_CLOEXEC);
-if (flags == -1) {
-ret = errno;
-DEBUG(SSSDBG_CRIT_FAILURE,
-  "fcntl F_SETFD failed [%d][%s].\n", ret, strerror(ret));
-return ret;
-}
+int ret;
 
 /* SO_KEEPALIVE and TCP_NODELAY are set by OpenLDAP client libraries but
  * failures are ignored.*/
@@ -77,7 +102,6 @@ static errno_t set_fd_flags_and_opts(int fd)
 
 
 struct sssd_async_connect_state {
-long old_flags;
 struct tevent_fd *fde;
 int fd;
 socklen_t addr_len;
@@ -96,15 +120,7 @@ struct tevent_req *sssd_async_connect_send(TALLOC_CTX *mem_ctx,
 {
 struct teve

[SSSD] Re: [PATCH] IPA SUDO: download externalUser attribute

[Jakub Hrozek]

On Fri, Mar 04, 2016 at 02:01:54PM +0100, Pavel Březina wrote:
> This allows configuration with id_provider = proxy
> and sudo_provider = ipa when someone needs to fetch
> rules for local users.

> From a6f23fd5ab16d7903b8388d90eb3bb995c4426d0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
> Date: Tue, 1 Mar 2016 14:00:26 +0100
> Subject: [PATCH 1/4] IPA SUDO: download externalUser attribute

ACK

tested with a combination of id_provider=proxy and sudo_provider=ipa

I also think it's fine to not document the option, we didn't document
the other sudo options either.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] LDAP: Do not print "null" in the DEBUG message

[Jakub Hrozek]

On Tue, Mar 08, 2016 at 09:34:29AM +0100, Lukas Slebodnik wrote:
> On (25/02/16 11:06), Jakub Hrozek wrote:
> >On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
> >> On (24/02/16 16:43), Jakub Hrozek wrote:
> >> >We don't know the group name at that point yet, so better not print
> >> >"null" in the debug message..
> >> 
> >> >From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001
> >> >From: Jakub Hrozek 
> >> >Date: Tue, 26 Jan 2016 16:29:08 +0100
> >> >Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
> >> >
> >> >---
> >> > src/providers/ldap/sdap_async_groups.c | 3 +--
> >> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >> >
> >> >diff --git a/src/providers/ldap/sdap_async_groups.c 
> >> >b/src/providers/ldap/sdap_async_groups.c
> >> >index 
> >> >5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c
> >> > 100644
> >> >--- a/src/providers/ldap/sdap_async_groups.c
> >> >+++ b/src/providers/ldap/sdap_async_groups.c
> >> >@@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
> >> > goto done;
> >> > }
> >> > } else if (ret == ENOENT) {
> >> >-DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group 
> >> >[%s].\n",
> >> >- group_name);
> >> >+DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n");
> >> > sid_str = NULL;
> >> > } else {
> >> > DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: 
> >> > [%s]\n",
> >> 
> >> 
> >> Could we move " sdap_get_group_primary_name(..., &group_name)"
> >> before "sdap_attrs_get_sid_str"?
> >
> >No, because we need to know the object domain first to format the name
> >properly and in order to find the correct domain, we need to know the
> >SID. See 970c5afb Maybe we should add a comment to that function, too,
> >so that someone doesn't try to 'optimize' it in future..
> Ahh, I missed that we might need different domain for
> sdap_get_group_primary_name.
> 
> But there is a question. Do we really need this trace debug message?
> IMHO it's confusing debug message with ldap provider.

Dunno, we can remove it as well. I'm fine either way, but printing NULL
is potentionally dangerous.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

[Simo Sorce]

On Tue, 2016-03-08 at 17:48 +0100, Jakub Hrozek wrote:
> On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
> > Fixing everything else commented before.
> > 
> > On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
> > > And this is the question. The new code doesn't restore the flags, is
> > > this an intentional change? Do you know why we restored the flags
> > > previously?
> > 
> > Yes, it is an intentional change as restoring the flags was not needed.
> > What happens if the function fails is that we are going to close the
> > socket anyway, so what's the point of restoring flags (which means
> > removing O_NONBLOCK in the end, somethign we never want to do as all
> > sockets must be non-blocking in SSSD to avoig hangs.
> > 
> > 
> > Fixed patches attacched.
> > 
> > Simo.
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> > From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Wed, 2 Mar 2016 15:49:27 -0500
> > Subject: [PATCH 2/3] Util: Set socket options and flags separately
> > 
> > Reorganize functions to set options and flags, all flags can be set at once,
> > and there is no need to keep old falgs around as nothing ever used that for
> > anything useful.
> > 
> > Related:
> > https://fedorahosted.org/sssd/ticket/2968
> 
> This patch breaks failover for me. I can't really figure out why, the
> code looks OK, though.
> 
> What I'm seeing is that the connect() call blocks. If I just add another
> SETFD with O_NONBLOCK, everything works fine.
> 
> I really don't see the error though, can you? I can reliably reproduce
> the error with:
> - search for a user to establish connection
> - pause the VM with the server
> - search again

My bad, I see the error, proper patch coming soon.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

[Petr Cech]

On 03/08/2016 05:09 PM, Pavel Reichl wrote:

Hello Petr,

I just run through the code and I have some code style suggestions, feel
free to disagree :-).


I will address some suggestions tomorrow. I am doing tests for 
sysdb_sudo_rules now. Thanks for comments.


Petr



On 03/08/2016 01:11 PM, Petr Cech wrote:

0001-SYSDB-Add-new-funtions-into-sysdb_sudo.patch

...

+
+errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
+struct sss_domain_info *domain,
+const char *sub_filter,
+const char **attrs,
+size_t *msgs_count,
+struct ldb_message ***msgs)


if count and msgs are output parameters then please add _ as a prefix.


+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *filter;
+int ret;

errno_t might be better here.

+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}


I think we prefere != NULL form - but this is not important.


+
+dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
SYSDB_TMPL_CUSTOM_SUBTREE,
+SUDORULE_SUBDIR, domain->name);
+if (!dn) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
+ret = ENOMEM;
+goto done;
+}
+
+if (sub_filter != NULL) {
+filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
+ SUDO_ALL_FILTER, sub_filter);
+} else {
+filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
+}
+if (!filter) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
+ret = ENOMEM;
+goto done;
+}
+
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Search sudo rules with filter: %s\n", filter);
+
+ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
+ LDB_SCOPE_SUBTREE, filter, attrs,
+ msgs_count, msgs);


You could use tmp_ctx context here, it would make code a bit longer
because you would have to steal but also a bit more mem leak resistant.

+
+if (ret != EOK) {
+if (ret == ENOENT) {
+DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
+}
+else if (ret) {
+DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
strerror(ret));
+}
+}


You can save indentation.

if (ret == ENOENT) {
...
} else if (ret != EOK) {
...
}

   Please use sss_strerror instead of strerror().



+
+done:
+talloc_zfree(tmp_ctx);
+return ret;
+}
+





0002-TOOL-Invalidation-of-sudo-rules-at-sss_cache.patch


 From e0143502fce82d353955352d8717202346fed6b6 Mon Sep 17 00:00:00 2001
From: Petr Cech
Date: Tue, 23 Feb 2016 10:11:15 -0500
Subject: [PATCH 2/2] TOOL: Invalidation of sudo rules at sss_cache

This patch adds new functionality to sss_cach for invalidation of given
sudo rule or all sudo rules.

Resolves:
https://fedorahosted.org/sssd/ticket/2081
---

...

  }
@@ -577,6 +611,7 @@ errno_t init_context(int argc, const char *argv[],
struct cache_tool_ctx **tctx)
  char *service = NULL;
  char *map = NULL;
  char *ssh_host = NULL;
+char *sudo_rule = NULL;
  char *domain = NULL;
  int debug = SSSDBG_DEFAULT;
  errno_t ret = EOK;
@@ -616,6 +651,12 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
  { "ssh-hosts", 'H', POPT_ARG_NONE, NULL, 'h',
  _("Invalidate all SSH hosts"), NULL },
  #endif /* BUILD_SSH */
+#ifdef BUILD_SUDO
+{ "sudo-rule", 'r', POPT_ARG_STRING, &sudo_rule, 0,
+_("Invalidate particular sudo rule"), NULL },
+{ "sudo-rules", 'R', POPT_ARG_NONE, NULL, 'r',
+_("Invalidate all cached sudo rules"), NULL },
+#endif /* BUILD_SUDO */
  { "domain", 'd', POPT_ARG_STRING, &domain, 0,
  _("Only invalidate entries from a particular domain"),
NULL },
  POPT_TABLEEND
@@ -650,8 +691,12 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
  case 'h':
  idb |= INVALIDATE_SSH_HOSTS;
  break;
+case 'r':
+idb |= INVALIDATE_SUDO_RULES;
+break;
  case 'e':
  idb = INVALIDATE_EVERYTHING;
+idb |= INVALIDATE_SUDO_RULES;
  break;
  }
  }
@@ -664,7 +709,7 @@ errno_t init_context(int argc, const char *argv[],
struct cache_tool_ctx **tctx)
  }

  if (idb == INVALIDATE_NONE && !user && !group &&
-!netgroup && !service && !map && !ssh_host) {
+!netgroup && !service && !map && !ssh_host && !sudo_rule) {
  BAD_POPT_PARAMS(pc,
  _("Please select at least one object to invalidate\n"),
  ret, fini);
@@ -729,18 +774,28 @@ errno_t init_context(int argc, const char
*argv[], struct cache_tool_ctx **tctx)
  ctx->upda

[SSSD] [PATCH] pam_sss: reorder pam_message array

[Sumit Bose]

Hi,

This patch fixes a 2FA issues observed with sudo. See commit message for
details.

bye,
Sumit
From 2c38adad7b527aceb4f9cb41c7d7b4c66d4580c9 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 7 Mar 2016 17:07:16 +0100
Subject: [PATCH] pam_sss: reorder pam_message array

There are different expectations about how the pam_message array is
organized, details can be found in the pam_start man page. E.g. sudo was
not able to handle the Linux-PAM style but expected the Solaris PAM
style. With this patch both styles should work as expected.

Resolves https://fedorahosted.org/sssd/ticket/2971
---
 src/sss_client/pam_sss.c | 28 ++--
 1 file changed, 10 insertions(+), 18 deletions(-)

diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
index 
b4f7efe49017870186f1cd9e91603033a5354770..f16f12c4579f603d40fe31abb1ab7a3a2fafd4cd
 100644
--- a/src/sss_client/pam_sss.c
+++ b/src/sss_client/pam_sss.c
@@ -1260,8 +1260,7 @@ static int prompt_2fa(pam_handle_t *pamh, struct 
pam_items *pi,
 int ret;
 const struct pam_conv *conv;
 const struct pam_message *mesg[2] = { NULL, NULL };
-struct pam_message *m1;
-struct pam_message *m2;
+struct pam_message *m;
 struct pam_response *resp = NULL;
 size_t needed_size;
 
@@ -1270,29 +1269,22 @@ static int prompt_2fa(pam_handle_t *pamh, struct 
pam_items *pi,
 return ret;
 }
 
-m1 = malloc(sizeof(struct pam_message));
-if (m1 == NULL) {
+m = malloc(2 * sizeof(struct pam_message));
+if (m == NULL) {
 D(("Malloc failed."));
 return PAM_SYSTEM_ERR;
 }
 
-m2 = malloc(sizeof(struct pam_message));
-if (m2 == NULL) {
-D(("Malloc failed."));
-free(m1);
-return PAM_SYSTEM_ERR;
-}
-m1->msg_style = PAM_PROMPT_ECHO_OFF;
-m1->msg = prompt_fa1;
-m2->msg_style = PAM_PROMPT_ECHO_OFF;
-m2->msg = prompt_fa2;
+m[0].msg_style = PAM_PROMPT_ECHO_OFF;
+m[0].msg = prompt_fa1;
+m[1].msg_style = PAM_PROMPT_ECHO_OFF;
+m[1].msg = prompt_fa2;
 
-mesg[0] = (const struct pam_message *) m1;
-mesg[1] = (const struct pam_message *) m2;
+mesg[0] = (const struct pam_message *) m;
+mesg[1] = & (( *mesg )[1]);
 
 ret = conv->conv(2, mesg, &resp, conv->appdata_ptr);
-free(m1);
-free(m2);
+free(m);
 if (ret != PAM_SUCCESS) {
 D(("Conversation failure: %s.", pam_strerror(pamh, ret)));
 return ret;
-- 
2.1.0

___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] make async connect generic

[Jakub Hrozek]

On Tue, Mar 08, 2016 at 10:18:46AM -0500, Simo Sorce wrote:
> Fixing everything else commented before.
> 
> On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
> > And this is the question. The new code doesn't restore the flags, is
> > this an intentional change? Do you know why we restored the flags
> > previously?
> 
> Yes, it is an intentional change as restoring the flags was not needed.
> What happens if the function fails is that we are going to close the
> socket anyway, so what's the point of restoring flags (which means
> removing O_NONBLOCK in the end, somethign we never want to do as all
> sockets must be non-blocking in SSSD to avoig hangs.
> 
> 
> Fixed patches attacched.
> 
> Simo.
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> From 5551dc918890cf445cadb1b39c42d9a6dffa8bb8 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Wed, 2 Mar 2016 15:49:27 -0500
> Subject: [PATCH 2/3] Util: Set socket options and flags separately
> 
> Reorganize functions to set options and flags, all flags can be set at once,
> and there is no need to keep old falgs around as nothing ever used that for
> anything useful.
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2968

This patch breaks failover for me. I can't really figure out why, the
code looks OK, though.

What I'm seeing is that the connect() call blocks. If I just add another
SETFD with O_NONBLOCK, everything works fine.

I really don't see the error though, can you? I can reliably reproduce
the error with:
- search for a user to establish connection
- pause the VM with the server
- search again
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

[Pavel Reichl]

Hello Petr,

I just run through the code and I have some code style suggestions, feel free 
to disagree :-).

On 03/08/2016 01:11 PM, Petr Cech wrote:

0001-SYSDB-Add-new-funtions-into-sysdb_sudo.patch

...

+
+errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
+struct sss_domain_info *domain,
+const char *sub_filter,
+const char **attrs,
+size_t *msgs_count,
+struct ldb_message ***msgs)


if count and msgs are output parameters then please add _ as a prefix.


+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *filter;
+int ret;

errno_t might be better here.

+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}


I think we prefere != NULL form - but this is not important.


+
+dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb, SYSDB_TMPL_CUSTOM_SUBTREE,
+SUDORULE_SUBDIR, domain->name);
+if (!dn) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
+ret = ENOMEM;
+goto done;
+}
+
+if (sub_filter != NULL) {
+filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
+ SUDO_ALL_FILTER, sub_filter);
+} else {
+filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
+}
+if (!filter) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
+ret = ENOMEM;
+goto done;
+}
+
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Search sudo rules with filter: %s\n", filter);
+
+ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
+ LDB_SCOPE_SUBTREE, filter, attrs,
+ msgs_count, msgs);


You could use tmp_ctx context here, it would make code a bit longer because you 
would have to steal but also a bit more mem leak resistant.

+
+if (ret != EOK) {
+if (ret == ENOENT) {
+DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
+}
+else if (ret) {
+DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret, 
strerror(ret));
+}
+}


   You can save indentation.

   if (ret == ENOENT) {
   ...
   } else if (ret != EOK) {
   ...
   }

  Please use sss_strerror instead of strerror().



+
+done:
+talloc_zfree(tmp_ctx);
+return ret;
+}
+





0002-TOOL-Invalidation-of-sudo-rules-at-sss_cache.patch


 From e0143502fce82d353955352d8717202346fed6b6 Mon Sep 17 00:00:00 2001
From: Petr Cech
Date: Tue, 23 Feb 2016 10:11:15 -0500
Subject: [PATCH 2/2] TOOL: Invalidation of sudo rules at sss_cache

This patch adds new functionality to sss_cach for invalidation of given
sudo rule or all sudo rules.

Resolves:
https://fedorahosted.org/sssd/ticket/2081
---

...

  }
@@ -577,6 +611,7 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  char *service = NULL;
  char *map = NULL;
  char *ssh_host = NULL;
+char *sudo_rule = NULL;
  char *domain = NULL;
  int debug = SSSDBG_DEFAULT;
  errno_t ret = EOK;
@@ -616,6 +651,12 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  { "ssh-hosts", 'H', POPT_ARG_NONE, NULL, 'h',
  _("Invalidate all SSH hosts"), NULL },
  #endif /* BUILD_SSH */
+#ifdef BUILD_SUDO
+{ "sudo-rule", 'r', POPT_ARG_STRING, &sudo_rule, 0,
+_("Invalidate particular sudo rule"), NULL },
+{ "sudo-rules", 'R', POPT_ARG_NONE, NULL, 'r',
+_("Invalidate all cached sudo rules"), NULL },
+#endif /* BUILD_SUDO */
  { "domain", 'd', POPT_ARG_STRING, &domain, 0,
  _("Only invalidate entries from a particular domain"), NULL },
  POPT_TABLEEND
@@ -650,8 +691,12 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  case 'h':
  idb |= INVALIDATE_SSH_HOSTS;
  break;
+case 'r':
+idb |= INVALIDATE_SUDO_RULES;
+break;
  case 'e':
  idb = INVALIDATE_EVERYTHING;
+idb |= INVALIDATE_SUDO_RULES;
  break;
  }
  }
@@ -664,7 +709,7 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  }

  if (idb == INVALIDATE_NONE && !user && !group &&
-!netgroup && !service && !map && !ssh_host) {
+!netgroup && !service && !map && !ssh_host && !sudo_rule) {
  BAD_POPT_PARAMS(pc,
  _("Please select at least one object to invalidate\n"),
  ret, fini);
@@ -729,18 +774,28 @@ errno_t init_context(int argc, const char *argv[], struct 
cache_tool_ctx **tctx)
  ctx->update_ssh_host_filter = true;
  }

+if (idb & INVALIDATE_SUDO_RULES) {
+ctx->sudo_rule_filter = talloc_asprintf(ctx, "(%s=*)", SYSDB_NAME);
+

[SSSD] Re: [PATCH] make async connect generic

[Simo Sorce]

Fixing everything else commented before.

On Sat, 2016-03-05 at 15:31 +0100, Jakub Hrozek wrote:
> And this is the question. The new code doesn't restore the flags, is
> this an intentional change? Do you know why we restored the flags
> previously?

Yes, it is an intentional change as restoring the flags was not needed.
What happens if the function fails is that we are going to close the
socket anyway, so what's the point of restoring flags (which means
removing O_NONBLOCK in the end, somethign we never want to do as all
sockets must be non-blocking in SSSD to avoig hangs.


Fixed patches attacched.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York
From 9b8fd65b6eb242936a5d0734eb05e3c09d3268a5 Mon Sep 17 00:00:00 2001
From: Simo Sorce 
Date: Wed, 2 Mar 2016 14:33:38 -0500
Subject: [PATCH 1/3] Util: Move socket setup in a common utility file

Other components may need to connect sockets, the code here is generic enough
that with minimal modifications can be used for non-ldap connections too.

So create a sss_sockets.c/h utility file with all the non-ldap specific socket
setup functions and make them available for other uses.

Resolves:
https://fedorahosted.org/sssd/ticket/2968
---
 Makefile.am|   5 +
 src/util/sss_ldap.c| 258 ++-
 src/util/sss_sockets.c | 356 +
 src/util/sss_sockets.h |  39 ++
 4 files changed, 413 insertions(+), 245 deletions(-)
 create mode 100644 src/util/sss_sockets.c
 create mode 100644 src/util/sss_sockets.h

diff --git a/Makefile.am b/Makefile.am
index 4e4f38a5eaf5bfa2cfafa88b9b32848e6c27131b..d6eb0fc732a3b566dba91952bd6598a31f8d8d3e 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -541,6 +541,7 @@ dist_noinst_HEADERS = \
 src/util/sss_python.h \
 src/util/sss_krb5.h \
 src/util/sss_selinux.h \
+src/util/sss_sockets.h \
 src/util/sss_utf8.h \
 src/util/sss_ssh.h \
 src/util/sss_ini.h \
@@ -1663,6 +1664,7 @@ ipa_ldap_opt_tests_SOURCES = \
 src/providers/ad/ad_opts.c \
 src/providers/ipa/ipa_opts.c \
 src/providers/krb5/krb5_opts.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 src/tests/ipa_ldap_opt-tests.c
 ipa_ldap_opt_tests_CFLAGS = \
@@ -1877,6 +1879,7 @@ TEST_MOCK_RESP_OBJ = \
  src/responder/common/responder_cache_req.c
 
 TEST_MOCK_PROVIDER_OBJ = \
+ src/util/sss_sockets.c \
  src/util/sss_ldap.c \
  src/providers/data_provider_opts.c \
  src/providers/ldap/ldap_opts.c \
@@ -2264,6 +2267,7 @@ sdap_tests_SOURCES = \
 src/providers/ldap/sdap_range.c \
 src/providers/ldap/ldap_opts.c \
 src/providers/ipa/ipa_opts.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 src/tests/cmocka/test_sdap.c \
 $(NULL)
@@ -2879,6 +2883,7 @@ libsss_ldap_common_la_SOURCES = \
 src/providers/ldap/sdap.c \
 src/providers/ipa/ipa_dn.c \
 src/util/user_info_msg.c \
+src/util/sss_sockets.c \
 src/util/sss_ldap.c \
 $(NULL)
 libsss_ldap_common_la_CFLAGS = \
diff --git a/src/util/sss_ldap.c b/src/util/sss_ldap.c
index c440d5445133c8b31f662fc69b35e2296c3f1702..7fdaadb5cebf5d3e7fe7f8fb1780f0db3dbcae4a 100644
--- a/src/util/sss_ldap.c
+++ b/src/util/sss_ldap.c
@@ -17,18 +17,13 @@
 You should have received a copy of the GNU General Public License
 along with this program.  If not, see .
 */
-#include 
-#include 
-#include 
-#include 
-#include 
-#include 
 
 #include "config.h"
-
-#include "providers/ldap/sdap.h"
-#include "util/sss_ldap.h"
 #include "util/util.h"
+#include "util/sss_sockets.h"
+#include "util/sss_ldap.h"
+
+#include "providers/ldap/sdap.h"
 
 const char* sss_ldap_err2string(int err)
 {
@@ -103,183 +98,6 @@ int sss_ldap_control_create(const char *oid, int iscritical,
 }
 
 #ifdef HAVE_LDAP_INIT_FD
-struct sdap_async_sys_connect_state {
-long old_flags;
-struct tevent_fd *fde;
-int fd;
-socklen_t addr_len;
-struct sockaddr_storage addr;
-};
-
-static void sdap_async_sys_connect_done(struct tevent_context *ev,
-struct tevent_fd *fde, uint16_t flags,
-void *priv);
-
-static struct tevent_req *sdap_async_sys_connect_send(TALLOC_CTX *mem_ctx,
-struct tevent_context *ev,
-int fd,
-const struct sockaddr *addr,
-socklen_t addr_len)
-{
-struct tevent_req *req;
-struct sdap_async_sys_connect_state *state;
-long flags;
-int ret;
-int fret;
-
-flags = fcntl(fd, F_GETFL, 0);
-if (flags == -1) {
-DEBUG(SSSDBG_CRIT_FAILURE, "fcntl F_GETFL failed.\n");
-return NULL;
-}
-
-req = tevent_req_create(mem_ctx, &state,
-struct sdap_async_sys_connect_state);
-if (req == 

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

[Petr Cech]

On 03/07/2016 01:53 PM, Pavel Březina wrote:

On 03/07/2016 01:11 PM, Pavel Březina wrote:

On 03/02/2016 05:04 PM, Petr Cech wrote:

Hi all,

attached two patches resolve [1]. This ticket has design page [2].

In my opinion it could be fine to have tests on sysdb_sudo. I have
started write some, but there were troubles with memory leak. Maybe I
haven't understood necessary logic properly.

However I could continue with tests or create new ticket for it.

And little question. Is the name of sudo rule case sensitive? If yes, I
have to do one little change.


I think we are good if we use domain settings so no change is needed.



[1] https://fedorahosted.org/sssd/ticket/2081
[2]
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRulesInvalidate

Regards


Hi,
see comments inline.


errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *sub_filter,
const char **attrs,
size_t *msgs_count,
struct ldb_message ***msgs)
{
TALLOC_CTX *tmp_ctx;
struct ldb_dn *dn;
char *filter;
int ret;

tmp_ctx = talloc_new(NULL);
if (!tmp_ctx) {
return ENOMEM;
}

dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
SYSDB_TMPL_CUSTOM_SUBTREE,
SUDORULE_SUBDIR, domain->name);
if (!dn) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
ret = ENOMEM;
goto fail;
}

filter = talloc_asprintf(tmp_ctx, "(&%s%s)", SUDO_ALL_FILTER,
sub_filter);


You also need to add case where sub_filter is NULL.


if (!filter) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
ret = ENOMEM;
goto fail;
}

DEBUG(SSSDBG_TRACE_INTERNAL,
  "Search services with filter: %s\n", filter);


sudo rule, not "services"



ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
 LDB_SCOPE_SUBTREE, filter, attrs,
 msgs_count, msgs);
if (ret) {
goto fail;
}

talloc_zfree(tmp_ctx);
return EOK;

fail:
if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
}
else if (ret) {
DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
strerror(ret));
}
talloc_zfree(tmp_ctx);


There is already good debug message per dn and filter case. Move this to
sysdb_search_entry and use done scheme instead of fail, please.


return ret;
}


Also atm -E won't trigger invalidation of sudo rules which I think is
also desired. Instead of modifying the current #ifdefs thingy, I'd
suggest using |= operator in init_context. I.e.:

 case 'e':
 idb = INVALIDATE_EVERYTHING;
 idb |= SUDO...
 break;


There are new fixed patch set. We discussed offline and all comments are 
addressed.


--
Petr^4 Čech
>From 107d729c833f8dc54d0b1ebfdf1b48bcf591af9f Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Wed, 24 Feb 2016 09:12:41 -0500
Subject: [PATCH 1/2] SYSDB: Add new funtions into sysdb_sudo

This patch adds two new functions into public
API of sysdb_sudo:
* sysdb_search_sudo_rules
* sysdb_set_sudo_rule_attr

Resolves:
https://fedorahosted.org/sssd/ticket/2081
---
 src/db/sysdb_sudo.c | 94 +
 src/db/sysdb_sudo.h | 15 +
 2 files changed, 109 insertions(+)

diff --git a/src/db/sysdb_sudo.c b/src/db/sysdb_sudo.c
index 76116abacb20219f0c1dcdde755e8268e10fd293..69ca9db27d7e456664bcf76da3e0ec2ac59bddb6 100644
--- a/src/db/sysdb_sudo.c
+++ b/src/db/sysdb_sudo.c
@@ -889,3 +889,97 @@ done:
 
 return ret;
 }
+
+errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
+struct sss_domain_info *domain,
+const char *sub_filter,
+const char **attrs,
+size_t *msgs_count,
+struct ldb_message ***msgs)
+{
+TALLOC_CTX *tmp_ctx;
+struct ldb_dn *dn;
+char *filter;
+int ret;
+
+tmp_ctx = talloc_new(NULL);
+if (!tmp_ctx) {
+return ENOMEM;
+}
+
+dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb, SYSDB_TMPL_CUSTOM_SUBTREE,
+SUDORULE_SUBDIR, domain->name);
+if (!dn) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
+ret = ENOMEM;
+goto done;
+}
+
+if (sub_filter != NULL) {
+filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
+ SUDO_ALL_FILTER, sub_filter);
+} else {
+filter = talloc_asprintf(tmp_ctx, "(&%s)", SUDO_ALL_FILTER);
+}
+if (!filter) {
+DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
+ret = ENOMEM;
+goto done;
+}
+
+DEBUG(SSSDBG_TRACE_INTERNAL,
+  "Search sudo rules with filt

[SSSD] Re: [PATCH SET] TOOL: Invalidation of sudo rules at sss_cache

[Pavel Březina]

On 03/07/2016 03:26 PM, Petr Cech wrote:

On 03/07/2016 01:11 PM, Pavel Březina wrote:

Hi,
see comments inline.


errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *sub_filter,
const char **attrs,
size_t *msgs_count,
struct ldb_message ***msgs)
{
TALLOC_CTX *tmp_ctx;
struct ldb_dn *dn;
char *filter;
int ret;

tmp_ctx = talloc_new(NULL);
if (!tmp_ctx) {
return ENOMEM;
}

dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
SYSDB_TMPL_CUSTOM_SUBTREE,
SUDORULE_SUBDIR, domain->name);
if (!dn) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
ret = ENOMEM;
goto fail;
}

filter = talloc_asprintf(tmp_ctx, "(&%s%s)", SUDO_ALL_FILTER,
sub_filter);


You also need to add case where sub_filter is NULL.


Thank you Pavel for comments. I have little question:

If sub_filter equal to NULL is given then
filter = (&(objectClass=sudoRule))


No it's not. Passing NULL pointer to asprintf results in undefined 
behaviour. The filter will be either (&(objectClass=sudoRule)(null)) or 
it will crash depending on implementation.


The desire is to have (objectClass=sudoRule) filter in sub_filter is NULL.


and that means that we can find all the rules.

Is it wrong?




if (!filter) {
DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
ret = ENOMEM;
goto fail;
}

DEBUG(SSSDBG_TRACE_INTERNAL,
  "Search sudo rules with filter: %s\n", filter);



___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] NSS: Move a DEBUG message so that it's less confusing

[Lukas Slebodnik]

On (04/03/16 12:14), Pavel Březina wrote:
>On 03/04/2016 12:08 PM, Jakub Hrozek wrote:
>>On Fri, Mar 04, 2016 at 11:56:03AM +0100, Pavel Reichl wrote:
>>>
>>>
>>>On 03/04/2016 11:46 AM, Jakub Hrozek wrote:
Hi,

the attached patch would hopefully make analyzing of NSS logs files in a
multi-domain scenario (typically a trust setup) less confusing.
Previously, we would print "Domain not found" even if the ID was
actually found..

>>>
>>>Jakub, you might have attached by accident po/ca.po.
>>
>>Yeah, we po files will be regenerated only after the next release..
>Ack.

master:
* 37b467244f48869ef3690c908033da8ba37635c0

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] TOOL: Invalidation of sudo rules at sss_cache

[Petr Cech]

On 03/07/2016 03:57 PM, Petr Cech wrote:

On 03/07/2016 01:53 PM, Pavel Březina wrote:

On 03/07/2016 01:11 PM, Pavel Březina wrote:

On 03/02/2016 05:04 PM, Petr Cech wrote:




ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
 LDB_SCOPE_SUBTREE, filter, attrs,
 msgs_count, msgs);
if (ret) {
goto fail;
}

talloc_zfree(tmp_ctx);
return EOK;

fail:
if (ret == ENOENT) {
DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
}
else if (ret) {
DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
strerror(ret));
}
talloc_zfree(tmp_ctx);


There is already good debug message per dn and filter case. Move this to
sysdb_search_entry and use done scheme instead of fail, please.



Whole fail block is redundant, you've right. Addressed.



I would like to remove redundant error messages for other calls of 
sysdb_search_entry. Unfortunately, I found that I had to miss and look 
to other function. Therefore, I have to still fix the patch.


I am sorry.

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

[SSSD] Re: [PATCH] LDAP: Do not print "null" in the DEBUG message

[Lukas Slebodnik]

On (25/02/16 11:06), Jakub Hrozek wrote:
>On Wed, Feb 24, 2016 at 06:05:11PM +0100, Lukas Slebodnik wrote:
>> On (24/02/16 16:43), Jakub Hrozek wrote:
>> >We don't know the group name at that point yet, so better not print
>> >"null" in the debug message..
>> 
>> >From ffdc00755a9fbaeb54f781956a0025719e532b11 Mon Sep 17 00:00:00 2001
>> >From: Jakub Hrozek 
>> >Date: Tue, 26 Jan 2016 16:29:08 +0100
>> >Subject: [PATCH] LDAP: Do not print "null" in the DEBUG message
>> >
>> >---
>> > src/providers/ldap/sdap_async_groups.c | 3 +--
>> > 1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> >diff --git a/src/providers/ldap/sdap_async_groups.c 
>> >b/src/providers/ldap/sdap_async_groups.c
>> >index 
>> >5bb267fa5331c73cb6b9b86ab21f25fcd3b0df4f..b972863a17e543361c5544382cf8ebbdde91672c
>> > 100644
>> >--- a/src/providers/ldap/sdap_async_groups.c
>> >+++ b/src/providers/ldap/sdap_async_groups.c
>> >@@ -538,8 +538,7 @@ static int sdap_save_group(TALLOC_CTX *memctx,
>> > goto done;
>> > }
>> > } else if (ret == ENOENT) {
>> >-DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group 
>> >[%s].\n",
>> >- group_name);
>> >+DEBUG(SSSDBG_TRACE_ALL, "objectSID: not available for group.\n");
>> > sid_str = NULL;
>> > } else {
>> > DEBUG(SSSDBG_MINOR_FAILURE, "Could not identify objectSID: [%s]\n",
>> 
>> 
>> Could we move " sdap_get_group_primary_name(..., &group_name)"
>> before "sdap_attrs_get_sid_str"?
>
>No, because we need to know the object domain first to format the name
>properly and in order to find the correct domain, we need to know the
>SID. See 970c5afb Maybe we should add a comment to that function, too,
>so that someone doesn't try to 'optimize' it in future..
Ahh, I missed that we might need different domain for
sdap_get_group_primary_name.

But there is a question. Do we really need this trace debug message?
IMHO it's confusing debug message with ldap provider.

LS
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org