[SSSD] [PATCHES] SDAP: Be able to configure sssd to honor openldap account lock to restrict access via ssh key

Pavel Reichl preichl at redhat.com
Mon Aug 11 14:19:22 UTC 2014


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 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




More information about the sssd-devel mailing list