[SSSD] Re: Build for RHEL7

[Stephen Gallagher]

On 05/25/2017 12:16 PM, Joseph Fischetti wrote:
> I was able to build/install/test via copr, which is an awesome resource.
> 
> Thanks Lukas for the suggestion.  I'd still like to get it sorted at some 
> point so I can use a local repository for the package, but this will work for 
> the time being.

Take a look at the 'mock' package (`yum install mock`)

You can do: `mock -r epel-7-x86_64 /path/to/srpm` and it will automatically
download all of the build-dependencies and rebuild the SRPM into binary RPMs
locally on your system. (This is the tool that COPR uses internally).



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: Build for RHEL7

[Stephen Gallagher]

On 05/25/2017 12:04 PM, Lukas Slebodnik wrote:
> On (25/05/17 15:26), Joseph Fischetti wrote:
>> Thanks Lukas, 
>> I did find the optional (and extras) repos, enabled them, etc, etc. 
>>
>> After building from source rpm on a rhel7 machine, rpm/RPMS/x86_64 contains 
>> ~30 rpms.  rpm -Uvh * presents me with "python2-sssdconfig is needed".  For 
>> some reason it isn't among the rpm's that were created when building from 
>> source. 
>>
> Because python2-sssdconfig is not platform specific package
> and therefore is in different directory rpm/RPMS/noarch
> 

Also, on RHEL it is called "python-sssdconfig", not python2-sssdconfig. You will
need to modify the spec file to import the correct name.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: Design document - SSSD KCM server

[Stephen Gallagher]

On 11/22/2016 09:38 AM, Simo Sorce wrote:
> On Tue, 2016-11-22 at 09:23 -0500, Stephen Gallagher wrote:

>> OK, so the service is only semi-socket-activated? If we're keeping tevent 
>> timers
>> around for renewals and reaping, the service won't be exiting unless all 
>> tickets
>> have expired and been reaped.
>>
>> Would it be possible to look into setting non-persistent systemd timer units
>> instead that would "wake up" the sssd_kcm service at appropriate times to do
>> renewal and expiration?
>>
>> systemd provides the CreateTransientUnit() method on its public API that we
>> could use for this purpose. If we did it this way, then the service would 
>> only
>> need to be activated whenever a ticket was actually being retrieved from the
>> collection.
> 
> I am trying to think if this would gain us anything.
> What would you use as a reasonable timeout to decide to exit ?
> There are other events we may want to detect in future. for example we
> may want to decide to destroy all of a user ccaches if all his sessions
> go away, this requires active probing though, but I guess it could also
> be a timer or maybe systemd has a way to notify and start a process when
> this happens too ?
> 

Well, as far as a timeout to exit, I'd probably go with a minute or two (since
you may have a short flurry of activity, such as when a user first connects to a
VPN).

As far as notification of a user signing in or out, we *could* attach a helper
unit to the systemd user session default.target (and have an ExecStop command
for handling when it gets shut down too).

>> How do containers access the KCM? Would they have to run their own copy 
>> internal
>> to the container? Would we bind-mount the /var/run/.heim_org.h5l.kcm-socket 
>> and
>> then work some namespacing magic in the host?
> 
> Deployment specific, I can see either way as an option depending on what
> you are doing.
> 

OK, but the document doesn't describe how that might be done. We should identify
the set of supported approaches up-front and include them in the design.


>> You call out in the introduction that this will help address container
>> use-cases, but then don't describe that implementation. This seems like an
>> important piece of the puzzle that should be added to the page (or made more
>> clear, since if it's in there, I can't spot it).
> 
> What would you want to call out exactly ?
> 

Describe a couple use-cases and the expected user experience for setting them up
and using them. If we bind-mount the host's KCM into the container, would the
user namespacing be handled "magically" by the kernel or do we need to keep
track of which cgroup our client is and sort it into its own section of the
database? (Just for example).




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org
To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org

[SSSD] Re: Design document - SSSD KCM server

[Stephen Gallagher]

Some thoughts inline:

On 11/22/2016 02:51 AM, Jakub Hrozek wrote:

...

> === Implementation details ===
> A new SSSD responder will be added. Since accessing the Kerberos credentials
> is quite an infrequent operation, the responder will be socket-activated.
> 
> This responder would implement the same subset of the KCM protocol the MIT
> client libraries implement. Contrary to Heimdal's KCM server that just
> stores the credential caches in memory, the SSSD KCM server would store
> the ccaches in the secrets database through the sssd-secret's responder
> [https://jhrozek.fedorapeople.org/sssd/1.14.2/man/sssd-secrets.5.html public 
> REST API].
> 
> For user credentials the KCM Server would use a secrets responder URI
> like `/kcm/users/1234/X` where 1234 is the user ID and X is the residual.
> The client then gets assigned a KRB5CCNAME of KCM:1234:X. Internally in the
> secrets responder we will store the credential caches under a new base DN
> `cn=kcm`.
> 
> The secret responder's quota on secrets must be made modular to allow
> different number of secrets per base DN (so, different number of secrets
> and credentials pretty much). What to do in case the quota is reached is
> debatable - we should probably first remove service (non-TGT) tickets first
> for valid TGTs and if that's not possible, just fail. A failure in this
> case would be no different than a failure if a disk is full when trying
> to store a FILE-based ccache.
> 
> The KCM responder would renew the user credentials by starting a tevent
> timer which would then contact the SSSD Data Provider for the given UID
> and principal, asking for the credentials to be renewed. Another tevent
> timer would reap and remove a ccache that reaches its lifetime.
> 


OK, so the service is only semi-socket-activated? If we're keeping tevent timers
around for renewals and reaping, the service won't be exiting unless all tickets
have expired and been reaped.

Would it be possible to look into setting non-persistent systemd timer units
instead that would "wake up" the sssd_kcm service at appropriate times to do
renewal and expiration?

systemd provides the CreateTransientUnit() method on its public API that we
could use for this purpose. If we did it this way, then the service would only
need to be activated whenever a ticket was actually being retrieved from the
collection.


> In the future, SSSD-specific operations such as writing out a FILE-based
> ccache might be added. The SSSD D-Bus interface would also be extended to
> publish information about credentials activity (such as - a ticket being
> acquired, a ticket was renewed etc)


This should be a high priority, since it will benefit tools such as GNOME Online
Accounts greatly (right now, they have to poll the kernel keyring and are not
happy about it; with FILE and DIR caches, they at least could get notifications
via inotify)


> 
> === Configuration changes ===
> No KCM-specific configuration options will be added. The SSSD KCM responder
> would use the same common options like other SSSD services such as idle
> timeout.
> 
> We can add a configurable KCM socket location later, if needed, but for
> the start it's fine to default to `/var/run/.heim_org.h5l.kcm-socket`
> mostly because that's what MIT defaults to as well.
> 
> '''Q''': Should we add an option to explicitly enable ccache renewals and
> default to no renewals? I don't think this would any any security though,
> the attacker can just run 'kinit -R' on behalf of the user anyway.
> 

If the user asked for a renewable ticket in the first place (and the server
permitted it), then I don't see any reason not to just go ahead and renew it by
default.


> === How To Test ===
> In order for the admin to start using the KCM service, the sssd-kcm
> responder's systemd service must be enabled. Then, libkrb5 must also be
> configured to use KCM as its default ccache type in `/etc/krb5.conf`
> {{{
> [libdefaults]
> default_ccache_name = KCM
> }}}
> 
> After that, all common operations like kinit, kdestroy or login through
> pam_sss should just work and store their credentials in the KCM server.
> 
> The KCM server must implement access control correctly, so even
> trying to access other user's KCM credentials by setting KRB5CCNAME to
> `KCM:1234:RESIDUAL` would not work (except for root).
> 
> Restarting the KCM server or rebooting the machine must persist the tickets.
> 
> As far as automatic unit and integration testing is required, we need to make
> sure that MIT's testsuite passes with Kerberos ccache defaulting to KCM
> and SSSD KCM deamon running. In the SSSD upstream, we should write
> integration tests that run a MIT KDC under socket_wrapper to exercise the
> KCM server.
> 

How do containers access the KCM? Would they have to run their own copy internal
to the container? Would we bind-mount the /var/run/.heim_org.h5l.kcm-socket and
then work some namespacing magic in the host?

You call out in the introduction that this will help a

[SSSD] Re: [RFC] Cleaning up the IFP responder (mainly) and socket-activatable responders

[Stephen Gallagher]

On 08/31/2016 01:40 PM, Fabiano Fidêncio wrote:
> Howdy!
> 
> Taking a look on https://fedorahosted.org/sssd/ticket/2395 seems that
> there are a few ways to achieve what's proposed by Simo and I'd like
> to discuss one of those before start implementing it.
> 
> As far as I understand the bug, SSSD cannot be run as a non-root user
> because of IFP responder, which needs to be run as root in order to
> write to sssd.conf file. Simo's suggestion was to create a new
> service, which should be socket-activated, to handle this writing to
> sssd.conf part and Jakub's suggested to completely get rid of this
> part. However, Dmitri pointed out that we must have some service
> playing with sssd.conf as it's the way that cockpit and ansible can
> remotely manage the SSSD configuration.
> 

Well... we do have the SSSDConfigAPI, which is a python API. That's going to
cover the Ansible case, I should think. That said, it would be *really* nice if
we could have a public API that tools like Cockpit or realmd could use (or even
the mythical authconfig rewrite we keep hearing about).

With OpenLMI officially deceased, I don't believe that the code we have
available currently is actually consumed by anyone, so I'm slightly in favor of
just killing it off entirely and implementing the new service if and when we
have a specific use-case that would make it valuable.



> Okay, in order to actually start solving the issue I'd like to propose
> the follow steps:
> 
> 1) Create a new service, still explicitly enabled in sssd.conf, that
> would take care of the managing the SSSD configuration.

Not possible; such a service would need to be capable of generating the initial
configuration if none exists yet. So having explicit enablement in sssd.conf
would be a circular dependency.


> 2) Get rid of the code dealing with the SSSD configuration from the
> IFP responder.


*Grabs the flamethrower*


> 3) Get rid of the OpenLMI specific code from the IFP responder
> 
> Till this point the problem wasn't solved at all, but those seem to be
> the logical steps in order to make our own lives easier for working in
> the next two steps.
> 

*Refuels flamethrower*


> 4) Make the new service (created in 1) socket-activated/bus-activated.

This seems less like another step and more like a design requirement for the
config service, but yes.


> 5) Make the IFP responder also bus-activated/socket-activated.
> 

This is definitely something we want to accomplish, but it seems somewhat out of
place in the current discussion.


> So, does it sound like a reasonable way to approach the problem?
> 
> If yes, I'd like to ask a few questions, mainly about items 4 and 5,
> as it eems that in order to achieve what we want,
> https://fedorahosted.org/sssd/ticket/2243 and
> https://fedorahosted.org/sssd/ticket/3129 have to be solved together,
> which seems to be a not so small task.
> 

I don't think these need to be solved together at all. It can be iterative (we
can use the IFP responder as a proof-of-concept).


> So, do we have to care about systems that don't have systemd support?
> IOW, do we have to keep the current code and also add
> socket-activation? If yes, would it be explicitly enabled/disabled on
> configure time? Maybe a lame question, but what are the advantages of
> the socket-activation over the bus-activation approach (I still didn't
> look at any of those :-\)?
> 

At least in the case of IFP, I think it makes sense to just do things via D-BUS
activation. It will be compatible with any D-BUS system, not just systemd 
platforms.

I think there's a conversation to be had about dropping support for non-systemd
platforms and aligning the responders in general (ticket 2243) with systemd
socket activation for power-consumption reasons, but I think that's a separate
and much larger change than this one needs to be.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] GPO: Cat vals with same key from different GPOs

[Stephen Gallagher]

On 08/31/2016 01:24 PM, Simo Sorce wrote:
> On Wed, 2016-08-31 at 17:41 +0200, Michal Židek wrote:
>> Hi,
>>
>> here is patch for ticket #3161.
>>
>> See more in the ticket description.
>>
>> I was thinking why we originally replaced
>> the lists and I think it comes from confusion
>> on how we handle the same keys in single
>> GPO ini file, however that is handled by
>> libini not by SSSD.
> 
> Sorry to come to this late, but do you have a documentation reference
> that says that merging is the correct behavior ?

Merging is not (nor has it ever been) the correct behavior. The way that AD GPOs
work is with a strict hierarchy. I'd have to double-check the code to remember
which order it is, but I *think* it's:

Domain is overridden by site which is overridden by individual machine.


Basically, if I have a GPO on domain EXAMPLE.COM that include
SeInteractiveLogonRight=foo and another GPO on machine client.example.com that
has SeInteractiveLogonRight="bar, baz", then "bar, baz" completely replaces
"foo" for that machine. It does not merge in any way.


This is the reason that we switched to storing this information in the LDB
cache; we were able to do the processing the same way Windows does with the
registry, by having each iteration through the hierarchy replace the previous 
one.

Again, no merging should happen here.

> I forgot a lot about how multiple GPOs are supposed to be merged but I
> seem to recall there may be a policy that actually controls how merging
> is done.
> 
> CCing Günther who has worked around GPO processing a few years ago.
> 
> Simo.
> 




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] Add support for disabling netlink use

[Stephen Gallagher]

On 08/17/2016 09:17 AM, Justin Stephenson wrote:
> 
> On 08/17/2016 07:34 AM, Lukas Slebodnik wrote:
>> On (16/08/16 09:57), Justin Stephenson wrote:
>>> Thanks for the info, yes please go ahead and squash them.
>>>
>>> Kind regards,
>>> Justin Stephenson
>>>
>>> On 08/16/2016 09:32 AM, Jakub Hrozek wrote:
 On Fri, Aug 12, 2016 at 11:26:15AM -0400, Justin Stephenson wrote:
> code patch and man page attached, also added the PATCH: prefix to the 
> commit
> message for the code patch.
 You don't have to add the PATCH prefix, git adds that automatically when
 you format the patch with "git format-patch".
>> We have a basic template fot git commit messages.
>> But It need to be enabled by developer.
>> e.g. git config commit.template .git-commit-template
>> https://git.fedorahosted.org/cgit/sssd.git/commit/?id=3d9edb4c510028def2df41aa7b0ce705b197e6fc
>>
>>
>> BTW Michal recently updated our contribution wiki page
>> and information about git commit template is already there :-)
>> https://fedorahosted.org/sssd/wiki/Contribute
> Thanks Lukas, I applied the git commit template but is there a list of
> Components somewhere that we would decide to choose from?
> 

It's not formal, it's mostly a hint. In this case, I'd use either "Monitor:" or
"Netlink:" for the component.

> Let me know if I need to clean anything else up for this patch. I was going to
> squash the commits together but there are other commits in between these two 
> in
> my interactive rebase, perhaps I just need to reorder them then squash?
> 
> Kind regards,
> Justin Stephenson
>>
>> 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




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] Add support for disabling netlink use

[Stephen Gallagher]

On 08/12/2016 11:26 AM, Justin Stephenson wrote:
> code patch and man page attached, also added the PATCH: prefix to the commit
> message for the code patch.
> 
> Kind regards,
> 
> Justin Stephenson
> 
> 
> On 08/12/2016 06:00 AM, Jakub Hrozek wrote:
>> On Tue, Aug 09, 2016 at 12:04:56PM -0400, Justin Stephenson wrote:
>>> Hello,
>>>
>>> The attached patch fixes: https://fedorahosted.org/sssd/ticket/2860
>>>
>>> This provides an option to allow sssd testing to ignore network status
>>> changes from the netlink interface(remaining in offline mode, for example)
>>>
>>> Kind regards,
>>>
>>> Justin Stephenson
>>>
>> The patch works fine and the code looks good to me as well. But it only
>> occured to me now that we also want to document the new option in
>> src/man/sssd.8.xml

Could you write a more useful manpage entry for this? The description doesn't
add anything to the option.

Perhaps:

"SSSD will ignore netlink changes when making decisions about when to attempt to
restore online operation."




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH SET] AD_PROVIDER: ad_enabled_domains

[Stephen Gallagher]

On 08/16/2016 09:26 AM, Jakub Hrozek wrote:
> On Tue, Aug 16, 2016 at 03:17:19PM +0200, Petr Cech wrote:
 From 24d32d0eb12ddc433e64ffd6411e9e13f0067b35 Mon Sep 17 00:00:00 2001
 From: Petr Cech 
 Date: Fri, 13 May 2016 05:21:07 -0400
 Subject: [PATCH 1/5] AD_PROVIDER: Add ad_enabled_domains option

 Resolves:
 https://fedorahosted.org/sssd/ticket/2828
>>>
>>> Did you already have the manpage hunk checked by some native English
>>> speaker?
>>
>> No native speaker have seen it.
> 
> OK, can you please ask Dan or Stephen to help us word the manpage piece
> better?


Proposed man page change:



ad_enabled_domains (string)
   A comma-separated list of enabled Active Directory domains.
   If provided, SSSD will ignore any domains not listed in this
   option. If left unset, all domains from the AD forest will
   be available.

   For proper operation, this option must be specified in all
   lower-case and as the fully qualified domain name of the
   Active Directory domain. For example:

   ad_enabled_domains = sales.example.com, eng.example.com

   The short domain name (also known as the NetBIOS or the flat
   name) will be autodetected by SSSD.

   Default: Not set




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] CONFIG: full_name_format is an allowed option for all domains

[Stephen Gallagher]

On 08/12/2016 07:30 AM, Lukas Slebodnik wrote:
> On (12/08/16 13:24), Jakub Hrozek wrote:
>> Hi,
>>
>> a simple one-liner is attached.
> 
>>From c7bd0b7e695d031258ab47d8c425c9d5843d4069 Mon Sep 17 00:00:00 2001
>> From: Jakub Hrozek 
>> Date: Fri, 12 Aug 2016 13:23:16 +0200
>> Subject: [PATCH] CONFIG: full_name_format is an allowed option for all 
>> domains
>>
>> ---
>> src/config/cfg_rules.ini | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/src/config/cfg_rules.ini b/src/config/cfg_rules.ini
>> index 
>> 09f53fa41eb2904f11a78af333b6d79619d2759c..febe4289832f3778b7e974ef4e8b3f6d9d8bffd8
>>  100644
>> --- a/src/config/cfg_rules.ini
>> +++ b/src/config/cfg_rules.ini
>> @@ -287,6 +287,7 @@ option = subdomain_refresh_interval
>> option = subdomain_inherit
>> option = cached_auth_timeout
>> option = wildcard_limit
>> +option = full_name_format
>>
>> #Entry cache timeouts
>> option = entry_cache_user_timeout
> 
> Could you also update src/config/etc/sssd.api.* ?
> 

Which will probably mean updating the SSSDConfigTest also.



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] LDAP: Removing of useless debug message

[Stephen Gallagher]

On 07/12/2016 03:40 AM, Petr Cech wrote:
> On 07/11/2016 08:22 PM, Jakub Hrozek wrote:
>> On Mon, Jul 11, 2016 at 09:49:15AM -0400, Stephen Gallagher wrote:
>>> On 07/11/2016 09:33 AM, Petr Cech wrote:
>>>> Hi list,
>>>>
>>>> how Jakub mentioned on internal list this debug message should be removed. 
>>>> So I
>>>> attached simple patch for it.
>>>>
>>>
>>> I'd recommend changing it to "Trace: end of ldap_result list" rather than
>>> deleting it.
> 
> Sure, this is better solution. Thanks, Stephen.
> I looked at the code and it makes sense to me.
> 
>> In addition, can you also link https://fedorahosted.org/sssd/ticket/3091 to
>> the patch in the commit message? I've seen someone again being confused
>> by the error message this week, that's why I filed the ticket in the
>> first place.
> 
> I linked the ticket.
> 
>> (and #3090 is a similar ticket although I'm not sure if you're familiar
>> with the IPA subdomains code..)
> 
> New version is attached.
> 
> Regards
> 
> 

Ack




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: sssctl: Use localtime for time stamps

[Stephen Gallagher]

On 07/18/2016 03:40 PM, Stephen Gallagher wrote:
> On 07/14/2016 11:06 AM, Fabiano Fidêncio wrote:
>> Best Regards,
>>
>>
> 
> Looks like it's too late, but I disagree with this patch. The reason that the
> logs are all in UTC is to make it easy to correlate them if you are managing
> geographically-diverse environments. If there's actual confusion about the 
> time
> zone that the logs are showing, then we should probably just include the text
> UTC after it. Converting to local time has many issues (including confusion
> occurring around daylight-savings changes).
> 

*sigh* I didn't read it closely enough. This is the sssctl which is unlikely to
be run on a remote system. Please disregard.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: sssctl: Use localtime for time stamps

[Stephen Gallagher]

On 07/14/2016 11:06 AM, Fabiano Fidêncio wrote:
> Best Regards,
> 
> 

Looks like it's too late, but I disagree with this patch. The reason that the
logs are all in UTC is to make it easy to correlate them if you are managing
geographically-diverse environments. If there's actual confusion about the time
zone that the logs are showing, then we should probably just include the text
UTC after it. Converting to local time has many issues (including confusion
occurring around daylight-savings changes).




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] LDAP: Removing of useless debug message

[Stephen Gallagher]

On 07/11/2016 09:33 AM, Petr Cech wrote:
> Hi list,
> 
> how Jakub mentioned on internal list this debug message should be removed. So 
> I
> attached simple patch for it.
> 

I'd recommend changing it to "Trace: end of ldap_result list" rather than
deleting it.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] Add winbind idmap plugin

[Stephen Gallagher]

On 06/20/2016 05:48 AM, Sumit Bose wrote:
> On Mon, Jun 20, 2016 at 11:15:20AM +0200, Lukas Slebodnik wrote:
>> BTW we can add Requires/Recommends into pacakge sssd-ad for this sub-pacakge.
>> So it will be installed by default.
> 
> I think this is not needed. It is only needed for samba, not on an
> general AD client. And even with samba people might want to select
> between the idmap plugin and SSSD's libwbclient implementation.
> 

What is the advantage of one over the other? This seems to me like one of those
situations where we have two solutions that people aren't realistically going to
know which to pick. In that case, we should probably consider using dependencies
to select the preferred one if neither is currently present.

We can do this by having both packages add a virtual `Provides:
sssd-samba-id-mapping` and have sssd-ad add

Recommends: sssd-samba-id-mapping
Suggests: sssd-libwbclient

(or Suggests: sssd-winbind-idmap whichever one is more feature-complete)


What this will do is pull in the Suggests dependency unless the other one was
specified manually (or already exists on the system) to satisfy the virtual
Provides.



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] confd: Make it possible to use config snippets

[Stephen Gallagher]

On 06/20/2016 03:09 PM, Jakub Hrozek wrote:
> On Mon, Jun 20, 2016 at 08:54:18PM +0200, Lukas Slebodnik wrote:
>> ehlo,
>>
>> Attached is a sligtly modified version of Michal's patch.
> 
> The same patch is attached twice. Was it by accident or did you mean to
> send two patches?
> 
>> I fixed few coding style issues + added missing creation of directory
>> + spec file change.
> 
> You should have sent fixups to get credit, but meh :) Thanks for doing
> the work nonetheless.
> 
>>
>> You might notice that Michal removed detection of sssd.conf modified time.
>> It is because mtime could be obtiained from sssd.conf before parsing.
>> However, snippets files are open after parsing sssd.conf and mtime
>> of snippet files is ignored in the process.
>>
>> We have few options.
>> * check mtime directly in sssd
>> * add new function to libini_config to get latest mtime before parsing
>>   (max_mtime(main.conf + alowed snippet files)
>>   // it's little bit a complication for user of libini_config
>>   // because user will need to paste regex for allowed snippets twice
>>   // 1st time in new function for checking mtime and 2nd time in function
>>   // ini_config_augment
>> * modify libini_config to set max mtime while parsing snippet files
>>   // but we will need to parse files anyway. So I'm not sure what will be
>>   // benefit of cehcking mtime after parsing.
>> * last option is to ignore mtime. (Michal's current version)
>>   // and remove FIXME :-)
> 
> Is there actually any downside to /always/ reading the config file and
> always creating the confdb from scratch? I would say that sssd restarts
> are a rare operation and the parsing and writes are not too big to slow
> down the startup significantly.
> 
> I think the whole mtime logic was there only to allow online config
> changes, which is something we tried in the past, but could never code
> it up properly.
> 
> 

I think as long as we keep the parsing time very short, this is probably not an
issue. However, if we ever move SSSD to a socket-activated behavior, we will
want this to be very fast (or otherwise skippable).


>>
>> The main purpose of this mail is to decide wheteer we want change in 
>> ding-libs
>> or no.
>>
>> BTW. We cannot change directory for snippet files from command line.
>> Do we want such feature?
>> [root@graviton ~]# /usr/sbin/sssd --help
>> Usage: sssd [OPTION...]
>>   -d, --debug-level=INTDebug level
>>   -f, --debug-to-files Send the debug output to files instead of
>>stderr
>>   --debug-timestamps=INT   Add debug timestamps
>>   --debug-microseconds=INT Show timestamps with microseconds
>>   -D, --daemon Become a daemon (default)
>>   -i, --interactiveRun interactive (not a daemon)
>>   -c, --config=STRING  Specify a non-default config file
> 
> Can you think of any use for this option? There can be only one sssd on
> the system, so I actually wonder if we can remove it..
> 

I think we originally added this thinking of a future in which we might want to
be able to run a live SSSD for automated tests, but we obviously went another
route with it.

The only use I can think of for this would be if someone actually wanted to move
their sssd.conf to a mounted read-only location (since IIRC we actually
explicitly disallow sssd.conf as a symlink).



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] DEBUG: Add `debug` alias for debug_level

[Stephen Gallagher]

On 04/28/2016 09:30 AM, Lukas Slebodnik wrote:
> On (27/04/16 15:18), Stephen Gallagher wrote:
>> On 04/27/2016 05:57 AM, Pavel Březina wrote:
>>> On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
>>>> Our users constantly make the mistake of typing `debug = 9` in the 
>>>> sssd.conf
>>>> instead of `debug_level = 9` as would be correct. This happens 
>>>> frequently-enough
>>>> that we should just alias it rather than continue to have people make 
>>>> mistakes.
>>>>
>>>> Resolves:
>>>> https://fedorahosted.org/sssd/ticket/2999
>>>
>>> I don't really oppose but I'd rather print a warning instead of aliasing it,
>>> otherwise we can end up aliasing everything. It may be done as part of
>>> configuration check patches that should detect typos.'
>>
>>
>> Yeah, I don't want this to become a common thing (we shouldn't really be
>> aliasing anything), but this is such a *common* mistake that it's bordering 
>> on
>> ridiculous not to just make an exception here.
>>
>> When you get right down to it, most projects use the more abbreviated term
>> "debug" anyway, so we're kind of the outlier.
>>
>> (You will notice I intentionally didn't add it to the manual; this is meant 
>> to
>> be a hidden convenience feature, not the primary method. Also, `debug_level`
>> will always overrule `debug` if both are present.)
>>
> I don't prefer undocumeted options.
> If we really want to have an alias then it should be documented.
> Otherwise users might wonder why it magicaly works.
> 
> The option/alias will need to be added to the list of valid options anyway
> with the config validation (coming soon)
> 

Ok, I added documentation and tests for it. See attached patch.

From 4a753d3a754ffa5a3e48eb6ba2fd8ebc73fd20be Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 26 Apr 2016 11:04:36 -0400
Subject: [PATCH] DEBUG: Add `debug` alias for debug_level

Our users constantly make the mistake of typing `debug = 9` in the
sssd.conf instead of `debug_level = 9` as would be correct. This
happens frequently-enough that we should just alias it rather than
continue to have people make mistakes.

Resolves:
https://fedorahosted.org/sssd/ticket/2999
---
 src/confdb/confdb.h  |  1 +
 src/config/SSSDConfig/__init__.py.in |  1 +
 src/config/SSSDConfigTest.py |  3 +++
 src/config/etc/sssd.api.conf |  2 ++
 src/man/sssd.conf.5.xml  | 13 +
 src/util/server.c| 15 ++-
 6 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index a9b1c4362b5c0c6b158830b1bf2ef68db09d8d06..153e68a0761463723945667004b4505acbc5a0b9 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -51,10 +51,11 @@
 
 /* Services */
 #define CONFDB_SERVICE_PATH_TMPL "config/%s"
 #define CONFDB_SERVICE_COMMAND "command"
 #define CONFDB_SERVICE_DEBUG_LEVEL "debug_level"
+#define CONFDB_SERVICE_DEBUG_LEVEL_ALIAS "debug"
 #define CONFDB_SERVICE_DEBUG_TIMESTAMPS "debug_timestamps"
 #define CONFDB_SERVICE_DEBUG_MICROSECONDS "debug_microseconds"
 #define CONFDB_SERVICE_DEBUG_TO_FILES "debug_to_files"
 #define CONFDB_SERVICE_TIMEOUT "timeout"
 #define CONFDB_SERVICE_FORCE_TIMEOUT "force_timeout"
diff --git a/src/config/SSSDConfig/__init__.py.in b/src/config/SSSDConfig/__init__.py.in
index 1a0893cbc180394d994b9d97fc0fa863da656549..1c490cf714e06e75b1ce8d5dee1aec27dfc2d12b 100644
--- a/src/config/SSSDConfig/__init__.py.in
+++ b/src/config/SSSDConfig/__init__.py.in
@@ -38,10 +38,11 @@ else:
 _ = translation.ugettext
 
 # TODO: This needs to be made external
 option_strings = {
 # [service]
+'debug' : _('Set the verbosity of the debug logging'),
 'debug_level' : _('Set the verbosity of the debug logging'),
 'debug_timestamps' : _('Include timestamps in debug logs'),
 'debug_microseconds' : _('Include microseconds in timestamps in debug logs'),
 'debug_to_files' : _('Write debug messages to logfiles'),
 'timeout' : _('Ping timeout before restarting service'),
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py
index e518c756599be42dddf3397a47451ac6afa05d29..6ec30234e24b7b48ccab6a98e1f9396990509190 100755
--- a/src/config/SSSDConfigTest.py
+++ b/src/config/SSSDConfigTest.py
@@ -297,10 +297,11 @@ class SSSDConfigTestSSSDService(unittest.TestCase):
 're_expression',
 'full_name_format',
 

[SSSD] Re: [PATCH] GPO: Add "polkit-1" to ad_gpo_map_allow

[Stephen Gallagher]

On 05/13/2016 09:07 AM, Stephen Gallagher wrote:
> Polkit is an authorization mechanism of its own (similar to sudo). SSSD 
> doesn't
> need to apply additional authorization decisions atop it, so we'll just accept
> it as "allow".
> 
> Resolves:
> https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
> 

Bump




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCHES] Support starting SSSD from a default configuration

