[SSSD] Re: [PATCH] util-tests: Initialize boolean variable to default value

2016-01-14 Thread Lukas Slebodnik
On (08/04/15 11:35), Jakub Hrozek wrote:
>On Wed, Apr 08, 2015 at 11:10:42AM +0200, Pavel Reichl wrote:
>> On 04/07/2015 04:33 PM, Lukas Slebodnik wrote:
>> >ehlo,
>> >
>> >The boolean variable found_nss could be used uninitialized
>> >in test test_known_service if service "nss" would not be found.
>> >We would catch it with valgind.
>> >
>> >Simple patch is attached.
>> >
>> >LS
>> >
>> >
>> >___
>> >sssd-devel mailing list
>> >sssd-devel@lists.fedorahosted.org
>> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> Obvious ACK, ci passed:
>> 
>> http://sssd-ci.duckdns.org/logs/job/12/60/summary.html
>> 
>
>* master: 2f84032c28ac414b77a5c1ad470eb69ed6c6a1b4

* sssd-1-12: d0fd102be43ccfd95f2d189fcdceb443fdfdafcd

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


[SSSD] Re: [PATCH] util-tests: Initialize boolean variable to default value

2016-01-14 Thread Lukas Slebodnik
On (08/04/15 15:25), Jakub Hrozek wrote:
>On Wed, Apr 08, 2015 at 03:17:42PM +0200, Lukas Slebodnik wrote:
>> ACK
>> 
>> http://sssd-ci.duckdns.org/logs/job/12/69/summary.html
>> 
>> LS
>
>Thanks for the review, pushed to master:
>e11b9f85b5ad0454cdf3828c4876ec7de3f4799a

* sssd-1-12: 573c25869df246a48a1a82b282b71061851d4d87

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


[SSSD] Re: [PATCH] KRB5: Adding DNS SRV lookup for krb5 provider

2016-01-14 Thread Jakub Hrozek
On Mon, Jan 11, 2016 at 02:55:57PM +0100, Petr Cech wrote:
> On 01/11/2016 02:03 PM, Pavel Březina wrote:
> >On 01/11/2016 12:43 PM, Petr Cech wrote:
> >Hi,
> >I believe the code belongs to sssm_krb5_auth_init.
> 
> Right, it is working the same way. I moved DNS SRV Lookup adding into
> sssm_krb5_auth_init after all other stuff.
> 
> -- 
> Petr^4 Cech

The code looks good to me and now a combination of id_provider=proxy and
SRV lookups in auth_provider=krb5 work well, too.

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


[SSSD] Re: [PATCH] Improve negcache with subdomains

2016-01-14 Thread Lukas Slebodnik
On (09/04/15 08:47), Jakub Hrozek wrote:
>On Wed, Apr 08, 2015 at 07:46:04PM +0200, Lukas Slebodnik wrote:
>> >Thank you for the review, new patches are attached.
>> 
>> ACK
>> http://sssd-ci.duckdns.org/logs/job/12/74/summary.html
>> 
>> LS
>
>master:
>* da3fcbec493dd8d7f5af1d6c6be2a37440a1442e
>* 0528fdec17d0031996e919fcd852459e86592c35
>* 0d19785f9ffd9c66df5b30d208ec7b0216a9555b
>* 1aa492ce890f362564bfac21f3cfb0a3e38608bd
>* d338bb46b8c03c33e6182e725911af6d778bcf00

sssd-1-12:
* 97a952f98891f3d17f08114683c9bbd7be19f5ad
* 8d7073a515bdfccda3b39f3f54893760511dc5f6
* 37b653e18c122bc939e7d984e18d165d280b6c6c
* 4d4d62412febe27c0f5820b7b96141ec2d9f0117
* 3717200fb0acaa8514fd90fcc892ab7f65bec20c

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

2016-01-14 Thread Jakub Hrozek
On Tue, Jan 12, 2016 at 04:27:16PM +0100, Lukas Slebodnik wrote:
> 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

> +%dir %{_libdir}/cifs-utils
>  %{_libdir}/cifs-utils/cifs_idmap_sss.so
> +%dir %{_sysconfdir}/cifs-utils

Why do we need to own these? On my system, they are already owned by
cifs-utils..
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


[SSSD] Re: [PATCH] SDAP: do not fail if refs are found but not processed

2016-01-14 Thread Pavel Březina

On 01/13/2016 03:45 PM, Stephen Gallagher wrote:

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/13/2016 07:25 AM, Pavel Březina wrote:

https://fedorahosted.org/sssd/ticket/2906

Hi, I'm CCing Stephen as he is original author of the code.

