From 12b4fba7ce62a3cbd5f90a42690bfb2518897ae7 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Thu, 14 Mar 2024 09:18:45 +0100 Subject: [PATCH 1/4] krb5: add OTP to krb5 response selection Originally where there was only password and OTP authentication we checked for password authentication and used OTP as a fallback. This was continued as other (pre)-authentication types were added. But so far only one authentication type was returned. This changed recently to allow the user a better selection and as a result OTP cannot be handled as a fallback anymore but has to be added to the selection. In case there are no types (questions) available now password is used as a fallback. Resolves: https://github.com/SSSD/sssd/issues/7152 --- src/providers/krb5/krb5_child.c | 107 ++++++++++++++++++++++---------- 1 file changed, 75 insertions(+), 32 deletions(-) diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index b5d7ffa0635..c06adc2bff4 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1200,6 +1200,44 @@ static krb5_error_code answer_passkey(krb5_context kctx, #endif /* BUILD_PASSKEY */ } +static krb5_error_code answer_password(krb5_context kctx, + struct krb5_req *kr, + krb5_responder_context rctx) +{ + krb5_error_code kerr; + int ret; + const char *pwd; + + kr->password_prompting = true; + + if ((kr->pd->cmd == SSS_PAM_AUTHENTICATE + || kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM + || kr->pd->cmd == SSS_PAM_CHAUTHTOK) + && sss_authtok_get_type(kr->pd->authtok) + == SSS_AUTHTOK_TYPE_PASSWORD) { + ret = sss_authtok_get_password(kr->pd->authtok, &pwd, NULL); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sss_authtok_get_password failed.\n"); + return ret; + } + + kerr = krb5_responder_set_answer(kctx, rctx, + KRB5_RESPONDER_QUESTION_PASSWORD, + pwd); + if (kerr != 0) { + DEBUG(SSSDBG_OP_FAILURE, + "krb5_responder_set_answer failed.\n"); + } + + return kerr; + } + + /* For SSS_PAM_PREAUTH and the other remaining commands the caller should + * continue to iterate over the available authentication methods. */ + return EAGAIN; +} + static krb5_error_code sss_krb5_responder(krb5_context ctx, void *data, krb5_responder_context rctx) @@ -1207,9 +1245,7 @@ static krb5_error_code sss_krb5_responder(krb5_context ctx, struct krb5_req *kr = talloc_get_type(data, struct krb5_req); const char * const *question_list; size_t c; - const char *pwd; - int ret; - krb5_error_code kerr; + krb5_error_code kerr = EINVAL; if (kr == NULL) { return EINVAL; @@ -1221,34 +1257,18 @@ static krb5_error_code sss_krb5_responder(krb5_context ctx, for (c = 0; question_list[c] != NULL; c++) { DEBUG(SSSDBG_TRACE_ALL, "Got question [%s].\n", question_list[c]); + /* It is expected that the answer_*() functions only return EOK + * (success) if the authentication was successful, i.e. during + * SSS_PAM_AUTHENTICATE. In all other cases, e.g. during + * SSS_PAM_PREAUTH either EAGAIN should be returned to indicate + * that the other available authentication methods should be + * checked as well. Or some other error code to indicate a fatal + * error where no other methods should be tried. + * Especially if setting the answer failed neither EOK nor EAGAIN + * should be returned. */ if (strcmp(question_list[c], KRB5_RESPONDER_QUESTION_PASSWORD) == 0) { - kr->password_prompting = true; - - if ((kr->pd->cmd == SSS_PAM_AUTHENTICATE - || kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM - || kr->pd->cmd == SSS_PAM_CHAUTHTOK) - && sss_authtok_get_type(kr->pd->authtok) - == SSS_AUTHTOK_TYPE_PASSWORD) { - ret = sss_authtok_get_password(kr->pd->authtok, &pwd, NULL); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "sss_authtok_get_password failed.\n"); - return ret; - } - - kerr = krb5_responder_set_answer(ctx, rctx, - KRB5_RESPONDER_QUESTION_PASSWORD, - pwd); - if (kerr != 0) { - DEBUG(SSSDBG_OP_FAILURE, - "krb5_responder_set_answer failed.\n"); - } - - return kerr; - } - - kerr = EOK; + kerr = answer_password(ctx, kr, rctx); } else if (strcmp(question_list[c], KRB5_RESPONDER_QUESTION_PKINIT) == 0 && (sss_authtok_get_type(kr->pd->authtok) @@ -1265,6 +1285,8 @@ static krb5_error_code sss_krb5_responder(krb5_context ctx, continue; } kerr = answer_passkey(ctx, kr, rctx); + } else if (strcmp(question_list[c], KRB5_RESPONDER_QUESTION_OTP) == 0) { + kerr = answer_otp(ctx, kr, rctx); } else { DEBUG(SSSDBG_MINOR_FAILURE, "Unknown question type [%s]\n", question_list[c]); kerr = EINVAL; @@ -1274,16 +1296,37 @@ static krb5_error_code sss_krb5_responder(krb5_context ctx, * handled by the answer_* function. This allows fallback between auth * types, such as passkey -> password. */ if (kerr == EAGAIN) { - DEBUG(SSSDBG_TRACE_ALL, "Auth type [%s] could not be handled by answer function, " - "continuing to next question.\n", question_list[c]); + /* During pre-auth iterating over all authentication methods + * is expected and no message will be displayed. */ + if (kr->pd->cmd == SSS_PAM_AUTHENTICATE) { + DEBUG(SSSDBG_TRACE_ALL, + "Auth type [%s] could not be handled by answer " + "function, continuing to next question.\n", + question_list[c]); + } continue; } else { return kerr; } } + } else { + kerr = answer_password(ctx, kr, rctx); } - return answer_otp(ctx, kr, rctx); + /* During SSS_PAM_PREAUTH 'EAGAIN' is expected because we will run + * through all offered authentication methods and all are expect to return + * 'EAGAIN' in the positive case to indicate that the other methods should + * be checked as well. If all methods are checked we are done and should + * return success. + * In the other steps, especially SSS_PAM_AUTHENTICATE, having 'EAGAIN' at + * this stage would mean that no method feels responsible for the provided + * credentials i.e. authentication failed and we should return an error. + */ + if (kr->pd->cmd == SSS_PAM_PREAUTH) { + return kerr == EAGAIN ? 0 : kerr; + } else { + return kerr; + } } #endif /* HAVE_KRB5_GET_INIT_CREDS_OPT_SET_RESPONDER */ From 9036a42e0637b103869a35527ec56bd9b690ca52 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Fri, 15 Mar 2024 11:29:47 +0100 Subject: [PATCH 2/4] krb5: make sure answer_pkinit() use matching debug messages Resolves: https://github.com/SSSD/sssd/issues/7152 --- src/providers/krb5/krb5_child.c | 77 ++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 35 deletions(-) diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index c06adc2bff4..6079b35020e 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -745,51 +745,58 @@ static krb5_error_code answer_pkinit(krb5_context ctx, DEBUG(SSSDBG_TRACE_ALL, "Setting pkinit_prompting.\n"); kr->pkinit_prompting = true; - if (kr->pd->cmd == SSS_PAM_AUTHENTICATE - && (sss_authtok_get_type(kr->pd->authtok) + if (kr->pd->cmd == SSS_PAM_AUTHENTICATE) { + if ((sss_authtok_get_type(kr->pd->authtok) == SSS_AUTHTOK_TYPE_SC_PIN || sss_authtok_get_type(kr->pd->authtok) == SSS_AUTHTOK_TYPE_SC_KEYPAD)) { - kerr = sss_authtok_get_sc(kr->pd->authtok, &pin, NULL, - &token_name, NULL, - &module_name, NULL, - NULL, NULL, NULL, NULL); - if (kerr != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "sss_authtok_get_sc failed.\n"); - goto done; - } + kerr = sss_authtok_get_sc(kr->pd->authtok, &pin, NULL, + &token_name, NULL, + &module_name, NULL, + NULL, NULL, NULL, NULL); + if (kerr != EOK) { + DEBUG(SSSDBG_OP_FAILURE, + "sss_authtok_get_sc failed.\n"); + goto done; + } - for (c = 0; chl->identities[c] != NULL; c++) { - if (chl->identities[c]->identity != NULL - && pkinit_identity_matches(chl->identities[c]->identity, - token_name, module_name)) { - break; + for (c = 0; chl->identities[c] != NULL; c++) { + if (chl->identities[c]->identity != NULL + && pkinit_identity_matches(chl->identities[c]->identity, + token_name, module_name)) { + break; + } } - } - if (chl->identities[c] == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, - "No matching identity for [%s][%s] found in pkinit challenge.\n", - token_name, module_name); - kerr = EINVAL; - goto done; - } + if (chl->identities[c] == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, + "No matching identity for [%s][%s] found in pkinit " + "challenge.\n", token_name, module_name); + kerr = EINVAL; + goto done; + } - kerr = krb5_responder_pkinit_set_answer(ctx, rctx, - chl->identities[c]->identity, - pin); - if (kerr != 0) { - DEBUG(SSSDBG_OP_FAILURE, - "krb5_responder_set_answer failed.\n"); - } + kerr = krb5_responder_pkinit_set_answer(ctx, rctx, + chl->identities[c]->identity, + pin); + if (kerr != 0) { + DEBUG(SSSDBG_OP_FAILURE, + "krb5_responder_set_answer failed.\n"); + } - goto done; + goto done; + } else { + DEBUG(SSSDBG_MINOR_FAILURE, + "Unexpected authentication token type [%s]\n", + sss_authtok_type_to_str(sss_authtok_get_type(kr->pd->authtok))); + kerr = EAGAIN; + goto done; + } } else { - DEBUG(SSSDBG_MINOR_FAILURE, "Unexpected authentication token type [%s]\n", - sss_authtok_type_to_str(sss_authtok_get_type(kr->pd->authtok))); + /* We only expect SSS_PAM_PREAUTH here, but also for all other + * commands the graceful solution would be to let the caller + * check other authentication methods as well. */ kerr = EAGAIN; - goto done; } done: From 825383b213b48cbd62af9991cdfd99d38a00ece7 Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Fri, 15 Mar 2024 12:35:00 +0100 Subject: [PATCH 3/4] krb5: make prompter and pre-auth debug message less irritating Resolves: https://github.com/SSSD/sssd/issues/7152 --- src/providers/krb5/krb5_child.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 6079b35020e..cb5a1bea224 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1355,13 +1355,14 @@ static krb5_error_code sss_krb5_prompter(krb5_context context, void *data, int ret; size_t c; struct krb5_req *kr = talloc_get_type(data, struct krb5_req); + const char *err_msg; if (kr == NULL) { return EINVAL; } DEBUG(SSSDBG_TRACE_ALL, - "sss_krb5_prompter name [%s] banner [%s] num_prompts [%d] EINVAL.\n", + "sss_krb5_prompter name [%s] banner [%s] num_prompts [%d].\n", name, banner, num_prompts); if (num_prompts != 0) { @@ -1370,7 +1371,12 @@ static krb5_error_code sss_krb5_prompter(krb5_context context, void *data, prompts[c].prompt); } - DEBUG(SSSDBG_FUNC_DATA, "Prompter interface isn't used for password prompts by SSSD.\n"); + err_msg = krb5_get_error_message(context, KRB5_LIBOS_CANTREADPWD); + DEBUG(SSSDBG_FUNC_DATA, + "Prompter interface isn't used for prompting by SSSD." + "Returning the expected error [%ld/%s].\n", + KRB5_LIBOS_CANTREADPWD, err_msg); + krb5_free_error_message(context, err_msg); return KRB5_LIBOS_CANTREADPWD; } @@ -2839,8 +2845,9 @@ static errno_t tgt_req_child(struct krb5_req *kr) * should now know which authentication methods are available to * update the password. */ DEBUG(SSSDBG_TRACE_FUNC, - "krb5_get_init_creds_password returned [%d] during pre-auth, " - "ignored.\n", kerr); + "krb5_get_init_creds_password returned [%d] while collecting " + "available authentication types, errors are expected " + "and ignored.\n", kerr); ret = pam_add_prompting(kr); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "pam_add_prompting failed.\n"); From 90f904535aa1c9e97276791bb1b93eeba16f8aba Mon Sep 17 00:00:00 2001 From: Sumit Bose Date: Wed, 20 Mar 2024 11:26:16 +0100 Subject: [PATCH 4/4] pam_sss: prefer Smartcard authentication The current behavior is that Smartcard authentication is preferred if possible, i.e. if a Smartcard is present. Since the Smartcard (or equivalent) must be inserted manually the assumption is that if the user has inserted it they most probably want to use it for authentication. With the latest patches pam_sss might receive multiple available authentication methods. With this patch the checks for available authentication types start Smartcard authentication to mimic the existing behavior. Resolves: https://github.com/SSSD/sssd/issues/7152 --- src/sss_client/pam_sss.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/sss_client/pam_sss.c b/src/sss_client/pam_sss.c index 47f3f6bd38e..5171e58ec5c 100644 --- a/src/sss_client/pam_sss.c +++ b/src/sss_client/pam_sss.c @@ -2544,17 +2544,7 @@ static int get_authtok_for_authentication(pam_handle_t *pamh, } else if (pi->pc != NULL) { ret = prompt_by_config(pamh, pi); } else { - if (flags & PAM_CLI_FLAGS_USE_2FA - || (pi->otp_vendor != NULL && pi->otp_token_id != NULL - && pi->otp_challenge != NULL)) { - if (pi->password_prompting) { - ret = prompt_2fa(pamh, pi, _("First Factor: "), - _("Second Factor (optional): ")); - } else { - ret = prompt_2fa(pamh, pi, _("First Factor: "), - _("Second Factor: ")); - } - } else if (pi->cert_list != NULL) { + if (pi->cert_list != NULL) { if (pi->cert_list->next == NULL) { /* Only one certificate */ pi->selected_cert = pi->cert_list; @@ -2570,6 +2560,16 @@ static int get_authtok_for_authentication(pam_handle_t *pamh, || (pi->flags & PAM_CLI_FLAGS_REQUIRE_CERT_AUTH)) { /* Use pin prompt as fallback for gdm-smartcard */ ret = prompt_sc_pin(pamh, pi); + } else if (flags & PAM_CLI_FLAGS_USE_2FA + || (pi->otp_vendor != NULL && pi->otp_token_id != NULL + && pi->otp_challenge != NULL)) { + if (pi->password_prompting) { + ret = prompt_2fa(pamh, pi, _("First Factor: "), + _("Second Factor (optional): ")); + } else { + ret = prompt_2fa(pamh, pi, _("First Factor: "), + _("Second Factor: ")); + } } else if (pi->passkey_prompt_pin) { ret = prompt_passkey(pamh, pi, _("Insert your passkey device, then press ENTER."),