[Stephen Gallagher]

On 05/13/2016 10:29 AM, Lukas Slebodnik wrote:
> On (11/05/16 17:35), Lukas Slebodnik wrote:
>> On (10/05/16 17:06), Jakub Hrozek wrote:
>>> On Tue, May 10, 2016 at 09:51:18AM -0400, Stephen Gallagher wrote:
>>>> On 05/10/2016 09:45 AM, Jakub Hrozek wrote:

>> This basic sssd configuration expects that proxy provider will
>> be installed by default. And that's not reality.
>>
>> It's only installed with meta-package sssd.
>> But It will not work with minimal installation of sssd.
>> "yum install -y sssd-ldap"
>> or even just "sssd-common"
>>
>> If should start without failures and with default configuration
>> then we should change pacakging as well and recommend similar packaging 
>> change
>> to other downstreams (debian, opensuse ...)


Sorry it took me so long to reply on this. I should have examined this more
closely. It was probably a bad idea to have the proxy provided be the default
setup, since it might or might not be installed (and we probably don't want to
force its inclusion). I guess it would make more sense for the native 'local'
provider to be the default.

As for the AVC, I spotted that in my testing and I think I reported it to the
selinux-policy package, but I'm not certain (I can't find it right now).



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] GPO: Add "polkit-1" to ad_gpo_map_allow

[Stephen Gallagher]

Polkit is an authorization mechanism of its own (similar to sudo). SSSD doesn't 
need to apply additional authorization decisions atop it, so we'll just accept 
it as "allow".


Resolves:
https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415

From 32457dcdbebd25ffbfc6c4d4f08fe231a693a469 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Fri, 13 May 2016 09:03:29 -0400
Subject: [PATCH] GPO: Add "polkit-1" to ad_gpo_map_allow

Polkit is an authorization mechanism of its own (similar to sudo).
SSSD doesn't need to apply additional authorization decisions atop
it, so we'll just accept it as "allow".

Resolves:
https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
---
 src/man/sssd-ad.5.xml | 5 +
 src/providers/ad/ad_gpo.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 265409e58fa397179a9ab272a05ec948c6c9a827..ef27976dd62e164cfb91359efc69bd54e1aa9711 100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -640,10 +640,15 @@ ad_gpo_map_permit = +my_pam_service, -sudo
 
 Default: the default set of PAM service names includes:
 
 
 
+polkit-1
+
+
+
+
 sudo
 
 
 
 
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 22ac803378d61b8001ea7a842a0c54964614b238..1648511992a6cf40b7619e46060192d6f45d48e6 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -192,10 +192,11 @@ int ad_gpo_process_cse_recv(struct tevent_req *req);
 #define GPO_XDM "xdm"
 #define GPO_SSHD "sshd"
 #define GPO_FTP "ftp"
 #define GPO_SAMBA "samba"
 #define GPO_CROND "crond"
+#define GPO_POLKIT "polkit-1"
 #define GPO_SUDO "sudo"
 #define GPO_SUDO_I "sudo-i"
 #define GPO_SYSTEMD_USER "systemd-user"
 #define GPO_COCKPIT "cockpit"
 
@@ -214,11 +215,12 @@ const char *gpo_map_interactive_defaults[] =
 const char *gpo_map_remote_interactive_defaults[] = {GPO_SSHD, GPO_COCKPIT,
  NULL};
 const char *gpo_map_network_defaults[] = {GPO_FTP, GPO_SAMBA, NULL};
 const char *gpo_map_batch_defaults[] = {GPO_CROND, NULL};
 const char *gpo_map_service_defaults[] = {NULL};
-const char *gpo_map_permit_defaults[] = {GPO_SUDO, GPO_SUDO_I,
+const char *gpo_map_permit_defaults[] = {GPO_POLKIT,
+ GPO_SUDO, GPO_SUDO_I,
  GPO_SYSTEMD_USER,  NULL};
 const char *gpo_map_deny_defaults[] = {NULL};
 
 struct gpo_map_option_entry gpo_map_option_entries[] = {
 {GPO_MAP_INTERACTIVE, AD_GPO_MAP_INTERACTIVE, gpo_map_interactive_defaults,
-- 
2.7.4



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCHES] Support starting SSSD from a default configuration

[Stephen Gallagher]

On 05/10/2016 09:45 AM, Jakub Hrozek wrote:
> On Tue, Apr 19, 2016 at 02:09:14PM -0400, Stephen Gallagher wrote:
>> These patches provide support for shipping a default configuration file that 
>> the
>> monitor will automatically copy to /etc/sssd/sssd.conf if none already 
>> exists.
>> The idea is for distributions to be able to provide a default (and 
>> resettable)
>> configuration for out-of-the-box behavior.
>>
>> I considered writing the patch to check /etc/sssd and then check 
>> /usr/lib*/sssd
>> in turn, but I realized that this would be too complicated with the infopipe
>> interactions (which would need to be updated to do a copy-on-write the first
>> time they changed something). It was simpler to just always create the /etc
>> version and use that.
>>
>>
>> Patch 0001: Create a secure copy function that can be used to duplicate the
>> default configuration
>>
>> Patch 0002: Cosmetic patch; changes the name of an internal macro variable to
>> make it clear that it's the active configuration file, not the default one.
>>
>> Patch 0003: Add the logic to confdb_setup.c to copy over the default
>> configuration if and only if our attempt to load the configuration came up 
>> with
>> ERR_MISSING_CONF. It will then try to load it again and proceed or fail from 
>> there.
>>
>> The default configuration provided here is to load the SSSD with a single 
>> proxy
>> provider that reads from nss_files (and supports authentication through
>> pam_unix). This does not have to be shipped with any downstream package; the
>> idea is that downstreams would be expected to modify this configuration to 
>> their
>> own needs. This would need to be called out in the release announcement for
>> whatever version of SSSD incorporates this change.
> 
> Wow, it took me long to get back to the review :-(
> 
> I had to slightly fix the unit test otherwise it was failing for me. The
> follow up patch is at:
> https://github.com/jhrozek/sssd/tree/conf-review
> if you agree with squashing the patch into your patchset, I can ACK the
> patches.
> 

LGTM



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: Idea for multilib handling in Fedora and RHEL

[Stephen Gallagher]

On 05/10/2016 09:00 AM, Lukas Slebodnik wrote:
> On (10/05/16 08:42), Stephen Gallagher wrote:
>> On 05/10/2016 07:24 AM, Lukas Slebodnik wrote:
>>> On (10/05/16 06:40), Stephen Gallagher wrote:
>>>> I was thinking this morning again about how we could deal with the 32-bit
>>>> on 64-bit problem. On Fedora 24 and newer, we have the ability to use rich
>>>> RPM dependencies (Recommends: sssd-client.i686 if glibc.i686) That doesn't
>>>> help on older Fedora or RHEL systems though.
>>>>
>>> Fedora 23 has rpm-4.13.0-0.rc1.13.fc23 and it should work well with weak
>>> dependencies.
>>>
>>> Fedora 22 will be EOL in 2 months. (one month after fedora 24 final
>>> release[2])
>>>
>>> So we needn't care much about fedora 22. :-)
>>>
>>
>> I don't particularly care about Fedora 22. I *do* care about RHEL/CentOS 6 
>> and 7.
> OK
> 
> I think we all agree on separate package for libnss_sss.so.2 :-)
> 
> So let's split discussion into two parts
> A) fedora + weak dependencies
> Would that work for you?
> libnss-sss
> Supplements: glibc-common%{?_isa}
> 

What I'd really like to do is have sssd-common have the following:
Name: sssd-common
Requires: sssd-clients-nss%{_isa}
Requires: sssd-clients-nss%{multilibarch} if glibc%{multilibarch}

This approach would make it a hard dependency to have the appropariate client
software if glibc was installed.

Unfortunately, that's impossible right now due to limitations in the compose
tools of Fedora (see this[1] thread for an explanation). Also we'd need to
create %{multilibarch} or get redhat-rpm-config to add it as an available 
option.



So for now, I think we'd actually need to do:
Name: sssd-clients-nss
Supplements: (glibc-common%{?_isa} if sssd-common)

Otherwise, we would be installing the NSS module unconditionally for all glibc
installations, which is probably not quite what we want.



> 
> B) rhel
> IIRC your initial mail you proposed to
> add requirement to sssd-common on require 32 and 64 bit "libnss-sss"
> + remove automatically generated dependencies for libnss-sss.i686
> 
> otherwise the installation of sssd-common.x86_64 would install
> 32bit glibc
> 
> If yes please file downstream bugs for rhel6 and rhel7.
> I am not sure whether we want to complicate upstream spec file for rhel.
> (It's already complicated enough due to rhel/fedora compatibility)
> Or do you prefer to have such change also in upstream spec file?
> 

Maybe it's time to split the usptream spec file into a Fedora-specific one and a
RHEL-specific one. There's still plenty of value in being able to produce a
RHEL-installable set of packages straight from the upstream sources, but you may
be right that keeping all the logic in a single spec file is getting unwieldy.
I'll leave that to you and Jakub to decide, though.



[1]
https://lists.fedoraproject.org/archives/list/devel-annou...@lists.fedoraproject.org/thread/Q5LMMPVEORM76IOPGKYS4XJ6VZ2WLAAX/




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: Idea for multilib handling in Fedora and RHEL

[Stephen Gallagher]

On 05/10/2016 07:24 AM, Lukas Slebodnik wrote:
> On (10/05/16 06:40), Stephen Gallagher wrote:
>> I was thinking this morning again about how we could deal with the 32-bit
>> on 64-bit problem. On Fedora 24 and newer, we have the ability to use rich
>> RPM dependencies (Recommends: sssd-client.i686 if glibc.i686) That doesn't
>> help on older Fedora or RHEL systems though.
>> 
> Fedora 23 has rpm-4.13.0-0.rc1.13.fc23 and it should work well with weak
> dependencies.
> 
> Fedora 22 will be EOL in 2 months. (one month after fedora 24 final
> release[2])
> 
> So we needn't care much about fedora 22. :-)
> 

I don't particularly care about Fedora 22. I *do* care about RHEL/CentOS 6 and 
7.


>> What if we were to split the nss_sss.so.2 library into its own subpackage
>> and then turn off the automatic dependency generation for it? We could then
>> have sssd-common Requires: the one for the same arch and Recommends: the
>> one for the other architecture (or Requires: for older systems that don't
>> support Recommends:)
>> 
> Debian has already nss modeule in separate package libnss-sss 
> https://packages.debian.org/jessie/libnss-sss I'm fine with separate
> packsge.
> 
> Do we want to have Recommends or weaker dependency only Suggest?
> 

Suggests is functionally meaningless for this purpose. Our tools (dnf,
PackageKit) do not readily have a mechanism for directly installing Suggests. In
practice, Suggests: is only useful for disambiguation.

For example:
Requires: virtual(database)
Suggests: mariadb

(Which means that if no package on the system already provides
"virtual(database)", then mariadb will be installed to satisfy it.

"Recommends" is basically the same as "Requires" except that you can uninstall
it later without affecting the package that pulled it in. (Or you can tell DNF
to install only required packages)


> But there is a question. Should sssd-common recommend "libnss-sss" or
> recommend shoudl be added to glibc.
> 
> glibc-common.i686 Recommends: libnss-sss%{?_isa}
> 


I'm pretty sure we can't do that because it would effectively be a circular
dependency. To remove that cycle, we'd still have to remove the autorequires on
glibc, which would eliminate the advantage to having glibc pull it in on its
own. I think it's better to keep the dependencies tied to SSSD itself.


> or it is purpose of reverse weak dependencies (Supplements, Enhances)??? 
> libnss-sss.686 Supplements: glibc-common%{?_isa}
> 


Reverse deps aren't available on RHEL 6 or RHEL 7.


>> The result would be that default installations of the OS could have both
>> versions of nss_sss.so.2 but of course the 32-bit one wouldn't actually do
>> anything unless someone installs glibc.i686 (or it is pulled in by
>> something else).
>> 
>> This would not be an approach I would recommend in general, but the NSS
>> client is a special case: it's only meaningful if glibc is installed
>> because otherwise nothing would ever call into it. Even for the primary
>> arch, it's a safe assumption that glibc will always be installed at least
>> for that arch, so there's no reason to add that dependency explicitly.
> 
> LS
> 
> [1] https://bodhi.fedoraproject.org/updates/?packages=rpm [2]
> http://fedoraproject.org/wiki/Releases/24/Schedule 
> ___ sssd-devel mailing list 
> sssd-devel@lists.fedorahosted.org 
> https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
> 




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Idea for multilib handling in Fedora and RHEL

[Stephen Gallagher]

I was thinking this morning again about how we could deal with the 32-bit on 
64-bit problem. On Fedora 24 and newer, we have the ability to use rich RPM 
dependencies (Recommends: sssd-client.i686 if glibc.i686) That doesn't help on 
older Fedora or RHEL systems though.

What if we were to split the nss_sss.so.2 library into its own subpackage and 
then turn off the automatic dependency generation for it? We could then have 
sssd-common Requires: the one for the same arch and Recommends: the one for the 
other architecture (or Requires: for older systems that don't support 
Recommends:) 

The result would be that default installations of the OS could have both 
versions of nss_sss.so.2 but of course the 32-bit one wouldn't actually do 
anything unless someone installs glibc.i686 (or it is pulled in by something 
else).

This would not be an approach I would recommend in general, but the NSS client 
is a special case: it's only meaningful if glibc is installed because otherwise 
nothing would ever call into it. Even for the primary arch, it's a safe 
assumption that glibc will always be installed at least for that arch, so 
there's no reason to add that dependency explicitly.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] GPO: Add "unity" to ad_gpo_map_interactive

[Stephen Gallagher]

On 05/06/2016 07:05 AM, Lukas Slebodnik wrote:
> On (06/05/16 06:58), Stephen Gallagher wrote:
>>> On May 6, 2016, at 6:55 AM, Lukas Slebodnik  wrote:
>>>
>>>> On (05/05/16 10:46), Stephen Gallagher wrote:
>>>> Ubuntu systems use "unity" as their screen-locker. Without this in the 
>>>> defaults,
>>>> people often get locked out of their machines when the screen locks.
>>>>
>>>> Resolves:
>>>> https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
>>>
>>> Patch is confirmed by reporter in launchpad
>>>
>>> But they seems to have problem also with polkit-1
>>>
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [be_req_set_domain] (0x0400): Changing 
>>> request domain from [INET.ACTIVARSAS.COM] to [INET.ACTIVARSAS.COM]
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler] (0x0100): Got request with 
>>> the following data
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): command: 
>>> SSS_PAM_ACCT_MGMT
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): domain: 
>>> INET.ACTIVARSAS.COM
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): user: cvargasc
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): service: polkit-1
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): tty: 
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): ruser: cvargasc
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): rhost: 
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): authtok type: 0
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): newauthtok type: >>> 0
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): priv: 0
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): cli_pid: 2431
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): logon name: not 
>>> set
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [sdap_access_send] (0x0400): Performing 
>>> access check for user [cvargasc]
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [sdap_account_expired_ad] (0x0400): 
>>> Performing AD access check for user [cvargasc]
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_send] (0x0400): using 
>>> default right
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_send] (0x0400): service 
>>> polkit-1 maps to Denied
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_done] (0x0040): GPO-based 
>>> access control failed.
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Backend 
>>> returned: (0, 6, ) [Success]
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Sending 
>>> result [6][INET.ACTIVARSAS.COM]
>>> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Sent 
>>> result [6][INET.ACTIVARSAS.COM]
>>>
>>>
>>> Do we want to change it separate patch?
>>>
>>> LS
>>
>>
>> The unity one seemed pretty generic, but I am not sure "polkit-1" is right 
>> for upstream (since it sounds rather like an Ubuntu-ism". What do we call it 
>> in Fedora?
>>
> I have no idea and I do not use gnome :-)
> 
>> I guess as long as it isn't in conflict, we could merge it in here, though. 
> OK, it can be a separate patch if needed.
> 
> unity patch was pushed to master:
> 89376da80b2250b82d256ea85ec349ce29fe5b51
> 
> Thank you very much

I checked this morning and we call it polkit-1 on Fedora as well. (By the way,
polkit isn't GNOME-specific, it's a FreeDesktop project)

So, the more I think of this, the more I expect that we probably wouldn't want
to treat this as an interactive logon value. Polkit can function through remote
interfaces as well (such as SSH) because it has a text-based prompter as well.

I think the closest parallel for polkit-1 would be sudo, which we have mapped to
ad_gpo_map_permit defaults. Given that sudo and polkit are both access-control
mechanisms themselves, I think it's reasonable to trust their judgment rather
than SSSD's on whether to allow or deny. If you agree, I'll send another patch
to add it there.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] GPO: Add "unity" to ad_gpo_map_interactive

[Stephen Gallagher]

> On May 6, 2016, at 6:55 AM, Lukas Slebodnik  wrote:
> 
>> On (05/05/16 10:46), Stephen Gallagher wrote:
>> Ubuntu systems use "unity" as their screen-locker. Without this in the 
>> defaults,
>> people often get locked out of their machines when the screen locks.
>> 
>> Resolves:
>> https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
> 
> Patch is confirmed by reporter in launchpad
> 
> But they seems to have problem also with polkit-1
> 
> [sssd[be[INET.ACTIVARSAS.COM]]] [be_req_set_domain] (0x0400): Changing 
> request domain from [INET.ACTIVARSAS.COM] to [INET.ACTIVARSAS.COM]
> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler] (0x0100): Got request with 
> the following data
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): command: 
> SSS_PAM_ACCT_MGMT
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): domain: 
> INET.ACTIVARSAS.COM
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): user: cvargasc
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): service: polkit-1
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): tty: 
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): ruser: cvargasc
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): rhost: 
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): authtok type: 0
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): newauthtok type: 0
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): priv: 0
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): cli_pid: 2431
> [sssd[be[INET.ACTIVARSAS.COM]]] [pam_print_data] (0x0100): logon name: not set
> [sssd[be[INET.ACTIVARSAS.COM]]] [sdap_access_send] (0x0400): Performing 
> access check for user [cvargasc]
> [sssd[be[INET.ACTIVARSAS.COM]]] [sdap_account_expired_ad] (0x0400): 
> Performing AD access check for user [cvargasc]
> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_send] (0x0400): using default 
> right
> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_send] (0x0400): service 
> polkit-1 maps to Denied
> [sssd[be[INET.ACTIVARSAS.COM]]] [ad_gpo_access_done] (0x0040): GPO-based 
> access control failed.
> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Backend 
> returned: (0, 6, ) [Success]
> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Sending 
> result [6][INET.ACTIVARSAS.COM]
> [sssd[be[INET.ACTIVARSAS.COM]]] [be_pam_handler_callback] (0x0100): Sent 
> result [6][INET.ACTIVARSAS.COM]
> 
> 
> Do we want to change it separate patch?
> 
> LS


The unity one seemed pretty generic, but I am not sure "polkit-1" is right for 
upstream (since it sounds rather like an Ubuntu-ism". What do we call it in 
Fedora?

I guess as long as it isn't in conflict, we could merge it in here, though. 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] GPO: Add "unity" to ad_gpo_map_interactive

[Stephen Gallagher]

Ubuntu systems use "unity" as their screen-locker. Without this in the defaults,
people often get locked out of their machines when the screen locks.

Resolves:
https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
From dac0f10e3e5139cfd378d52c8e57659629f3ba6f Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Thu, 5 May 2016 10:44:24 -0400
Subject: [PATCH] GPO: Add "unity" to ad_gpo_map_interactive

Ubuntu systems use "unity" as their screen-locker. Without this in the
defaults, people often get locked out of their machines when the screen
locks.

Resolves:
https://bugs.launchpad.net/ubuntu/+source/sssd/+bug/1578415
---
 src/man/sssd-ad.5.xml | 5 +
 src/providers/ad/ad_gpo.c | 3 ++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index dc17a2f3c878f463ee914487876ed7f73bfdb7a5..265409e58fa397179a9ab272a05ec948c6c9a827 100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -437,10 +437,15 @@ ad_gpo_map_interactive = +my_pam_service, -login
 sddm
 
 
 
 
+unity
+
+
+
+
 xdm
 
 
 
 
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 3029ffe138598b603b23a8dc49b6a5914e0efed4..22ac803378d61b8001ea7a842a0c54964614b238 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -186,10 +186,11 @@ int ad_gpo_process_cse_recv(struct tevent_req *req);
 #define GPO_GDM_SMARTCARD "gdm-smartcard"
 #define GPO_KDM "kdm"
 #define GPO_LIGHTDM "lightdm"
 #define GPO_LXDM "lxdm"
 #define GPO_SDDM "sddm"
+#define GPO_UNITY "unity"
 #define GPO_XDM "xdm"
 #define GPO_SSHD "sshd"
 #define GPO_FTP "ftp"
 #define GPO_SAMBA "samba"
 #define GPO_CROND "crond"
@@ -207,11 +208,11 @@ struct gpo_map_option_entry {
 };
 
 const char *gpo_map_interactive_defaults[] =
 {GPO_LOGIN, GPO_SU, GPO_SU_L,
  GPO_GDM_FINGERPRINT, GPO_GDM_PASSWORD, GPO_GDM_SMARTCARD, GPO_KDM,
- GPO_LIGHTDM, GPO_LXDM, GPO_SDDM, GPO_XDM, NULL};
+ GPO_LIGHTDM, GPO_LXDM, GPO_SDDM, GPO_UNITY, GPO_XDM, NULL};
 const char *gpo_map_remote_interactive_defaults[] = {GPO_SSHD, GPO_COCKPIT,
  NULL};
 const char *gpo_map_network_defaults[] = {GPO_FTP, GPO_SAMBA, NULL};
 const char *gpo_map_batch_defaults[] = {GPO_CROND, NULL};
 const char *gpo_map_service_defaults[] = {NULL};
-- 
2.7.4



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] DEBUG: Add `debug` alias for debug_level

[Stephen Gallagher]

On 04/27/2016 06:44 AM, Petr Cech wrote:
> On 04/27/2016 08:47 AM, Petr Cech wrote:
>> On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
>>> Our users constantly make the mistake of typing `debug = 9` in the
>>> sssd.conf
>>> instead of `debug_level = 9` as would be correct. This happens
>>> frequently-enough
>>> that we should just alias it rather than continue to have people make
>>> mistakes.
>>>
>>> Resolves:
>>> https://fedorahosted.org/sssd/ticket/2999
>>
>> Hello Stephen,
>>
>> thank you for your patch.
>> I see little disruption of our coding style. My comments are inlined.
> 
> Hi Stephen,
> 
> I took the liberty and I fixed the nitpicks. Are you alright with attached 
> patch?
> 

Looks fine to me. Thanks!

(It's been a long while and an editor-change since I last hacked on SSSD, so I
guess my automatic formatter isn't quite right.)




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] DEBUG: Add `debug` alias for debug_level

[Stephen Gallagher]

On 04/27/2016 05:57 AM, Pavel Březina wrote:
> On 04/26/2016 05:08 PM, Stephen Gallagher wrote:
>> Our users constantly make the mistake of typing `debug = 9` in the sssd.conf
>> instead of `debug_level = 9` as would be correct. This happens 
>> frequently-enough
>> that we should just alias it rather than continue to have people make 
>> mistakes.
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2999
> 
> I don't really oppose but I'd rather print a warning instead of aliasing it,
> otherwise we can end up aliasing everything. It may be done as part of
> configuration check patches that should detect typos.'


Yeah, I don't want this to become a common thing (we shouldn't really be
aliasing anything), but this is such a *common* mistake that it's bordering on
ridiculous not to just make an exception here.

When you get right down to it, most projects use the more abbreviated term
"debug" anyway, so we're kind of the outlier.

(You will notice I intentionally didn't add it to the manual; this is meant to
be a hidden convenience feature, not the primary method. Also, `debug_level`
will always overrule `debug` if both are present.)



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCH] DEBUG: Add `debug` alias for debug_level

[Stephen Gallagher]

Our users constantly make the mistake of typing `debug = 9` in the sssd.conf
instead of `debug_level = 9` as would be correct. This happens frequently-enough
that we should just alias it rather than continue to have people make mistakes.

Resolves:
https://fedorahosted.org/sssd/ticket/2999
From f59256f027bb15a5cff317e5b1d418107b4a0a95 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 26 Apr 2016 11:04:36 -0400
Subject: [PATCH] DEBUG: Add `debug` alias for debug_level

Our users constantly make the mistake of typing `debug = 9` in the
sssd.conf instead of `debug_level = 9` as would be correct. This
happens frequently-enough that we should just alias it rather than
continue to have people make mistakes.

Resolves:
https://fedorahosted.org/sssd/ticket/2999
---
 src/confdb/confdb.h |  1 +
 src/util/server.c   | 15 ++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h
index a9b1c4362b5c0c6b158830b1bf2ef68db09d8d06..153e68a0761463723945667004b4505acbc5a0b9 100644
--- a/src/confdb/confdb.h
+++ b/src/confdb/confdb.h
@@ -51,10 +51,11 @@
 
 /* Services */
 #define CONFDB_SERVICE_PATH_TMPL "config/%s"
 #define CONFDB_SERVICE_COMMAND "command"
 #define CONFDB_SERVICE_DEBUG_LEVEL "debug_level"
+#define CONFDB_SERVICE_DEBUG_LEVEL_ALIAS "debug"
 #define CONFDB_SERVICE_DEBUG_TIMESTAMPS "debug_timestamps"
 #define CONFDB_SERVICE_DEBUG_MICROSECONDS "debug_microseconds"
 #define CONFDB_SERVICE_DEBUG_TO_FILES "debug_to_files"
 #define CONFDB_SERVICE_TIMEOUT "timeout"
 #define CONFDB_SERVICE_FORCE_TIMEOUT "force_timeout"
diff --git a/src/util/server.c b/src/util/server.c
index 67a25955783c30dc03f3d6d9193c8547c625f134..074dc34848ae2b8dd98bf6b218cc6c9c8441104d 100644
--- a/src/util/server.c
+++ b/src/util/server.c
@@ -565,18 +565,31 @@ int server_setup(const char *name, int flags,
 
 if (debug_level == SSSDBG_UNRESOLVED) {
 /* set debug level if any in conf_entry */
 ret = confdb_get_int(ctx->confdb_ctx, conf_entry,
  CONFDB_SERVICE_DEBUG_LEVEL,
- SSSDBG_DEFAULT,
+ SSSDBG_UNRESOLVED,
  &debug_level);
 if (ret != EOK) {
 DEBUG(SSSDBG_FATAL_FAILURE, "Error reading from confdb (%d) "
  "[%s]\n", ret, strerror(ret));
 return ret;
 }
 
+if (debug_level == SSSDBG_UNRESOLVED) {
+/* Check for the `debug` alias */
+ret = confdb_get_int(ctx->confdb_ctx, conf_entry,
+CONFDB_SERVICE_DEBUG_LEVEL_ALIAS,
+SSSDBG_DEFAULT,
+&debug_level);
+if (ret != EOK) {
+DEBUG(SSSDBG_FATAL_FAILURE, "Error reading from confdb (%d) "
+"[%s]\n", ret, strerror(ret));
+return ret;
+}
+}
+
 debug_level = debug_convert_old_level(debug_level);
 }
 
 /* same for debug timestamps */
 if (debug_timestamps == SSSDBG_TIMESTAMP_UNRESOLVED) {
-- 
2.7.4



signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCHES] Support starting SSSD from a default configuration

[Stephen Gallagher]

These patches provide support for shipping a default configuration file that the
monitor will automatically copy to /etc/sssd/sssd.conf if none already exists.
The idea is for distributions to be able to provide a default (and resettable)
configuration for out-of-the-box behavior.

I considered writing the patch to check /etc/sssd and then check /usr/lib*/sssd
in turn, but I realized that this would be too complicated with the infopipe
interactions (which would need to be updated to do a copy-on-write the first
time they changed something). It was simpler to just always create the /etc
version and use that.


Patch 0001: Create a secure copy function that can be used to duplicate the
default configuration

Patch 0002: Cosmetic patch; changes the name of an internal macro variable to
make it clear that it's the active configuration file, not the default one.

Patch 0003: Add the logic to confdb_setup.c to copy over the default
configuration if and only if our attempt to load the configuration came up with
ERR_MISSING_CONF. It will then try to load it again and proceed or fail from 
there.

The default configuration provided here is to load the SSSD with a single proxy
provider that reads from nss_files (and supports authentication through
pam_unix). This does not have to be shipped with any downstream package; the
idea is that downstreams would be expected to modify this configuration to their
own needs. This would need to be called out in the release announcement for
whatever version of SSSD incorporates this change.




These patches will require a change to the SELinux policy, since the monitor
needs to be able to write to the /etc/sssd directory.

type=AVC msg=audit(1461088081.353:550): avc:  denied  { write } for  pid=3721
comm="sssd" name="sssd" dev="dm-0" ino=4600013
scontext=system_u:system_r:sssd_t:s0 tcontext=system_u:object_r:sssd_conf_t:s0
tclass=dir permissive=0
Was caused by:
Missing type enforcement (TE) allow rule.

You can use audit2allow to generate a loadable module to allow 
this access.
From 0ec3577f3cc543b2d9b0b8edc47705e679327ee4 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 19 Apr 2016 09:17:52 -0400
Subject: [PATCH 1/3] UTIL: Add secure copy function

This is a precursor to supporting a static default configuration file.
We need to be able to copy the default into the mutable location if the
infopipe is asked to modify it.

This patch opens both the source and destination files together in order
to avoid time-of-check/time-of-use bugs.
---
 src/tests/files-tests.c |  45 +++-
 src/tools/files.c   | 141 +++-
 src/tools/tools_util.h  |   6 +++
 3 files changed, 152 insertions(+), 40 deletions(-)

diff --git a/src/tests/files-tests.c b/src/tests/files-tests.c
index 09df5cbd48ae056c7d089204f15c6d3b32d98477..769e058fa44c9ac0dde35a2ab33a202ee64575d2 100644
--- a/src/tests/files-tests.c
+++ b/src/tests/files-tests.c
@@ -45,12 +45,12 @@ static TALLOC_CTX *test_ctx = NULL;
 
 static void setup_files_test(void)
 {
 /* create a temporary directory that we fill with stuff later on */
 test_ctx = talloc_new(NULL);
-dir_path = mkdtemp(talloc_strdup(test_ctx, tpl_dir));
-dst_path = mkdtemp(talloc_strdup(test_ctx, tpl_dir));
+dir_path = mkdtemp(talloc_asprintf(test_ctx, "%s/%s", TEST_DIR, tpl_dir));
+dst_path = mkdtemp(talloc_asprintf(test_ctx, "%s/%s", TEST_DIR, tpl_dir));
 
 uid = getuid();
 gid = getgid();
 }
 
@@ -197,10 +197,50 @@ START_TEST(test_simple_copy)
 close(fd);
 talloc_free(tmp);
 }
 END_TEST
 
