[SSSD] [PATCH] Add support for krb5 1.11's responder callback.

Jakub Hrozek jhrozek at redhat.com
Fri Mar 8 12:09:49 UTC 2013


On Tue, Mar 05, 2013 at 03:58:54PM -0500, Nathaniel McCallum wrote:
> This email provides the patch as an attachment. It also provides a more
> substantial commit message and fixes one comment formatting issue.
> 
> Nathaniel

Hi Nathaniel,

thanks for the patch! I'm not familiar with the OTP development in Kerberos
much, so I would prefer if there was additional opinion on this patch.

Functionality-wise, I only tested that with with krb5 1.10 and 1.11, when
using passwords, nothing breaks and I could still log in.

Code-wise, do you expect that the code in krb5_child.c will grow much when
implementing the missing features? If so, you might want to split it into
a new file. krb5_child.c is getting quite big already.

We have recently introduced a new set of error codes to the SSSD that
would gradually supersede the errno codes (for anything other than libc
API that really uses errno). It would be nice if the patch introduced new
error codes to src/util/util_errors.h instead of using EPROTO or ENOTSUP. I
know your development predates these new error codes, but I'd still prefer
if new code would use these error codes instead of errno.

Couple of other comments inline.

> From a544ee1457c8fd928d7e4d8fd9a3c8ee7ced5503 Mon Sep 17 00:00:00 2001
> From: Nathaniel McCallum <npmccallum at redhat.com>
> Date: Tue, 5 Mar 2013 15:57:45 -0500
> Subject: [PATCH] Add support for krb5 1.11's responder callback.
> 
> krb5 1.11 adds support for a new method for responding to
> structured data queries. This method, called the responder,
> provides an alternative to the prompter interface.
> 
> This patch adds support for this method. It takes the password
> and provides it via a responder instead of the prompter. In the
> case of OTP authentication, it also disables the caching of
> credentials (since the credentials are one-time only).
> ---
>  src/external/krb5.m4                    |   1 +
>  src/providers/krb5/krb5_auth.c          |   2 +-
>  src/providers/krb5/krb5_auth.h          |   1 +
>  src/providers/krb5/krb5_child.c         | 202 ++++++++++++++++++++++++++++++++
>  src/providers/krb5/krb5_child_handler.c |   7 ++
>  src/sss_client/sss_cli.h                |   3 +
>  6 files changed, 215 insertions(+), 1 deletion(-)
> 
> diff --git a/src/external/krb5.m4 b/src/external/krb5.m4
> index f1679a1..56c6484 100644
> --- a/src/external/krb5.m4
> +++ b/src/external/krb5.m4
> @@ -50,6 +50,7 @@ AC_CHECK_FUNCS([krb5_get_init_creds_opt_alloc krb5_get_error_message \
>                  krb5_get_init_creds_opt_set_fast_ccache_name \
>                  krb5_get_init_creds_opt_set_fast_flags \
>                  krb5_get_init_creds_opt_set_canonicalize \
> +                krb5_get_init_creds_opt_set_responder \
>                  krb5_unparse_name_flags \
>                  krb5_get_init_creds_opt_set_change_password_prompt \
>                  krb5_free_keytab_entry_contents \
> diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
> index e41e1a1..d3e11a2 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -1107,7 +1107,7 @@ static void krb5_auth_done(struct tevent_req *subreq)
>          goto done;
>      }
>  
> -    if (state->be_ctx->domain->cache_credentials == TRUE) {
> +    if (state->be_ctx->domain->cache_credentials == TRUE && !res->otp) {
>          krb5_auth_store_creds(state->sysdb, state->domain, pd);
>      }
>  
> diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
> index 078a31d..cf290ca 100644
> --- a/src/providers/krb5/krb5_auth.h
> +++ b/src/providers/krb5/krb5_auth.h
> @@ -81,6 +81,7 @@ struct krb5_child_response {
>      struct tgt_times tgtt;
>      char *ccname;
>      char *correct_upn;
> +    bool otp;
>  };
>  
>  errno_t
> diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
> index 9cc0f55..9f13fef 100644
> --- a/src/providers/krb5/krb5_child.c
> +++ b/src/providers/krb5/krb5_child.c
> @@ -146,6 +146,199 @@ static void sss_krb5_expire_callback_func(krb5_context context, void *data,
>      return;
>  }
>  
> +#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_RESPONDER
> +/*
> + * TODO: features that don't work:
> + *   1. tokeninfo selection
> + *   2. challenge
> + *   3. discreet token/pin prompting
> + *   4. interactive otp format correction
> + *   5. nextOTP
> + */
> +typedef int (*checker)(int c);
> +
> +static inline checker pick_checker(int format)
> +{
> +    switch (format) {
> +    case KRB5_RESPONDER_OTP_FORMAT_DECIMAL:
> +        return isdigit;
> +    case KRB5_RESPONDER_OTP_FORMAT_HEXADECIMAL:
> +        return isxdigit;
> +    case KRB5_RESPONDER_OTP_FORMAT_ALPHANUMERIC:
> +        return isalnum;
> +    }
> +
> +    return NULL;
> +}
> +
> +static krb5_error_code tokeninfo_matches(const struct pam_data *pd,
> +                                         const krb5_responder_otp_tokeninfo *ti,
> +                                         char **out_token, char **out_pin)
> +{

In general in code that uses talloc, functions that return an allocated
value should accept TALLOC_CTX *mem_ctx as the first parameter. The
output parameters should then be allocated on that memory context. This
is far better than allocating on NULL as you are sure that the memory
would eventually be freed. You wouldn't have to explicitly free the pin
and/or token but if for example they were allocated on top of the
krb5_req, then when krb5_req was freed, so would be token and pin.

> +    char *token = NULL, *pin = NULL;
> +    checker check = NULL;
> +    int i;
> +
> +    if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_NEXTOTP) {
> +        return ENOTSUP;
> +    }
> +
> +    if (ti->challenge != NULL) {
> +        return ENOTSUP;
> +    }
> +
> +    /* This is a non-sensical value. */
> +    if (ti->length == 0) {
> +        return EPROTO;
> +    }
> +
> +    if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_COLLECT_TOKEN) {
> +        /* ASSUMPTION: authtok has one of the following formats:
> +         *   1. TokenValue
> +         *   2. PIN+TokenValue
> +         */
> +        token = talloc_strndup(NULL, (char*) pd->authtok.data,
> +                               pd->authtok.length);
> +        if (token == NULL) {
> +            return ENOMEM;
> +        }
> +
> +        if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_COLLECT_PIN) {
> +            /* If the server desires a separate pin, we will split it.
> +             * ASSUMPTION: Format of authtok is PIN+TokenValue. */
> +            if (ti->flags & KRB5_RESPONDER_OTP_FLAGS_SEPARATE_PIN) {
> +                if (ti->length < 1) {
> +                    talloc_free(token);
> +                    return ENOTSUP;
> +                }
> +
> +                if (ti->length >= pd->authtok.length) {
> +                    talloc_free(token);
> +                    return EMSGSIZE;
> +                }
> +
> +                /* Copy the PIN from the front of the value. */
> +                pin = talloc_strndup(NULL, (char*) pd->authtok.data,
> +                                     pd->authtok.length - ti->length);
> +                if (pin == NULL) {
> +                    talloc_free(token);
> +                    return ENOMEM;
> +                }
> +
> +                /* Remove the PIN from the front of the token value. */
> +                memmove(token, token + pd->authtok.length - ti->length,
> +                        ti->length + 1);
> +
> +                check = pick_checker(ti->format);
> +            } else {
> +                if (ti->length > 0 && ti->length > pd->authtok.length) {
> +                    talloc_free(token);
> +                    return EMSGSIZE;
> +                }
> +            }
> +        } else {
> +            if (ti->length > 0 && ti->length != pd->authtok.length) {
> +                talloc_free(token);
> +                return EMSGSIZE;
> +            }
> +
> +            check = pick_checker(ti->format);
> +        }
> +    } else {
> +        pin = talloc_strndup(NULL, (char*) pd->authtok.data,
> +                             pd->authtok.length);
> +        if (pin == NULL) {
> +            return ENOMEM;
> +        }
> +    }
> +
> +    /* If check is set, we need to verify the contents of the token. */
> +    for (i = 0; check != NULL && token[i] != '\0'; i++) {
> +        if (!check(token[i])) {
> +            talloc_free(token);
> +            talloc_free(pin);
> +            return EBADMSG;
> +        }
> +    }
> +
> +    *out_token = token;
> +    *out_pin = pin;
> +    return 0;
> +}
> +
> +static krb5_error_code answer_otp(krb5_context ctx,
> +                                  struct krb5_req *kr,
> +                                  krb5_responder_context rctx)
> +{
> +    krb5_responder_otp_challenge *chl;
> +    char *token = NULL, *pin = NULL;
> +    krb5_error_code ret;
> +    size_t i;
> +
> +    ret = krb5_responder_otp_get_challenge(ctx, rctx, &chl);
> +    if (ret != EOK || chl == NULL) {
> +        /* Either an error, or nothing to do. */
> +        return ret;
> +    }
> +
> +    if (chl->tokeninfo == NULL || chl->tokeninfo[0] == NULL) {
> +        /* No tokeninfos? Absurd! */
> +        ret = EINVAL;
> +        goto done;
> +    }
> +
> +    /* Validate our assumptions about the contents of authtok. */
> +    if (kr->pd->authtok.type != SSS_AUTHTOK_TYPE_PASSWORD) {
> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    /* Find the first supported tokeninfo which matches our authtoken. */
> +    for (i = 0; chl->tokeninfo[i] != NULL; i++) {
> +        ret = tokeninfo_matches(kr->pd, chl->tokeninfo[i], &token, &pin);
> +        if (ret == EOK)
> +            break;
> +        else if (ret == ENOMEM)
> +            goto done;

I think it would be more defensive to explicitly enumerate the error
codes you want to safely ignore (currently EBADMSG, EMSGSIZE, EPROTO and
ENOTSUP) and error out with any other unexpected code. I know the above
works fine now, but maybe in future you would add a new fatal error code
to tokeninfo_matches() which would then be ignored.

In SSSD we also tend to enclose even single-line statements in {}
brackets.

> +    }
> +    if (chl->tokeninfo[i] == NULL) {
> +        DEBUG(1, ("No tokeninfos found which match our credentials.\n"));

Please use the new SSSDBG_ macros when introducing new code. The
existing decimal numbers are legacy code that we eventually convert to
these new  macros.

> +        ret = EOK;
> +        goto done;
> +    }
> +
> +    if (chl->tokeninfo[i]->flags & KRB5_RESPONDER_OTP_FLAGS_COLLECT_TOKEN) {
> +        /* Don't let SSSD cache the OTP authtok since it is single-use. */
> +        ret = pam_add_response(kr->pd, SSS_OTP, 0, NULL);
> +        if (ret != EOK) {
> +            DEBUG(1, ("pam_add_response failed.\n"));
> +            goto done;
> +        }
> +    }
> +
> +    /* Respond with the appropriate answer. */
> +    ret = krb5_responder_otp_set_answer(ctx, rctx, i, token, pin);
> +done:
> +    talloc_free(token);
> +    talloc_free(pin);
> +    krb5_responder_otp_challenge_free(ctx, rctx, chl);
> +    return ret;
> +}
> +
> +static krb5_error_code sss_krb5_responder(krb5_context ctx,
> +                                          void *data,
> +                                          krb5_responder_context rctx)
> +{
> +    struct krb5_req *kr = talloc_get_type(data, struct krb5_req);
> +
> +    if (kr == NULL) {
> +        return EINVAL;
> +    }
> +
> +    return answer_otp(ctx, kr, rctx);
> +}
> +#endif
> +
>  static krb5_error_code sss_krb5_prompter(krb5_context context, void *data,
>                                           const char *name, const char *banner,
>                                           int num_prompts, krb5_prompt prompts[])
> @@ -1746,6 +1939,15 @@ static int k5c_setup(struct krb5_req *kr, uint32_t offline)
>          return kerr;
>      }
>  
> +#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_RESPONDER
> +    kerr = krb5_get_init_creds_opt_set_responder(kr->ctx, kr->options,
> +                                                 sss_krb5_responder, kr);
> +    if (kerr != 0) {
> +        KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr);
> +        return kerr;
> +    }
> +#endif
> +
>  #ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_CHANGE_PASSWORD_PROMPT
>      /* A prompter is used to catch messages about when a password will
>       * expired. The library shall not use the prompter to ask for a new password
> diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c
> index 5adbcf7..99922b4 100644
> --- a/src/providers/krb5/krb5_child_handler.c
> +++ b/src/providers/krb5/krb5_child_handler.c
> @@ -482,6 +482,7 @@ parse_krb5_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf, ssize_t len,
>      struct krb5_child_response *res;
>      const char *upn = NULL;
>      size_t upn_len;
> +    bool otp = false;
>  
>      if ((size_t) len < sizeof(int32_t)) {
>          DEBUG(SSSDBG_CRIT_FAILURE, ("message too short.\n"));
> @@ -563,6 +564,11 @@ parse_krb5_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf, ssize_t len,
>              }
>          }
>  
> +        if (msg_type == SSS_OTP) {
> +            otp = true;
> +            skip = true;
> +        }
> +
>          if (!skip) {
>              ret = pam_add_response(pd, msg_type, msg_len, &buf[p]);
>              if (ret != EOK) {
> @@ -583,6 +589,7 @@ parse_krb5_child_response(TALLOC_CTX *mem_ctx, uint8_t *buf, ssize_t len,
>      res = talloc_zero(mem_ctx, struct krb5_child_response);
>      if (!res) return ENOMEM;
>  
> +    res->otp = otp;
>      res->msg_status = msg_status;
>      memcpy(&res->tgtt, &tgtt, sizeof(tgtt));
>  
> diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h
> index 372bcee..b0d7b82 100644
> --- a/src/sss_client/sss_cli.h
> +++ b/src/sss_client/sss_cli.h
> @@ -368,6 +368,9 @@ enum response_type {
>                            * the user.This should only be used in the case where
>                            * it is not possile to use SSS_PAM_USER_INFO.
>                            * @param A zero terminated string. */
> +    SSS_OTP,             /**< Indicates that the autotok was a OTP, so don't
> +                          * cache it. There is no message.
> +                          * @param None. */
>  };
>  
>  /**
> -- 
> 1.8.1.4
> 



More information about the sssd-devel mailing list