On Fri, Feb 20, 2015 at 02:44:45PM +0100, Pavel Reichl wrote:
On 02/20/2015 02:33 PM, Lukas Slebodnik wrote:
>On (20/02/15 14:23), Pavel Reichl wrote:
>>On 02/19/2015 06:16 PM, Sumit Bose wrote:
>>>On Tue, Feb 17, 2015 at 10:56:43PM +0100, Pavel Reichl wrote:
>>>>Hello,
>>>>
>>>>please see attached patches resolving
>>>>https://fedorahosted.org/sssd/ticket/2167
>>>>
>>>>1st patch contains some minor refactoring useful for 2nd patch.
>>>>
>>>>How to test:
>>>>On OpenLDAP following attributes should be set:
>>>> * shadowMax - how many days after shadowLastChange will password
expire
>>>> * shadowWarning - how many days before password expires should warning
be
>>>>displayed.
>>>>
>>>>In sssd.conf domain section should contain something similar to:
>>>>
>>>>access_provider = ldap
>>>>ldap_access_order = expire, expire_policy
>>>>ldap_account_expire_policy = shadow
>>>>ldap_pwd_policy = shadow
>>>>
>>>>Thanks!
>>>Hi Pavel,
>>>
>>>I've tested the patch with FreeIPA and ldap_pwd_policy = mit_kerberos
>>>and it is working as expected. Nevertheless I have a few comments.
>>>
>>>First, I would recommend to rename the option 'expire_policy' is
>>>ambiguous because it is not clear which expiration is meant, account or
>>>password. Since it is tightly related to ldap_pwd_policy I would prefer
>>>'pwd_policy' or 'pwd_expire_policy'
('ldap_pwd_policy' might not be a
>>>good choice be because then we have an option name and an option value
>>>with the same name which might be confusing).
>>done
>>>My main comment is about moving the check if configured. Although I see
>>>the point of doing the check only once I think it should be done in the
>>>auth and access provider. Since it is run from the cache there shouldn't
>>>be any great delay for the user. This will make your patch easier and
>>>the check is still done for applications only calling pam_authenticate.
>>I did it to avoid duplicate output for services like ssh when using
>>passwords. I should have consult that.
>>>In the man page you should explicitly mention that ldap_pwd_policy must
>>>be set to make the new option work, because the default for
>>>ldap_pwd_policy is 'none'.
>>dobe
>>>bye,
>>>Sumit
>>Thanks for comments and testing. Please see updated patchset (only second
>>patch differs).
>>
>>From 8551fd21aa91be1f9632d08c86a30d433a104db0 Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl <preichl(a)redhat.com>
>>Date: Mon, 16 Feb 2015 18:56:25 -0500
>>Subject: [PATCH 1/2] SDAP: refactor pwexpire policy
>>
>>Move part of pwexpire policy code to a separate function.
>>
>>Relates to:
>>https://fedorahosted.org/sssd/ticket/2167
>>---
>>src/providers/ldap/ldap_auth.c | 76 ++++++++++++++++++++++++------------------
>>src/providers/ldap/ldap_auth.h | 46 +++++++++++++++++++++++++
>>2 files changed, 90 insertions(+), 32 deletions(-)
>>create mode 100644 src/providers/ldap/ldap_auth.h
>>
>>diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
>>index
5a40c1359f138c42eb915e873fe21a50ab038e81..1f1add53713201c83a13928ebb66f6bd130d7bac 100644
>>--- a/src/providers/ldap/ldap_auth.c
>>+++ b/src/providers/ldap/ldap_auth.c
>>@@ -46,16 +46,10 @@
>>#include "providers/ldap/ldap_common.h"
>>#include "providers/ldap/sdap_async.h"
>>#include "providers/ldap/sdap_async_private.h"
>>+#include "providers/ldap/ldap_auth.h"
>>
>>#define LDAP_PWEXPIRE_WARNING_TIME 0
>>
>>-enum pwexpire {
>>- PWEXPIRE_NONE = 0,
>>- PWEXPIRE_LDAP_PASSWORD_POLICY,
>>- PWEXPIRE_KERBEROS,
>>- PWEXPIRE_SHADOW
>>-};
>>-
>>static errno_t add_expired_warning(struct pam_data *pd, long exp_time)
>>{
>> int ret;
>>@@ -248,10 +242,41 @@ done:
>> return ret;
>>}
>>
>>-static errno_t find_password_expiration_attributes(TALLOC_CTX *mem_ctx,
>>- const struct ldb_message *msg,
>>- struct dp_option *opts,
>>- enum pwexpire *type, void
**data)
>>+errno_t check_pwexpire_policy(enum pwexpire pw_expire_type,
>>+ void *pw_expire_data,
>>+ struct pam_data *pd,
>>+ int pwd_expiration_warning)
>>+{
>>+ errno_t ret;
>>+
>>+ switch (pw_expire_type) {
>>+ case PWEXPIRE_SHADOW:
>>+ ret = check_pwexpire_shadow(pw_expire_data, time(NULL), pd);
>>+ break;
>>+ case PWEXPIRE_KERBEROS:
>>+ ret = check_pwexpire_kerberos(pw_expire_data, time(NULL), pd,
>>+ pwd_expiration_warning);
>>+ break;
>>+ case PWEXPIRE_LDAP_PASSWORD_POLICY:
>>+ ret = check_pwexpire_ldap(pd, pw_expire_data,
>>+ pwd_expiration_warning);
>>+ break;
>>+ case PWEXPIRE_NONE:
>>+ ret = EOK;
>>+ break;
>>+ default:
>>+ DEBUG(SSSDBG_CRIT_FAILURE, "Unknown password expiration
type.\n");
>>+ ret = -1;
>>+ }
>>+
>>+ return ret;
>>+}
>>+
>>+static errno_t
>>+find_password_expiration_attributes(TALLOC_CTX *mem_ctx,
>>+ const struct ldb_message *msg,
>>+ struct dp_option *opts,
>>+ enum pwexpire *type, void **data)
>>{
>> const char *mark;
>> const char *val;
>>@@ -492,7 +517,7 @@ static int get_user_dn_recv(TALLOC_CTX *mem_ctx, struct
tevent_req *req,
>> return EOK;
>>}
>>
>>-static int get_user_dn(TALLOC_CTX *memctx,
>>+int get_user_dn(TALLOC_CTX *memctx,
>> struct sss_domain_info *domain,
>> struct sdap_options *opts,
>> const char *username,
>>@@ -998,7 +1023,7 @@ static void sdap_auth4chpass_done(struct tevent_req *req)
>> case PWEXPIRE_NONE:
>> break;
>> default:
>>- DEBUG(SSSDBG_CRIT_FAILURE, "Unknow pasword expiration
type.\n");
>>+ DEBUG(SSSDBG_CRIT_FAILURE, "Unknown password expiration
type.\n");
>> state->pd->pam_status = PAM_SYSTEM_ERR;
>> goto done;
>> }
>>@@ -1247,25 +1272,12 @@ static void sdap_pam_auth_done(struct tevent_req *req)
>> talloc_zfree(req);
>>
>> if (ret == EOK) {
>>- switch (pw_expire_type) {
>>- case PWEXPIRE_SHADOW:
>>- ret = check_pwexpire_shadow(pw_expire_data, time(NULL),
state->pd);
>>- break;
>>- case PWEXPIRE_KERBEROS:
>>- ret = check_pwexpire_kerberos(pw_expire_data, time(NULL),
>>- state->pd,
>>-
be_ctx->domain->pwd_expiration_warning);
>>- break;
>>- case PWEXPIRE_LDAP_PASSWORD_POLICY:
>>- ret = check_pwexpire_ldap(state->pd, pw_expire_data,
>>-
be_ctx->domain->pwd_expiration_warning);
>>- break;
>>- case PWEXPIRE_NONE:
>>- break;
>>- default:
>>- DEBUG(SSSDBG_CRIT_FAILURE, "Unknow pasword expiration
type.\n");
>>- state->pd->pam_status = PAM_SYSTEM_ERR;
>>- goto done;
>>+ ret = check_pwexpire_policy(pw_expire_type, pw_expire_data,
state->pd,
>>+
be_ctx->domain->pwd_expiration_warning);
>>+ if (ret == -1) {
>>+ /* Unknown password expiration type. */
>>+ state->pd->pam_status = PAM_SYSTEM_ERR;
>>+ goto done;
>> }
>> }
>>
>>diff --git a/src/providers/ldap/ldap_auth.h b/src/providers/ldap/ldap_auth.h
>>new file mode 100644
>>index
0000000000000000000000000000000000000000..1038ddc7d136db925f2e9416c848e4505c45f4cf
>>--- /dev/null
>>+++ b/src/providers/ldap/ldap_auth.h
>>@@ -0,0 +1,46 @@
>>+/*
>>+ SSSD
>>+
>>+ Copyright (C) Pavel Reichl <preichl(a)redhat.com>
>>+
>>+ This program is free software; you can redistribute it and/or modify
>>+ it under the terms of the GNU General Public License as published by
>>+ the Free Software Foundation; either version 3 of the License, or
>>+ (at your option) any later version.
>>+
>>+ This program is distributed in the hope that it will be useful,
>>+ but WITHOUT ANY WARRANTY; without even the implied warranty of
>>+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>>+ GNU General Public License for more details.
>>+
>>+ You should have received a copy of the GNU General Public License
>>+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
>>+*/
>>+
>>+#ifndef _LDAP_AUTH_H_
>>+#define _LDAP_AUTH_H_
>>+
>>+#include "config.h"
>>+
>>+enum pwexpire {
>>+ PWEXPIRE_NONE = 0,
>>+ PWEXPIRE_LDAP_PASSWORD_POLICY,
>>+ PWEXPIRE_KERBEROS,
>>+ PWEXPIRE_SHADOW
>>+};
>>+
>>+int get_user_dn(TALLOC_CTX *memctx,
>>+ struct sss_domain_info *domain,
>>+ struct sdap_options *opts,
>>+ const char *username,
>>+ char **user_dn,
>>+ enum pwexpire *user_pw_expire_type,
>>+ void **user_pw_expire_data);
>>+
>>+errno_t check_pwexpire_policy(enum pwexpire pw_expire_type,
>>+ void *pw_expire_data,
>>+ struct pam_data *pd,
>>+ errno_t checkb);
>>+
>>+
>>+#endif /* _LDAP_AUTH_H_ */
>>--
>>2.1.0
>>
>>From add61e4819be0c013da2c452cc86d0d98d803440 Mon Sep 17 00:00:00 2001
>>From: Pavel Reichl <preichl(a)redhat.com>
>>Date: Wed, 18 Feb 2015 01:03:40 -0500
>>Subject: [PATCH 2/2] SDAP: enable change phase of pw expire policy check
>>
>>Implement new option which does checking password expiration policy
>>in accounting phase.
>>
>>This allows SSSD to issue shadow expiration warning even if alternate
>>authentication method is used.
>>
>>Resolves:
>>https://fedorahosted.org/sssd/ticket/2167
>>---
>>Makefile.am | 1 +
>>src/man/sssd-ldap.5.xml | 12 +++++++++++
>>src/providers/ldap/ldap_auth.c | 1 +
>>src/providers/ldap/ldap_init.c | 3 +++
>>src/providers/ldap/sdap_access.c | 43 +++++++++++++++++++++++++++++++++++++++-
>>src/providers/ldap/sdap_access.h | 2 ++
>>6 files changed, 61 insertions(+), 1 deletion(-)
>>
>>diff --git a/Makefile.am b/Makefile.am
>>index
70278550fd22b820528f9613d30e11a723ea6c5c..35e3b39027f1163078a61a0528df2306367d6187 100644
>>--- a/Makefile.am
>>+++ b/Makefile.am
>>@@ -562,6 +562,7 @@ dist_noinst_HEADERS = \
>> src/providers/ldap/sdap_autofs.h \
>> src/providers/ldap/sdap_id_op.h \
>> src/providers/ldap/ldap_opts.h \
>>+ src/providers/ldap/ldap_auth.h \
>> src/providers/ldap/sdap_range.h \
>> src/providers/ldap/sdap_users.h \
>> src/providers/ldap/sdap_dyndns.h \
>This change should be part of 1st patch otherwise our CI will complain that
>make distcheck fails after applying 1st patch.
>
>LS
>_______________________________________________
>sssd-devel mailing list
>sssd-devel(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Thanks Lukas, that wasn't very smart from me.
Updated patch attached.
Thank you, please find comments in-line:
From ac6de4dd1410d3aa91378afc145dfe915b47292b Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Mon, 16 Feb 2015 18:56:25 -0500
Subject: [PATCH 1/2] SDAP: refactor pwexpire policy
Move part of pwexpire policy code to a separate function.
Relates to:
https://fedorahosted.org/sssd/ticket/2167
---
Makefile.am | 1 +
src/providers/ldap/ldap_auth.c | 76 ++++++++++++++++++++++++------------------
src/providers/ldap/ldap_auth.h | 46 +++++++++++++++++++++++++
3 files changed, 91 insertions(+), 32 deletions(-)
create mode 100644 src/providers/ldap/ldap_auth.h
...
+errno_t check_pwexpire_policy(enum pwexpire pw_expire_type,
+ void *pw_expire_data,
+ struct pam_data *pd,
+ int pwd_expiration_warning)
+{
+ errno_t ret;
+
+ switch (pw_expire_type) {
+ case PWEXPIRE_SHADOW:
+ ret = check_pwexpire_shadow(pw_expire_data, time(NULL), pd);
+ break;
+ case PWEXPIRE_KERBEROS:
+ ret = check_pwexpire_kerberos(pw_expire_data, time(NULL), pd,
+ pwd_expiration_warning);
+ break;
+ case PWEXPIRE_LDAP_PASSWORD_POLICY:
+ ret = check_pwexpire_ldap(pd, pw_expire_data,
+ pwd_expiration_warning);
+ break;
+ case PWEXPIRE_NONE:
+ ret = EOK;
+ break;
+ default:
+ DEBUG(SSSDBG_CRIT_FAILURE, "Unknown password expiration type.\n");
+ ret = -1;
We rarely return -1 as error code. I think EINVAL would be more
suitable or if you want a specific error code for the unknown type you
can add an SSSD internal error code..
+ }
+
+ return ret;
+}
+
...
@@ -1247,25 +1272,12 @@ static void sdap_pam_auth_done(struct
tevent_req *req)
talloc_zfree(req);
if (ret == EOK) {
- switch (pw_expire_type) {
- case PWEXPIRE_SHADOW:
- ret = check_pwexpire_shadow(pw_expire_data, time(NULL), state->pd);
- break;
- case PWEXPIRE_KERBEROS:
- ret = check_pwexpire_kerberos(pw_expire_data, time(NULL),
- state->pd,
-
be_ctx->domain->pwd_expiration_warning);
- break;
- case PWEXPIRE_LDAP_PASSWORD_POLICY:
- ret = check_pwexpire_ldap(state->pd, pw_expire_data,
- be_ctx->domain->pwd_expiration_warning);
- break;
- case PWEXPIRE_NONE:
- break;
- default:
- DEBUG(SSSDBG_CRIT_FAILURE, "Unknow pasword expiration type.\n");
- state->pd->pam_status = PAM_SYSTEM_ERR;
- goto done;
+ ret = check_pwexpire_policy(pw_expire_type, pw_expire_data, state->pd,
+ be_ctx->domain->pwd_expiration_warning);
+ if (ret == -1) {
+ /* Unknown password expiration type. */
+ state->pd->pam_status = PAM_SYSTEM_ERR;
+ goto done;
see above, a SSSD internal error code might be better here.
}
}
diff --git a/src/providers/ldap/ldap_auth.h b/src/providers/ldap/ldap_auth.h
new file mode 100644
index 0000000000000000000000000000000000000000..1038ddc7d136db925f2e9416c848e4505c45f4cf
--- /dev/null
+++ b/src/providers/ldap/ldap_auth.h
@@ -0,0 +1,46 @@
+/*
+ SSSD
+
+ Copyright (C) Pavel Reichl <preichl(a)redhat.com>
I'm not sure how important it is but typically we add the year here as
well, see COPYING file.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 3 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program. If not, see <
http://www.gnu.org/licenses/>.
+*/
+
+#ifndef _LDAP_AUTH_H_
+#define _LDAP_AUTH_H_
+
+#include "config.h"
+
+enum pwexpire {
+ PWEXPIRE_NONE = 0,
+ PWEXPIRE_LDAP_PASSWORD_POLICY,
+ PWEXPIRE_KERBEROS,
+ PWEXPIRE_SHADOW
+};
+
+int get_user_dn(TALLOC_CTX *memctx,
+ struct sss_domain_info *domain,
+ struct sdap_options *opts,
+ const char *username,
+ char **user_dn,
+ enum pwexpire *user_pw_expire_type,
+ void **user_pw_expire_data);
+
+errno_t check_pwexpire_policy(enum pwexpire pw_expire_type,
+ void *pw_expire_data,
+ struct pam_data *pd,
+ errno_t checkb);
+
+
+#endif /* _LDAP_AUTH_H_ */
--
2.1.0
From 9a561a5fb61d6c7ce44d4081c2f50bb1be4ea0c3 Mon Sep 17 00:00:00
2001
From: Pavel Reichl <preichl(a)redhat.com>
Date: Wed, 18 Feb 2015 01:03:40 -0500
Subject: [PATCH 2/2] SDAP: enable change phase of pw expire policy check
Implement new option which does checking password expiration policy
in accounting phase.
This allows SSSD to issue shadow expiration warning even if alternate
authentication method is used.
Resolves:
https://fedorahosted.org/sssd/ticket/2167
---
src/man/sssd-ldap.5.xml | 12 +++++++++++
src/providers/ldap/ldap_auth.c | 1 +
src/providers/ldap/ldap_init.c | 3 +++
src/providers/ldap/sdap_access.c | 43 +++++++++++++++++++++++++++++++++++++++-
src/providers/ldap/sdap_access.h | 2 ++
5 files changed, 60 insertions(+), 1 deletion(-)
diff --git a/src/man/sssd-ldap.5.xml b/src/man/sssd-ldap.5.xml
index 9f2e9ac34add13e40d316374094024afdcc4ae31..4e3f3510250b19b5f397125fa3e3a376e0d3701f
100644
--- a/src/man/sssd-ldap.5.xml
+++ b/src/man/sssd-ldap.5.xml
@@ -1959,6 +1959,18 @@ ldap_access_filter = (employeeType=admin)
ldap_account_expire_policy
</para>
<para>
+ <emphasis>pwd_expire_policy</emphasis>:
+ This option is useful if users are interested in
+ seeing password expiration warning when authenticating
+ using different method then passwords - for example
+ SSH keys.
It's not about seeing a warning but about denying access based on an
expired password. I think you should be more clear here.
+ </para>
+ <para>
+ Please note that 'access_provider = ldap' must
+ be set for this feature to work. Also
'ldap_pwd_policy'
+ must be set to appropriate password policy.
+ </para>
+ <para>
<emphasis>authorized_service</emphasis>: use
the authorizedService attribute to determine
access
...
+static errno_t perform_pwexpire_policy(TALLOC_CTX *mem_ctx,
+ struct sss_domain_info *domain,
+ struct pam_data *pd,
+ struct sdap_options *opts)
+{
+ enum pwexpire pw_expire_type;
+ void *pw_expire_data;
+ errno_t ret;
+ char *dn;
+
+ ret = get_user_dn(mem_ctx, domain, opts, pd->user, &dn, &pw_expire_type,
+ &pw_expire_data);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "get_user_dn returned %d:[%s]\n.",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+ ret = check_pwexpire_policy(pw_expire_type, pw_expire_data, pd,
+ domain->pwd_expiration_warning);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ "check_pwexpire_policy returned %d:[%s]\n.",
typo, '.' after \n
+ ret, sss_strerror(ret));
+ goto done;
+ }
+
+done:
+ return ret;
I think you have to modify the return code here to match the access
control expectations. check_pwexpire_policy() will e.g. return
ERR_PASSWORD_EXPIRED but the access control code expects
ERR_ACCESS_DENIED. As a result I see the following in the logs:
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [find_password_expiration_attributes]
(0x4000): Found Kerberos password expiration attributes.
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [check_pwexpire_kerberos] (0x4000): Time
info: tzname[0] [CET] tzname[1] [CEST] timezone [-3600] daylight [1] now [1424685607]
expire_time [1
421771276].
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [check_pwexpire_kerberos] (0x0100):
Kerberos password expired.
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [perform_pwexpire_policy] (0x0080):
check_pwexpire_policy returned 1432158224:[Password Expired]
.(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [sdap_access_done] (0x0020): Error
retrieving access check result.
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [be_pam_handler_callback] (0x0100):
Backend returned: (3, 4, <NULL>) [Internal Error (Systemfehler)]
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [be_pam_handler_callback] (0x0100):
Sending result [4][ipa.devel]
(Mon Feb 23 11:00:07 2015) [sssd[be[ipa.devel]]] [be_pam_handler_callback] (0x0100): Sent
result [4][ipa.devel]
(Mon Feb 23 11:00:13 2015) [sssd[be[ipa.devel]]] [sbus_dispatch] (0x4000): dbus conn:
0xb830ff80
(Mon Feb 23 11:00:13 2015) [sssd[be[ipa.devel]]] [sbus_dispatch] (0x4000): Dispatching.
So, access is denied, but with a generic system error.
bye,
Sumit