+START_TEST(test_copy_file)
+{
+TALLOC_CTX *tmp_ctx = talloc_new(test_ctx);
+int ret;
+char origpath[PATH_MAX+1];
+char *foo_path;
+char *bar_path;
+int fd = -1;
+
+errno = 0;
+fail_unless(getcwd(origpath, PATH_MAX) == origpath, "Cannot getcwd\n");
+fail_unless(errno == 0, "Cannot getcwd\n");
+
+/* create a file */
+ret = chdir(dir_path);
+fail_if(ret == -1, "Cannot chdir1\n");
+
+ret = create_simple_file("foo", "foo");
+fail_if(ret == -1, "Cannot create foo\n");
+foo_path = talloc_asprintf(tmp_ctx, "%s/foo", dir_path);
+bar_path = talloc_asprintf(tmp_ctx, "%s/bar", dst_path);
+
+
+/* Copy this file to a new file */
+DEBUG(SSSDBG_FUNC_DATA,
+  "Will copy from 'foo' to 'bar'\n");
+ret = copy_file_secure(foo_path, bar_path, 0700, uid, gid, 0);
+fail_unless(ret == EOK, "copy_file_secure failed\n");
+
+/* check if really copied */
+ret = access(bar_path, F_OK);
+fail_unless(ret == 0, "destination file 'bar' not there\n");
+
+ret = check_and_open_readonly(bar_path, &fd, uid, gid, S_IFREG|S_IRWXU, 0);
+fail_unless(ret == EOK, "Cannot open %s

[SSSD] Re: [PATCH] Netlink: Ignore RTM_NEWADDR signals from link-local

[Stephen Gallagher]

> On Apr 7, 2016, at 3:27 AM, Lukas Slebodnik  wrote:
> 
>> On (06/04/16 15:38), Jakub Hrozek wrote:
>>> On Wed, Apr 06, 2016 at 03:16:20PM +0200, Jakub Hrozek wrote:
>>>> On Wed, Apr 06, 2016 at 08:39:39AM -0400, Stephen Gallagher wrote:
>>>> 
>>>> 
>>>>>> On Apr 6, 2016, at 8:37 AM, Jakub Hrozek  wrote:
>>>>>> 
>>>>>> On Tue, Apr 05, 2016 at 02:34:33PM -0400, Stephen Gallagher wrote:
>>>>>> We only need to go online if we receive a netlink signal that might
>>>>>> indicate that the external connection might have become available. This
>>>>>> will never be true for link-local addresses.
>>>>>> 
>>>>> 
>>>>> The indentation of DEBUG messages is a bit off on two of places. I fixed
>>>>> them locally, if you agree, I would like to push:
>>>>>   
>>>>> https://github.com/jhrozek/sssd/commit/818eada2a68b6c9cff9eb3285ff6126ba4032e31
>>>>> 
>>>>> (Just the intentation changed, nothing else..)
>>>> 
>>>> 
>>>> Fine by me.
>>> 
>>> OK, ACK
>>> 
>>> Thank you for the patch and the investigation!
>> 
>> CI: http://sssd-ci.duckdns.org/logs/job/40/75/summary.html
>> 
>> btw I tested by bringing up and down the loopback interface and making
>> sure that addresses like ::1 are filtered out.
>> 
>> * master: a9d1b4b61b614a954c784f224b8fe7a47b6dd206
> Stephen,
> 
> I think you would like to have this patch in fedora 24 :-)
> I can backport this patch just to fedora if you file a BZ
> or we can backport this patch to 1.13 branch
> and it will be in fedora with next release (1.13.4)
> which should be next week.
> 
> What do you prefer?
> 
> LS


Next week is fine. I'm carrying the patch locally on my machine and I haven't 
heard any other complaints. 
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: [PATCH] Netlink: Ignore RTM_NEWADDR signals from link-local

[Stephen Gallagher]

> On Apr 6, 2016, at 8:37 AM, Jakub Hrozek  wrote:
> 
>> On Tue, Apr 05, 2016 at 02:34:33PM -0400, Stephen Gallagher wrote:
>> We only need to go online if we receive a netlink signal that might
>> indicate that the external connection might have become available. This
>> will never be true for link-local addresses.
>> 
> 
> The indentation of DEBUG messages is a bit off on two of places. I fixed
> them locally, if you agree, I would like to push:
>
> https://github.com/jhrozek/sssd/commit/818eada2a68b6c9cff9eb3285ff6126ba4032e31
> 
> (Just the intentation changed, nothing else..)


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

[SSSD] [PATCH] Netlink: Ignore RTM_NEWADDR signals from link-local

[Stephen Gallagher]

We only need to go online if we receive a netlink signal that might
indicate that the external connection might have become available. This
will never be true for link-local addresses.

From 672b2335c4f94a16a9955814ff77c85462934043 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 5 Apr 2016 12:43:49 -0400
Subject: [PATCH] Netlink: Ignore RTM_NEWADDR signals from link-local

We only need to go online if we receive a netlink signal that might
indicate that the external connection might have become available. This
will never be true for link-local addresses.
---
 src/monitor/monitor_netlink.c | 50 +++
 1 file changed, 50 insertions(+)

diff --git a/src/monitor/monitor_netlink.c b/src/monitor/monitor_netlink.c
index 7e6f8cbbd3c4815fb2c9991698ecfd4ee5df..22262949c67744493dfa722ff38257a75a5b8291 100644
--- a/src/monitor/monitor_netlink.c
+++ b/src/monitor/monitor_netlink.c
@@ -667,20 +667,70 @@ static void addr_msg_debug_print(struct rtnl_addr *addr_obj)
   "addr %s flags 0x%X (%s)\n", ifidx, buf, flags, str_flags);
 }
 
 static void addr_msg_handler(struct nl_object *obj, void *arg)
 {
+int err;
 struct netlink_ctx *ctx = (struct netlink_ctx *) arg;
 struct rtnl_addr *addr_obj;
+struct nl_addr *local_addr;
+struct sockaddr_in sa4;
+struct sockaddr_in6 sa6;
+socklen_t salen;
 
 if (!nlw_is_addr_object(obj)) return;
 
 addr_obj = (struct rtnl_addr *) obj;
 if (debug_level & SSSDBG_TRACE_LIBS) {
 addr_msg_debug_print(addr_obj);
 }
 
+local_addr = rtnl_addr_get_local(addr_obj);
+if (local_addr == NULL) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+  "Received RTM_NEWADDR with no address\n");
+return;
+}
+
+switch (nl_addr_get_family(local_addr)) {
+case AF_INET6:
+salen = sizeof(struct sockaddr_in6);
+err = nl_addr_fill_sockaddr(local_addr,
+(struct sockaddr *) &sa6,
+&salen);
+if (err < 0) {
+  DEBUG(SSSDBG_MINOR_FAILURE,
+"Unknown error in nl_addr_fill_sockaddr\n");
+  return;
+}
+
+if (!check_ipv6_addr(&sa6.sin6_addr, SSS_NO_SPECIAL)) {
+DEBUG(SSSDBG_TRACE_LIBS, "Ignoring special address.\n");
+return;
+}
+break;
+
+case AF_INET:
+salen = sizeof(struct sockaddr_in);
+err = nl_addr_fill_sockaddr(local_addr,
+(struct sockaddr *) &sa4,
+ &salen);
+if (err < 0) {
+DEBUG(SSSDBG_MINOR_FAILURE,
+"Unknown error in nl_addr_fill_sockaddr\n");
+return;
+}
+if (check_ipv4_addr(&sa4.sin_addr, SSS_NO_SPECIAL)) {
+DEBUG(SSSDBG_TRACE_LIBS, "Ignoring special address.\n");
+return;
+}
+break;
+default:
+DEBUG(SSSDBG_CRIT_FAILURE, "Unknown address family\n");
+return;
+}
+
 ctx->change_cb(ctx->cb_data);
 }
 
 static void link_msg_handler(struct nl_object *obj, void *arg)
 {
-- 
2.7.3




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: is this a GPO bug?

[Stephen Gallagher]

On 04/04/2016 08:54 AM, Jakub Hrozek wrote:
> On Mon, Apr 04, 2016 at 02:30:16PM +0200, Lukas Slebodnik wrote:
>> On (04/04/16 13:57), Jakub Hrozek wrote:
>>> Hi,
>>>
>>> I'm looking at a logfile from one sssd installation and I'm wondering if
>>> it's a GPO bug. The relevant part of the logs is:
>>>
>>> [sssd[be[example.com]]] [sdap_parse_entry] (0x1000): OriginalDN: 
>>> [cn={BCB10A5A-630C-477E-8E2D-996F06E36DBD},cn=policies,cn=system,DC=example,DC=com].
>>> [sssd[be[example.com]]] [sdap_parse_entry] (0x1000): Entry has no 
>>> attributes [0(Success)]!?
>>> [sssd[be[example.com]]] [sdap_get_generic_op_finished] (0x0400): Search 
>>> result: Success(0), no errmsg set
>>> [sssd[be[example.com]]] [ad_gpo_sd_process_attrs] (0x0040): 
>>> sysdb_attrs_get_string failed: [2](No such file or directory)
>> It can be either attribute "cn" or gPCFileSysPath
>> #define AD_AT_CN "cn"
>> #define AD_AT_FILE_SYS_PATH "gPCFileSysPath"
> 
> Yes, unfortunately the two debug messages are the same and I don't have
> more verbose logs at the moment. But also note the message before:
> [sssd[be[example.com]]] [sdap_parse_entry] (0x1000): Entry has no 
> attributes [0(Success)]!?
> 
> This read to me as if no attributes were downloaded..
> 


Well, the obvious thing to check would be to perform that actual query and see
if the GPO entry is indeed missing content (could be a misconfiguration on the
AD side). We only request that entry if we're referred over to it, so if it's
incomplete, I think throwing an error is probably the right answer.




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] Re: Design document - sssctl

[Stephen Gallagher]

On 03/22/2016 07:42 AM, Pavel Reichl wrote:
> Hello,
> 
> Pavel Březina and I have prepared the 1st draft of design document. We mostly
> focused on summing up its future functionality and its interface.
> 
> Please comment if you miss some essential functionality or if you would prefer
> some different interface.
> 
> Thanks!
> 
> https://fedorahosted.org/sssd/wiki/DesignDocs/sssctl



"This tool will communicate with InfoPipe responder through its public D-Bus
interface."

Presumably this means that the sssctl CLI will be a thin presentation-layer shim
over the D-BUS API (meaning that the CLI would contain no logic outside of
argument processing)? This would be the ideal case, since it would also allow
plugging in other front-ends for this (such as Cockpit).




signature.asc
Description: OpenPGP digital signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

[SSSD] [PATCHES] Add new default PAM services for AD GPOs

[Stephen Gallagher]

[PATCH 1/2] GPO: Add Cockpit to the Remote Interactive defaults

The Cockpit Project is an administrative console that is gaining in
popularity and is a default component on some operating systems (such
as Fedora Server). Since it is becoming more common, we should ensure
that it is part of the standard mapping.


[PATCH 2/2] GPO: Add other display managers to interactive logon

Gone are the days when all systems used GDM or KDM. We need to support
other display managers in the default configuration to avoid issues
when enrolled in AD domains.
From f396730e4ac063863611e3380157b22806569e0b Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Fri, 26 Feb 2016 13:10:50 -0500
Subject: [PATCH 1/2] GPO: Add Cockpit to the Remote Interactive defaults

The Cockpit Project is an administrative console that is gaining in
popularity and is a default component on some operating systems (such
as Fedora Server). Since it is becoming more common, we should ensure
that it is part of the standard mapping.
---
 src/man/sssd-ad.5.xml | 5 +
 src/providers/ad/ad_gpo.c | 4 +++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 05520d14d0b6d7b61210e6a67d7f1b88ee492ad0..17e301a46d874e9c187daae97fbfecfecec3a940 100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -461,10 +461,15 @@ ad_gpo_map_remote_interactive = +my_pam_service, -sshd
 
 
 sshd
 
 
+
+
+cockpit
+
+
 
 
 
 
 
diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 069196c3b616ebf7661ac9ffe577504b6ebcb674..fb4d46580df081b9ccded39bf85d3f11af1f85ae 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -190,10 +190,11 @@ int ad_gpo_process_cse_recv(struct tevent_req *req);
 #define GPO_SAMBA "samba"
 #define GPO_CROND "crond"
 #define GPO_SUDO "sudo"
 #define GPO_SUDO_I "sudo-i"
 #define GPO_SYSTEMD_USER "systemd-user"
+#define GPO_COCKPIT "cockpit"
 
 struct gpo_map_option_entry {
 enum gpo_map_type gpo_map_type;
 enum ad_basic_opt ad_basic_opt;
 const char **gpo_map_defaults;
@@ -202,11 +203,12 @@ struct gpo_map_option_entry {
 };
 
 const char *gpo_map_interactive_defaults[] =
 {GPO_LOGIN, GPO_SU, GPO_SU_L,
  GPO_GDM_FINGERPRINT, GPO_GDM_PASSWORD, GPO_GDM_SMARTCARD, GPO_KDM, NULL};
-const char *gpo_map_remote_interactive_defaults[] = {GPO_SSHD, NULL};
+const char *gpo_map_remote_interactive_defaults[] = {GPO_SSHD, GPO_COCKPIT,
+ NULL};
 const char *gpo_map_network_defaults[] = {GPO_FTP, GPO_SAMBA, NULL};
 const char *gpo_map_batch_defaults[] = {GPO_CROND, NULL};
 const char *gpo_map_service_defaults[] = {NULL};
 const char *gpo_map_permit_defaults[] = {GPO_SUDO, GPO_SUDO_I,
  GPO_SYSTEMD_USER,  NULL};
-- 
2.7.1

From 4480fa2292a0f0dc0dc32be6a5aa347d79d0e1ea Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Fri, 26 Feb 2016 13:21:23 -0500
Subject: [PATCH 2/2] GPO: Add other display managers to interactive logon

Gone are the days when all systems used GDM or KDM. We need to support
other display managers in the default configuration to avoid issues
when enrolled in AD domains.
---
 src/man/sssd-ad.5.xml | 20 
 src/providers/ad/ad_gpo.c |  7 ++-
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 17e301a46d874e9c187daae97fbfecfecec3a940..dc17a2f3c878f463ee914487876ed7f73bfdb7a5 100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -420,10 +420,30 @@ ad_gpo_map_interactive = +my_pam_service, -login
 
 
 kdm
 
 
+
+
+lightdm
+
+
+
+
+lxdm
+
+
+
+
+sddm
+
+
+
+ 

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

[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

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

[Stephen Gallagher]

-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
.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlaWYv0ACgkQeiVVYja6o6OAPgCeJ4clX3AQHAmneAMZDLWusY3D
qKkAnj+ozhh83H3IYukaYbuwlBUd8fl0
=9PKT
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org

Re: [SSSD] [PATCH] SSSD: Add a new command diag_cmd

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 11/09/2015 05:32 AM, Petr Cech wrote:
> On 11/04/2015 11:24 AM, Jakub Hrozek wrote:
>> Hi,
>> 
>> I created this patch to try to diagnose an issue where sssd
>> would randomly restart on any of machines in a VM cluster without
>> giving too much advise why. I think it might be useful to merge
>> in general.
> 
> Hi Jakub,
> 
> I reviewed the patch. Code looks good to me. CI tests passed:
> http://sssd-ci.duckdns.org/logs/job/32/25/summary.html
> 
> Then I tried to test new functionality.
> 
> Man pages are right, I found diag_cmd in sssd.conf.
> 
> And I really got the right message when I kill sss_pam: # (Mon Nov
> 9 04:30:47 2015) [sssd] [svc_child_info] (0x0040): Child [25767]
> terminated with signal [9]
> 
> I would like to see output of pstack, but I don't know, how to get
> the right state of SSSD. Can you help me, please?
> 
> Regards
> 
> Petr


There are problems inherent with passing the PID to the child process.
There's no guarantee that the process still exists. In the worst-case,
the PID could actually be reassigned to a new process and the output
you got back from something like pstack could be reading from a
different executable entirely.

-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlZA4zwACgkQeiVVYja6o6PjVQCgoTLJPxp46rdMzBmuTrbXi329
ZMEAn0fFVamHQugQAFjCNxQ4hCnTawmC
=VOCY
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] DEBUG: Don't error on chown of nonexistent file

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/28/2015 04:48 PM, Lukas Slebodnik wrote:
> On (28/10/15 09:03), Stephen Gallagher wrote:
>> On 10/27/2015 05:33 PM, Lukas Slebodnik wrote:
>>> On (27/10/15 09:48), Stephen Gallagher wrote:
>>>> We get an error message if we start up SSSD and the debug
>>>> log does not yet exist.
>>> 
>>>> From 53592734f73c50029fa573b9bc070437304ea489 Mon Sep 17
>>>> 00:00:00 2001 From: Stephen Gallagher 
>>>> Date: Tue, 27 Oct 2015 09:39:01 -0400 Subject: [PATCH] DEBUG:
>>>> Don't error on chown of nonexistent file
>>>> 
>>>> We get an error message if we start up SSSD and the debug
>>>> log does not yet exist. --- src/util/debug.c | 9 ++--- 1
>>>> file changed, 6 insertions(+), 3 deletions(-)
>>>> 
>>>> diff --git a/src/util/debug.c b/src/util/debug.c index 
>>>> a8eea32740155ec3daf6be71ef9a8af6592f74a9..729d9f99d35c7208950a9a1af1f
>>
>>>> 
df3942b23a147
>>>> 100644 --- a/src/util/debug.c +++ b/src/util/debug.c @@
>>>> -331,13 +331,16 @@ int chown_debug_file(const char
>>>> *filename,
>>>> 
>>>> ret = chown(logpath, uid, gid); free(logpath); if (ret != 0)
>>>> { ret = errno; -DEBUG(SSSDBG_FATAL_FAILURE,
>>>> "chown failed for [%s]: [%d]\n", -  log_file,
>>>> ret); - return ret; +if (ret != ENOENT) { +
>>>> /* Don't write an error message for a nonexistent file */ + 
>>>> DEBUG(SSSDBG_FATAL_FAILURE, "chown failed for [%s]: [%d]\n",
>>>> + log_file, ret); +return ret; +
>>>> }
>>> Patch make sense, But I cannot see an error message even with
>>> empty directory /var/log/sssd. Do you have an idea why?
>>> 
>> 
>> Start the sssd in interactive mode instead of sending to files
>> and it will become clear :)
>> 
> I'm sorry but it is not clear :-)
> 
> [root@host ~]# rm -f /var/log/sssd/* /var/lib/sss/{db,mc}/* 
> [root@host ~]# sssd -i -d 3 (Wed Oct 28 21:43:57 2015)
> [sssd[be[redhat.com]]] [sysdb_idmap_get_mappings] (0x0080): Could
> not locate ID mappings: [No such file or directory] (Wed Oct 28
> 21:43:57 2015) [sssd[be[redhat.com]]] [be_process_init] (0x0080): 
> No SUDO module provided for [redhat.com] !! (Wed Oct 28 21:43:57
> 2015) [sssd[be[redhat.com]]] [be_process_init] (0x0020): No selinux
> module provided for [redhat.com] !! (Wed Oct 28 21:43:57 2015)
> [sssd[be[redhat.com]]] [be_process_init] (0x0020): No host info
> module provided for [redhat.com] !! (Wed Oct 28 21:43:57 2015)
> [sssd[be[redhat.com]]] [be_process_init] (0x0020): Subdomains are
> not supported for [redhat.com] !! (Wed Oct 28 21:44:27 2015)
> [sssd[be[redhat.com]]] [be_run_online_cb] (0x0080): Going online.
> Running callbacks.
> 
> I think it was fixed in ticket or did I miss something? 
> https://fedorahosted.org/sssd/ticket/2493
> 

It's entirely possible; I was comparing against the latest Fedora
release and I missed that patch going in. Feel free to drop this.

-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlYyGc0ACgkQeiVVYja6o6ObkACePb/EfAA/heF7+VLMALdq05Mt
pE8An1+QkoT8oNfR2OQdacTlb5kwFLl6
=xcV8
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] DEBUG: Don't error on chown of nonexistent file

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/27/2015 05:33 PM, Lukas Slebodnik wrote:
> On (27/10/15 09:48), Stephen Gallagher wrote:
>> We get an error message if we start up SSSD and the debug log
>> does not yet exist.
> 
>> From 53592734f73c50029fa573b9bc070437304ea489 Mon Sep 17 00:00:00
>> 2001 From: Stephen Gallagher  Date: Tue, 27
>> Oct 2015 09:39:01 -0400 Subject: [PATCH] DEBUG: Don't error on
>> chown of nonexistent file
>> 
>> We get an error message if we start up SSSD and the debug log
>> does not yet exist. --- src/util/debug.c | 9 ++--- 1 file
>> changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/src/util/debug.c b/src/util/debug.c index
>> a8eea32740155ec3daf6be71ef9a8af6592f74a9..729d9f99d35c7208950a9a1af1f
df3942b23a147
>> 100644 --- a/src/util/debug.c +++ b/src/util/debug.c @@ -331,13
>> +331,16 @@ int chown_debug_file(const char *filename,
>> 
>> ret = chown(logpath, uid, gid); free(logpath); if (ret != 0) { 
>> ret = errno; -DEBUG(SSSDBG_FATAL_FAILURE, "chown
>> failed for [%s]: [%d]\n", -  log_file, ret); -
>> return ret; +if (ret != ENOENT) { +/*
>> Don't write an error message for a nonexistent file */ +
>> DEBUG(SSSDBG_FATAL_FAILURE, "chown failed for [%s]: [%d]\n", +
>> log_file, ret); +return ret; +}
> Patch make sense, But I cannot see an error message even with empty
> directory /var/log/sssd. Do you have an idea why?
> 

Start the sssd in interactive mode instead of sending to files and it
will become clear :)

-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlYwx58ACgkQeiVVYja6o6NxwACdFgv3zeVpmh7CodmER0Q0t7/u
OpUAoKpHFcXaNX6KhVrM579GaePX/uyz
=dIVo
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] Monitor: Show service pings at debug level 8

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

SSSDBG_CONF_SETTINGS is reserved for configuration information. These
pings are generally just noise (when they fail, this is logged at
SSDBG_FATAL_FAILURE). We should only log these at SSSDBG_TRACE_INTERNAL.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlYvgvQACgkQeiVVYja6o6NeVQCgocyUqrHud6p+KyyDULRdtx+/
Vj0AoIEfXJAbEgwDEgAmDJBuRLNv0v+n
=h2IT
-END PGP SIGNATURE-
>From 9a53c342335ce68ec8196c5d05fecf8e12197411 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 27 Oct 2015 09:55:11 -0400
Subject: [PATCH] Monitor: Show service pings at debug level 8

SSSDBG_CONF_SETTINGS is reserved for configuration information. These
pings are generally just noise (when they fail, this is logged at
SSDBG_FATAL_FAILURE). We should only log these at SSSDBG_TRACE_INTERNAL.
---
 src/monitor/monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index 3776caba4ea8644e57bde098c7a6895093cbb9d3..9a634340424debd64080588ba814b9189ea16d04 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -2528,11 +2528,11 @@ static int service_send_ping(struct mt_svc *svc)
 if (!svc->conn) {
 DEBUG(SSSDBG_TRACE_INTERNAL, "Service not yet initialized\n");
 return ENXIO;
 }
 
-DEBUG(SSSDBG_CONF_SETTINGS,"Pinging %s\n", svc->name);
+DEBUG(SSSDBG_TRACE_INTERNAL,"Pinging %s\n", svc->name);
 
 /*
  * Set up identity request
  * This should be a well-known path and method
  * for all services
@@ -2590,11 +2590,11 @@ static void ping_check(DBusPendingCall *pending, void *data)
 switch (type) {
 case DBUS_MESSAGE_TYPE_METHOD_RETURN:
 /* ok peer replied,
  * make sure we reset the failure counter in the service structure */
 
-DEBUG(SSSDBG_CONF_SETTINGS,"Service %s replied to ping\n", svc->name);
+DEBUG(SSSDBG_TRACE_INTERNAL,"Service %s replied to ping\n", svc->name);
 
 svc->failed_pongs = 0;
 break;
 
 case DBUS_MESSAGE_TYPE_ERROR:
-- 
2.5.0



0001-Monitor-Show-service-pings-at-debug-level-8.patch.sig
Description: PGP signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] DEBUG: Don't error on chown of nonexistent file

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

We get an error message if we start up SSSD and the debug log does not
yet exist.
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlYvgJsACgkQeiVVYja6o6PsvgCfUj+BFPTavtddwIcT8uoCAVpL
2AwAnirJFDFasy+zTf+mGeNCHDwK2ZtE
=g977
-END PGP SIGNATURE-
>From 53592734f73c50029fa573b9bc070437304ea489 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 27 Oct 2015 09:39:01 -0400
Subject: [PATCH] DEBUG: Don't error on chown of nonexistent file

We get an error message if we start up SSSD and the debug log does not
yet exist.
---
 src/util/debug.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/util/debug.c b/src/util/debug.c
index a8eea32740155ec3daf6be71ef9a8af6592f74a9..729d9f99d35c7208950a9a1af1fdf3942b23a147 100644
--- a/src/util/debug.c
+++ b/src/util/debug.c
@@ -331,13 +331,16 @@ int chown_debug_file(const char *filename,
 
 ret = chown(logpath, uid, gid);
 free(logpath);
 if (ret != 0) {
 ret = errno;
-DEBUG(SSSDBG_FATAL_FAILURE, "chown failed for [%s]: [%d]\n",
-  log_file, ret);
-return ret;
+if (ret != ENOENT) {
+/* Don't write an error message for a nonexistent file */
+DEBUG(SSSDBG_FATAL_FAILURE, "chown failed for [%s]: [%d]\n",
+  log_file, ret);
+return ret;
+}
 }
 }
 
 return EOK;
 }
-- 
2.5.0



0001-DEBUG-Don-t-error-on-chown-of-nonexistent-file.patch.sig
Description: PGP signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] LDAP: Inform about small range size

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 10/08/2015 05:16 AM, Lukas Slebodnik wrote:
> On (23/01/15 12:27), Stephen Gallagher wrote:
>> On Fri, 2015-01-23 at 17:27 +0100, Jakub Hrozek wrote:
>>> On Fri, Jan 23, 2015 at 05:24:51PM +0100, Michal Židek wrote:
>>>> On 01/23/2015 04:35 PM, Lukas Slebodnik wrote:
>>>>> On (23/01/15 10:21), Stephen Gallagher wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Fri, 2015-01-23 at 14:39 +0100, Lukas Slebodnik
>>>>>> wrote:
>>>>>>> ehlo,
>>>>>>> 
>>>>>>> I was reprodicing other bug and it took me some time to
>>>>>>> find out why I was not able to resolve user. RID was
>>>>>>> bigger than range size.
>>>>>>> 
>>>>>>> I saw just general message about id mapping failer 
>>>>>>> [sdap_save_user] (0x0400): Processing user matthewbe 
>>>>>>> [sdap_save_user] (0x1000): Mapping user [matthewbe]
>>>>>>> objectSID 
>>>>>>> [S-1-5-21-2997650941-1802118864-3094776726-200065] to
>>>>>>> unix ID [sdap_idmap_sid_to_unix] (0x0080): Could not
>>>>>>> convert objectSID 
>>>>>>> [S-1-5-21-2997650941-1802118864-3094776726-200065] to a
>>>>>>> UNIX ID ^^ Default range size is 20 
>>>>>>> [sdap_save_user] (0x0020): Failed to save user
>>>>>>> [matthewbe] [sdap_save_users] (0x0040): Failed to store
>>>>>>> user 0. Ignoring.
>>>>>>> 
>>>>>>> 
>>>>>>> Feel free to propose better debug message. I think it
>>>>>>> would simplify debugging.
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> I'd avoid making a recommendation about changing the
>>>>>> range size. That will result in any other domain having
>>>>>> all of their IDs changed. That's not a good situation. We
>>>>>> should certainly log this at a very low level, though.
>>>>> 
>>>>> As you can see from debug message it is almost impossible
>>>>> to say why converting of objectSID failed. I have already
>>>>> seen such problem in customer reports. and reasonable hint
>>>>> could speed up fixing of a problem.
>>>>> 
>>>>> user issue -> bug report -> requests for log files -> 
>>>>> analysis of log file -> advice to increase
>>>>> ldap_idmap_range_size
>>>>> 
>>>>> 
>>>>> Maybe we can recommend just to double value of range size, 
>>>>> but current situation isn't user friendly.
>>>>> 
>>>>> LS
>>>> 
>>>> I think there is just problem with wording here. You used
>>>> "You should..." in the debug message. I would change it to
>>>> "You could/can..." and add a sentence that warns the user
>>>> about the consequences. like "But be careful because changing
>>>> the range size will also change the ID mappings in all
>>>> trusted domains." Or some better warning.
>>> 
>>> We can also point the user to the man page in the debug message
>>> to avoid being overly terse...then in the man page, we can
>>> explain all the pros and cons better.
>> 
>> Yeah, I like this approach. So maybe the phrasing should be:
>> 
>> "objectSID [%s] has a RID that is larger than the
>> ldap_idmap_range_size. See sssd-ad(5) for an explanation of how
>> to resolve this issue."
>> 
> I reminded this thread after seeing another BZ with such issue. But
> I would like to provide more hints. Maybe incorporate "ID MAPPING"
> section to the debug message and/or 3rd paragraph in this section.
> 
> Stephen, could you help with new message (or even prepare  updated
> patch.)
> 
> LS
> 

