[SSSD] [PATCH] Improvements for LDAP Password Policy support

Sumit Bose sbose at redhat.com
Fri Mar 19 16:40:31 UTC 2010


On Thu, Mar 18, 2010 at 03:12:14PM +0100, Ralf Haferkamp wrote:
> Am Donnerstag 18 März 2010 12:42:23 schrieb Simo Sorce:
> > On Wed, 17 Mar 2010 15:33:38 +0100
> > 
> > Ralf Haferkamp <rhafer at suse.de> wrote:
> > > Hi,
> > > 
> > > here's another set of enhancements to the LDAP Password Policy
> > > support in the PAM module and the LDAP backend. The PAM module now
> > > issues warning when either the grace counter or the expire counter
> > > of the LDAP Ppolicy Control in > 0.
> > > 
> > > Addtionally I changed the detection for Ppolicy support of the LDAP
> > > Server a bit. If the Server returned the Ppolicy Control in the Bind
> > > Response  ppolicy support is assumed.
> > > 
> > > I left the original check for "pwdAttribute" intact, though I think
> > > it doesn't make a lot of sense. The "pwdAttribute" LDAP Attribute is
> > > usually not part of the user's entry, it's part of the Entry that
> > > contains the Policy. Addtionally it might be protected by ACLs and
> > > not be returned for anonymous (without losing any functionality).
> > 
> > Ralf,
> > patch looks mostly good, but there are some heavy coding style
> > violations.
> > 
> > if ( condition ){ is not good, it should be: is (condition) {
> > 
> > same for some functions foo( ccc ); is not good, use foo(ccc);
> > 
> > Ie, no space in parenthesis, a space only after keywords, and
> > always before the opening {
> > 
> > Don't use ++x, but x++ if possible.
> > 
> > Also there is at least one place where the return of talloc is not
> > checked.
> > 
> > Finally please try to keep the 80 columns limit where possible.
> 
> Updated patch attached. I think I fixed the coding style issues. 
> Additionally I just noticed that my orginal patch broke password resets 
> via LDAP password policies. This should be fixed now as well.
> 
> regards,
> 	Ralf

I have tested this new version and didn't find any issues. Password
resets are working, too.

It would be nice if you can fix the whitespace errors:

Applying: Improvements for LDAP Password Policy support
/home/sbose/wip/freeipa/sssd/.git/rebase-apply/patch:187: trailing whitespace.
                    DEBUG(4, 
/home/sbose/wip/freeipa/sssd/.git/rebase-apply/patch:291: trailing whitespace.
    ret = snprintf(user_msg, sizeof(user_msg), 
/home/sbose/wip/freeipa/sssd/.git/rebase-apply/patch:300: trailing whitespace.
               
/home/sbose/wip/freeipa/sssd/.git/rebase-apply/patch:337: trailing whitespace.
    ret = snprintf(user_msg, sizeof(user_msg), 
/home/sbose/wip/freeipa/sssd/.git/rebase-apply/patch:344: trailing whitespace.
               
warning: squelched 2 whitespace errors
warning: 7 lines add whitespace errors.

bye,
Sumit

> From 0b1f123f20f4944425d70b02bb346b78176d0abe Mon Sep 17 00:00:00 2001
> From: Ralf Haferkamp <rhafer at suse.de>
> Date: Fri, 12 Mar 2010 10:54:40 +0100
> Subject: [PATCH] Improvements for LDAP Password Policy support
> 
> Display warnings about remaining grace logins and password
> expiration to the user, when LDAP Password Policies are used.
> 
> Improved detection if LDAP Password policies are supported by
> LDAP Server.
> ---
>  src/providers/ldap/ldap_auth.c             |   52 +++++++++++++++++-
>  src/providers/ldap/sdap.h                  |    5 ++
>  src/providers/ldap/sdap_async.h            |    6 ++-
>  src/providers/ldap/sdap_async_connection.c |   53 +++++++++++++++----
>  src/sss_client/pam_sss.c                   |   82 ++++++++++++++++++++++++++++
>  src/sss_client/sss_cli.h                   |   23 ++++++---
>  6 files changed, 201 insertions(+), 20 deletions(-)
> 
> diff --git a/src/providers/ldap/ldap_auth.c b/src/providers/ldap/ldap_auth.c
> index 5228703..b572bf2 100644
> --- a/src/providers/ldap/ldap_auth.c
> +++ b/src/providers/ldap/ldap_auth.c
> @@ -7,6 +7,7 @@
>          Sumit Bose <sbose at redhat.com>
>  
>      Copyright (C) 2008 Red Hat
> +    Copyright (C) 2010, rhafer at suse.de, Novell Inc.
>  
>      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
> @@ -135,6 +136,39 @@ static errno_t check_pwexpire_shadow(struct spwd *spwd, time_t now,
>      return EOK;
>  }
>  
> +static errno_t check_pwexpire_ldap(struct pam_data *pd,
> +                                   struct sdap_ppolicy_data *ppolicy,
> +                                   enum sdap_result *result)
> +{
> +    if (ppolicy->grace > 0 || ppolicy->expire > 0) {
> +        uint32_t *data;
> +        uint32_t *ptr;
> +
> +        data = talloc_size(pd, 2* sizeof(uint32_t));
> +        if (*data == NULL) {
> +            DEBUG(1, ("talloc_size failed.\n"));
> +            return ENOMEM;
> +        }
> +
> +        ptr = data;
> +        if (ppolicy->grace > 0) {
> +            *ptr = SSS_PAM_USER_INFO_GRACE_LOGIN;
> +            ptr++;
> +            *ptr = ppolicy->grace;
> +        } else if (ppolicy->expire > 0) {
> +            *ptr = SSS_PAM_USER_INFO_EXPIRE_WARN;
> +            ptr++;
> +            *ptr = ppolicy->expire;
> +        }
> +
> +        pam_add_response(pd, SSS_PAM_USER_INFO, 2* sizeof(uint32_t),
> +                         (uint8_t*)data);
> +    }
> +
> +    *result = SDAP_AUTH_SUCCESS;
> +    return EOK;
> +}
> +
>  static errno_t string_to_shadowpw_days(const char *s, long *d)
>  {
>      long l;
> @@ -569,8 +603,15 @@ static void auth_bind_user_done(struct tevent_req *subreq)
>      struct auth_state *state = tevent_req_data(req,
>                                                      struct auth_state);
>      int ret;
> -
> -    ret = sdap_auth_recv(subreq, &state->result);
> +    struct sdap_ppolicy_data *ppolicy;
> +
> +    ret = sdap_auth_recv(subreq, state, &state->result, &ppolicy);
> +    if (ppolicy != NULL) {
> +        DEBUG(9,("Found ppolicy data, "
> +                 "assuming LDAP password policies are active.\n"));
> +        state->pw_expire_type = PWEXPIRE_LDAP_PASSWORD_POLICY;
> +        state->pw_expire_data = ppolicy;
> +    }
>      talloc_zfree(subreq);
>      if (ret) {
>          tevent_req_error(req, ret);
> @@ -960,6 +1001,13 @@ static void sdap_pam_auth_done(struct tevent_req *req)
>                  }
>                  break;
>              case PWEXPIRE_LDAP_PASSWORD_POLICY:
> +                ret = check_pwexpire_ldap(state->pd, pw_expire_data, &result);
> +                if (ret != EOK) {
> +                    DEBUG(1, ("check_pwexpire_ldap failed.\n"));
> +                    state->pd->pam_status = PAM_SYSTEM_ERR;
> +                    goto done;
> +                }
> +                break;
>              case PWEXPIRE_NONE:
>                  break;
>              default:
> diff --git a/src/providers/ldap/sdap.h b/src/providers/ldap/sdap.h
> index 007185f..f0e345e 100644
> --- a/src/providers/ldap/sdap.h
> +++ b/src/providers/ldap/sdap.h
> @@ -85,6 +85,11 @@ struct sdap_service {
>      char *uri;
>  };
>  
> +struct sdap_ppolicy_data {
> +    int grace;
> +    int expire;
> +};
> +
>  #define SYSDB_SHADOWPW_LASTCHANGE "shadowLastChange"
>  #define SYSDB_SHADOWPW_MIN "shadowMin"
>  #define SYSDB_SHADOWPW_MAX "shadowMax"
> diff --git a/src/providers/ldap/sdap_async.h b/src/providers/ldap/sdap_async.h
> index 3c52d23..888df6b 100644
> --- a/src/providers/ldap/sdap_async.h
> +++ b/src/providers/ldap/sdap_async.h
> @@ -76,7 +76,11 @@ struct tevent_req *sdap_auth_send(TALLOC_CTX *memctx,
>                                    const char *user_dn,
>                                    const char *authtok_type,
>                                    struct dp_opt_blob authtok);
> -int sdap_auth_recv(struct tevent_req *req, enum sdap_result *result);
> +
> +int sdap_auth_recv(struct tevent_req *req,
> +                   TALLOC_CTX *memctx,
> +                   enum sdap_result *result,
> +                   struct sdap_ppolicy_data **ppolicy);
>  
>  struct tevent_req *sdap_get_initgr_send(TALLOC_CTX *memctx,
>                                          struct tevent_context *ev,
> diff --git a/src/providers/ldap/sdap_async_connection.c b/src/providers/ldap/sdap_async_connection.c
> index fe8a501..4a75c1d 100644
> --- a/src/providers/ldap/sdap_async_connection.c
> +++ b/src/providers/ldap/sdap_async_connection.c
> @@ -4,6 +4,7 @@
>      Async LDAP Helper routines
>  
>      Copyright (C) Simo Sorce <ssorce at redhat.com> - 2009
> +    Copyright (C) 2010, rhafer at suse.de, Novell Inc.
>  
>      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
> @@ -278,6 +279,7 @@ struct simple_bind_state {
>      struct sdap_op *op;
>  
>      struct sdap_msg *reply;
> +    struct sdap_ppolicy_data *ppolicy;
>      int result;
>  };
>  
> @@ -401,6 +403,7 @@ static void simple_bind_done(struct sdap_op *op,
>  
>      if (response_controls == NULL) {
>          DEBUG(5, ("Server returned no controls.\n"));
> +        state->ppolicy = NULL;
>      } else {
>          for (c = 0; response_controls[c] != NULL; c++) {
>              DEBUG(9, ("Server returned control [%s].\n",
> @@ -420,12 +423,30 @@ static void simple_bind_done(struct sdap_op *op,
>                  DEBUG(7, ("Password Policy Response: expire [%d] grace [%d] "
>                            "error [%s].\n", pp_expire, pp_grace,
>                            ldap_passwordpolicy_err2txt(pp_error)));
> -
> -                if ((state->result == LDAP_SUCCESS &&
> -                        (pp_error == PP_changeAfterReset || pp_grace > 0)) ||
> -                    (state->result == LDAP_INVALID_CREDENTIALS &&
> -                        pp_error == PP_passwordExpired ) ) {
> -                    DEBUG(4, ("User must set a new password.\n"));
> +                state->ppolicy = talloc(state, struct sdap_ppolicy_data);
> +                if (state->ppolicy == NULL) {
> +                    DEBUG(1, ("talloc failed.\n"));
> +                    ret = ENOMEM;
> +                    goto done;
> +                }
> +                state->ppolicy->grace = pp_grace;
> +                state->ppolicy->expire = pp_expire;
> +                if (state->result == LDAP_SUCCESS) {
> +                    if (pp_error == PP_changeAfterReset) {
> +                        DEBUG(4, ("Password was reset. "
> +                                  "User must set a new password.\n"));
> +                        state->result = LDAP_X_SSSD_PASSWORD_EXPIRED;
> +                    } else if (pp_grace > 0) {
> +                        DEBUG(4, ("Password expired. "
> +                                  "[%d] grace logins remaining.\n", pp_grace));
> +                    } else if (pp_expire > 0) {
> +                        DEBUG(4, ("Password will expire in [%d] seconds.\n",
> +                                  pp_expire));
> +                    }
> +                } else if (state->result == LDAP_INVALID_CREDENTIALS &&
> +                           pp_error == PP_passwordExpired) {
> +                    DEBUG(4, 
> +                          ("Password expired user must set a new password.\n"));
>                      state->result = LDAP_X_SSSD_PASSWORD_EXPIRED;
>                  }
>              }
> @@ -446,7 +467,10 @@ done:
>      }
>  }
>  
> -static int simple_bind_recv(struct tevent_req *req, int *ldaperr)
> +static int simple_bind_recv(struct tevent_req *req,
> +                            TALLOC_CTX *memctx,
> +                            int *ldaperr,
> +                            struct sdap_ppolicy_data **ppolicy)
>  {
>      struct simple_bind_state *state = tevent_req_data(req,
>                                              struct simple_bind_state);
> @@ -455,6 +479,7 @@ static int simple_bind_recv(struct tevent_req *req, int *ldaperr)
>      TEVENT_REQ_RETURN_ON_ERROR(req);
>  
>      *ldaperr = state->result;
> +    *ppolicy = talloc_steal(memctx, state->ppolicy);
>      return EOK;
>  }
>  
> @@ -704,6 +729,7 @@ int sdap_kinit_recv(struct tevent_req *req, enum sdap_result *result)
>  struct sdap_auth_state {
>      const char *user_dn;
>      struct berval pw;
> +    struct sdap_ppolicy_data *ppolicy;
>  
>      int result;
>      bool is_sasl;
> @@ -766,8 +792,9 @@ static void sdap_auth_done(struct tevent_req *subreq)
>  
>      if (state->is_sasl) {
>          ret = sasl_bind_recv(subreq, &state->result);
> +        state->ppolicy = NULL;
>      } else {
> -        ret = simple_bind_recv(subreq, &state->result);
> +        ret = simple_bind_recv(subreq, state, &state->result, &state->ppolicy);
>      }
>      if (ret != EOK) {
>          tevent_req_error(req, ret);
> @@ -777,7 +804,10 @@ static void sdap_auth_done(struct tevent_req *subreq)
>      tevent_req_done(req);
>  }
>  
> -int sdap_auth_recv(struct tevent_req *req, enum sdap_result *result)
> +int sdap_auth_recv(struct tevent_req *req,
> +                   TALLOC_CTX *memctx,
> +                   enum sdap_result *result,
> +                   struct sdap_ppolicy_data **ppolicy)
>  {
>      struct sdap_auth_state *state = tevent_req_data(req,
>                                                   struct sdap_auth_state);
> @@ -785,6 +815,9 @@ int sdap_auth_recv(struct tevent_req *req, enum sdap_result *result)
>      *result = SDAP_ERROR;
>      TEVENT_REQ_RETURN_ON_ERROR(req);
>  
> +    if (ppolicy != NULL) {
> +        *ppolicy = talloc_steal(memctx, state->ppolicy);
> +    }
>      switch (state->result) {
>          case LDAP_SUCCESS:
>              *result = SDAP_AUTH_SUCCESS;
> @@ -1078,7 +1111,7 @@ static void sdap_cli_auth_done(struct tevent_req *subreq)
>      enum sdap_result result;
>      int ret;
>  
> -    ret = sdap_auth_recv(subreq, &result);
> +    ret = sdap_auth_recv(subreq, NULL, &result, NULL);
>      talloc_zfree(subreq);
>      if (ret) {
>          tevent_req_error(req, ret);
> diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c
> index 2ba6f15..013a4e5 100644
> --- a/src/sss_client/pam_sss.c
> +++ b/src/sss_client/pam_sss.c
> @@ -3,6 +3,7 @@
>          Sumit Bose <sbose at redhat.com>
>  
>      Copyright (C) 2009 Red Hat
> +    Copyright (C) 2010, rhafer at suse.de, Novell Inc.
>  
>      This program is free software; you can redistribute it and/or modify
>      it under the terms of the GNU Lesser General Public License as published by
> @@ -436,6 +437,81 @@ static int user_info_offline_auth(pam_handle_t *pamh, size_t buflen,
>      return PAM_SUCCESS;
>  }
>  
> +static int user_info_grace_login(pam_handle_t *pamh,
> +                                 size_t buflen,
> +                                 uint8_t *buf)
> +{
> +    int ret;
> +    uint32_t grace;
> +    char user_msg[256];
> +
> +    if (buflen != 2* sizeof(uint32_t)) {
> +        D(("User info response data has the wrong size"));
> +        return PAM_BUF_ERR;
> +    }
> +    memcpy(&grace, buf + sizeof(uint32_t), sizeof(uint32_t));
> +    ret = snprintf(user_msg, sizeof(user_msg), 
> +                   _("Your password has expired. "
> +                     "You have %d grace login(s) remaining."),
> +                   grace);
> +    if (ret < 0 || ret >= sizeof(user_msg)) {
> +        D(("snprintf failed."));
> +        return PAM_SYSTEM_ERR;
> +    }
> +    ret = do_pam_conversation(pamh, PAM_TEXT_INFO, user_msg, NULL, NULL);
> +               
> +    if (ret != PAM_SUCCESS) {
> +        D(("do_pam_conversation failed."));
> +        return PAM_SYSTEM_ERR;
> +    }
> +
> +    return PAM_SUCCESS;
> +}
> +
> +#define MINSEC 60
> +#define HOURSEC (60*MINSEC)
> +#define DAYSEC (24*HOURSEC)
> +static int user_info_expire_warn(pam_handle_t *pamh,
> +                                 size_t buflen,
> +                                 uint8_t *buf)
> +{
> +    int ret;
> +    uint32_t expire;
> +    char user_msg[256];
> +    const char* unit="second(s)";
> +
> +    if (buflen != 2* sizeof(uint32_t)) {
> +        D(("User info response data has the wrong size"));
> +        return PAM_BUF_ERR;
> +    }
> +    memcpy(&expire, buf + sizeof(uint32_t), sizeof(uint32_t));
> +    if (expire >= DAYSEC) {
> +        expire /= DAYSEC;
> +        unit = "day(s)";
> +    } else if (expire >= HOURSEC) {
> +        expire /= HOURSEC;
> +        unit = "hour(s)";
> +    } else if (expire >= MINSEC) {
> +        expire /= MINSEC;
> +        unit = "minute(s)";
> +    }
> +
> +    ret = snprintf(user_msg, sizeof(user_msg), 
> +                   _("Your password will expire in %d %s."), expire, unit);
> +    if (ret < 0 || ret >= sizeof(user_msg)) {
> +        D(("snprintf failed."));
> +        return PAM_SYSTEM_ERR;
> +    }
> +    ret = do_pam_conversation(pamh, PAM_TEXT_INFO, user_msg, NULL, NULL);
> +               
> +    if (ret != PAM_SUCCESS) {
> +        D(("do_pam_conversation failed."));
> +        return PAM_SYSTEM_ERR;
> +    }
> +
> +    return PAM_SUCCESS;
> +}
> +
>  static int user_info_offline_auth_delayed(pam_handle_t *pamh, size_t buflen,
>                                    uint8_t *buf)
>  {
> @@ -563,6 +639,12 @@ static int eval_user_info_response(pam_handle_t *pamh, size_t buflen,
>          case SSS_PAM_USER_INFO_OFFLINE_AUTH:
>              ret = user_info_offline_auth(pamh, buflen, buf);
>              break;
> +        case SSS_PAM_USER_INFO_GRACE_LOGIN:
> +            ret = user_info_grace_login(pamh, buflen, buf);
> +            break;
> +        case SSS_PAM_USER_INFO_EXPIRE_WARN:
> +            ret = user_info_expire_warn(pamh, buflen, buf);
> +            break;
>          case SSS_PAM_USER_INFO_OFFLINE_AUTH_DELAYED:
>              ret = user_info_offline_auth_delayed(pamh, buflen, buf);
>              break;
> diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h
> index 2edd158..e1420c8 100644
> --- a/src/sss_client/sss_cli.h
> +++ b/src/sss_client/sss_cli.h
> @@ -377,13 +377,22 @@ enum user_info_type {
>                                         * possible to change the password while
>                                         * the system is offline. This message
>                                         * is generated by the PAM responder. */
> -    SSS_PAM_USER_INFO_CHPASS_ERROR /**< Tell the user that a password change
> -                                    * failed and optionally give a reason.
> -                                    * @param Size of the message as unsigned
> -                                    * 32-bit integer value. A value of 0
> -                                    * indicates that no message is following.
> -                                    * @param String with the specified
> -                                    * length. */
> +    SSS_PAM_USER_INFO_CHPASS_ERROR, /**< Tell the user that a password change
> +                                     * failed and optionally give a reason.
> +                                     * @param Size of the message as unsigned
> +                                     * 32-bit integer value. A value of 0
> +                                     * indicates that no message is following.
> +                                     * @param String with the specified
> +                                     * length. */
> +    SSS_PAM_USER_INFO_GRACE_LOGIN, /**< Warn the user that the password is 
> +                                    * expired and inform about the remaining
> +                                    * number of grace logins.
> +                                    * @param The number of remaining grace
> +                                    * logins as uint32_t */
> +    SSS_PAM_USER_INFO_EXPIRE_WARN /**< Warn the user that the password will 
> +                                   * expire soon.
> +                                   * @param Number of seconds before the user's
> +                                   * password will expire. */
>  };
>  /**
>   * @}
> -- 
> 1.6.4.2
> 

> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel




More information about the sssd-devel mailing list