[SSSD] Re: [PATCH] Make responder connectin code more generic

[Simo Sorce]

On Tue, 2016-01-12 at 14:04 +0100, Jakub Hrozek wrote:
> On Mon, Jan 11, 2016 at 01:39:33PM -0500, Simo Sorce wrote:
> > The following 2 patches change the connection setup code to be more
> > flexible.
> > 
> > They are the groundwork to add a new secrets[1] responder that uses a
> > REST API over a unix socket and therefore requires a different protocol
> > handler.
> > 
> > The first patch move some protocol and state data into opaque pointers
> > so that responders can arbitrarily set them.
> > Ticket: https://fedorahosted.org/sssd/ticket/2918
> > 
> > The second patch adds the ability to use socket activation for
> > responders. Although this is not currently wired in for use in
> > currently.
> > It is related to: https://fedorahosted.org/sssd/ticket/2243
> > 
> > These patches are being worked on on my secsrv branch here:
> > https://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=secsrv
> > 
> > 
> > [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SecretsService
> > 
> > -- 
> > Simo Sorce * Red Hat, Inc * New York
> 
> > From e82a6c4fc5bd0c6dc264961c5b0fa4f97372e984 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Fri, 8 Jan 2016 17:51:06 -0500
> > Subject: [PATCH] Responders: Make the client context more generic
> > 
> > This is useufl to allow reusing the responder code with other protocols.
> > Store protocol data and responder state data behind opaque pointers and
> > use tallog_get_type to check they are of the right type.
> > 
> > This also allows to store per responder state_ctx so that, for example,
> > the autofs responder does not have to carry useless variables used only
> > by the nss responder.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2918
> 
> There are some new Coverity warnings with this code:
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:756: var_tested_neg: 
> Assigning: "rctx->lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns: 
> "rctx->lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:745:5: 
> neg_sink_parm_call: Passing "rctx->lfd" to "close", which cannot accept a 
> negative number.
> #  788|   #endif
> #  789|   
> #  790|-> ret = set_unix_socket(rctx, conn_setup);
> #  791|   if (ret != EOK) {
> #  792|   DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing 
> sockets\n");
> 
> Error: NEGATIVE_RETURNS (CWE-394):
> sssd-1.13.90/src/responder/common/responder_common.c:757: var_tested_neg: 
> Assigning: "rctx->priv_lfd" = a negative value.
> sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns: 
> "rctx->priv_lfd" is passed to a parameter that cannot be negative.
> sssd-1.13.90/src/responder/common/responder_common.c:746:5: 
> neg_sink_parm_call: Passing "rctx->priv_lfd" to "close", which cannot accept 
> a negative number.
> #  788|   #endif
> #  789|   
> #  790|-> ret = set_unix_socket(rctx, conn_setup);
> #  791|   if (ret != EOK) {
> #  792|   DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing 
> sockets\n");
> 
> But in general it looks good to me. I need to run more tests, though..

I'll check these out.

> > From 796ad20628994e671b9af903ed396eb8103d18c4 Mon Sep 17 00:00:00 2001
> > From: Simo Sorce 
> > Date: Thu, 7 Jan 2016 10:17:38 -0500
> > Subject: [PATCH 2/2] Responders: Add support for socket activation
> > 
> > Add helper that uses systemd socket activation if available to accept a
> > pre-listining socket at startup.
> > 
> > Related:
> > https://fedorahosted.org/sssd/ticket/2913
> 
> [...]
> 
> > +ret = sd_listen_fds(1);
> > +if (ret < 0) {
> > +DEBUG(SSSDBG_MINOR_FAILURE,
> > +  "Unexpected error probing for active sockets. "
> > +  "Will proceed with no sockets. [Error %d (%s)]\n",
> > +  -ret, sss_strerror(-ret));
> > +} else if (ret > numfds) {
> > +DEBUG(SSSDBG_FATAL_FAILURE,
> > +  "Too many activated sockets have been found, "
> > +  "expected %d, found %d\n", numfds, ret);
> > +ret = E2BIG;
> > +goto done;
> > +}
> > +
> > +if (ret == numfds) {
> 
> Is this exact comparison correct? If we had a deamon that has both public
> and private sockets and the client only wrote to the public socket,
> wouldn't sd_listen_fds() only return 1?

It is correct, what sd_listen_fds() tells you is how many socket systemd
has created for you and is passing to you upon activation. It tells
nothing about clients.
After you epoll the socket you can gain any knowledge on whether there
are clients waiting.

Simo.

> > +rctx->lfd = SD_LISTEN_FDS_START;
> > +ret = sss_fd_nonblocking(rctx->lfd);
> > +if (ret != EOK) goto done;
> > +if (numfds == 2) {
> > +rctx->priv_lfd = SD_LISTEN_FDS_START + 1;
> > +ret = sss_fd_nonblocking(rctx->priv_lfd);
> > +if (r

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

[Pavel Reichl]

On 01/06/2016 04:34 PM, Sumit Bose wrote:

On Mon, Jan 04, 2016 at 03:40:05PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments. I attached rebased patch with fixes as you 
proposed.

On 12/10/2015 01:23 PM, Sumit Bose wrote:


Some unit-test where failing for me because of an issue in cleaning up
the secondary slices. 'it = it->next' does not work because it was just
freed in the body of the loop. I fixed it with the change below, but
maybe a do-while loop would be even nicer here?


Sorry to hear that. I fixed that in attached patch set.
Which test failed for you? make check is always passing in my environment.


This is what I see:

$ ./sss_idmap-tests
Running suite(s): IDMAP
88%: Checks: 25, Failures: 0, Errors: 3
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ss:0: (after this point) 
Received signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_uid2sid_ss:0: (after this point) 
Received signal 11 (Segmentation fault)
(null):-1:S:IDMAP mapping tests:idmap_test_sid2uid_ext_sec_slices:0: (after 
this point) Received signal 11 (Segmentation fault)

and here is what valgrind has to say:

$ valgrind .libs/lt-sss_idmap-tests
==17919== Memcheck, a memory error detector
==17919== Copyright (C) 2002-2013, and GNU GPL'd, by Julian Seward et al.
==17919== Using Valgrind-3.10.0 and LibVEX; rerun with -h for copyright info
==17919== Command: .libs/lt-sss_idmap-tests
==17919==
Running suite(s): IDMAP
==17919== Invalid read of size 4
==17919==at 0x407C595: sss_idmap_free_domain.part.1 (sss_idmap.c:220)
==17919==by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919==by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919==by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919==by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x80492D1: main (sss_idmap-tests.c:747)
==17919==  Address 0x4437d68 is 64 bytes inside a block of size 68 free'd
==17919==at 0x402C26D: free (in 
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==17919==by 0x4068C9E: _talloc_free_internal (talloc.c:1057)
==17919==by 0x4068C9E: _talloc_free (talloc.c:1581)
==17919==by 0x407C594: sss_idmap_free_domain.part.1 (sss_idmap.c:222)
==17919==by 0x407C859: sss_idmap_free_domain (sss_idmap.c:240)
==17919==by 0x407C859: sss_idmap_free (sss_idmap.c:241)
==17919==by 0x804B60D: idmap_ctx_teardown (sss_idmap-tests.c:75)
==17919==by 0x4050271: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x405080D: ??? (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x4050BED: srunner_run (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x40511D3: srunner_run_all (in /usr/lib/libcheck.so.0.0.0)
==17919==by 0x80492D1: main (sss_idmap-tests.c:747)
==17919==
100%: Checks: 25, Failures: 0, Errors: 0
==17919==
==17919== HEAP SUMMARY:
==17919== in use at exit: 0 bytes in 0 blocks
==17919==   total heap usage: 1,918 allocs, 1,918 frees, 749,697 bytes allocated
==17919==
==17919== All heap blocks were freed -- no leaks are possible
==17919==
==17919== For counts of detected and suppressed errors, rerun with: -v
==17919== ERROR SUMMARY: 32 errors from 1 contexts (suppressed: 0 from 0)






diff --git a/src/lib/idmap/sss_idmap.c b/src/lib/idmap/sss_idmap.c
index c4ae811..7ab5c66 100644
--- a/src/lib/idmap/sss_idmap.c
+++ b/src/lib/idmap/sss_idmap.c
@@ -210,6 +210,7 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,
struct idmap_domain_info *dom)
  {
  struct idmap_range_params *it;
+struct idmap_range_params *next;

  if (ctx == NULL || dom == NULL) {
  return;
@@ -217,7 +218,8 @@ static void sss_idmap_free_domain(struct
sss_idmap_ctx *ctx,

  ctx->free_func(dom->range_params.range_id, ctx->alloc_pvt);
  /* Free list of secondary ranges */
-for (it = dom->sec_ranges; it != NULL; it = it->next) {
+for (it = dom->sec_ranges; it != NULL; it = next) {
+next = it->next;
  ctx->free_func(it->range_id, ctx->alloc_pvt);
  ctx->free_func(it, ctx->alloc_pvt);
  }


While checking and playing with the test I came across

+/* Todo - Add tests */
+/* Add size of primary slice for first_rid of secondary slices. */
+rid += ctx->idmap_opts.rangesize;

in sss_idmap_add_auto_domain_ex(). Does the 'Todo' only refer to 'Add
tests' or to 'Add size of primary slice for first_rid of secondary
slices.' as well? I'm asking because you really have to use the size of
the primary slice and not ctx->idmap_opts.rangesize here because the
primary range is given as a parameter to sss_idmap_add_auto_domain_ex()
and the size might be different then ctx->idmap_opts.rangesize. E.g. in
the unit-test the range is [1234,9876].


I removed the misleading commen

[SSSD] Re: [PATCH] SPEC: Fix unowned directories

[Lukas Slebodnik]

On (12/01/16 10:38), Jakub Hrozek wrote:
>On Mon, Jan 11, 2016 at 11:49:45AM +0100, Lukas Slebodnik wrote:
>> On (11/01/16 10:29), Jakub Hrozek wrote:
>> >On Fri, Jan 08, 2016 at 09:29:57AM +0100, Lukas Slebodnik wrote:
>> >> ehlo,
>> >> 
>> >> patch should fix fedora bug 1266940
>> >> 
>> >> LS
>> >
>> >Thanks for the patch, but what other patches should I have in order to
>> >apply?
>> >
>> >Right now I have:
>> >a055c02 SPEC: Move libsss_sudo.so outside sssd-common
>> >18d722c SPEC: Change package ownership of %{pubconfpath}/krb5.include.d
>> >fc3cf30 AD SRV: prefer site-local DCs in LDAP ping
>> >
>> I created all patches on top of master and i didn't realize there might be
>> conflicts. There still might be conflict with the patch "libsss_sudo.so".
>> First patch will win :-)
>> 
>> Updated patch is attached.
>> 
>> There is also a small change in sssd-client and sssd-libwbclient
>> previous I added there only directory "%dir %{_libdir}/%{name}/modules"
>> because directory "%dir %{_libdir}/%{name}" was owned by sssd-common.
>> However, it was not right because sssd-cliet does not depend on sssd-common.
>> So this directory woudl not be owned by annyone iff only sssd-client is
>> installed (container use-case)
>> 
>> LS
>
>I can't build a package locally in mock after applying this patch..
>
>RPM build errors:
>Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang
>Directory not found: 
> /builddir/build/BUILDROOT/sssd-1.13.90-0.20160112.1027.git990e793.fc22.x86_64/etc/cifs-utils
I did the same think as in package cifs-utils
but I didn't notice they create directory for alternative
in "%install" section of spec file.

After this issue I realized there are another unowened files in fedora.
sh$ rpm -qf /usr/lib64/libwbclient.so.0*
file /usr/lib64/libwbclient.so.0 is not owned by any package
file /usr/lib64/libwbclient.so.0.12 is not owned by any package

it's not problem of upstream spec file because we do not install
alternatives for libwbclient. But I might add "%ghost" to the
downstream (fedora) spec file

LS
>From 692970eb6498a1c7f63047ea90eca0dc9d711c89 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 11 Jan 2016 11:23:46 +0100
Subject: [PATCH] SPEC: Fix unowned directories

https://fedoraproject.org/wiki/Packaging:UnownedDirectories
---
 contrib/sssd.spec.in | 22 --
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/contrib/sssd.spec.in b/contrib/sssd.spec.in
index 
9855e11a8bb0ff3f50ceeae98f383c514011cc90..f4c60b3c2788c5ddb07d4eb4c62225792ec2bedc
 100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -604,6 +604,12 @@ install -m644 src/examples/logrotate 
$RPM_BUILD_ROOT%{_sysconfdir}/logrotate.d/s
 mkdir -p $RPM_BUILD_ROOT/%{_sysconfdir}/rwtab.d
 install -m644 src/examples/rwtab $RPM_BUILD_ROOT%{_sysconfdir}/rwtab.d/sssd
 
+%if (0%{?with_cifs_utils_plugin} == 1)
+# Create directory for cifs-idmap alternative
+# Otherwise this directory could not be owned by sssd-client
+mkdir -p $RPM_BUILD_ROOT/%{_sysconfdir}/cifs-utils
+%endif
+
 # Remove .la files created by libtool
 find $RPM_BUILD_ROOT -name "*.la" -exec rm -f {} \;
 
@@ -697,6 +703,8 @@ rm -rf $RPM_BUILD_ROOT
 %{_libexecdir}/%{servicename}/p11_child
 
 %if (0%{?install_pcscd_polkit_rule} == 1)
+%dir %{_datadir}/polkit-1
+%dir %{_datadir}/polkit-1/rules.d
 %{_datadir}/polkit-1/rules.d/*
 %endif
 
@@ -714,7 +722,8 @@ rm -rf $RPM_BUILD_ROOT
 %{_libdir}/%{name}/libsss_semanage.so
 
 # 3rd party application libraries
-%{_libdir}/sssd/modules/libsss_autofs.so
+%dir %{_libdir}/%{name}/modules
+%{_libdir}/%{name}/modules/libsss_autofs.so
 %{_libdir}/libsss_sudo.so
 %{_libdir}/libnfsidmap/sss.so
 
@@ -732,9 +741,9 @@ rm -rf $RPM_BUILD_ROOT
 %ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/group
 %ghost %attr(0644,sssd,sssd) %verify(not md5 size mtime) %{mcpath}/initgroups
 %attr(755,sssd,sssd) %dir %{pipepath}
+%attr(700,sssd,sssd) %dir %{pipepath}/private
 %attr(755,sssd,sssd) %dir %{pubconfpath}
 %attr(755,sssd,sssd) %dir %{gpocachepath}
-%attr(700,sssd,sssd) %dir %{pipepath}/private
 %attr(750,sssd,sssd) %dir %{_var}/log/%{name}
 %attr(711,sssd,sssd) %dir %{_sysconfdir}/sssd
 %ghost %attr(0600,sssd,sssd) %config(noreplace) %{_sysconfdir}/sssd/sssd.conf
@@ -742,7 +751,9 @@ rm -rf $RPM_BUILD_ROOT
 %attr(755,root,root) %dir %{_sysconfdir}/systemd/system/sssd.service.d
 %config(noreplace) %{_sysconfdir}/systemd/system/sssd.service.d/journal.conf
 %endif
+%dir %{_sysconfdir}/logrotate.d
 %config(noreplace) %{_sysconfdir}/logrotate.d/sssd
+%dir %{_sysconfdir}/rwtab.d
 %config(noreplace) %{_sysconfdir}/rwtab.d/sssd
 %dir %{_datadir}/sssd
 %{_datadir}/sssd/sssd.api.conf
@@ -831,10 +842,14 @@ rm -rf $RPM_BUILD_ROOT
 %{_libdir}/krb5/plugins/libkrb5/sssd_krb5_locator_plugin.so
 %{_libdir}/krb5/plugins/authdata/sssd_pac_plugin.so
 %if (0%{?with_cifs_utils_plugin} == 1)
+%dir %{_libdir}/cifs-utils
 %{_libdir}/cifs-utils/cifs_idmap_sss.so
+%dir %{_sysconfdir}/cifs-ut

[SSSD] Re: [PATCHES] Fix warnings Wsign-compare

[Lukas Slebodnik]

On (24/11/15 14:28), Lukas Slebodnik wrote:
>On (23/11/15 15:29), Lukas Slebodnik wrote:
>>On (20/11/15 10:17), Lukas Slebodnik wrote:
>>>ehlo,
>>>
>>>I found few warnings on rawhide when I build sssd-1.13.2
>>>https://kojipkgs.fedoraproject.org//work/tasks/8618/11918618/build.log
>>>
>>>LS
>>
>>I missed one warning in pysss_nss_idmap.cwhich was not visible
>>on 64 bit architecture.
>>
>>@see 32 bit build log
>>https://kojipkgs.fedoraproject.org//packages/sssd/1.13.2/1.fc24/data/logs/i686/build.log
>>
>>BTW warning are caused by newly added warnings in python3.5 cflags
>>
>>PYTHON3_CFLAGS='-I/usr/include/python3.5m -I/usr/include/python3.5m
>>-Wno-unused-result -Wsign-compare -Wunreachable-code -O2 -g -pipe -Wall
>>-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>>-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>>-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom
>>-fasynchronous-unwind-tables -D_GNU_SOURCE -fPIC -fwrapv
>>-DDYNAMIC_ANNOTATIONS_ENABLED=1 -DNDEBUG -O2 -g -pipe -Wall
>>-Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions
>>-fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches
>>-specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m32 -march=i686 -mtune=atom
>>-fasynchronous-unwind-tables -D_GNU_SOURCE -fPIC -fwrapv'
>>
>>
>>python3.5 is only on rawhide.
>>
>>Updated patches are attached.
>>
>>LS
>
>>From c3e22efaacac525476ade16fd7d81bdfe7115e5c Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik 
>>Date: Thu, 19 Nov 2015 13:07:10 +
>>Subject: [PATCH 1/3] TOOLS: Fix warning Wsign-compare
>>MIME-Version: 1.0
>>Content-Type: text/plain; charset=UTF-8
>>Content-Transfer-Encoding: 8bit
>>
>>src/tools/tools_util.c: In function ‘parse_groups’:
>>src/tools/tools_util.c:116:19: error: comparison between
>>signed and unsigned integer expressions [-Werror=sign-compare]
>> for (i = 0; i < tokens; i++) {
>>   ^
>>---
>> src/tools/tools_util.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/src/tools/tools_util.c b/src/tools/tools_util.c
>>index 
>>f9dca728751f0429bb2f1c3ef46ffc269ecfba40..f475a51ef380f5d0397deb22d161fd2c8d2a81b8
>> 100644
>>--- a/src/tools/tools_util.c
>>+++ b/src/tools/tools_util.c
>>@@ -94,7 +94,7 @@ int parse_groups(TALLOC_CTX *mem_ctx, const char *optstr, 
>>char ***_out)
>> char *orig, *n, *o;
>> char delim = ',';
>> unsigned int tokens = 1;
>>-int i;
>>+unsigned int i;
>> 
>> orig = talloc_strdup(mem_ctx, optstr);
>> if (!orig) return ENOMEM;
>>-- 
>>2.5.0
>>
>
>>From b79f1fe6f6ffec7b9a7c9f045c12bc6c894faa78 Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik 
>>Date: Thu, 19 Nov 2015 13:07:52 +
>>Subject: [PATCH 2/3] pysss_murmur: Fix warning Wsign-compare
>>MIME-Version: 1.0
>>Content-Type: text/plain; charset=UTF-8
>>Content-Transfer-Encoding: 8bit
>>
>>src/python/pysss_murmur.c: In function ‘py_murmurhash3’:
>>src/python/pysss_murmur.c:47:17: error: comparison between
>> signed and unsigned integer expressions [-Werror=sign-compare]
>> key_len > strlen(key)) {
>> ^
>>
>>uint32_t murmurhash3(const char *key, int len, uint32_t seed)
>>The second argument of the function murmurhash3 has type int.
>>But the code expects to be unsigned integer.
>>
>>There is code in python wrapper py_murmurhash3
>>which check boundaries of that argument.
>>It should be an unsigned "key_len > INT_MAX || key_len < 0".
>>An exception should be thrown for negative number.
>>
>>Moreover, the length should be shorter then a length of input string.
>>The strlen returns size_t which is unsigned and key_len is signed long.
>>We already checked that value is unsigned so
>>we can safely cast key_len to size_t
>>---
>> src/python/pysss_murmur.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>>diff --git a/src/python/pysss_murmur.c b/src/python/pysss_murmur.c
>>index 
>>97d752b2a7734a332a5d5da07d75b594638015c8..b14a672025c218ae3ab314c3ad2cf2c5ced40870
>> 100644
>>--- a/src/python/pysss_murmur.c
>>+++ b/src/python/pysss_murmur.c
>>@@ -44,7 +44,7 @@ static PyObject * py_murmurhash3(PyObject *module, PyObject 
>>*args)
>> }
>> 
>> if (seed > UINT32_MAX || key_len > INT_MAX || key_len < 0 ||
>>-key_len > strlen(key)) {
>>+(size_t)key_len > strlen(key)) {
>> PyErr_Format(PyExc_ValueError, "Invalid value\n");
>> return NULL;
>> }
>>-- 
>>2.5.0
>>
>
>>From 07d5ea89b83e0176f7e51bd8b0ae9881aadff3bf Mon Sep 17 00:00:00 2001
>>From: Lukas Slebodnik 
>>Date: Thu, 19 Nov 2015 14:17:36 +
>>Subject: [PATCH 3/3] pyhbac: Fix warning Wsign-compare
>>MIME-Version: 1.0
>>Content-Type: text/plain; charset=UTF-8
>>Content-Transfer-Encoding: 8bit
>>
>>src/python/pyhbac.c: In function ‘HbacRuleElement_repr’:
>>src/python/pyhbac.c:506:59: error: comparison between
>>signed and unsigned integer expressions [-Werror=sign-compare]
>> if (strnames == NULL || strgroups == NULL || categ

[SSSD] Re: [PATCHES] Support IPA sudo schema

[Pavel Březina]

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

On 01/12/2016 01:19 PM, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 12:16:29PM +0100, Pavel Březina wrote:

On 01/08/2016 04:37 PM, Sumit Bose wrote:

On Fri, Jan 08, 2016 at 11:27:41AM +0100, Pavel Březina wrote:

On 01/07/2016 02:01 PM, Sumit Bose wrote:

On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:

On 12/18/2015 02:36 PM, Pavel Březina wrote:

Hello list,
the support of IPA sudo schema is almost complete. The only
thing that
remains is to finish smart refresh implementation and one patch to
reduce code duplication between LDAP and IPA implementation.
Then I need
to run some tests but I don't expect much troubles here since I
tested
it a lot during development. I'll finish all of this after my
Christmas
vacation.

The patches are probably too big to be sent as an attachment, so
until I
complete the last two patches, you can check it on my git repo,
branch
sudo [1]. I don't really expect anyone to review them during
Christmas
break, but I thought it's a good thing to present if in case
someone
will get really bored from all the candies and family visits :-)

Happy reviewing.

[1]
https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo



Final patches are attached. Happy reviewing.



Here are my first comments:

0001:
timeout == 0 is checked twice


Fixed.


+
+for (state->map_num_attrs = 0;
+state->map[state->map_num_attrs].opt_name != NULL;

wouldn't is be safer to check if map is not NULL before
deferencinf it?


Check added.


Is there a reason for incrementing base_iter at the end of
sdap_search_bases_next_base(), I think it would be more clear to
increment it
in sdap_search_bases_done() immediately before calling
sdap_search_bases_next_base() again?


It depends on the point of view, I guess. This way the iteration
logic is
all in one place which is better in my opinion.


hm, I see your point. What about initializing base_iter to some magic
value in sdap_search_bases_send and doing something like

 state->base_iter = (state->base_iter == MAGIC) ? 0 :
++state->base_iter;

then state->base_iter always points to the current base and you do not
need this odd

 base = state->bases[state->base_iter - 1]

in sdap_search_bases_done(). But that's only cosmetic and personal
preference, feel free to ignore it.




0005:

+int sysdb_compare_usn(const char *a, const char *b)
+{
+if (a == NULL) {
+return -1;
+}
+
+if (b == NULL) {
+return 1;
+}
+
+/* less digits means lower number */

this is only true as long as a and b do not start with 0. I would
recommend to
add a
   if (*a != '0' && *b != '0') {



I added a cycle to remove leading zeros.


+if (strlen(a) < strlen(b)) {
+return -1;
+}
+
+/* more digits means bigger number */
+if (strlen(a) > strlen(b)) {
+return 1;
+}
+
+/* now we can compare digits since alphabetical order is the
same
+ * as numeric order */
+return strcmp(a, b);
+}
+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
+  struct sysdb_attrs **attrs,
+  size_t num_attrs,
+  char **_usn)
+{
+const char *highest = NULL;
+const char *current = NULL;
+char *usn;
+errno_t ret;
+size_t i;
+
+if (num_attrs == 0 || attrs == NULL) {

you can save a few lines since highest == NULL with:
  goto done;


Done.


+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC ")"
+

I would prefer a more specific name like e.g. SUDO_ALL_FILTER.


Done.


0008:

@@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct
tevent_req *subreq)
  tevent_req_error(req, ret);
  }
  return;
+} else if (ret != EOK) {
+tevent_req_error(req, ret);


Isn't a return missing here?


Yes, fixed.


0009:

+static bool check_dn(struct ldb_dn *dn,
+ const char *rdn_attr,
+ va_list in_ap)
+{
+const struct ldb_val *ldbval;
+const char *strval;
+const char *ldbattr;
+const char *attr;
+const char *val;
+va_list ap;
+int num_comp;
+int comp;
+
+/* check RDN attribute */
+ldbattr = ldb_dn_get_rdn_name(dn);
+if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) {
+return false;
+}
+
+/* Check DN components. First we check if all attr=value
pairs match input.
+ * Then we check that the next attribute is a domain component.
+ */
+
+comp = 1;
+num_comp = ldb_dn_get_comp_num(dn);
+
+va_copy(ap, in_ap);
+while ((attr = va_arg(ap, const char *)) != NULL) {
+val = va_arg(ap, const char *);
+if (val == NULL) {
+return false;
+}
+
+if (comp > num_comp) {
+return false;
+}
+
+ldbattr = ldb_dn_get_component_name(dn, comp);
+if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
+

[SSSD] Re: [PATCH] DP: Print warning when the handler is not configured

[Jakub Hrozek]

On Tue, Jan 12, 2016 at 10:14:42AM +0100, Jakub Hrozek wrote:
> Hi,
> 
> the attached patch fixes a bug spotted by Lukas.

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

[SSSD] Re: [PATCH] Make responder connectin code more generic

[Jakub Hrozek]

On Mon, Jan 11, 2016 at 01:39:33PM -0500, Simo Sorce wrote:
> The following 2 patches change the connection setup code to be more
> flexible.
> 
> They are the groundwork to add a new secrets[1] responder that uses a
> REST API over a unix socket and therefore requires a different protocol
> handler.
> 
> The first patch move some protocol and state data into opaque pointers
> so that responders can arbitrarily set them.
> Ticket: https://fedorahosted.org/sssd/ticket/2918
> 
> The second patch adds the ability to use socket activation for
> responders. Although this is not currently wired in for use in
> currently.
> It is related to: https://fedorahosted.org/sssd/ticket/2243
> 
> These patches are being worked on on my secsrv branch here:
> https://fedorapeople.org/cgit/simo/public_git/sssd.git/log/?h=secsrv
> 
> 
> [1] https://fedorahosted.org/sssd/wiki/DesignDocs/SecretsService
> 
> -- 
> Simo Sorce * Red Hat, Inc * New York

> From e82a6c4fc5bd0c6dc264961c5b0fa4f97372e984 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Fri, 8 Jan 2016 17:51:06 -0500
> Subject: [PATCH] Responders: Make the client context more generic
> 
> This is useufl to allow reusing the responder code with other protocols.
> Store protocol data and responder state data behind opaque pointers and
> use tallog_get_type to check they are of the right type.
> 
> This also allows to store per responder state_ctx so that, for example,
> the autofs responder does not have to carry useless variables used only
> by the nss responder.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2918

There are some new Coverity warnings with this code:
Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.90/src/responder/common/responder_common.c:756: var_tested_neg: 
Assigning: "rctx->lfd" = a negative value.
sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns: 
"rctx->lfd" is passed to a parameter that cannot be negative.
sssd-1.13.90/src/responder/common/responder_common.c:745:5: neg_sink_parm_call: 
Passing "rctx->lfd" to "close", which cannot accept a negative number.
#  788|   #endif
#  789|   
#  790|-> ret = set_unix_socket(rctx, conn_setup);
#  791|   if (ret != EOK) {
#  792|   DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing 
sockets\n");

Error: NEGATIVE_RETURNS (CWE-394):
sssd-1.13.90/src/responder/common/responder_common.c:757: var_tested_neg: 
Assigning: "rctx->priv_lfd" = a negative value.
sssd-1.13.90/src/responder/common/responder_common.c:790: negative_returns: 
"rctx->priv_lfd" is passed to a parameter that cannot be negative.
sssd-1.13.90/src/responder/common/responder_common.c:746:5: neg_sink_parm_call: 
Passing "rctx->priv_lfd" to "close", which cannot accept a negative number.
#  788|   #endif
#  789|   
#  790|-> ret = set_unix_socket(rctx, conn_setup);
#  791|   if (ret != EOK) {
#  792|   DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing 
sockets\n");

But in general it looks good to me. I need to run more tests, though..

> From 796ad20628994e671b9af903ed396eb8103d18c4 Mon Sep 17 00:00:00 2001
> From: Simo Sorce 
> Date: Thu, 7 Jan 2016 10:17:38 -0500
> Subject: [PATCH 2/2] Responders: Add support for socket activation
> 
> Add helper that uses systemd socket activation if available to accept a
> pre-listining socket at startup.
> 
> Related:
> https://fedorahosted.org/sssd/ticket/2913

[...]

> +ret = sd_listen_fds(1);
> +if (ret < 0) {
> +DEBUG(SSSDBG_MINOR_FAILURE,
> +  "Unexpected error probing for active sockets. "
> +  "Will proceed with no sockets. [Error %d (%s)]\n",
> +  -ret, sss_strerror(-ret));
> +} else if (ret > numfds) {
> +DEBUG(SSSDBG_FATAL_FAILURE,
> +  "Too many activated sockets have been found, "
> +  "expected %d, found %d\n", numfds, ret);
> +ret = E2BIG;
> +goto done;
> +}
> +
> +if (ret == numfds) {

Is this exact comparison correct? If we had a deamon that has both public
and private sockets and the client only wrote to the public socket,
wouldn't sd_listen_fds() only return 1?

> +rctx->lfd = SD_LISTEN_FDS_START;
> +ret = sss_fd_nonblocking(rctx->lfd);
> +if (ret != EOK) goto done;
> +if (numfds == 2) {
> +rctx->priv_lfd = SD_LISTEN_FDS_START + 1;
> +ret = sss_fd_nonblocking(rctx->priv_lfd);
> +if (ret != EOK) goto done;
> +}
> +}
> +#endif
> +
> +ret = set_unix_socket(rctx, conn_setup);
> +if (ret != EOK) {
> +DEBUG(SSSDBG_FATAL_FAILURE, "Fatal error initializing sockets\n");
> +goto done;
> +}
> +
> +done:
> +return ret;
> +}
> +
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] SPEC: Change package ownership of %{pubconfpath}/krb5.include.d

[Jakub Hrozek]

On Tue, Jan 12, 2016 at 01:44:24PM +0100, Lukas Slebodnik wrote:
> On (12/01/16 13:36), Jakub Hrozek wrote:
> >On Thu, Jan 07, 2016 at 11:17:54AM +0100, Jakub Hrozek wrote:
> >> This looks like a bug, when I install from source, the directory is
> >> owned by sssd.sssd.
> >> 
> >> btw when I tested this, I think I found another issue -- we try to bump
> >> the mtime of /etc/krb5.conf, but since the file is only writable by
> >> root, we fail:
> >> (Thu Jan  7 10:11:00 2016) [sssd[be[ipa.test]]] 
> >> [sss_write_domain_mappings] (0x0200): Mapping file for domain [ipa.test] 
> >> is [/var/lib/sss/pubconf/krb5.include.d/domain_realm_ipa_test]
> >> (Thu Jan  7 10:12:04 2016) [sssd[be[ipa.test]]] 
> >> [sss_krb5_touch_config] (0x0020): Unable to change mtime of 
> >> "/etc/krb5.conf" [13]: Permission denied
> >> 
> >> I wonder if we should open krb.conf during startup and then call
> >> futimens() instead?
> >
> >Hmm, this seems to not work, even if I have a FD I opened as root,
> >calling futimens() on that fd returns EPERM..time for another setuid
> >helper?
> or libkrb5 should be changed to check also mtime for all
> included directories.
> 
> e.g.
> includedir /etc/krb5.conf.d/
> includedir /var/lib/sss/pubconf/krb5.include.d/
> ...
> 
> LS

Robbie, is this a reasonable thing to ask?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] BUILD: Enable the sssd krb5 localauth plugin by default

[Lukas Slebodnik]

On (12/01/16 13:40), Lukas Slebodnik wrote:
>On (05/11/15 13:51), Sumit Bose wrote:
>>On Thu, Nov 05, 2015 at 01:02:07PM +0100, Lukas Slebodnik wrote:
>>> On (05/11/15 12:42), Sumit Bose wrote:
>>> >On Thu, Nov 05, 2015 at 12:12:17PM +0100, Lukas Slebodnik wrote:
>>> >> ehlo,
>>> >> 
>>> >> attached simple patch is a result of "Fedora end of life"
>>> >> message for related Fedora ticket.
>>> >> 
>>> >> If you have an idea about better names I will be glad to change them.
>>> >> 
>>> >> BTW shoulw we also remove this part from function
>>> >> sss_write_krb5_conf_snippet
>>> >> 
>>> >> LS
>>> >
>>> >> From c4d56af303ba4385cf9ef1c9053545c243f07a44 Mon Sep 17 00:00:00 2001
>>> >> From: Lukas Slebodnik 
>>> >> Date: Thu, 5 Nov 2015 11:08:36 +0100
>>> >> Subject: [PATCH] BUILD: Enable the sssd krb5 localauth plugin by default
>>> >> 
>>> >> It will be installed to /etc/krb.conf.d/ only on these
>>> >> platforms which has krb5 with this directory
>>> >> 
>>> >> Resolves:
>>> >> https://fedorahosted.org/sssd/ticket/2449
>>> >
>>> >...
>>> >
>>> >
>>> >> new file mode 100644
>>> >> index 
>>> >> ..950cab8200eb50d7fc878723d38c93d5b616e468
>>> >> --- /dev/null
>>> >> +++ b/src/examples/sssd_localauth.conf.in
>>> >> @@ -0,0 +1,5 @@
>>> >> +[plugins]
>>> >> + localauth = {
>>> >> +  module = sssd:@krb5localauth_plugindir@/sssd_krb5_localauth_plugin.so
>>> >> +  enable_only = sssd
>>> >> + }
>>> >
>>> >just a comment, I think enable_only should not be used here. I added it
>>> >originally becasue I thought no other modules would be needed anymore,
>>> >but I was wrong, see e.g. https://fedorahosted.org/sssd/ticket/2788 or
>>> >https://fedorahosted.org/sssd/ticket/2707. 
>>> >
>>> I inspired in /var/lib/sss/pubconf/krb5.include.d/localauth_plugin
>>> 
>>> I removed the option enable_only.
>>> Will it solve #2707 and #2788?
>>> or it is unrelated.
>>
>>It depends. If e.g. the AD and IPA providers would not create
>>/var/lib/sss/pubconf/krb5.include.d/localauth_plugin anymore if
>>/etc/krb5.conf.d/sssd_localauth.conf exists I think #2788 is fixed
>>because we would fall back to the builtin k5login check if enable_only
>>is not set in /etc/krb5.conf.d/sssd_localauth.conf. If both files exists
>>and there is a 'includedir /var/lib/sss/pubconf/krb5.include.d/' in
>>/etc/krb5.conf it depends which file is processed first so I think we
>>should try to avoid it.
>>
>OK, I removed "enable_only" from both places.
>
>>Btw, what about the domain_realm mapping files we create in
>>/var/lib/sss/pubconf/krb5.include.d/localauth_plugin ? Should they be
>>created in /etc/krb5.conf.d/ if the directory exists? (Must not be
>>solved in the context of this ticket).
>>
>It would be good to store domain_realm mapping files there
>but it would not be allowed in non-root mode.
>
>sh$ ls -ld /etc/krb5.conf.d/
>drwxr-xr-x. 1 root root 30 Dec 23 17:12 /etc/krb5.conf.d/
>
>>If the file is labeled as '%config(noreplace)' in the spec
>>file we could say that the list is now configurable because changes stay
>>and close #2707 as well.
>>
>BTW /etc/krb5.conf.d/ is available (and included in krb5.conf)
>only on fedora 23+. So older distributions will still
>generate the file into /var/lib/sss/pubconf/krb5.include.d/
>
>LS

ups,
I sent wrong patches. New version is attached.

LS
>From 8fbe324a52878bbfb206bd1ff9dfdf930cea7c68 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 12 Jan 2016 12:56:31 +0100
Subject: [PATCH 1/2] UTIL: Rmove enable_only from krb5 localauth config

Resolves:
https://fedorahosted.org/sssd/ticket/2788
---
 src/util/domain_info_utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 
0791da3046c35e28cb1b479bb05610412acdb53c..4d7a927a0b946baed0658315104abe0ea3567279
 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -531,7 +531,6 @@ done:
 "[plugins]\n" \
 " localauth = {\n" \
 "  module = sssd:"APP_MODULES_PATH"/sssd_krb5_localauth_plugin.so\n" \
-"  enable_only = sssd\n" \
 " }"
 
 static errno_t sss_write_krb5_localauth_snippet(const char *path)
-- 
2.5.0

>From 24cec8410bac9501181b0bdbf63c8c70b9535e9c Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Thu, 5 Nov 2015 11:08:36 +0100
Subject: [PATCH 2/2] BUILD: Enable the sssd krb5 localauth plugin by default

It will be installed to /etc/krb.conf.d/ only on these
platforms which has krb5 with this directory

Resolves:
https://fedorahosted.org/sssd/ticket/2449
---
 Makefile.am | 15 ++-
 contrib/sssd.spec.in|  3 +++
 src/examples/sssd_localauth.conf.in |  4 
 src/external/krb5.m4|  4 
 src/tests/cmocka/test_utils.c   |  8 +++-
 src/util/domain_info_utils.c|  7 ++-
 6 files changed, 38 insertions(+), 3 deletions(-)
 create mode 100644 src/examples/sssd_localauth.conf.in

diff --git a/Makefile.am b/Makefile.am
index 
a9d3f25d3775f6ac824b9f9b85

[SSSD] Re: [PATCHES] Support IPA sudo schema

[Pavel Březina]

On 01/12/2016 01:19 PM, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 12:16:29PM +0100, Pavel Březina wrote:

On 01/08/2016 04:37 PM, Sumit Bose wrote:

On Fri, Jan 08, 2016 at 11:27:41AM +0100, Pavel Březina wrote:

On 01/07/2016 02:01 PM, Sumit Bose wrote:

On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:

On 12/18/2015 02:36 PM, Pavel Březina wrote:

Hello list,
the support of IPA sudo schema is almost complete. The only thing that
remains is to finish smart refresh implementation and one patch to
reduce code duplication between LDAP and IPA implementation. Then I need
to run some tests but I don't expect much troubles here since I tested
it a lot during development. I'll finish all of this after my Christmas
vacation.

The patches are probably too big to be sent as an attachment, so until I
complete the last two patches, you can check it on my git repo, branch
sudo [1]. I don't really expect anyone to review them during Christmas
break, but I thought it's a good thing to present if in case someone
will get really bored from all the candies and family visits :-)

Happy reviewing.

[1] https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo


Final patches are attached. Happy reviewing.



Here are my first comments:

0001:
timeout == 0 is checked twice


Fixed.


+
+for (state->map_num_attrs = 0;
+state->map[state->map_num_attrs].opt_name != NULL;

wouldn't is be safer to check if map is not NULL before deferencinf it?


Check added.


Is there a reason for incrementing base_iter at the end of
sdap_search_bases_next_base(), I think it would be more clear to increment it
in sdap_search_bases_done() immediately before calling
sdap_search_bases_next_base() again?


It depends on the point of view, I guess. This way the iteration logic is
all in one place which is better in my opinion.


hm, I see your point. What about initializing base_iter to some magic
value in sdap_search_bases_send and doing something like

 state->base_iter = (state->base_iter == MAGIC) ? 0 : ++state->base_iter;

then state->base_iter always points to the current base and you do not
need this odd

 base = state->bases[state->base_iter - 1]

in sdap_search_bases_done(). But that's only cosmetic and personal
preference, feel free to ignore it.




0005:

+int sysdb_compare_usn(const char *a, const char *b)
+{
+if (a == NULL) {
+return -1;
+}
+
+if (b == NULL) {
+return 1;
+}
+
+/* less digits means lower number */

this is only true as long as a and b do not start with 0. I would recommend to
add a
   if (*a != '0' && *b != '0') {



I added a cycle to remove leading zeros.


+if (strlen(a) < strlen(b)) {
+return -1;
+}
+
+/* more digits means bigger number */
+if (strlen(a) > strlen(b)) {
+return 1;
+}
+
+/* now we can compare digits since alphabetical order is the same
+ * as numeric order */
+return strcmp(a, b);
+}
+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
+  struct sysdb_attrs **attrs,
+  size_t num_attrs,
+  char **_usn)
+{
+const char *highest = NULL;
+const char *current = NULL;
+char *usn;
+errno_t ret;
+size_t i;
+
+if (num_attrs == 0 || attrs == NULL) {

you can save a few lines since highest == NULL with:
  goto done;


Done.


+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC ")"
+

I would prefer a more specific name like e.g. SUDO_ALL_FILTER.


Done.


0008:

@@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req 
*subreq)
  tevent_req_error(req, ret);
  }
  return;
+} else if (ret != EOK) {
+tevent_req_error(req, ret);


Isn't a return missing here?


Yes, fixed.


0009:

+static bool check_dn(struct ldb_dn *dn,
+ const char *rdn_attr,
+ va_list in_ap)
+{
+const struct ldb_val *ldbval;
+const char *strval;
+const char *ldbattr;
+const char *attr;
+const char *val;
+va_list ap;
+int num_comp;
+int comp;
+
+/* check RDN attribute */
+ldbattr = ldb_dn_get_rdn_name(dn);
+if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) {
+return false;
+}
+
+/* Check DN components. First we check if all attr=value pairs match input.
+ * Then we check that the next attribute is a domain component.
+ */
+
+comp = 1;
+num_comp = ldb_dn_get_comp_num(dn);
+
+va_copy(ap, in_ap);
+while ((attr = va_arg(ap, const char *)) != NULL) {
+val = va_arg(ap, const char *);
+if (val == NULL) {
+return false;
+}
+
+if (comp > num_comp) {
+return false;
+}
+
+ldbattr = ldb_dn_get_component_name(dn, comp);
+if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
+return false;
+}
+
+ldbval = l

[SSSD] Re: [PATCH] SPEC: Change package ownership of %{pubconfpath}/krb5.include.d

[Lukas Slebodnik]

On (12/01/16 13:36), Jakub Hrozek wrote:
>On Thu, Jan 07, 2016 at 11:17:54AM +0100, Jakub Hrozek wrote:
>> This looks like a bug, when I install from source, the directory is
>> owned by sssd.sssd.
>> 
>> btw when I tested this, I think I found another issue -- we try to bump
>> the mtime of /etc/krb5.conf, but since the file is only writable by
>> root, we fail:
>> (Thu Jan  7 10:11:00 2016) [sssd[be[ipa.test]]] 
>> [sss_write_domain_mappings] (0x0200): Mapping file for domain [ipa.test] is 
>> [/var/lib/sss/pubconf/krb5.include.d/domain_realm_ipa_test]
>> (Thu Jan  7 10:12:04 2016) [sssd[be[ipa.test]]] [sss_krb5_touch_config] 
>> (0x0020): Unable to change mtime of "/etc/krb5.conf" [13]: Permission denied
>> 
>> I wonder if we should open krb.conf during startup and then call
>> futimens() instead?
>
>Hmm, this seems to not work, even if I have a FD I opened as root,
>calling futimens() on that fd returns EPERM..time for another setuid
>helper?
or libkrb5 should be changed to check also mtime for all
included directories.

e.g.
includedir /etc/krb5.conf.d/
includedir /var/lib/sss/pubconf/krb5.include.d/
...

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

[SSSD] Re: [PATCH] BUILD: Enable the sssd krb5 localauth plugin by default

[Lukas Slebodnik]

On (05/11/15 13:51), Sumit Bose wrote:
>On Thu, Nov 05, 2015 at 01:02:07PM +0100, Lukas Slebodnik wrote:
>> On (05/11/15 12:42), Sumit Bose wrote:
>> >On Thu, Nov 05, 2015 at 12:12:17PM +0100, Lukas Slebodnik wrote:
>> >> ehlo,
>> >> 
>> >> attached simple patch is a result of "Fedora end of life"
>> >> message for related Fedora ticket.
>> >> 
>> >> If you have an idea about better names I will be glad to change them.
>> >> 
>> >> BTW shoulw we also remove this part from function
>> >> sss_write_krb5_conf_snippet
>> >> 
>> >> LS
>> >
>> >> From c4d56af303ba4385cf9ef1c9053545c243f07a44 Mon Sep 17 00:00:00 2001
>> >> From: Lukas Slebodnik 
>> >> Date: Thu, 5 Nov 2015 11:08:36 +0100
>> >> Subject: [PATCH] BUILD: Enable the sssd krb5 localauth plugin by default
>> >> 
>> >> It will be installed to /etc/krb.conf.d/ only on these
>> >> platforms which has krb5 with this directory
>> >> 
>> >> Resolves:
>> >> https://fedorahosted.org/sssd/ticket/2449
>> >
>> >...
>> >
>> >
>> >> new file mode 100644
>> >> index 
>> >> ..950cab8200eb50d7fc878723d38c93d5b616e468
>> >> --- /dev/null
>> >> +++ b/src/examples/sssd_localauth.conf.in
>> >> @@ -0,0 +1,5 @@
>> >> +[plugins]
>> >> + localauth = {
>> >> +  module = sssd:@krb5localauth_plugindir@/sssd_krb5_localauth_plugin.so
>> >> +  enable_only = sssd
>> >> + }
>> >
>> >just a comment, I think enable_only should not be used here. I added it
>> >originally becasue I thought no other modules would be needed anymore,
>> >but I was wrong, see e.g. https://fedorahosted.org/sssd/ticket/2788 or
>> >https://fedorahosted.org/sssd/ticket/2707. 
>> >
>> I inspired in /var/lib/sss/pubconf/krb5.include.d/localauth_plugin
>> 
>> I removed the option enable_only.
>> Will it solve #2707 and #2788?
>> or it is unrelated.
>
>It depends. If e.g. the AD and IPA providers would not create
>/var/lib/sss/pubconf/krb5.include.d/localauth_plugin anymore if
>/etc/krb5.conf.d/sssd_localauth.conf exists I think #2788 is fixed
>because we would fall back to the builtin k5login check if enable_only
>is not set in /etc/krb5.conf.d/sssd_localauth.conf. If both files exists
>and there is a 'includedir /var/lib/sss/pubconf/krb5.include.d/' in
>/etc/krb5.conf it depends which file is processed first so I think we
>should try to avoid it.
>
OK, I removed "enable_only" from both places.

>Btw, what about the domain_realm mapping files we create in
>/var/lib/sss/pubconf/krb5.include.d/localauth_plugin ? Should they be
>created in /etc/krb5.conf.d/ if the directory exists? (Must not be
>solved in the context of this ticket).
>
It would be good to store domain_realm mapping files there
but it would not be allowed in non-root mode.

sh$ ls -ld /etc/krb5.conf.d/
drwxr-xr-x. 1 root root 30 Dec 23 17:12 /etc/krb5.conf.d/

>If the file is labeled as '%config(noreplace)' in the spec
>file we could say that the list is now configurable because changes stay
>and close #2707 as well.
>
BTW /etc/krb5.conf.d/ is available (and included in krb5.conf)
only on fedora 23+. So older distributions will still
generate the file into /var/lib/sss/pubconf/krb5.include.d/

LS
>From 8fbe324a52878bbfb206bd1ff9dfdf930cea7c68 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Tue, 12 Jan 2016 12:56:31 +0100
Subject: [PATCH 1/2] UTIL: Rmove enable_only from krb5 localauth config

Resolves:
https://fedorahosted.org/sssd/ticket/2788
---
 src/util/domain_info_utils.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 
0791da3046c35e28cb1b479bb05610412acdb53c..4d7a927a0b946baed0658315104abe0ea3567279
 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -531,7 +531,6 @@ done:
 "[plugins]\n" \
 " localauth = {\n" \
 "  module = sssd:"APP_MODULES_PATH"/sssd_krb5_localauth_plugin.so\n" \
-"  enable_only = sssd\n" \
 " }"
 
 static errno_t sss_write_krb5_localauth_snippet(const char *path)
-- 
2.5.0

>From 4cb6466eece100e026ef2c96f6d9f7b799a0a13b Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Thu, 5 Nov 2015 11:08:36 +0100
Subject: [PATCH 2/2] BUILD: Enable the sssd krb5 localauth plugin by default

It will be installed to /etc/krb.conf.d/ only on these
platforms which has krb5 with this directory

Resolves:
https://fedorahosted.org/sssd/ticket/2449
---
 Makefile.am | 15 ++-
 contrib/sssd.spec.in|  3 +++
 src/examples/sssd_localauth.conf.in |  4 
 src/external/krb5.m4|  4 
 src/util/domain_info_utils.c|  7 ++-
 5 files changed, 31 insertions(+), 2 deletions(-)
 create mode 100644 src/examples/sssd_localauth.conf.in

diff --git a/Makefile.am b/Makefile.am
index 
a9d3f25d3775f6ac824b9f9b85dd0412417c33d3..526bbd44926d40d4d3a9a5dc0b3528eed97d7600
 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -55,6 +55,7 @@ sssdapiplugindir = $(sssddatadir)/sssd.api.d
 dbuspolicydir = $(sysconfdir)/dbus-1/system.d
 dbusservicedir = $(datad

[SSSD] Re: [PATCH] SPEC: Change package ownership of %{pubconfpath}/krb5.include.d

[Jakub Hrozek]

On Thu, Jan 07, 2016 at 11:17:54AM +0100, Jakub Hrozek wrote:
> This looks like a bug, when I install from source, the directory is
> owned by sssd.sssd.
> 
> btw when I tested this, I think I found another issue -- we try to bump
> the mtime of /etc/krb5.conf, but since the file is only writable by
> root, we fail:
> (Thu Jan  7 10:11:00 2016) [sssd[be[ipa.test]]] 
> [sss_write_domain_mappings] (0x0200): Mapping file for domain [ipa.test] is 
> [/var/lib/sss/pubconf/krb5.include.d/domain_realm_ipa_test]
> (Thu Jan  7 10:12:04 2016) [sssd[be[ipa.test]]] [sss_krb5_touch_config] 
> (0x0020): Unable to change mtime of "/etc/krb5.conf" [13]: Permission denied
> 
> I wonder if we should open krb.conf during startup and then call
> futimens() instead?

Hmm, this seems to not work, even if I have a FD I opened as root,
calling futimens() on that fd returns EPERM..time for another setuid
helper?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCHES] Support IPA sudo schema

[Sumit Bose]

On Mon, Jan 11, 2016 at 12:16:29PM +0100, Pavel Březina wrote:
> On 01/08/2016 04:37 PM, Sumit Bose wrote:
> >On Fri, Jan 08, 2016 at 11:27:41AM +0100, Pavel Březina wrote:
> >>On 01/07/2016 02:01 PM, Sumit Bose wrote:
> >>>On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:
> On 12/18/2015 02:36 PM, Pavel Březina wrote:
> >Hello list,
> >the support of IPA sudo schema is almost complete. The only thing that
> >remains is to finish smart refresh implementation and one patch to
> >reduce code duplication between LDAP and IPA implementation. Then I need
> >to run some tests but I don't expect much troubles here since I tested
> >it a lot during development. I'll finish all of this after my Christmas
> >vacation.
> >
> >The patches are probably too big to be sent as an attachment, so until I
> >complete the last two patches, you can check it on my git repo, branch
> >sudo [1]. I don't really expect anyone to review them during Christmas
> >break, but I thought it's a good thing to present if in case someone
> >will get really bored from all the candies and family visits :-)
> >
> >Happy reviewing.
> >
> >[1] 
> >https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudo
> 
> Final patches are attached. Happy reviewing.
> 
> >>>
> >>>Here are my first comments:
> >>>
> >>>0001:
> >>>timeout == 0 is checked twice
> >>
> >>Fixed.
> >>
> >>>+
> >>>+for (state->map_num_attrs = 0;
> >>>+state->map[state->map_num_attrs].opt_name != NULL;
> >>>
> >>>wouldn't is be safer to check if map is not NULL before deferencinf it?
> >>
> >>Check added.
> >>
> >>>Is there a reason for incrementing base_iter at the end of
> >>>sdap_search_bases_next_base(), I think it would be more clear to increment 
> >>>it
> >>>in sdap_search_bases_done() immediately before calling
> >>>sdap_search_bases_next_base() again?
> >>
> >>It depends on the point of view, I guess. This way the iteration logic is
> >>all in one place which is better in my opinion.
> >
> >hm, I see your point. What about initializing base_iter to some magic
> >value in sdap_search_bases_send and doing something like
> >
> > state->base_iter = (state->base_iter == MAGIC) ? 0 : ++state->base_iter;
> >
> >then state->base_iter always points to the current base and you do not
> >need this odd
> >
> > base = state->bases[state->base_iter - 1]
> >
> >in sdap_search_bases_done(). But that's only cosmetic and personal
> >preference, feel free to ignore it.
> >
> >>
> >>>0005:
> >>>
> >>>+int sysdb_compare_usn(const char *a, const char *b)
> >>>+{
> >>>+if (a == NULL) {
> >>>+return -1;
> >>>+}
> >>>+
> >>>+if (b == NULL) {
> >>>+return 1;
> >>>+}
> >>>+
> >>>+/* less digits means lower number */
> >>>
> >>>this is only true as long as a and b do not start with 0. I would 
> >>>recommend to
> >>>add a
> >>>   if (*a != '0' && *b != '0') {
> >>>
> >>
> >>I added a cycle to remove leading zeros.
> >>
> >>>+if (strlen(a) < strlen(b)) {
> >>>+return -1;
> >>>+}
> >>>+
> >>>+/* more digits means bigger number */
> >>>+if (strlen(a) > strlen(b)) {
> >>>+return 1;
> >>>+}
> >>>+
> >>>+/* now we can compare digits since alphabetical order is the same
> >>>+ * as numeric order */
> >>>+return strcmp(a, b);
> >>>+}
> >>>+errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx,
> >>>+  struct sysdb_attrs **attrs,
> >>>+  size_t num_attrs,
> >>>+  char **_usn)
> >>>+{
> >>>+const char *highest = NULL;
> >>>+const char *current = NULL;
> >>>+char *usn;
> >>>+errno_t ret;
> >>>+size_t i;
> >>>+
> >>>+if (num_attrs == 0 || attrs == NULL) {
> >>>
> >>>you can save a few lines since highest == NULL with:
> >>>  goto done;
> >>
> >>Done.
> >>
> >>>+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC ")"
> >>>+
> >>>
> >>>I would prefer a more specific name like e.g. SUDO_ALL_FILTER.
> >>
> >>Done.
> >>
> >>>0008:
> @@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req 
> *subreq)
>   tevent_req_error(req, ret);
>   }
>   return;
> +} else if (ret != EOK) {
> +tevent_req_error(req, ret);
> >>>
> >>>Isn't a return missing here?
> >>
> >>Yes, fixed.
> >>
> >>>0009:
> >>>
> >>>+static bool check_dn(struct ldb_dn *dn,
> >>>+ const char *rdn_attr,
> >>>+ va_list in_ap)
> >>>+{
> >>>+const struct ldb_val *ldbval;
> >>>+const char *strval;
> >>>+const char *ldbattr;
> >>>+const char *attr;
> >>>+const char *val;
> >>>+va_list ap;
> >>>+int num_comp;
> >>>+int comp;
> >>>+
> >>>+/* check RDN attribute */
> >>>+ldbattr = ldb_dn_get_rdn_name(dn);
> >>>+if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr)

[SSSD] Re: [PATCH] DP: Print warning when the handler is not configured

[Petr Cech]

On 01/12/2016 10:14 AM, Jakub Hrozek wrote:

Hi,

the attached patch fixes a bug spotted by Lukas.


0001-DP-Print-warning-when-the-handler-is-not-configured.patch


From 0e2e48102aec5d7ffb15aeec23830e595bc91961 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek
Date: Mon, 11 Jan 2016 14:56:41 +0100
Subject: [PATCH] DP: Print warning when the handler is not configured


Hello,

CI tests: http://sssd-ci.duckdns.org/logs/job/35/37/summary.html

=> ACK

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

[SSSD] Re: [PATCH] DP: Print warning when the handler is not configured

[Lukas Slebodnik]

On (12/01/16 10:14), Jakub Hrozek wrote:
>Hi,
>
>the attached patch fixes a bug spotted by Lukas.

>From 0e2e48102aec5d7ffb15aeec23830e595bc91961 Mon Sep 17 00:00:00 2001
>From: Jakub Hrozek 
>Date: Mon, 11 Jan 2016 14:56:41 +0100
>Subject: [PATCH] DP: Print warning when the handler is not configured
>
>We would previously only print the generic warning, not the
>user-supplied error message.
>---
> src/providers/data_provider_be.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/src/providers/data_provider_be.c 
>b/src/providers/data_provider_be.c
>index 
>41680c73ae6305aa4c9379c7545ab88afdb7ba48..a653acd44153e0c5c433be7c6153011b4bdd0c4f
> 100644
>--- a/src/providers/data_provider_be.c
>+++ b/src/providers/data_provider_be.c
>@@ -272,7 +272,9 @@ static errno_t be_sbus_reply(struct sbus_request *sbus_req,
> safe_err_msg = safe_be_req_err_msg(err_msg, err_maj);
> 
> if (err_maj == DP_ERR_FATAL && err_min == ENODEV) {
>-DEBUG(SSSDBG_TRACE_LIBS, "Handler not configured\n");
>+DEBUG(SSSDBG_TRACE_LIBS,
>+  "Cannot handle request: %s",
>+  err_msg ? err_msg : "Handler not configured\n");
> } else {
> DEBUG(SSSDBG_TRACE_LIBS,
>   "Request processed. Returned [%s]:%d,%d,%s\n",

ACK

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

[SSSD] Re: [DESIGN] ID mapping - Automatically assign new slices for any AD domain

[Pavel Březina]

On 01/11/2016 02:55 PM, Pavel Reichl wrote:



On 01/11/2016 02:32 PM, Pavel Březina wrote:

On 01/11/2016 02:21 PM, Jakub Hrozek wrote:

On Mon, Jan 11, 2016 at 02:09:48PM +0100, Sumit Bose wrote:

On Mon, Jan 11, 2016 at 01:03:33PM +0100, Pavel Reichl wrote:

Hello Sumit, thanks for comments and sorry for my delayed response,
I'm addressing the issues right now, in this mail I just want
quickly discuss the concern you have about
in loop declaration.

I generally I prefer this kind of declaration as it limits scope of
variables to minimum and thus makes it easier to read and
comprehend (It makes it obvious that variable is
used just in this single loop and it's insignificant for the rest
of the code).

In my experience there's not a performance hit at all some even
claim that by limiting scope of variables you give hints to
compiler and thus generated code is faster.

This topic was discussed for example here:

http://stackoverflow.com/questions/407255/difference-between-declaring-variables-before-or-in-loop

http://stackoverflow.com/questions/1956353/efficiency-of-c-variable-declaration


I think that the conclusion of these threads complies with what I
have stated above. But I suppose it would be possible to find
threads claiming opposite so I'm aware it's
not definite source of truth :-)

Anyway, If you prefer If I kept all declaration at the begging of
the function for legacy purposes or other I will do it (without any
hard feelings).


Thank you for the link, so it looks compilers can handle this well. I'm
fine with this scheme, iirc we had this discussion for the loop index
before and agreed that it makes sense to reduce the scope where
possilbe
to the loop. But I don't remember if there was an agreement about
variabled inside the loop as well? But if we can agree on this I would
like to ask you to update the paragraph in the coding style which
currently reads "HIGHLY RECOMMENDED: Always declare all variables at
the
top of the function, normally try to avoid declaring local variables in
internal loops.".


I personally think that as soon as you start mixing variable
declarations into code, you should maybe think about splitting the code
into more function..

IMO with the exception of indexes mixing code and declaration actually
/decreases/ readability.


I must agree with Jakub on this one. Such scenario is a perfect hint
that you should decompose the code into functions. In case of
get_sec_slices() it just prolongs the for loop and makes the code
harder to understand since you need to keep the scope
in mind.


I think that compiler keeps scope in mind so I should make things easier.



BTW I haven't read whole Sumit's review so he might have already
mentioned it but prev seems to be pretty much useless.


Why do you think so?

I checked the code and I think it's used to link members of list...I
suppose there's better way but I can't see it right now. Do you?


I'm sorry for the harsh wording. I believe you can use 
DLINK_LIST_ADD[_END] here.

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

[SSSD] Re: [PATCH] SPEC: Fix unowned directories

[Jakub Hrozek]

On Mon, Jan 11, 2016 at 11:49:45AM +0100, Lukas Slebodnik wrote:
> On (11/01/16 10:29), Jakub Hrozek wrote:
> >On Fri, Jan 08, 2016 at 09:29:57AM +0100, Lukas Slebodnik wrote:
> >> ehlo,
> >> 
> >> patch should fix fedora bug 1266940
> >> 
> >> LS
> >
> >Thanks for the patch, but what other patches should I have in order to
> >apply?
> >
> >Right now I have:
> >a055c02 SPEC: Move libsss_sudo.so outside sssd-common
> >18d722c SPEC: Change package ownership of %{pubconfpath}/krb5.include.d
> >fc3cf30 AD SRV: prefer site-local DCs in LDAP ping
> >
> I created all patches on top of master and i didn't realize there might be
> conflicts. There still might be conflict with the patch "libsss_sudo.so".
> First patch will win :-)
> 
> Updated patch is attached.
> 
> There is also a small change in sssd-client and sssd-libwbclient
> previous I added there only directory "%dir %{_libdir}/%{name}/modules"
> because directory "%dir %{_libdir}/%{name}" was owned by sssd-common.
> However, it was not right because sssd-cliet does not depend on sssd-common.
> So this directory woudl not be owned by annyone iff only sssd-client is
> installed (container use-case)
> 
> LS

I can't build a package locally in mock after applying this patch..

RPM build errors:
Empty %files file /builddir/build/BUILD/sssd-1.13.90/sssd_client.lang
Directory not found: 
/builddir/build/BUILDROOT/sssd-1.13.90-0.20160112.1027.git990e793.fc22.x86_64/etc/cifs-utils
ERROR: 
Exception(/home/remote/jhrozek/devel/sssd/rpmbuild/SRPMS/sssd-1.13.90-0.20160112.1027.git990e793.fc22.src.rpm)
 Config(fedora-22-x86_64) 2 minutes 13 seconds
INFO: Results and/or logs in: /var/lib/mock/fedora-22-x86_64/result
ERROR: Command failed. See logs for output.
 # bash --login -c /usr/bin/rpmbuild -bb --target x86_64 --nodeps  
/builddir/build/SPECS/sssd.spec 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] SDAP_ASYNC: Avoid useless debug message

[Petr Cech]

On 01/06/2016 02:19 PM, Jakub Hrozek wrote:

On Wed, Jan 06, 2016 at 11:03:45AM +0100, Sumit Bose wrote:

On Wed, Jan 06, 2016 at 10:47:13AM +0100, Pavel Březina wrote:

On 01/05/2016 05:33 PM, Jakub Hrozek wrote:

On Tue, Jan 05, 2016 at 02:12:51PM +0100, Pavel Březina wrote:

On 12/14/2015 03:36 PM, Petr Cech wrote:

Hi all,

there is patch for https://fedorahosted.org/sssd/ticket/2791 attached.

Result of patch:

The message:
Dec 14 14:16:11 vm-058-166 sssd[be[uma.dev]]: dereference processing
failed : Input/output error
is replaced by
Dec 14 15:29:26 vm-058-166 sssd[be[uma.dev]]: LDAP server claims to
support deref, but deref search failed. Disabling deref for further
requests. You can permanently disable deref by setting
ldap_deref_threshold to 0 in domain configuration.

But I am little afraid of this patch. Why? I tested this work on RHEL 6
how it is described in the ticket.
If I tried to set
   ldap_deref_threshold = 0
it was still falling into bad case and printed I/O error. So, if we
considered patch is right, there is question if advice in new message is
valid for IPA 3.x.

Regards

Petr


This threshold parameter applies for groups membership, not for views where
dereference is always used in current code.

The question is, why does sdap_x_deref_search_send fail with EIO?
Dereference should be supported with IPA...


Because the dereferenced attribute doesn't exist on the server IIRC.


Then the debug message is misleading since dereference is in fact working.
We should return different error code if possible (ENOENT?) and this case
should be handled in views code.


Unfortunately there is no specific LDAP error code for this case, see
e.g. comments in a50b229c8ea1e22c9efa677760b94d8c48c3ec89. Different
versions of 389ds return LDAP_UNAVAILABLE_CRITICAL_EXTENSION or
LDAP_PROTOCOL_ERROR, not sure what OpenLDAP does.

I think the error handling code should be moved to the callers because
only they know how to handle different error conditions.


+1

we can also translate the LDAP-specific error codes into sssd private
error codes instead of overloading ENOENT/EIO/ENOSUP (ENOSUP is IMO the
only one that really fits the problem..)



As an alternative now options/flags can be added to
sdap_deref_search_send() as it was done for sdap_get_generic_ext_send()
some time ago, but I would prefer the other.


No, I also prefer to let the low-level function be dumb and just return
an error which the caller deals with.


Hi,

thanks for all comments.

I found processing of LDAP_UNAVAILABLE_CRITICAL_EXTENSION at
sdap_get_generic_op_finished() function. There is no processing of 
LDAP_PROTOCOL_ERROR. So I added it.


Now, the error codes for LDAP_RES_SEARCH_RESULT are ENOTSUP for our case 
and EIO for the others.


Next processing of those EIO and ENOTSUP is in sdap_deref_search_done(),
it is added by 4709ff46db0dbe073aef061b796d2fd7adeaf18f commit. I am not 
sure, after Pavel Brezina comment, if the debug message for ENOTSUP case 
is right here.


Attached patch is applicable for master and sssd-1-12 too.

Regards
--
Petr^4 Cech
>From fb418ba8466fc1e9060f31c65be13708f6f49a03 Mon Sep 17 00:00:00 2001
From: Petr Cech 
Date: Tue, 12 Jan 2016 04:06:57 -0500
Subject: [PATCH] SDAP_ASYNC: Avoid useless debug message

FreeIPA (3.x) doesn't support views. There were very useless message in
syslog 'dereference processing failed : Input/output error' during
SSSD start. This patch avoids this message.

Resolves:
https://fedorahosted.org/sssd/ticket/2791
---
 src/providers/ldap/sdap_async.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 668bd7b465bbfefad13ab0b7061cd16a05dfbef1..2bc50a43ff25a93901f35b3fd23b648d7d3e2aed 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1529,7 +1529,8 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 ldap_memfree(errmsg);
 tevent_req_error(req, EIO);
 return;
-} else if (result == LDAP_UNAVAILABLE_CRITICAL_EXTENSION) {
+} else if (result == LDAP_UNAVAILABLE_CRITICAL_EXTENSION
+   || result == LDAP_PROTOCOL_ERROR) {
 ldap_memfree(errmsg);
 tevent_req_error(req, ENOTSUP);
 return;
-- 
2.4.3

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

[SSSD] Re: [PATCH] tests: use unittest.TestCase.assertCountEqual if possible

[Jakub Hrozek]

On Tue, Jan 12, 2016 at 10:21:15AM +0100, Lukas Slebodnik wrote:
> >I fixed the new pep8 warnings in the first patch and all the others in
> >that module in the second patch.
> >
> >Thanks for the review.
> 
> >From 89e56266a17661d6219107912b4501fbb1071bf2 Mon Sep 17 00:00:00 2001
> >From: Sumit Bose 
> >Date: Mon, 21 Dec 2015 15:51:09 +0100
> >Subject: [PATCH] ldap: remove originalMeberOf if there is no memberOf
> >
> Have you sent the correct patch ?
> 
> LS

No :-) See the ones in attachment..
>From 7bdeb696c6e3f42624a3bc9459354fd1a8dced85 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Tue, 10 Nov 2015 17:32:27 +0100
Subject: [PATCH 1/2] tests: use unittest.TestCase.assertCountEqual if possible

We used to defined a compat method for assertItemsEqual that existed on
Python 2.7, but not on old Python 2.x. As an effect, we used our compat
code even if assertCountEqual was available from standard library.

The recent Python 3.x versions renamed assertItemsEqual to
assertCountEqual. Therefore we should use the modern version which is in
the standard library over a compat version provided by ourselves.
---
 src/tests/pyhbac-test.py   | 99 +++---
 src/tests/pysss_murmur-test.py | 13 --
 2 files changed, 55 insertions(+), 57 deletions(-)

diff --git a/src/tests/pyhbac-test.py b/src/tests/pyhbac-test.py
index 
9d8fd1a333bf54ecf21d14d3b6293f7294a0d53e..54aeae778c549b0cc7f5089d2fe467d63b86317d
 100755
--- a/src/tests/pyhbac-test.py
+++ b/src/tests/pyhbac-test.py
@@ -14,19 +14,30 @@ MODPATH = srcdir + "/.libs" #FIXME - is there a way to get 
this from libtool?
 if sys.version_info[0] > 2:
 unicode = str
 
-def compat_assertItemsEqual(this, expected_seq, actual_seq, msg=None):
-return this.assertEqual(sorted(expected_seq), sorted(actual_seq))
 
 def compat_assertIsInstance(this, obj, cls, msg=None):
 return this.assertTrue(isinstance(obj, cls))
 
-# add compat methods for old unittest.TestCase versions
-# (python < 2.7, RHEL5 for instance)
-if not hasattr(unittest.TestCase, "assertItemsEqual"):
-setattr(unittest.TestCase, "assertItemsEqual", compat_assertItemsEqual)
+
+def compat_assertItemsEqual(this, expected_seq, actual_seq, msg=None):
+return this.assertEqual(sorted(expected_seq), sorted(actual_seq))
+
+# add compat assertIsInstance for old unittest.TestCase versions
+# (python < 2.7, RHEL6 for instance)
 if not hasattr(unittest.TestCase, "assertIsInstance"):
 setattr(unittest.TestCase, "assertIsInstance", compat_assertIsInstance)
 
+# Python3 renamed assertItemsEqual to assertCountEqual but at the same time
+# Python2 doesn't have assertCountEqual, see http://bugs.python.org/issue17866
+if not hasattr(unittest.TestCase, "assertCountEqual"):
+if not hasattr(unittest.TestCase, "assertItemsEqual"):
+# This is RHEL-6
+setattr(unittest.TestCase, "assertItemsEqual", compat_assertItemsEqual)
+
+setattr(unittest.TestCase,
+"assertCountEqual",
+unittest.TestCase.assertItemsEqual)
+
 class PyHbacImport(unittest.TestCase):
 def setUp(self):
 " Make sure we load the in-tree module "
@@ -66,38 +77,38 @@ class PyHbacImport(unittest.TestCase):
 class PyHbacRuleElementTest(unittest.TestCase):
 def testInstantiateEmpty(self):
 el = pyhbac.HbacRuleElement()
-self.assertItemsEqual(el.names, [])
-self.assertItemsEqual(el.groups, [])
-self.assertItemsEqual(el.category, set([pyhbac.HBAC_CATEGORY_NULL]))
+self.assertCountEqual(el.names, [])
+self.assertCountEqual(el.groups, [])
+self.assertCountEqual(el.category, set([pyhbac.HBAC_CATEGORY_NULL]))
 
 def testInit(self):
 names = [ "foo", "bar" ]
 el = pyhbac.HbacRuleElement(names=names)
-self.assertItemsEqual(el.names, names)
+self.assertCountEqual(el.names, names)
 
 groups = [ "abc", "def" ]
 el = pyhbac.HbacRuleElement(groups=groups)
-self.assertItemsEqual(el.groups, groups)
+self.assertCountEqual(el.groups, groups)
 
 def testGetSet(self):
 names = [ "foo", "bar" ]
 el = pyhbac.HbacRuleElement()
-self.assertItemsEqual(el.names, [])
+self.assertCountEqual(el.names, [])
 el.names = names
-self.assertItemsEqual(el.names, names)
+self.assertCountEqual(el.names, names)
 
 groups = [ "abc", "def" ]
 el = pyhbac.HbacRuleElement()
-self.assertItemsEqual(el.groups, [])
+self.assertCountEqual(el.groups, [])
 el.groups = groups
-self.assertItemsEqual(el.groups, groups)
+self.assertCountEqual(el.groups, groups)
 
 # Test other iterables than list
 groups = ( "abc", "def" )
 el = pyhbac.HbacRuleElement()
-self.assertItemsEqual(el.groups, [])
+self.assertCountEqual(el.groups, [])
 el.groups = groups
-self.assertItemsEqual(el.groups, groups)
+self.assertCountEqua

[SSSD] Re: [PATCH] tests: use unittest.TestCase.assertCountEqual if possible

[Lukas Slebodnik]

On (12/01/16 10:11), Jakub Hrozek wrote:
>On Thu, Dec 10, 2015 at 01:44:26PM +0100, Lukas Slebodnik wrote:
>> On (10/12/15 13:36), Petr Cech wrote:
>> >On 12/10/2015 01:27 PM, Lukas Slebodnik wrote:
>> >>On (10/12/15 12:54), Petr Cech wrote:
>> >>>On 11/23/2015 03:09 PM, Jakub Hrozek wrote:
>> On Wed, Nov 11, 2015 at 12:26:41PM +0100, Jakub Hrozek wrote:
>> >When Python3 was used, the tests used our own compat function for
>> >checking sequences. It's better to stick to the standard-library
>> >provided methods.
>> >
>> >More details are in the commit message..
>> 
>> Sorry, the patch wasn't correct, a new one is attached.
>> 
>> >>>
>> >>>Hi Jakub,
>> >>>
>> >>>I have done review. Code looks good to me and I catched this:
>> >>>
>> >>>http://sssd-ci.duckdns.org/logs/job/34/36/summary.html
>> >>>
>> >>>I asked the CI list... ok, it is not connected to your patch, so:
>> >>>
>> >>CI passed but new pep8 warnings were introduced.
>> >>We get rid of them in integration best.
>> >>But we needn't introduce new in other python files.
>> >>
>> >>sh$ git diff HEAD~..HEAD | pep8 --diff
>> >>./src/tests/pyhbac-test.py:17:1: E302 expected 2 blank lines, found 1
>> >>./src/tests/pyhbac-test.py:20:1: E302 expected 2 blank lines, found 1
>> >>./src/tests/pyhbac-test.py:39:1: E302 expected 2 blank lines, found 1
>> >>./src/tests/pyhbac-test.py:75:1: E302 expected 2 blank lines, found 1
>> >>./src/tests/pyhbac-test.py:83:18: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:83:31: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:87:19: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:87:32: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:92:18: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:92:31: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:98:19: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:98:32: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:105:19: E201 whitespace after '('
>> >>./src/tests/pyhbac-test.py:105:32: E202 whitespace before ')'
>> >>./src/tests/pyhbac-test.py:138:42: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:138:55: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:139:43: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:139:56: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:226:80: E501 line too long (90 > 79 characters)
>> >>./src/tests/pyhbac-test.py:236:50: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:236:63: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:237:51: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:237:64: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:269:41: E201 whitespace after '('
>> >>./src/tests/pyhbac-test.py:272:78: E202 whitespace before ')'
>> >>./src/tests/pyhbac-test.py:272:80: E501 line too long (80 > 79 characters)
>> >>./src/tests/pyhbac-test.py:279:41: E201 whitespace after '('
>> >>./src/tests/pyhbac-test.py:280:78: E202 whitespace before ')'
>> >>./src/tests/pyhbac-test.py:280:80: E501 line too long (80 > 79 characters)
>> >>./src/tests/pyhbac-test.py:282:29: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:282:37: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:288:1: E302 expected 2 blank lines, found 1
>> >>./src/tests/pyhbac-test.py:299:19: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:299:32: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:310:19: E201 whitespace after '['
>> >>./src/tests/pyhbac-test.py:310:32: E202 whitespace before ']'
>> >>./src/tests/pyhbac-test.py:317:19: E201 whitespace after '('
>> >>./src/tests/pyhbac-test.py:317:32: E202 whitespace before ')'
>> >>./src/tests/pysss_murmur-test.py:29:28: E261 at least two spaces before 
>> >>inline comment
>> >>./src/tests/pysss_murmur-test.py:29:29: E262 inline comment should start 
>> >>with '# '
>> >>./src/tests/pysss_murmur-test.py:31:1: E302 expected 2 blank lines, found 1
>> >>
>> >>LS
>> >
>> >Well, that seems to be valid point. I take back my ACK.
>> >Lukas, I see that you run PEP8 locally. Is there plan to add PEP8 to CI?
>> >
>> we have many pep8 warnings in old python code
>> https://fedorahosted.org/sssd/ticket/2774
>> 
>> that's the reason why I shared command how to checked newly introduced
>> warnings.
>> git diff HEAD~..HEAD | pep8 --diff
>> 
>> Feel free to integrate this command to CI.
>> 
>
>I fixed the new pep8 warnings in the first patch and all the others in
>that module in the second patch.
>
>Thanks for the review.

>From 89e56266a17661d6219107912b4501fbb1071bf2 Mon Sep 17 00:00:00 2001
>From: Sumit Bose 
>Date: Mon, 21 Dec 2015 15:51:09 +0100
>Subject: [PATCH] ldap: remove originalMeberOf if there is no memberOf
>
Have you sent the correct patch ?

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

[SSSD] [PATCHES] UTIL: Provide varargs version of debug_fn

[Lukas Slebodnik]

ehlo,

The main reason for these patch was to improve
recently added logging to hbac.

Side effect of these change is improvement for libldb
and libsemanage (6th patch)

4th patch is not API/ABI change because
such version has not beeen released yet.
If you do not like change in hbac callback
hbac_debug_fn_t then we should also remove
because it is too internal then we should
remove also the first two arguments.
"file", "line" also leaks internal data from libhbac.
Removing the first two arguments would be almost
consistent callbacks in libldb and libsemanage.

LS
>From d75eb5652b8740163c11f866bcb128a71913c6c9 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 11 Jan 2016 18:54:40 +0100
Subject: [PATCH 1/6] UTIL: Use prefix for debug function

---
 src/providers/ipa/ipa_access.c |  4 ++--
 src/util/debug.c   | 12 ++--
 src/util/sss_semanage.c|  2 +-
 src/util/util.h| 16 
 4 files changed, 17 insertions(+), 17 deletions(-)

diff --git a/src/providers/ipa/ipa_access.c b/src/providers/ipa/ipa_access.c
index 
65a791c3fe4b56d0f50d6e501a69d6d4b13f1d9a..e6028ed8f50f524bd837ec9341773f88aa2a5a2b
 100644
--- a/src/providers/ipa/ipa_access.c
+++ b/src/providers/ipa/ipa_access.c
@@ -77,8 +77,8 @@ void hbac_debug_messages(const char *file, int line,
 return;
 }
 
-debug_fn(__FILE__, __LINE__, "hbac", loglevel, "[%s:%i] %s",
- file, line, message);
+sss_debug_fn(__FILE__, __LINE__, "hbac", loglevel, "[%s:%i] %s",
+ file, line, message);
 free(message);
 }
 }
diff --git a/src/util/debug.c b/src/util/debug.c
index 
a8eea32740155ec3daf6be71ef9a8af6592f74a9..c0a7732e90a1727f7c9514ad7ee6daf426b6075b
 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -206,11 +206,11 @@ journal_done:
 }
 #endif /* WiTH_JOURNALD */
 
-void debug_fn(const char *file,
-  long line,
-  const char *function,
-  int level,
-  const char *format, ...)
+void sss_debug_fn(const char *file,
+  long line,
+  const char *function,
+  int level,
+  const char *format, ...)
 {
 va_list ap;
 struct timeval tv;
@@ -301,7 +301,7 @@ void ldb_debug_messages(void *context, enum ldb_debug_level 
level,
 }
 
 if (DEBUG_IS_SET(loglevel))
-debug_fn(__FILE__, __LINE__, "ldb", loglevel, "%s\n", message);
+sss_debug_fn(__FILE__, __LINE__, "ldb", loglevel, "%s\n", message);
 
 free(message);
 }
diff --git a/src/util/sss_semanage.c b/src/util/sss_semanage.c
index 
d1d03988c05dc011dbd465051d50fe6acca4f845..4fb9df589bbfddcc815ed321b6e3b32655d44a0c
 100644
--- a/src/util/sss_semanage.c
+++ b/src/util/sss_semanage.c
@@ -64,7 +64,7 @@ static void sss_semanage_error_callback(void *varg,
 }
 
 if (DEBUG_IS_SET(level))
-debug_fn(__FILE__, __LINE__, "libsemanage", level, "%s\n", message);
+sss_debug_fn(__FILE__, __LINE__, "libsemanage", level, "%s\n", 
message);
 free(message);
 }
 
diff --git a/src/util/util.h b/src/util/util.h
index 
e1245bb0fbab742dd58522f1892c66d08a45b59c..662d786ad362027e2f816688a02fd6fab36df14c
 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -77,11 +77,11 @@ extern int debug_microseconds;
 extern int debug_to_file;
 extern int debug_to_stderr;
 extern const char *debug_log_file;
-void debug_fn(const char *file,
-  long line,
-  const char *function,
-  int level,
-  const char *format, ...) SSS_ATTRIBUTE_PRINTF(5,6);
+void sss_debug_fn(const char *file,
+  long line,
+  const char *function,
+  int level,
+  const char *format, ...) SSS_ATTRIBUTE_PRINTF(5, 6);
 int debug_convert_old_level(int old_level);
 errno_t set_debug_file_from_fd(const int fd);
 int get_fd_from_debug_file(void);
@@ -135,9 +135,9 @@ int get_fd_from_debug_file(void);
 #define DEBUG(level, format, ...) do { \
 int __debug_macro_level = level; \
 if (DEBUG_IS_SET(__debug_macro_level)) { \
-debug_fn(__FILE__, __LINE__, __FUNCTION__, \
- __debug_macro_level, \
- format, ##__VA_ARGS__); \
+sss_debug_fn(__FILE__, __LINE__, __FUNCTION__, \
+ __debug_macro_level, \
+ format, ##__VA_ARGS__); \
 } \
 } while (0)
 
-- 
2.5.0

>From 4a605f25824f51782322615399e696b4ed4e1332 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik 
Date: Mon, 11 Jan 2016 11:06:22 +0100
Subject: [PATCH 2/6] UTIL: Provide varargs version of debug_fn

---
 src/util/debug.c | 29 +++--
 src/util/util.h  |  6 ++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/src/util/debug.c b/src/util/debug.c
index 
c0a7732e90a1727f7c9514ad7ee6daf426b6075b..3dcfbf008b61d1611eb72e0ee0f1009a13dda6c4
 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -206,13

[SSSD] [PATCH] DP: Print warning when the handler is not configured

[Jakub Hrozek]

Hi,

the attached patch fixes a bug spotted by Lukas.
>From 0e2e48102aec5d7ffb15aeec23830e595bc91961 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek 
Date: Mon, 11 Jan 2016 14:56:41 +0100
Subject: [PATCH] DP: Print warning when the handler is not configured

We would previously only print the generic warning, not the
user-supplied error message.
---
 src/providers/data_provider_be.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/providers/data_provider_be.c b/src/providers/data_provider_be.c
index 
41680c73ae6305aa4c9379c7545ab88afdb7ba48..a653acd44153e0c5c433be7c6153011b4bdd0c4f
 100644
--- a/src/providers/data_provider_be.c
+++ b/src/providers/data_provider_be.c
@@ -272,7 +272,9 @@ static errno_t be_sbus_reply(struct sbus_request *sbus_req,
 safe_err_msg = safe_be_req_err_msg(err_msg, err_maj);
 
 if (err_maj == DP_ERR_FATAL && err_min == ENODEV) {
-DEBUG(SSSDBG_TRACE_LIBS, "Handler not configured\n");
+DEBUG(SSSDBG_TRACE_LIBS,
+  "Cannot handle request: %s",
+  err_msg ? err_msg : "Handler not configured\n");
 } else {
 DEBUG(SSSDBG_TRACE_LIBS,
   "Request processed. Returned [%s]:%d,%d,%s\n",
-- 
2.4.3

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

[SSSD] Re: [PATCH] tests: use unittest.TestCase.assertCountEqual if possible

[Jakub Hrozek]

On Thu, Dec 10, 2015 at 01:44:26PM +0100, Lukas Slebodnik wrote:
> On (10/12/15 13:36), Petr Cech wrote:
> >On 12/10/2015 01:27 PM, Lukas Slebodnik wrote:
> >>On (10/12/15 12:54), Petr Cech wrote:
> >>>On 11/23/2015 03:09 PM, Jakub Hrozek wrote:
> On Wed, Nov 11, 2015 at 12:26:41PM +0100, Jakub Hrozek wrote:
> >When Python3 was used, the tests used our own compat function for
> >checking sequences. It's better to stick to the standard-library
> >provided methods.
> >
> >More details are in the commit message..
> 
> Sorry, the patch wasn't correct, a new one is attached.
> 
> >>>
> >>>Hi Jakub,
> >>>
> >>>I have done review. Code looks good to me and I catched this:
> >>>
> >>>http://sssd-ci.duckdns.org/logs/job/34/36/summary.html
> >>>
> >>>I asked the CI list... ok, it is not connected to your patch, so:
> >>>
> >>CI passed but new pep8 warnings were introduced.
> >>We get rid of them in integration best.
> >>But we needn't introduce new in other python files.
> >>
> >>sh$ git diff HEAD~..HEAD | pep8 --diff
> >>./src/tests/pyhbac-test.py:17:1: E302 expected 2 blank lines, found 1
> >>./src/tests/pyhbac-test.py:20:1: E302 expected 2 blank lines, found 1
> >>./src/tests/pyhbac-test.py:39:1: E302 expected 2 blank lines, found 1
> >>./src/tests/pyhbac-test.py:75:1: E302 expected 2 blank lines, found 1
> >>./src/tests/pyhbac-test.py:83:18: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:83:31: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:87:19: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:87:32: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:92:18: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:92:31: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:98:19: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:98:32: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:105:19: E201 whitespace after '('
> >>./src/tests/pyhbac-test.py:105:32: E202 whitespace before ')'
> >>./src/tests/pyhbac-test.py:138:42: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:138:55: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:139:43: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:139:56: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:226:80: E501 line too long (90 > 79 characters)
> >>./src/tests/pyhbac-test.py:236:50: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:236:63: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:237:51: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:237:64: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:269:41: E201 whitespace after '('
> >>./src/tests/pyhbac-test.py:272:78: E202 whitespace before ')'
> >>./src/tests/pyhbac-test.py:272:80: E501 line too long (80 > 79 characters)
> >>./src/tests/pyhbac-test.py:279:41: E201 whitespace after '('
> >>./src/tests/pyhbac-test.py:280:78: E202 whitespace before ')'
> >>./src/tests/pyhbac-test.py:280:80: E501 line too long (80 > 79 characters)
> >>./src/tests/pyhbac-test.py:282:29: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:282:37: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:288:1: E302 expected 2 blank lines, found 1
> >>./src/tests/pyhbac-test.py:299:19: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:299:32: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:310:19: E201 whitespace after '['
> >>./src/tests/pyhbac-test.py:310:32: E202 whitespace before ']'
> >>./src/tests/pyhbac-test.py:317:19: E201 whitespace after '('
> >>./src/tests/pyhbac-test.py:317:32: E202 whitespace before ')'
> >>./src/tests/pysss_murmur-test.py:29:28: E261 at least two spaces before 
> >>inline comment
> >>./src/tests/pysss_murmur-test.py:29:29: E262 inline comment should start 
> >>with '# '
> >>./src/tests/pysss_murmur-test.py:31:1: E302 expected 2 blank lines, found 1
> >>
> >>LS
> >
> >Well, that seems to be valid point. I take back my ACK.
> >Lukas, I see that you run PEP8 locally. Is there plan to add PEP8 to CI?
> >
> we have many pep8 warnings in old python code
> https://fedorahosted.org/sssd/ticket/2774
> 
> that's the reason why I shared command how to checked newly introduced
> warnings.
> git diff HEAD~..HEAD | pep8 --diff
> 
> Feel free to integrate this command to CI.
> 

I fixed the new pep8 warnings in the first patch and all the others in
that module in the second patch.

Thanks for the review.
>From 89e56266a17661d6219107912b4501fbb1071bf2 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Mon, 21 Dec 2015 15:51:09 +0100
Subject: [PATCH] ldap: remove originalMeberOf if there is no memberOf

Since originalMemerberOf is not mapped directly to an original attribute
and is handled specially it is not automatically removed if there is no
memberOf in the original object anymore. This patch put
originalMemerberOf on the list of attribute which should be removed in
that case.

Resolves https://fedorahosted.org/sssd/ticket/2917
---
 src/providers/i

[SSSD] Re: [PATCH] ldap: remove originalMeberOf if there is no memberOf

[Jakub Hrozek]

On Mon, Jan 11, 2016 at 11:39:06AM +0100, Jakub Hrozek wrote:
> On Thu, Jan 07, 2016 at 06:54:32PM +0100, Sumit Bose wrote:
> > Hi,
> > 
> > this patch should solve https://fedorahosted.org/sssd/ticket/2917 by
> > properly removing originalMemberOf if there is no memberOf in the
> > original object anymore.
> > 
> > bye,
> > Sumit
> 
> ACK
> 
> good catch with the "+2" when allocating pointers array in
> list_missing_attrs()..
> 
> CI: http://sssd-ci.duckdns.org/logs/job/35/27/summary.html

master: 9a2f018c0f68a3ada4cea4128a861a7f85893f22
sssd-1-13: 93b758232f57fb02ab4f9208f997448668f289f8
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] TEST_TOOLS_COLONDB: Add tests for sss_colondb_* public API

[Petr Cech]

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