Updated patch attached. I didn't change anything in the manpage
because there's already an involved section explaining the mechanism
for and consequences of changing the ID range values.

-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iEYEARECAAYFAlYWjhcACgkQeiVVYja6o6PovACfU5O8wcBDW6Bb07h5fF9GMQ2U
w3gAn26RVDFjwdS0dWXOb8F31mIZoxjU
=JQ25
-END PGP SIGNATURE-
>From a710c8cbfd37a2851c29f8f57a278d1b92f584ef Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Thu, 8 Oct 2015 11:33:30 -0400
Sub

[SSSD] [PATCH] AD: Handle cases where no GPOs apply

[Stephen Gallagher]

It is possible to have a machine where none of the GPOs associated with
it include access-control rules. Currently, this results in a
denial-by-system-error.

We need to treat this case as allowing the user (see the test cases in
https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryGPOIntegra
tion

We also need to delete the result object from the cache to ensure that
offline operation will also grant access.

Resolves:
https://fedorahosted.org/sssd/ticket/2691From 06e58a26fd5b59631b479f2f076e80ecfae425b8 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Mon, 20 Jul 2015 09:29:19 -0400
Subject: [PATCH] AD: Handle cases where no GPOs apply

It is possible to have a machine where none of the GPOs associated with
it include access-control rules. Currently, this results in a
denial-by-system-error.

We need to treat this case as allowing the user (see the test cases in
https://fedorahosted.org/sssd/wiki/DesignDocs/ActiveDirectoryGPOIntegration

We also need to delete the result object from the cache to ensure that
offline operation will also grant access.

Resolves:
https://fedorahosted.org/sssd/ticket/2691
---
 src/providers/ad/ad_gpo.c | 46 +++---
 1 file changed, 43 insertions(+), 3 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 974fd04b99709055f25ed2a3b77821b3caec09ad..0d310b87696feb810b6a096d31adede38c72d16a 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1947,15 +1947,37 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
 
 talloc_zfree(subreq);
 
 ret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
 
-if (ret != EOK) {
+if (ret != EOK && ret != ENOENT) {
 DEBUG(SSSDBG_OP_FAILURE,
   "Unable to get GPO list: [%d](%s)\n",
   ret, sss_strerror(ret));
-ret = ENOENT;
+goto done;
+} else if (ret == ENOENT) {
+DEBUG(SSSDBG_OP_FAILURE,
+  "No GPOs found that apply to this system.\n");
+/*
+ * Delete the result object list, since there are no
+ * GPOs to include in it.
+ */
+ret = sysdb_gpo_delete_gpo_result_object(state, state->host_domain);
+if (ret != EOK) {
+switch (ret) {
+case ENOENT:
+DEBUG(SSSDBG_TRACE_FUNC, "No GPO Result available in cache\n");
+break;
+default:
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "Could not delete GPO Result from cache: [%s]\n",
+  sss_strerror(ret));
+goto done;
+}
+}
+
+ret = EOK;
 goto done;
 }
 
 ret = ad_gpo_filter_gpos_by_dacl(state, state->user, state->user_domain,
  state->opts->idmap_ctx->map,
@@ -1971,10 +1993,29 @@ ad_gpo_process_gpo_done(struct tevent_req *subreq)
 
 if (state->dacl_filtered_gpos[0] == NULL) {
 /* since no applicable gpos were found, there is nothing to enforce */
 DEBUG(SSSDBG_TRACE_FUNC,
   "no applicable gpos found after dacl filtering\n");
+
+/*
+ * Delete the result object list, since there are no
+ * GPOs to include in it.
+ */
+ret = sysdb_gpo_delete_gpo_result_object(state, state->host_domain);
+if (ret != EOK) {
+switch (ret) {
+case ENOENT:
+DEBUG(SSSDBG_TRACE_FUNC, "No GPO Result available in cache\n");
+break;
+default:
+DEBUG(SSSDBG_FATAL_FAILURE,
+  "Could not delete GPO Result from cache: [%s]\n",
+  sss_strerror(ret));
+goto done;
+}
+}
+
 ret = EOK;
 goto done;
 }
 
 for (i = 0; i < state->num_dacl_filtered_gpos; i++) {
@@ -3420,11 +3461,10 @@ ad_gpo_process_gpo_send(TALLOC_CTX *mem_ctx,
 
 if (ret != EOK) {
 DEBUG(SSSDBG_OP_FAILURE,
   "Unable to retrieve GPO List: [%d](%s)\n",
   ret, sss_strerror(ret));
-ret = ENOENT;
 goto immediately;
 }
 
 if (state->candidate_gpos == NULL) {
 DEBUG(SSSDBG_OP_FAILURE, "no gpos found\n");
-- 
2.4.3



signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] RFC: Improving the debug messages

[Stephen Gallagher]

On Tue, 2015-06-30 at 12:04 +0200, Jakub Hrozek wrote:
> On Tue, Jun 30, 2015 at 10:30:16AM +0200, Jan Pazdziora wrote:
> > > - Running sssd in environment where all actions complete 
> > > successfully
> > >   should emit no debug messages. Default log level should be 
> > > moved to
> > >   SSSDBG_OP_FAILURE or CRIT_FAILURE. (This basically amounts to 
> > > checking
> > >   all OP, FATAL and CRIT failure messages..)
> > > 
> > >   The reason is that sometimes sssd fails, but because logging is
> > >   totally silent, we don't know what happened at all. Currently 
> > > we have
> > >   a couple of small bugs where we might print a loud DEBUG 
> > > message just
> > >   because we search for an entry which is not there etc.
> > 
> > It'd be great if the default option was to emit one error message 
> > per
> > failure (per operation, where operation should be defined from 
> > user's
> > point of view, not developer's point of view) and no message for
> > success. The next level would be to have one error message per 
> > failure
> > and one message per success, so that it's easy to observe what is
> > going on without getting into too much detail.
> 
> What someone (sorry, I already forgot who :-/) also suggested during 
> a
> discussion was to add a new log level for start and end of an 
> operation.


The other thing we might want to be doing is looking at some of the
more advanced journald logging capabilities. For example, if you look
at the Cockpit and realmd projects, they have implemented a mechanism
to use journald tagging to easily identify activities being performed
by specific clients. We should do the same, so it's possible to limit
our view of the journal to just the actions we're interested in (which
will make life easier on a busy server)

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [SSSD-users] Announcing SSSD 1.12.5

[Stephen Gallagher]

On Fri, 2015-06-12 at 21:30 +0200, Jakub Hrozek wrote:
> On Fri, Jun 12, 2015 at 06:33:16PM +0200, Lukas Slebodnik wrote:
> > On (12/06/15 16:45), Jakub Hrozek wrote:
> > >   === SSSD 1.12.5 ===
> > > 
> > > The SSSD team is proud to announce the release of version 1.12.5 
> > > of
> > > the System Security Services Daemon.
> > > 
> > > As always, the source is available from 
> > > https://fedorahosted.org/sssd
> > > 
> > > RPM packages will be made available for Fedora 21, 22 and rawhide 
> > > shortly.
> > > 
> > Packages for some older distributions then fedora 21 are available 
> > in
> > COPR 
> > http://copr-fe.cloud.fedoraproject.org/coprs/lslebodn/sssd-1-12/
> 
> This repo has been really useful. I wonder if we should promote it 
> more
> officially, on the sssd wiki pages?
> 

Seems reasonable to me.


> btw does COPR allow to set up some kind of a team account?

No, but you can grant other FAS users privileges on your account, which
is functionally what you're looking for, I think.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: Fix incorrect strerror on GPO access denial

[Stephen Gallagher]

On Thu, 2015-06-11 at 16:19 +0200, Lukas Slebodnik wrote:
> On (11/06/15 09:35), Stephen Gallagher wrote:
> > On Thu, 2015-06-11 at 09:19 -0400, Stephen Gallagher wrote:
> > > We're attempting to use strerror() to print the result from
> > > ad_gpo_access_check(), but that function returns an extended SSSD
> > > errno.
> > > 
> > > This resulted in "Unknown Error" being printed to the logs.
> > 
> > And now with the patch attached...
> 
> > From 108c6e9f47560a53d2d044c0399e66528615d73d Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Thu, 11 Jun 2015 09:17:02 -0400
> > Subject: [PATCH] GPO: Fix incorrect strerror on GPO access denial
> > 
> > We're attempting to use strerror() to print the result from
> > ad_gpo_access_check(), but that function returns an extended SSSD 
> > errno
> > ---
> > src/providers/ad/ad_gpo.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
> > index 
> > 4e7afb12a73313ece00c6ffdd24cfad1b73a831d..995bd083b27710814350bef8d1e43b874d5a8a27
> >  
> > 100644
> > --- a/src/providers/ad/ad_gpo.c
> > +++ b/src/providers/ad/ad_gpo.c
> > @@ -1446,11 +1446,11 @@ ad_gpo_perform_hbac_processing(TALLOC_CTX 
> > *mem_ctx,
> >   deny_size);
> > 
> > if (ret != EOK) {
> > DEBUG(SSSDBG_OP_FAILURE,
> >   "GPO access check failed: [%d](%s)\n",
> > -  ret, strerror(ret));
> > +  ret, sss_strerror(ret));
> > goto done;
> > }
> 
> You can also replace strerror on line 1152 after 
> ad_gpo_extract_policy_setting
>   1178 after 
> ad_gpo_extract_policy_setting
>   1673 after 
> sdap_id_op_connect_recv
>   1995 after 
> ad_gpo_filter_gpos_by_cse_guid
>   2044 after 
> sysdb_gpo_delete_gpo_result_object
>   2136 after 
> sysdb_gpo_get_gpo_by_guid
>   4037 after 
> ad_gpo_parse_gpo_child_response
> 
> But strerror can be replaced on all places with sss_strerror
> because in value is not within sssd error code range then strerror is 
> called.


To be clear, is this a Nack? I was really only trying to address a
specific message that was proving to be misleading. If you really want
me to replace all instances of strerror(), that's a bigger effort.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: Fix incorrect strerror on GPO access denial

[Stephen Gallagher]

On Thu, 2015-06-11 at 09:19 -0400, Stephen Gallagher wrote:
> We're attempting to use strerror() to print the result from
> ad_gpo_access_check(), but that function returns an extended SSSD
> errno.
> 
> This resulted in "Unknown Error" being printed to the logs.

And now with the patch attached...
From 108c6e9f47560a53d2d044c0399e66528615d73d Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Thu, 11 Jun 2015 09:17:02 -0400
Subject: [PATCH] GPO: Fix incorrect strerror on GPO access denial

We're attempting to use strerror() to print the result from
ad_gpo_access_check(), but that function returns an extended SSSD errno
---
 src/providers/ad/ad_gpo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 4e7afb12a73313ece00c6ffdd24cfad1b73a831d..995bd083b27710814350bef8d1e43b874d5a8a27 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1446,11 +1446,11 @@ ad_gpo_perform_hbac_processing(TALLOC_CTX *mem_ctx,
   deny_size);
 
 if (ret != EOK) {
 DEBUG(SSSDBG_OP_FAILURE,
   "GPO access check failed: [%d](%s)\n",
-  ret, strerror(ret));
+  ret, sss_strerror(ret));
 goto done;
 }
 
  done:
 return ret;
-- 
2.4.2



signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] GPO: Fix incorrect strerror on GPO access denial

[Stephen Gallagher]

We're attempting to use strerror() to print the result from
ad_gpo_access_check(), but that function returns an extended SSSD
errno.

This resulted in "Unknown Error" being printed to the logs.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Add Vagrant configuration for SSSD

[Stephen Gallagher]

On Wed, 2015-05-27 at 15:54 -0400, Stephen Gallagher wrote:
> On Wed, 2015-05-27 at 21:36 +0200, Lukas Slebodnik wrote:
> > On (27/05/15 15:30), Stephen Gallagher wrote:
> > > On Wed, 2015-05-27 at 13:31 -0400, Stephen Gallagher wrote:
> > > > To set up a Vagrant development environment:
> > > > * Install the Vagrant packages for your development system
> > > >   * On Fedora 22 and later: 'dnf install vagrant-libvirt'
> > > > * Deploy the Vagrant box:
> > > >   * 'vagrant up'
> > > > * Build SSSD:
> > > >   * vagrant ssh -c "cd /vagrant; reconfig; chmake"
> > > > 
> > > > Vagrant can keep your development tree in-sync with the Vagrant 
> > > > 
> > > > box
> > > > by running 'vagrant rsync-auto' in a shell (this will continue 
> > > > to
> > > > run, monitoring for changes and syncing them as they are 
> > > > saved).
> > > > Alternately, it can be manually synced with 'vagrant rsync' at 
> > > > will.
> > > 
> > > One minor update; added a few more packages to the deployment
> > > installation (adcli, oddjob-mkhomedir and polkit) to support 
> > > realmd.
> > 
> > > From a652a5ea9233b1cac6a84b205a4e71cc9b659aaf Mon Sep 17 00:00:00 
> > > 
> > > 2001
> > > From: Stephen Gallagher 
> > > Date: Wed, 27 May 2015 13:17:40 -0400
> > > Subject: [PATCH] Add Vagrant configuration for SSSD
> > > 
> > > To set up a Vagrant development environment:
> > > * Install the Vagrant packages for your development system
> > >  * On Fedora 22 and later: 'dnf install vagrant-libvirt'
> > > * Deploy the Vagrant box:
> > >  * 'vagrant up'
> > > * Build SSSD:
> > >  * vagrant ssh -c "cd /vagrant; reconfig; chmake"
> > > 
> > > Vagrant can keep your development tree in-sync with the Vagrant 
> > > box
> > > by running 'vagrant rsync-auto' in a shell (this will continue to
> > > run, monitoring for changes and syncing them as they are saved).
> > > Alternately, it can be manually synced with 'vagrant rsync' at 
> > > will.
> > > ---
> > > Vagrantfile  | 74 
> > > 
> > > vagrant/bootstrap.sh | 21 +++
> > Could it be in contrib directory?
> > 
> 
> Well, bootstrap.sh could be, I suppose. Unfortunately, "Vagrantfile"
> must always be in the root of the source tree.


Sorry, this response failed to ask: Is this a nack? Do you want me to
move the bootstrap.sh file and resubmit it?


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] SSSDConfigTest: Use unique temporary directory

[Stephen Gallagher]

On Fri, 2015-05-22 at 16:13 +0200, Jakub Hrozek wrote:
> On Thu, May 21, 2015 at 01:43:19PM +0200, Lukas Slebodnik wrote:
> > ehlo,
> > 
> > There were some failed tests in ci log
> > http://sssd-ci.duckdns.org/logs/job/12/67/fedora_rawhide/ci.html
> > http://sssd-ci.duckdns.org/logs/job/14/07/fedora_rawhide/ci.html
> > http://sssd-ci.duckdns.org/logs/job/14/97/debian_testing/ci.html
> > 
> > Attached patch should fix race condition between two SSSDConfigTest 
> > tests.
> > The problem is just in master due to python{2,3} changes.
> > 
> > LS
> 
> The intent is good, but I would prefer using the setUp() and 
> tearDown()
> methods for creating and removing the directory. It's safe because 
> the
> Python documentation says:
> If setUp() succeeded, the tearDown() method will be run whether
> runTest() succeeded or not.


It's ambiguous if there is an unhandled exception in the tests, isn't
it?

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] Support GPOs referred from other domains

[Stephen Gallagher]

On Wed, 2015-05-27 at 11:15 +0200, Jakub Hrozek wrote:
> On Tue, May 26, 2015 at 03:56:35PM -0400, Stephen Gallagher wrote:
> > Sorry for the delay; two new patches attached.
> > 
> > This patch fixes the two missing error checks in the AD GPO code as
> > well as making several changes to the general LDAP support of
> > referrals.
> > 
> > While I was looking through this, I discovered a bug that resulted 
> > in
> > referrals for more information not being returned to the caller 
> > (this
> > is different from referral-as-final-result). I tweaked the code so 
> > that
> > this would come back as well. I also added some extra debugging at
> > level 9 so we can see when these occur, what they were and that 
> > they
> > were ignored.
> > 
> > As discussed above, I just changed the referral check around
> > sdap_get_generic_ext_recv() to check for ref_count > 0 instead of
> > ERR_REFERRALS. I didn't remove that completely from the system just 
> > in
> > case we decide to use the error for something else involving 
> > referrals
> > in the future.
> > 
> > I retested these patches against the problematic cross-realm GPO 
> > case,
> > which worked successfully.
> 
> 
> > From 20191f9c34336b3920db8d5774593c4562cefdb7 Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Fri, 1 May 2015 11:42:06 -0400
> > Subject: [PATCH 1/2] LDAP: Support returning referral information
> 
> Thank you, this was really useful. I tested the referral patch with
> universal groups and GC support disabled which always triggers 
> referrals
> to the trusted domains. The referrals were ignored as they're 
> supposed
> to, so we're not going to regress there..
> 
> ACK
> 
> > From 9369111f0775cbc4c10b5857046c756ae510033f Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Fri, 1 May 2015 16:26:36 -0400
> > Subject: [PATCH 2/2] AD GPO: Support processing referrals
> > 
> > For GPOs assigned to a site, it's possible that their definition
> > actually exists in another domain. To retrieve this information,
> > we need to follow the referral and perform a base search on
> > another domain controller.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2645
> 
> Some nitpicks are inline, but mostly the patch looks good!
> 
> [...]
> 
> > +
> > +subreq = ad_gpo_get_sd_referral_send(state, state->ev,
> > + state
> > ->access_ctx,
> > + state->opts,
> > + refs[0],
> > + state
> > ->host_domain,
> > + state->timeout);
> 
> Missing NULL check for subreq
> 

Good catch. Fixed.


> > +/* Request the referred GPO data */
> > +subreq = sdap_sd_search_send(state, state->ev, state->opts,
> > + sdap_id_op_handle(state->ref_op),
> > + state->gpo_dn,
> > + SECINFO_DACL,
> > + attrs,
> > + state->timeout);
> > +if (subreq == NULL) {
> > +DEBUG(SSSDBG_OP_FAILURE, "sdap_sd_search_send failed.\n");
> 
> Copy-n-paste bug; the error handler should read:
>tevent_req_error(req, ENOMEM);
>return;
> 
> Interestingly enough, gcc emits a warning here, but Coverity didn't
> catch this bug..
> 

I'm curious what warning GCC saw. In any case, fixed.

> > +return ENOMEM;
> > +}
> > +tevent_req_set_callback(subreq, 
> > ad_gpo_get_sd_referral_search_done, req);
> > +
> > +}
> 
> [...]
> 
> > index 
> > 2ffc2a170c2277aa2ea40e84bb697c62542aa266..760fb3df5148f46bf0e7e0fdb9110456685a914c
> >  
> > 100644
> > --- a/src/providers/ldap/sdap_async.c
> > +++ b/src/providers/ldap/sdap_async.c
> 
> The changes to the sdap_async module look OK to me, but can you split
> them into a separate patch, please?
> 

I considered it, but then decided against splitting it. There really
isn't sufficient value in splitting it. The sdap_async.c portion is
fairly short and clearly only useful in context.


