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.
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@openldap `hostname`
>>>>Connection closed by UNKNOWN
>>>>
>>>>$ ssh -l max@openldap `hostname`
>>>>Last login: Fri Aug 1 15:16:21 2014 from sssd.dev.work
>>>>
>>>>$ ssh -l dick@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(a)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(a)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(a)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(a)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(a)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(a)lists.fedorahosted.org
>https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel