While working on a different refactoring patch I came across the krb5_child code and found it very hard to read and follow due to the growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and easier to follow and read. A bunch of dead stuff that really confused me initially also got removed.
(I propose this for master only)
Simo.
On Thu, 2012-11-22 at 15:19 -0500, Simo Sorce wrote:
While working on a different refactoring patch I came across the krb5_child code and found it very hard to read and follow due to the growth it has gone through in the past.
I couldn't force myself not to refactor it to make it more linear and easier to follow and read. A bunch of dead stuff that really confused me initially also got removed.
(I propose this for master only)
Sorry, just noticed the name of a function got out wrong (k5_send_data instead of k5c_send_data). Amended patch attached.
Simo
I also noticed I missed to send a pre-requisite style-fix patch that I had in my tree.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Use the standard 'done' label for exceptions. --- src/providers/krb5/krb5_child.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index d1a42d56f1cb7137f2e21b54f6959da2fec5f6d0..01da84021c4fbaa9033e7e6539b704e7067c5a32 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1089,7 +1089,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) ret, strerror(ret))); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { @@ -1100,7 +1100,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length); @@ -1115,7 +1115,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); if (kerr != 0) { pam_status = kerr_handle_error(kerr); - goto sendresponse; + goto done; }
sss_authtok_set_empty(&kr->pd->authtok); @@ -1126,7 +1126,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) "successful.\n")); krb5_free_cred_contents(kr->ctx, kr->creds); pam_status = PAM_SUCCESS; - goto sendresponse; + goto done; }
ret = sss_authtok_get_password(&kr->pd->newauthtok, &newpassword, NULL); @@ -1134,7 +1134,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) DEBUG(1, ("Failed to fetch new password [%d] %s.\n", ret, strerror(ret))); kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
memset(&result_code_string, 0, sizeof(krb5_data)); @@ -1145,7 +1145,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
if (kerr == KRB5_KDC_UNREACH) { pam_status = PAM_AUTHTOK_LOCK_BUSY; - goto sendresponse; + goto done; }
if (kerr != 0 || result_code != 0) { @@ -1191,7 +1191,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) }
pam_status = PAM_AUTHTOK_ERR; - goto sendresponse; + goto done; }
krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1202,7 +1202,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n")); @@ -1226,7 +1226,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_OP_FAILURE, ("Unknown authtok type\n")); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
kerr = get_and_save_tgt(kr, password); @@ -1248,7 +1248,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, @@ -1269,7 +1269,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n"));
The aim of this refactoring is to make the code readable and understandable. This code has grown organically over time and has becomed confused and baroque enough that understanding it's very simple flow had become very complex for the uninitiated. Complex flows easily hide nasty bugs.
Improvements: - Remove dead/unused data storage - Fix and simplify talloc hierarchy, use a memory context (kr) for the whole code and allocate kr->pd where it is filled up. - Rename some functions to create a better name space (easier for searching fucntions across the tree) - Streamline setup function, by spliting out fast setup in a subroutine. - Avoid confusing indirection in executng actual functions by not using the krb5_req child_req member. - Make main() flow s now simmetric, send abck data from the main function instead of delegating a reply to every inner function that implements a command.
Now the flow is evident from the main function: 1. read request 2. setup data 3. execute command 4. send reply back --- src/providers/krb5/krb5_child.c | 490 ++++++++++++++++---------------------- 1 files changed, 206 insertions(+), 284 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 01da84021c4fbaa9033e7e6539b704e7067c5a32..c53be4e67a5239eb20f6af986c2d3791b2fb3a6b 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -40,53 +40,16 @@
#define SSSD_KRB5_CHANGEPW_PRINCIPAL "kadmin/changepw"
-struct krb5_child_ctx { - /* opts taken from kinit */ - /* in seconds */ - krb5_deltat starttime; - krb5_deltat lifetime; - krb5_deltat rlife; - - int forwardable; - int proxiable; - int addresses; - - int not_forwardable; - int not_proxiable; - int no_addresses; - - int verbose; - - char* principal_name; - char* service_name; - char* keytab_name; - char* k5_cache_name; - char* k4_cache_name; - - action_type action; - - char *kdcip; - char *realm; - char *ccache_dir; - char *ccname_template; - int auth_timeout; -}; - struct krb5_req { krb5_context ctx; krb5_principal princ; char* name; krb5_creds *creds; krb5_get_init_creds_opt *options; - pid_t child_pid; - int read_from_child_fd; - int write_to_child_fd;
- struct be_req *req; struct pam_data *pd; - struct krb5_child_ctx *krb5_ctx; - errno_t (*child_req)(int fd, struct krb5_req *kr);
+ char *realm; char *ccname; char *keytab; bool validate; @@ -676,14 +639,14 @@ static struct response *prepare_response_message(struct krb5_req *kr, return resp; }
-static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status, - struct krb5_req *kr) +static errno_t k5c_send_data(struct krb5_req *kr, int fd, + krb5_error_code kerr, int status) { struct response *resp; size_t written; int ret;
- resp = prepare_response_message(kr, kerr, pam_status); + resp = prepare_response_message(kr, kerr, status); if (resp == NULL) { DEBUG(1, ("prepare_response_message failed.\n")); return ENOMEM; @@ -1063,7 +1026,7 @@ static int kerr_to_status(krb5_error_code kerr) return kerr_handle_error(kerr); }
-static errno_t changepw_child(int fd, struct krb5_req *kr) +static errno_t changepw_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1203,15 +1166,11 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t tgt_req_child(int fd, struct krb5_req *kr) +static errno_t tgt_req_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1270,18 +1229,13 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t kuserok_child(int fd, struct krb5_req *kr) +static errno_t kuserok_child(struct krb5_req *kr, int *_status) { krb5_boolean access_allowed; - int status; int ret; krb5_error_code kerr;
@@ -1295,7 +1249,7 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) "krb5_kuserok will most certainly fail.\n")); }
- kerr = krb5_set_default_realm(kr->ctx, kr->krb5_ctx->realm); + kerr = krb5_set_default_realm(kr->ctx, kr->realm); if (kerr != 0) { DEBUG(1, ("krb5_set_default_realm failed, " "krb5_kuserok may fail.\n")); @@ -1305,17 +1259,11 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_TRACE_LIBS, ("Access was %s\n", access_allowed ? "allowed" : "denied"));
- status = access_allowed ? 0 : 1; - - ret = sendresponse(fd, 0, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = access_allowed ? 0 : 1; return ret; }
-static errno_t renew_tgt_child(int fd, struct krb5_req *kr) +static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) { int ret; int status = PAM_AUTHTOK_ERR; @@ -1396,15 +1344,11 @@ done: krb5_cc_close(kr->ctx, ccache); }
- ret = sendresponse(fd, kerr, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = status; return ret; }
-static errno_t create_empty_ccache(int fd, struct krb5_req *kr) +static errno_t create_empty_ccache(struct krb5_req *kr, int *_status) { int ret; int pam_status = PAM_SUCCESS; @@ -1418,11 +1362,7 @@ static errno_t create_empty_ccache(int fd, struct krb5_req *kr) pam_status = PAM_SYSTEM_ERR; }
- ret = sendresponse(fd, ret, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
@@ -1458,19 +1398,27 @@ static errno_t unpack_authtok(TALLOC_CTX *mem_ctx, struct sss_auth_token *tok, return ret; }
-static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, +static errno_t unpack_buffer(uint8_t *buf, size_t size, struct krb5_req *kr, uint32_t *offline) { size_t p = 0; uint32_t len; uint32_t validate; uint32_t different_realm; + struct pam_data *pd; errno_t ret;
DEBUG(SSSDBG_TRACE_LIBS, ("total buffer size: [%d]\n", size));
if (!offline || !kr) return EINVAL;
+ pd = talloc_zero(kr, struct pam_data); + if (pd == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); + return ENOMEM; + } + kr->pd = pd; + SAFEALIGN_COPY_UINT32_CHECK(&pd->cmd, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->uid, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->gid, buf + p, size, &p); @@ -1542,9 +1490,8 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, return EOK; }
-static int krb5_cleanup(void *ptr) +static int krb5_cleanup(struct krb5_req *kr) { - struct krb5_req *kr = talloc_get_type(ptr, struct krb5_req); if (kr == NULL) return EOK;
if (kr->options != NULL) { @@ -1562,9 +1509,6 @@ static int krb5_cleanup(void *ptr) if (kr->ctx != NULL) krb5_free_context(kr->ctx);
- if (kr->krb5_ctx != NULL) { - memset(kr->krb5_ctx, 0, sizeof(struct krb5_child_ctx)); - } memset(kr, 0, sizeof(struct krb5_req));
return EOK; @@ -1616,10 +1560,11 @@ done: return krberr; }
-static krb5_error_code check_fast_ccache(krb5_context ctx, const char *primary, +static krb5_error_code check_fast_ccache(TALLOC_CTX *mem_ctx, + krb5_context ctx, + const char *primary, const char *realm, const char *keytab_name, - TALLOC_CTX *mem_ctx, char **fast_ccname) { TALLOC_CTX *tmp_ctx = NULL; @@ -1716,120 +1661,159 @@ done: return kerr; }
-static errno_t -set_child_debugging(krb5_context ctx) +static errno_t k5c_recv_data(struct krb5_req *kr, int fd, uint32_t *offline) { + uint8_t buf[IN_BUF_SIZE]; + ssize_t len; + errno_t ret; + + errno = 0; + len = sss_atomic_read_s(fd, buf, IN_BUF_SIZE); + if (len == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + ("read failed [%d][%s].\n", ret, strerror(ret))); + return ret; + } + + ret = unpack_buffer(buf, len, kr, offline); + if (ret != EOK) { + DEBUG(1, ("unpack_buffer failed.\n")); + } + + return ret; +} + +static int k5c_setup_fast(struct krb5_req *kr, char *lifetime_str, bool demand) +{ + krb5_principal fast_princ_struct; + krb5_data *realm_data; + char *fast_principal_realm; + char *fast_principal; krb5_error_code kerr; + char *tmp_str;
- /* Set the global error context */ - krb5_error_ctx = ctx; + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_LIFETIME, lifetime_str));
- if (debug_level & SSSDBG_TRACE_ALL) { - kerr = sss_child_set_krb5_tracing(ctx); + tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); + if (tmp_str) { + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_FAST_PRINCIPAL, tmp_str)); + kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); if (kerr) { - KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); - return EIO; + DEBUG(1, ("krb5_parse_name failed.\n")); + return kerr; + } + kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, + KRB5_PRINCIPAL_UNPARSE_NO_REALM, + &tmp_str); + if (kerr) { + DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); + return kerr; + } + fast_principal = talloc_strdup(kr, tmp_str); + if (!fast_principal) { + DEBUG(1, ("talloc_strdup failed.\n")); + return KRB5KRB_ERR_GENERIC; + } + free(tmp_str); + realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); + fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); + if (!fast_principal_realm) { + DEBUG(1, ("talloc_asprintf failed.\n")); + return ENOMEM; + } + } else { + fast_principal_realm = kr->realm; + fast_principal = NULL; + } + + kerr = check_fast_ccache(kr, kr->ctx, fast_principal, fast_principal_realm, + kr->keytab, &kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("check_fast_ccache failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, + kr->options, + kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + if (demand) { + kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, + kr->options, + SSS_KRB5_FAST_REQUIRED); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; } }
return EOK; }
-static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) +static int k5c_setup(struct krb5_req *kr, uint32_t offline) { - krb5_error_code kerr = 0; + krb5_error_code kerr; char *lifetime_str; char *use_fast_str; - char *tmp_str; - krb5_data *realm_data; - krb5_principal fast_princ_struct; - char *fast_principal = NULL; - const char *fast_principal_realm = NULL; krb5_deltat lifetime;
- kr->krb5_ctx = talloc_zero(kr, struct krb5_child_ctx); - if (kr->krb5_ctx == NULL) { - DEBUG(1, ("talloc failed.\n")); - kerr = ENOMEM; - goto failed; - } - - kr->krb5_ctx->realm = getenv(SSSD_KRB5_REALM); - if (kr->krb5_ctx->realm == NULL) { + kr->realm = getenv(SSSD_KRB5_REALM); + if (kr->realm == NULL) { DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot read [%s] from environment.\n", SSSD_KRB5_REALM)); }
- switch(kr->pd->cmd) { - case SSS_PAM_AUTHENTICATE: - /* If we are offline, we need to create an empty ccache file */ - if (offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); - kr->child_req = create_empty_ccache; - } else { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); - kr->child_req = tgt_req_child; - } - break; - case SSS_PAM_CHAUTHTOK: - case SSS_PAM_CHAUTHTOK_PRELIM: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); - kr->child_req = changepw_child; - break; - case SSS_PAM_ACCT_MGMT: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); - kr->child_req = kuserok_child; - break; - case SSS_CMD_RENEW: - if (!offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); - kr->child_req = renew_tgt_child; - } else { - DEBUG(1, ("Cannot renew TGT while offline.\n")); - kerr = KRB5_KDC_UNREACH; - goto failed; - } - break; - default: - DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); - kerr = EINVAL; - goto failed; - } - kerr = krb5_init_context(&kr->ctx); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
- kerr = set_child_debugging(kr->ctx); - if (kerr != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set krb5_child debugging\n")); + /* Set the global error context */ + krb5_error_ctx = kr->ctx; + + if (debug_level & SSSDBG_TRACE_ALL) { + kerr = sss_child_set_krb5_tracing(kr->ctx); + if (kerr) { + KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); + return EIO; + } }
kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kerr = krb5_unparse_name(kr->ctx, kr->princ, &kr->name); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kr->creds = calloc(1, sizeof(krb5_creds)); if (kr->creds == NULL) { DEBUG(1, ("talloc_zero failed.\n")); - kerr = ENOMEM; - goto failed; + return ENOMEM; }
kerr = sss_krb5_get_init_creds_opt_alloc(kr->ctx, &kr->options); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_CHANGE_PASSWORD_PROMPT @@ -1839,7 +1823,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) krb5_get_init_creds_opt_set_change_password_prompt(kr->options, 0); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } #endif
@@ -1853,7 +1837,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_RENEWABLE_LIFETIME, lifetime_str)); @@ -1870,7 +1854,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_LIFETIME, lifetime_str)); @@ -1883,78 +1867,13 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) use_fast_str = getenv(SSSD_KRB5_USE_FAST); if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") == 0) { DEBUG(SSSDBG_CONF_SETTINGS, ("Not using FAST.\n")); - } else if (strcasecmp(use_fast_str, "try") == 0 || - strcasecmp(use_fast_str, "demand") == 0) { - - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_LIFETIME, lifetime_str)); - tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); - if (!tmp_str) { - fast_principal = NULL; - fast_principal_realm = kr->krb5_ctx->realm; - } else { - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_FAST_PRINCIPAL, lifetime_str)); - kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); - if (kerr) { - DEBUG(1, ("krb5_parse_name failed.\n")); - goto failed; - } - kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, - KRB5_PRINCIPAL_UNPARSE_NO_REALM, - &tmp_str); - if (kerr) { - DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); - goto failed; - } - fast_principal = talloc_strdup(kr, tmp_str); - if (!fast_principal) { - DEBUG(1, ("talloc_strdup failed.\n")); - kerr = KRB5KRB_ERR_GENERIC; - goto failed; - } - free(tmp_str); - realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); - fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); - if (!fast_principal_realm) { - DEBUG(1, ("talloc_asprintf failed.\n")); - goto failed; - } - } - - kerr = check_fast_ccache(kr->ctx, fast_principal, fast_principal_realm, kr->keytab, - kr, &kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("check_fast_ccache failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, - kr->options, - kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - if (strcasecmp(use_fast_str, "demand") == 0) { - kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, - kr->options, - SSS_KRB5_FAST_REQUIRED); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - } + } else if (strcasecmp(use_fast_str, "try") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, false); + } else if (strcasecmp(use_fast_str, "demand") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, true); } else { DEBUG(1, ("Unsupported value [%s] for krb5_use_fast.\n")); - kerr = EINVAL; - goto failed; + return EINVAL; } }
@@ -1969,24 +1888,18 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) * krb5_get_init_creds_opt_set_pa */
- return EOK; - -failed: - return kerr; }
int main(int argc, const char *argv[]) { - uint8_t *buf = NULL; - int ret; - ssize_t len = 0; - struct pam_data *pd = NULL; struct krb5_req *kr = NULL; uint32_t offline; int opt; poptContext pc; int debug_fd = -1; + int status; + int ret;
struct poptOption long_options[] = { POPT_AUTOHELP @@ -2019,10 +1932,16 @@ int main(int argc, const char *argv[])
DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid()); + kr = talloc_zero(NULL, struct krb5_req); + if (kr == NULL) { + DEBUG(1, ("talloc failed.\n")); + exit(-1); + } + + debug_prg_name = talloc_asprintf(kr, "[sssd[krb5_child[%d]]]", getpid()); if (!debug_prg_name) { DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf failed.\n")); - goto fail; + goto done; }
if (debug_fd != -1) { @@ -2034,65 +1953,68 @@ int main(int argc, const char *argv[])
DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child started.\n"));
- pd = talloc_zero(NULL, struct pam_data); - if (pd == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); - talloc_free(discard_const(debug_prg_name)); - goto fail; - } - talloc_steal(pd, debug_prg_name); - - buf = talloc_size(pd, sizeof(uint8_t)*IN_BUF_SIZE); - if (buf == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("malloc failed.\n")); - goto fail; - } - - errno = 0; - len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE); - if (len == -1) { - ret = errno; - DEBUG(SSSDBG_CRIT_FAILURE, ("read failed [%d][%s].\n", ret, strerror(ret))); - goto fail; + ret = k5c_recv_data(kr, STDIN_FILENO, &offline); + if (ret != EOK) { + goto done; }
close(STDIN_FILENO);
- kr = talloc_zero(pd, struct krb5_req); - if (kr == NULL) { - DEBUG(1, ("talloc failed.\n")); - goto fail; - } - talloc_set_destructor((TALLOC_CTX *) kr, krb5_cleanup); - kr->pd = pd; - - ret = unpack_buffer(buf, len, pd, kr, &offline); + ret = k5c_setup(kr, offline); if (ret != EOK) { - DEBUG(1, ("unpack_buffer failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child_setup failed.\n")); + goto done; }
- ret = krb5_child_setup(kr, offline); - if (ret != EOK) { - DEBUG(1, ("krb5_child_setup failed.\n")); - goto fail; + switch(kr->pd->cmd) { + case SSS_PAM_AUTHENTICATE: + /* If we are offline, we need to create an empty ccache file */ + if (offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); + ret = create_empty_ccache(kr, &status); + } else { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); + ret = tgt_req_child(kr, &status); + } + break; + case SSS_PAM_CHAUTHTOK: + case SSS_PAM_CHAUTHTOK_PRELIM: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); + ret = changepw_child(kr, &status); + break; + case SSS_PAM_ACCT_MGMT: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); + ret = kuserok_child(kr, &status); + break; + case SSS_CMD_RENEW: + if (!offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); + ret = renew_tgt_child(kr, &status); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot renew TGT while offline\n")); + ret = KRB5_KDC_UNREACH; + goto done; + } + break; + default: + DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); + ret = EINVAL; + goto done; }
- ret = kr->child_req(STDOUT_FILENO, kr); + ret = k5c_send_data(kr, STDOUT_FILENO, ret, status); if (ret != EOK) { - DEBUG(1, ("Child request failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send reply\n")); }
- DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); - close(STDOUT_FILENO); - talloc_free(pd); - - return 0; - -fail: - DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); - close(STDOUT_FILENO); - talloc_free(pd); - exit(-1); +done: + krb5_cleanup(kr); + talloc_free(kr); + if (ret == EOK) { + DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); + exit(0); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); + exit(-1); + } }
On 11/24/2012 05:39 AM, Simo Sorce wrote:
I also noticed I missed to send a pre-requisite style-fix patch that I had in my tree.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Hi, can you rebase one more time please?
Rebased as per PAvel's requests.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Use the standard 'done' label for exceptions. --- src/providers/krb5/krb5_child.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index d1a42d56f1cb7137f2e21b54f6959da2fec5f6d0..01da84021c4fbaa9033e7e6539b704e7067c5a32 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1089,7 +1089,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) ret, strerror(ret))); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { @@ -1100,7 +1100,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length); @@ -1115,7 +1115,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); if (kerr != 0) { pam_status = kerr_handle_error(kerr); - goto sendresponse; + goto done; }
sss_authtok_set_empty(&kr->pd->authtok); @@ -1126,7 +1126,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) "successful.\n")); krb5_free_cred_contents(kr->ctx, kr->creds); pam_status = PAM_SUCCESS; - goto sendresponse; + goto done; }
ret = sss_authtok_get_password(&kr->pd->newauthtok, &newpassword, NULL); @@ -1134,7 +1134,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) DEBUG(1, ("Failed to fetch new password [%d] %s.\n", ret, strerror(ret))); kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
memset(&result_code_string, 0, sizeof(krb5_data)); @@ -1145,7 +1145,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
if (kerr == KRB5_KDC_UNREACH) { pam_status = PAM_AUTHTOK_LOCK_BUSY; - goto sendresponse; + goto done; }
if (kerr != 0 || result_code != 0) { @@ -1191,7 +1191,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) }
pam_status = PAM_AUTHTOK_ERR; - goto sendresponse; + goto done; }
krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1202,7 +1202,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n")); @@ -1226,7 +1226,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_OP_FAILURE, ("Unknown authtok type\n")); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
kerr = get_and_save_tgt(kr, password); @@ -1248,7 +1248,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, @@ -1269,7 +1269,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n"));
The aim of this refactoring is to make the code readable and understandable. This code has grown organically over time and has becomed confused and baroque enough that understanding it's very simple flow had become very complex for the uninitiated. Complex flows easily hide nasty bugs.
Improvements: - Remove dead/unused data storage - Fix and simplify talloc hierarchy, use a memory context (kr) for the whole code and allocate kr->pd where it is filled up. - Rename some functions to create a better name space (easier for searching fucntions across the tree) - Streamline setup function, by spliting out fast setup in a subroutine. - Avoid confusing indirection in executng actual functions by not using the krb5_req child_req member. - Make main() flow s now simmetric, send abck data from the main function instead of delegating a reply to every inner function that implements a command.
Now the flow is evident from the main function: 1. read request 2. setup data 3. execute command 4. send reply back --- src/providers/krb5/krb5_child.c | 490 ++++++++++++++++---------------------- 1 files changed, 206 insertions(+), 284 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 01da84021c4fbaa9033e7e6539b704e7067c5a32..c53be4e67a5239eb20f6af986c2d3791b2fb3a6b 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -40,53 +40,16 @@
#define SSSD_KRB5_CHANGEPW_PRINCIPAL "kadmin/changepw"
-struct krb5_child_ctx { - /* opts taken from kinit */ - /* in seconds */ - krb5_deltat starttime; - krb5_deltat lifetime; - krb5_deltat rlife; - - int forwardable; - int proxiable; - int addresses; - - int not_forwardable; - int not_proxiable; - int no_addresses; - - int verbose; - - char* principal_name; - char* service_name; - char* keytab_name; - char* k5_cache_name; - char* k4_cache_name; - - action_type action; - - char *kdcip; - char *realm; - char *ccache_dir; - char *ccname_template; - int auth_timeout; -}; - struct krb5_req { krb5_context ctx; krb5_principal princ; char* name; krb5_creds *creds; krb5_get_init_creds_opt *options; - pid_t child_pid; - int read_from_child_fd; - int write_to_child_fd;
- struct be_req *req; struct pam_data *pd; - struct krb5_child_ctx *krb5_ctx; - errno_t (*child_req)(int fd, struct krb5_req *kr);
+ char *realm; char *ccname; char *keytab; bool validate; @@ -676,14 +639,14 @@ static struct response *prepare_response_message(struct krb5_req *kr, return resp; }
-static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status, - struct krb5_req *kr) +static errno_t k5c_send_data(struct krb5_req *kr, int fd, + krb5_error_code kerr, int status) { struct response *resp; size_t written; int ret;
- resp = prepare_response_message(kr, kerr, pam_status); + resp = prepare_response_message(kr, kerr, status); if (resp == NULL) { DEBUG(1, ("prepare_response_message failed.\n")); return ENOMEM; @@ -1063,7 +1026,7 @@ static int kerr_to_status(krb5_error_code kerr) return kerr_handle_error(kerr); }
-static errno_t changepw_child(int fd, struct krb5_req *kr) +static errno_t changepw_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1203,15 +1166,11 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t tgt_req_child(int fd, struct krb5_req *kr) +static errno_t tgt_req_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1270,18 +1229,13 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t kuserok_child(int fd, struct krb5_req *kr) +static errno_t kuserok_child(struct krb5_req *kr, int *_status) { krb5_boolean access_allowed; - int status; int ret; krb5_error_code kerr;
@@ -1295,7 +1249,7 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) "krb5_kuserok will most certainly fail.\n")); }
- kerr = krb5_set_default_realm(kr->ctx, kr->krb5_ctx->realm); + kerr = krb5_set_default_realm(kr->ctx, kr->realm); if (kerr != 0) { DEBUG(1, ("krb5_set_default_realm failed, " "krb5_kuserok may fail.\n")); @@ -1305,17 +1259,11 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_TRACE_LIBS, ("Access was %s\n", access_allowed ? "allowed" : "denied"));
- status = access_allowed ? 0 : 1; - - ret = sendresponse(fd, 0, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = access_allowed ? 0 : 1; return ret; }
-static errno_t renew_tgt_child(int fd, struct krb5_req *kr) +static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) { int ret; int status = PAM_AUTHTOK_ERR; @@ -1396,15 +1344,11 @@ done: krb5_cc_close(kr->ctx, ccache); }
- ret = sendresponse(fd, kerr, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = status; return ret; }
-static errno_t create_empty_ccache(int fd, struct krb5_req *kr) +static errno_t create_empty_ccache(struct krb5_req *kr, int *_status) { int ret; int pam_status = PAM_SUCCESS; @@ -1418,11 +1362,7 @@ static errno_t create_empty_ccache(int fd, struct krb5_req *kr) pam_status = PAM_SYSTEM_ERR; }
- ret = sendresponse(fd, ret, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
@@ -1458,19 +1398,27 @@ static errno_t unpack_authtok(TALLOC_CTX *mem_ctx, struct sss_auth_token *tok, return ret; }
-static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, +static errno_t unpack_buffer(uint8_t *buf, size_t size, struct krb5_req *kr, uint32_t *offline) { size_t p = 0; uint32_t len; uint32_t validate; uint32_t different_realm; + struct pam_data *pd; errno_t ret;
DEBUG(SSSDBG_TRACE_LIBS, ("total buffer size: [%d]\n", size));
if (!offline || !kr) return EINVAL;
+ pd = talloc_zero(kr, struct pam_data); + if (pd == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); + return ENOMEM; + } + kr->pd = pd; + SAFEALIGN_COPY_UINT32_CHECK(&pd->cmd, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->uid, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->gid, buf + p, size, &p); @@ -1542,9 +1490,8 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, return EOK; }
-static int krb5_cleanup(void *ptr) +static int krb5_cleanup(struct krb5_req *kr) { - struct krb5_req *kr = talloc_get_type(ptr, struct krb5_req); if (kr == NULL) return EOK;
if (kr->options != NULL) { @@ -1562,9 +1509,6 @@ static int krb5_cleanup(void *ptr) if (kr->ctx != NULL) krb5_free_context(kr->ctx);
- if (kr->krb5_ctx != NULL) { - memset(kr->krb5_ctx, 0, sizeof(struct krb5_child_ctx)); - } memset(kr, 0, sizeof(struct krb5_req));
return EOK; @@ -1616,10 +1560,11 @@ done: return krberr; }
-static krb5_error_code check_fast_ccache(krb5_context ctx, const char *primary, +static krb5_error_code check_fast_ccache(TALLOC_CTX *mem_ctx, + krb5_context ctx, + const char *primary, const char *realm, const char *keytab_name, - TALLOC_CTX *mem_ctx, char **fast_ccname) { TALLOC_CTX *tmp_ctx = NULL; @@ -1716,120 +1661,159 @@ done: return kerr; }
-static errno_t -set_child_debugging(krb5_context ctx) +static errno_t k5c_recv_data(struct krb5_req *kr, int fd, uint32_t *offline) { + uint8_t buf[IN_BUF_SIZE]; + ssize_t len; + errno_t ret; + + errno = 0; + len = sss_atomic_read_s(fd, buf, IN_BUF_SIZE); + if (len == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + ("read failed [%d][%s].\n", ret, strerror(ret))); + return ret; + } + + ret = unpack_buffer(buf, len, kr, offline); + if (ret != EOK) { + DEBUG(1, ("unpack_buffer failed.\n")); + } + + return ret; +} + +static int k5c_setup_fast(struct krb5_req *kr, char *lifetime_str, bool demand) +{ + krb5_principal fast_princ_struct; + krb5_data *realm_data; + char *fast_principal_realm; + char *fast_principal; krb5_error_code kerr; + char *tmp_str;
- /* Set the global error context */ - krb5_error_ctx = ctx; + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_LIFETIME, lifetime_str));
- if (debug_level & SSSDBG_TRACE_ALL) { - kerr = sss_child_set_krb5_tracing(ctx); + tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); + if (tmp_str) { + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_FAST_PRINCIPAL, tmp_str)); + kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); if (kerr) { - KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); - return EIO; + DEBUG(1, ("krb5_parse_name failed.\n")); + return kerr; + } + kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, + KRB5_PRINCIPAL_UNPARSE_NO_REALM, + &tmp_str); + if (kerr) { + DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); + return kerr; + } + fast_principal = talloc_strdup(kr, tmp_str); + if (!fast_principal) { + DEBUG(1, ("talloc_strdup failed.\n")); + return KRB5KRB_ERR_GENERIC; + } + free(tmp_str); + realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); + fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); + if (!fast_principal_realm) { + DEBUG(1, ("talloc_asprintf failed.\n")); + return ENOMEM; + } + } else { + fast_principal_realm = kr->realm; + fast_principal = NULL; + } + + kerr = check_fast_ccache(kr, kr->ctx, fast_principal, fast_principal_realm, + kr->keytab, &kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("check_fast_ccache failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, + kr->options, + kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + if (demand) { + kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, + kr->options, + SSS_KRB5_FAST_REQUIRED); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; } }
return EOK; }
-static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) +static int k5c_setup(struct krb5_req *kr, uint32_t offline) { - krb5_error_code kerr = 0; + krb5_error_code kerr; char *lifetime_str; char *use_fast_str; - char *tmp_str; - krb5_data *realm_data; - krb5_principal fast_princ_struct; - char *fast_principal = NULL; - const char *fast_principal_realm = NULL; krb5_deltat lifetime;
- kr->krb5_ctx = talloc_zero(kr, struct krb5_child_ctx); - if (kr->krb5_ctx == NULL) { - DEBUG(1, ("talloc failed.\n")); - kerr = ENOMEM; - goto failed; - } - - kr->krb5_ctx->realm = getenv(SSSD_KRB5_REALM); - if (kr->krb5_ctx->realm == NULL) { + kr->realm = getenv(SSSD_KRB5_REALM); + if (kr->realm == NULL) { DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot read [%s] from environment.\n", SSSD_KRB5_REALM)); }
- switch(kr->pd->cmd) { - case SSS_PAM_AUTHENTICATE: - /* If we are offline, we need to create an empty ccache file */ - if (offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); - kr->child_req = create_empty_ccache; - } else { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); - kr->child_req = tgt_req_child; - } - break; - case SSS_PAM_CHAUTHTOK: - case SSS_PAM_CHAUTHTOK_PRELIM: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); - kr->child_req = changepw_child; - break; - case SSS_PAM_ACCT_MGMT: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); - kr->child_req = kuserok_child; - break; - case SSS_CMD_RENEW: - if (!offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); - kr->child_req = renew_tgt_child; - } else { - DEBUG(1, ("Cannot renew TGT while offline.\n")); - kerr = KRB5_KDC_UNREACH; - goto failed; - } - break; - default: - DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); - kerr = EINVAL; - goto failed; - } - kerr = krb5_init_context(&kr->ctx); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
- kerr = set_child_debugging(kr->ctx); - if (kerr != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set krb5_child debugging\n")); + /* Set the global error context */ + krb5_error_ctx = kr->ctx; + + if (debug_level & SSSDBG_TRACE_ALL) { + kerr = sss_child_set_krb5_tracing(kr->ctx); + if (kerr) { + KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); + return EIO; + } }
kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kerr = krb5_unparse_name(kr->ctx, kr->princ, &kr->name); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kr->creds = calloc(1, sizeof(krb5_creds)); if (kr->creds == NULL) { DEBUG(1, ("talloc_zero failed.\n")); - kerr = ENOMEM; - goto failed; + return ENOMEM; }
kerr = sss_krb5_get_init_creds_opt_alloc(kr->ctx, &kr->options); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_CHANGE_PASSWORD_PROMPT @@ -1839,7 +1823,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) krb5_get_init_creds_opt_set_change_password_prompt(kr->options, 0); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } #endif
@@ -1853,7 +1837,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_RENEWABLE_LIFETIME, lifetime_str)); @@ -1870,7 +1854,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_LIFETIME, lifetime_str)); @@ -1883,78 +1867,13 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) use_fast_str = getenv(SSSD_KRB5_USE_FAST); if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") == 0) { DEBUG(SSSDBG_CONF_SETTINGS, ("Not using FAST.\n")); - } else if (strcasecmp(use_fast_str, "try") == 0 || - strcasecmp(use_fast_str, "demand") == 0) { - - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_LIFETIME, lifetime_str)); - tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); - if (!tmp_str) { - fast_principal = NULL; - fast_principal_realm = kr->krb5_ctx->realm; - } else { - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_FAST_PRINCIPAL, lifetime_str)); - kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); - if (kerr) { - DEBUG(1, ("krb5_parse_name failed.\n")); - goto failed; - } - kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, - KRB5_PRINCIPAL_UNPARSE_NO_REALM, - &tmp_str); - if (kerr) { - DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); - goto failed; - } - fast_principal = talloc_strdup(kr, tmp_str); - if (!fast_principal) { - DEBUG(1, ("talloc_strdup failed.\n")); - kerr = KRB5KRB_ERR_GENERIC; - goto failed; - } - free(tmp_str); - realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); - fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); - if (!fast_principal_realm) { - DEBUG(1, ("talloc_asprintf failed.\n")); - goto failed; - } - } - - kerr = check_fast_ccache(kr->ctx, fast_principal, fast_principal_realm, kr->keytab, - kr, &kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("check_fast_ccache failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, - kr->options, - kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - if (strcasecmp(use_fast_str, "demand") == 0) { - kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, - kr->options, - SSS_KRB5_FAST_REQUIRED); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - } + } else if (strcasecmp(use_fast_str, "try") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, false); + } else if (strcasecmp(use_fast_str, "demand") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, true); } else { DEBUG(1, ("Unsupported value [%s] for krb5_use_fast.\n")); - kerr = EINVAL; - goto failed; + return EINVAL; } }
@@ -1969,24 +1888,18 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) * krb5_get_init_creds_opt_set_pa */
- return EOK; - -failed: - return kerr; }
int main(int argc, const char *argv[]) { - uint8_t *buf = NULL; - int ret; - ssize_t len = 0; - struct pam_data *pd = NULL; struct krb5_req *kr = NULL; uint32_t offline; int opt; poptContext pc; int debug_fd = -1; + int status; + int ret;
struct poptOption long_options[] = { POPT_AUTOHELP @@ -2019,10 +1932,16 @@ int main(int argc, const char *argv[])
DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid()); + kr = talloc_zero(NULL, struct krb5_req); + if (kr == NULL) { + DEBUG(1, ("talloc failed.\n")); + exit(-1); + } + + debug_prg_name = talloc_asprintf(kr, "[sssd[krb5_child[%d]]]", getpid()); if (!debug_prg_name) { DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf failed.\n")); - goto fail; + goto done; }
if (debug_fd != -1) { @@ -2034,65 +1953,68 @@ int main(int argc, const char *argv[])
DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child started.\n"));
- pd = talloc_zero(NULL, struct pam_data); - if (pd == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); - talloc_free(discard_const(debug_prg_name)); - goto fail; - } - talloc_steal(pd, debug_prg_name); - - buf = talloc_size(pd, sizeof(uint8_t)*IN_BUF_SIZE); - if (buf == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("malloc failed.\n")); - goto fail; - } - - errno = 0; - len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE); - if (len == -1) { - ret = errno; - DEBUG(SSSDBG_CRIT_FAILURE, ("read failed [%d][%s].\n", ret, strerror(ret))); - goto fail; + ret = k5c_recv_data(kr, STDIN_FILENO, &offline); + if (ret != EOK) { + goto done; }
close(STDIN_FILENO);
- kr = talloc_zero(pd, struct krb5_req); - if (kr == NULL) { - DEBUG(1, ("talloc failed.\n")); - goto fail; - } - talloc_set_destructor((TALLOC_CTX *) kr, krb5_cleanup); - kr->pd = pd; - - ret = unpack_buffer(buf, len, pd, kr, &offline); + ret = k5c_setup(kr, offline); if (ret != EOK) { - DEBUG(1, ("unpack_buffer failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child_setup failed.\n")); + goto done; }
- ret = krb5_child_setup(kr, offline); - if (ret != EOK) { - DEBUG(1, ("krb5_child_setup failed.\n")); - goto fail; + switch(kr->pd->cmd) { + case SSS_PAM_AUTHENTICATE: + /* If we are offline, we need to create an empty ccache file */ + if (offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); + ret = create_empty_ccache(kr, &status); + } else { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); + ret = tgt_req_child(kr, &status); + } + break; + case SSS_PAM_CHAUTHTOK: + case SSS_PAM_CHAUTHTOK_PRELIM: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); + ret = changepw_child(kr, &status); + break; + case SSS_PAM_ACCT_MGMT: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); + ret = kuserok_child(kr, &status); + break; + case SSS_CMD_RENEW: + if (!offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); + ret = renew_tgt_child(kr, &status); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot renew TGT while offline\n")); + ret = KRB5_KDC_UNREACH; + goto done; + } + break; + default: + DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); + ret = EINVAL; + goto done; }
- ret = kr->child_req(STDOUT_FILENO, kr); + ret = k5c_send_data(kr, STDOUT_FILENO, ret, status); if (ret != EOK) { - DEBUG(1, ("Child request failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send reply\n")); }
- DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); - close(STDOUT_FILENO); - talloc_free(pd); - - return 0; - -fail: - DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); - close(STDOUT_FILENO); - talloc_free(pd); - exit(-1); +done: + krb5_cleanup(kr); + talloc_free(kr); + if (ret == EOK) { + DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); + exit(0); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); + exit(-1); + } }
On 12/02/2012 06:01 AM, Simo Sorce wrote:
Rebased as per PAvel's requests.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi, I am unable to apply these patches to current master. It says that some blob is missing. Do they require something else?
On Fri, 2013-01-11 at 09:46 +0100, Pavel Březina wrote:
On 12/02/2012 06:01 AM, Simo Sorce wrote:
Rebased as per PAvel's requests.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hi, I am unable to apply these patches to current master. It says that some blob is missing. Do they require something else?
I think they may have been dependent on the authtok patches. I will rebase and send again asap.
Simo.
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Use the standard 'done' label for exceptions. --- src/providers/krb5/krb5_child.c | 22 +++++++++++----------- 1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index d1a42d56f1cb7137f2e21b54f6959da2fec5f6d0..01da84021c4fbaa9033e7e6539b704e7067c5a32 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1089,7 +1089,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) ret, strerror(ret))); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
if (kr->pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM) { @@ -1100,7 +1100,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
sss_krb5_princ_realm(kr->ctx, kr->princ, &realm_name, &realm_length); @@ -1115,7 +1115,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) sss_krb5_get_init_creds_opt_free(kr->ctx, chagepw_options); if (kerr != 0) { pam_status = kerr_handle_error(kerr); - goto sendresponse; + goto done; }
sss_authtok_set_empty(&kr->pd->authtok); @@ -1126,7 +1126,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) "successful.\n")); krb5_free_cred_contents(kr->ctx, kr->creds); pam_status = PAM_SUCCESS; - goto sendresponse; + goto done; }
ret = sss_authtok_get_password(&kr->pd->newauthtok, &newpassword, NULL); @@ -1134,7 +1134,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) DEBUG(1, ("Failed to fetch new password [%d] %s.\n", ret, strerror(ret))); kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
memset(&result_code_string, 0, sizeof(krb5_data)); @@ -1145,7 +1145,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
if (kerr == KRB5_KDC_UNREACH) { pam_status = PAM_AUTHTOK_LOCK_BUSY; - goto sendresponse; + goto done; }
if (kerr != 0 || result_code != 0) { @@ -1191,7 +1191,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) }
pam_status = PAM_AUTHTOK_ERR; - goto sendresponse; + goto done; }
krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1202,7 +1202,7 @@ static errno_t changepw_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n")); @@ -1226,7 +1226,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_OP_FAILURE, ("Unknown authtok type\n")); pam_status = PAM_CRED_INSUFFICIENT; kerr = KRB5KRB_ERR_GENERIC; - goto sendresponse; + goto done; }
kerr = get_and_save_tgt(kr, password); @@ -1248,7 +1248,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) kerr = get_changepw_options(kr->ctx, &chagepw_options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, ("get_changepw_options failed.\n")); - goto sendresponse; + goto done; }
kerr = krb5_get_init_creds_password(kr->ctx, kr->creds, kr->princ, @@ -1269,7 +1269,7 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr)
pam_status = kerr_to_status(kerr);
-sendresponse: +done: ret = sendresponse(fd, kerr, pam_status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n"));
The aim of this refactoring is to make the code readable and understandable. This code has grown organically over time and has becomed confused and baroque enough that understanding it's very simple flow had become very complex for the uninitiated. Complex flows easily hide nasty bugs.
Improvements: - Remove dead/unused data storage - Fix and simplify talloc hierarchy, use a memory context (kr) for the whole code and allocate kr->pd where it is filled up. - Rename some functions to create a better name space (easier for searching fucntions across the tree) - Streamline setup function, by spliting out fast setup in a subroutine. - Avoid confusing indirection in executng actual functions by not using the krb5_req child_req member. - Make main() flow s now simmetric, send abck data from the main function instead of delegating a reply to every inner function that implements a command.
Now the flow is evident from the main function: 1. read request 2. setup data 3. execute command 4. send reply back --- src/providers/krb5/krb5_child.c | 490 ++++++++++++++++---------------------- 1 files changed, 206 insertions(+), 284 deletions(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index 01da84021c4fbaa9033e7e6539b704e7067c5a32..c53be4e67a5239eb20f6af986c2d3791b2fb3a6b 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -40,53 +40,16 @@
#define SSSD_KRB5_CHANGEPW_PRINCIPAL "kadmin/changepw"
-struct krb5_child_ctx { - /* opts taken from kinit */ - /* in seconds */ - krb5_deltat starttime; - krb5_deltat lifetime; - krb5_deltat rlife; - - int forwardable; - int proxiable; - int addresses; - - int not_forwardable; - int not_proxiable; - int no_addresses; - - int verbose; - - char* principal_name; - char* service_name; - char* keytab_name; - char* k5_cache_name; - char* k4_cache_name; - - action_type action; - - char *kdcip; - char *realm; - char *ccache_dir; - char *ccname_template; - int auth_timeout; -}; - struct krb5_req { krb5_context ctx; krb5_principal princ; char* name; krb5_creds *creds; krb5_get_init_creds_opt *options; - pid_t child_pid; - int read_from_child_fd; - int write_to_child_fd;
- struct be_req *req; struct pam_data *pd; - struct krb5_child_ctx *krb5_ctx; - errno_t (*child_req)(int fd, struct krb5_req *kr);
+ char *realm; char *ccname; char *keytab; bool validate; @@ -676,14 +639,14 @@ static struct response *prepare_response_message(struct krb5_req *kr, return resp; }
-static errno_t sendresponse(int fd, krb5_error_code kerr, int pam_status, - struct krb5_req *kr) +static errno_t k5c_send_data(struct krb5_req *kr, int fd, + krb5_error_code kerr, int status) { struct response *resp; size_t written; int ret;
- resp = prepare_response_message(kr, kerr, pam_status); + resp = prepare_response_message(kr, kerr, status); if (resp == NULL) { DEBUG(1, ("prepare_response_message failed.\n")); return ENOMEM; @@ -1063,7 +1026,7 @@ static int kerr_to_status(krb5_error_code kerr) return kerr_handle_error(kerr); }
-static errno_t changepw_child(int fd, struct krb5_req *kr) +static errno_t changepw_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1203,15 +1166,11 @@ static errno_t changepw_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t tgt_req_child(int fd, struct krb5_req *kr) +static errno_t tgt_req_child(struct krb5_req *kr, int *_status) { int ret; krb5_error_code kerr = 0; @@ -1270,18 +1229,13 @@ static errno_t tgt_req_child(int fd, struct krb5_req *kr) pam_status = kerr_to_status(kerr);
done: - ret = sendresponse(fd, kerr, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
-static errno_t kuserok_child(int fd, struct krb5_req *kr) +static errno_t kuserok_child(struct krb5_req *kr, int *_status) { krb5_boolean access_allowed; - int status; int ret; krb5_error_code kerr;
@@ -1295,7 +1249,7 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) "krb5_kuserok will most certainly fail.\n")); }
- kerr = krb5_set_default_realm(kr->ctx, kr->krb5_ctx->realm); + kerr = krb5_set_default_realm(kr->ctx, kr->realm); if (kerr != 0) { DEBUG(1, ("krb5_set_default_realm failed, " "krb5_kuserok may fail.\n")); @@ -1305,17 +1259,11 @@ static errno_t kuserok_child(int fd, struct krb5_req *kr) DEBUG(SSSDBG_TRACE_LIBS, ("Access was %s\n", access_allowed ? "allowed" : "denied"));
- status = access_allowed ? 0 : 1; - - ret = sendresponse(fd, 0, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = access_allowed ? 0 : 1; return ret; }
-static errno_t renew_tgt_child(int fd, struct krb5_req *kr) +static errno_t renew_tgt_child(struct krb5_req *kr, int *_status) { int ret; int status = PAM_AUTHTOK_ERR; @@ -1396,15 +1344,11 @@ done: krb5_cc_close(kr->ctx, ccache); }
- ret = sendresponse(fd, kerr, status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = status; return ret; }
-static errno_t create_empty_ccache(int fd, struct krb5_req *kr) +static errno_t create_empty_ccache(struct krb5_req *kr, int *_status) { int ret; int pam_status = PAM_SUCCESS; @@ -1418,11 +1362,7 @@ static errno_t create_empty_ccache(int fd, struct krb5_req *kr) pam_status = PAM_SYSTEM_ERR; }
- ret = sendresponse(fd, ret, pam_status, kr); - if (ret != EOK) { - DEBUG(1, ("sendresponse failed.\n")); - } - + *_status = pam_status; return ret; }
@@ -1458,19 +1398,27 @@ static errno_t unpack_authtok(TALLOC_CTX *mem_ctx, struct sss_auth_token *tok, return ret; }
-static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, +static errno_t unpack_buffer(uint8_t *buf, size_t size, struct krb5_req *kr, uint32_t *offline) { size_t p = 0; uint32_t len; uint32_t validate; uint32_t different_realm; + struct pam_data *pd; errno_t ret;
DEBUG(SSSDBG_TRACE_LIBS, ("total buffer size: [%d]\n", size));
if (!offline || !kr) return EINVAL;
+ pd = talloc_zero(kr, struct pam_data); + if (pd == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); + return ENOMEM; + } + kr->pd = pd; + SAFEALIGN_COPY_UINT32_CHECK(&pd->cmd, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->uid, buf + p, size, &p); SAFEALIGN_COPY_UINT32_CHECK(&kr->gid, buf + p, size, &p); @@ -1542,9 +1490,8 @@ static errno_t unpack_buffer(uint8_t *buf, size_t size, struct pam_data *pd, return EOK; }
-static int krb5_cleanup(void *ptr) +static int krb5_cleanup(struct krb5_req *kr) { - struct krb5_req *kr = talloc_get_type(ptr, struct krb5_req); if (kr == NULL) return EOK;
if (kr->options != NULL) { @@ -1562,9 +1509,6 @@ static int krb5_cleanup(void *ptr) if (kr->ctx != NULL) krb5_free_context(kr->ctx);
- if (kr->krb5_ctx != NULL) { - memset(kr->krb5_ctx, 0, sizeof(struct krb5_child_ctx)); - } memset(kr, 0, sizeof(struct krb5_req));
return EOK; @@ -1616,10 +1560,11 @@ done: return krberr; }
-static krb5_error_code check_fast_ccache(krb5_context ctx, const char *primary, +static krb5_error_code check_fast_ccache(TALLOC_CTX *mem_ctx, + krb5_context ctx, + const char *primary, const char *realm, const char *keytab_name, - TALLOC_CTX *mem_ctx, char **fast_ccname) { TALLOC_CTX *tmp_ctx = NULL; @@ -1716,120 +1661,159 @@ done: return kerr; }
-static errno_t -set_child_debugging(krb5_context ctx) +static errno_t k5c_recv_data(struct krb5_req *kr, int fd, uint32_t *offline) { + uint8_t buf[IN_BUF_SIZE]; + ssize_t len; + errno_t ret; + + errno = 0; + len = sss_atomic_read_s(fd, buf, IN_BUF_SIZE); + if (len == -1) { + ret = errno; + DEBUG(SSSDBG_CRIT_FAILURE, + ("read failed [%d][%s].\n", ret, strerror(ret))); + return ret; + } + + ret = unpack_buffer(buf, len, kr, offline); + if (ret != EOK) { + DEBUG(1, ("unpack_buffer failed.\n")); + } + + return ret; +} + +static int k5c_setup_fast(struct krb5_req *kr, char *lifetime_str, bool demand) +{ + krb5_principal fast_princ_struct; + krb5_data *realm_data; + char *fast_principal_realm; + char *fast_principal; krb5_error_code kerr; + char *tmp_str;
- /* Set the global error context */ - krb5_error_ctx = ctx; + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_LIFETIME, lifetime_str));
- if (debug_level & SSSDBG_TRACE_ALL) { - kerr = sss_child_set_krb5_tracing(ctx); + tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); + if (tmp_str) { + DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", + SSSD_KRB5_FAST_PRINCIPAL, tmp_str)); + kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); if (kerr) { - KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); - return EIO; + DEBUG(1, ("krb5_parse_name failed.\n")); + return kerr; + } + kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, + KRB5_PRINCIPAL_UNPARSE_NO_REALM, + &tmp_str); + if (kerr) { + DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); + return kerr; + } + fast_principal = talloc_strdup(kr, tmp_str); + if (!fast_principal) { + DEBUG(1, ("talloc_strdup failed.\n")); + return KRB5KRB_ERR_GENERIC; + } + free(tmp_str); + realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); + fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); + if (!fast_principal_realm) { + DEBUG(1, ("talloc_asprintf failed.\n")); + return ENOMEM; + } + } else { + fast_principal_realm = kr->realm; + fast_principal = NULL; + } + + kerr = check_fast_ccache(kr, kr->ctx, fast_principal, fast_principal_realm, + kr->keytab, &kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("check_fast_ccache failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, + kr->options, + kr->fast_ccname); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; + } + + if (demand) { + kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, + kr->options, + SSS_KRB5_FAST_REQUIRED); + if (kerr != 0) { + DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " + "failed.\n")); + KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); + return kerr; } }
return EOK; }
-static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) +static int k5c_setup(struct krb5_req *kr, uint32_t offline) { - krb5_error_code kerr = 0; + krb5_error_code kerr; char *lifetime_str; char *use_fast_str; - char *tmp_str; - krb5_data *realm_data; - krb5_principal fast_princ_struct; - char *fast_principal = NULL; - const char *fast_principal_realm = NULL; krb5_deltat lifetime;
- kr->krb5_ctx = talloc_zero(kr, struct krb5_child_ctx); - if (kr->krb5_ctx == NULL) { - DEBUG(1, ("talloc failed.\n")); - kerr = ENOMEM; - goto failed; - } - - kr->krb5_ctx->realm = getenv(SSSD_KRB5_REALM); - if (kr->krb5_ctx->realm == NULL) { + kr->realm = getenv(SSSD_KRB5_REALM); + if (kr->realm == NULL) { DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot read [%s] from environment.\n", SSSD_KRB5_REALM)); }
- switch(kr->pd->cmd) { - case SSS_PAM_AUTHENTICATE: - /* If we are offline, we need to create an empty ccache file */ - if (offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); - kr->child_req = create_empty_ccache; - } else { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); - kr->child_req = tgt_req_child; - } - break; - case SSS_PAM_CHAUTHTOK: - case SSS_PAM_CHAUTHTOK_PRELIM: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); - kr->child_req = changepw_child; - break; - case SSS_PAM_ACCT_MGMT: - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); - kr->child_req = kuserok_child; - break; - case SSS_CMD_RENEW: - if (!offline) { - DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); - kr->child_req = renew_tgt_child; - } else { - DEBUG(1, ("Cannot renew TGT while offline.\n")); - kerr = KRB5_KDC_UNREACH; - goto failed; - } - break; - default: - DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); - kerr = EINVAL; - goto failed; - } - kerr = krb5_init_context(&kr->ctx); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
- kerr = set_child_debugging(kr->ctx); - if (kerr != EOK) { - DEBUG(SSSDBG_MINOR_FAILURE, ("Cannot set krb5_child debugging\n")); + /* Set the global error context */ + krb5_error_ctx = kr->ctx; + + if (debug_level & SSSDBG_TRACE_ALL) { + kerr = sss_child_set_krb5_tracing(kr->ctx); + if (kerr) { + KRB5_CHILD_DEBUG(SSSDBG_MINOR_FAILURE, kerr); + return EIO; + } }
kerr = krb5_parse_name(kr->ctx, kr->upn, &kr->princ); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kerr = krb5_unparse_name(kr->ctx, kr->princ, &kr->name); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
kr->creds = calloc(1, sizeof(krb5_creds)); if (kr->creds == NULL) { DEBUG(1, ("talloc_zero failed.\n")); - kerr = ENOMEM; - goto failed; + return ENOMEM; }
kerr = sss_krb5_get_init_creds_opt_alloc(kr->ctx, &kr->options); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; }
#ifdef HAVE_KRB5_GET_INIT_CREDS_OPT_SET_CHANGE_PASSWORD_PROMPT @@ -1839,7 +1823,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) krb5_get_init_creds_opt_set_change_password_prompt(kr->options, 0); if (kerr != 0) { KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } #endif
@@ -1853,7 +1837,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_RENEWABLE_LIFETIME, lifetime_str)); @@ -1870,7 +1854,7 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) DEBUG(1, ("krb5_string_to_deltat failed for [%s].\n", lifetime_str)); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; + return kerr; } DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", SSSD_KRB5_LIFETIME, lifetime_str)); @@ -1883,78 +1867,13 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) use_fast_str = getenv(SSSD_KRB5_USE_FAST); if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") == 0) { DEBUG(SSSDBG_CONF_SETTINGS, ("Not using FAST.\n")); - } else if (strcasecmp(use_fast_str, "try") == 0 || - strcasecmp(use_fast_str, "demand") == 0) { - - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_LIFETIME, lifetime_str)); - tmp_str = getenv(SSSD_KRB5_FAST_PRINCIPAL); - if (!tmp_str) { - fast_principal = NULL; - fast_principal_realm = kr->krb5_ctx->realm; - } else { - DEBUG(SSSDBG_CONF_SETTINGS, ("%s is set to [%s]\n", - SSSD_KRB5_FAST_PRINCIPAL, lifetime_str)); - kerr = krb5_parse_name(kr->ctx, tmp_str, &fast_princ_struct); - if (kerr) { - DEBUG(1, ("krb5_parse_name failed.\n")); - goto failed; - } - kerr = sss_krb5_unparse_name_flags(kr->ctx, fast_princ_struct, - KRB5_PRINCIPAL_UNPARSE_NO_REALM, - &tmp_str); - if (kerr) { - DEBUG(1, ("sss_krb5_unparse_name_flags failed.\n")); - goto failed; - } - fast_principal = talloc_strdup(kr, tmp_str); - if (!fast_principal) { - DEBUG(1, ("talloc_strdup failed.\n")); - kerr = KRB5KRB_ERR_GENERIC; - goto failed; - } - free(tmp_str); - realm_data = krb5_princ_realm(kr->ctx, fast_princ_struct); - fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, realm_data->data); - if (!fast_principal_realm) { - DEBUG(1, ("talloc_asprintf failed.\n")); - goto failed; - } - } - - kerr = check_fast_ccache(kr->ctx, fast_principal, fast_principal_realm, kr->keytab, - kr, &kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("check_fast_ccache failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - kerr = sss_krb5_get_init_creds_opt_set_fast_ccache_name(kr->ctx, - kr->options, - kr->fast_ccname); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_ccache_name " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - - if (strcasecmp(use_fast_str, "demand") == 0) { - kerr = sss_krb5_get_init_creds_opt_set_fast_flags(kr->ctx, - kr->options, - SSS_KRB5_FAST_REQUIRED); - if (kerr != 0) { - DEBUG(1, ("sss_krb5_get_init_creds_opt_set_fast_flags " - "failed.\n")); - KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); - goto failed; - } - } + } else if (strcasecmp(use_fast_str, "try") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, false); + } else if (strcasecmp(use_fast_str, "demand") == 0) { + kerr = k5c_setup_fast(kr, lifetime_str, true); } else { DEBUG(1, ("Unsupported value [%s] for krb5_use_fast.\n")); - kerr = EINVAL; - goto failed; + return EINVAL; } }
@@ -1969,24 +1888,18 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) * krb5_get_init_creds_opt_set_pa */
- return EOK; - -failed: - return kerr; }
int main(int argc, const char *argv[]) { - uint8_t *buf = NULL; - int ret; - ssize_t len = 0; - struct pam_data *pd = NULL; struct krb5_req *kr = NULL; uint32_t offline; int opt; poptContext pc; int debug_fd = -1; + int status; + int ret;
struct poptOption long_options[] = { POPT_AUTOHELP @@ -2019,10 +1932,16 @@ int main(int argc, const char *argv[])
DEBUG_INIT(debug_level);
- debug_prg_name = talloc_asprintf(NULL, "[sssd[krb5_child[%d]]]", getpid()); + kr = talloc_zero(NULL, struct krb5_req); + if (kr == NULL) { + DEBUG(1, ("talloc failed.\n")); + exit(-1); + } + + debug_prg_name = talloc_asprintf(kr, "[sssd[krb5_child[%d]]]", getpid()); if (!debug_prg_name) { DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_asprintf failed.\n")); - goto fail; + goto done; }
if (debug_fd != -1) { @@ -2034,65 +1953,68 @@ int main(int argc, const char *argv[])
DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child started.\n"));
- pd = talloc_zero(NULL, struct pam_data); - if (pd == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("talloc_zero failed.\n")); - talloc_free(discard_const(debug_prg_name)); - goto fail; - } - talloc_steal(pd, debug_prg_name); - - buf = talloc_size(pd, sizeof(uint8_t)*IN_BUF_SIZE); - if (buf == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, ("malloc failed.\n")); - goto fail; - } - - errno = 0; - len = sss_atomic_read_s(STDIN_FILENO, buf, IN_BUF_SIZE); - if (len == -1) { - ret = errno; - DEBUG(SSSDBG_CRIT_FAILURE, ("read failed [%d][%s].\n", ret, strerror(ret))); - goto fail; + ret = k5c_recv_data(kr, STDIN_FILENO, &offline); + if (ret != EOK) { + goto done; }
close(STDIN_FILENO);
- kr = talloc_zero(pd, struct krb5_req); - if (kr == NULL) { - DEBUG(1, ("talloc failed.\n")); - goto fail; - } - talloc_set_destructor((TALLOC_CTX *) kr, krb5_cleanup); - kr->pd = pd; - - ret = unpack_buffer(buf, len, pd, kr, &offline); + ret = k5c_setup(kr, offline); if (ret != EOK) { - DEBUG(1, ("unpack_buffer failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child_setup failed.\n")); + goto done; }
- ret = krb5_child_setup(kr, offline); - if (ret != EOK) { - DEBUG(1, ("krb5_child_setup failed.\n")); - goto fail; + switch(kr->pd->cmd) { + case SSS_PAM_AUTHENTICATE: + /* If we are offline, we need to create an empty ccache file */ + if (offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform offline auth\n")); + ret = create_empty_ccache(kr, &status); + } else { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform online auth\n")); + ret = tgt_req_child(kr, &status); + } + break; + case SSS_PAM_CHAUTHTOK: + case SSS_PAM_CHAUTHTOK_PRELIM: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform password change\n")); + ret = changepw_child(kr, &status); + break; + case SSS_PAM_ACCT_MGMT: + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform account management\n")); + ret = kuserok_child(kr, &status); + break; + case SSS_CMD_RENEW: + if (!offline) { + DEBUG(SSSDBG_TRACE_FUNC, ("Will perform ticket renewal\n")); + ret = renew_tgt_child(kr, &status); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("Cannot renew TGT while offline\n")); + ret = KRB5_KDC_UNREACH; + goto done; + } + break; + default: + DEBUG(1, ("PAM command [%d] not supported.\n", kr->pd->cmd)); + ret = EINVAL; + goto done; }
- ret = kr->child_req(STDOUT_FILENO, kr); + ret = k5c_send_data(kr, STDOUT_FILENO, ret, status); if (ret != EOK) { - DEBUG(1, ("Child request failed.\n")); - goto fail; + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to send reply\n")); }
- DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); - close(STDOUT_FILENO); - talloc_free(pd); - - return 0; - -fail: - DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); - close(STDOUT_FILENO); - talloc_free(pd); - exit(-1); +done: + krb5_cleanup(kr); + talloc_free(kr); + if (ret == EOK) { + DEBUG(SSSDBG_TRACE_FUNC, ("krb5_child completed successfully\n")); + exit(0); + } else { + DEBUG(SSSDBG_CRIT_FAILURE, ("krb5_child failed!\n")); + exit(-1); + } }
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Otherwise it seems good and working, but I'm not sure how to test FAST. Anyone can help me?
On Mon, Jan 14, 2013 at 11:33:46AM +0100, Pavel Březina wrote:
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Nack, the new krb5_child allows login with *any* password. Seems like we're not handling error codes properly:
[[sssd[krb5_child[15317]]]] [sss_child_krb5_trace_cb] (0x4000): [15317] 1361959672.153511: Received error from KDC: -1765328353/Decrypt integrity check failed [[sssd[krb5_child[15317]]]] [get_and_save_tgt] (0x0020): 941: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [kerr_handle_error] (0x0020): 994: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [prepare_response_message] (0x0400): Building response for result [0] [[sssd[krb5_child[15317]]]] [pack_response_packet] (0x2000): response packet size: [54] [[sssd[krb5_child[15317]]]] [k5c_send_data] (0x4000): Response sent. [[sssd[krb5_child[15317]]]] [main] (0x0400): krb5_child completed successfully [sssd[be[IPA]]] [read_pipe_handler] (0x0400): EOF received, client finished [sssd[be[IPA]]] [parse_krb5_child_response] (0x1000): child response [0][3][42]. ^^^^^^ The response should not be 0 here [sssd[be[IPA]]] [fo_set_port_status] (0x0100): Marking port 0 of server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]] [set_server_common_status] (0x0100): Marking server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]][cc_dir_cache_for_princ] (0x0400): No principal for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc [sssd[be[IPA]]] [krb5_auth_done] (0x0020): No ccache for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc?
etc..but the auth succeeds. I have also tested your branch with the new Kerberos auth codes and there access is denied properly. But I really don't want to push a security problem to master knowingly, sorry.
Otherwise it seems good and working, but I'm not sure how to test FAST. Anyone can help me? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Wed, 2013-02-27 at 11:24 +0100, Jakub Hrozek wrote:
On Mon, Jan 14, 2013 at 11:33:46AM +0100, Pavel Březina wrote:
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Nack, the new krb5_child allows login with *any* password.
How lovely ... Maybe I should test with a wrong password next time :-)
Seems like we're not handling error codes properly:
[[sssd[krb5_child[15317]]]] [sss_child_krb5_trace_cb] (0x4000): [15317] 1361959672.153511: Received error from KDC: -1765328353/Decrypt integrity check failed [[sssd[krb5_child[15317]]]] [get_and_save_tgt] (0x0020): 941: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [kerr_handle_error] (0x0020): 994: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [prepare_response_message] (0x0400): Building response for result [0] [[sssd[krb5_child[15317]]]] [pack_response_packet] (0x2000): response packet size: [54] [[sssd[krb5_child[15317]]]] [k5c_send_data] (0x4000): Response sent. [[sssd[krb5_child[15317]]]] [main] (0x0400): krb5_child completed successfully [sssd[be[IPA]]] [read_pipe_handler] (0x0400): EOF received, client finished [sssd[be[IPA]]] [parse_krb5_child_response] (0x1000): child response [0][3][42]. ^^^^^^ The response should not be 0 here [sssd[be[IPA]]] [fo_set_port_status] (0x0100): Marking port 0 of server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]] [set_server_common_status] (0x0100): Marking server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]][cc_dir_cache_for_princ] (0x0400): No principal for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc [sssd[be[IPA]]] [krb5_auth_done] (0x0020): No ccache for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc?
etc..but the auth succeeds. I have also tested your branch with the new Kerberos auth codes and there access is denied properly. But I really don't want to push a security problem to master knowingly, sorry.
Not a problem, let me check what is going on and send revised patches.
Thanks a lot for thorough testing, it would be nice if we could find this type of errors using the new cmocka test stuff.
Simo.
On Wed, 2013-02-27 at 09:13 -0500, Simo Sorce wrote:
On Wed, 2013-02-27 at 11:24 +0100, Jakub Hrozek wrote:
On Mon, Jan 14, 2013 at 11:33:46AM +0100, Pavel Březina wrote:
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Nack, the new krb5_child allows login with *any* password.
How lovely ... Maybe I should test with a wrong password next time :-)
Seems like we're not handling error codes properly:
[[sssd[krb5_child[15317]]]] [sss_child_krb5_trace_cb] (0x4000): [15317] 1361959672.153511: Received error from KDC: -1765328353/Decrypt integrity check failed [[sssd[krb5_child[15317]]]] [get_and_save_tgt] (0x0020): 941: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [kerr_handle_error] (0x0020): 994: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [prepare_response_message] (0x0400): Building response for result [0] [[sssd[krb5_child[15317]]]] [pack_response_packet] (0x2000): response packet size: [54] [[sssd[krb5_child[15317]]]] [k5c_send_data] (0x4000): Response sent. [[sssd[krb5_child[15317]]]] [main] (0x0400): krb5_child completed successfully [sssd[be[IPA]]] [read_pipe_handler] (0x0400): EOF received, client finished [sssd[be[IPA]]] [parse_krb5_child_response] (0x1000): child response [0][3][42]. ^^^^^^ The response should not be 0 here [sssd[be[IPA]]] [fo_set_port_status] (0x0100): Marking port 0 of server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]] [set_server_common_status] (0x0100): Marking server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]][cc_dir_cache_for_princ] (0x0400): No principal for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc [sssd[be[IPA]]] [krb5_auth_done] (0x0020): No ccache for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc?
etc..but the auth succeeds. I have also tested your branch with the new Kerberos auth codes and there access is denied properly. But I really don't want to push a security problem to master knowingly, sorry.
Not a problem, let me check what is going on and send revised patches.
Thanks a lot for thorough testing, it would be nice if we could find this type of errors using the new cmocka test stuff.
Simo.
Ok there was an error in which return variable was used in some functions which was canceled out by the refactoring of the error code returns.
Attached find rebased patch that fixes this problem.
Simo.
On Wed, Feb 27, 2013 at 09:50:22AM -0500, Simo Sorce wrote:
On Wed, 2013-02-27 at 09:13 -0500, Simo Sorce wrote:
On Wed, 2013-02-27 at 11:24 +0100, Jakub Hrozek wrote:
On Mon, Jan 14, 2013 at 11:33:46AM +0100, Pavel Březina wrote:
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Nack, the new krb5_child allows login with *any* password.
How lovely ... Maybe I should test with a wrong password next time :-)
Seems like we're not handling error codes properly:
[[sssd[krb5_child[15317]]]] [sss_child_krb5_trace_cb] (0x4000): [15317] 1361959672.153511: Received error from KDC: -1765328353/Decrypt integrity check failed [[sssd[krb5_child[15317]]]] [get_and_save_tgt] (0x0020): 941: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [kerr_handle_error] (0x0020): 994: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [prepare_response_message] (0x0400): Building response for result [0] [[sssd[krb5_child[15317]]]] [pack_response_packet] (0x2000): response packet size: [54] [[sssd[krb5_child[15317]]]] [k5c_send_data] (0x4000): Response sent. [[sssd[krb5_child[15317]]]] [main] (0x0400): krb5_child completed successfully [sssd[be[IPA]]] [read_pipe_handler] (0x0400): EOF received, client finished [sssd[be[IPA]]] [parse_krb5_child_response] (0x1000): child response [0][3][42]. ^^^^^^ The response should not be 0 here [sssd[be[IPA]]] [fo_set_port_status] (0x0100): Marking port 0 of server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]] [set_server_common_status] (0x0100): Marking server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]][cc_dir_cache_for_princ] (0x0400): No principal for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc [sssd[be[IPA]]] [krb5_auth_done] (0x0020): No ccache for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc?
etc..but the auth succeeds. I have also tested your branch with the new Kerberos auth codes and there access is denied properly. But I really don't want to push a security problem to master knowingly, sorry.
Not a problem, let me check what is going on and send revised patches.
Thanks a lot for thorough testing, it would be nice if we could find this type of errors using the new cmocka test stuff.
Simo.
Ok there was an error in which return variable was used in some functions which was canceled out by the refactoring of the error code returns.
Attached find rebased patch that fixes this problem.
Simo.
Ack to both now.
On Thu, Feb 28, 2013 at 12:50:50PM +0100, Jakub Hrozek wrote:
On Wed, Feb 27, 2013 at 09:50:22AM -0500, Simo Sorce wrote:
On Wed, 2013-02-27 at 09:13 -0500, Simo Sorce wrote:
On Wed, 2013-02-27 at 11:24 +0100, Jakub Hrozek wrote:
On Mon, Jan 14, 2013 at 11:33:46AM +0100, Pavel Březina wrote:
On 01/12/2013 09:27 PM, Simo Sorce wrote:
Rebased on top of current master.
Simo.
Simo Sorce (2): krb5_child style fix Refactor krb5 child
src/providers/krb5/krb5_child.c | 512 +++++++++++++++++---------------------- 1 files changed, 217 insertions(+), 295 deletions(-)
Patch 1 ack.
Patch 2 nack:
src/providers/krb5/krb5_child.c: In function ‘main’: src/providers/krb5/krb5_child.c:2013:8: warning: ‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
$ echo $CFLAGS -m64 -mtune=generic -O3 -fstack-protector-all -Wall -Wextra -Wno-sign-compare -Wno-unused-parameter
Nack, the new krb5_child allows login with *any* password.
How lovely ... Maybe I should test with a wrong password next time :-)
Seems like we're not handling error codes properly:
[[sssd[krb5_child[15317]]]] [sss_child_krb5_trace_cb] (0x4000): [15317] 1361959672.153511: Received error from KDC: -1765328353/Decrypt integrity check failed [[sssd[krb5_child[15317]]]] [get_and_save_tgt] (0x0020): 941: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [kerr_handle_error] (0x0020): 994: [-1765328353][Decrypt integrity check failed] [[sssd[krb5_child[15317]]]] [prepare_response_message] (0x0400): Building response for result [0] [[sssd[krb5_child[15317]]]] [pack_response_packet] (0x2000): response packet size: [54] [[sssd[krb5_child[15317]]]] [k5c_send_data] (0x4000): Response sent. [[sssd[krb5_child[15317]]]] [main] (0x0400): krb5_child completed successfully [sssd[be[IPA]]] [read_pipe_handler] (0x0400): EOF received, client finished [sssd[be[IPA]]] [parse_krb5_child_response] (0x1000): child response [0][3][42]. ^^^^^^ The response should not be 0 here [sssd[be[IPA]]] [fo_set_port_status] (0x0100): Marking port 0 of server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]] [set_server_common_status] (0x0100): Marking server 'vm-101.idm.lab.bos.redhat.com' as 'working' [sssd[be[IPA]]][cc_dir_cache_for_princ] (0x0400): No principal for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc [sssd[be[IPA]]] [krb5_auth_done] (0x0020): No ccache for user@IDM.LAB.BOS.REDHAT.COM in DIR:/run/user/782400060/krb5cc?
etc..but the auth succeeds. I have also tested your branch with the new Kerberos auth codes and there access is denied properly. But I really don't want to push a security problem to master knowingly, sorry.
Not a problem, let me check what is going on and send revised patches.
Thanks a lot for thorough testing, it would be nice if we could find this type of errors using the new cmocka test stuff.
Simo.
Ok there was an error in which return variable was used in some functions which was canceled out by the refactoring of the error code returns.
Attached find rebased patch that fixes this problem.
Simo.
Ack to both now.
Pushed both to master.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Simo Sorce