> >  int sdap_sd_search_recv(struct tevent_req *req,
> >  TALLOC_CTX *mem_ctx,
> >  

Re: [SSSD] [PATCH] Add Vagrant configuration for SSSD

[Stephen Gallagher]

On Wed, 2015-05-27 at 21:36 +0200, Lukas Slebodnik wrote:
> On (27/05/15 15:30), Stephen Gallagher wrote:
> > On Wed, 2015-05-27 at 13:31 -0400, Stephen Gallagher wrote:
> > > To set up a Vagrant development environment:
> > > * Install the Vagrant packages for your development system
> > >   * On Fedora 22 and later: 'dnf install vagrant-libvirt'
> > > * Deploy the Vagrant box:
> > >   * 'vagrant up'
> > > * Build SSSD:
> > >   * vagrant ssh -c "cd /vagrant; reconfig; chmake"
> > > 
> > > Vagrant can keep your development tree in-sync with the Vagrant 
> > > box
> > > by running 'vagrant rsync-auto' in a shell (this will continue to
> > > run, monitoring for changes and syncing them as they are saved).
> > > Alternately, it can be manually synced with 'vagrant rsync' at 
> > > will.
> > 
> > One minor update; added a few more packages to the deployment
> > installation (adcli, oddjob-mkhomedir and polkit) to support 
> > realmd.
> 
> > From a652a5ea9233b1cac6a84b205a4e71cc9b659aaf Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Wed, 27 May 2015 13:17:40 -0400
> > Subject: [PATCH] Add Vagrant configuration for SSSD
> > 
> > To set up a Vagrant development environment:
> > * Install the Vagrant packages for your development system
> >  * On Fedora 22 and later: 'dnf install vagrant-libvirt'
> > * Deploy the Vagrant box:
> >  * 'vagrant up'
> > * Build SSSD:
> >  * vagrant ssh -c "cd /vagrant; reconfig; chmake"
> > 
> > Vagrant can keep your development tree in-sync with the Vagrant box
> > by running 'vagrant rsync-auto' in a shell (this will continue to
> > run, monitoring for changes and syncing them as they are saved).
> > Alternately, it can be manually synced with 'vagrant rsync' at 
> > will.
> > ---
> > Vagrantfile  | 74 
> > 
> > vagrant/bootstrap.sh | 21 +++
> Could it be in contrib directory?
> 

Well, bootstrap.sh could be, I suppose. Unfortunately, "Vagrantfile"
must always be in the root of the source tree.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Add Vagrant configuration for SSSD

[Stephen Gallagher]

On Wed, 2015-05-27 at 13:31 -0400, Stephen Gallagher wrote:
> To set up a Vagrant development environment:
> * Install the Vagrant packages for your development system
>   * On Fedora 22 and later: 'dnf install vagrant-libvirt'
> * Deploy the Vagrant box:
>   * 'vagrant up'
> * Build SSSD:
>   * vagrant ssh -c "cd /vagrant; reconfig; chmake"
> 
> Vagrant can keep your development tree in-sync with the Vagrant box
> by running 'vagrant rsync-auto' in a shell (this will continue to
> run, monitoring for changes and syncing them as they are saved).
> Alternately, it can be manually synced with 'vagrant rsync' at will.

One minor update; added a few more packages to the deployment
installation (adcli, oddjob-mkhomedir and polkit) to support realmd.From a652a5ea9233b1cac6a84b205a4e71cc9b659aaf Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 27 May 2015 13:17:40 -0400
Subject: [PATCH] Add Vagrant configuration for SSSD

To set up a Vagrant development environment:
* Install the Vagrant packages for your development system
  * On Fedora 22 and later: 'dnf install vagrant-libvirt'
* Deploy the Vagrant box:
  * 'vagrant up'
* Build SSSD:
  * vagrant ssh -c "cd /vagrant; reconfig; chmake"

Vagrant can keep your development tree in-sync with the Vagrant box
by running 'vagrant rsync-auto' in a shell (this will continue to
run, monitoring for changes and syncing them as they are saved).
Alternately, it can be manually synced with 'vagrant rsync' at will.
---
 Vagrantfile  | 74 
 vagrant/bootstrap.sh | 21 +++
 2 files changed, 95 insertions(+)
 create mode 100644 Vagrantfile
 create mode 100644 vagrant/bootstrap.sh

diff --git a/Vagrantfile b/Vagrantfile
new file mode 100644
index ..31fd3a048ecd2626b43f015c2704e6d2f2e43669
--- /dev/null
+++ b/Vagrantfile
@@ -0,0 +1,74 @@
+# -*- mode: ruby -*-
+# vi: set ft=ruby :
+
+# All Vagrant configuration is done below. The "2" in Vagrant.configure
+# configures the configuration version (we support older styles for
+# backwards compatibility). Please don't change it unless you know what
+# you're doing.
+Vagrant.configure(2) do |config|
+  # The most common configuration options are documented and commented below.
+  # For a complete reference, please see the online documentation at
+  # https://docs.vagrantup.com.
+
+  # Every Vagrant development environment requires a box. You can search for
+  # boxes at https://atlas.hashicorp.com/search.
+  config.vm.box = "fedora-22"
+  config.vm.box_url = "http://download.fedoraproject.org/pub/fedora/linux/releases/22/Cloud/x86_64/Images/Fedora-Cloud-Base-Vagrant-22-20150521.x86_64.vagrant-libvirt.box";
+
+  # Disable automatic box update checking. If you disable this, then
+  # boxes will only be checked for updates when the user runs
+  # `vagrant box outdated`. This is not recommended.
+  # config.vm.box_check_update = false
+
+  # Create a forwarded port mapping which allows access to a specific port
+  # within the machine from a port on the host machine. In the example below,
+  # accessing "localhost:8080" will access port 80 on the guest machine.
+  # config.vm.network "forwarded_port", guest: 80, host: 8080
+
+  # Create a private network, which allows host-only access to the machine
+  # using a specific IP.
+  # config.vm.network "private_network", ip: "192.168.33.10"
+
+  # Create a public network, which generally matched to bridged network.
+  # Bridged networks make the machine appear as another physical device on
+  # your network.
+  # config.vm.network "public_network"
+
+  # Share an additional folder to the guest VM. The first argument is
+  # the path on the host to the actual folder. The second argument is
+  # the path on the guest to mount the folder. And the optional third
+  # argument is a set of non-required options.
+  # config.vm.synced_folder "../data", "/vagrant_data"
+
+  # Provider-specific configuration so you can fine-tune various
+  # backing providers for Vagrant. These expose provider-specific options.
+  # Example for VirtualBox:
+  #
+  # config.vm.provider "virtualbox" do |vb|
+  #   # Display the VirtualBox GUI when booting the machine
+  #   vb.gui = true
+  #
+  #   # Customize the amount of memory on the VM:
+  #   vb.memory = "1024"
+  # end
+  #
+  # View the documentation for the provider you are using for more
+  # information on available options.
+
+  # Define a Vagrant Push strategy for pushing to Atlas. Other push strategies
+  # such as FTP and Heroku are also available. See the documentation at
+  # https://docs.vagrantup.com/v2/push/atlas.html for more information.
+  # config.push.define "atla

Re: [SSSD] [PATCHES] Support GPOs referred from other domains

[Stephen Gallagher]

On Fri, 2015-05-22 at 13:04 +0200, Jakub Hrozek wrote:
> On Thu, May 14, 2015 at 05:58:49PM +0200, Jakub Hrozek wrote:
> > On Thu, May 14, 2015 at 11:49:17AM -0400, Stephen Gallagher wrote:
> > > On Thu, 2015-05-14 at 17:42 +0200, Jakub Hrozek wrote:
> > > > On Wed, May 06, 2015 at 02:26:30PM -0400, Stephen Gallagher 
> > > > wrote:
> > > > > Patch 0001: LDAP: Support returning referral information
> > > > > 
> > > > > Some callers may be interested in the raw referral values 
> > > > > returned 
> > > > > from
> > > > > a lookup. This patch allows interested consumers to get these 
> > > > > 
> > > > > referrals
> > > > > back and process them if they wish. It does not implement a 
> > > > > generic
> > > > > automatic following of referrals.
> > > > > 
> > > > > 
> > > > > 
> > > > > Patch 0002: AD GPO: Support processing referrals
> > > > > 
> > > > > For GPOs assigned to a site, it's possible that their 
> > > > > definition
> > > > > actually exists in another domain. To retrieve this 
> > > > > information,
> > > > > we need to follow the referral and perform a base search on
> > > > > another domain controller.
> > > > > 
> > > > > Resolves:
> > > > > https://fedorahosted.org/sssd/ticket/2645
> > > > 
> > > > Thanks a lot for the patches! I have one questions about the 
> > > > first 
> > > > patch
> > > > and two nitpicks about the second. I'm also still waiting for 
> > > > the
> > > > Coverity results, but the queue seems to be quite long..
> > > > 
> > > > > From 3f8826061d34639ddaaf245947085ea577e77fbe Mon Sep 17 
> > > > > 00:00:00 
> > > > > 2001
> > > > > From: Stephen Gallagher 
> > > > > Date: Fri, 1 May 2015 11:42:06 -0400
> > > > > Subject: [PATCH 1/2] LDAP: Support returning referral 
> > > > > information
> > > > > 
> > > > > Some callers may be interested in the raw referral values 
> > > > > returned 
> > > > > from
> > > > > a lookup. This patch allows interested consumers to get these 
> > > > > 
> > > > > referrals
> > > > > back and process them if they wish. It does not implement a 
> > > > > generic
> > > > > automatic following of referrals.
> > > > 
> > > > [...]
> > > > 
> > > > >  } else if (result == LDAP_REFERRAL) {
> > > > > -if (refs != NULL) {
> > > > > -for (i = 0; refs[i]; i++) {
> > > > > -DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", 
> > > > > refs[i]);
> > > > > -}
> > > > > -ldap_memvfree((void **) refs);
> > > > > +ret = sdap_get_generic_ext_add_references(state, 
> > > > > 
> > > > > refs);
> > > > > +if (ret != EOK) {
> > > > > +DEBUG(SSSDBG_OP_FAILURE,
> > > > > +  "sdap_get_generic_ext_add_references 
> > > > > failed: 
> > > > > %s(%d)",
> > > > > +  sss_strerror(ret), ret);
> > > > > +tevent_req_error(req, ret);
> > > > >  }
> > > > > -ldap_memfree(errmsg);
> > > > > -tevent_req_error(req, ERR_REFERRAL);
> > > > > -return;
> > > > 
> > > > This is a change in behaviour. Previously, we would always 
> > > > return
> > > > ERR_REFERRAL and let the caller handle the error code based on 
> > > > whether
> > > > the caller ignores referrals (like the AD Provider does by 
> > > > default) 
> > > > or
> > > > not.
> > > > 
> > > > Do you think it's OK to always treat referrals as a success 
> > > > now, even
> > > > for the plain LDAP provider? If so, then I guess we can remove 
> > > > the
> > > > ERR_REFERRAL special-case and the error code as well.
> > > > 
> > > 
> > > Hmm, that's a diff

Re: [SSSD] [PATCHES] Support GPOs referred from other domains

[Stephen Gallagher]

On Thu, 2015-05-14 at 17:42 +0200, Jakub Hrozek wrote:
> On Wed, May 06, 2015 at 02:26:30PM -0400, Stephen Gallagher wrote:
> > Patch 0001: LDAP: Support returning referral information
> > 
> > Some callers may be interested in the raw referral values returned 
> > from
> > a lookup. This patch allows interested consumers to get these 
> > referrals
> > back and process them if they wish. It does not implement a generic
> > automatic following of referrals.
> > 
> > 
> > 
> > Patch 0002: AD GPO: Support processing referrals
> > 
> > For GPOs assigned to a site, it's possible that their definition
> > actually exists in another domain. To retrieve this information,
> > we need to follow the referral and perform a base search on
> > another domain controller.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2645
> 
> Thanks a lot for the patches! I have one questions about the first 
> patch
> and two nitpicks about the second. I'm also still waiting for the
> Coverity results, but the queue seems to be quite long..
> 
> > From 3f8826061d34639ddaaf245947085ea577e77fbe Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Fri, 1 May 2015 11:42:06 -0400
> > Subject: [PATCH 1/2] LDAP: Support returning referral information
> > 
> > Some callers may be interested in the raw referral values returned 
> > from
> > a lookup. This patch allows interested consumers to get these 
> > referrals
> > back and process them if they wish. It does not implement a generic
> > automatic following of referrals.
> 
> [...]
> 
> >  } else if (result == LDAP_REFERRAL) {
> > -if (refs != NULL) {
> > -for (i = 0; refs[i]; i++) {
> > -DEBUG(SSSDBG_TRACE_LIBS, "Ref: %s\n", 
> > refs[i]);
> > -}
> > -ldap_memvfree((void **) refs);
> > +ret = sdap_get_generic_ext_add_references(state, 
> > refs);
> > +if (ret != EOK) {
> > +DEBUG(SSSDBG_OP_FAILURE,
> > +  "sdap_get_generic_ext_add_references failed: 
> > %s(%d)",
> > +  sss_strerror(ret), ret);
> > +tevent_req_error(req, ret);
> >  }
> > -ldap_memfree(errmsg);
> > -tevent_req_error(req, ERR_REFERRAL);
> > -return;
> 
> This is a change in behaviour. Previously, we would always return
> ERR_REFERRAL and let the caller handle the error code based on 
> whether
> the caller ignores referrals (like the AD Provider does by default) 
> or
> not.
> 
> Do you think it's OK to always treat referrals as a success now, even
> for the plain LDAP provider? If so, then I guess we can remove the
> ERR_REFERRAL special-case and the error code as well.
> 

Hmm, that's a difficult question. I'd *prefer* to always return the
reference list (as I'm doing here) and leave the decision on whether or
how to handle it up to the callers. But you're right, it's a change in
behavior right now.

That being said, it shouldn't functionally be any different, since we
pretty much ignore ERR_REFERRAL anyway (we only return the records we
actually received, if any). Or am I mistaken in that?


> > +/* For referrals, we need to fall through as if it was 
> > LDAP_SUCCESS */
> >  } else if (result != LDAP_SUCCESS && result != 
> > LDAP_NO_SUCH_OBJECT) {
> >  DEBUG(SSSDBG_OP_FAILURE,
> >"Unexpected result from ldap: %s(%d), %s\n",
> > sss_ldap_err2string(result), result,
> > errmsg ? errmsg : "no errmsg set");
> 
> > From 9a45e8523540c9eff8a8b9d84da81c9ee77a91de Mon Sep 17 00:00:00 
> > 2001
> > From: Stephen Gallagher 
> > Date: Fri, 1 May 2015 16:26:36 -0400
> > Subject: [PATCH 2/2] AD GPO: Support processing referrals
> > 
> > For GPOs assigned to a site, it's possible that their definition
> > actually exists in another domain. To retrieve this information,
> > we need to follow the referral and perform a base search on
> > another domain controller.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2645
> 
> [...]
> 
> > +static void
> > +ad_gpo_get_sd_referral_conn_done(struct tevent_req *subreq)
> > +{
> > +errno_t ret;
> > +int dp_error;
> > +const char *attrs[] = AD_GPO_ATTRS;
> > +
> > +struct teve

Re: [SSSD] [PATCH] Amend the man page for refresh_expired_interval

[Stephen Gallagher]

On Mon, 2015-05-11 at 09:52 +0200, Jakub Hrozek wrote:
> Hi,
> 
> while triaging a performance-related issue, I realized our manpage
> doesn't say also users and groups are now supported by the background
> refresh. The attached patch fixes that.


I'd recommend the phrasing:

"The background refresh will process users, groups and netgroups in the
cache."

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] LDAP: disable the cleanup task by default

[Stephen Gallagher]

On Mon, 2015-05-11 at 19:15 +0200, Jakub Hrozek wrote:
> On Mon, May 11, 2015 at 03:18:55PM +0200, Lukas Slebodnik wrote:
> > On (11/05/15 12:51), Jakub Hrozek wrote:
> > > On Mon, May 11, 2015 at 11:15:29AM +0200, Lukas Slebodnik wrote:
> > > > Please document in man pages that it is not possible to turn 
> > > > off clean-up task
> > > > with enabled enumeration and that default value is 10800 in 
> > > > that case.
> > > 
> > > OK, see the attached patch.
> > 
> > > From 049fe229e1e6ae1550cf26fe1ccd289340f10118 Mon Sep 17 00:00:00 
> > > 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 28 Apr 2015 13:16:51 +0200
> > > Subject: [PATCH] LDAP: disable the cleanup task by default
> > > 
> > > Resolves:
> > >https://fedorahosted.org/sssd/ticket/2627
> > > 
> > > The cleanup task was designed to keep the cache size within 
> > > certain
> > > limits. This is how it roughly works now:
> > >- find users who have never logged in by default. If
> > >  account_cache_expiration is set, find users who loggged in 
> > > later
> > >  than account_cache_expiration
> > >- delete the matching set of users
> > >- find groups that have no members
> > >- delete the matching set of groups
> > > 
> > > So unless account_cache_expiration is set to something sensible, 
> > > only empty
> > > groups and expired users who never logged in are removed and 
> > > that's quite
> > > a corner case. The above effectivelly walks the whole database, 
> > > especially
> > > the groups step is quite slow with a huge database. The whole 
> > > cleanup task
> > > also runs in a single sysdb transaction, which means all other 
> > > transactions
> > > are blocked while the cleanup task crunches the database.
> > > ---
> > > src/man/sssd-ldap.5.xml   |  9 +++--
> > > src/providers/ad/ad_opts.h|  2 +-
> > > src/providers/ipa/ipa_opts.h  |  2 +-
> > > src/providers/ldap/ldap_id_enum.c | 19 +++
> > > src/providers/ldap/ldap_opts.h|  2 +-
> > > 5 files changed, 29 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
> > > index 
> > > 83ec9b668fc129859646c01a0b690cabece0df32..9756a554701462a094c538bd00cf74b1b622c280
> > >  
> > > 100644
> > > --- a/src/man/sssd-ldap.5.xml
> > > +++ b/src/man/sssd-ldap.5.xml
> > > @@ -719,10 +719,15 @@
> > > 
> > > 
> > > Setting this option to zero will 
> > > disable the
> > > -cache cleanup operation.
> > > +cache cleanup operation. Please note 
> > > that if
> > > +enumeration is enabled, the cleanup 
> > > task is
> > > +required in order to detect entries 
> > > removed from
> > > +the server and can't be disabled. By 
> > > default,
> > > +the cleanup task will run every 3 
> > > hours with
> > > +enumeration enabled.
> > > 
> > > 
> > > -Default: 10800 (3 hours)
> > > +Default: 0 (disabled)
> > > 
> > > 
> > > 
> > Code wise ACK.
> > http://sssd-ci.duckdns.org/logs/job/15/08/summary.html
> > 
> > It would be good to have a blessing for man page from native 
> > speaker.
> 
> Stephen, do you have a minute to check if the manpage hunk sounds 
> good?

Looks fine to me. Ack to the manpages.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCHES] Support GPOs referred from other domains

[Stephen Gallagher]

Patch 0001: LDAP: Support returning referral information

Some callers may be interested in the raw referral values returned from
a lookup. This patch allows interested consumers to get these referrals
back and process them if they wish. It does not implement a generic
automatic following of referrals.



Patch 0002: AD GPO: Support processing referrals

For GPOs assigned to a site, it's possible that their definition
actually exists in another domain. To retrieve this information,
we need to follow the referral and perform a base search on
another domain controller.

Resolves:
https://fedorahosted.org/sssd/ticket/2645From 3f8826061d34639ddaaf245947085ea577e77fbe Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Fri, 1 May 2015 11:42:06 -0400
Subject: [PATCH 1/2] LDAP: Support returning referral information

Some callers may be interested in the raw referral values returned from
a lookup. This patch allows interested consumers to get these referrals
back and process them if they wish. It does not implement a generic
automatic following of referrals.
---
 src/providers/ldap/sdap_async.c | 106 ++--
 1 file changed, 92 insertions(+), 14 deletions(-)

diff --git a/src/providers/ldap/sdap_async.c b/src/providers/ldap/sdap_async.c
index 0d1f0195c0b96d44151487642d30b631063d617a..7ad24902911a5baf6686da8cfcb2a0c151085c29 100644
--- a/src/providers/ldap/sdap_async.c
+++ b/src/providers/ldap/sdap_async.c
@@ -1154,10 +1154,13 @@ struct sdap_get_generic_ext_state {
 
 LDAPControl **serverctrls;
 int nserverctrls;
 LDAPControl **clientctrls;
 
+size_t ref_count;
+char **refs;
+
 sdap_parse_cb parse_cb;
 void *cb_data;
 
 bool allow_paging;
 };
@@ -1375,19 +1378,59 @@ static errno_t sdap_get_generic_ext_step(struct tevent_req *req)
 
 done:
 return ret;
 }
 
+static errno_t
+sdap_get_generic_ext_add_references(struct sdap_get_generic_ext_state *state,
+char **refs)
+{
+int i;
+
+if (refs == NULL) {
+/* Rare, but it's possible that we might get a reference result with
+ * no references attached.
+ */
+return EOK;
+}
+
+for (i = 0; refs[i]; i++) {
+DEBUG(SSSDBG_TRACE_LIBS, "Additional References: %s\n", refs[i]);
+}
+
+/* Extend the size of the ref array */
+state->refs = talloc_realloc(state, state->refs, char *,
+ state->ref_count + i);
+if (state->refs == NULL) {
+DEBUG(SSSDBG_CRIT_FAILURE,
+  "talloc_realloc failed extending ref_array.\n");
+return ENOMEM;
+}
+
+/* Copy in all the references */
+for (i = 0; refs[i]; i++) {
+state->refs[state->ref_count + i] =
+talloc_strdup(state->refs, refs[i]);
+
+if (state->refs[state->ref_count + i] == NULL) {
+return ENOMEM;
+}
+}
+
+state->ref_count += i;
+
+return EOK;
+}
+
 static void sdap_get_generic_op_finished(struct sdap_op *op,
  struct sdap_msg *reply,
  int error, void *pvt)
 {
 struct tevent_req *req = talloc_get_type(pvt, struct tevent_req);
 struct sdap_get_generic_ext_state *state = tevent_req_data(req,
 struct sdap_get_generic_ext_state);
 char *errmsg = NULL;
-int i;
 char **refs = NULL;
 int result;
 int ret;
 int lret;
 ber_int_t total_count;
@@ -1400,12 +1443,31 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 return;
 }
 
 switch (ldap_msgtype(reply->msg)) {
 case LDAP_RES_SEARCH_REFERENCE:
-/* ignore references for now */
-talloc_free(reply);
+ret = ldap_parse_reference(state->sh->ldap, reply->msg,
+   &refs, NULL, 0);
+if (ret != LDAP_SUCCESS) {
+DEBUG(SSSDBG_OP_FAILURE,
+  "ldap_parse_reference failed (%d)\n", state->op->msgid);
+tevent_req_error(req, EIO);
+return;
+}
+
+ret = sdap_get_generic_ext_add_references(state, refs);
+if (ret != EOK) {
+DEBUG(SSSDBG_OP_FAILURE,
+  "sdap_get_generic_ext_add_references failed: %s(%d)",
+  sss_strerror(ret), ret);
+ldap_memvfree((void **)refs);
+tevent_req_error(req, ret);
+return;
+}
+
+/* Remove the original strings */
+ldap_memvfree((void **)refs);
 
 /* unlock the operation so that we can proceed with the next result */
 sdap_unlock_next_reply(state->op);
 break;
 
@@ -1454,19 +1516,18 @@ static void sdap_get_generic_op_finished(struct sdap_op *op,
 } else if (result == LDAP_UNAVAILABLE_CRITICAL_EXTENSION) {
 ldap_memf

Re: [SSSD] [PATCH] GPO: Fix crash with GPO and missing security descriptor

[Stephen Gallagher]

On Wed, 2015-04-29 at 18:50 +0200, Lukas Slebodnik wrote:
> On (29/04/15 08:00), Stephen Gallagher wrote:
> > I'm not aware of any situation where this would be a sensible 
> > reply,
> > so this should be fine (and at worst, safe).
> > 
> > I suspect (but since Yassir isn't here any more cannot confirm) 
> > that
> > the original intent here was to skip this GPO, but that wasn't
> > correctly implemented. Good thing too, as it would have been a
> > security bug as previously noted.
> > 
> > Given that this is fairly likely to be hit, I suggest that we need 
> > to
> > open an RFE bug upstream and then change the message to refer to 
> > it. I
> > suggest the following:
> > 
> > "BUG: No attrs found for GPO [%s]. This was likely caused by the 
> > GPO
> > entry being a referred to another domain controller. SSSD does not 
> > yet
> > support this configuration. See [insert SSSD bug number] for more
> > information."
> 
> Updated patch attached.
> 
> LS

Ack. Tested with my reproduction environment and the user was denied
login and the logs showed the message as expected.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: Fix crash with GPO and missing security descriptor

[Stephen Gallagher]

On Wed, 2015-04-29 at 09:38 +0200, Lukas Slebodnik wrote:
> On (24/04/15 14:07), Jakub Hrozek wrote:
> > On Fri, Apr 24, 2015 at 02:01:11PM +0200, Lukas Slebodnik wrote:
> > > On (24/04/15 12:43), Jakub Hrozek wrote:
> > > > On Thu, Apr 23, 2015 at 07:29:07AM -0400, Stephen Gallagher 
> > > > wrote:
> > > > > On Thu, 2015-04-23 at 08:14 +0200, Lukas Slebodnik wrote:
> > > > > > On (20/04/15 14:38), Stephen Gallagher wrote:
> > > > > > > On Mon, 2015-04-20 at 08:53 +0200, Lukas Slebodnik wrote:
> > > > > > > > ehlo,
> > > > > > > > 
> > > > > > > > attached patch fixes crash in
> > > > > > > > https://fedorahosted.org/sssd/ticket/2629
> > > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > Nack. I'd rather we fixed the root of this problem. I 
> > > > > > > did some 
> > > > > > > digging
> > > > > > > this afternoon and tracked the issue back to ad_gpo.c 
> > > > > > > line 3499 (in
> > > > > > > current master). If we get back a NULL result or 
> > > > > > > num_results == 0,
> > > > > > > then we just skip over this item in the list and start 
> > > > > > > processing 
> > > > > > > the
> > > > > > > next one. Unfortunately, that leaves an item in the 
> > > > > > > candidate_gpos
> > > > > > > list that was never properly constructed.
> > > > > > > 
> > > > > > You are right.
> > > > > > 
> > > > > > We got a referral to GPO and therefore we do not find any 
> > > > > > attributes.
> > > > > > 
> > > > > > [sdap_sd_search_send] (0x0400): Searching entry 
> > > > > > [cn={2BA15B73-9524-
> > > > > > 419F-B4B7
> > > > > > -185E1F0D3DCF},cn=policies,cn=system,DC=example,DC=com] 
> > > > > > using SD
> > > > > > [sdap_print_server] (0x2000): Searching 10.1.1.14
> > > > > > [sdap_get_generic_ext_step] (0x0400): calling 
> > > > > > ldap_search_ext with 
> > > > > > [(objectclass=*)][cn={2BA15B73-9524-419F-B4B7-
> > > > > > 185E1F0D3DCF},cn=policies,cn=system,DC=example,DC=com].
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [nTSecurityDescriptor]
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [cn]
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [gPCFileSysPath]
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [gPCMachineExtensionNames]
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [gPCFunctionalityVersion]
> > > > > > [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> > > > > > [flags]
> > > > > > [sdap_get_generic_ext_step] (0x2000): ldap_search_ext 
> > > > > > called, msgid =
> > > > > > 14
> > > > > > [sdap_process_result] (0x2000): Trace: sh[0x7f5d409a8c60], 
> > > > > > connected[1], ops[0x7f5d409d2c10], ldap[0x7f5d409a9ed0]
> > > > > > [sdap_process_result] (0x2000): Trace: ldap_result found 
> > > > > > nothing!
> > > > > > [sdap_process_result] (0x2000): Trace: sh[0x7f5d409a8c60], 
> > > > > > connected[1], ops[0x7f5d409d2c10], ldap[0x7f5d409a9ed0]
> > > > > > [sdap_process_message] (0x4000): Message type: 
> > > > > > [LDAP_RES_SEARCH_RESULT] 
> > > > > > [sdap_get_generic_op_finished] (0x0400): Search result: 
> > > > > > Referral(10), 202B: RefErr: DSID-0310063C, data 0, 1 
> > > > > > access 
> > > > > > points
> > > > > > ref 1: 'lzb.hq'
> > > > > > 
> > > > > > [sdap_get_generic_op_finished] (0x1000): Ref: 
> > > > > > ldap://lzb.hq/cn=%7B2BA15B73-9524-419F-B4B7-
> > > > > > 185E1F0D3DCF%7D,cn=policies,cn=system,DC=example,DC=com
> > > > > > [ad_gpo_get_gpo_attrs_done] (0x0040): no attrs found for 

Re: [SSSD] [PATCH] GPO: Fix crash with GPO and missing security descriptor

[Stephen Gallagher]

On Thu, 2015-04-23 at 08:14 +0200, Lukas Slebodnik wrote:
> On (20/04/15 14:38), Stephen Gallagher wrote:
> > On Mon, 2015-04-20 at 08:53 +0200, Lukas Slebodnik wrote:
> > > ehlo,
> > > 
> > > attached patch fixes crash in
> > > https://fedorahosted.org/sssd/ticket/2629
> > > 
> > 
> > 
> > Nack. I'd rather we fixed the root of this problem. I did some 
> > digging
> > this afternoon and tracked the issue back to ad_gpo.c line 3499 (in
> > current master). If we get back a NULL result or num_results == 0,
> > then we just skip over this item in the list and start processing 
> > the
> > next one. Unfortunately, that leaves an item in the candidate_gpos
> > list that was never properly constructed.
> > 
> You are right.
> 
> We got a referral to GPO and therefore we do not find any attributes.
> 
> [sdap_sd_search_send] (0x0400): Searching entry [cn={2BA15B73-9524-
> 419F-B4B7-185E1F0D3DCF},cn=policies,cn=system,DC=example,DC=com] 
> using SD
> [sdap_print_server] (0x2000): Searching 10.1.1.14
> [sdap_get_generic_ext_step] (0x0400): calling ldap_search_ext with 
> [(objectclass=*)][cn={2BA15B73-9524-419F-B4B7-
> 185E1F0D3DCF},cn=policies,cn=system,DC=example,DC=com].
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> [nTSecurityDescriptor]
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [cn]
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> [gPCFileSysPath]
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> [gPCMachineExtensionNames]
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: 
> [gPCFunctionalityVersion]
> [sdap_get_generic_ext_step] (0x1000): Requesting attrs: [flags]
> [sdap_get_generic_ext_step] (0x2000): ldap_search_ext called, msgid =
> 14
> [sdap_process_result] (0x2000): Trace: sh[0x7f5d409a8c60], 
> connected[1], ops[0x7f5d409d2c10], ldap[0x7f5d409a9ed0]
> [sdap_process_result] (0x2000): Trace: ldap_result found nothing!
> [sdap_process_result] (0x2000): Trace: sh[0x7f5d409a8c60], 
> connected[1], ops[0x7f5d409d2c10], ldap[0x7f5d409a9ed0]
> [sdap_process_message] (0x4000): Message type: 
> [LDAP_RES_SEARCH_RESULT] 
> [sdap_get_generic_op_finished] (0x0400): Search result: 
> Referral(10), 202B: RefErr: DSID-0310063C, data 0, 1 access 
> points
> ref 1: 'lzb.hq'
> 
> [sdap_get_generic_op_finished] (0x1000): Ref: 
> ldap://lzb.hq/cn=%7B2BA15B73-9524-419F-B4B7-
> 185E1F0D3DCF%7D,cn=policies,cn=system,DC=example,DC=com
> [ad_gpo_get_gpo_attrs_done] (0x0040): no attrs found for GPO; try 
> next GPO.
> 
> > Under what circumstances can the secinfo_dacl search return success
> > but with zero results? Is there a bug or a race here (such as the 
> > AD
> > server has updated the GPO since we got the list of candidate 
> > GPOs?).
> > How best to handle this?
> > 
> > With your patch here, it looks like we're assuming that it's okay 
> > to
> > just skip over this GPO. If that's the case, then what we really 
> > need
> > to be doing in ad_gpo_get_gpo_attrs_step() is to mark the gp_gpo as
> > being invalid and then after we've gone through them all, shrink 
> > the
> > array, removing all of the invalid entries. This will be more 
> > future-
> > proof, as it's not just the gpo_sd that is uninitialized here. 
> > Every
> > member of this gp_gpo is NULL except for the DN.
> > 
> > If it's *not* okay that we've gotten no results for this lookup 
> > (such
> > as in the race case; we don't want to be skipping over a GPO that
> > might properly be denying users), we may need to restart 
> > processing at
> > least a couple times to try to avoid the race and go offline if we
> > can't complete the processing (so we at least stick to our cached
> > rules).
> 
> I'm sorry I didn't noticed it in log files the first time.
> Now we know the root of problem.
> Which of your proposal do you prefer now?
> 
> LS

OK, I hadn't considered the referral scenario. (I wasn't actually 
aware that GPOs could *be* referred to elsewhere). Is this referral 
pointing to a different machine than the host is enrolled to? It's 
trying to reach a DN at lzb.hq that is identical to the one we just 
requested.

I'd really like to know how to reproduce the setup for this situation; 
is it inheriting a GPO from a parent domain somehow?

Neither of my above proposals are a proper solution for this case, 
unfortunately. The first one would make the crash go away, but if this 
GPO really exists and is restrictive, not processing it is *really 
bad*. Right now we'

Re: [SSSD] [PATCH] GPO: Fix crash with GPO and missing security descriptor

[Stephen Gallagher]

On Mon, 2015-04-20 at 08:53 +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> attached patch fixes crash in 
> https://fedorahosted.org/sssd/ticket/2629
> 


Nack. I'd rather we fixed the root of this problem. I did some digging 
this afternoon and tracked the issue back to ad_gpo.c line 3499 (in 
current master). If we get back a NULL result or num_results == 0, 
then we just skip over this item in the list and start processing the 
next one. Unfortunately, that leaves an item in the candidate_gpos 
list that was never properly constructed.

Under what circumstances can the secinfo_dacl search return success 
but with zero results? Is there a bug or a race here (such as the AD 
server has updated the GPO since we got the list of candidate GPOs?). 
How best to handle this?

With your patch here, it looks like we're assuming that it's okay to 
just skip over this GPO. If that's the case, then what we really need 
to be doing in ad_gpo_get_gpo_attrs_step() is to mark the gp_gpo as 
being invalid and then after we've gone through them all, shrink the 
array, removing all of the invalid entries. This will be more future-
proof, as it's not just the gpo_sd that is uninitialized here. Every 
member of this gp_gpo is NULL except for the DN.

If it's *not* okay that we've gotten no results for this lookup (such 
as in the race case; we don't want to be skipping over a GPO that 
might properly be denying users), we may need to restart processing at 
least a couple times to try to avoid the race and go offline if we 
can't complete the processing (so we at least stick to our cached 
rules).

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] AD GPO: Change default to "enforcing"

[Stephen Gallagher]

When a user enrolls a system against Active Directory, the expectation
is that the client will honor the centrally-managed settings. In the
past, we avoided changing the default (and left it in permissive mode,
to warn admins that the security policy wasn't being honored) in order
to avoid breaking existing Active Directory enrollments.

However, sufficient time has likely passed for users to become
accustomed to using GPOs to manage access-control for their systems.

This patch changes the default to enforcing and adds a configure flag
for distributions to use if they wish to provide a different default
value.From 3ef7523f4e0e8bd6a5e182bd64790b6ab9f5c310 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Mon, 20 Apr 2015 10:51:04 -0400
Subject: [PATCH] AD GPO: Change default to "enforcing"

When a user enrolls a system against Active Directory, the expectation
is that the client will honor the centrally-managed settings. In the
past, we avoided changing the default (and left it in permissive mode,
to warn admins that the security policy wasn't being honored) in order
to avoid breaking existing Active Directory enrollments.

However, sufficient time has likely passed for users to become
accustomed to using GPOs to manage access-control for their systems.

This patch changes the default to enforcing and adds a configure flag
for distributions to use if they wish to provide a different default
value.
---
 configure.ac   |  1 +
 src/conf_macros.m4 | 22 ++
 src/man/Makefile.am|  7 ++-
 src/man/sssd-ad.5.xml  |  5 -
 src/providers/ad/ad_opts.h |  3 ++-
 5 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index e30405f3a17ffd2c9899b6eb17af85ec9bc15234..b349d0c9036e1ece46df2848f841e236a6bde92c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -121,10 +121,11 @@ WITH_PYTHON2_BINDINGS
 WITH_PYTHON3_BINDINGS
 WITH_CIFS_PLUGIN_PATH
 WITH_SELINUX
 WITH_NSCD
 WITH_SEMANAGE
+WITH_AD_GPO_DEFAULT
 WITH_GPO_CACHE_PATH
 WITH_NOLOGIN_SHELL
 WITH_APP_LIBS
 WITH_SUDO
 WITH_SUDO_LIB_PATH
diff --git a/src/conf_macros.m4 b/src/conf_macros.m4
index 9ed0a4c44c209e88fc896d0cd3040cb572b358c9..571d636718997511a5e63811130762440ba41dfc 100644
--- a/src/conf_macros.m4
+++ b/src/conf_macros.m4
@@ -784,5 +784,27 @@ AC_DEFUN([WITH_SSSD_USER],
 
 AC_SUBST(SSSD_USER)
 AC_DEFINE_UNQUOTED(SSSD_USER, "$SSSD_USER", ["The default user to run SSSD as"])
 AM_CONDITIONAL([SSSD_USER], [test x"$with_sssd_user" != x])
   ])
+
+  AC_DEFUN([WITH_AD_GPO_DEFAULT],
+[ AC_ARG_WITH([ad-gpo-default],
+  [AS_HELP_STRING([--with-ad-gpo-default=[enforcing|permissive]],
+  [Default enforcing level for AD GPO access-control (enforcing)]
+ )
+  ]
+ )
+  GPO_DEFAULT=enforcing
+
+  if test x"$with_ad_gpo_default" != x; then
+  if test ! "$with_ad_gpo_default" = "enforcing" -a ! "$with_ad_gpo_default" = "permissive"; then 
+  AC_MSG_ERROR("GPO Default must be either "enforcing" or "permissive")
+  else
+  GPO_DEFAULT=$with_ad_gpo_default
+  fi
+  fi
+
+  AC_SUBST(GPO_DEFAULT)
+  AC_DEFINE_UNQUOTED(AD_GPO_ACCESS_MODE_DEFAULT, "$GPO_DEFAULT", ["The default enforcing level for AD GPO access-control"])
+  AM_CONDITIONAL([GPO_DEFAULT_ENFORCING], [test x"$GPO_DEFAULT" = xenforcing])
+  ])
diff --git a/src/man/Makefile.am b/src/man/Makefile.am
index 6a1cf7dcea7bb033c9653452075ef92b7d52f7c1..1ef1da48cce74f7d1ad77e3751ee6ac3450f0259 100644
--- a/src/man/Makefile.am
+++ b/src/man/Makefile.am
@@ -22,11 +22,16 @@ if BUILD_PAC_RESPONDER
 PAC_RESPONDER_CONDS = ;with_pac_responder
 endif
 if BUILD_IFP
 IFP_CONDS = ;with_ifp
 endif
-CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(IFP_CONDS)
+if GPO_DEFAULT_ENFORCING
+GPO_CONDS = ;gpo_default_enforcing
+else
+GPO_CONDS = ;gpo_default_permissive
+endif
+CONDS = with_false$(SUDO_CONDS)$(AUTOFS_CONDS)$(SSH_CONDS)$(PAC_RESPONDER_CONDS)$(IFP_CONDS)$(GPO_CONDS)
 
 
 #Special Rules:
 export SGML_CATALOG_FILES
 DOCBOOK_XSLT = @DOCBOOK_XSLT@
diff --git a/src/man/sssd-ad.5.xml b/src/man/sssd-ad.5.xml
index 55c7a404527bbd279deadc08b17549c517773719..938a443e027b9bf83c75c240a7d6b2a0876b92c8 100644
--- a/src/man/sssd-ad.5.xml
+++ b/src/man/sssd-ad.5.xml
@@ -322,13 +322,16 @@ FOREST:EXAMPLE.COM:(memberOf=cn=admins,ou=groups,dc=example,dc=com)
 value were set to enforcing.
 
 
 
 
-
+  

[SSSD] [PATCHES] Fix GPO processing for users from subdomains

[Stephen Gallagher]

Patch 0001: AD: Clean up ad_access_gpo
Just a minor cleanup to ad_gpo_access_send to adhere to our tevent 
conventions. This is purely for aesthetic and maintainability reasons; 
it has no functional effect.

Patch 0002: AD: Always get domain-specific ID connection
This one is a little tricky. It turns out that in some circumstances, 
ad_ctx->ldap_ctx may actually be pointing at a subdomain rather than 
the enrolled domain. I don't know the reasons for this (and it appears 
to be a race-condition, because I could only get it to happen if I was 
quick to test logins right after restarting SSSD). However, the fix is 
fairly straightforward: sdap_domain_get()->pvt->ldap_ctx always 
provides the real ldap_ctx for the requested domain (either the 
enrolled domain or any of the trusted domains). The IS_SUBDOMAIN() 
check and shortcut to ad_ctx->ldap_ctx was unnecessary and (thanks to 
the odd race) incorrect. This patch removes this conditional shortcut 
and forces us to get the correct ldap_ctx. This proved to be the last 
piece necessary to get Patch 0003 to work.

Patch 0003: AD GPO: Always look up GPOs from machine domain

When dealing with users from a child domain, SSSD was attempting to use
the subdomain for lookups. However, all GPOs applicable to this machine
are stored in the primary domain (the domain the host directly joined).

This patch has the GPO processing use the primary domain instead of the
user domain.

Resolves:
https://fedorahosted.org/sssd/ticket/2606From 39a0dc5dd670cb251e3c9a3b35aca9dbb2ede061 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 14 Apr 2015 13:07:36 -0400
Subject: [PATCH 1/3] AD: Clean up ad_access_gpo

Align goto usage with conventions in the rest of the source.
---
 src/providers/ad/ad_gpo.c | 12 +---
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index a881741a6ead9244ac123608234d1a0c35f830e3..54e5545a57b7e697f730431ae35a95ccabbe21db 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1532,12 +1532,10 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
 DEBUG(SSSDBG_TRACE_FUNC, "service %s maps to %s\n", service,
   gpo_map_type_string(gpo_map_type));
 
 if (gpo_map_type == GPO_MAP_PERMIT) {
 ret = EOK;
-tevent_req_done(req);
-tevent_req_post(req, ev);
 goto immediately;
 }
 
 if (gpo_map_type == GPO_MAP_DENY) {
 switch (ctx->gpo_access_control_mode) {
@@ -1549,12 +1547,10 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
 sss_log_ext(SSS_LOG_WARNING, LOG_AUTHPRIV, "Warning: user would " \
 "have been denied GPO-based logon access if the " \
 "ad_gpo_access_control option were set to enforcing " \
 "mode.");
 ret = EOK;
-tevent_req_done(req);
-tevent_req_post(req, ev);
 goto immediately;
 default:
 ret = EINVAL;
 goto immediately;
 }
@@ -1590,19 +1586,21 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
ret, sss_strerror(ret));
 goto immediately;
 }
 tevent_req_set_callback(subreq, ad_gpo_connect_done, req);
 
-ret = EOK;
+return req;
 
 immediately:
 
-if (ret != EOK) {
+if (ret == EOK) {
+tevent_req_done(req);
+} else {
 tevent_req_error(req, ret);
-tevent_req_post(req, ev);
 }
 
+tevent_req_post(req, ev);
 return req;
 }
 
 static errno_t
 process_offline_gpos(TALLOC_CTX *mem_ctx,
-- 
2.3.5

From 5e57bf4e92fd898a1879dc773c7a380b1f96b7ad Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 14 Apr 2015 21:50:36 -0400
Subject: [PATCH 2/3] AD: Always get domain-specific ID connection

ad_get_dom_ldap_conn() assumed that ad_ctx->ldap_ctx always points at
the LDAP connection for the primary domain, however it turns out that
this is not always the case. It's currently unclear why, but this
connection can sometimes be pointing at a subdomain. Since the value of
subdom_id_ctx->ldap_ctx always points to the correct domain (including
the primary domain case), there's no benefit to trying to shortcut to
the ad_ctx->ldap_ctx when performing this lookup.

This patch also makes a minor tweak to the tests so that the primary
domain passes the sdap_domain_get() check for validity (since it needs
to have a private member assigned).
---
 src/providers/ad/ad_common.c  | 18 +++---
 src/tests/cmocka/test_ad_common.c |  1 +
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/providers/ad/ad_common.c b/src/providers/ad/ad_common.c
index 120878977d08aab04bbd9e3cf87a00a4b018b6e4..5eeb8dd74d1df89a1a0afa50560b8341b0088778 100644
--- a/src/providers/ad/ad_common.c
+++ b/src/providers/ad/ad_common.c
@@ -1138,22 +1138,18 @@ ad_get_dom_ldap_conn(struct ad_id_ctx

Re: [SSSD] [PATCH] MAN: Update ppolicy description

[Stephen Gallagher]

On Fri, 2015-03-27 at 11:00 +0100, Pavel Reichl wrote:
> On 03/26/2015 06:09 PM, Stephen Gallagher wrote:
> > On Thu, 2015-03-26 at 17:51 +0100, Pavel Reichl wrote:
> > > Hello,
> > > 
> > > please see this trivial patch.
> > > 
> > > I CCed Stephen in hope that he would be so kind and do the 
> > > language
> > > review.
> > > 
> > > Thanks!
> > "The value of 'pwdAccountLockedTime' attribute must end with 'Z' as
> > only UTC time zone is currently supported otherwise access is 
> > denied
> > for any other time specifications."
> > 
> > That's a bit awkward in English. May I suggest:
> > 
> > The value of the 'pwdAccountLockedTime' attribute must end with 
> > 'Z',
> > which denotes the UTC time zone. Other time zones are not currently
> > supported and will result in "access-denied" when users attempt to 
> > log
> > in.
> Great, thanks!
> 
> Updated patch is attached.

Ack

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] MAN: Update ppolicy description

[Stephen Gallagher]

On Thu, 2015-03-26 at 17:51 +0100, Pavel Reichl wrote:
> Hello,
> 
> please see this trivial patch.
> 
> I CCed Stephen in hope that he would be so kind and do the language 
> review.
> 
> Thanks!

"The value of 'pwdAccountLockedTime' attribute must end with 'Z' as 
only UTC time zone is currently supported otherwise access is denied 
for any other time specifications."

That's a bit awkward in English. May I suggest:

The value of the 'pwdAccountLockedTime' attribute must end with 'Z', 
which denotes the UTC time zone. Other time zones are not currently 
supported and will result in "access-denied" when users attempt to log 
in.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Remove useless assignment to function parameter

[Stephen Gallagher]

On Mon, 2015-03-02 at 14:43 +0100, Lukas Slebodnik wrote:
> On (02/03/15 14:39), Sumit Bose wrote:
> > On Mon, Mar 02, 2015 at 11:27:09AM +0100, Sumit Bose wrote:
> > > On Mon, Mar 02, 2015 at 10:43:36AM +0100, Jakub Hrozek wrote:
> > > > On Mon, Mar 02, 2015 at 10:41:22AM +0100, Pavel Reichl wrote:
> > > > > 
> > > > > On 03/02/2015 10:38 AM, Jakub Hrozek wrote:
> > > > > > On Mon, Mar 02, 2015 at 10:29:00AM +0100, Pavel Reichl 
> > > > > > wrote:
> > > > > > > On 03/02/2015 10:02 AM, Lukas Slebodnik wrote:
> > > > > > > > On (28/02/15 22:24), Pavel Reichl wrote:
> > > > > > > > > On 02/27/2015 05:27 PM, Lukas Slebodnik wrote:
> > > > > > > > > > ehlo,
> > > > > > > > > > 
> > > > > > > > > > I found this patch in my old branches. It still 
> > > > > > > > > > applies to sssd and remove dead
> > > > > > > > > > code.
> > > > > > > > > > 
> > > > > > > > > > LS
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > Hello Lukas,
> > > > > > > > > 
> > > > > > > > > what I don't like about your patch is the fact that 
> > > > > > > > > you are changing the intentions of the author of the 
> > > > > > > > > original code.
> > > > > > > > > 
> > > > > > > > I did not change anything. It is dead code and it's 
> > > > > > > > very likely compiler optimize it out anyway.
> > > > > > > > 
> > > > > > > > There is a BIG difference between intention of author 
> > > > > > > > and what code does :-)
> > > > > > > Yes, I'm well aware of that.
> > > > > > > > > Still there's no way how to fix function to do what 
> > > > > > > > > was intended because we
> > > > > > > > > can't change the type of the parameter. At least not 
> > > > > > > > > for sssd_krb5_locator_close() and free_exp_data().
> > > > > > > > > 
> > > > > > > > > I'm not sure whether we can modify definition of 
> > > > > > > > > hbac_free_info(). Do you
> > > > > > > > > know?
> > > > > > > > > 
> > > > > > > > We cannot change API of hbac_free_info either. It is 
> > > > > > > > part of public API $ nm --defined-only --dynamic 
> > > > > > > > /usr/lib64/libipa_hbac.so | grep free 1030 
> > > > > > > > T hbac_free_info
> > > > > > > OK, thanks.
> > > > > > > 
> > > > > > > ACK (ci passed:http://sssd-
> > > > > > > ci.duckdns.org/logs/job/8/43/summary.html)
> > > > > > Did either of you reach out to the original authors to see 
> > > > > > if they're OK with the removal? If not, can you?
> > > > > OK, I'll do it, but I'm not sure if one of the authors is 
> > > > > Yassir.
> > > > 
> > > > Stephen wrote the HBAC evaluator, Sumit created both the krb5 
> > > > plugin and pam_sss.
> > > 
> > > Hi,
> > > 
> > > since both the krb5 locator plugin and pam_sss are loaded into 
> > > other programs I wanted to make sure to clean up as much as 
> > > possible to avoid issues in the caller even if the caller 
> > > misbehaves.
> > > 
> > > Although I see the point that this might hide issues in the 
> > > caller I think it would makes sense to stay defensive here.
> > 
> > I'm sorry, Pavel pointed out to me that we only clear the local 
> > copy of the pointer and not the origin (we only get * not **). In 
> > this case the assignment is useless.
> > 
> That's exactly reason why cppcheck reported it as dead assignment.
> 


Yeah, this looks like it was excessive defense, not useful 
functionality. Go ahead and kill it.

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Use FQDN if default domain was set

[Stephen Gallagher]

On Thu, 2015-02-26 at 14:01 +0100, Jakub Hrozek wrote:
> On Thu, Feb 26, 2015 at 11:26:13AM +0100, Lukas Slebodnik wrote:
> > On (26/02/15 11:17), Jakub Hrozek wrote:
> > > On Wed, Feb 25, 2015 at 11:53:00PM +0100, Lukas Slebodnik wrote:
> > > > On (25/02/15 23:34), Jakub Hrozek wrote:
> > > > > On Wed, Feb 25, 2015 at 11:29:20PM +0100, Lukas Slebodnik 
> > > > > wrote:
> > > > > > On (25/02/15 20:57), Jakub Hrozek wrote:
> > > > > > > On Fri, Jan 30, 2015 at 04:42:13PM +0100, Michal Židek 
> > > > > > > wrote:
> > > > > > > > https://fedorahosted.org/sssd/ticket/2569
> > > > > > > > 
> > > > > > > > Simple patch is attached.
> > > > > > > > I do not think that man page update is necessary.
> > > > > > > > 
> > > > > > > > Michal
> > > > > > > 
> > > > > > > I'm sorry the review took so long. Seems to work fine:
> > > > > > > [jhrozek@client] sssd $ [(review)] getent passwd 
> > > > > > > administrator 
> > > > > > > administra...@ad.example.com:*:198600500:198600500:Administrator:/home/AD.EXAMPLE.COM/administrator:
> > > > > > > 
> > > > > > > [jhrozek@client] sssd $ [(review)] getent passwd admin
> > > > > > > [jhrozek@client] sssd $ [(review)] getent passwd 
> > > > > > > ad...@ipa.example.com 
> > > > > > > ad...@ipa.example.com:*:154660:154660:Administrator:/home/admin:/bin/bash
> > > > > > > 
> > > > > > > [jhrozek@client] sssd $ [(review)] su - 
> > > > > > > ad...@ipa.example.com
> > > > > > > Password:
> > > > > > > [ad...@ipa.example.com@client ~]$ logout
> > > > > > > [jhrozek@client] sssd $ [(review)] getent passwd admin
> > > > > > > [jhrozek@client] sssd $ [(review)] su - administrator
> > > > > > > Password:
> > > > > > > -sh-4.3$ logout
> > > > > > > 
> > > > > > > The code looks almost good to me, I will just put a 
> > > > > > > space between "if" and "(".
> > > > > > > 
> > > > > > > CI link: 
> > > > > > > http://sssd-ci.duckdns.org/logs/job/8/31/summary.html
> > > > > > I would prefer to add debug message(SSSDBG_CONF_SETTINGS) 
> > > > > > that we use different
> > > > > > default value for use_fully_qualified name.
> > > > > 
> > > > > It's just internal issue, see below.
> > > > > 
> > > > I do not see any reason why it cannot be logged.
> > > 
> > > Fine, OK.
> > > 
> > > > 
> > > > > > 
> > > > > > And it should be also documented in manual pages.
> > > > > 
> > > > > default_domain_suffix documentation already says:
> > > > > "Please note that if this option is set all users from the 
> > > > > primary domain have to use their fully qualified name, e.g. 
> > > > > u...@domain.name, to log in"
> > > > > 
> > > > > The fact that we say fqdn=true internally is just our 
> > > > > internal issue.
> > > > It's not just an internal change.
> > > > 
> > > > The result of this patch is that configuration line
> > > >  "use_fully_qualified_names = false" will be ignored if the 
> > > > option
> > > >  default_domain_suffix is configured
> > > > 
> > > > And it is not documented.
> > > 
> > > Yes, but that's an invalid configuration with 
> > > default_domain_suffix enabled. I don't feel too strongly, 
> > > though, I just wonder if documenting every corner case in man 
> > > pages can turn into noise...if you think it's important for 
> > > users to have this case documented, let's add it..
> > If you don't want to document all corner cases then we should not 
> > create hacks (ignore/override user specified options).
> > 
> > IMHO, We should just give an advice (message to syslog?) that user 
> > should
> > enable the option use_fully_qualified_names if the option 
> > default_domain_suffix
> > is configured.
> > 
> > It does not mean I'm against current patch with updating 
> > documentation.
> 
> OK, let's update the documentation, then.


I'd also recommend that if default_domain_suffix is specified, it 
should be a configuration error to set use_fully_qualified_names=False 
and that SSSD should fail to start.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] BUILD: Add possibility to build python{2, 3} bindings