Without this patch I am not able to work with AD when
ldap_referrals=true, with this patch it works. I'm not sure though
if it is a correct fix or if we can find a better one.

As the removed comment says the referrals should be processed by
openldap so why aren't they?


So, I never actually tested that particular code-path, I think.
Looking at the code now, I think I see the mistake.

There are two cases where the openldap lookup can return referrals.
One is the case where the result code of the lookup is LDAP_REFERRAL.
That is the situation where the LDAP server has responded that ALL
results must come from somewhere else (equivalent to HTTPs 304 code).
However, there's also the case where we get LDAP_SUCCESS which also
has some referrals that may contain *additional* results. This is the
codepath that you're hitting with this bug.

Now, I have no idea why the openldap libraries actually return the
referral information on this codepath, since in theory it is just
supposed to follow it magically under the hood, but I have some
suspicions.

Among many other abominations, AD's returned referral URIs are often
unfollowable. Instead of returning referrals to specific systems, they
return referrals whose hostname is simply the domain in question.
(Which means that the client is randomly sent to one of the domain
controllers that serve that domain). However, Windows machines do some
sort of magic under the hood in order to have the TLS certificates
actually work in that case. So my guess here is that when openldap
discovers it cannot *actually* process the referral, rather than
failing it just returns it as extra information so the application can
attempt to solve it on its own.

I should also note that OpenLDAP upstream discourages the use of the
internal referral chasing as it rarely works properly.

In conclusion: go ahead and remove that section of code entirely.
While you're there, could you fix a minor memory bug as well? I forgot
to talloc_free(refs) on the success case. It's not a major issue since
it's allocated on a request that almost certainly gets freed soon
after this function returns, but since the memory is no longer used
after this (we don't return it), it's a good convention to clear it here


Patch is attached.


From 5cfb45d8120db00f7bd277ce3c86fef724eb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pavel=20B=C5=99ezina?= 
Date: Wed, 13 Jan 2016 13:15:09 +0100
Subject: [PATCH] SDAP: do not fail if refs are found but not processed

It is possible to end up with not-processed referrals when
using AD provider and ldap_referrals=true.

Resolves:
https://fedorahosted.org/sssd/ticket/2906
---
 src/providers/ldap/sdap_async.c | 11 +--
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 668bd7b465bbfefad13ab0b7061cd16a05dfbef1..5260aafebf7570291876b2433dbcf44ffb5b0011 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1653,16 +1653,6 @@ static void generic_ext_search_handler(struct tevent_req *subreq,
 }
 
 if (ref_count > 0) {
-if (dp_opt_get_bool(opts->basic, SDAP_REFERRALS)) {
-/* We got back referrals here, but they should have
- * been processed internally by openldap libs.
- * This should never happen.
- */
-talloc_free(refs);
-tevent_req_error(req, EINVAL);
-return;
-}
-
 /* We will ignore referrals in the generic handler */
 DEBUG(SSSDBG_TRACE_ALL,
   "Request included referrals which were ignored.\n");
@@ -1674,6 +1664,7 @@ static void generic_ext_search_handler(struct tevent_req *subreq,
 }
 }
 
+talloc_free(refs);
 tevent_req_done(req);
 }
 
-- 
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] SPEC: Move libsss_sudo.so outside sssd-common

2016-01-14 Thread Jakub Hrozek
On Mon, Jan 11, 2016 at 12:22:19PM +0100, Lukas Slebodnik wrote:
> On (11/01/16 10:26), Jakub Hrozek wrote:
> >On Wed, Jan 06, 2016 at 04:48:00PM +0100, Lukas Slebodnik wrote:
> >> ehlo,
> >> 
> >> This change is required to reduce dependency tree in client
> >> container (or on atomic host, ...)
> >> 
> >> Patch is attached.
> >> 
> >> LS
> >
> >I have some questions..
> >
> >1) Why not just move the file ownership to sssd-common? Are you
> >concerned that not everyone needs sudo and it would be an extra file on
> >the filesystem?
> >
> It was in sssd-common but it should be moved outside.
> I dnifn't notice I forgot to do that :-)
> 
> >2) The src/sss_client/sudo/sss_sudo.c only lists GPLv3, not LGPLv3, are
> >you sure adding the lesser copying is correct?
> >
> I didn't check header of file. I used the same license as in was in 1.9.x
> Fixed.
> 
> >3) I think this is a good opportunity to move the library outside
> >libdir, at least in F-24 (this would require coordination with sudo and
> >adding versioned Requires)
> They are unrelated changes and shoudl be discussed with sudo maintainer.
> BTW atm sssd-common(and thus libsss_sudo.so) does not depend on sudo.
> 
> Updated patch is attached.
> 
> LS

