[SSSD] [PATCHES] SDAP: Be able to configure sssd to honor openldap account lock to restrict access via ssh key
Jakub Hrozek
jhrozek at redhat.com
Mon Aug 11 14:41:39 UTC 2014
On Mon, Aug 11, 2014 at 04:19:22PM +0200, Pavel Reichl wrote:
> Thank you Jakub for comments I have replied on some inline others are
> obvious or I'll reply later.
>
> I was wondering If you or Pavel would mind If I broke the patch set into 2
> separate sets and posted them in separate threads. Changes in first patches
> makes me to manually edit rebase conflicts in later patches and this is
> getting a bit tiring.
no problem here.
>
>
> On 08/10/2014 06:25 PM, Jakub Hrozek wrote:
> >On Wed, Aug 06, 2014 at 06:36:02PM +0200, Pavel Reichl wrote:
> >>On 08/04/2014 02:37 PM, Pavel Březina wrote:
> >>>On 08/01/2014 07:48 PM, Pavel Reichl wrote:
> >>>>Hello,
> >>>>
> >>>>please see attached patches. Every patch is documented in its commit
> >>>>message.
> >>>>
> >>>>Generally speaking the first 6 patches are preparation for patch #8.
> >>>>
> >>>>Thanks,
> >>>>
> >>>>Pavel Reichl
> >>>>
> >>>>PS: I also attached output of my testing to make it more obvious how the
> >>>>patches are supposed to work.
> >>>>
> >>>>------------------------------------------------------------------------
> >>>># john, people, example.com
> >>>>dn: uid=john,ou=people,dc=example,dc=com
> >>>>pwdAccountLockedTime: 000001010000Z
> >>>>
> >>>># max, people, example.com
> >>>>dn: uid=max,ou=people,dc=example,dc=com
> >>>>
> >>>># dick, people, example.com
> >>>>dn: uid=dick,ou=people,dc=example,dc=com
> >>>>pwdAccountLockedTime: 20140801115742Z
> >>>>--------------------------------------------------------------------------
> >>>>
> >>>>$ ssh -l john at openldap `hostname`
> >>>>Connection closed by UNKNOWN
> >>>>
> >>>>$ ssh -l max at openldap `hostname`
> >>>>Last login: Fri Aug 1 15:16:21 2014 from sssd.dev.work
> >>>>
> >>>>$ ssh -l dick at openldap `hostname`
> >>>>Last login: Fri Aug 1 12:57:33 2014
> >>>Patch #1:
> >>>Tentative ack, although you don't have to use tmp_ctx in
> >>>sdap_save_user_cache_bool(). You can just allocate attrs on NULL since it
> >>>is the only talloc context you use there.
> >>>
> >>>Patch #2:
> >>>Nack. I see it is safe here but please avoid initializing ret to EOK. Set
> >>>it to EOK at the end of the function instead.
> >>>
> >>>Patch #3:
> >>>Ack.
> >>>
> >>>Patch #4:
> >>>Typo in commit message "Also remove *sdap_access_filter_recv()* as it is
> >>>replaced by *sdap_access_filter_recv()*."
> >>>
> >>>But it's nack from me anyway.
> >>>
> >>>Tevent naming convention seems to be broken all around this code and this
> >>>patch actually makes it a little bit better. But you shouldn't use
> >>>function named "sdap_access_recv" inside "sdap_access_done".
> >>>
> >>>I see that sdap_access_send can invoke two different requests depending on
> >>>the configuration. Ideally both should have custom _recv and you should
> >>>use some kind of switch to determine which one should be called.
> >>>
> >>>I would be willing to tentatively ack a patch with let's say
> >>>"sdap_access_check_recv" or "sdap_access_subreq_recv" or similar used in
> >>>sdap_access_done.
> >>>
> >>>Patch #5:
> >>>Ack.
> >>>
> >>>Patch #6:
> >>>Ack.
> >>>
> >>>Patch #7:
> >>>Ack. But please swap this ticket with #8. The feature should be first
> >>>implemented then documented.
> >>>
> >>>Patch #8:
> >>>I believe the following comment is copy & paste? It shouldn't say filter
> >>>request :)
> >>>+ /* Connection to LDAP succeeded
> >>>+ * Send filter request
> >>>+ */
> >>>
> >>>+ if (strcasecmp(pwdAccountLockedTime,
> >>>+ PERMANENTLY_LOCKED_ACCOUNT) == 0) {
> >>>+ locked = true;
> >>>+ } else {
> >>>+ DEBUG(SSSDBG_TRACE_FUNC,
> >>>+ "Account of: %s is beeing blocked by password
> >>>policy, "
> >>>+ "but value: [%s] value is ignored by SSSD.\n",
> >>>+ state->username, pwdAccountLockedTime);
> >>>+ }
> >>>
> >>>Why do we ignore the value? Isn't the value a time? Shouldn't we check if
> >>>lock_time < machine_time?
> >>Yes, it's time, but we care only if its the special value '000001010000Z'
> >>otherwise we grant the access.
> >>>Also the ticket says that this policy is only enabled if pwdLockout is
> >>>true. Is it possible to acquire the attribute if pwdLockout is false? I
> >>>see you don't check it.
> >>Yes it's possible to acquire, I have updated the patches.
> >>>Those are questions that should be answered in the code if they are not
> >>>bugs.
> >>>
> >>>I haven't test the code yet - will it work also with 389ds or do you have
> >>>an OpenLDAP server I could use handy?
> >>>
> >>I'll try to grant you access to my VM as I find configuring ppolicy as very
> >>painful experience.
> >>>_______________________________________________
> >>>sssd-devel mailing list
> >>>sssd-devel at lists.fedorahosted.org
> >>>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> >>I hope I have addressed all you concerns and didn't produce many new bugs.
> >>:-)
> >>
> >>
> >Mostly looks OK to me (although I just quickly read the patches..), some
> >questions inline.
> >
> >> From 96bb7442a9b9fc3f21e772e565fcf758b55c2dc9 Mon Sep 17 00:00:00 2001
> >>From: Pavel Reichl <preichl at redhat.com>
> >>Date: Fri, 1 Aug 2014 11:00:48 +0100
> >>Subject: [PATCH 2/9] SDAP: refactor sdap_access_filter_send
> >>
> >>As preparation for ticket #2364 separate code for parsing user basedn
> >>to a new function sdap_get_basedn_user_entry().
> >>---
> >> src/providers/ldap/sdap_access.c | 52 ++++++++++++++++++++++++++++------------
> >> 1 file changed, 37 insertions(+), 15 deletions(-)
> >>
> >>diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> >>index 0702c565a466ba067350595233f9746443d0864d..ddf623616f662d92fd751ff7952f08527c60be45 100644
> >>--- a/src/providers/ldap/sdap_access.c
> >>+++ b/src/providers/ldap/sdap_access.c
> >>@@ -45,6 +45,11 @@ static errno_t sdap_save_user_cache_bool(struct sss_domain_info *domain,
> >> const char *attr_name,
> >> bool value);
> >>+static errno_t sdap_get_basedn_user_entry(TALLOC_CTX *mem_ctx,
> >>+ struct ldb_message *user_entry,
> >>+ const char *username,
> >>+ char **_basedn);
> >>+
> >> static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> struct tevent_context *ev,
> >> struct be_ctx *be_ctx,
> >>@@ -666,7 +671,6 @@ static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> {
> >> struct sdap_access_filter_req_ctx *state;
> >> struct tevent_req *req;
> >>- const char *basedn;
> >> char *clean_username;
> >> errno_t ret = ERR_INTERNAL;
> >> char *name;
> >>@@ -704,20 +708,9 @@ static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> goto done;
> >> }
> >>- /* Perform online operation */
> >>- basedn = ldb_msg_find_attr_as_string(user_entry, SYSDB_ORIG_DN, NULL);
> >>- if (basedn == NULL) {
> >>- DEBUG(SSSDBG_CRIT_FAILURE,"Could not find originalDN for user [%s]\n",
> >>- state->username);
> >>- ret = EINVAL;
> >>- goto done;
> >>- }
> >>-
> >>- state->basedn = talloc_strdup(state, basedn);
> >>- if (state->basedn == NULL) {
> >>- DEBUG(SSSDBG_CRIT_FAILURE,
> >>- "Could not allocate memory for originalDN\n");
> >>- ret = ENOMEM;
> >>+ ret = sdap_get_basedn_user_entry(state, user_entry, state->username,
> >>+ &state->basedn);
> >>+ if (ret != EOK) {
> >> goto done;
> >> }
> >>@@ -1141,3 +1134,32 @@ static errno_t sdap_access_host(struct ldb_message *user_entry)
> >> return ret;
> >> }
> >>+
> >>+static errno_t sdap_get_basedn_user_entry(TALLOC_CTX *mem_ctx,
> >>+ struct ldb_message *user_entry,
> >>+ const char *username,
> >>+ char **_basedn)
> >>+{
> >>+ const char *basedn;
> >>+ errno_t ret;
> >>+
> >>+ basedn = ldb_msg_find_attr_as_string(user_entry, SYSDB_ORIG_DN, NULL);
> >>+ if (basedn == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE,"Could not find originalDN for user [%s]\n",
> >>+ username);
> >>+ ret = EINVAL;
> >>+ goto done;
> >>+ }
> >>+
> >>+ *_basedn = talloc_strdup(mem_ctx, basedn);
> >>+ if (*_basedn == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE,
> >>+ "Could not allocate memory for originalDN\n");
> >>+ ret = ENOMEM;
> >>+ goto done;
> >>+ }
> >Do we need to strdup the basedn here?
> I think that we actually do not need to call strdup. I guess we can just
> point to address in user_entry as it's allocated on parent memory context,
> so it should be accessible for lifetime 'filter' and 'lock' requests.
> >
> >>+ ret = EOK;
> >>+
> >>+done:
> >>+ return ret;
> >>+}
> >>--
> >>1.9.3
> >>
> >> From e510dbbb83b75982ce755cc27d8754875832aa3a Mon Sep 17 00:00:00 2001
> >>From: Pavel Reichl <preichl at redhat.com>
> >>Date: Fri, 1 Aug 2014 12:22:21 +0100
> >>Subject: [PATCH 4/9] SDAP: refactor sdap_access_filter_done
> >>
> >>As preparation for ticket #2364 move code from sdap_access_filter_done()
> >>into sdap_access_done() to make its reuse possible and thus avoid code
> >>duplication.
> >>---
> >> src/providers/ldap/sdap_access.c | 41 ++++++++++++++++++++++++++++++++++++----
> >> 1 file changed, 37 insertions(+), 4 deletions(-)
> >>
> >>diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> >>index e54f4e0d5d13d72e8cb38bd1c827ea2f52e7dcef..68633643f7430c26885a78d2903e7a4dd636e1d5 100644
> >>--- a/src/providers/ldap/sdap_access.c
> >>+++ b/src/providers/ldap/sdap_access.c
> >>@@ -60,8 +60,11 @@ static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> struct sdap_id_conn_ctx *conn,
> >> const char *username,
> >> struct ldb_message *user_entry);
> >>+
> >> static errno_t sdap_access_filter_recv(struct tevent_req *req);
> >>+static errno_t sdap_access_subreq_recv(struct tevent_req *req);
> >>+
> >> static errno_t sdap_account_expired(struct sdap_access_ctx *access_ctx,
> >> struct pam_data *pd,
> >> struct ldb_message *user_entry);
> >>@@ -219,14 +222,14 @@ static errno_t check_next_rule(struct sdap_access_req_ctx *state,
> >> return ret;
> >> }
> >>-static void sdap_access_filter_done(struct tevent_req *subreq)
> >>+static void sdap_access_done(struct tevent_req *subreq)
> >> {
> >> errno_t ret;
> >> struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
> >> struct sdap_access_req_ctx *state =
> >> tevent_req_data(req, struct sdap_access_req_ctx);
> >>- ret = sdap_access_filter_recv(subreq);
> >>+ ret = sdap_access_subreq_recv(subreq);
> >> talloc_zfree(subreq);
> >> if (ret != EOK) {
> >> DEBUG(SSSDBG_CRIT_FAILURE, "Error retrieving access check result.\n");
> >>@@ -249,6 +252,11 @@ static void sdap_access_filter_done(struct tevent_req *subreq)
> >> }
> >> }
> >>+static void sdap_access_filter_done(struct tevent_req *subreq)
> >>+{
> >>+ sdap_access_done(subreq);
> >>+}
> >>+
> >> errno_t sdap_access_recv(struct tevent_req *req)
> >> {
> >> TEVENT_REQ_RETURN_ON_ERROR(req);
> >>@@ -256,7 +264,6 @@ errno_t sdap_access_recv(struct tevent_req *req)
> >> return EOK;
> >> }
> >>-
> >> #define SHADOW_EXPIRE_MSG "Account expired according to shadow attributes"
> >> static errno_t sdap_account_expired_shadow(struct pam_data *pd,
> >>@@ -644,6 +651,10 @@ static errno_t sdap_account_expired(struct sdap_access_ctx *access_ctx,
> >> return ret;
> >> }
> >>+enum sdap_access_control_type {
> >>+ SDAP_ACCESS_CONTROL_FILTER,
> >>+};
> >>+
> >> struct sdap_access_filter_req_ctx {
> >> const char *username;
> >> const char *filter;
> >>@@ -656,6 +667,7 @@ struct sdap_access_filter_req_ctx {
> >> struct sss_domain_info *domain;
> >> bool cached_access;
> >> char *basedn;
> >>+ enum sdap_access_control_type ac_type;
> >> };
> >> static errno_t sdap_access_filter_decide_offline(struct tevent_req *req);
> >>@@ -696,6 +708,7 @@ static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> state->ev = ev;
> >> state->access_ctx = access_ctx;
> >> state->domain = domain;
> >>+ state->ac_type = SDAP_ACCESS_CONTROL_FILTER;
> >> DEBUG(SSSDBG_TRACE_FUNC,
> >> "Performing access filter check for user [%s]\n", username);
> >>@@ -772,6 +785,27 @@ done:
> >> return req;
> >> }
> >>+static errno_t sdap_access_subreq_recv(struct tevent_req *req)
> >>+{
> >>+ struct sdap_access_filter_req_ctx *state;
> >>+ errno_t ret;
> >>+
> >>+ state = tevent_req_data(req, struct sdap_access_filter_req_ctx);
> >>+
> >>+ switch(state->ac_type) {
> >>+ case SDAP_ACCESS_CONTROL_FILTER:
> >>+ ret = sdap_access_filter_recv(req);
> >>+ break;
> >>+ default:
> >>+ ret = EINVAL;
> >>+ DEBUG(SSSDBG_MINOR_FAILURE, "Unknown access control type: %d.",
> >>+ state->ac_type);
> >>+ break;
> >>+ }
> >>+
> >>+ return ret;
> >>+}
> >>+
> >This function is misplaced, it immediatelly follows _send() in the code.
> >_recv() should be the last in the flow.
> Hopefully it's fixed now.
> >> static errno_t sdap_access_filter_decide_offline(struct tevent_req *req)
> >> {
> >> struct sdap_access_filter_req_ctx *state =
> >>@@ -956,7 +990,6 @@ static errno_t sdap_access_filter_recv(struct tevent_req *req)
> >> return EOK;
> >> }
> >>-
> >> #define AUTHR_SRV_MISSING_MSG "Authorized service attribute missing, " \
> >> "access denied"
> >> #define AUTHR_SRV_DENY_MSG "Access denied by authorized service attribute"
> >>--
> >>1.9.3
> >>
> >> From 7f612f108f158d942beee6740ee10293c259dbfc Mon Sep 17 00:00:00 2001
> >>From: Pavel Reichl <preichl at redhat.com>
> >>Date: Wed, 6 Aug 2014 16:05:53 +0100
> >>Subject: [PATCH 7/9] SDAP: new option - DN to ppolicy on LDAP
> >Do we need to make the DN configurable? When you install the ppolicy,
> >can you actually set the DN?
> I haven't tested setting different DN, but I believe it's possible.
> During weekend I was putting my notes for setting ppolicy on OpenLDAP on
> this wiki page
> https://fedorahosted.org/sssd/wiki/openldap_ppolicy#Loadingppolicyoverlay
> (this is work in progress and I'm not even sure we care to have such a
> how-to on our wiki), but I believe you can see from there that DN is
> configurable.
>
> Still we could use a default DN if user didn't set DN himself:
>
> "cn=ppolicy,ou=policies,$search_base"
>
> >
> >> From f6cdc0ce09645cae4254c4e6cb6dfc24de0545e1 Mon Sep 17 00:00:00 2001
> >>From: Pavel Reichl <preichl at redhat.com>
> >>Date: Fri, 1 Aug 2014 17:44:24 +0100
> >>Subject: [PATCH 8/9] SDAP: account lock to restrict access via ssh key
> >>
> >>Be able to configure sssd to honor openldap account lock to restrict
> >>access via ssh key. Introduce new ldap_access_order value ('lock')
> >>for enabling/disabling this feature.
> >>
> >>Resolves:
> >>https://fedorahosted.org/sssd/ticket/2364
> >>---
> >> src/providers/ldap/ldap_init.c | 2 +
> >> src/providers/ldap/sdap_access.c | 439 +++++++++++++++++++++++++++++++++++++++
> >> src/providers/ldap/sdap_access.h | 4 +
> >> 3 files changed, 445 insertions(+)
> >>
> >>diff --git a/src/providers/ldap/ldap_init.c b/src/providers/ldap/ldap_init.c
> >>index 9960fd3ab7a6e446e5cdc1e8fa4984aab812d980..85d9acef1d2dd55361cba2ae67376673fe67d021 100644
> >>--- a/src/providers/ldap/ldap_init.c
> >>+++ b/src/providers/ldap/ldap_init.c
> >>@@ -421,6 +421,8 @@ int sssm_ldap_access_init(struct be_ctx *bectx,
> >> access_ctx->access_rule[c] = LDAP_ACCESS_SERVICE;
> >> } else if (strcasecmp(order_list[c], LDAP_ACCESS_HOST_NAME) == 0) {
> >> access_ctx->access_rule[c] = LDAP_ACCESS_HOST;
> >>+ } else if (strcasecmp(order_list[c], LDAP_ACCESS_LOCK_NAME) == 0) {
> >>+ access_ctx->access_rule[c] = LDAP_ACCESS_LOCK;
> >> } else {
> >> DEBUG(SSSDBG_CRIT_FAILURE,
> >> "Unexpected access rule name [%s].\n", order_list[c]);
> >>diff --git a/src/providers/ldap/sdap_access.c b/src/providers/ldap/sdap_access.c
> >>index 248ffbad44d9aa4794b6042f9a825639277cc1e9..e1bc018835f98661683bb8971ff2c8be6b3bcf99 100644
> >>--- a/src/providers/ldap/sdap_access.c
> >>+++ b/src/providers/ldap/sdap_access.c
> >>@@ -40,6 +40,7 @@
> >> #include "providers/data_provider.h"
> >> #include "providers/dp_backend.h"
> >>+#define PERMANENTLY_LOCKED_ACCOUNT "000001010000Z"
> >> #define MALFORMED_FILTER "Malformed access control filter [%s]\n"
> >> static errno_t sdap_save_user_cache_bool(struct sss_domain_info *domain,
> >>@@ -52,6 +53,18 @@ static errno_t sdap_get_basedn_user_entry(TALLOC_CTX *mem_ctx,
> >> const char *username,
> >> char **_basedn);
> >>+static struct tevent_req *
> >>+sdap_access_lock_send(TALLOC_CTX *mem_ctx,
> >>+ struct tevent_context *ev,
> >>+ struct be_ctx *be_ctx,
> >>+ struct sss_domain_info *domain,
> >>+ struct sdap_access_ctx *access_ctx,
> >>+ struct sdap_id_conn_ctx *conn,
> >>+ const char *username,
> >>+ struct ldb_message *user_entry);
> >>+
> >>+static void sdap_access_lock_done(struct tevent_req *subreq);
> >>+
> >> static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> struct tevent_context *ev,
> >> struct be_ctx *be_ctx,
> >>@@ -63,6 +76,8 @@ static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> static errno_t sdap_access_filter_recv(struct tevent_req *req);
> >>+static errno_t sdap_access_lock_recv(struct tevent_req *req);
> >>+
> >> static errno_t sdap_access_subreq_recv(struct tevent_req *req);
> >> static errno_t sdap_account_expired(struct sdap_access_ctx *access_ctx,
> >>@@ -182,6 +197,24 @@ static errno_t check_next_rule(struct sdap_access_req_ctx *state,
> >> /* we are done with no errors */
> >> return EOK;
> >>+ case LDAP_ACCESS_LOCK:
> >>+ ret = EOK;
> >>+
> >>+ subreq = sdap_access_lock_send(state, state->ev, state->be_ctx,
> >>+ state->domain,
> >>+ state->access_ctx,
> >>+ state->conn,
> >>+ state->pd->user,
> >>+ state->user_entry);
> >>+ if (subreq == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, "sdap_access_lock_send failed.\n");
> >>+ return ENOMEM;
> >>+ }
> >>+
> >>+ tevent_req_set_callback(subreq, sdap_access_lock_done, req);
> >>+ return EAGAIN;
> >>+ break;
> >>+
> >> case LDAP_ACCESS_FILTER:
> >> subreq = sdap_access_filter_send(state, state->ev, state->be_ctx,
> >> state->domain,
> >>@@ -658,6 +691,7 @@ static errno_t sdap_account_expired(struct sdap_access_ctx *access_ctx,
> >> enum sdap_access_control_type {
> >> SDAP_ACCESS_CONTROL_FILTER,
> >>+ SDAP_ACCESS_CONTROL_LOCK,
> >The code looks for a specific lock type, so the enum should be named
> >with a more specific name, too, maybe something like
> >SDAP_ACCESS_PPOLICY_LOCK?
> >
>
> >> };
> >> struct sdap_access_filter_req_ctx {
> >>@@ -678,8 +712,10 @@ struct sdap_access_filter_req_ctx {
> >> static errno_t sdap_access_decide_offline(struct tevent_req *req);
> >> static int sdap_access_filter_retry(struct tevent_req *req);
> >>+static void sdap_access_lock_connect_done(struct tevent_req *subreq);
> >> static void sdap_access_filter_connect_done(struct tevent_req *subreq);
> >> static void sdap_access_filter_get_access_done(struct tevent_req *req);
> >>+
> >> static struct tevent_req *sdap_access_filter_send(TALLOC_CTX *mem_ctx,
> >> struct tevent_context *ev,
> >> struct be_ctx *be_ctx,
> >>@@ -802,6 +838,9 @@ static errno_t sdap_access_subreq_recv(struct tevent_req *req)
> >> case SDAP_ACCESS_CONTROL_FILTER:
> >> ret = sdap_access_filter_recv(req);
> >> break;
> >>+ case SDAP_ACCESS_CONTROL_LOCK:
> >>+ ret = sdap_access_lock_recv(req);
> >>+ break;
> >> default:
> >> ret = EINVAL;
> >> DEBUG(SSSDBG_MINOR_FAILURE, "Unknown access control type: %d.",
> >>@@ -996,6 +1035,13 @@ static errno_t sdap_access_filter_recv(struct tevent_req *req)
> >> return EOK;
> >> }
> >>+static errno_t sdap_access_lock_recv(struct tevent_req *req)
> >>+{
> >>+ TEVENT_REQ_RETURN_ON_ERROR(req);
> >>+
> >>+ return EOK;
> >>+}
> >>+
> >This function seems misplaced, the order of tevent code is
> >
> >foo_send()
> >foo_step1()
> >foo_step2()
> >foo_recv();
> >
> >Here you have recv() before send();
> >
> >> #define AUTHR_SRV_MISSING_MSG "Authorized service attribute missing, " \
> >> "access denied"
> >> #define AUTHR_SRV_DENY_MSG "Access denied by authorized service attribute"
> >>@@ -1172,6 +1218,399 @@ static errno_t sdap_access_host(struct ldb_message *user_entry)
> >> return ret;
> >> }
> >>+static void sdap_access_lock_get_lockout_done(struct tevent_req *subreq);
> >>+static int sdap_access_lock_retry(struct tevent_req *req);
> >>+static struct tevent_req*
> >>+sdap_access_locked_time_send(struct tevent_req *req);
> >>+static void sdap_access_locked_time_done(struct tevent_req *subreq);
> >>+
> >>+static struct tevent_req *
> >>+sdap_access_lock_send(TALLOC_CTX *mem_ctx,
> >>+ struct tevent_context *ev,
> >>+ struct be_ctx *be_ctx,
> >>+ struct sss_domain_info *domain,
> >>+ struct sdap_access_ctx *access_ctx,
> >>+ struct sdap_id_conn_ctx *conn,
> >>+ const char *username,
> >>+ struct ldb_message *user_entry)
> >>+{
> >>+ struct sdap_access_filter_req_ctx *state;
> >Please don't reuse one state for two requests, that's quite dangerous.
> >
> >>+ struct tevent_req *req;
> >>+ errno_t ret = ERR_INTERNAL;
> >>+
> >>+ req = tevent_req_create(mem_ctx,
> >>+ &state, struct sdap_access_filter_req_ctx);
> >>+ if (req == NULL) {
> >>+ return NULL;
> >>+ }
> >>+
> >>+ state->filter = NULL;
> >>+ state->username = username;
> >>+ state->opts = access_ctx->id_ctx->opts;
> >>+ state->conn = conn;
> >>+ state->ev = ev;
> >>+ state->access_ctx = access_ctx;
> >>+ state->domain = domain;
> >>+ state->ac_type = SDAP_ACCESS_CONTROL_LOCK;
> >>+
> >>+ DEBUG(SSSDBG_TRACE_FUNC,
> >>+ "Performing access lock check for user [%s]\n", username);
> >>+
> >>+ state->cached_access = ldb_msg_find_attr_as_bool(user_entry,
> >>+ SYSDB_LDAP_ACCESS_CACHED,
> >>+ false);
> >>+
> >>+ /* Ok, we have one result, check if we are online or offline */
> >>+ if (be_is_offline(be_ctx)) {
> >>+ /* Ok, we're offline. Return from the cache */
> >>+ ret = sdap_access_decide_offline(req);
> >>+ goto done;
> >>+ }
> >>+
> >>+ ret = sdap_get_basedn_user_entry(state, user_entry, state->username,
> >>+ &state->basedn);
> >>+ if (ret != EOK) {
> >>+ goto done;
> >>+ }
> >>+
> >>+ DEBUG(SSSDBG_TRACE_FUNC, "Checking lock against LDAP\n");
> >>+
> >>+ state->sdap_op = sdap_id_op_create(state,
> >>+ state->conn->conn_cache);
> >>+ if (!state->sdap_op) {
> >>+ DEBUG(SSSDBG_OP_FAILURE, "sdap_id_op_create failed\n");
> >>+ ret = ENOMEM;
> >>+ goto done;
> >>+ }
> >>+
> >>+ ret = sdap_access_lock_retry(req);
> >>+ if (ret != EOK) {
> >>+ goto done;
> >>+ }
> >>+
> >>+ return req;
> >>+
> >>+done:
> >>+ if (ret == EOK) {
> >>+ tevent_req_done(req);
> >>+ } else {
> >>+ tevent_req_error(req, ret);
> >>+ }
> >>+ tevent_req_post(req, ev);
> >>+ return req;
> >>+}
> >>+
> >>+static int sdap_access_lock_retry(struct tevent_req *req)
> >>+{
> >>+ struct sdap_access_filter_req_ctx *state;
> >>+ struct tevent_req *subreq;
> >>+ int ret;
> >>+
> >>+ state = tevent_req_data(req, struct sdap_access_filter_req_ctx);
> >>+ subreq = sdap_id_op_connect_send(state->sdap_op, state, &ret);
> >>+ if (!subreq) {
> >>+ DEBUG(SSSDBG_OP_FAILURE,
> >>+ "sdap_id_op_connect_send failed: %d (%s)\n", ret, strerror(ret));
> >>+ return ret;
> >>+ }
> >>+
> >>+ tevent_req_set_callback(subreq, sdap_access_lock_connect_done, req);
> >>+ return EOK;
> >>+}
> >>+
> >>+static void sdap_access_lock_connect_done(struct tevent_req *subreq)
> >>+{
> >>+ struct tevent_req *req;
> >>+ struct sdap_access_filter_req_ctx *state;
> >>+ int ret, dp_error;
> >>+ const char *attrs[] = { SYSDB_LDAP_ACCESS_LOCKOUT, NULL };
> >>+ const char *ppolicy_dn;
> >>+
> >>+ req = tevent_req_callback_data(subreq, struct tevent_req);
> >>+ state = tevent_req_data(req, struct sdap_access_filter_req_ctx);
> >>+
> >>+ ret = sdap_id_op_connect_recv(subreq, &dp_error);
> >>+ talloc_zfree(subreq);
> >>+
> >>+ if (ret != EOK) {
> >>+ if (dp_error == DP_ERR_OFFLINE) {
> >>+ ret = sdap_access_decide_offline(req);
> >>+ if (ret == EOK) {
> >>+ tevent_req_done(req);
> >>+ return;
> >>+ }
> >>+ }
> >>+
> >>+ tevent_req_error(req, ret);
> >>+ return;
> >>+ }
> >>+
> >>+ ppolicy_dn = dp_opt_get_string(state->opts->basic,
> >>+ SDAP_PPOLICY_SEARCH_BASE);
> >>+ /* We can't find out if ppolicy is enabled, so we deny access */
> >>+ if (ppolicy_dn == NULL) {
> >>+ DEBUG(SSSDBG_CONF_SETTINGS,
> >>+ "ldap_ppolicy_search_base was not defined in configuration file."
> >>+ " Access is denied!\n");
> >>+ tevent_req_error(req, ERR_ACCESS_DENIED);
> >>+ return;
> >>+ }
> >>+
> >>+ /* Connection to LDAP succeeded
> >>+ * Send 'pwdLockout' request
> >>+ */
> >>+ subreq = sdap_get_generic_send(state,
> >>+ state->ev,
> >>+ state->opts,
> >>+ sdap_id_op_handle(state->sdap_op),
> >>+ ppolicy_dn,
> >>+ LDAP_SCOPE_BASE,
> >>+ NULL, attrs,
> >>+ NULL, 0,
> >>+ dp_opt_get_int(state->opts->basic,
> >>+ SDAP_SEARCH_TIMEOUT),
> >>+ false);
> >>+ if (subreq == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, "Could not start LDAP communication\n");
> >>+ tevent_req_error(req, EIO);
> >>+ return;
> >>+ }
> >>+
> >>+ tevent_req_set_callback(subreq, sdap_access_lock_get_lockout_done, req);
> >>+}
> >>+
> >>+static void sdap_access_lock_get_lockout_done(struct tevent_req *subreq)
> >>+{
> >>+ int ret, tret, dp_error;
> >>+ size_t num_results;
> >>+ bool pwdLockout = false;
> >>+ struct sysdb_attrs **results;
> >>+ struct tevent_req *req;
> >>+ struct sdap_access_filter_req_ctx *state;
> >>+
> >>+ req = tevent_req_callback_data(subreq, struct tevent_req);
> >>+ state = tevent_req_data(req, struct sdap_access_filter_req_ctx);
> >>+
> >>+ ret = sdap_get_generic_recv(subreq, state, &num_results, &results);
> >>+ talloc_zfree(subreq);
> >>+
> >>+ /* Check the number of responses we got
> >>+ * If it's exactly 1, we passed the check
> >>+ * If it's < 1, we failed the check
> >>+ * Anything else is an error
> >>+ */
> >>+ if (num_results < 1) {
> >>+ // todo doplnit base
> >czech todo and a C++ comment...baad :-)
> >
> >>+ DEBUG(SSSDBG_CONF_SETTINGS,
> >>+ "[%s] was not found."
> >>+ "Granting access.\n", SYSDB_LDAP_ACCESS_LOCKOUT);
> >>+ } else if (results == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, "num_results > 0, but results is NULL\n");
> >>+ ret = ERR_INTERNAL;
> >>+ goto done;
> >>+ } else if (num_results > 1) {
> >>+ /* It should not be possible to get more than one reply
> >>+ * here, since we're doing a base-scoped search
> >>+ */
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, "Received multiple replies\n");
> >>+ ret = ERR_INTERNAL;
> >>+ goto done;
> >>+ } else { /* Ok, we got a single reply */
> >>+ ret = sysdb_attrs_get_bool(results[0], SYSDB_LDAP_ACCESS_LOCKOUT,
> >>+ &pwdLockout);
> >You don't check ret here.
> >
> >>+ }
> >>+
> >>+ if (pwdLockout) {
> >>+ DEBUG(SSSDBG_TRACE_FUNC,
> >>+ "Password policy is enabled on LDAP server.\n");
> >>+
> >>+ subreq = sdap_access_locked_time_send(req);
> >This function name shouldn't end with _send, it's not a standalone
> >request. We tend to use _step for these. Also is there any reason why
> >you can't set the callback inside the function and return just errno?
> >
> >Alternatively, don't wrap the search in a function and just put the
> >contents of sdap_access_locked_time_send() here.
> >>+ if (subreq == NULL) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE, "sdap_access_lock_send failed.\n");
> >>+ ret = ENOMEM;
> >>+ goto done;
> >>+ }
> >>+
> >>+ tevent_req_set_callback(subreq, sdap_access_locked_time_done, req);
> >>+ ret = EAGAIN;
> >>+ goto done;
> >>+
> >>+ } else {
> >>+ DEBUG(SSSDBG_TRACE_FUNC,
> >>+ "Password policy is disabled on LDAP server "
> >>+ "- storing 'access granted' in sysdb.\n");
> >>+ tret = sdap_save_user_cache_bool(state->domain, state->username,
> >>+ SYSDB_LDAP_ACCESS_CACHED, true);
> >>+ if (tret != EOK) {
> >>+ /* Failing to save to the cache is non-fatal.
> >>+ * Just return the result.
> >>+ */
> >>+ DEBUG(SSSDBG_CRIT_FAILURE,
> >>+ "Failed to set user locked attribute\n");
> >>+ goto done;
> >>+ }
> >>+
> >>+ ret = EOK;
> >>+ goto done;
> >>+ }
> >>+
> >>+done:
> >>+ if (ret != EAGAIN) {
> >>+ /* release connection */
> >>+ tret = sdap_id_op_done(state->sdap_op, ret, &dp_error);
> >>+ if (tret != EOK) {
> >>+ DEBUG(SSSDBG_CRIT_FAILURE,
> >>+ "sdap_get_generic_send() returned error [%d][%s]\n",
> >>+ ret, sss_strerror(ret));
> >>+ }
> >>+
> >>+ if (ret == EOK) {
> >>+ tevent_req_done(req);
> >>+ } else {
> >>+ tevent_req_error(req, ret);
> >>+ }
> >>+ }
> >>+}
> >>+
> >_______________________________________________
> >sssd-devel mailing list
> >sssd-devel at lists.fedorahosted.org
> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list