[Stephen Gallagher]

On Tue, 2015-02-24 at 11:10 +0100, Lukas Slebodnik wrote:
> On (24/02/15 01:26), Lukas Slebodnik wrote:
> > On (18/02/15 16:36), Stephen Gallagher wrote:
> > > 
> > > 
> > > 
> > > On Tue, 2015-02-10 at 23:40 +0100, Lukas Slebodnik wrote:
> > > > ehlo,
> > > > 
> > > > Attached patches:
> > > > * drop support for python < 2.6
> > > > * fix packaging of binding (backward incompatible change)
> > > > * add possibility to build python{2,3} bindings
> > > > 
> > > > There are also small other enhancements.
> > > 
> > > 
> > > 
> > > Patch 0005-0013: Ack
> > > 
> > > Patch 0014:
> > > The Obsoletes: lines are wrong. These should really be < 1.12.5 
> > > (presuming that this lands in 1.12.5, otherwise it should be be <
> > > 1.13) The Obsoletes needs to be static and always indicate the 
> > > exact version at which we are replacing it. (The reasoning 
> > > behind this is complicated). I'm not sure why you picked 1.9.90 
> > > here.
> > > 
> > Nice catch.
> > It was copy and paste problem. I inspired in libsss_sudo which was 
> > obsoleted by
> > sssd-common.
> > 
> > The current version of sssd is
> > sh$ grep VERSION version.m4
> > m4_define([VERSION_NUMBER], [1.12.90])
> > 
> > So I used
> > Obsoletes: libipa_hbac-python < 1.12.90
> > 
> > I'm not sure wheter we shoudl push such changes to 1.12.5.
> > We can push some build patches but I would like to avoid doing big 
> > packaging
> > changes in stable release.
> > 

As long as it's backwards-compatible (any system using the older 
version gets the newer one), I'd recommend to make the change, if only 
to save yourself trouble maintaining the branch going forward.


> > > Patch 0015:
> > > sss_obfuscate relies on pysss.so. We need to add a Requires: 
> > > python-sss to the sssd-tools subpackage. (As noted on IRC, it 
> > > also Requires: python-sssdconfig and that Requires: is also 
> > > missing).
> > > 
> > Added
> > 
> > > Also, the sss_obfuscate tool itself is currently hard-coded
> > > to /usr/bin/python (AKA python2). According to the Fedora 
> > > packaging guidelines:
> > > "If only one executable is to be shipped, then it owns its own 
> > > slot and should use /usr/bin/python3 from Fedora 22 on."[1]
> > I added simple sed command in last patch s/python/python3/
> > 
> > > 
> > > Patch 0016:
> > > The three python tests (pyhbac-test.py, pysss_murmur-test.py and 
> > > python-test.py) are only run against python 2 during 'make 
> > > check'. Please run them against both versions. (This may require 
> > > creating a wrapper script around them to force them to be called 
> > > with python3)
> > > 
> > I added wrappers in separate patch because the patch "BUILD-Add-
> > possibility-to-build-python-2-3-bindings" is complicated enough.
> > 
> > > Patch 0017: Ack
> > > 
> > > Patch 0018:
> > > You typo python as "pyhton" in numerous places in the spec file.
> > > 
> > Fixed
> > 
> > BTW: The meta pacakge sssd requires package "python-sssdconfig". 
> > Do you remember why? Should we drop this requirements?
> > 

Yes, when we originally created the 'sssd' package, we wanted it to be 
"the complete set of SSSD code". At the time, python-sssdconfig was 
shipping as part of sssd-common and authconfig was depending on that 
package. We maintained it in the meta-package for backwards-
compatibility.

I think we can probably break that compatibility with an announcement 
in F22, but I'd maintain it in F21 and older, just in case.


> > Thank you very much for review.
> > The new version of patchset is attached.
> > 
> > LS
> > 
> > > 
> > > [1] https://fedoraproject.org/wiki/Packaging:Python#Naming
> > > 
> 
> I forgot to remove python3 bytecode files.
> Updated version is attached.
> 


Ack to all the patches. Good work!

signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] BUILD: Add possibility to build python{2, 3} bindings

[Stephen Gallagher]

On Tue, 2015-02-10 at 23:40 +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> Attached patches:
> * drop support for python < 2.6
> * fix packaging of binding (backward incompatible change)
> * add possibility to build python{2,3} bindings
> 
> There are also small other enhancements.



Patch 0005-0013: Ack

Patch 0014:
The Obsoletes: lines are wrong. These should really be < 1.12.5
(presuming that this lands in 1.12.5, otherwise it should be be < 1.13)
The Obsoletes needs to be static and always indicate the exact version
at which we are replacing it. (The reasoning behind this is
complicated). I'm not sure why you picked 1.9.90 here.

Patch 0015:
sss_obfuscate relies on pysss.so. We need to add a Requires: python-sss
to the sssd-tools subpackage. (As noted on IRC, it also Requires:
python-sssdconfig and that Requires: is also missing).

Also, the sss_obfuscate tool itself is currently hard-coded
to /usr/bin/python (AKA python2). According to the Fedora packaging
guidelines:
"If only one executable is to be shipped, then it owns its own slot and
should use /usr/bin/python3 from Fedora 22 on."[1]

Patch 0016:
The three python tests (pyhbac-test.py, pysss_murmur-test.py and
python-test.py) are only run against python 2 during 'make check'.
Please run them against both versions. (This may require creating a
wrapper script around them to force them to be called with python3)

Patch 0017: Ack

Patch 0018:
You typo python as "pyhton" in numerous places in the spec file.


[1] https://fedoraproject.org/wiki/Packaging:Python#Naming



signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] sssd-devel@lists.fedorahosted.org

[Stephen Gallagher]

On Thu, 2015-02-12 at 19:32 +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> attached is a simple patch for ticket #2572
> 
> My reproducer:
> * start sssd
> * attach gdb to some service e.g. nss
> - DO NOT RUN any command (we just need to simulate unresponsive service)
> * wait until monitor send SIGKILL to service
>"[nss][5835] is not responding to SIGTERM. Sending SIGKILL."
> * quit debugger
> 
> Result:
> the main sssd process (monitor) will crash.



WHoops! Good catch. Ack.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] LDAP: Inform about small range size

[Stephen Gallagher]

On Fri, 2015-01-23 at 17:27 +0100, Jakub Hrozek wrote:
> On Fri, Jan 23, 2015 at 05:24:51PM +0100, Michal Židek wrote:
> > On 01/23/2015 04:35 PM, Lukas Slebodnik wrote:
> > >On (23/01/15 10:21), Stephen Gallagher wrote:
> > >>
> > >>
> > >>
> > >>On Fri, 2015-01-23 at 14:39 +0100, Lukas Slebodnik wrote:
> > >>>ehlo,
> > >>>
> > >>>I was reprodicing other bug and it took me some time to find out why I 
> > >>>was not
> > >>>able to resolve user. RID was bigger than range size.
> > >>>
> > >>>I saw just general message about id mapping failer
> > >>>[sdap_save_user] (0x0400): Processing user matthewbe
> > >>>[sdap_save_user] (0x1000): Mapping user [matthewbe] objectSID
> > >>> [S-1-5-21-2997650941-1802118864-3094776726-200065] to unix ID
> > >>>[sdap_idmap_sid_to_unix] (0x0080): Could not convert objectSID
> > >>>  [S-1-5-21-2997650941-1802118864-3094776726-200065] to a UNIX ID
> > >>> ^^
> > >>> Default range size is 20
> > >>>[sdap_save_user] (0x0020): Failed to save user [matthewbe]
> > >>>[sdap_save_users] (0x0040): Failed to store user 0. Ignoring.
> > >>>
> > >>>
> > >>>Feel free to propose better debug message. I think it would simplify 
> > >>>debugging.
> > >>
> > >>
> > >>
> > >>I'd avoid making a recommendation about changing the range size. That
> > >>will result in any other domain having all of their IDs changed. That's
> > >>not a good situation. We should certainly log this at a very low level,
> > >>though.
> > >
> > >As you can see from debug message it is almost impossible to say why 
> > >converting
> > >of objectSID failed. I have already seen such problem in customer reports.
> > >and reasonable hint could speed up fixing of a problem.
> > >
> > >user issue -> bug report -> requests for log files ->
> > >  analysis of log file -> advice to increase ldap_idmap_range_size
> > >
> > >
> > >Maybe we can recommend just to double value of range size,
> > >but current situation isn't user friendly.
> > >
> > >LS
> > 
> > I think there is just problem with wording here. You
> > used "You should..." in the debug message. I would change
> > it to "You could/can..." and add a sentence that warns the
> > user about the consequences. like "But be careful because
> > changing the range size will also change the ID mappings in
> > all trusted domains." Or some better warning.
> 
> We can also point the user to the man page in the debug message to avoid
> being overly terse...then in the man page, we can explain all the pros
> and cons better.

Yeah, I like this approach. So maybe the phrasing should be:

"objectSID [%s] has a RID that is larger than the ldap_idmap_range_size.
See sssd-ad(5) for an explanation of how to resolve this issue."



signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] LDAP: Inform about small range size

[Stephen Gallagher]

On Fri, 2015-01-23 at 14:39 +0100, Lukas Slebodnik wrote:
> ehlo,
> 
> I was reprodicing other bug and it took me some time to find out why I was not
> able to resolve user. RID was bigger than range size.
> 
> I saw just general message about id mapping failer
> [sdap_save_user] (0x0400): Processing user matthewbe
> [sdap_save_user] (0x1000): Mapping user [matthewbe] objectSID
> [S-1-5-21-2997650941-1802118864-3094776726-200065] to unix ID
> [sdap_idmap_sid_to_unix] (0x0080): Could not convert objectSID
>  [S-1-5-21-2997650941-1802118864-3094776726-200065] to a UNIX ID
> ^^
> Default range size is 20
> [sdap_save_user] (0x0020): Failed to save user [matthewbe]
> [sdap_save_users] (0x0040): Failed to store user 0. Ignoring.
> 
> 
> Feel free to propose better debug message. I think it would simplify 
> debugging.



I'd avoid making a recommendation about changing the range size. That
will result in any other domain having all of their IDs changed. That's
not a good situation. We should certainly log this at a very low level,
though.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] AD: support for AD site override

[Stephen Gallagher]

On Mon, 2015-01-19 at 09:58 +0100, Jakub Hrozek wrote:
> On Mon, Jan 19, 2015 at 09:39:41AM +0100, Pavel Reichl wrote:
> > >>man page:
> > >>Specify AD site client should try to connect to.
> > >>Specify AD site to which client should try to connect.
> > >>
> > >>Which one sounds better?
> > >I don't care much, maybe the second one. Should I prepare a new patch or
> > >will you Jakub amend the patch while pushing it? Thanks.
> 
> Oops, sorry, this one slipped.
> 
> > Attached patch updates man page with Pavel's second suggestion.
> > Can I get the final ack? I would like to prepare scratch build for customer
> > ASAP.
> 
> I like the second form as well, I think it should say "the client".
> Could you ping some native speaker to help us out?
> 
> btw go ahead and built the test package, I'm sure the customer is more
> interested in functionality rather than a typo in a man page..


Slight change to the manpage:

"Specify AD site to which client should try to connect. If this option
is not provided, the AD site will be auto-discovered."


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: add systemd-user to default gpo list

[Stephen Gallagher]

On Wed, 2015-01-14 at 13:34 +0100, Pavel Reichl wrote:
> On 01/13/2015 08:39 PM, Stephen Gallagher wrote:
> >
> >
> > On Tue, 2015-01-13 at 18:58 +0100, Pavel Reichl wrote:
> >> Hello,
> >>
> >> please see simple patch attached.
> >>
> >> Thanks!
> >
> > Nack.
> >
> > First, what exactly is this service doing? I don't think we would want
> > to map it to ServiceLogonRight. That's intended for granting access to
> > the machine from a service (as opposed to a human user).
> >
> > Looking at it, this PAM stack (systemd-user) is called whenever the
> > system invokes starts a session instance of systemd. It seems to me,
> > this really belongs added to the list of default options for
> > ad_gpo_map_permit to always allow access (since this has to be allowed
> > for system functionality to work properly).
> >
> > Also, please update the manpages to match, since they specifically list
> > all of the default values.
> Thanks for comments, updated patch attached.

Ack


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: add systemd-user to default gpo list

[Stephen Gallagher]

On Tue, 2015-01-13 at 18:58 +0100, Pavel Reichl wrote:
> Hello,
> 
> please see simple patch attached.
> 
> Thanks!


Nack.

First, what exactly is this service doing? I don't think we would want
to map it to ServiceLogonRight. That's intended for granting access to
the machine from a service (as opposed to a human user).

Looking at it, this PAM stack (systemd-user) is called whenever the
system invokes starts a session instance of systemd. It seems to me,
this really belongs added to the list of default options for
ad_gpo_map_permit to always allow access (since this has to be allowed
for system functionality to work properly).

Also, please update the manpages to match, since they specifically list
all of the default values.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] monitor: Service restart fixes

[Stephen Gallagher]

On Wed, 2014-12-10 at 14:59 -0500, Stephen Gallagher wrote:
> There are actually two bugs here:
> 
> 1) When either the kill(SIGTERM) or kill(SIGKILL) commands returned
> failure (for any reason), we would talloc_free(svc) which removed it
> from being eligible for restart, resulting in the service never
> starting again without an SSSD service restart.
> 
> 2) There is a fairly wide race condition where it's possible for a
> SIGKILL timer to "catch up" to the child exit handler between us
> noticing the termination and actually restarting it. The race
> happens because we re-enter the mainloop and add a restart
> timeout to avoid a quick failure if we keep restarting due to a
> transitory issue (the mt_svc object, and therefore the SIGKILL
> timer, were never freed until we got to the actual service
> restart).
> 
> We can minimize this race by recording  the timer_event for the
> SIGKILL timeout in the mt_svc object. This way, if the process
> exits via SIGTERM, we will immediately remove the timer for the
> SIGKILL.
> 
> This patch also removes the incorrect talloc_free(svc) calls on the
> kill() failures and replaces them with an attempt to just start up
> the service again and hope for the best.
> 
> Resolves:
> https://fedorahosted.org/sssd/ticket/2525


Just after sending this, I noticed another enhancement I could make to
basically eliminate the potential race condition. Updated patch
attached.
From cc21a1fe1dcb39e0cf5287a83a26b5880f1a701d Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 10 Dec 2014 14:16:49 -0500
Subject: [PATCH] monitor: Service restart fixes

There are actually two bugs here:

1) When either the kill(SIGTERM) or kill(SIGKILL) commands returned
failure (for any reason), we would talloc_free(svc) which removed it
from being eligible for restart, resulting in the service never
starting again without an SSSD service restart.

2) There is a fairly wide race condition where it's possible for a
SIGKILL timer to "catch up" to the child exit handler between us
noticing the termination and actually restarting it. The race
happens because we re-enter the mainloop and add a restart
timeout to avoid a quick failure if we keep restarting due to a
transitory issue (the mt_svc object, and therefore the SIGKILL
timer, were never freed until we got to the actual service
restart).

We can minimize this race by recording  the timer_event for the
SIGKILL timeout in the mt_svc object. This way, if the process
exits via SIGTERM, we will immediately remove the timer for the
SIGKILL. Additionally, we'll catch the special-case of an ESRCH
response from the kill(SIGKILL) and assume that it means that the
process has exited. The only other two possible errors are
 * EINVAL: (an invalid signal was specified) - This should be
   impossible, obviously.
 * EPERM: This process doesn't have permission to send signals to
  this PID. If this happens, it's either an SELinux bug or
  else the process has terminated and a new process that
  SSSD doesn't control has taken the ID over.

So in the incredibly unlikely case that one of those occurs, we'll
just go ahead and try to start a new process.

This patch also removes the incorrect talloc_free(svc) calls on the
kill() failures and replaces them with an attempt to just start up
the service again and hope for the best.

Resolves:
https://fedorahosted.org/sssd/ticket/2525
---
 src/monitor/monitor.c | 94 ---
 1 file changed, 74 insertions(+), 20 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index c6834a1159b393bedd7251d9c3d28d3326cdc547..9a6f58d67e7d5a05bc01803bf86bb959f6ede70e 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -117,10 +117,12 @@ struct mt_svc {
 pid_t pid;
 
 int ping_time;
 int kill_time;
 
+struct tevent_timer *kill_timer;
+
 bool svc_started;
 
 int restarts;
 time_t last_restart;
 int failed_pongs;
@@ -586,49 +588,50 @@ static void set_tasks_checker(struct mt_svc *svc)
 /* FIXME: shutdown ? */
 }
 svc->ping_ev = te;
 }
 