> From 32b9f09e54f71747bf7d9efc00e36df12ee0640c Mon Sep 17 00:00:00 2001
> From: Lukas Slebodnik 
> Date: Wed, 6 Jan 2016 16:32:55 +0100
> Subject: [PATCH] SPEC: Move libsss_sudo.so outside sssd-common
> 
> The module ${libdir}/libsss_sudo.so is used only by /usr/bin/sudo.
> If libsss_sudo.so was part of sssd-client then 32 bit version would
> never be used on 64 bit machine and files in sssd-client can be used
> by multilib applications e.g. libnss_sss.so can be indirectly "dlopened"
> by 64 bit applications and 32 bit application.
> (32-bit web browser; ordinary 64bit applications ...)
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2855
> (cherry picked from commit 64c256abfaa4377e995eff505b0fd9f10215113e)

ACK

the patch looks good and upgrade pulls in libsss-common
___
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

2016-01-14 Thread Lukas Slebodnik
On (14/01/16 11:16), Jakub Hrozek wrote:
>On Tue, Jan 12, 2016 at 04:27:16PM +0100, Lukas Slebodnik wrote:
>> 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
>
>> +%dir %{_libdir}/cifs-utils
>>  %{_libdir}/cifs-utils/cifs_idmap_sss.so
>> +%dir %{_sysconfdir}/cifs-utils
>
>Why do we need to own these? On my system, they are already owned by
>cifs-utils..
Because you do not have a minimal system
and sssd-client does not depend on cifs-utils.
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

[root@host /]# rpm -q sssd-client cifs-utils
sssd-client-1.13.1-3.fc23.x86_64
package cifs-utils is not installed

[root@host /]# rpm -qf /etc/cifs-utils/ /etc/cifs-utils/idmap-plugin
file /etc/cifs-utils is not owned by any package
sssd-client-1.13.1-3.fc23.x86_64

[root@host /]# rpm -qf /usr/lib64/cifs-utils/  
/usr/lib64/cifs-utils/cifs_idmap_sss.so
file /usr/lib64/cifs-utils is not owned by any package
sssd-client-1.13.1-3.fc23.x86_64

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


[SSSD] Re: [PATCH] SPEC: Move libsss_sudo.so outside sssd-common

2016-01-14 Thread Lukas Slebodnik
On (14/01/16 11:20), Jakub Hrozek wrote:
>On Mon, Jan 11, 2016 at 12:22:19PM +0100, Lukas Slebodnik wrote:
>> On (11/01/16 10:26), Jakub Hrozek wrote:
>> >On Wed, Jan 06, 2016 at 04:48:00PM +0100, Lukas Slebodnik wrote:
>> >> ehlo,
>> >> 
>> >> This change is required to reduce dependency tree in client
>> >> container (or on atomic host, ...)
>> >> 
>> >> Patch is attached.
>> >> 
>> >> LS
>> >
>> >I have some questions..
>> >
>> >1) Why not just move the file ownership to sssd-common? Are you
>> >concerned that not everyone needs sudo and it would be an extra file on
>> >the filesystem?
>> >
>> It was in sssd-common but it should be moved outside.
>> I dnifn't notice I forgot to do that :-)
>> 
>> >2) The src/sss_client/sudo/sss_sudo.c only lists GPLv3, not LGPLv3, are
>> >you sure adding the lesser copying is correct?
>> >
>> I didn't check header of file. I used the same license as in was in 1.9.x
>> Fixed.
>> 
>> >3) I think this is a good opportunity to move the library outside
>> >libdir, at least in F-24 (this would require coordination with sudo and
>> >adding versioned Requires)
>> They are unrelated changes and shoudl be discussed with sudo maintainer.
>> BTW atm sssd-common(and thus libsss_sudo.so) does not depend on sudo.
>> 
>> Updated patch is attached.
>> 
>> LS
>
>> From 32b9f09e54f71747bf7d9efc00e36df12ee0640c Mon Sep 17 00:00:00 2001
>> From: Lukas Slebodnik 
>> Date: Wed, 6 Jan 2016 16:32:55 +0100
>> Subject: [PATCH] SPEC: Move libsss_sudo.so outside sssd-common
>> 
>> The module ${libdir}/libsss_sudo.so is used only by /usr/bin/sudo.
>> If libsss_sudo.so was part of sssd-client then 32 bit version would
>> never be used on 64 bit machine and files in sssd-client can be used
>> by multilib applications e.g. libnss_sss.so can be indirectly "dlopened"
>> by 64 bit applications and 32 bit application.
>> (32-bit web browser; ordinary 64bit applications ...)
>> 
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2855
>> (cherry picked from commit 64c256abfaa4377e995eff505b0fd9f10215113e)
>
>ACK
>
>the patch looks good and upgrade pulls in libsss-common

