Re: [SSSD] [PATCH] Fix LDAP AutoFS object class man page
On Tue, Jul 07, 2015 at 01:00:37AM +0100, Robin McCorkell wrote: > Fixed default parameter value and description for > ldap_autofs_entry_object_class > --- > src/man/sssd-ldap.5.xml | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml > index f140908..3436f9e 100644 > --- a/src/man/sssd-ldap.5.xml > +++ b/src/man/sssd-ldap.5.xml > @@ -2493,11 +2493,12 @@ ldap_access_filter = (employeeType=admin) > ldap_autofs_entry_object_class (string) > > > -The object class of an automount map entry > -in LDAP. > +The object class of an automount entry > +in LDAP. The entry usually corresponds to a mount > +point. > > > -Default: automountMap > +Default: automount > > > > -- > 2.4.5 Thank you, this patch is correct and I'm fine with commiting it as-is, but looking at the options, I also wonder if we should document them better per-schema? For instance, the ldap_autofs_map_name defaults to "ou" if schema is set to rfc2307 but defaults to "automountMapName" with rfc2307bis. And IIRC the bis schema is what most autofs deployments use, so we might save some remapping for bis configs... ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] [PATCH] Fix LDAP AutoFS object class man page
Fixed default parameter value and description for ldap_autofs_entry_object_class --- src/man/sssd-ldap.5.xml | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml index f140908..3436f9e 100644 --- a/src/man/sssd-ldap.5.xml +++ b/src/man/sssd-ldap.5.xml @@ -2493,11 +2493,12 @@ ldap_access_filter = (employeeType=admin) ldap_autofs_entry_object_class (string) -The object class of an automount map entry -in LDAP. +The object class of an automount entry +in LDAP. The entry usually corresponds to a mount +point. -Default: automountMap +Default: automount -- 2.4.5 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] Announcing SSSD 1.13.0
On Mon, Jul 06, 2015 at 10:57:15PM +0200, Jakub Hrozek wrote: > === SSSD 1.13.0 === > > The SSSD team is proud to announce the release of version 1.13.0 of > the System Security Services Daemon. Sorry about the copy-n-paste bug in Subject. Of course it should have read "Announcing SSSD 1.13.0" without the "Alpha". ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] Announcing SSSD 1.13.0 Alpha
=== SSSD 1.13.0 === The SSSD team is proud to announce the release of version 1.13.0 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 rawhide shortly. == Feedback == Please provide comments, bugs and other feedback via the sssd-devel or sssd-users mailing lists: https://lists.fedorahosted.org/mailman/listinfo/sssd-devel https://lists.fedorahosted.org/mailman/listinfo/sssd-users == Highlights == * Support for separate prompts when using two-factor authentication was added * Added support for one-way trusts between an IPA and Active Directory environment. Please note that this SSSD functionality depends on IPA code that is not released at the moment. * The fast memory cache now also supports the initgroups operation. * The PAM responder is now capable of caching authentication for configurable period, which might reduce server load in cases where accounts authenticate very frequently. Please refer to the cached_auth_timeout option in the sssd.conf manual page. * The Active Directory provider has changed the default value of the ad_gpo_access_control option from permissive to enforcing. As a consequence, the GPO access control now affects all clients that set access_provider to ad. In order to restore the previous behaviour, set ad_gpo_access_control to permissive or use a different access_provider type. * Group Policy objects defined in a different AD domain that the computer object is defined in are now supported. * Credential caching and Offline authentication are also available when using two-factor authentication * Many enhancements to the InfoPipe D-Bus API. Notably, the SSSD users and groups are now exposed as first-class objects. The users and groups can also be marked as cached and would subsequently show up in the Introspection output * The DBus interface is now also able to look up User objects by certificate. This is a first part of work that will eventually allow smart-card authentication in SSSD. * The LDAP cleanup task is now disabled by default, unless enumeration is enabled. Please refer to the ldap_purge_cache_timeout option in case your environment requires the cleanup task * The Python bindings are now built for both Python2 and Python3 * The LDAP bind timeout, StartTLS timeout and password change timeout are now configurable using the ldap_opt_timeout option == Packaging Changes == * A new directory /var/lib/sss/keytabs is present and owned by the sssd-ipa subpackage. The SSSD stores keytabs for one-way trust relationships in this directory. Downstreams should make sure that the directory is only readable to the user who runs the SSSD service. * Several packaging changes are present in this release to support the Python3 bindings, notably new python-sss and python-sss-murmur subpackages are introduced in upstream RPM packaging * All python bindings now have a Python3 and a Python2 version in the upstream RPM packaging scheme * The OpenSSL development library such as openssl-devel on RHEL/Fedora or Debian/Ubuntu? libssl-dev is now required to support certificate operations * A new internal library libsss_cert.so is present in this release. * The fast initgroups memcache is represented by a new file /var/lib/sss/mc/initgroups == Documentation Changes == * The ad_gpo_access_control option default has changed from permissive to enforcing * The default value of ldap_purge_cache_timeout changed to 0, thus effectivelly disabling the cleanup task. * A new option cache_credentials_minimal_first_factor_length was added. This option sets constraints on the password length if One-Time passwords are used and credentials are to be cached. Please see the sssd.conf(5) man page for more details * The cached authentication is controlled by new option cached_auth_timeout. By default the cached authentication is disabled. == Tickets Fixed == https://fedorahosted.org/sssd/ticket/897 sssd should pass -d to nsupdate when running with high log level https://fedorahosted.org/sssd/ticket/1501 Make the LDAP bind operation timeout configurable https://fedorahosted.org/sssd/ticket/2150 [RFE] Expose listing calls over D-BUS https://fedorahosted.org/sssd/ticket/2224 nsupdate stderr is not captured https://fedorahosted.org/sssd/ticket/2236 The cleanup task has no DEBUG statements https://fedorahosted.org/sssd/ticket/2326 SBUS: Flush the UID cache when we receive NameOwnerChanged https://fedorahosted.org/sssd/ticket/2338 [RFE] Implement object caching on the bus https://fedorahosted.org/sssd/ticket/2339 IFP: support multiple interfaces for object https://fedorahosted.org/sssd/ticket/2540 SSSD does not update Dynamic DNS records if the IPA domain differs from machine hostname's domain https://fedorahosted.org/sssd/ticket/2569 In ipa-ad trust, with 'default_domain_suff
Re: [SSSD] [PATCHES] PAM: authenticate agains cache
On Mon, Jul 06, 2015 at 04:02:59PM +0200, Sumit Bose wrote: > On Mon, Jul 06, 2015 at 01:44:36PM +0200, Jakub Hrozek wrote: > > On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote: > > > On 07/02/2015 03:06 PM, Pavel Reichl wrote: > > > >Hello, > > > > > > > >new round of patches is attached. I discussed the required changes with > > > >Jakub to be sure we are on the same page as this thread is quite > > > >voluminous. > > > > > > > >Basically I just made the setters/getters static and removed tests of > > > >this > > > >functions. I hope that by this change we finally reached the desired > > > >level > > > >of design quality. > > > > > > > >Thanks! > > > > > > > >PS: Feel free to ignore patch 3. It's just a by-product of this effort. > > > > > > > > > > > >___ > > > >sssd-devel mailing list > > > >sssd-devel@lists.fedorahosted.org > > > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > > Attached patch set fixes minor code style issues. > > > > These patches work for me, but I also tested OTP and it doesn't behave > > as we agreed on in this thread: > > https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html > > > > Currently only the first factor was considered during the cached auth > > period, so anything could be input as the second factor. I propose this > > patch: > > > > https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&id=848ccf531afebe54be5d9c120fa90c18b0749cfe > > ACK. CI: http://sssd-ci.duckdns.org/logs/job/18/63/summary.html > One might argue that pam_is_authtok_cachable() can be written in a > more compact way, but imo it is the compilier's job to optimize to > code. Yes, but I tried to keep the same style as pam_is_cmd_cachable that is just above that. And from the experience of debugging optimized code I know the result is nothing like the source :) Thank you for the review. Pushed to master: * 7e798b94cfffe7bf8f7b477d540b95d52ca1f6e4 * 6aff93510b36799c1773d368cc218cd533c43161 * 0aa18cc0bf3447ca734476926724f1632e160807 * 32cc237aa0f3c70a4e0bc0491ec0cba0016aaf5a ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCHES] PAM: authenticate agains cache
On Mon, Jul 06, 2015 at 01:44:36PM +0200, Jakub Hrozek wrote: > On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote: > > On 07/02/2015 03:06 PM, Pavel Reichl wrote: > > >Hello, > > > > > >new round of patches is attached. I discussed the required changes with > > >Jakub to be sure we are on the same page as this thread is quite > > >voluminous. > > > > > >Basically I just made the setters/getters static and removed tests of this > > >functions. I hope that by this change we finally reached the desired level > > >of design quality. > > > > > >Thanks! > > > > > >PS: Feel free to ignore patch 3. It's just a by-product of this effort. > > > > > > > > >___ > > >sssd-devel mailing list > > >sssd-devel@lists.fedorahosted.org > > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > > Attached patch set fixes minor code style issues. > > These patches work for me, but I also tested OTP and it doesn't behave > as we agreed on in this thread: > https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html > > Currently only the first factor was considered during the cached auth > period, so anything could be input as the second factor. I propose this > patch: > > https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&id=848ccf531afebe54be5d9c120fa90c18b0749cfe ACK. One might argue that pam_is_authtok_cachable() can be written in a more compact way, but imo it is the compilier's job to optimize to code. bye, Sumit > > We should also amend the manpage if the code is OK. > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 02:05:04PM +0200, Sumit Bose wrote: > CI passes for me as well > http://sssd-ci.duckdns.org/logs/job/18/60/summary.html. > > ACK. Thank you for the review, I pushed the patches upstream: * 01ec08efd0e166ac6f390f8627c6d08dcc63ccc4 * eca74a9559ce1b0f123c14906ad8394fc303f468 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, 2015-07-06 at 11:46 +0200, Sumit Bose wrote: > On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > > On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: > > > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: > > > > > > Hi, > > > > > > > > > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701 > > > > > > > > > > > > The first patch just adds a common function instead of copying the > > > > > > same > > > > > > pattern again to the new test. > > > > > > > > > > > > The second adds a new request krb5_auth_queue_send() that wraps > > > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I > > > > > > hope > > > > > > the unit tests cover a lot of use-cases, if not, please suggest > > > > > > more! > > > > > > > > > > > > btw I was thinking that the chaining might not always be necessary > > > > > > if > > > > > > the ccache is of type MEMORY and I hope that the serializaton > > > > > > wouldn't > > > > > > be perceived as performance regression for users. Shall we say that > > > > > > Pavel's cached auth patches are a more systematic solution that > > > > > > doesn't > > > > > > rely on properties of the ccache type in that case? > > > > > > > > > > I'm sorry, but CI fails on Debian because of wrong linking with > > > > > libraries. I'm already testing a fix. Review of the rest is > > > > > appreciated > > > > > :-) > > > > > > > > If we are serializing all authentications then a busy server would be > > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > > NACK the approach sorry. > > > > > > The current cache queue is per user, see add_to_wait_queue() for > > > details. > > > > I actually did look to check that and failed, I blame the late hour :-) > > > > Per-user is fine by me, though I would seriously consider de-duplication > > while we are here. > > I think this might be a useful feature, although I think it should not > be on by default. We have discussed this already a bit 5 years ago > together with the initial implementation of the auth queues in the krb5 > responder. Here are some links for reference: > > https://fedorahosted.org/sssd/ticket/533 > https://lists.fedorahosted.org/pipermail/sssd-devel/2010-October/004821.html > https://lists.fedorahosted.org/pipermail/sssd-devel/2010-December/005307.html > > But I think this feature would need some more discussion. E.g. where > should it be implemented, in the PAM responder on in the auth backends? auth backend, the pam responder may not have enough context to properly decide. > How does is related to the cached authentication feature? It doesn't relate in any way IMO. > How should failed authentication be handled, de-dup as well or send them to > the > backend to allow the server to update the failed logon counter properly? The backend de-dups them and you won't know if an authentication is successful or failed until the answer comes back, so de-duping has already happened. > Shall de-duplication be done by user or by user and by service? This is a good question, I think it makes sense to have it per-user, however you must always check that everything is the same, user, password, any other option. Reading #533 I think the original concern about counting failures is a little bit misguided. The reason why we have unsuccessful counts is to avoid password guessing where an attacker tries a different password at every attempt. It is not a measure to somehow punish the user for sending the wrong password. So if an "attacker" where to send 100 auth requests all with the same password it makes no sense to lock up the user account, they SHOULD, ideally, count as a single failure. And actually de-duplication may end up saving a user with scripts. Think of a situation where the user has a script that makes 10 actions that require authentication when he inputs his password. If he enters the wrong password he may lock himself out for X time with just one attempt. With de-dup instead if chewed only one attempt and he can try again (assuming all auths happen at the same time). > Given that I would recommend to commit the patches in the current state > and add a RFE ticket to discussion de-duplication for one of the next > releases. Sure, we can open a separate ticket. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 01:35:32PM +0200, Jakub Hrozek wrote:
> On Mon, Jul 06, 2015 at 11:52:45AM +0200, Jakub Hrozek wrote:
> > On Fri, Jul 03, 2015 at 08:29:30PM +0200, Sumit Bose wrote:
> > > On Fri, Jul 03, 2015 at 11:59:53AM +0200, Jakub Hrozek wrote:
> > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote:
> > > > > Hi,
> > > > >
> > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701
> > > > >
> > > > > The first patch just adds a common function instead of copying the
> > > > > same
> > > > > pattern again to the new test.
> > > > >
> > > > > The second adds a new request krb5_auth_queue_send() that wraps
> > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I
> > > > > hope
> > > > > the unit tests cover a lot of use-cases, if not, please suggest more!
> > > > >
> > > > > btw I was thinking that the chaining might not always be necessary if
> > > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't
> > > > > be perceived as performance regression for users. Shall we say that
> > > > > Pavel's cached auth patches are a more systematic solution that
> > > > > doesn't
> > > > > rely on properties of the ccache type in that case?
> > > >
> > > > I'm sorry, but CI fails on Debian because of wrong linking with
> > > > libraries. I'm already testing a fix. Review of the rest is appreciated
> > > > :-)
> > >
> > > ACK to the first patch.
> > >
> > > The second patch has a trailing whitespace
> > >
> > > > +}
> > > > +
> > > > +static void test_krb5_wait_mock(struct test_krb5_wait_queue *test_ctx,
> > > ^
> > > > +const char *username,
> > > > +time_t us_delay,
> > > > +int ret,
> > > > +int pam_status,
> > > > +int dp_err)
> > >
> > > Otherwise the patch looks good and works as expected. But since you
> > > mentioned
> > > that it needs a respin for the Debian issue I would like to ask to add
> > > changes similar to the following:
> > >
> > > diff --git a/src/providers/krb5/krb5_wait_queue.c
> > > b/src/providers/krb5/krb5_wait_queue.c
> > > index d0cc0c1..1262096 100644
> > > --- a/src/providers/krb5/krb5_wait_queue.c
> > > +++ b/src/providers/krb5/krb5_wait_queue.c
> > > @@ -271,17 +271,17 @@ struct tevent_req *krb5_auth_queue_send(TALLOC_CTX
> > > *mem_ctx,
> > > ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx);
> > > if (ret == EOK) {
> > > DEBUG(SSSDBG_TRACE_LIBS,
> > > - "Request successfully added to wait queue "
> > > - "of user [%s].\n", pd->user);
> > > + "Request [%p] successfully added to wait queue "
> > > + "of user [%s].\n", req, pd->user);
> > > ret = EOK;
> > > goto immediate;
> > > } else if (ret == ENOENT) {
> > > DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, "
> > > - "running request immediately.\n", pd->user);
> > > + "running request [%p] immediately.\n", pd->user, req);
> > > } else {
> > > DEBUG(SSSDBG_MINOR_FAILURE,
> > >"Failed to add request to wait queue of user [%s], "
> > > - "running request immediately.\n", pd->user);
> > > + "running request [%p] immediately.\n", pd->user, req);
> > > }
> > >
> > > subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx);
> > > @@ -322,7 +322,7 @@ static void krb5_auth_queue_done(struct tevent_req
> > > *subreq)
> > > return;
> > > }
> > >
> > > -DEBUG(SSSDBG_TRACE_INTERNAL, "krb5_auth_queue done\n");
> > > +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n",
> > > req);
> > > tevent_req_done(req);
> > > }
> > >
> > > @@ -346,6 +346,7 @@ static void krb5_auth_queue_finish(struct tevent_req
> > > *req,
> > > if (ret != EOK) {
> > > tevent_req_error(req, ret);
> > > } else {
> > > +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n",
> > > req);
> > > tevent_req_done(req);
> > > }
> > > }
> > >
> > >
> > > I was a bit irritated when I verified the by following the logs that every
> > > entry to the wait queue was logged but only the exit of the immediate run
> > > was
> > > found in the logs. This is basically the last change.
> > >
> > > The other changes add the address of the request pointer to the log
> > > message to make it easier to find matching enter and exit entries. I
> > > hope this is in agreement with the recent discussion about improving log
> > > messages.
> > >
> > > Finally the log levels are aligned.
> > >
> > > Feel free to drop any changes you don't like but I would appreciate it
> > > if you can at least add a don message in krb5_auth_queue_finish().
> > >
> > > bye,
> > > Sumit
Re: [SSSD] [PATCHES] PAM: authenticate agains cache
On Fri, Jul 03, 2015 at 05:44:27PM +0200, Pavel Reichl wrote: > On 07/02/2015 03:06 PM, Pavel Reichl wrote: > >Hello, > > > >new round of patches is attached. I discussed the required changes with > >Jakub to be sure we are on the same page as this thread is quite > >voluminous. > > > >Basically I just made the setters/getters static and removed tests of this > >functions. I hope that by this change we finally reached the desired level > >of design quality. > > > >Thanks! > > > >PS: Feel free to ignore patch 3. It's just a by-product of this effort. > > > > > >___ > >sssd-devel mailing list > >sssd-devel@lists.fedorahosted.org > >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel > Attached patch set fixes minor code style issues. These patches work for me, but I also tested OTP and it doesn't behave as we agreed on in this thread: https://www.redhat.com/archives/freeipa-users/2015-June/msg00575.html Currently only the first factor was considered during the cached auth period, so anything could be input as the second factor. I propose this patch: https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/commit/?h=review&id=848ccf531afebe54be5d9c120fa90c18b0749cfe We should also amend the manpage if the code is OK. ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 11:52:45AM +0200, Jakub Hrozek wrote:
> On Fri, Jul 03, 2015 at 08:29:30PM +0200, Sumit Bose wrote:
> > On Fri, Jul 03, 2015 at 11:59:53AM +0200, Jakub Hrozek wrote:
> > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701
> > > >
> > > > The first patch just adds a common function instead of copying the same
> > > > pattern again to the new test.
> > > >
> > > > The second adds a new request krb5_auth_queue_send() that wraps
> > > > krb5_auth_send() and also uses the Kerberos authentication queue. I hope
> > > > the unit tests cover a lot of use-cases, if not, please suggest more!
> > > >
> > > > btw I was thinking that the chaining might not always be necessary if
> > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't
> > > > be perceived as performance regression for users. Shall we say that
> > > > Pavel's cached auth patches are a more systematic solution that doesn't
> > > > rely on properties of the ccache type in that case?
> > >
> > > I'm sorry, but CI fails on Debian because of wrong linking with
> > > libraries. I'm already testing a fix. Review of the rest is appreciated
> > > :-)
> >
> > ACK to the first patch.
> >
> > The second patch has a trailing whitespace
> >
> > > +}
> > > +
> > > +static void test_krb5_wait_mock(struct test_krb5_wait_queue *test_ctx,
> > ^
> > > +const char *username,
> > > +time_t us_delay,
> > > +int ret,
> > > +int pam_status,
> > > +int dp_err)
> >
> > Otherwise the patch looks good and works as expected. But since you
> > mentioned
> > that it needs a respin for the Debian issue I would like to ask to add
> > changes similar to the following:
> >
> > diff --git a/src/providers/krb5/krb5_wait_queue.c
> > b/src/providers/krb5/krb5_wait_queue.c
> > index d0cc0c1..1262096 100644
> > --- a/src/providers/krb5/krb5_wait_queue.c
> > +++ b/src/providers/krb5/krb5_wait_queue.c
> > @@ -271,17 +271,17 @@ struct tevent_req *krb5_auth_queue_send(TALLOC_CTX
> > *mem_ctx,
> > ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx);
> > if (ret == EOK) {
> > DEBUG(SSSDBG_TRACE_LIBS,
> > - "Request successfully added to wait queue "
> > - "of user [%s].\n", pd->user);
> > + "Request [%p] successfully added to wait queue "
> > + "of user [%s].\n", req, pd->user);
> > ret = EOK;
> > goto immediate;
> > } else if (ret == ENOENT) {
> > DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, "
> > - "running request immediately.\n", pd->user);
> > + "running request [%p] immediately.\n", pd->user, req);
> > } else {
> > DEBUG(SSSDBG_MINOR_FAILURE,
> >"Failed to add request to wait queue of user [%s], "
> > - "running request immediately.\n", pd->user);
> > + "running request [%p] immediately.\n", pd->user, req);
> > }
> >
> > subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx);
> > @@ -322,7 +322,7 @@ static void krb5_auth_queue_done(struct tevent_req
> > *subreq)
> > return;
> > }
> >
> > -DEBUG(SSSDBG_TRACE_INTERNAL, "krb5_auth_queue done\n");
> > +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", req);
> > tevent_req_done(req);
> > }
> >
> > @@ -346,6 +346,7 @@ static void krb5_auth_queue_finish(struct tevent_req
> > *req,
> > if (ret != EOK) {
> > tevent_req_error(req, ret);
> > } else {
> > +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n",
> > req);
> > tevent_req_done(req);
> > }
> > }
> >
> >
> > I was a bit irritated when I verified the by following the logs that every
> > entry to the wait queue was logged but only the exit of the immediate run
> > was
> > found in the logs. This is basically the last change.
> >
> > The other changes add the address of the request pointer to the log
> > message to make it easier to find matching enter and exit entries. I
> > hope this is in agreement with the recent discussion about improving log
> > messages.
> >
> > Finally the log levels are aligned.
> >
> > Feel free to drop any changes you don't like but I would appreciate it
> > if you can at least add a don message in krb5_auth_queue_finish().
> >
> > bye,
> > Sumit
>
> Thank you for the review, I squashed in your changes in full. It's
> really important to keep DEBUG messages helpful.
>
> I couldn't see the whitespace issue in the patch, maybe I fixed it in
> parallel.
>
> Attached are new patches, CI is running so far, but I've done the same
> change as you showed me on IRC, so I
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 08:29:30PM +0200, Sumit Bose wrote:
> On Fri, Jul 03, 2015 at 11:59:53AM +0200, Jakub Hrozek wrote:
> > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701
> > >
> > > The first patch just adds a common function instead of copying the same
> > > pattern again to the new test.
> > >
> > > The second adds a new request krb5_auth_queue_send() that wraps
> > > krb5_auth_send() and also uses the Kerberos authentication queue. I hope
> > > the unit tests cover a lot of use-cases, if not, please suggest more!
> > >
> > > btw I was thinking that the chaining might not always be necessary if
> > > the ccache is of type MEMORY and I hope that the serializaton wouldn't
> > > be perceived as performance regression for users. Shall we say that
> > > Pavel's cached auth patches are a more systematic solution that doesn't
> > > rely on properties of the ccache type in that case?
> >
> > I'm sorry, but CI fails on Debian because of wrong linking with
> > libraries. I'm already testing a fix. Review of the rest is appreciated
> > :-)
>
> ACK to the first patch.
>
> The second patch has a trailing whitespace
>
> > +}
> > +
> > +static void test_krb5_wait_mock(struct test_krb5_wait_queue *test_ctx,
> ^
> > +const char *username,
> > +time_t us_delay,
> > +int ret,
> > +int pam_status,
> > +int dp_err)
>
> Otherwise the patch looks good and works as expected. But since you mentioned
> that it needs a respin for the Debian issue I would like to ask to add
> changes similar to the following:
>
> diff --git a/src/providers/krb5/krb5_wait_queue.c
> b/src/providers/krb5/krb5_wait_queue.c
> index d0cc0c1..1262096 100644
> --- a/src/providers/krb5/krb5_wait_queue.c
> +++ b/src/providers/krb5/krb5_wait_queue.c
> @@ -271,17 +271,17 @@ struct tevent_req *krb5_auth_queue_send(TALLOC_CTX
> *mem_ctx,
> ret = add_to_wait_queue(be_ctx, req, pd, krb5_ctx);
> if (ret == EOK) {
> DEBUG(SSSDBG_TRACE_LIBS,
> - "Request successfully added to wait queue "
> - "of user [%s].\n", pd->user);
> + "Request [%p] successfully added to wait queue "
> + "of user [%s].\n", req, pd->user);
> ret = EOK;
> goto immediate;
> } else if (ret == ENOENT) {
> DEBUG(SSSDBG_TRACE_LIBS, "Wait queue of user [%s] is empty, "
> - "running request immediately.\n", pd->user);
> + "running request [%p] immediately.\n", pd->user, req);
> } else {
> DEBUG(SSSDBG_MINOR_FAILURE,
>"Failed to add request to wait queue of user [%s], "
> - "running request immediately.\n", pd->user);
> + "running request [%p] immediately.\n", pd->user, req);
> }
>
> subreq = krb5_auth_send(req, ev, be_ctx, pd, krb5_ctx);
> @@ -322,7 +322,7 @@ static void krb5_auth_queue_done(struct tevent_req
> *subreq)
> return;
> }
>
> -DEBUG(SSSDBG_TRACE_INTERNAL, "krb5_auth_queue done\n");
> +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n", req);
> tevent_req_done(req);
> }
>
> @@ -346,6 +346,7 @@ static void krb5_auth_queue_finish(struct tevent_req *req,
> if (ret != EOK) {
> tevent_req_error(req, ret);
> } else {
> +DEBUG(SSSDBG_TRACE_LIBS, "krb5_auth_queue request [%p] done.\n",
> req);
> tevent_req_done(req);
> }
> }
>
>
> I was a bit irritated when I verified the by following the logs that every
> entry to the wait queue was logged but only the exit of the immediate run was
> found in the logs. This is basically the last change.
>
> The other changes add the address of the request pointer to the log
> message to make it easier to find matching enter and exit entries. I
> hope this is in agreement with the recent discussion about improving log
> messages.
>
> Finally the log levels are aligned.
>
> Feel free to drop any changes you don't like but I would appreciate it
> if you can at least add a don message in krb5_auth_queue_finish().
>
> bye,
> Sumit
Thank you for the review, I squashed in your changes in full. It's
really important to keep DEBUG messages helpful.
I couldn't see the whitespace issue in the patch, maybe I fixed it in
parallel.
Attached are new patches, CI is running so far, but I've done the same
change as you showed me on IRC, so I'm confident it would help.
>From 4706bef89f951e3f9ca686b0b24297df12500c56 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek
Date: Fri, 3 Jul 2015 11:15:32 +0200
Subject: [PATCH 1/2] tests: Reduce duplication with new function test_ev_done
---
src/tests/cmocka/test_ipa_subdomains_server.c | 11 ++-
src/
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: > > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: > > > > > Hi, > > > > > > > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701 > > > > > > > > > > The first patch just adds a common function instead of copying the > > > > > same > > > > > pattern again to the new test. > > > > > > > > > > The second adds a new request krb5_auth_queue_send() that wraps > > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I > > > > > hope > > > > > the unit tests cover a lot of use-cases, if not, please suggest more! > > > > > > > > > > btw I was thinking that the chaining might not always be necessary if > > > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't > > > > > be perceived as performance regression for users. Shall we say that > > > > > Pavel's cached auth patches are a more systematic solution that > > > > > doesn't > > > > > rely on properties of the ccache type in that case? > > > > > > > > I'm sorry, but CI fails on Debian because of wrong linking with > > > > libraries. I'm already testing a fix. Review of the rest is appreciated > > > > :-) > > > > > > If we are serializing all authentications then a busy server would be > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > NACK the approach sorry. > > > > The current cache queue is per user, see add_to_wait_queue() for > > details. > > I actually did look to check that and failed, I blame the late hour :-) > > Per-user is fine by me, though I would seriously consider de-duplication > while we are here. I think this might be a useful feature, although I think it should not be on by default. We have discussed this already a bit 5 years ago together with the initial implementation of the auth queues in the krb5 responder. Here are some links for reference: https://fedorahosted.org/sssd/ticket/533 https://lists.fedorahosted.org/pipermail/sssd-devel/2010-October/004821.html https://lists.fedorahosted.org/pipermail/sssd-devel/2010-December/005307.html But I think this feature would need some more discussion. E.g. where should it be implemented, in the PAM responder on in the auth backends? How does is related to the cached authentication feature? How should failed authentication be handled, de-dup as well or send them to the backend to allow the server to update the failed logon counter properly? Shall de-duplication be done by user or by user and by service? Given that I would recommend to commit the patches in the current state and add a RFE ticket to discussion de-duplication for one of the next releases. bye, Sumit > > Simo. > > -- > Simo Sorce * Red Hat, Inc * New York > > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Mon, Jul 06, 2015 at 09:24:50AM +0200, Jakub Hrozek wrote: > On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > > On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: > > > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: > > > > > > Hi, > > > > > > > > > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701 > > > > > > > > > > > > The first patch just adds a common function instead of copying the > > > > > > same > > > > > > pattern again to the new test. > > > > > > > > > > > > The second adds a new request krb5_auth_queue_send() that wraps > > > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I > > > > > > hope > > > > > > the unit tests cover a lot of use-cases, if not, please suggest > > > > > > more! > > > > > > > > > > > > btw I was thinking that the chaining might not always be necessary > > > > > > if > > > > > > the ccache is of type MEMORY and I hope that the serializaton > > > > > > wouldn't > > > > > > be perceived as performance regression for users. Shall we say that > > > > > > Pavel's cached auth patches are a more systematic solution that > > > > > > doesn't > > > > > > rely on properties of the ccache type in that case? > > > > > > > > > > I'm sorry, but CI fails on Debian because of wrong linking with > > > > > libraries. I'm already testing a fix. Review of the rest is > > > > > appreciated > > > > > :-) > > > > > > > > If we are serializing all authentications then a busy server would be > > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > > NACK the approach sorry. > > > > > > The current cache queue is per user, see add_to_wait_queue() for > > > details. > > > > I actually did look to check that and failed, I blame the late hour :-) > > Ah, I thought that goes without saying it's not a responder-global queue ~~~ backend-global, sorry > :-) > > > > > Per-user is fine by me, though I would seriously consider de-duplication > > while we are here. > > What exactly do you propose here? > ___ > sssd-devel mailing list > sssd-devel@lists.fedorahosted.org > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Re: [SSSD] [PATCH] Chain authentication requests in all Kerberos-based providers
On Fri, Jul 03, 2015 at 05:01:13PM -0400, Simo Sorce wrote: > On Fri, 2015-07-03 at 21:34 +0200, Sumit Bose wrote: > > On Fri, Jul 03, 2015 at 02:12:34PM -0400, Simo Sorce wrote: > > > On Fri, 2015-07-03 at 11:59 +0200, Jakub Hrozek wrote: > > > > On Fri, Jul 03, 2015 at 11:54:46AM +0200, Jakub Hrozek wrote: > > > > > Hi, > > > > > > > > > > the attached patches fix https://fedorahosted.org/sssd/ticket/2701 > > > > > > > > > > The first patch just adds a common function instead of copying the > > > > > same > > > > > pattern again to the new test. > > > > > > > > > > The second adds a new request krb5_auth_queue_send() that wraps > > > > > krb5_auth_send() and also uses the Kerberos authentication queue. I > > > > > hope > > > > > the unit tests cover a lot of use-cases, if not, please suggest more! > > > > > > > > > > btw I was thinking that the chaining might not always be necessary if > > > > > the ccache is of type MEMORY and I hope that the serializaton wouldn't > > > > > be perceived as performance regression for users. Shall we say that > > > > > Pavel's cached auth patches are a more systematic solution that > > > > > doesn't > > > > > rely on properties of the ccache type in that case? > > > > > > > > I'm sorry, but CI fails on Debian because of wrong linking with > > > > libraries. I'm already testing a fix. Review of the rest is appreciated > > > > :-) > > > > > > If we are serializing all authentications then a busy server would be > > > swamped. I do not see a per-user/per-cache queue so I would tentatively > > > NACK the approach sorry. > > > > The current cache queue is per user, see add_to_wait_queue() for > > details. > > I actually did look to check that and failed, I blame the late hour :-) Ah, I thought that goes without saying it's not a responder-global queue :-) > > Per-user is fine by me, though I would seriously consider de-duplication > while we are here. What exactly do you propose here? ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
[SSSD] RFC: 1.13.0 release notes
Hi, we need to release 1.13.0 to satisfy a downstream deadline. I've amended the 1.13 Alpha release notes with the features that will be present in 1.13.0 (cached auth, memcache for initgroups): https://fedorahosted.org/sssd/wiki/Releases/Notes-1.13.0 ___ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