+static void monitor_restart_service(struct mt_svc *svc);
 static void mt_svc_sigkill(struct tevent_context *ev,
struct tevent_timer *te,
struct timeval t, void *ptr);
 static int monitor_kill_service (struct mt_svc *svc)
 {
 int ret;
 struct timeval tv;
-struct tevent_timer *te;
 
 ret = kill(svc->pid, SIGTERM);
 if (ret == -1) {
 ret = errno;
 DEBUG(SSSDBG_FATAL_FAILURE,
   "Sending signal to child (%s:%d) failed: [%d]: %s! "
"Ignore and pretend child is dead.\n",
svc->name, svc->pid, ret, strerror(ret));
-goto done;
+

[SSSD] [PATCH] monitor: Service restart fixes

[Stephen Gallagher]

There are actually two bugs here:

1) When either the kill(SIGTERM) or kill(SIGKILL) commands returned
failure (for any reason), we would talloc_free(svc) which removed it
from being eligible for restart, resulting in the service never
starting again without an SSSD service restart.

2) There is a fairly wide race condition where it's possible for a
SIGKILL timer to "catch up" to the child exit handler between us
noticing the termination and actually restarting it. The race
happens because we re-enter the mainloop and add a restart
timeout to avoid a quick failure if we keep restarting due to a
transitory issue (the mt_svc object, and therefore the SIGKILL
timer, were never freed until we got to the actual service
restart).

We can minimize this race by recording  the timer_event for the
SIGKILL timeout in the mt_svc object. This way, if the process
exits via SIGTERM, we will immediately remove the timer for the
SIGKILL.

This patch also removes the incorrect talloc_free(svc) calls on the
kill() failures and replaces them with an attempt to just start up
the service again and hope for the best.

Resolves:
https://fedorahosted.org/sssd/ticket/2525
From 02417814befc89609e2ca6490a4791de5032dc99 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 10 Dec 2014 14:16:49 -0500
Subject: [PATCH] monitor: Service restart fixes

There are actually two bugs here:

1) When either the kill(SIGTERM) or kill(SIGKILL) commands returned
failure (for any reason), we would talloc_free(svc) which removed it
from being eligible for restart, resulting in the service never
starting again without an SSSD service restart.

2) There is a fairly wide race condition where it's possible for a
SIGKILL timer to "catch up" to the child exit handler between us
noticing the termination and actually restarting it. The race
happens because we re-enter the mainloop and add a restart
timeout to avoid a quick failure if we keep restarting due to a
transitory issue (the mt_svc object, and therefore the SIGKILL
timer, were never freed until we got to the actual service
restart).

We can minimize this race by recording  the timer_event for the
SIGKILL timeout in the mt_svc object. This way, if the process
exits via SIGTERM, we will immediately remove the timer for the
SIGKILL.

This patch also removes the incorrect talloc_free(svc) calls on the
kill() failures and replaces them with an attempt to just start up
the service again and hope for the best.

Resolves:
https://fedorahosted.org/sssd/ticket/2525
---
 src/monitor/monitor.c | 80 ++-
 1 file changed, 60 insertions(+), 20 deletions(-)

diff --git a/src/monitor/monitor.c b/src/monitor/monitor.c
index c6834a1159b393bedd7251d9c3d28d3326cdc547..73d7554358f446e0eb2f0ea9338abc67a723388c 100644
--- a/src/monitor/monitor.c
+++ b/src/monitor/monitor.c
@@ -117,10 +117,12 @@ struct mt_svc {
 pid_t pid;
 
 int ping_time;
 int kill_time;
 
+struct tevent_timer *kill_timer;
+
 bool svc_started;
 
 int restarts;
 time_t last_restart;
 int failed_pongs;
@@ -586,49 +588,50 @@ static void set_tasks_checker(struct mt_svc *svc)
 /* FIXME: shutdown ? */
 }
 svc->ping_ev = te;
 }
 
+static void monitor_restart_service(struct mt_svc *svc);
 static void mt_svc_sigkill(struct tevent_context *ev,
struct tevent_timer *te,
struct timeval t, void *ptr);
 static int monitor_kill_service (struct mt_svc *svc)
 {
 int ret;
 struct timeval tv;
-struct tevent_timer *te;
 
 ret = kill(svc->pid, SIGTERM);
 if (ret == -1) {
 ret = errno;
 DEBUG(SSSDBG_FATAL_FAILURE,
   "Sending signal to child (%s:%d) failed: [%d]: %s! "
"Ignore and pretend child is dead.\n",
svc->name, svc->pid, ret, strerror(ret));
-goto done;
+/* The only thing we can try here is to launch a new process
+ * and hope that it works.
+ */
+monitor_restart_service(svc);
+return EOK;
 }
 
 /* Set up a timer to send SIGKILL if this process
  * doesn't exit within sixty seconds
  */
 tv = tevent_timeval_current_ofs(svc->kill_time, 0);
-te = tevent_add_timer(svc->mt_ctx->ev, svc, tv, mt_svc_sigkill, svc);
-if (te == NULL) {
+svc->kill_timer = tevent_add_timer(svc->mt_ctx->ev,
+   svc,
+   tv,
+   mt_svc_sigkill,
+   svc);
+if (svc->kill_timer == NULL) {
 /* Nothing much we can do */
-ret = ENOMEM;
 DEBUG(SSSDBG_CRIT_FAILURE,
   "Failed to allocate timed event: mt_svc_sigkill.\n");
-goto done;
+/* We'll just have to hope that the SIGTERM succeeds */
   

Re: [SSSD] sssd.conf ownership

[Stephen Gallagher]

On Sat, 2014-11-22 at 14:24 +0100, Jakub Hrozek wrote:
> On Fri, Nov 21, 2014 at 04:26:58PM -0500, Stephen Gallagher wrote:
> > 
> > 
> > 
> > On Fri, 2014-11-21 at 20:03 +0100, Jakub Hrozek wrote:
> > > Hi,
> > > 
> > > I was going through our design page that describes the rootless sssd and
> > > I'd like to discuss the default ownership of sssd.conf a bit more.
> > > 
> > > In the design document we proposed to change the default ownership to
> > > sssd.sssd. This wouldn't widen sssd.conf access as only root and the sssd
> > > user could read the config. One reason for the change was the dbus helper
> > > to change the config, which would otherwise run privileged.
> > > 
> > > But I wonder whether it's really the best approach. If we changed the
> > > ownership to sssd.sssd, then we'd have to be careful about chowning the
> > > file each time on startup because tools like authconfig or even customer's
> > > puppet modules or whatnot will keep writing out the file as root.root. We
> > > can't reasonably change all the external tools and chowning on each
> > > startup seems a bit fragile (as opposed to chowning the databases which
> > > is a one-time operation).
> > > 
> > > Also, if we kept the the config file owned by root, then making the
> > > confdb read-only for worker processes would mean the worker processes
> > > have no means of altering the config file even if they were taken over.
> > > 
> > > The downside of keeping the sssd.conf as root is that the augeas calls
> > > would have to be moved to a setuid process -- and a new setuid process is
> > > always a bit of a burden.
> > > 
> > > To sum up:
> > > * sssd.sssd
> > > (+) don't need any privileged code to access sssd.conf
> > > (-) need to chown the file on each startup
> > > * root.root
> > > (+) config file is only writable by privileged processes
> > > (+) we're able to make the confdb read-only
> > > (-) need privileged code to perform sssd.conf changes
> > > 
> > > I do realize that the complexity of creating the setuid helpers is much
> > > larger than the complexity of writing a function to chown a file, but
> > > for some reason the chowning feels a bit fragile to me and my gut
> > > feeling is that the config file would better be only accessible by
> > > root...
> > > 
> > > What do the other developers think?
> > 
> > Why exactly would it need to be chown()ed? I'm unclear on that point.
> > Presumably, SSSD is still being launched as root and then dropping
> > privileges (otherwise it wouldn't be able to perform the chown). So to
> > me it seems like it makes sense to just open the file read-only prior to
> > dropping privileges, then pass the filehandle along.
> 
> Passing a fd would be best, but infortunately the augeas API we use to
> change the configuration only works with path names ...

Hmm, that seems problematic and racy. There's no way for augeas to take
a descriptor? That seems like a pretty big oversight.

> 
> > The sssd can then
> > generate the read-only confdb as the 'sssd' user.
> 
> Normally yes, but if the user goes from root to non-root, we need to
> chown the confdb anyway (we could also unlink as root and then generate
> after we drop privileges..)
> 

Right, unlink and generate as non-root was what I meant there.

> > 
> > So I guess I'm completely missing the need or advantage of chowning it.
> 
> ___
> sssd-devel mailing list
> sssd-devel@lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] sssd.conf ownership

[Stephen Gallagher]

On Fri, 2014-11-21 at 20:03 +0100, Jakub Hrozek wrote:
> Hi,
> 
> I was going through our design page that describes the rootless sssd and
> I'd like to discuss the default ownership of sssd.conf a bit more.
> 
> In the design document we proposed to change the default ownership to
> sssd.sssd. This wouldn't widen sssd.conf access as only root and the sssd
> user could read the config. One reason for the change was the dbus helper
> to change the config, which would otherwise run privileged.
> 
> But I wonder whether it's really the best approach. If we changed the
> ownership to sssd.sssd, then we'd have to be careful about chowning the
> file each time on startup because tools like authconfig or even customer's
> puppet modules or whatnot will keep writing out the file as root.root. We
> can't reasonably change all the external tools and chowning on each
> startup seems a bit fragile (as opposed to chowning the databases which
> is a one-time operation).
> 
> Also, if we kept the the config file owned by root, then making the
> confdb read-only for worker processes would mean the worker processes
> have no means of altering the config file even if they were taken over.
> 
> The downside of keeping the sssd.conf as root is that the augeas calls
> would have to be moved to a setuid process -- and a new setuid process is
> always a bit of a burden.
> 
> To sum up:
> * sssd.sssd
> (+) don't need any privileged code to access sssd.conf
> (-) need to chown the file on each startup
> * root.root
> (+) config file is only writable by privileged processes
> (+) we're able to make the confdb read-only
> (-) need privileged code to perform sssd.conf changes
> 
> I do realize that the complexity of creating the setuid helpers is much
> larger than the complexity of writing a function to chown a file, but
> for some reason the chowning feels a bit fragile to me and my gut
> feeling is that the config file would better be only accessible by
> root...
> 
> What do the other developers think?

Why exactly would it need to be chown()ed? I'm unclear on that point.
Presumably, SSSD is still being launched as root and then dropping
privileges (otherwise it wouldn't be able to perform the chown). So to
me it seems like it makes sense to just open the file read-only prior to
dropping privileges, then pass the filehandle along. The sssd can then
generate the read-only confdb as the 'sssd' user.

So I guess I'm completely missing the need or advantage of chowning it.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] sss_client: Return a different error when sssd is not running

[Stephen Gallagher]

On Thu, 2014-11-13 at 13:39 +0100, Lukas Slebodnik wrote:
> On (13/11/14 12:22), Jakub Hrozek wrote:
> >On Thu, Nov 13, 2014 at 11:17:15AM +0100, Lukas Slebodnik wrote:
> >> On (13/11/14 10:44), Jakub Hrozek wrote:
> >> >On Wed, Nov 12, 2014 at 08:04:46PM -0500, Simo Sorce wrote:
> >> >> On Wed, 12 Nov 2014 16:36:00 +0100
> >> >> Lukas Slebodnik  wrote:
> >> >> 
> >> >> > On (12/11/14 10:00), Simo Sorce wrote:
> >> >> > >I would create a helper function to be called on return that
> >> >> > >transforms the error accordingly. This will allow to write the code
> >> >> > >_and_ the comment once.
> >> >> > >
> >> >> > In this case, Stephan's patch is better
> >> >> > https://bugzilla.redhat.com/attachment.cgi?id=788567
> >> >> 
> >> >> Yes, this is a valid alternative.
> >> >> 
> >> >> > >The comment should be changed to something like this in either case:
> >> >> > >/* When sssd is stopped return a safe error code as if sss was not
> >> >> > >configured at all in nsswitch. This prevents bogus errors from
> >> >> > >causing issues in applications, before sssd starts or if it fails to
> >> >> > >respond. */
> >> >> > >
> >> >> > >No need to mention that sss is by default in nsswitch, as it is not
> >> >> > >in all distributions and it really is inconsequential, the same
> >> >> > >behaviour change hleps when sss is not the default but is has been
> >> >> > >manually added and sssd is stopped or not started yet (for example
> >> >> > >during boot).
> >> >> > nss-pam-ldapd has the same behaviour in the same situation.
> >> >> > Will we patch it as well? It's very likely we won't.
> >> >> 
> >> >> Sorry, I do not see how that matters :)
> >> >> 
> >> >> > The biggest problem is that sss is by default in nsswitch on
> >> >> > fedora/rhel>=7 due to glibc caching and problem with GNOME,
> >> >> > a) sssd-client is installed by default on this platforms.
> >> >> > b) sssd need't be configured by default and in most cases won't be
> >> >> > => sssd cannot run
> >> >> > c) glibc developers don't want to adjust the error return code in
> >> >> > glibc
> >> >> > 
> >> >> > As a result of this, we need to patch sssd.
> >> >> > I would say we should patch sssd just in downstream and
> >> >> > Stephan's patch works well. I tested it.
> >> >> 
> >> >> Ok, then let's go with Stephen's patch.
> >> >> 
> >> >> Simo.
> >> >> 
> >> >
> >> >I don't personally mind whether we use Lukas' or Stephen's patch but I
> >> >would /strongly/ prefer the patch to be pushed upstream.
> >> >
> >> I don't understant why we should push patch to upstream.
> >> glibc maintainer confirmed that sssd behaves corectly:
> >> "In all honesty, SSSD is *behaving* correctly. It returns:
> >> status==NSS_STATUS_UNAVAIL, and errno==ENOENT, to indicate "A necessary 
> >> input
> >> file cannot be found."[1] I can only assume this is because it is true, the
> >> daemon is not running and the return status as indicated implies that. 
> >> Such an
> >> error will be propagated up to the client, getgrent in this case, as a NULL
> >> return with errno set to ENOENT."
> >> (nss-pam-ldapd do the same with stopped nslcd)
> >> 
> >> The problem is that behaviour is unacceptable with having sss in the
> >> nsswitch.conf by default (rhel>=7, fedora). I checked glibc upstream and
> >> other distributions (debian, ubuntu, opensuse) and they don't have sss
> >> in /etc/nsswitch.conf by default.
> >> 
> >> The patch in upstream can hide problems with stopped sssd. Some user can 
> >> rely
> >> on such behaviuour.
> >> 
> >> Why we should patch sssd in upstream due to some downstream patch in 
> >> different
> >> project?
> >
> >Because I think having sss in nsswitch.conf by default is the right
> >thing to do moving forward, especially when we start managing local
> >users. Handling this situation gracefully makes sense.
> >
> That's good point. On the other hand, we are no there yet.
> It's just future plan.
> 
> >Let me turn the question around -- do you see a downside of pushing the
> >patch downstream?
> No. It will fix problem in downstream for users who do not use sssd.
> 
> >Or do you consider it a hack, patching about glibc
> >bug? Then maybe we should raise a libc bug to skip over unavailable
> >sources by default..
> If I read bugzilla ticket(s) correctly they will not do that.
> 
> I'm not sure wheter we should push patch to upstream as well.


The core of the upstream problem is a classic one: when you have
multiple operations going on and one of them "fails", how do you report
it?

In the case of glibc, all errors are suppressed except for the last one
on the list. So if SSSD wasn't the final entry on the passwd: (or other)
line, then its NSS_STATUS_UNAVAIL error would just be ignored and
replaced by the return code of the later plugin.

This (to me) seems like a fundamental bug in the way glibc handles
nsswitch. Unfortunately, we're dealing with a situation where we can't
fix the return values because of standards-compliance.

The hack I submitted was wh

Re: [SSSD] [PATCH] BUILD: Use $(MKDIR_P) in Makefile.am

[Stephen Gallagher]

On Fri, 2014-10-03 at 16:46 +0200, Jakub Hrozek wrote:
> Hi,
> 
> While I was talking to the Fedora automake maintainer about an 
> enhancement related to test environment, he suggested to make the change
> in the attached patch.


How far back does that macro go? Does it cover all platforms that SSSD
presently supports? (I'm thinking primarily of RHEL 6 here)


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: Use argument ndg_flags instead of constant

[Stephen Gallagher]

On Thu, 2014-10-02 at 19:27 +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> Some internal gpo functions [1] were called just once and with constant
> NDR_SCALARS as 2nd argument(ndr_flags), but 2nd argument was not used
> in these functions[1]. They used constant NDR_SCALARS.
> 
> [1] ndr_pull_security_ace_flags, ndr_pull_security_ace_type,
> ndr_pull_security_ace_object_flags, ndr_pull_security_acl_revision,
> ndr_pull_security_descriptor_revision,
> ndr_pull_security_descriptor_type
> 
> Patch is attached.


Ack


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] GPO: remove unused talloc contexts

[Stephen Gallagher]

On Thu, 2014-10-02 at 19:29 +0200, Lukas Slebodnik wrote:
> ehlo,
> 
> Talloc context was not used in functions ad_gpo_parse_gpo_child_response
> ad_gpo_process_cse_recv, ad_gpo_store_policy_settings.
> 
> Patch is attached.



Ack


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] AD: conflicting gpo policy settings not being resolved correctly

[Stephen Gallagher]

On Thu, 2014-10-02 at 11:45 +0200, Jakub Hrozek wrote:
> On Wed, Oct 01, 2014 at 10:50:26PM -0400, Stephen Gallagher wrote:
> > Sorry it took me so long to finish this review. The code is mostly
> > right, but I found three issues that needed to be addressed before we
> > could commit it.
> > 
> > 1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
> > was resulting in the PAM conversation ultimately returning
> > PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> > in the 0001 patch attached. Can be applied separately.
> 
> ACK
> 
> > 
> > 2) There was an 'attrs' variable being passed in the
> > sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> > causing a crash when accessing LDB.
> > 
> > 3) The code could not properly handle when a GPO had an explicitly empty
> > value (such as one might use to expressly disable a setting from a
> > lower-priority GPO).
> > 
> > Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> > of the changes I made, see
> > https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> > my review there as I went through, so I wouldn't miss any corrections
> > and as a sort of proof-of-concept)
> 
> Thank you, the diff looks good to me.
> 
> > 
> > 
> > I (believe) I have done a very thorough review of this code, so I'd ask
> > that someone double-check my updates and then we should be good to go
> > with committing this. It's a pretty important set of fixes.
> 
> I will push the second patch if you agree to squash in two additional fixes
> in the attached diff. One removes an unused function (I have -Werror set
> for these types of warnings), the other fixes a potential uninitialized
> pointer access.


Thanks, good catches. I squashed your patch into the attached ones.
From 84b624e7e1af863d6f32f609a62aa8e7bc231aee Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 1 Oct 2014 20:42:31 -0400
Subject: [PATCH 1/2] AD GPO: Fix incorrect return of EACCES

In the access providers, we expect to receive ERR_ACCESS_DENIED when
access is denied, but we were returning EACCES here. The effect was the
same, except that it presented ultimately as a system error instead of
a proper denial.
---
 src/providers/ad/ad_gpo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 4e31a4832e8dd7ae4dc2c924206ae61b96a799f3..7cb9619ca7d4fa2c8fba84a98a976c35a5e8093e 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1123,7 +1123,7 @@ ad_gpo_access_check(TALLOC_CTX *mem_ctx,
 } else {
 switch (gpo_mode) {
 case GPO_ACCESS_CONTROL_ENFORCING:
-return EACCES;
+return ERR_ACCESS_DENIED;
 case GPO_ACCESS_CONTROL_PERMISSIVE:
 DEBUG(SSSDBG_TRACE_FUNC, "access denied: permissive mode\n");
 sss_log_ext(SSS_LOG_WARNING, LOG_AUTHPRIV, "Warning: user would " \
@@ -1271,7 +1271,7 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
 if (gpo_map_type == GPO_MAP_DENY) {
 switch (ctx->gpo_access_control_mode) {
 case GPO_ACCESS_CONTROL_ENFORCING:
-ret = EACCES;
+ret = ERR_ACCESS_DENIED;
 goto immediately;
 case GPO_ACCESS_CONTROL_PERMISSIVE:
 DEBUG(SSSDBG_TRACE_FUNC, "access denied: permissive mode\n");
-- 
2.1.0

From 825029e267a213204e914e0c13b402b1e560dd2c Mon Sep 17 00:00:00 2001
From: Yassir Elley 
Date: Tue, 9 Sep 2014 15:37:05 -0400
Subject: [PATCH 2/2] AD-GPO resolve conflicting policy settings correctly

---
 src/db/sysdb.h |  29 +-
 src/db/sysdb_gpo.c | 357 +---
 src/providers/ad/ad_gpo.c  | 913 +++--
 src/tests/cmocka/test_ad_gpo.c |   1 -
 4 files changed, 744 insertions(+), 556 deletions(-)

diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 901b6129b5d84a829637ea0d3e0b05cbe9ac48d9..81b39252c6604fd7c2b604b0b4bc7f00e035252b 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -876,6 +876,8 @@ errno_t sysdb_search_object_by_sid(TALLOC_CTX *mem_ctx,
 
 #define SYSDB_GPO_CONTAINER "cn=gpos,cn=ad,cn=custom"
 
+/* === Functions related to GPO entries === */
+
 #define SYSDB_GPO_OC "gpo"
 #define SYSDB_GPO_FILTER "(objectClass="SYSDB_GPO_OC")"
 #define SYSDB_GPO_GUID_FILTER "(&(objectClass="SYSDB_GPO_OC")("SYSDB_GPO_GUID_ATTR"=%s))"
@@ -908,9 +910,28 @@ errno_t sysdb_gpo_get_gpos(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
struct ldb_result **_result);
 
-errno_t sysdb_gpo_delete_stale_gpos(TALLOC_

Re: [SSSD] [PATCH] AD: conflicting gpo policy settings not being resolved correctly

[Stephen Gallagher]

On Wed, 2014-10-01 at 22:50 -0400, Stephen Gallagher wrote:
> 
> 
> On Thu, 2014-09-11 at 23:51 -0400, Yassir Elley wrote:
> > 
> > - Original Message -
> > > 
> > > 
> > > - Original Message -
> > > > Hi,
> > > > 
> > > > The attached patch fixes ticket #2437 (conflicting gpo policy settings 
> > > > not
> > > > being resolved correctly)
> > > > 
> > > > https://fedorahosted.org/sssd/ticket/2437
> > > > 
> > > > Regards,
> > > > Yassir.
> > > > 
> > > > ___
> > > > sssd-devel mailing list
> > > > sssd-devel@lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > 
> > > 
> > > I discovered some more issues while doing further testing on this ticket
> > > relating to "defined but empty" policy settings (see additional comments 
> > > on
> > > ticket #2347). I have also made some minor changes to this patch, b/c they
> > > didn't warrant their own ticket. Namely, I have removed some attributes 
> > > that
> > > we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we
> > > handle machine_ext_names by allowing for the possibility of a "present but
> > > empty" value for AD_AT_MACHINE_EXT_NAMES.
> > > 
> > > I am attaching a revised path to address these issues.
> > > 
> > > Regards,
> > > Yassir.
> > > ___
> > > sssd-devel mailing list
> > > sssd-devel@lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > I have implemented the new design (described in ticket #2347) in which 
> > policy settings are written (and potentially over-written) to a "GPO 
> > Enforcement" object (which I ended up calling "GPO Result" object) in the 
> > sysdb cache. This allows policy settings to be resolved correctly for both 
> > the online and offline cases. It is a reasonably large patch, but I think 
> > the final implementation is cleaner and simpler. Revised patch is attached.
> 
> 
> 
> Sorry it took me so long to finish this review. The code is mostly
> right, but I found three issues that needed to be addressed before we
> could commit it.
> 
> 1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
> was resulting in the PAM conversation ultimately returning
> PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
> in the 0001 patch attached. Can be applied separately.
> 
> 2) There was an 'attrs' variable being passed in the
> sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
> causing a crash when accessing LDB.
> 
> 3) The code could not properly handle when a GPO had an explicitly empty
> value (such as one might use to expressly disable a setting from a
> lower-priority GPO).
> 
> Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
> of the changes I made, see
> https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
> my review there as I went through, so I wouldn't miss any corrections
> and as a sort of proof-of-concept)
> 
> 
> I (believe) I have done a very thorough review of this code, so I'd ask
> that someone double-check my updates and then we should be good to go
> with committing this. It's a pretty important set of fixes.


One additional note. This incorrect resolution order does *NOT* cause a
security issue. I've carefully tested that the way processing was
happening would only cause unintentional denials. (Specifically, we were
too zealous about returning a denial as soon as we saw one, even though
AD's GPOs permit the deny list to be overruled by a higher-priority
GPO.) However, because of the way we were doing the lookups, the reverse
was not true. A grant of permission overridden by an exclusion of that
user/group from the permitted list would still not grant permission,
because we were re-validating access permission for each GPO, so it had
to be positively accepted at all levels to succeed.


signature.asc
Description: This is a digitally signed message part
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] AD: conflicting gpo policy settings not being resolved correctly

[Stephen Gallagher]

On Thu, 2014-09-11 at 23:51 -0400, Yassir Elley wrote:
> 
> - Original Message -
> > 
> > 
> > - Original Message -
> > > Hi,
> > > 
> > > The attached patch fixes ticket #2437 (conflicting gpo policy settings not
> > > being resolved correctly)
> > > 
> > > https://fedorahosted.org/sssd/ticket/2437
> > > 
> > > Regards,
> > > Yassir.
> > > 
> > > ___
> > > sssd-devel mailing list
> > > sssd-devel@lists.fedorahosted.org
> > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > I discovered some more issues while doing further testing on this ticket
> > relating to "defined but empty" policy settings (see additional comments on
> > ticket #2347). I have also made some minor changes to this patch, b/c they
> > didn't warrant their own ticket. Namely, I have removed some attributes that
> > we weren't using (AD_AT_USER_EXT_NAMES, etc). I have also fixed the way we
> > handle machine_ext_names by allowing for the possibility of a "present but
> > empty" value for AD_AT_MACHINE_EXT_NAMES.
> > 
> > I am attaching a revised path to address these issues.
> > 
> > Regards,
> > Yassir.
> > ___
> > sssd-devel mailing list
> > sssd-devel@lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > 
> 
> I have implemented the new design (described in ticket #2347) in which policy 
> settings are written (and potentially over-written) to a "GPO Enforcement" 
> object (which I ended up calling "GPO Result" object) in the sysdb cache. 
> This allows policy settings to be resolved correctly for both the online and 
> offline cases. It is a reasonably large patch, but I think the final 
> implementation is cleaner and simpler. Revised patch is attached.



Sorry it took me so long to finish this review. The code is mostly
right, but I found three issues that needed to be addressed before we
could commit it.

1) The GPO code was returning EACCES instead of ERR_ACCESS_DENIED which
was resulting in the PAM conversation ultimately returning
PAM_SYSTEM_ERR. (Pre-existing, but found while testing this patch) Fixed
in the 0001 patch attached. Can be applied separately.

2) There was an 'attrs' variable being passed in the
sysdb_gpo_get_gpo_result_setting() routine without a NULL terminator,
causing a crash when accessing LDB.

3) The code could not properly handle when a GPO had an explicitly empty
value (such as one might use to expressly disable a setting from a
lower-priority GPO).

Issues 2) and 3) are both addressed by the 0002 patch. For an easy view
of the changes I made, see
https://reviewboard-fedoraserver.rhcloud.com/r/80/diff/2-3/ (I tracked
my review there as I went through, so I wouldn't miss any corrections
and as a sort of proof-of-concept)


I (believe) I have done a very thorough review of this code, so I'd ask
that someone double-check my updates and then we should be good to go
with committing this. It's a pretty important set of fixes.
From 84b624e7e1af863d6f32f609a62aa8e7bc231aee Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 1 Oct 2014 20:42:31 -0400
Subject: [PATCH 1/2] AD GPO: Fix incorrect return of EACCES