master:
* 5e532ad5c1326c68caa4914c43663677dabf03b3

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


[SSSD] Re: [PATCH] SDAP: handle ret properly in ldap_get_options()

2016-01-14 Thread Jakub Hrozek
On Wed, Jan 13, 2016 at 11:59:45AM +0100, Pavel Březina wrote:
> Just found this when working on other stuff...

ACK (Waiting for CI before pushing..)
___
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

2016-01-14 Thread Lukas Slebodnik
On (14/01/16 11:22), Lukas Slebodnik wrote:
>On (14/01/16 11:16), Jakub Hrozek wrote:
>>On Tue, Jan 12, 2016 at 04:27:16PM +0100, Lukas Slebodnik wrote:
>>> 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
>>
>>> +%dir %{_libdir}/cifs-utils
>>>  %{_libdir}/cifs-utils/cifs_idmap_sss.so
>>> +%dir %{_sysconfdir}/cifs-utils
>>
>>Why do we need to own these? On my system, they are already owned by
>>cifs-utils..
>Because you do not have a minimal system
>and sssd-client does not depend on cifs-utils.
>https://fedoraproject.org/wiki/Packaging:UnownedDirectories
>
>[root@host /]# rpm -q sssd-client cifs-utils
>sssd-client-1.13.1-3.fc23.x86_64
>package cifs-utils is not installed
>
>[root@host /]# rpm -qf /etc/cifs-utils/ /etc/cifs-utils/idmap-plugin
>file /etc/cifs-utils is not owned by any package
>sssd-client-1.13.1-3.fc23.x86_64
>
>[root@host /]# rpm -qf /usr/lib64/cifs-utils/  
>/usr/lib64/cifs-utils/cifs_idmap_sss.so
>file /usr/lib64/cifs-utils is not owned by any package
>sssd-client-1.13.1-3.fc23.x86_64
>
Attached is a patch which fixes conflict due to libsss_sudo changes

LS
>From f622ea0734160239e9caf6a19ece493aff3fc3a8 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 
13ea532041bfd2d354a36ce2975633fbd0516363..76066de9ac8b8ac1f9b14bf3bc787169d317fe2f
 100644
--- a/contrib/sssd.spec.in
+++ b/contrib/sssd.spec.in
@@ -613,6 +613,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 {} \;
 
@@ -706,6 +712,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
 
@@ -723,7 +731,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}/libnfsidmap/sss.so
 
 %{ldb_modulesdir}/memberof.so
@@ -740,9 +749,9 @@ rm -rf $R

[SSSD] [PATCH] p11: add gnome-screensaver to list of allowed services

2016-01-14 Thread Sumit Bose
Hi,

this simple patch aims to fix https://fedorahosted.org/sssd/ticket/2925
which is needed for older platforms like CentOS6 or RHEL6 to allow to
unlock the screen with the help of a Smartcard as well.

To make it more easy changeable in future I added
https://fedorahosted.org/sssd/ticket/2926 as a reminder for me.

bye,
Sumit
From bae5361041edad65e4d5f83a30d85ca10af1a6c5 Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Thu, 14 Jan 2016 11:42:26 +0100
Subject: [PATCH] p11: add gnome-screensaver to list of allowed services

Resolves https://fedorahosted.org/sssd/ticket/2925
---
 src/responder/pam/pamsrv_p11.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 
58310a2530287fc6d08a7195c8e879f96dcc5403..bcac439dde57bda2adeab07a4123672060be9259
 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -45,7 +45,7 @@ bool may_do_cert_auth(struct pam_ctx *pctx, struct pam_data 
*pd)
 size_t c;
 const char *sc_services[] = { "login", "su", "su-l", "gdm-smartcard",
   "gdm-password", "kdm", "sudo", "sudo-i",
-  NULL };
+  "gnome-screensaver", NULL };
 if (!pctx->cert_auth) {
 return false;
 }
-- 
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] fix account lockout reporting with the krb5 provider

2016-01-14 Thread Jakub Hrozek
On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote:
> subj says it all,
> bug: https://fedorahosted.org/sssd/ticket/2924
> 
> I have compiled and run make check|intgcheck but "not" actively tested
> this patch.

I did test the patch by crating an account in AD and then ticking the
"Account is disabled" box. PAM returned error code 6, which is what I'd
expect.

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


