URL: https://github.com/SSSD/sssd/pull/69 Author: sumit-bose Title: #69: krb5: Use command line arguments instead env vars for krb5_child Action: opened
PR body: """ The first patch resolves https://fedorahosted.org/sssd/ticket/697. The third patch adds some checks for functions I added or extended. As a result I found two memory leaks which are fixed by the second patch. """ To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/69/head:pr69 git checkout pr69
From fb40c4d9811c0a718f2464167ddf79063142ced1 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 28 Oct 2016 21:29:45 +0200 Subject: [PATCH 1/3] krb5: Use command line arguments instead env vars for krb5_child Resolves https://fedorahosted.org/sssd/ticket/697 --- src/providers/krb5/krb5_child.c | 124 ++++++++++++++++++------------ src/providers/krb5/krb5_child_handler.c | 132 ++++++++++++++++++++++++++++++-- src/providers/krb5/krb5_common.c | 91 +++++++++------------- src/providers/krb5/krb5_common.h | 27 ++++--- src/providers/krb5/krb5_init_shared.c | 5 +- 5 files changed, 256 insertions(+), 123 deletions(-) diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index df94bc4..9fe3da9 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -48,6 +48,15 @@ enum k5c_fast_opt { K5C_FAST_DEMAND, }; +struct cli_opts { + char *realm; + char *lifetime; + char *rtime; + char *use_fast_str; + char *fast_principal; + bool canonicalize; +}; + struct krb5_req { krb5_context ctx; krb5_principal princ; @@ -81,73 +90,68 @@ struct krb5_req { uid_t fast_uid; gid_t fast_gid; + + struct cli_opts *cli_opts; }; static krb5_context krb5_error_ctx; #define KRB5_CHILD_DEBUG(level, error) KRB5_DEBUG(level, krb5_error_ctx, error) -static krb5_error_code set_lifetime_options(krb5_get_init_creds_opt *options) +static krb5_error_code set_lifetime_options(struct cli_opts *cli_opts, + krb5_get_init_creds_opt *options) { - char *lifetime_str; krb5_error_code kerr; krb5_deltat lifetime; - lifetime_str = getenv(SSSD_KRB5_RENEWABLE_LIFETIME); - if (lifetime_str == NULL) { - DEBUG(SSSDBG_CONF_SETTINGS, "Cannot read [%s] from environment.\n", - SSSD_KRB5_RENEWABLE_LIFETIME); + if (cli_opts->rtime == NULL) { + DEBUG(SSSDBG_CONF_SETTINGS, + "No specific renewable lifetime requested.\n"); /* Unset option flag to make sure defaults from krb5.conf are used. */ options->flags &= ~(KRB5_GET_INIT_CREDS_OPT_RENEW_LIFE); } else { - kerr = krb5_string_to_deltat(lifetime_str, &lifetime); + kerr = krb5_string_to_deltat(cli_opts->rtime, &lifetime); if (kerr != 0) { DEBUG(SSSDBG_CRIT_FAILURE, - "krb5_string_to_deltat failed for [%s].\n", - lifetime_str); + "krb5_string_to_deltat failed for [%s].\n", cli_opts->rtime); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); return kerr; } - DEBUG(SSSDBG_CONF_SETTINGS, "%s is set to [%s]\n", - SSSD_KRB5_RENEWABLE_LIFETIME, lifetime_str); + DEBUG(SSSDBG_CONF_SETTINGS, "Renewable lifetime is set to [%s]\n", + cli_opts->rtime); krb5_get_init_creds_opt_set_renew_life(options, lifetime); } - lifetime_str = getenv(SSSD_KRB5_LIFETIME); - if (lifetime_str == NULL) { - DEBUG(SSSDBG_CONF_SETTINGS, "Cannot read [%s] from environment.\n", - SSSD_KRB5_LIFETIME); + if (cli_opts->lifetime == NULL) { + DEBUG(SSSDBG_CONF_SETTINGS, "No specific lifetime requested.\n"); /* Unset option flag to make sure defaults from krb5.conf are used. */ options->flags &= ~(KRB5_GET_INIT_CREDS_OPT_TKT_LIFE); } else { - kerr = krb5_string_to_deltat(lifetime_str, &lifetime); + kerr = krb5_string_to_deltat(cli_opts->lifetime, &lifetime); if (kerr != 0) { DEBUG(SSSDBG_CRIT_FAILURE, "krb5_string_to_deltat failed for [%s].\n", - lifetime_str); + cli_opts->lifetime); KRB5_CHILD_DEBUG(SSSDBG_CRIT_FAILURE, kerr); return kerr; } - DEBUG(SSSDBG_CONF_SETTINGS, - "%s is set to [%s]\n", SSSD_KRB5_LIFETIME, lifetime_str); + DEBUG(SSSDBG_CONF_SETTINGS, "Lifetime is set to [%s]\n", + cli_opts->lifetime); krb5_get_init_creds_opt_set_tkt_life(options, lifetime); } return 0; } -static void set_canonicalize_option(krb5_get_init_creds_opt *opts) +static void set_canonicalize_option(struct cli_opts *cli_opts, + krb5_get_init_creds_opt *opts) { int canonicalize = 0; - char *tmp_str; - tmp_str = getenv(SSSD_KRB5_CANONICALIZE); - if (tmp_str != NULL && strcasecmp(tmp_str, "true") == 0) { - canonicalize = 1; - } - DEBUG(SSSDBG_CONF_SETTINGS, "%s is set to [%s]\n", - SSSD_KRB5_CANONICALIZE, tmp_str ? tmp_str : "not set"); + canonicalize = cli_opts->canonicalize ? 1 : 0; + DEBUG(SSSDBG_CONF_SETTINGS, "Canonicalization is set to [%s]\n", + cli_opts->canonicalize ? "true" : "false"); sss_krb5_get_init_creds_opt_set_canonicalize(opts, canonicalize); } @@ -160,18 +164,19 @@ static void set_changepw_options(krb5_get_init_creds_opt *options) krb5_get_init_creds_opt_set_tkt_life(options, 5*60); } -static void revert_changepw_options(krb5_get_init_creds_opt *options) +static void revert_changepw_options(struct cli_opts *cli_opts, + krb5_get_init_creds_opt *options) { krb5_error_code kerr; - set_canonicalize_option(options); + set_canonicalize_option(cli_opts, options); /* Currently we do not set forwardable and proxiable explicitly, the flags * must be removed so that libkrb5 can take the defaults from krb5.conf */ options->flags &= ~(KRB5_GET_INIT_CREDS_OPT_FORWARDABLE); options->flags &= ~(KRB5_GET_INIT_CREDS_OPT_PROXIABLE); - kerr = set_lifetime_options(options); + kerr = set_lifetime_options(cli_opts, options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, "set_lifetime_options failed.\n"); } @@ -1218,6 +1223,7 @@ static krb5_error_code validate_tgt(struct krb5_req *kr) } static krb5_error_code get_and_save_tgt_with_keytab(krb5_context ctx, + struct cli_opts *cli_opts, krb5_principal princ, krb5_keytab keytab, char *ccname) @@ -1232,7 +1238,7 @@ static krb5_error_code get_and_save_tgt_with_keytab(krb5_context ctx, krb5_get_init_creds_opt_set_address_list(&options, NULL); krb5_get_init_creds_opt_set_forwardable(&options, 0); krb5_get_init_creds_opt_set_proxiable(&options, 0); - set_canonicalize_option(&options); + set_canonicalize_option(cli_opts, &options); kerr = krb5_get_init_creds_keytab(ctx, &creds, princ, keytab, 0, NULL, &options); @@ -1582,7 +1588,7 @@ static errno_t changepw_child(struct krb5_req *kr, bool prelim) /* We changed some of the gic options for the password change, now we have * to change them back to get a fresh TGT. */ - revert_changepw_options(kr->options); + revert_changepw_options(kr->cli_opts, kr->options); ret = sss_authtok_set_password(kr->pd->authtok, newpassword, 0); if (ret != EOK) { @@ -2053,6 +2059,7 @@ static krb5_error_code check_fast_ccache(TALLOC_CTX *mem_ctx, krb5_context ctx, uid_t fast_uid, gid_t fast_gid, + struct cli_opts *cli_opts, const char *primary, const char *realm, const char *keytab_name, @@ -2149,7 +2156,7 @@ static krb5_error_code check_fast_ccache(TALLOC_CTX *mem_ctx, DEBUG(SSSDBG_TRACE_INTERNAL, "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(), getegid()); - kerr = get_and_save_tgt_with_keytab(ctx, client_princ, + kerr = get_and_save_tgt_with_keytab(ctx, cli_opts, client_princ, keytab, ccname); if (kerr != 0) { DEBUG(SSSDBG_CRIT_FAILURE, @@ -2255,14 +2262,14 @@ static int k5c_setup_fast(struct krb5_req *kr, bool demand) char *fast_principal_realm; char *fast_principal; krb5_error_code kerr; - char *tmp_str; + char *tmp_str = NULL; char *new_ccname; - 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 (kr->cli_opts->fast_principal) { + DEBUG(SSSDBG_CONF_SETTINGS, "Fast principal is set to [%s]\n", + kr->cli_opts->fast_principal); + kerr = krb5_parse_name(kr->ctx, kr->cli_opts->fast_principal, + &fast_princ_struct); if (kerr) { DEBUG(SSSDBG_CRIT_FAILURE, "krb5_parse_name failed.\n"); return kerr; @@ -2281,7 +2288,8 @@ static int k5c_setup_fast(struct krb5_req *kr, bool demand) } 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); + fast_principal_realm = talloc_asprintf(kr, "%.*s", realm_data->length, + realm_data->data); if (!fast_principal_realm) { DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n"); return ENOMEM; @@ -2292,6 +2300,7 @@ static int k5c_setup_fast(struct krb5_req *kr, bool demand) } kerr = check_fast_ccache(kr, kr->ctx, kr->fast_uid, kr->fast_gid, + kr->cli_opts, fast_principal, fast_principal_realm, kr->keytab, &kr->fast_ccname); if (kerr != 0) { @@ -2336,12 +2345,11 @@ static int k5c_setup_fast(struct krb5_req *kr, bool demand) return EOK; } -static errno_t check_use_fast(enum k5c_fast_opt *_fast_val) +static errno_t check_use_fast(const char *use_fast_str, + enum k5c_fast_opt *_fast_val) { - char *use_fast_str; enum k5c_fast_opt fast_val; - 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"); fast_val = K5C_FAST_NEVER; @@ -2560,14 +2568,14 @@ static int k5c_setup(struct krb5_req *kr, uint32_t offline) krb5_get_init_creds_opt_set_change_password_prompt(kr->options, 0); #endif - kerr = set_lifetime_options(kr->options); + kerr = set_lifetime_options(kr->cli_opts, kr->options); if (kerr != 0) { DEBUG(SSSDBG_OP_FAILURE, "set_lifetime_options failed.\n"); return kerr; } if (!offline) { - set_canonicalize_option(kr->options); + set_canonicalize_option(kr->cli_opts, kr->options); } /* TODO: set options, e.g. @@ -2591,10 +2599,9 @@ static krb5_error_code privileged_krb5_setup(struct krb5_req *kr, int ret; char *mem_keytab; - kr->realm = getenv(SSSD_KRB5_REALM); + kr->realm = kr->cli_opts->realm; if (kr->realm == NULL) { - DEBUG(SSSDBG_MINOR_FAILURE, - "Cannot read [%s] from environment.\n", SSSD_KRB5_REALM); + DEBUG(SSSDBG_MINOR_FAILURE, "Realm not available.\n"); } kerr = krb5_init_context(&kr->ctx); @@ -2609,7 +2616,7 @@ static krb5_error_code privileged_krb5_setup(struct krb5_req *kr, return kerr; } - ret = check_use_fast(&kr->fast_val); + ret = check_use_fast(kr->cli_opts->use_fast_str, &kr->fast_val); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "check_use_fast failed.\n"); return ret; @@ -2691,6 +2698,7 @@ int main(int argc, const char *argv[]) krb5_error_code kerr; uid_t fast_uid; gid_t fast_gid; + struct cli_opts cli_opts = { 0 }; struct poptOption long_options[] = { POPT_AUTOHELP @@ -2709,15 +2717,32 @@ int main(int argc, const char *argv[]) _("The user to create FAST ccache as"), NULL}, {"fast-ccache-gid", 0, POPT_ARG_INT, &fast_gid, 0, _("The group to create FAST ccache as"), NULL}, + {"realm", 0, POPT_ARG_STRING, &cli_opts.realm, 0, + _("Kerberos realm to use"), NULL}, + {"lifetime", 0, POPT_ARG_STRING, &cli_opts.lifetime, 0, + _("Requested lifetime of the ticket"), NULL}, + {"renewable-lifetime", 0, POPT_ARG_STRING, &cli_opts.rtime, 0, + _("Requested renewable lifetime of the ticket"), NULL}, + {"use-fast", 0, POPT_ARG_STRING, &cli_opts.use_fast_str, 0, + _("FAST options ('never', 'try', 'demand')"), NULL}, + {"use-principal", 0, POPT_ARG_STRING, &cli_opts.fast_principal, 0, + _("Specifies the server principal to use for FAST"), NULL}, + {"canonicalize", 0, POPT_ARG_NONE, NULL, 'C', + _("Requests canonicalization of the principal name"), NULL}, POPT_TABLEEND }; /* Set debug level to invalid value so we can decide if -d 0 was used. */ debug_level = SSSDBG_INVALID; + cli_opts.canonicalize = false; + pc = poptGetContext(argv[0], argc, argv, long_options, 0); while((opt = poptGetNextOpt(pc)) != -1) { switch(opt) { + case 'C': + cli_opts.canonicalize = true; + break; default: fprintf(stderr, "\nInvalid option %s: %s\n\n", poptBadOption(pc, 0), poptStrerror(opt)); @@ -2757,6 +2782,7 @@ int main(int argc, const char *argv[]) kr->fast_uid = fast_uid; kr->fast_gid = fast_gid; + kr->cli_opts = &cli_opts; ret = k5c_recv_data(kr, STDIN_FILENO, &offline); if (ret != EOK) { diff --git a/src/providers/krb5/krb5_child_handler.c b/src/providers/krb5/krb5_child_handler.c index 1eec726..648f3c7 100644 --- a/src/providers/krb5/krb5_child_handler.c +++ b/src/providers/krb5/krb5_child_handler.c @@ -273,21 +273,138 @@ static errno_t activate_child_timeout_handler(struct tevent_req *req, return EOK; } +errno_t set_extra_args(TALLOC_CTX *mem_ctx, struct krb5_ctx *krb5_ctx, + const char ***krb5_child_extra_args) +{ + const char **extra_args; + size_t c = 0; + int ret; + + if (krb5_ctx == NULL || krb5_child_extra_args == NULL) { + return EINVAL; + } + + extra_args = talloc_zero_array(mem_ctx, const char *, 9); + if (extra_args == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_zero_array failed.\n"); + return ENOMEM; + } + + extra_args[c] = talloc_asprintf(extra_args, "--fast-ccache-uid=%"SPRIuid, + getuid()); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + + extra_args[c] = talloc_asprintf(extra_args, "--fast-ccache-gid=%"SPRIgid, + getgid()); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + + if (krb5_ctx->realm != NULL) { + extra_args[c] = talloc_asprintf(extra_args, "--realm=%s", + krb5_ctx->realm); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + } + + if (krb5_ctx->lifetime_str != NULL) { + extra_args[c] = talloc_asprintf(extra_args, "--lifetime=%s", + krb5_ctx->lifetime_str); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + } + + if (krb5_ctx->rlife_str != NULL) { + extra_args[c] = talloc_asprintf(extra_args, "--renewable-lifetime=%s", + krb5_ctx->rlife_str); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + } + + if (krb5_ctx->use_fast_str != NULL) { + extra_args[c] = talloc_asprintf(extra_args, "--use-fast=%s", + krb5_ctx->use_fast_str); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + + if (krb5_ctx->fast_principal != NULL) { + extra_args[c] = talloc_asprintf(extra_args, "--fast-principal=%s", + krb5_ctx->fast_principal); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + } + } + + if (krb5_ctx->canonicalize) { + extra_args[c] = talloc_strdup(extra_args, "--canonicalize"); + if (extra_args[c] == NULL) { + DEBUG(SSSDBG_OP_FAILURE, "talloc_strdup failed.\n"); + ret = ENOMEM; + goto done; + } + c++; + } + + extra_args[c] = NULL; + + *krb5_child_extra_args = extra_args; + + ret = EOK; + +done: + + if (ret != EOK) { + talloc_free(extra_args); + } + + return ret; +} + static errno_t fork_child(struct tevent_req *req) { int pipefd_to_child[2] = PIPE_INIT; int pipefd_from_child[2] = PIPE_INIT; pid_t pid; errno_t ret; + const char **krb5_child_extra_args; struct handle_child_state *state = tevent_req_data(req, struct handle_child_state); - const char *k5c_extra_args[3]; - k5c_extra_args[0] = talloc_asprintf(state, "--fast-ccache-uid=%"SPRIuid, getuid()); - k5c_extra_args[1] = talloc_asprintf(state, "--fast-ccache-gid=%"SPRIgid, getgid()); - k5c_extra_args[2] = NULL; - if (k5c_extra_args[0] == NULL || k5c_extra_args[1] == NULL) { - return ENOMEM; + if (state->kr->krb5_ctx->krb5_child_extra_args == NULL) { + ret = set_extra_args(state, state->kr->krb5_ctx, + &krb5_child_extra_args); + if (ret != EOK) { + DEBUG(SSSDBG_OP_FAILURE, "set_extra_args failed.\n"); + goto fail; + } } ret = pipe(pipefd_from_child); @@ -311,7 +428,8 @@ static errno_t fork_child(struct tevent_req *req) exec_child_ex(state, pipefd_to_child, pipefd_from_child, KRB5_CHILD, state->kr->krb5_ctx->child_debug_fd, - k5c_extra_args, false, STDIN_FILENO, STDOUT_FILENO); + state->kr->krb5_ctx->krb5_child_extra_args, false, + STDIN_FILENO, STDOUT_FILENO); /* We should never get here */ DEBUG(SSSDBG_CRIT_FAILURE, "BUG: Could not exec KRB5 child\n"); diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c index 208a003..9117de4 100644 --- a/src/providers/krb5/krb5_common.c +++ b/src/providers/krb5/krb5_common.c @@ -38,31 +38,38 @@ #include <profile.h> #endif -errno_t check_and_export_lifetime(struct dp_option *opts, const int opt_id, - const char *env_name) +static errno_t check_lifetime(struct dp_option *opts, const int opt_id, + TALLOC_CTX *mem_ctx, char **lifetime_str) { int ret; - char *str; + char *str = NULL; krb5_deltat lifetime; - bool free_str = false; str = dp_opt_get_string(opts, opt_id); if (str == NULL || *str == '\0') { DEBUG(SSSDBG_FUNC_DATA, "No lifetime configured.\n"); + *lifetime_str = NULL; return EOK; } if (isdigit(str[strlen(str)-1])) { - str = talloc_asprintf(opts, "%ss", str); + str = talloc_asprintf(mem_ctx, "%ss", str); if (str == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed\n"); - return ENOMEM; + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf failed.\n"); + ret = ENOMEM; + goto done; } - free_str = true; ret = dp_opt_set_string(opts, opt_id, str); if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "dp_opt_set_string failed\n"); + DEBUG(SSSDBG_CRIT_FAILURE, "dp_opt_set_string failed.\n"); + goto done; + } + } else { + str = talloc_strdup(mem_ctx, str); + if (str == NULL) { + DEBUG(SSSDBG_CRIT_FAILURE, "talloc_strdup failed.\n"); + ret = ENOMEM; goto done; } } @@ -74,17 +81,12 @@ errno_t check_and_export_lifetime(struct dp_option *opts, const int opt_id, goto done; } - ret = setenv(env_name, str, 1); - if (ret != EOK) { - ret = errno; - DEBUG(SSSDBG_OP_FAILURE, "setenv [%s] failed.\n", env_name); - goto done; - } + *lifetime_str = str; ret = EOK; done: - if (free_str) { + if (ret != EOK) { talloc_free(str); } @@ -157,18 +159,19 @@ static void sss_check_cc_template(const char *cc_template) } } -errno_t check_and_export_options(struct dp_option *opts, - struct sss_domain_info *dom, - struct krb5_ctx *krb5_ctx) +errno_t krb5_check_options(struct dp_option *opts, struct sss_domain_info *dom, + struct krb5_ctx *krb5_ctx) { TALLOC_CTX *tmp_ctx = NULL; int ret; const char *realm; const char *dummy; - char *use_fast_str; - char *fast_principal; char *ccname; + if (opts == NULL || dom == NULL || krb5_ctx == NULL) { + return EINVAL; + } + tmp_ctx = talloc_new(NULL); if (!tmp_ctx) { ret = ENOMEM; @@ -185,15 +188,14 @@ errno_t check_and_export_options(struct dp_option *opts, realm = dom->name; } - ret = setenv(SSSD_KRB5_REALM, realm, 1); - if (ret != EOK) { + krb5_ctx->realm = talloc_strdup(krb5_ctx, realm); + if (krb5_ctx->realm == NULL) { DEBUG(SSSDBG_OP_FAILURE, - "setenv %s failed, authentication might fail.\n", - SSSD_KRB5_REALM); + "Failed to set realm, krb5_child might not work as expected.\n"); } - ret = check_and_export_lifetime(opts, KRB5_RENEWABLE_LIFETIME, - SSSD_KRB5_RENEWABLE_LIFETIME); + ret = check_lifetime(opts, KRB5_RENEWABLE_LIFETIME, krb5_ctx, + &krb5_ctx->rlife_str); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to check value of krb5_renewable_lifetime. [%d][%s]\n", @@ -201,8 +203,8 @@ errno_t check_and_export_options(struct dp_option *opts, goto done; } - ret = check_and_export_lifetime(opts, KRB5_LIFETIME, - SSSD_KRB5_LIFETIME); + ret = check_lifetime(opts, KRB5_LIFETIME, krb5_ctx, + &krb5_ctx->lifetime_str); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "Failed to check value of krb5_lifetime. [%d][%s]\n", @@ -211,29 +213,17 @@ errno_t check_and_export_options(struct dp_option *opts, } - use_fast_str = dp_opt_get_string(opts, KRB5_USE_FAST); - if (use_fast_str != NULL) { - ret = check_fast(use_fast_str, &krb5_ctx->use_fast); + krb5_ctx->use_fast_str = dp_opt_get_cstring(opts, KRB5_USE_FAST); + if (krb5_ctx->use_fast_str != NULL) { + ret = check_fast(krb5_ctx->use_fast_str, &krb5_ctx->use_fast); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "check_fast failed.\n"); goto done; } if (krb5_ctx->use_fast) { - ret = setenv(SSSD_KRB5_USE_FAST, use_fast_str, 1); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "setenv [%s] failed.\n", SSSD_KRB5_USE_FAST); - } else { - fast_principal = dp_opt_get_string(opts, KRB5_FAST_PRINCIPAL); - if (fast_principal != NULL) { - ret = setenv(SSSD_KRB5_FAST_PRINCIPAL, fast_principal, 1); - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "setenv [%s] failed.\n", SSSD_KRB5_FAST_PRINCIPAL); - } - } - } + krb5_ctx->fast_principal = dp_opt_get_cstring(opts, + KRB5_FAST_PRINCIPAL); } } @@ -241,15 +231,10 @@ errno_t check_and_export_options(struct dp_option *opts, * enterprise principal in an AS request but requires the canonicalize * flags to be set. To be on the safe side we always enable * canonicalization if enterprise principals are used. */ + krb5_ctx->canonicalize = false; if (dp_opt_get_bool(opts, KRB5_CANONICALIZE) || dp_opt_get_bool(opts, KRB5_USE_ENTERPRISE_PRINCIPAL)) { - ret = setenv(SSSD_KRB5_CANONICALIZE, "true", 1); - } else { - ret = setenv(SSSD_KRB5_CANONICALIZE, "false", 1); - } - if (ret != EOK) { - DEBUG(SSSDBG_OP_FAILURE, - "setenv [%s] failed.\n", SSSD_KRB5_CANONICALIZE); + krb5_ctx->canonicalize = true; } dummy = dp_opt_get_cstring(opts, KRB5_KDC); diff --git a/src/providers/krb5/krb5_common.h b/src/providers/krb5/krb5_common.h index 3bd5ed1..ace9332 100644 --- a/src/providers/krb5/krb5_common.h +++ b/src/providers/krb5/krb5_common.h @@ -33,14 +33,6 @@ #include "util/util.h" #include "util/sss_krb5.h" -#define SSSD_KRB5_KDC "SSSD_KRB5_KDC" -#define SSSD_KRB5_REALM "SSSD_KRB5_REALM" -#define SSSD_KRB5_RENEWABLE_LIFETIME "SSSD_KRB5_RENEWABLE_LIFETIME" -#define SSSD_KRB5_LIFETIME "SSSD_KRB5_LIFETIME" -#define SSSD_KRB5_USE_FAST "SSSD_KRB5_USE_FAST" -#define SSSD_KRB5_FAST_PRINCIPAL "SSSD_KRB5_FAST_PRINCIPAL" -#define SSSD_KRB5_CANONICALIZE "SSSD_KRB5_CANONICALIZE" - #define KDCINFO_TMPL PUBCONF_PATH"/kdcinfo.%s" #define KPASSWDINFO_TMPL PUBCONF_PATH"/kpasswdinfo.%s" @@ -100,7 +92,9 @@ struct krb5_ctx { /* in seconds */ krb5_deltat starttime; krb5_deltat lifetime; + char *lifetime_str; krb5_deltat rlife; + char *rlife_str; int forwardable; int proxiable; @@ -136,6 +130,15 @@ struct krb5_ctx { enum krb5_config_type config_type; struct map_id_name_to_krb_primary *name_to_primary; + + char *realm; + + const char *use_fast_str; + const char *fast_principal; + + bool canonicalize; + + const char **krb5_child_extra_args; }; struct remove_info_files_ctx { @@ -145,9 +148,8 @@ struct remove_info_files_ctx { const char *kpasswd_service_name; }; -errno_t check_and_export_options(struct dp_option *opts, - struct sss_domain_info *dom, - struct krb5_ctx *krb5_ctx); +errno_t krb5_check_options(struct dp_option *opts, struct sss_domain_info *dom, + struct krb5_ctx *krb5_ctx); errno_t krb5_try_kdcip(struct confdb_ctx *cdb, const char *conf_path, struct dp_option *opts, int opt_id); @@ -221,4 +223,7 @@ krb5_error_code copy_keytab_into_memory(TALLOC_CTX *mem_ctx, krb5_context kctx, const char *inp_keytab_file, char **_mem_name, krb5_keytab *_mem_keytab); + +errno_t set_extra_args(TALLOC_CTX *mem_ctx, struct krb5_ctx *krb5_ctx, + const char ***krb5_child_extra_args); #endif /* __KRB5_COMMON_H__ */ diff --git a/src/providers/krb5/krb5_init_shared.c b/src/providers/krb5/krb5_init_shared.c index c8fd859..64f11b4 100644 --- a/src/providers/krb5/krb5_init_shared.c +++ b/src/providers/krb5/krb5_init_shared.c @@ -64,10 +64,9 @@ errno_t krb5_child_init(struct krb5_ctx *krb5_auth_ctx, } } - ret = check_and_export_options(krb5_auth_ctx->opts, bectx->domain, - krb5_auth_ctx); + ret = krb5_check_options(krb5_auth_ctx->opts, bectx->domain, krb5_auth_ctx); if (ret != EOK) { - DEBUG(SSSDBG_CRIT_FAILURE, "check_and_export_opts failed.\n"); + DEBUG(SSSDBG_CRIT_FAILURE, "krb5_check_options failed.\n"); goto done; } From ad32aba6da5e05b3e74b051a21f40fa96565d617 Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Fri, 4 Nov 2016 09:49:21 +0100 Subject: [PATCH 2/3] krb5: fix two memory leaks --- src/providers/krb5/krb5_common.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/providers/krb5/krb5_common.c b/src/providers/krb5/krb5_common.c index 9117de4..97ec04e 100644 --- a/src/providers/krb5/krb5_common.c +++ b/src/providers/krb5/krb5_common.c @@ -113,6 +113,7 @@ static errno_t sss_get_system_ccname_template(TALLOC_CTX *mem_ctx, ret = profile_get_string(p, "libdefaults", "default_ccache_name", NULL, NULL, &value); + profile_release(p); if (ret) goto done; if (!value) { @@ -361,13 +362,7 @@ errno_t krb5_get_options(TALLOC_CTX *memctx, struct confdb_ctx *cdb, int ret; struct dp_option *opts; - opts = talloc_zero(memctx, struct dp_option); - if (opts == NULL) { - DEBUG(SSSDBG_CRIT_FAILURE, "talloc_zero failed.\n"); - return ENOMEM; - } - - ret = dp_get_options(opts, cdb, conf_path, default_krb5_opts, + ret = dp_get_options(memctx, cdb, conf_path, default_krb5_opts, KRB5_OPTS, &opts); if (ret != EOK) { DEBUG(SSSDBG_CRIT_FAILURE, "dp_get_options failed.\n"); From 5aea5475950e24349a17c1e73f4cc4886fc2f88c Mon Sep 17 00:00:00 2001 From: Sumit Bose <sb...@redhat.com> Date: Thu, 3 Nov 2016 16:22:04 +0100 Subject: [PATCH 3/3] krb5: add tests for common functions --- Makefile.am | 27 ++++ src/tests/cmocka/test_krb5_common.c | 297 ++++++++++++++++++++++++++++++++++++ 2 files changed, 324 insertions(+) create mode 100644 src/tests/cmocka/test_krb5_common.c diff --git a/Makefile.am b/Makefile.am index 6432876..271b6c0 100644 --- a/Makefile.am +++ b/Makefile.am @@ -268,6 +268,7 @@ if HAVE_CMOCKA test_dp_builtin \ test_ipa_dn \ simple-access-tests \ + krb5_common_test \ $(NULL) if HAVE_LIBRESOLV @@ -3081,6 +3082,32 @@ simple_access_tests_LDADD = \ libdlopen_test_providers.la \ $(NULL) +krb5_common_test_SOURCES = \ + src/tests/cmocka/test_krb5_common.c \ + src/providers/krb5/krb5_child_handler.c \ + src/providers/krb5/krb5_common.c \ + src/providers/krb5/krb5_opts.c \ + src/providers/data_provider_fo.c \ + src/providers/data_provider_opts.c \ + src/providers/data_provider_callbacks.c \ + src/util/sss_krb5.c \ + $(SSSD_FAILOVER_OBJ) \ + $(NULL) +krb5_common_test_CFLAGS = \ + $(KRB5_CFLAGS) \ + $(AM_CFLAGS) \ + $(NULL) +krb5_common_test_LDFLAGS = \ + $(NULL) +krb5_common_test_LDADD = \ + $(CMOCKA_LIBS) \ + $(KRB5_LIBS) \ + $(CARES_LIBS) \ + $(SSSD_LIBS) \ + $(SSSD_INTERNAL_LTLIBS) \ + libsss_test_common.la \ + $(NULL) + endif # HAVE_CMOCKA noinst_PROGRAMS = pam_test_client diff --git a/src/tests/cmocka/test_krb5_common.c b/src/tests/cmocka/test_krb5_common.c new file mode 100644 index 0000000..18ab2fe --- /dev/null +++ b/src/tests/cmocka/test_krb5_common.c @@ -0,0 +1,297 @@ +/* + SSSD + + krb5_common - Test for some krb5 utility functions + + Authors: + Sumit Bose <sb...@redhat.com> + + Copyright (C) 2016 Red Hat + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. +*/ + +#include <stdarg.h> +#include <stddef.h> +#include <setjmp.h> +#include <cmocka.h> +#include <popt.h> +#include <stdbool.h> + +#include "tests/cmocka/common_mock.h" +#include "tests/common.h" + +#include "src/providers/krb5/krb5_common.h" + +#define TEST_REALM "MY.REALM" +#define TEST_FAST_PRINC "fast_princ@" TEST_REALM +#define TEST_FAST_STR "dummy" +#define TEST_LIFE_STR "dummy-life" +#define TEST_RLIFE_STR "dummy-rlife" + +#define TESTS_PATH "tp_" BASE_FILE_STEM +#define TEST_CONF_DB "test_krb5_common_conf.ldb" +#define TEST_DOM_NAME "test.krb5.common" +#define TEST_ID_PROVIDER "ldap" + +struct test_ctx { + struct sss_test_ctx *tctx; +}; + +static int test_setup(void **state) +{ + struct test_ctx *test_ctx; + + assert_true(leak_check_setup()); + + test_ctx = talloc_zero(global_talloc_context, struct test_ctx); + assert_non_null(test_ctx); + + test_ctx->tctx = create_dom_test_ctx(test_ctx, TESTS_PATH, TEST_CONF_DB, + TEST_DOM_NAME, + TEST_ID_PROVIDER, NULL); + assert_non_null(test_ctx->tctx); + + check_leaks_push(test_ctx); + *state = test_ctx; + + return 0; +} + +static int test_teardown(void **state) +{ + struct test_ctx *test_ctx = talloc_get_type(*state, struct test_ctx); + + assert_true(check_leaks_pop(test_ctx) == true); + talloc_free(test_ctx); + assert_true(leak_check_teardown()); + return 0; +} + +void test_set_extra_args(void **state) +{ + int ret; + struct krb5_ctx *krb5_ctx; + char *uid_opt; + char *gid_opt; + const char **krb5_child_extra_args; + + ret = set_extra_args(NULL, NULL, NULL); + assert_int_equal(ret, EINVAL); + + krb5_ctx = talloc_zero(global_talloc_context, struct krb5_ctx); + assert_non_null(krb5_ctx); + uid_opt = talloc_asprintf(krb5_ctx, "--fast-ccache-uid=%"SPRIuid, getuid()); + assert_non_null(uid_opt); + + gid_opt = talloc_asprintf(krb5_ctx, "--fast-ccache-gid=%"SPRIgid, getgid()); + assert_non_null(gid_opt); + + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_null(krb5_child_extra_args[2]); + talloc_free(krb5_child_extra_args); + + krb5_ctx->canonicalize = true; + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_string_equal(krb5_child_extra_args[2], "--canonicalize"); + assert_null(krb5_child_extra_args[3]); + talloc_free(krb5_child_extra_args); + + krb5_ctx->realm = discard_const(TEST_REALM); + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_string_equal(krb5_child_extra_args[2], "--realm=" TEST_REALM); + assert_string_equal(krb5_child_extra_args[3], "--canonicalize"); + assert_null(krb5_child_extra_args[4]); + talloc_free(krb5_child_extra_args); + + /* --fast-principal will be only set if FAST is used */ + krb5_ctx->fast_principal = discard_const(TEST_FAST_PRINC); + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_string_equal(krb5_child_extra_args[2], "--realm=" TEST_REALM); + assert_string_equal(krb5_child_extra_args[3], "--canonicalize"); + assert_null(krb5_child_extra_args[4]); + talloc_free(krb5_child_extra_args); + + krb5_ctx->use_fast_str = discard_const(TEST_FAST_STR); + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_string_equal(krb5_child_extra_args[2], "--realm=" TEST_REALM); + assert_string_equal(krb5_child_extra_args[3], "--use-fast=" TEST_FAST_STR); + assert_string_equal(krb5_child_extra_args[4], + "--fast-principal=" TEST_FAST_PRINC); + assert_string_equal(krb5_child_extra_args[5], "--canonicalize"); + assert_null(krb5_child_extra_args[6]); + talloc_free(krb5_child_extra_args); + + krb5_ctx->lifetime_str = discard_const(TEST_LIFE_STR); + krb5_ctx->rlife_str = discard_const(TEST_RLIFE_STR); + ret = set_extra_args(global_talloc_context, krb5_ctx, + &krb5_child_extra_args); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_child_extra_args[0], uid_opt); + assert_string_equal(krb5_child_extra_args[1], gid_opt); + assert_string_equal(krb5_child_extra_args[2], "--realm=" TEST_REALM); + assert_string_equal(krb5_child_extra_args[3], "--lifetime=" TEST_LIFE_STR); + assert_string_equal(krb5_child_extra_args[4], + "--renewable-lifetime=" TEST_RLIFE_STR); + assert_string_equal(krb5_child_extra_args[5], "--use-fast=" TEST_FAST_STR); + assert_string_equal(krb5_child_extra_args[6], + "--fast-principal=" TEST_FAST_PRINC); + assert_string_equal(krb5_child_extra_args[7], "--canonicalize"); + assert_null(krb5_child_extra_args[8]); + talloc_free(krb5_child_extra_args); + + talloc_free(krb5_ctx); +} + +void test_krb5_check_options(void **state) +{ + int ret; + struct dp_option *opts; + struct test_ctx *test_ctx = talloc_get_type(*state, struct test_ctx); + struct krb5_ctx *krb5_ctx; + + ret = krb5_check_options(NULL, NULL, NULL); + assert_int_equal(ret, EINVAL); + + ret = krb5_get_options(test_ctx, test_ctx->tctx->confdb, + "[domain/" TEST_DOM_NAME "]", &opts); + assert_int_equal(ret, EOK); + assert_non_null(opts); + + krb5_ctx = talloc_zero(test_ctx, struct krb5_ctx); + assert_non_null(krb5_ctx); + + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_ctx->realm, TEST_DOM_NAME); + + /* check check_lifetime() indirectly */ + ret = dp_opt_set_string(opts, KRB5_LIFETIME, "123"); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_ctx->lifetime_str, "123s"); + + ret = dp_opt_set_string(opts, KRB5_LIFETIME, "abc"); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EINVAL); + + ret = dp_opt_set_string(opts, KRB5_LIFETIME, "s"); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EINVAL); + + ret = dp_opt_set_string(opts, KRB5_LIFETIME, "1d"); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_ctx->lifetime_str, "1d"); + + ret = dp_opt_set_string(opts, KRB5_LIFETIME, "7d 0h 0m 0s"); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_string_equal(krb5_ctx->lifetime_str, "7d 0h 0m 0s"); + + /* check canonicalize */ + assert_false(krb5_ctx->canonicalize); + + ret = dp_opt_set_bool(opts, KRB5_USE_ENTERPRISE_PRINCIPAL, true); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_true(krb5_ctx->canonicalize); + + ret = dp_opt_set_bool(opts, KRB5_USE_ENTERPRISE_PRINCIPAL, false); + assert_int_equal(ret, EOK); + ret = dp_opt_set_bool(opts, KRB5_CANONICALIZE, true); + assert_int_equal(ret, EOK); + ret = krb5_check_options(opts, test_ctx->tctx->dom, krb5_ctx); + assert_int_equal(ret, EOK); + assert_true(krb5_ctx->canonicalize); + + talloc_free(krb5_ctx); + talloc_free(opts); +} + +int main(int argc, const char *argv[]) +{ + int rv; + int no_cleanup = 0; + poptContext pc; + int opt; + struct poptOption long_options[] = { + POPT_AUTOHELP + SSSD_DEBUG_OPTS + {"no-cleanup", 'n', POPT_ARG_NONE, &no_cleanup, 0, + _("Do not delete the test database after a test run"), NULL }, + POPT_TABLEEND + }; + + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_set_extra_args, + test_setup, test_teardown), + cmocka_unit_test_setup_teardown(test_krb5_check_options, + test_setup, test_teardown), + }; + + /* Set debug level to invalid value so we can deside if -d 0 was used. */ + debug_level = SSSDBG_INVALID; + + pc = poptGetContext(argv[0], argc, argv, long_options, 0); + while((opt = poptGetNextOpt(pc)) != -1) { + switch(opt) { + default: + fprintf(stderr, "\nInvalid option %s: %s\n\n", + poptBadOption(pc, 0), poptStrerror(opt)); + poptPrintUsage(pc, stderr, 0); + return 1; + } + } + poptFreeContext(pc); + + DEBUG_CLI_INIT(debug_level); + + tests_set_cwd(); + test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_DB, TEST_DOM_NAME); + test_dom_suite_setup(TESTS_PATH); + + rv = cmocka_run_group_tests(tests, NULL, NULL); + if (rv == 0 && !no_cleanup) { + test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_DB, TEST_DOM_NAME); + } + + return rv; +}
_______________________________________________ sssd-devel mailing list -- sssd-devel@lists.fedorahosted.org To unsubscribe send an email to sssd-devel-le...@lists.fedorahosted.org