In the access providers, we expect to receive ERR_ACCESS_DENIED when
access is denied, but we were returning EACCES here. The effect was the
same, except that it presented ultimately as a system error instead of
a proper denial.
---
 src/providers/ad/ad_gpo.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index 4e31a4832e8dd7ae4dc2c924206ae61b96a799f3..7cb9619ca7d4fa2c8fba84a98a976c35a5e8093e 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1123,7 +1123,7 @@ ad_gpo_access_check(TALLOC_CTX *mem_ctx,
 } else {
 switch (gpo_mode) {
 case GPO_ACCESS_CONTROL_ENFORCING:
-return EACCES;
+return ERR_ACCESS_DENIED;
 case GPO_ACCESS_CONTROL_PERMISSIVE:
 DEBUG(SSSDBG_TRACE_FUNC, "access denied: permissive mode\n");
 sss_log_ext(SSS_LOG_WARNING, LOG_AUTHPRIV, "Warning: user would " \
@@ -1271,7 +1271,7 @@ ad_gpo_access_send(TALLOC_CTX *mem_ctx,
 if (gpo_map_type == GPO_MAP_DENY) {
 switch (ctx->gpo_access_control_mode) {
 case GPO_ACCESS_CONTROL_ENFORCING:
-ret = EACCES;
+ret = ERR_ACCESS_DENIED;
 goto immediately;
 case GPO_ACCESS_CONTROL_PERMISSIVE:
 DEBUG(SSSDBG_TRACE_FUNC, &q

Re: [SSSD] [PATCH] AD GPO: Fix incorrect sAMAccountName selection

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/25/2014 03:40 PM, Stephen Gallagher wrote:
> On 09/25/2014 04:56 AM, Jakub Hrozek wrote:
>> On Wed, Sep 24, 2014 at 11:10:00AM -0400, Stephen Gallagher
>> wrote: We were assuming that the ad_hostname value would match
>> the sAMAccountName attribute, but in practice this was almost
>> never the case on a properly-configured system.
> 
>> Microsoft's convention is that the sAMAccountName is always the 
>> portion of the FQDN before the first dot, so this patch makes
>> that same assumption.
> 
>>> From c179806c27ce6d25137306ba7bb37ecfae573c3b Mon Sep 17
>>> 00:00:00 2001 From: Stephen Gallagher 
>>> Date: Tue, 23 Sep 2014 17:44:41 -0400 Subject: [PATCH] AD GPO:
>>> Fix incorrect sAMAccountName selection
>>> 
>>> --- src/providers/ad/ad_gpo.c | 20 +++- 1 file 
>>> changed, 19 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/providers/ad/ad_gpo.c 
>>> b/src/providers/ad/ad_gpo.c index 
>>> de4d44166b85ccd85ed36bcb11f0596e0020af11..745af8b2786a5d6c71a2a3eb6c1448a61c151019
>>>
>>> 
100644 --- a/src/providers/ad/ad_gpo.c +++
>>> b/src/providers/ad/ad_gpo.c @@ -1479,6 +1479,8 @@ 
>>> ad_gpo_connect_done(struct tevent_req *subreq) struct
>>> tevent_req *req; struct ad_gpo_access_state *state; char
>>> *filter; +char *hostname; +char *shortname; char
>>> *sam_account_name; char *domain_dn; int dp_error; @@ -1519,7
>>> +1521,21 @@ ad_gpo_connect_done(struct tevent_req *subreq) } }
>>> 
>>> -sam_account_name = talloc_asprintf(state, "%s$", 
>>> state->ad_hostname); +hostname = talloc_strdup(state, 
>>> state->ad_hostname); +if (hostname == NULL) { +ret
>>> = ENOMEM; +goto done; +} +shortname = 
>>> strtok(hostname, "."); +if (shortname == NULL) { +
>>> /* This should never fail; if there's no dot, + * it
>>> should return the full string. + */ +ret = EIO;
>>> + goto done; +} +sam_account_name =
>>> talloc_asprintf(state, "%s$", hostname); +
>>> talloc_zfree(hostname); if (sam_account_name == NULL) { ret =
>>> ENOMEM; goto done;
> 
>> The fix works, but we already have code that does pretty much
>> the same for principal selection -- check out get_primary() in 
>> sss_krb5.c. Would it be better to split out lines 38 to 53 from 
>> get_primary() into a separate function and use it in ad_gpo.c,
>> too, to save code duplication?
> 
> 
> I didn't know about get_primary(). It's actually a perfect
> solution for this (and accomplishes everything I set out to do
> here, including appending the $ to the shortname).
> 
> See two new patches. The first renames get_primary() to 
> sss_krb5_get_primary() and makes it public, the second consumes it
> to generate an appropriate sAMAccountName value.
> 
> I tested this with my AD setup and it works correctly.

And this time with the patches attached...
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlQkb+IACgkQeiVVYja6o6OaewCdHU0NgaJQIJv08uKUXcYFYvlB
TLEAnRSMj/pPGjj9N71ID1Ky15K61Sus
=/aos
-END PGP SIGNATURE-
>From 00713c2b0044c2a91d5c69d55dd5c8c387c82c22 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Thu, 25 Sep 2014 15:34:20 -0400
Subject: [PATCH 1/2] krb5: make get_primary() a public call

This patch changes get_primary() into sss_krb5_get_primary() so it can
be used by the AD provider to get the sAMAccountName from the hostname.
---
 src/util/sss_krb5.c | 10 +++---
 src/util/sss_krb5.h |  6 ++
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/util/sss_krb5.c b/src/util/sss_krb5.c
index a7f1bf37cf46e588473b817d795e72f81fcafb07..b4012593d96bc951143e4bb2ba7a91d118b1a53c 100644
--- a/src/util/sss_krb5.c
+++ b/src/util/sss_krb5.c
@@ -26,8 +26,10 @@
 #include "util/util.h"
 #include "util/sss_krb5.h"
 
-static char *
-get_primary(TALLOC_CTX *mem_ctx, const char *pattern, const char *hostname)
+char *
+sss_krb5_get_primary(TALLOC_CTX *mem_ctx,
+ const char *pattern,
+ const char *hostname)
 {
 char *primary;
 char *dot;
@@ -132,7 +134,9 @@ errno_t select_principal_from_keytab(TALLOC_CTX *mem_ctx,
 
 do {
 if (primary_patterns[i]) {
-primary = get_primary(tmp_ctx, primary_patterns[i], hostname);
+primary = sss_krb5_get_primary(tmp_ctx,
+   primary_patterns[i],
+   hostname);
 if (primary == NULL

Re: [SSSD] [PATCH] AD GPO: Fix incorrect sAMAccountName selection

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/25/2014 04:56 AM, Jakub Hrozek wrote:
> On Wed, Sep 24, 2014 at 11:10:00AM -0400, Stephen Gallagher wrote: 
> We were assuming that the ad_hostname value would match the 
> sAMAccountName attribute, but in practice this was almost never
> the case on a properly-configured system.
> 
> Microsoft's convention is that the sAMAccountName is always the 
> portion of the FQDN before the first dot, so this patch makes that 
> same assumption.
> 
>> From c179806c27ce6d25137306ba7bb37ecfae573c3b Mon Sep 17 00:00:00
>> 2001 From: Stephen Gallagher  Date: Tue, 23
>> Sep 2014 17:44:41 -0400 Subject: [PATCH] AD GPO: Fix incorrect
>> sAMAccountName selection
>> 
>> --- src/providers/ad/ad_gpo.c | 20 +++- 1 file
>> changed, 19 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/providers/ad/ad_gpo.c
>> b/src/providers/ad/ad_gpo.c index
>> de4d44166b85ccd85ed36bcb11f0596e0020af11..745af8b2786a5d6c71a2a3eb6c1448a61c151019
>> 100644 --- a/src/providers/ad/ad_gpo.c +++
>> b/src/providers/ad/ad_gpo.c @@ -1479,6 +1479,8 @@
>> ad_gpo_connect_done(struct tevent_req *subreq) struct tevent_req
>> *req; struct ad_gpo_access_state *state; char *filter; +char
>> *hostname; +char *shortname; char *sam_account_name; char
>> *domain_dn; int dp_error; @@ -1519,7 +1521,21 @@
>> ad_gpo_connect_done(struct tevent_req *subreq) } }
>> 
>> -sam_account_name = talloc_asprintf(state, "%s$",
>> state->ad_hostname); +hostname = talloc_strdup(state,
>> state->ad_hostname); +if (hostname == NULL) { +ret =
>> ENOMEM; +goto done; +} +shortname =
>> strtok(hostname, "."); +if (shortname == NULL) { +/*
>> This should never fail; if there's no dot, + * it should
>> return the full string. + */ +ret = EIO; +
>> goto done; +} +sam_account_name = talloc_asprintf(state,
>> "%s$", hostname); +talloc_zfree(hostname); if
>> (sam_account_name == NULL) { ret = ENOMEM; goto done;
> 
> The fix works, but we already have code that does pretty much the
> same for principal selection -- check out get_primary() in
> sss_krb5.c. Would it be better to split out lines 38 to 53 from
> get_primary() into a separate function and use it in ad_gpo.c, too,
> to save code duplication?
> 

I didn't know about get_primary(). It's actually a perfect solution
for this (and accomplishes everything I set out to do here, including
appending the $ to the shortname).

See two new patches. The first renames get_primary() to
sss_krb5_get_primary() and makes it public, the second consumes it to
generate an appropriate sAMAccountName value.

I tested this with my AD setup and it works correctly.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlQkb6MACgkQeiVVYja6o6PNZQCfRx90vgnvRP7YUX7WtZtUcZJn
D28An3faKQ94a5ek1wGx3Lr348MOYheM
=RR7r
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] AD GPO: Fix incorrect sAMAccountName selection

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

We were assuming that the ad_hostname value would match the
sAMAccountName attribute, but in practice this was almost never the
case on a properly-configured system.

Microsoft's convention is that the sAMAccountName is always the
portion of the FQDN before the first dot, so this patch makes that
same assumption.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlQi3sgACgkQeiVVYja6o6OJDwCfRgV66Mp/oLWfNXXamXMC7S1i
KYwAoK4H6eogurMto2oUVM6V9pVDbZ0C
=1Nj3
-END PGP SIGNATURE-
>From c179806c27ce6d25137306ba7bb37ecfae573c3b Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Tue, 23 Sep 2014 17:44:41 -0400
Subject: [PATCH] AD GPO: Fix incorrect sAMAccountName selection

---
 src/providers/ad/ad_gpo.c | 20 +++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/src/providers/ad/ad_gpo.c b/src/providers/ad/ad_gpo.c
index de4d44166b85ccd85ed36bcb11f0596e0020af11..745af8b2786a5d6c71a2a3eb6c1448a61c151019 100644
--- a/src/providers/ad/ad_gpo.c
+++ b/src/providers/ad/ad_gpo.c
@@ -1479,6 +1479,8 @@ ad_gpo_connect_done(struct tevent_req *subreq)
 struct tevent_req *req;
 struct ad_gpo_access_state *state;
 char *filter;
+char *hostname;
+char *shortname;
 char *sam_account_name;
 char *domain_dn;
 int dp_error;
@@ -1519,7 +1521,21 @@ ad_gpo_connect_done(struct tevent_req *subreq)
 }
 }
 
-sam_account_name = talloc_asprintf(state, "%s$", state->ad_hostname);
+hostname = talloc_strdup(state, state->ad_hostname);
+if (hostname == NULL) {
+ret = ENOMEM;
+goto done;
+}
+shortname = strtok(hostname, ".");
+if (shortname == NULL) {
+/* This should never fail; if there's no dot,
+ * it should return the full string.
+ */
+ret = EIO;
+goto done;
+}
+sam_account_name = talloc_asprintf(state, "%s$", hostname);
+talloc_zfree(hostname);
 if (sam_account_name == NULL) {
 ret = ENOMEM;
 goto done;
@@ -1548,6 +1564,8 @@ ad_gpo_connect_done(struct tevent_req *subreq)
 goto done;
 }
 
+talloc_zfree(sam_account_name);
+
 subreq = sdap_get_generic_send(state, state->ev, state->opts,
sdap_id_op_handle(state->sdap_op),
domain_dn, LDAP_SCOPE_SUBTREE,
-- 
2.1.0



0001-AD-GPO-Fix-incorrect-sAMAccountName-selection.patch.sig
Description: PGP signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[SSSD] [PATCH] UTIL: Do not change SSSD domains in get_domains_head

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

When there was more than one SSSD domain configured, actions performed
against domains later in the list would be incorrectly told to use the
first domain as the base for locating subdomains. This was because we
were rewinding the ->prev list on the sss_domain_info object, which is
only intended to be used by confdb code. The correct approach was to
use only the parent linkage, which would take us up to the top-level
domain in this SSSD domain.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlQi3YEACgkQeiVVYja6o6PqAACgjb4ISPCELnMMBIoKKHX/tj8r
UdgAmQHRTCMC0BQo8oBlFy4ZKNj1gshs
=AaTR
-END PGP SIGNATURE-
>From fee75b35053029a9b856a231f99fa607bd91e8e4 Mon Sep 17 00:00:00 2001
From: Stephen Gallagher 
Date: Wed, 24 Sep 2014 11:00:44 -0400
Subject: [PATCH] UTIL: Do not change SSSD domains in get_domains_head

When there was more than one SSSD domain configured, actions performed
against domains later in the list would be incorrectly told to use the
first domain as the base for locating subdomains. This was because we
were rewinding the ->prev list on the sss_domain_info object, which is
only intended to be used by confdb code. The correct approach was to
use only the parent linkage, which would take us up to the top-level
domain in this SSSD domain.
---
 src/util/domain_info_utils.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/util/domain_info_utils.c b/src/util/domain_info_utils.c
index 2d29743c9b1a934ffb5d65d0943111b5184a2cec..8933f52353abd63ce825e80b1cde2aad03ed7797 100644
--- a/src/util/domain_info_utils.c
+++ b/src/util/domain_info_utils.c
@@ -34,9 +34,6 @@ struct sss_domain_info *get_domains_head(struct sss_domain_info *domain)
 /* get to the top level domain */
 for (dom = domain; dom->parent != NULL; dom = dom->parent);
 
-/* proceed to the list head */
-for (; dom->prev != NULL; dom = dom->prev);
-
 return dom;
 }
 
-- 
2.1.0



0001-UTIL-Do-not-change-SSSD-domains-in-get_domains_head.patch.sig
Description: PGP signature
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Patch to fix incorrect PAM return code when user enters invalid credentials

[Stephen Gallagher]

On 09/03/2014 07:59 PM, Stephen Gallagher wrote:
> -BEGIN PGP SIGNED MESSAGE-
> Hash: SHA1
>
> On 08/28/2014 10:00 PM, John Koelndorfer wrote:
> > Hey folks,
> >
> > Some quick background on this small patch I prepared. I run sssd on
> > my desktop (and servers) to authenticate against a Samba 4 DC. I
> > found that when I attempted to log in via KDM and misentered my
> > password, I got an error about the authentication system failing.
> > Similarly, `su` would return an error message I was not familar
> > with: "Failure setting user credentials".
> >
> > After some inspection of the sssd sources, I found that per
> > http://pubs.opengroup.org/onlinepubs/8329799/pam_sm_authenticate.htm,
> >
> >
> sssd's PAM module is returning the wrong error code when a user
> > entered bad credentials. PAM_CRED_ERR is being returned instead of
> > PAM_AUTH_ERR.
> >
> > Applying the attached patch and recompliing sssd brought back the
> > more familiar "Authentication failure" when su'ing with a bad
> > password. KDM also doesn't freak out when I enter an incorrect
> > password.
> >
> > If you have any questions about the patch, please be sure to
> > include me in the reply as I'm not on the sssd-devel list.
> >
> > Thanks for sssd, it has been awesome!
> >
>
> Thanks for the patch! Unfortunately, it's a little bit overzealous.
> There are several cases in here for which PAM_CRED_ERR is actually
> correct.
>
> For example, in krb5_auth.c, we return PAM_AUTH_ERR if the kerberos
> request returns ERR_AUTH_FAILED, but we return PAM_CRED_ERR
> specifically on kerberos returning ERR_CRED_ERR.
>
> PAM_AUTH_ERR is intended to be returned when the server was
> succesfully able to perform the authentication and explicitly
> determined that the user was denied. PAM_CRED_ERR should indicate that
> the password failed to validate for some other reason. According to
> krb5_child.c, that can happen as a result of three return codes:
>
> KRB5_PROG_ETYPE_NOSUPP (The client and the server don't have any
> matching encryption keys)
> KRB5_PREAUTH_FAILED (A one-time passwords or pkinit event failed)
> KRB5KDC_ERR_PREAUTH_FAILED (The same as above, or a failure while
> setting up a FAST tunnel occurred).
>
> Are you using FAST on your setup? (The option krb5_use_fast = True)


John and I debugged this some today on #sssd. It turns out that the Samba 4 DC 
(and we suspect the Active Directory DC) returns the KRB5KDC_ERR_PREAUTH_FAILED 
on an incorrect password, and not KRB5KRB_AP_ERR_BAD_INTEGRITY like the MIT KDC 
does.

Unfortunately, we have code in krb5_auth.c that is using 
KRB5KDC_ERR_PREAUTH_FAILED as one possible way of detecting that SSSD is in 
migration mode from FreeIPA, so this is going to require a little bit of 
careful design to solve properly.

I've been too far removed from the Kerberos code for too long, so I'd very much 
appreciate it if one of our resident Kerberos experts could take a closer look 
at this one.
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] Patch to fix incorrect PAM return code when user enters invalid credentials

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/28/2014 10:00 PM, John Koelndorfer wrote:
> Hey folks,
> 
> Some quick background on this small patch I prepared. I run sssd on
> my desktop (and servers) to authenticate against a Samba 4 DC. I
> found that when I attempted to log in via KDM and misentered my
> password, I got an error about the authentication system failing.
> Similarly, `su` would return an error message I was not familar
> with: "Failure setting user credentials".
> 
> After some inspection of the sssd sources, I found that per 
> http://pubs.opengroup.org/onlinepubs/8329799/pam_sm_authenticate.htm,
>
> 
sssd's PAM module is returning the wrong error code when a user
> entered bad credentials. PAM_CRED_ERR is being returned instead of 
> PAM_AUTH_ERR.
> 
> Applying the attached patch and recompliing sssd brought back the
> more familiar "Authentication failure" when su'ing with a bad
> password. KDM also doesn't freak out when I enter an incorrect
> password.
> 
> If you have any questions about the patch, please be sure to
> include me in the reply as I'm not on the sssd-devel list.
> 
> Thanks for sssd, it has been awesome!
> 

Thanks for the patch! Unfortunately, it's a little bit overzealous.
There are several cases in here for which PAM_CRED_ERR is actually
correct.

For example, in krb5_auth.c, we return PAM_AUTH_ERR if the kerberos
request returns ERR_AUTH_FAILED, but we return PAM_CRED_ERR
specifically on kerberos returning ERR_CRED_ERR.

PAM_AUTH_ERR is intended to be returned when the server was
succesfully able to perform the authentication and explicitly
determined that the user was denied. PAM_CRED_ERR should indicate that
the password failed to validate for some other reason. According to
krb5_child.c, that can happen as a result of three return codes:

KRB5_PROG_ETYPE_NOSUPP (The client and the server don't have any
matching encryption keys)
KRB5_PREAUTH_FAILED (A one-time passwords or pkinit event failed)
KRB5KDC_ERR_PREAUTH_FAILED (The same as above, or a failure while
setting up a FAST tunnel occurred).

Are you using FAST on your setup? (The option krb5_use_fast = True)

-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlQHq1QACgkQeiVVYja6o6PdiQCeOkmHb/nfSg3UwZFnNtsxlhz1
U84An3C6kPthahgNc26m5RhwCJpmzY7q
=URR1
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] Ignore referrals when ldap_referrals=false

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 08/20/2014 09:20 AM, Jakub Hrozek wrote:
> Hi,
> 
> with the current SSSD code, an LDAP search that results in a
> referral fails completely with EIO and usually sends the whole
> backend to offline mode. I think this is too strict and if the
> admin chose to ignore referrals, we should just skip these
> results.
> 
> John Hodrien in particular was hit by us treating referrals as
> fatal in environment where he needs to restrict the search scope by
> using custom LDAP search bases.
> 
> Also, in cases where Global Catalog support is disabled or GC not
> available and a group contains a user from a trusted domain, trying
> to search for this DN yields a referral.
> 
> Attached is a patch that ignores referrals when the admin set 
> ldap_referrals=false in the config file.
> 
> Given the sdap async code is quite old and I don't remember all
> the use-cases, I CC-ed Stephen directly to get some advice. Is
> there any risk in ignoring referrals?
> 

I'd say this has likely always been a bug, not intentional behavior.
Go ahead and ignore the referrals.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1

iEYEARECAAYFAlP3fV0ACgkQeiVVYja6o6Pc1wCgiVzMK186UfhW5xn6yVQZB704
5/gAnA521g0vMc7ZDNcKKi/I55MSLrdy
=Hlxt
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCHES] sss_case = preserving

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/22/2014 09:32 AM, Michal Židek wrote:
> On 07/22/2014 02:49 PM, Pavel Reichl wrote:
>> 
>> On 07/22/2014 02:03 PM, Pavel Reichl wrote:
>>> I finally tested the patches and it seems to me to be working
>>> with AD and LDAP provider, but does not seem to work with IPA
>>> provider.
>>> 
>>> I was not able to create an user in IPA with some upper case so
>>> I had to change it directly in LDAP.
>>> 
>>> This is what I see when using ipa:
>>> 
>>> ipa user-find max -- 1 user matched -- 
>>> User login: MaX First name: xx Last name: AAA Home directory:
>>> /home/max Login shell: /bin/sh Email address: m...@ipa.work UID:
>>> 199438 GID: 199438 Account disabled: False Password:
>>> False Kerberos keys available: False 
>>>  Number of entries returned 1 
>>> 
>>> 
>>> This is what I see when using sssd as ipa client:
>>> 
>>> case_sensitive = preserving: getent group m...@ipa.work 
>>> m...@ipa.work:*:199438:
>>> 
>>> case_sensitive = false: getent group m...@ipa.work 
>>> m...@ipa.work:*:199438:
>>> 
>>> case_sensitive = true: does not return anything
>>> 
>>> Was the patch working with IPA provider to you?
>>> 
>>> Thanks!
>>> 
>> Michal this seems irrelevant now, I can't replicate it anymore.
>> It must have been some quirk on my side. Sorry for that.
>> 
>> ACK to all patches except the 3rd (the man page update) which I
>> can't ack.
>> 
>> Thanks!
>> 
> 
> Thanks, I updated the man page changes (added the "value" word and
> AD comment). CC-ing Stephen for the man page review (3rd patch).
> 
> Michal
> 
> 

Ack to the manpage language.
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlPOaGEACgkQeiVVYja6o6NhvwCeIQ7upLLCa2PdfWha7tfssuSC
hSoAn1blLO+hqsJi4vSiD7NCr692u5jo
=uPQw
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH] ldap_opts: Get rid on 389ds specific values in rfc2307bis schema

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/20/2014 03:20 PM, Jakub Hrozek wrote:
> On Fri, Jul 18, 2014 at 05:32:09PM +0200, Lukas Slebodnik wrote:
>> On (18/07/14 16:34), Jakub Hrozek wrote:
>>> On Thu, Jul 17, 2014 at 04:35:31PM +0200, Lukas Slebodnik
>>> wrote:
 ehlo,
 
 There is problem with OpenLDAP server and dereferencing of
 attributes that is not in the schema  of the server?
 
 sh-4.2$ ldapsearch -x -LLL -h openldap.server.test -b
 'dc=example,dc=com' \ -E 'deref=member:uid,dummy_attr'
 cn=ref_grp Protocol error (2) Additional information:
 Dereference control: attribute decoding error sh-4.2$ echo
 $? 2
 
 The attribute nsUniqueID is a 389-only, non-standard
 attribute. It is an operational attribute that is not in the
 rfc2307bis nor inetOrgPerson nor posixAccount schema.
 OpenLDAP supports the standard entryUUID attribute, which is
 basically the same (uniquely identifies an entry throughout a
 replication topology), but uses the standard UUID format
 rather than the non-standard format used by 389.
 
 4x FIXME removed :-)
 
 
 Any comments are welcomed.
 
 LS
>>> 
>>> Thanks for the detective work on finding the root cause of the
>>> problem!
>>> 
>>> I wonder if we could remove the attribute completely, though.
>>> It appears to be completely unused now:
>>> 
>>> $ git grep -l SYSDB_UUID src/db/sysdb.h:#define SYSDB_UUID
>>> "uniqueID" src/providers/ad/ad_opts.h:{ "ldap_user_uuid",
>>> "objectGUID", SYSDB_UUID, NULL }, src/providers/ad/ad_opts.h:
>>> { "ldap_group_uuid", "objectGUID", SYSDB_UUID, NULL }, 
>>> src/providers/ad/ad_opts.h:{ "ldap_netgroup_uuid",
>>> "nsUniqueId", SYSDB_UUID, NULL }, src/providers/ipa/ipa_opts.h:
>>> { "ldap_user_uuid", "nsUniqueId", SYSDB_UUID, NULL }, 
>>> src/providers/ipa/ipa_opts.h:{ "ldap_group_uuid",
>>> "nsUniqueId", SYSDB_UUID, NULL }, src/providers/ipa/ipa_opts.h:
>>> { "ipa_netgroup_uuid", "ipaUniqueID", SYSDB_UUID, NULL }, 
>>> src/providers/ipa/ipa_opts.h:{ "ipa_host_uuid",
>>> "ipaUniqueID", SYSDB_UUID, NULL}, src/providers/ipa/ipa_opts.h:
>>> { "ipa_hostgroup_uuid", "ipaUniqueID", SYSDB_UUID, NULL}, 
>>> src/providers/ipa/ipa_opts.h:{ "ipa_selinux_usermap_uuid",
>>> "ipaUniqueID", SYSDB_UUID, NULL}, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_user_uuid", NULL,
>>> SYSDB_UUID, NULL }, src/providers/ldap/ldap_opts.h:{
>>> "ldap_group_uuid", NULL, SYSDB_UUID, NULL }, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_user_uuid",
>>> "nsUniqueId", SYSDB_UUID, NULL }, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_group_uuid",
>>> "nsUniqueId", SYSDB_UUID, NULL }, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_user_uuid",
>>> "objectGUID", SYSDB_UUID, NULL }, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_group_uuid",
>>> "objectGUID", SYSDB_UUID, NULL }, 
>>> src/providers/ldap/ldap_opts.h:{ "ldap_netgroup_uuid",
>>> "nsUniqueId", SYSDB_UUID, NULL },
>>> 
>>> And according to "git log -S" it was actually never used.
>>> 
>>> So my proposal is to just remove the attribute along with
>>> SYSDB_UUID.
>> I was thinking about this change but I thought that we need to
>> deprecate options before removing. I don't have a problem with
>> removing them. and it should not be a problem in this case. I
>> would like to know opinion of older developers (Sumit, Stephen,
>> ...) Why were these options introduced? Do they help with ldap
>> search?
>> 
>> LS
> 
> Sure, being defensive when removing anything is a good idea. But
> since the option doesn't do anything and never did, I think we're
> fine. You're right that removing an option requires two acks,
> though. So far you have mine :-)

I'll ack it as well. I suspect that this was planned for a future use
that we never ended up with. I don't remember any reason for it.

-BEGIN PGP SIGNATURE-
Version: GnuPG v1
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlPNLUkACgkQeiVVYja6o6MqVwCfXP/RfVbEL3ojKjBdEw3JLvXv
NeAAn10YBMy2Q2J75KQxDS7JOSP+VnJA
=2Gtf
-END PGP SIGNATURE-
___
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

Re: [SSSD] [PATCH][ding-libs] SPEC: Do not call autoreconf on epel5

[Stephen Gallagher]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 07/10/2014 12:24 PM, Lukas Slebodnik wrote:
> On (10/07/14 10:51), Stephen Gallagher wrote:
>> On 07/10/2014 05:46 AM, Lukas Slebodnik wrote:
>>> On (07/07/14 20:22), Stephen Gallagher wrote:
>>>> On 07/07/2014 10:05 AM, Lukas Slebodnik wrote:
>>>>> ehlo,
>>>>> 
>>>>> There is a bug in old version of autotools which cause 
>>>>> compilation problems
>>>>> 
>>>>> ./libtool: line 1161: X-c: command not found ./libtool:
>>>>> line 1213: Xpath_utils/path_utils.lo: No such file or
>>>>> directory ./libtool: line 1218: libtool: compile: cannot
>>>>> determine name of library object from `': command not found
>>>>> make[1]: *** [path_utils/path_utils.lo] Error 1 make[1]:
>>>>> Leaving directory `/builddir/build/BUILD/ding-libs-0.4.0'
>>>>> 
>>>>> I used this patch for building ding-libs in copr [1] but I 
>>>>> forgot to send patch to upstream.
>>>>> 
>>>>> LS
>>>>> 
>>>>> [1]
>>>>> https://copr.fedoraproject.org/coprs/lslebodn/ding-libs/
>>>>> 
>>>> 
>>>> Nack. It's always better to perform the autoreconf properly.
>>>> We hit this same bug back in 2011 with SSSD[1]. I just
>>>> performed a scratch build with the attached patch[2] which
>>>> worked successfully.
>>>> 
>>>> [1] Commit ID a1294c95a4f9f37bbe9a8635defa3a45e59213ab in
>>>> the SSSD git [2] 
>>>> http://koji.fedoraproject.org/koji/taskinfo?taskID=7114910
>>>> 
>>>> 
>>>> 
>>> 
>>>> From f31135f1abc3d856d9a5818b1a60dbb8414a6e5d Mon Sep 17
>>>> 00:00:00 2001 From: Stephen Gallagher 
>>>> Date: Mon, 7 Jul 2014 20:14:36 -0400 Subject: [PATCH] Fix
>>>> specfile for RHEL5
>>>> 
>>>> RHEL5 uses an old libtool. We need to forcibly remove certain
>>>> m4 files before running autoreconf to ensure that they get
>>>> replaced with the appropriate old versions. ---
>>>> contrib/ding-libs.spec.in | 10 ++ 1 file changed, 10
>>>> insertions(+)
>>>> 
>>>> diff --git a/contrib/ding-libs.spec.in 
>>>> b/contrib/ding-libs.spec.in index 
>>>> 87f2a1ab915c2225f0a2b83a1de1d2980489e210..30d6775401f42f565de0aef0cfa179cd9dc38e74
>>>>
>>>> 
100644 --- a/contrib/ding-libs.spec.in +++
>>>> b/contrib/ding-libs.spec.in @@ -324,7 +324,17 @@ structure
>>>> %setup -q
>>>> 
>>>> %build + +# RHEL 5 uses an old libtool, so we need to force
>>>> it to reconfigure +# This is safe to do on newer packages
>>>> too, as it will just +# gather the appropriate m4 files from
>>>> the libtool package
>>> This explanation is not right. The problem is not with libtool
>>> but with buggy version of autoreconf on RHEL5.
>>> 
>>> bash-3.2# autoreconf -if You should update your `aclocal.m4'
>>> by running aclocal. Putting files in AC_CONFIG_AUX_DIR,
>>> `build'. Makefile.am:259: variable
>>> `EXTRA_libini_config_la_DEPENDENCIES' is defined but no program
>>> or Makefile.am:259: library has `EXTRA_libini_config_la' as
>>> canonic name (possible typo)
>>> 
>>> As you can see there is a warning: "You should update your 
>>> `aclocal.m4' by running aclocal."
>>> 
>>> Warnings "EXTRA_*" are irrelevant in this case.
>>> 
>>> man autoreconf says: Run  `autoconf'  (and  `autoheader', 
>>> `aclocal', `automake', `autopoint' (formerly `gettextize'),
>>> and `libtoolize' where appropriate) repeatedly to  remake  the
>>> GNU Build System files in the DIRECTORIES or the direc‐ tory
>>> trees driven by CONFIGURE-AC (defaulting to `.').
>>> 
>>> So aclocal should be executed by autoreconf itself.
>>> 
>>>> +for i in libtool.m4  lt~obsolete.m4  ltoptions.m4
>>>> ltsugar.m4 ltversion.m4 +do +find . -name $i -exec rm -f
>>>> {} \; +done + autoreconf -ivf + %configure \
>>>> --disable-static
>>> 
>>> I would prefer to either run "aclocal" after autoreconf or
>>> remove all files from directory m4 (rm -rf m4/* 2>/dev/null).
>>> For loop is a kind of hack, because we know that all files will
>>> be stored in d