[SSSD] Re: [PATCH] p11: add gnome-screensaver to list of allowed services

2016-01-14 Thread Lukas Slebodnik
On (14/01/16 11:53), Sumit Bose wrote:
>Hi,
>
>this simple patch aims to fix https://fedorahosted.org/sssd/ticket/2925
>which is needed for older platforms like CentOS6 or RHEL6 to allow to
>unlock the screen with the help of a Smartcard as well.
>
>To make it more easy changeable in future I added
>https://fedorahosted.org/sssd/ticket/2926 as a reminder for me.
>
Would it be better to do it now?
Because xscreensaver is not on list
or is this feature supported only on gnome?

BTW we already have something similar in gpo and it should not took a days
to implement it :-)

man sssd-ad -> ad_gpo_map_interactive

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


[SSSD] Re: [PATCH] p11: add gnome-screensaver to list of allowed services

2016-01-14 Thread Sumit Bose
On Thu, Jan 14, 2016 at 01:09:39PM +0100, Lukas Slebodnik wrote:
> On (14/01/16 11:53), Sumit Bose wrote:
> >Hi,
> >
> >this simple patch aims to fix https://fedorahosted.org/sssd/ticket/2925
> >which is needed for older platforms like CentOS6 or RHEL6 to allow to
> >unlock the screen with the help of a Smartcard as well.
> >
> >To make it more easy changeable in future I added
> >https://fedorahosted.org/sssd/ticket/2926 as a reminder for me.
> >
> Would it be better to do it now?

In general yes, but I wanted to fix a downstream issue first without
adding additional options.

> Because xscreensaver is not on list
> or is this feature supported only on gnome?

no, of course not, but so far no one complained.

> 
> BTW we already have something similar in gpo and it should not took a days
> to implement it :-)
> 
> man sssd-ad -> ad_gpo_map_interactive

yes, I had the same in mind for the run-time configuration.

bye,
Sumit

> 
> LS
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
___
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

2016-01-14 Thread Pavel Březina

On 01/13/2016 05:12 PM, Sumit Bose wrote:


@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,

  if (ctx == NULL) {
  DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
-ctx->ret = ERR_INTERNAL;
  return false;
  }

@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,

  if (ctx == NULL) {
  DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
-ctx->ret = ERR_INTERNAL;
  return false;
  }



it looks like the last 2 hunks are not available in the latest tar ball.


Sorry about that. Added.


and there are some trailing whitespaces in patch 9 at line 41.


Fixed by git.

I also attached two more patches then converts usn into number and then 
use in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) && 
(entryUSN != cur) as per Thierry request.


It could probably be squashed and replace some previous patches but I 
think sending it as new patches is easier for you to review, given the 
previous patches are basically acked.


patches.tgz
Description: GNU Zip compressed data
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider

2016-01-14 Thread Simo Sorce
On Thu, 2016-01-14 at 12:41 +0100, Jakub Hrozek wrote:
> On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote:
> > subj says it all,
> > bug: https://fedorahosted.org/sssd/ticket/2924
> > 
> > I have compiled and run make check|intgcheck but "not" actively tested
> > this patch.
> 
> I did test the patch by crating an account in AD and then ticking the
> "Account is disabled" box. PAM returned error code 6, which is what I'd
> expect.
> 
> ACK

Did the old code return the wrong error too ?
I haven't looked at the AD case only IPA when I coded it but AD and IPA
share the krb5 provider so I guess testing either would be fine ...

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] fix account lockout reporting with the krb5 provider

2016-01-14 Thread Jakub Hrozek
On Thu, Jan 14, 2016 at 11:03:51AM -0500, Simo Sorce wrote:
> On Thu, 2016-01-14 at 12:41 +0100, Jakub Hrozek wrote:
> > On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote:
> > > subj says it all,
> > > bug: https://fedorahosted.org/sssd/ticket/2924
> > > 
> > > I have compiled and run make check|intgcheck but "not" actively tested
> > > this patch.
> > 
> > I did test the patch by crating an account in AD and then ticking the
> > "Account is disabled" box. PAM returned error code 6, which is what I'd
> > expect.
> > 
> > ACK
> 
> Did the old code return the wrong error too ?

Yes, before the patch I see:
Jan 14 16:23:51 adclient.win.trust.test su[3745]: pam_sss(su-l:auth): received 
for user lockuser: 13 (User account has expired)

With the patched code I see:
Jan 14 16:26:13 adclient.win.trust.test su[27524]: pam_sss(su-l:auth): received 
for user lockuser: 6 (Permission denied)

