[SSSD] [PATCH 1/5] Fix tevent_req style for krb5_auth

Jakub Hrozek jhrozek at redhat.com
Tue Nov 6 08:08:49 UTC 2012


On Wed, Oct 31, 2012 at 06:37:31PM -0400, Simo Sorce wrote:
> No functionality changes,
> just make the code respect the tevent_req style and naming conventions
> and enhance readability by adding some helper functions.
> ---
>  src/providers/krb5/krb5_access.c     |    6 +-
>  src/providers/krb5/krb5_auth.c       |  556 ++++++++++++++++------------------
>  src/providers/krb5/krb5_auth.h       |    2 +-
>  src/providers/krb5/krb5_wait_queue.c |   12 +-
>  4 files changed, 268 insertions(+), 308 deletions(-)
> 
> diff --git a/src/providers/krb5/krb5_access.c b/src/providers/krb5/krb5_access.c
> index 25612807d701a392f697caa82b3f31182416b824..970633eb2f02b68cd3a3e38432c10f6a347edc91 100644
> --- a/src/providers/krb5/krb5_access.c
> +++ b/src/providers/krb5/krb5_access.c
> @@ -38,7 +38,7 @@ struct krb5_access_state {
>      bool access_allowed;
>  };
>  
> -static void krb5_access_child_done(struct tevent_req *subreq);
> +static void krb5_access_done(struct tevent_req *subreq);
>  struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
>                                      struct tevent_context *ev,
>                                      struct be_ctx *be_ctx,
> @@ -141,7 +141,7 @@ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
>  
> -    tevent_req_set_callback(subreq, krb5_access_child_done, req);
> +    tevent_req_set_callback(subreq, krb5_access_done, req);
>      return req;
>  
>  done:
> @@ -154,7 +154,7 @@ done:
>      return req;
>  }
>  
> -static void krb5_access_child_done(struct tevent_req *subreq)
> +static void krb5_access_done(struct tevent_req *subreq)
>  {
>      struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
>      struct krb5_access_state *state = tevent_req_data(req,
> diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
> index f2e00fac164c4a8ff40e67edb3fa400a3f339b7c..c56dfb60a5c37c6cd6c825355fcad1e49767657a 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -270,12 +270,116 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
>      return EOK;
>  }
>  
> -static void krb5_resolve_kdc_done(struct tevent_req *subreq);
> -static void krb5_resolve_kpasswd_done(struct tevent_req *subreq);
> -static void krb5_find_ccache_step(struct tevent_req *req);
> -static void krb5_save_ccname_done(struct tevent_req *req);
> -static void krb5_child_done(struct tevent_req *req);
> -static void krb5_pam_handler_cache_auth_step(struct tevent_req *req);
> +
> +static void krb5_auth_cache_creds(struct krb5_ctx *krb5_ctx,
> +                                  struct sysdb_ctx *sysdb,
> +                                  struct confdb_ctx *cdb,
> +                                  struct pam_data *pd, uid_t uid,
> +                                  int *pam_status, int *dp_err)
> +{
> +    errno_t ret;
> +
> +    ret = sysdb_cache_auth(sysdb, pd->user, pd->authtok,
> +                           pd->authtok_size, cdb, true, NULL,
> +                           NULL);
> +    if (ret != EOK) {
> +        DEBUG(1, ("Offline authentication failed\n"));
> +        *pam_status = PAM_SYSTEM_ERR;
> +        *dp_err = DP_ERR_OK;
> +        return;
> +    }
> +
> +    ret = add_user_to_delayed_online_authentication(krb5_ctx, pd, uid);
> +    if (ret != EOK) {
> +        /* This error is not fatal */
> +        DEBUG(1, ("add_user_to_delayed_online_authentication failed.\n"));
> +    }
> +    *pam_status = PAM_AUTHINFO_UNAVAIL;
> +    *dp_err = DP_ERR_OFFLINE;
> +}
> +

I'm not quite sure I like a function that returns void but has two
output parameters..but then again, the intent of the function is to
return a (pam_status, dp_err) tuple.

> +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
> +                                             struct be_ctx *be_ctx,
> +                                             int *pam_status, int *dp_err)
> +{
> +    const char *ccname_template;
> +    bool private_path = false;
> +    errno_t ret;
> +
> +    if (!kr->is_offline) {
> +        kr->is_offline = be_is_offline(be_ctx);
> +    }
> +    if (kr->is_offline) {
> +        DEBUG(9, ("Preparing for offline operation.\n"));
> +    }
> +

Can you merge these two conditions into an if-else?

> +    /* The ccache file should be (re)created if one of the following conditions
> +     * is true:
> +     * - it doesn't exist (kr->ccname == NULL)
> +     * - the backend is online and the current ccache file is not used, i.e
> +     *   the related user is currently not logged in and it is not a renewal
> +     *   request
> +     *   (!kr->is_offline && !kr->active_ccache_present &&
> +     *    kr->pd->cmd != SSS_CMD_RENEW)
> +     * - the backend is offline and the current cache file not used and
> +     *   it does not contain a valid tgt
> +     *   (kr->is_offline &&
> +     *    !kr->active_ccache_present && !kr->valid_tgt_present)
> +     */
> +    if (kr->ccname == NULL ||
> +        (kr->is_offline &&
> +         !kr->active_ccache_present &&
> +         !kr->valid_tgt_present) ||
> +        (!kr->is_offline &&
> +         !kr->active_ccache_present &&
> +         kr->pd->cmd != SSS_CMD_RENEW)) {

Now that every condition is on a separate line, the whole if no longer
looks like OR of three cases at first sight. The comment above it helps
a little, but maybe this would be an acceptable case for breaking the 80
chars limit. I think this:

if (kr->ccname == NULL ||
    (kr->is_offline && !kr->active_ccache_present && !kr->valid_tgt_present) ||
    (!kr->is_offline && !kr->active_ccache_present && kr->pd->cmd != SSS_CMD_RENEW)) {

Is more clear.

Maybe just split the if a little differently to make it clear the line
continues:

if (kr->ccname == NULL ||
    (kr->is_offline && !kr->active_ccache_present && \
     !kr->valid_tgt_present) ||
    (!kr->is_offline && !kr->active_ccache_present && \
     kr->pd->cmd != SSS_CMD_RENEW)) {

> -
> -static void krb5_child_done(struct tevent_req *subreq)
> +static void krb5_auth_done(struct tevent_req *subreq)
>  {
>      struct tevent_req *req = tevent_req_callback_data(subreq, struct tevent_req);
>      struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -
>      struct krb5child_req *kr = state->kr;
>      struct pam_data *pd = state->pd;
>      int ret;
> @@ -750,18 +764,54 @@ static void krb5_child_done(struct tevent_req *subreq)
>      ssize_t len = -1;
>      struct krb5_child_response *res;
>      const char *store_ccname;
> +    struct fo_server *search_srv;
> +    char *password = NULL;
>  
>      ret = handle_child_recv(subreq, pd, &buf, &len);
>      talloc_zfree(subreq);
> -    if (ret != EOK) {
> +    if (ret != EOK && ret != ETIMEDOUT) {
>          DEBUG(1, ("child failed (%d [%s])\n", ret, strerror(ret)));
> -        if (ret == ETIMEDOUT) {
> -            if (krb5_next_server(req) == NULL) {
> -                tevent_req_error(req, ENOMEM);
> +        tevent_req_error(req, ret);
> +        return;
> +    }
> +    if (ret == ETIMEDOUT) {
> +

I find it more readable to keep the handling of a single return code in
a single if statement, ie:

if (ret == ETIMEDOUT) {
    /* Handle time out */
} else if (ret != EOK) {
    /* Handle error */
}

/* EOK */

> +        DEBUG(1, ("child timed out!\n"));
> +
> +        switch (pd->cmd) {
> +        case SSS_PAM_AUTHENTICATE:
> +        case SSS_CMD_RENEW:
> +            state->search_kpasswd = false;
> +            search_srv = kr->srv;
> +            break;
> +        case SSS_PAM_CHAUTHTOK:
> +        case SSS_PAM_CHAUTHTOK_PRELIM:
> +            if (state->kr->kpasswd_srv) {
> +                state->search_kpasswd = true;
> +                search_srv = kr->kpasswd_srv;
> +                break;
> +            } else {
> +                state->search_kpasswd = false;
> +                search_srv = kr->srv;
> +                break;
>              }
> -        } else {
> -            tevent_req_error(req, ret);
> +        default:
> +            DEBUG(1, ("Unexpected PAM task\n"));
> +            tevent_req_error(req, EINVAL);
> +            return;

The error handling in the function is not consistent. Some branches use
tevent_req_error(req, errno) and some use goto done.

The function is also quite big (300+ lines). I would suggest to also
split it into some helper functions. The timeout handling and password
caching operations seem like a good start.

>          }
> +
> +        be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
> +                              search_srv, PORT_NOT_WORKING);
> +        subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> +                                        state->krb5_ctx->kpasswd_service->name,
> +                                        search_srv == NULL ? true : false);
> +        if (subreq == NULL) {
> +            DEBUG(1, ("Failed resolved request.\n"));
> +            tevent_req_error(req, ENOMEM);
> +            return;
> +        }
> +        tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
>          return;
>      }
>  
> @@ -871,9 +921,16 @@ static void krb5_child_done(struct tevent_req *subreq)
>                                    state->krb5_ctx->service->name,
>                                    kr->kpasswd_srv, PORT_NOT_WORKING);
>              /* ..try to resolve next kpasswd server */
> -            if (krb5_next_kpasswd(req) == NULL) {
> +            state->search_kpasswd = true;
> +            subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> +                                state->krb5_ctx->kpasswd_service->name,
> +                                state->kr->kpasswd_srv == NULL ?  true : false);
> +            if (subreq == NULL) {
> +                DEBUG(1, ("Resolver request failed.\n"));
>                  tevent_req_error(req, ENOMEM);
> +                return;
>              }
> +            tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
>              return;
>          } else {
>              be_fo_set_port_status(state->be_ctx,
> @@ -891,9 +948,16 @@ static void krb5_child_done(struct tevent_req *subreq)
>              be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
>                                    kr->srv, PORT_NOT_WORKING);
>              /* ..try to resolve next KDC */
> -            if (krb5_next_kdc(req) == NULL) {
> +            state->search_kpasswd = false;
> +            subreq = be_resolve_server_send(state, state->ev, state->be_ctx,
> +                                            state->krb5_ctx->service->name,
> +                                            kr->srv == NULL ?  true : false);
> +            if (subreq == NULL) {
> +                DEBUG(1, ("Resolver request failed.\n"));
>                  tevent_req_error(req, ENOMEM);
> +                return;
>              }
> +            tevent_req_set_callback(subreq, krb5_auth_resolve_done, req);
>              return;
>          }
>      } else if (kr->srv != NULL) {
> @@ -949,104 +1013,19 @@ static void krb5_child_done(struct tevent_req *subreq)
>          }
>      }
>  
> -    krb5_save_ccname_done(req);
> -
> -    return;
> -
> -done:
> -    if (ret == EOK) {
> -        tevent_req_done(req);
> -    } else {
> -        tevent_req_error(req, ret);
> -    }
> -}
> -
> -static struct tevent_req *krb5_next_server(struct tevent_req *req)
> -{
> -    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -    struct pam_data *pd = state->pd;
> -    struct tevent_req *next_req = NULL;
> -
> -    switch (pd->cmd) {
> -        case SSS_PAM_AUTHENTICATE:
> -        case SSS_CMD_RENEW:
> -            be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
> -                                  state->kr->srv, PORT_NOT_WORKING);
> -            next_req = krb5_next_kdc(req);
> -            break;
> -        case SSS_PAM_CHAUTHTOK:
> -        case SSS_PAM_CHAUTHTOK_PRELIM:
> -            if (state->kr->kpasswd_srv) {
> -                be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
> -                                      state->kr->kpasswd_srv, PORT_NOT_WORKING);
> -                next_req = krb5_next_kpasswd(req);
> -                break;
> -            } else {
> -                be_fo_set_port_status(state->be_ctx, state->krb5_ctx->service->name,
> -                                      state->kr->srv, PORT_NOT_WORKING);
> -                next_req = krb5_next_kdc(req);
> -                break;
> -            }
> -        default:
> -            DEBUG(1, ("Unexpected PAM task\n"));
> -    }
> -
> -    return next_req;
> -}
> -
> -static struct tevent_req *krb5_next_kdc(struct tevent_req *req)
> -{
> -    struct tevent_req *next_req;
> -    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -
> -    next_req = be_resolve_server_send(state, state->ev,
> -                                      state->be_ctx,
> -                                      state->krb5_ctx->service->name,
> -                                      state->kr->srv == NULL ? true : false);
> -    if (next_req == NULL) {
> -        DEBUG(1, ("be_resolve_server_send failed.\n"));
> -        return NULL;
> -    }
> -    tevent_req_set_callback(next_req, krb5_resolve_kdc_done, req);
> -
> -    return next_req;
> -}
> -
> -static struct tevent_req *krb5_next_kpasswd(struct tevent_req *req)
> -{
> -    struct tevent_req *next_req;
> -    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -
> -    next_req = be_resolve_server_send(state, state->ev,
> -                                state->be_ctx,
> -                                state->krb5_ctx->kpasswd_service->name,
> -                                state->kr->kpasswd_srv == NULL ? true : false);
> -    if (next_req == NULL) {
> -        DEBUG(1, ("be_resolve_server_send failed.\n"));
> -        return NULL;
> -    }
> -    tevent_req_set_callback(next_req, krb5_resolve_kpasswd_done, req);
> -
> -    return next_req;
> -}
> -
> -static void krb5_save_ccname_done(struct tevent_req *req)
> -{
> -    struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -    struct krb5child_req *kr = state->kr;
> -    struct pam_data *pd = state->pd;
> -    int ret;
> -    char *password = NULL;
> -
>      if (kr->is_offline) {
> -        if (dp_opt_get_bool(kr->krb5_ctx->opts,KRB5_STORE_PASSWORD_IF_OFFLINE)) {
> -            krb5_pam_handler_cache_auth_step(req);
> -            return;
> +        if (dp_opt_get_bool(kr->krb5_ctx->opts,
> +                            KRB5_STORE_PASSWORD_IF_OFFLINE)) {
> +            krb5_auth_cache_creds(state->kr->krb5_ctx,
> +                                  state->be_ctx->sysdb,
> +                                  state->be_ctx->cdb,
> +                                  state->pd, state->kr->uid,
> +                                  &state->pam_status, &state->dp_err);
> +        } else {
> +            DEBUG(4, ("Backend is marked offline, retry later!\n"));
> +            state->pam_status = PAM_AUTHINFO_UNAVAIL;
> +            state->dp_err = DP_ERR_OFFLINE;
>          }
> -
> -        DEBUG(4, ("Backend is marked offline, retry later!\n"));
> -        state->pam_status = PAM_AUTHINFO_UNAVAIL;
> -        state->dp_err = DP_ERR_OFFLINE;
>          ret = EOK;
>          goto done;
>      }
> @@ -1115,32 +1094,15 @@ done:
>  
>  }
>  
> -static void krb5_pam_handler_cache_auth_step(struct tevent_req *req)
> +int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err)
>  {
>      struct krb5_auth_state *state = tevent_req_data(req, struct krb5_auth_state);
> -    struct pam_data *pd = state->pd;
> -    struct krb5_ctx *krb5_ctx = state->kr->krb5_ctx;
> -    int ret;
> +    *pam_status = state->pam_status;
> +    *dp_err = state->dp_err;
>  
> -    ret = sysdb_cache_auth(state->sysdb, pd->user, pd->authtok,
> -                           pd->authtok_size, state->be_ctx->cdb, true, NULL,
> -                           NULL);
> -    if (ret != EOK) {
> -        DEBUG(1, ("Offline authentication failed\n"));
> -        state->pam_status = PAM_SYSTEM_ERR;
> -        state->dp_err = DP_ERR_OK;
> -    } else {
> -        ret = add_user_to_delayed_online_authentication(krb5_ctx, pd,
> -                                                       state->kr->uid);
> -        if (ret != EOK) {
> -            /* This error is not fatal */
> -            DEBUG(1, ("add_user_to_delayed_online_authentication failed.\n"));
> -        }
> -        state->pam_status = PAM_AUTHINFO_UNAVAIL;
> -        state->dp_err = DP_ERR_OFFLINE;
> -    }
> +    TEVENT_REQ_RETURN_ON_ERROR(req);
>  
> -    tevent_req_done(req);
> +    return EOK;
>  }
>  
>  static void krb_reply(struct be_req *req, int dp_err, int result)
> @@ -1148,8 +1110,8 @@ static void krb_reply(struct be_req *req, int dp_err, int result)
>      req->fn(req, dp_err, result, NULL);
>  }
>  
> -void krb5_auth_done(struct tevent_req *req);
> -static void krb5_access_done(struct tevent_req *req);
> +void krb5_pam_handler_auth_done(struct tevent_req *req);
> +static void krb5_pam_handler_access_done(struct tevent_req *req);
>  
>  void krb5_pam_handler(struct be_req *be_req)
>  {
> @@ -1193,7 +1155,7 @@ void krb5_pam_handler(struct be_req *be_req)
>                  goto done;
>              }
>  
> -            tevent_req_set_callback(req, krb5_auth_done, be_req);
> +            tevent_req_set_callback(req, krb5_pam_handler_auth_done, be_req);
>              break;
>          case SSS_PAM_ACCT_MGMT:
>              req = krb5_access_send(be_req, be_req->be_ctx->ev, be_req->be_ctx,
> @@ -1203,7 +1165,7 @@ void krb5_pam_handler(struct be_req *be_req)
>                  goto done;
>              }
>  
> -            tevent_req_set_callback(req, krb5_access_done, be_req);
> +            tevent_req_set_callback(req, krb5_pam_handler_access_done, be_req);
>              break;
>          case SSS_PAM_SETCRED:
>          case SSS_PAM_OPEN_SESSION:
> @@ -1225,7 +1187,7 @@ done:
>      krb_reply(be_req, dp_err, pd->pam_status);
>  }
>  
> -void krb5_auth_done(struct tevent_req *req)
> +void krb5_pam_handler_auth_done(struct tevent_req *req)
>  {
>      int ret;
>      struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
> @@ -1255,7 +1217,7 @@ void krb5_auth_done(struct tevent_req *req)
>      krb_reply(be_req, dp_err, pd->pam_status);
>  }
>  
> -static void krb5_access_done(struct tevent_req *req)
> +static void krb5_pam_handler_access_done(struct tevent_req *req)
>  {
>      int ret;
>      struct be_req *be_req = tevent_req_callback_data(req, struct be_req);
> diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
> index 9133472abb9ab0a42f1bac861a52f867d393e644..6f9679d1641d2689b87e10ef49d6124531f332cf 100644
> --- a/src/providers/krb5/krb5_auth.h
> +++ b/src/providers/krb5/krb5_auth.h
> @@ -61,6 +61,7 @@ errno_t krb5_setup(TALLOC_CTX *mem_ctx, struct pam_data *pd,
>                     struct krb5_ctx *krb5_ctx, struct krb5child_req **krb5_req);
>  
>  void krb5_pam_handler(struct be_req *be_req);
> +void krb5_pam_handler_auth_done(struct tevent_req *req);
>  
>  struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>                                    struct tevent_context *ev,
> @@ -68,7 +69,6 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>                                    struct pam_data *pd,
>                                    struct krb5_ctx *krb5_ctx);
>  int krb5_auth_recv(struct tevent_req *req, int *pam_status, int *dp_err);
> -void krb5_auth_done(struct tevent_req *req);
>  
>  struct tevent_req *handle_child_send(TALLOC_CTX *mem_ctx,
>                                       struct tevent_context *ev,
> diff --git a/src/providers/krb5/krb5_wait_queue.c b/src/providers/krb5/krb5_wait_queue.c
> index 3863b1bdc1386739ca0651278c9d941d85e46909..da1e35b7db8956950e9b1164ae983d21994dde69 100644
> --- a/src/providers/krb5/krb5_wait_queue.c
> +++ b/src/providers/krb5/krb5_wait_queue.c
> @@ -41,20 +41,18 @@ struct queue_entry {
>  static void wait_queue_auth(struct tevent_context *ev, struct tevent_timer *te,
>                               struct timeval current_time, void *private_data)
>  {
> -    struct queue_entry *queue_entry = talloc_get_type(private_data,
> -                                                      struct queue_entry);
> +    struct queue_entry *qe = talloc_get_type(private_data, struct queue_entry);
>      struct tevent_req *req;
>  
> -    req = krb5_auth_send(queue_entry->be_req, queue_entry->be_req->be_ctx->ev,
> -                         queue_entry->be_req->be_ctx, queue_entry->pd,
> -                         queue_entry->krb5_ctx);
> +    req = krb5_auth_send(qe->be_req, qe->be_req->be_ctx->ev,
> +                         qe->be_req->be_ctx, qe->pd, qe->krb5_ctx);
>      if (req == NULL) {
>          DEBUG(1, ("krb5_auth_send failed.\n"));
>      } else {
> -        tevent_req_set_callback(req, krb5_auth_done, queue_entry->be_req);
> +        tevent_req_set_callback(req, krb5_pam_handler_auth_done, qe->be_req);
>      }
>  
> -    talloc_zfree(queue_entry);
> +    talloc_zfree(qe);
>  }
>  
>  static void wait_queue_del_cb(hash_entry_t *entry, hash_destroy_enum type,
> -- 
> 1.7.1
> 
> _______________________________________________
> 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