> I haven't looked at the AD case only IPA when I coded it but AD and IPA
> share the krb5 provider so I guess testing either would be fine ...

Yes, the PAM error codes are exactly the same after "ipa user-disable".

OK to push now?
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


[SSSD] [PATCHES] AD: add task to renew the machine account password if needed

2016-01-14 Thread Sumit Bose
Hi,

this patch adds a task to the AD provider which calls adcli on a regular
basis to update the machine account password if needed. adcli supports
this functionality since version 0.8.0. Adding support other utilities like
msktutil shouldn't be hard.

Since adcli (and other external tools) does not understand the SSSD
default options like --debug_level a change in exec_child_ex() was
needed which is covered in the first patch.

bye,
Sumit
From 86b623111e4ab4fb176c68c6cea2fbf099f7e21a Mon Sep 17 00:00:00 2001
From: Sumit Bose 
Date: Thu, 14 Jan 2016 13:33:53 +0100
Subject: [PATCH 1/2] UTIL: allow to skip default options for child processes

Currently the SSSD default options like e.g. --debug-level are added
unconditionally to the command line options of a child process when
started with the child helper functions.

If a binary from a different source should be started as a child by SSSD
those options might not be known or used differently. This patch adds an
option to exec_child_ex() which allows to skip the default options and
only add specific options.
---
 src/providers/ad/ad_gpo.c   |  2 +-
 src/providers/krb5/krb5_child_handler.c |  2 +-
 src/responder/pam/pamsrv_p11.c  |  2 +-
 src/tests/cmocka/test_child_common.c|  4 +-
 src/util/child_common.c | 73 ++---
 src/util/child_common.h |  2 +-
 6 files changed, 47 insertions(+), 38 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 
cca5e58618bf861c0d754ef1916583a73c32141a..069196c3b616ebf7661ac9ffe577504b6ebcb674
 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -4144,7 +4144,7 @@ gpo_fork_child(struct tevent_req *req)
 if (pid == 0) { /* child */
 err = exec_child_ex(state,
 pipefd_to_child, pipefd_from_child,
-GPO_CHILD, gpo_child_debug_fd, NULL,
+GPO_CHILD, gpo_child_debug_fd, NULL, false,
 STDIN_FILENO, AD_GPO_CHILD_OUT_FILENO);
 DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec gpo_child: [%d][%s].\n",
   err, strerror(err));
diff --git a/src/providers/krb5/krb5_child_handler.c 
b/src/providers/krb5/krb5_child_handler.c
index 
fa1055eb7fc7e9aa6fabef1c1759c272b217a395..167a2b2ad09b67908cdce8051d8a37e557c91545
 100644
--- a/src/providers/krb5/krb5_child_handler.c
+++ b/src/providers/krb5/krb5_child_handler.c
@@ -312,7 +312,7 @@ static errno_t fork_child(struct tevent_req *req)
 err = exec_child_ex(state,
 pipefd_to_child, pipefd_from_child,
 KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd,
-k5c_extra_args, STDIN_FILENO, STDOUT_FILENO);
+k5c_extra_args, false, STDIN_FILENO, 
STDOUT_FILENO);
 if (err != EOK) {
 DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec KRB5 child: 
[%d][%s].\n",
   err, strerror(err));
diff --git a/src/responder/pam/pamsrv_p11.c b/src/responder/pam/pamsrv_p11.c
index 
bcac439dde57bda2adeab07a4123672060be9259..ad1670136dbf8efc41df6950af744ff8b06e6a11
 100644
--- a/src/responder/pam/pamsrv_p11.c
+++ b/src/responder/pam/pamsrv_p11.c
@@ -322,7 +322,7 @@ struct tevent_req *pam_check_cert_send(TALLOC_CTX *mem_ctx,
 child_pid = fork();
 if (child_pid == 0) { /* child */
 ret = exec_child_ex(state, pipefd_to_child, pipefd_from_child,
-P11_CHILD_PATH, child_debug_fd, extra_args,
+P11_CHILD_PATH, child_debug_fd, extra_args, false,
 STDIN_FILENO, STDOUT_FILENO);
 if (ret != EOK) {
 DEBUG(SSSDBG_CRIT_FAILURE, "Could not exec p11 child: [%d][%s].\n",
diff --git a/src/tests/cmocka/test_child_common.c 
b/src/tests/cmocka/test_child_common.c
index 
bf500fa5a1f2b2fe79833e23a53cdf0b06b81260..9ed9c1ae42dd93cef833b738c29259a18e791339
 100644
--- a/src/tests/cmocka/test_child_common.c
+++ b/src/tests/cmocka/test_child_common.c
@@ -139,7 +139,7 @@ void test_exec_child_extra_args(void **state)
 ret = exec_child_ex(child_tctx,
 child_tctx->pipefd_to_child,
 child_tctx->pipefd_from_child,
-CHILD_DIR"/"TEST_BIN, 2, extra_args,
+CHILD_DIR"/"TEST_BIN, 2, extra_args, false,
 STDIN_FILENO, STDOUT_FILENO);
 assert_int_equal(ret, EOK);
 } else {
@@ -287,7 +287,7 @@ void test_exec_child_echo(void **state)
 ret = exec_child_ex(child_tctx,
 child_tctx->pipefd_to_child,
 child_tctx->pipefd_from_child,
-CHILD_DIR"/"TEST_BIN, 2, NULL,
+CHILD_DIR"/"TEST_BIN, 2, NULL, false,
 STDIN_FILENO, 3);
 assert_int_equal(

[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider

2016-01-14 Thread Simo Sorce
On Thu, 2016-01-14 at 17:30 +0100, Jakub Hrozek wrote:
> On Thu, Jan 14, 2016 at 11:03:51AM -0500, Simo Sorce wrote:
> > On Thu, 2016-01-14 at 12:41 +0100, Jakub Hrozek wrote:
> > > On Wed, Jan 13, 2016 at 02:56:25PM -0500, Simo Sorce wrote:
> > > > subj says it all,
> > > > bug: https://fedorahosted.org/sssd/ticket/2924
> > > > 
> > > > I have compiled and run make check|intgcheck but "not" actively tested
> > > > this patch.
> > > 
> > > I did test the patch by crating an account in AD and then ticking the
> > > "Account is disabled" box. PAM returned error code 6, which is what I'd
> > > expect.
> > > 
> > > ACK
> > 
> > Did the old code return the wrong error too ?
> 
> Yes, before the patch I see:
> Jan 14 16:23:51 adclient.win.trust.test su[3745]: pam_sss(su-l:auth): 
> received for user lockuser: 13 (User account has expired)
> 
> With the patched code I see:
> Jan 14 16:26:13 adclient.win.trust.test su[27524]: pam_sss(su-l:auth): 
> received for user lockuser: 6 (Permission denied)
> 
> > I haven't looked at the AD case only IPA when I coded it but AD and IPA
> > share the krb5 provider so I guess testing either would be fine ...
> 
> Yes, the PAM error codes are exactly the same after "ipa user-disable".
> 
> OK to push now?

Yes please :-)

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: [PATCHES] Support IPA sudo schema

2016-01-14 Thread Sumit Bose
On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> On 01/13/2016 05:12 PM, Sumit Bose wrote:
> >>
> >>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> >>
> >>  if (ctx == NULL) {
> >>  DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> >>-ctx->ret = ERR_INTERNAL;
> >>  return false;
> >>  }
> >>
> >>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> >>
> >>  if (ctx == NULL) {
> >>  DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> >>-ctx->ret = ERR_INTERNAL;
> >>  return false;
> >>  }
> >>
> >
> >it looks like the last 2 hunks are not available in the latest tar ball.
> 
> Sorry about that. Added.
> 
> >and there are some trailing whitespaces in patch 9 at line 41.
> 
> Fixed by git.
> 
> I also attached two more patches then converts usn into number and then use
> in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> (entryUSN != cur) as per Thierry request.
> 
> It could probably be squashed and replace some previous patches but I think
> sending it as new patches is easier for you to review, given the previous
> patches are basically acked.

> +errno = 0;
>  usn_number = strtoul(usn, &endptr, 10);
> -if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> - && (usn_number > srv_opts->last_usn)) {
> - srv_opts->last_usn = usn_number;
> +if (endptr != NULL && *endptr != '\0') {

Currently an empty string in usn would result in usn_number==0, I wonder
if this is valid or if it would be better to error out in this case?

Otherwise all looks good, CI and Coverity test are currently running.

bye,
Sumit

> +DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> +return;
> +} else if (errno != 0) {
> +ret = errno;
> +DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> %s\n",
> +  usn, ret, sss_strerror(ret));
> +return;
>  }
> 

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


[SSSD] Re: [PATCH] fix account lockout reporting with the krb5 provider

2016-01-14 Thread Jakub Hrozek
On Thu, Jan 14, 2016 at 12:09:12PM -0500, Simo Sorce wrote:
> > OK to push now?
> 
> Yes please :-)
> 
> Simo

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


[SSSD] Re: [PATCH] SDAP: handle ret properly in ldap_get_options()

2016-01-14 Thread Jakub Hrozek
On Thu, Jan 14, 2016 at 11:34:06AM +0100, Jakub Hrozek wrote:
> On Wed, Jan 13, 2016 at 11:59:45AM +0100, Pavel Březina wrote:
> > Just found this when working on other stuff...
> 
> ACK (Waiting for CI before pushing..)

CI: http://sssd-ci.duckdns.org/logs/job/35/55/summary.html
* master: 383840c0c9c440710352076f844a64745121d251
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


[SSSD] Re: [PATCH] KRB5: Adding DNS SRV lookup for krb5 provider

2016-01-14 Thread Jakub Hrozek
On Thu, Jan 14, 2016 at 10:47:32AM +0100, Jakub Hrozek wrote:
> On Mon, Jan 11, 2016 at 02:55:57PM +0100, Petr Cech wrote:
> > On 01/11/2016 02:03 PM, Pavel Březina wrote:
> > >On 01/11/2016 12:43 PM, Petr Cech wrote:
> > >Hi,
> > >I believe the code belongs to sssm_krb5_auth_init.
> > 
> > Right, it is working the same way. I moved DNS SRV Lookup adding into
> > sssm_krb5_auth_init after all other stuff.
> > 
> > -- 
> > Petr^4 Cech
> 
> The code looks good to me and now a combination of id_provider=proxy and
> SRV lookups in auth_provider=krb5 work well, too.
> 
> ACK

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

* master: 684191e61d891b1c34f3742a40d5a2ed6a1192dd
* sssd-1-13: dd5a52db9653d83bef26da468157c216df45f715 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org


[SSSD] Re: [PATCH] SDAP: do not fail if refs are found but not processed

2016-01-14 Thread Stephen Gallagher
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 01/14/2016 05:19 AM, Pavel Březina wrote:
> On 01/13/2016 03:45 PM, Stephen Gallagher wrote:
>> -BEGIN PGP SIGNED MESSAGE- Hash: SHA1
>> 
>> On 01/13/2016 07:25 AM, Pavel Březina wrote:
>>> https://fedorahosted.org/sssd/ticket/2906
>>> 
>>> Hi, I'm CCing Stephen as he is original author of the code.
>>> 
>>> Without this patch I am not able to work with AD when 
>>> ldap_referrals=true, with this patch it works. I'm not sure
>>> though if it is a correct fix or if we can find a better one.
>>> 
>>> As the removed comment says the referrals should be processed
>>> by openldap so why aren't they?
>> 
>> So, I never actually tested that particular code-path, I think. 
>> Looking at the code now, I think I see the mistake.
>> 
>> There are two cases where the openldap lookup can return
>> referrals. One is the case where the result code of the lookup is
>> LDAP_REFERRAL. That is the situation where the LDAP server has
>> responded that ALL results must come from somewhere else
>> (equivalent to HTTPs 304 code). However, there's also the case
>> where we get LDAP_SUCCESS which also has some referrals that may
>> contain *additional* results. This is the codepath that you're
>> hitting with this bug.
>> 
>> Now, I have no idea why the openldap libraries actually return
>> the referral information on this codepath, since in theory it is
>> just supposed to follow it magically under the hood, but I have
>> some suspicions.
>> 
>> Among many other abominations, AD's returned referral URIs are
>> often unfollowable. Instead of returning referrals to specific
>> systems, they return referrals whose hostname is simply the
>> domain in question. (Which means that the client is randomly sent
>> to one of the domain controllers that serve that domain).
>> However, Windows machines do some sort of magic under the hood in
>> order to have the TLS certificates actually work in that case. So
>> my guess here is that when openldap discovers it cannot
>> *actually* process the referral, rather than failing it just
>> returns it as extra information so the application can attempt to
>> solve it on its own.
>> 
>> I should also note that OpenLDAP upstream discourages the use of
>> the internal referral chasing as it rarely works properly.
>> 
>> In conclusion: go ahead and remove that section of code
>> entirely. While you're there, could you fix a minor memory bug as
>> well? I forgot to talloc_free(refs) on the success case. It's not
>> a major issue since it's allocated on a request that almost
>> certainly gets freed soon after this function returns, but since
>> the memory is no longer used after this (we don't return it),
>> it's a good convention to clear it here
> 
> Patch is attached.
> 
> 

Ack and good catch!
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlaX3hMACgkQeiVVYja6o6NNWQCgh26RPecLGwFpwNvHrxgGA4is
JroAn0X7CeGD5QtYfdEvieQNzDstN0iU
=wmdB
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org