On Fri, Dec 17, 2010 at 04:13:18PM -0500, Stephen Gallagher wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 12/17/2010 05:31 AM, Sumit Bose wrote: > > On Tue, Dec 14, 2010 at 12:21:39PM -0500, Stephen Gallagher wrote: > > > > Nack. > > > > For future compatibility, it would be better if we keyed on the > > principal, rather than the username. Then we can support in the future > > one user with one ccache per principal. > > > > We're actively working with MIT to try and get them to support multiple > > simultaneous TGTs, and I'd really like for us not to have to re-fix this > > when that comes through. > > > > > >> ok, the new patch uses the principal. I have added another patch to > >> allow a delete callback to be passed to sss_hash_create(). This is > >> useful if hash_enter() is used to update entries. > > > > > Patch 0001: Nack > I'd rather that you split out sss_hash_create() and > sss_hash_create_ex(). The sss_hash_create() should omit the delete > callback information. I would prefer sss_hash_create() to be the common > case. > > Patch 0002: Nack > Will need to be updated to match the above change > Fix the patch commit comment to say "principal" rather than username > Otherwise, the patch looks good. >
New patches attached. bye, Sumit > > - -- > Stephen Gallagher > RHCE 804006346421761 > > Delivering value year after year. > Red Hat ranks #1 in value among software vendors. > http://www.redhat.com/promo/vendor/ > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1.4.11 (GNU/Linux) > Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/ > > iEYEARECAAYFAk0L0m4ACgkQeiVVYja6o6PmgwCfVPK7xWXB57coNTI57MEXoI+9 > Xv0AoI+/VWdF2Giyq5QMsa3PJQTLUayZ > =0Kki > -----END PGP SIGNATURE----- > _______________________________________________ > sssd-devel mailing list > [email protected] > https://fedorahosted.org/mailman/listinfo/sssd-devel
From f8d24e44e897bb52454ba2165861034a1d0d2194 Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Wed, 15 Dec 2010 17:46:23 +0100 Subject: [PATCH 1/2] Introduce sss_hash_create_ex() --- src/util/util.c | 25 +++++++++++++++++++------ src/util/util.h | 10 ++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/src/util/util.c b/src/util/util.c index 67f9880..39275ef 100644 --- a/src/util/util.c +++ b/src/util/util.c @@ -473,9 +473,15 @@ static void hash_talloc_free(void *ptr, void *pvt) talloc_free(ptr); } -errno_t sss_hash_create(TALLOC_CTX *mem_ctx, - unsigned long count, - hash_table_t **tbl) +errno_t sss_hash_create_ex(TALLOC_CTX *mem_ctx, + unsigned long count, + hash_table_t **tbl, + unsigned int directory_bits, + unsigned int segment_bits, + unsigned long min_load_factor, + unsigned long max_load_factor, + hash_delete_callback *delete_callback, + void *delete_private_data) { errno_t ret; hash_table_t *table; @@ -487,9 +493,10 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx, return ENOMEM; } - hret = hash_create_ex(count, &table, 0, 0, 0, 0, - hash_talloc, hash_talloc_free, - internal_ctx, NULL, NULL); + hret = hash_create_ex(count, &table, directory_bits, segment_bits, + min_load_factor, max_load_factor, + hash_talloc, hash_talloc_free, internal_ctx, + delete_callback, delete_private_data); switch (hret) { case HASH_SUCCESS: /* Steal the table pointer onto the mem_ctx, @@ -517,6 +524,12 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx, return ret; } +errno_t sss_hash_create(TALLOC_CTX *mem_ctx, unsigned long count, + hash_table_t **tbl) +{ + return sss_hash_create_ex(mem_ctx, count, tbl, 0, 0, 0, 0, NULL, NULL); +} + errno_t sss_filter_sanitize(TALLOC_CTX *mem_ctx, const char *input, char **sanitized) diff --git a/src/util/util.h b/src/util/util.h index e454f8b..3e181bb 100644 --- a/src/util/util.h +++ b/src/util/util.h @@ -365,6 +365,16 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx, unsigned long count, hash_table_t **tbl); +errno_t sss_hash_create_ex(TALLOC_CTX *mem_ctx, + unsigned long count, + hash_table_t **tbl, + unsigned int directory_bits, + unsigned int segment_bits, + unsigned long min_load_factor, + unsigned long max_load_factor, + hash_delete_callback *delete_callback, + void *delete_private_data); + /* Copy a NULL-terminated string list * Returns NULL on out of memory error or invalid input */ -- 1.7.3.2
From 64c13f79c4bb222b7919bdf51aaf519d42147827 Mon Sep 17 00:00:00 2001 From: Sumit Bose <[email protected]> Date: Mon, 13 Dec 2010 22:36:05 +0100 Subject: [PATCH 2/2] Fixes for automatic ticket renewal - do not recreate the ccache file when renewing the TGT - use user principal name as hash key instead of ccfile name - let krb5_child return Kerberos error codes --- src/providers/krb5/krb5_auth.c | 45 ++++++++++++-------- src/providers/krb5/krb5_auth.h | 3 +- src/providers/krb5/krb5_child.c | 17 +++++++- src/providers/krb5/krb5_renew_tgt.c | 79 ++++++++++++++++++++++++---------- 4 files changed, 100 insertions(+), 44 deletions(-) diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c index e6b680e..a0ac0e9 100644 --- a/src/providers/krb5/krb5_auth.c +++ b/src/providers/krb5/krb5_auth.c @@ -161,6 +161,8 @@ static int krb5_save_ccname(TALLOC_CTX *mem_ctx, return EINVAL; } + DEBUG(9, ("Save ccname [%s] for user [%s].\n", ccname, name)); + tmpctx = talloc_new(mem_ctx); if (!tmpctx) { return ENOMEM; @@ -349,8 +351,10 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx, } if (be_is_offline(be_ctx) && - (pd->cmd == SSS_PAM_CHAUTHTOK || pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM)) { - DEBUG(9, ("Password changes are not possible while offline.\n")); + (pd->cmd == SSS_PAM_CHAUTHTOK || pd->cmd == SSS_PAM_CHAUTHTOK_PRELIM || + pd->cmd == SSS_CMD_RENEW)) { + DEBUG(9, ("Password changes and ticket renewal are not possible " + "while offline.\n")); state->pam_status = PAM_AUTHINFO_UNAVAIL; state->dp_err = DP_ERR_OFFLINE; ret = EOK; @@ -582,8 +586,10 @@ static void krb5_find_ccache_step(struct tevent_req *req) * is true: * - it doesn't exist (kr->ccname == NULL) * - the backend is online and the current ccache file is not used, i.e - * the related user is currently not logged in - * (!kr->is_offline && !kr->active_ccache_present) + * the related user is currently not logged in and it is not a renewal + * request + * (!kr->is_offline && !kr->active_ccache_present && + * pd->cmd != SSS_CMD_RENEW) * - the backend is offline and the current cache file not used and * it does not contain a valid tgt * (kr->is_offline && @@ -592,7 +598,8 @@ static void krb5_find_ccache_step(struct tevent_req *req) if (kr->ccname == NULL || (kr->is_offline && !kr->active_ccache_present && !kr->valid_tgt_present) || - (!kr->is_offline && !kr->active_ccache_present)) { + (!kr->is_offline && !kr->active_ccache_present && + pd->cmd != SSS_CMD_RENEW)) { DEBUG(9, ("Recreating ccache file.\n")); kr->ccname = expand_ccname_template(kr, kr, dp_opt_get_cstring(kr->krb5_ctx->opts, @@ -790,19 +797,6 @@ static void krb5_child_done(struct tevent_req *subreq) } } - if (msg_status == PAM_SUCCESS && - dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0 && - (pd->cmd == SSS_PAM_AUTHENTICATE || pd->cmd == SSS_CMD_RENEW || - pd->cmd == SSS_PAM_CHAUTHTOK) && - tgtt.renew_till > tgtt.endtime && kr->ccname != NULL) { - DEBUG(7, ("Adding [%s] for automatic renewal.\n", kr->ccname)); - ret = add_tgt_to_renew_table(kr->krb5_ctx, kr->ccname, &tgtt, pd); - if (ret != EOK) { - DEBUG(1, ("add_tgt_to_renew_table failed, " - "automatic renewal not possible.\n")); - } - } - /* If the child request failed, but did not return an offline error code, * return with the status */ if (msg_status != PAM_SUCCESS && msg_status != PAM_AUTHINFO_UNAVAIL && @@ -889,7 +883,22 @@ static void krb5_child_done(struct tevent_req *subreq) goto done; } + if (msg_status == PAM_SUCCESS && + dp_opt_get_int(kr->krb5_ctx->opts, KRB5_RENEW_INTERVAL) > 0 && + (pd->cmd == SSS_PAM_AUTHENTICATE || pd->cmd == SSS_CMD_RENEW || + pd->cmd == SSS_PAM_CHAUTHTOK) && + tgtt.renew_till > tgtt.endtime && kr->ccname != NULL) { + DEBUG(7, ("Adding [%s] for automatic renewal.\n", kr->ccname)); + ret = add_tgt_to_renew_table(kr->krb5_ctx, kr->ccname, &tgtt, pd, + kr->upn); + if (ret != EOK) { + DEBUG(1, ("add_tgt_to_renew_table failed, " + "automatic renewal not possible.\n")); + } + } + krb5_save_ccname_done(req); + return; done: diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h index f40cbcb..2f08ad9 100644 --- a/src/providers/krb5/krb5_auth.h +++ b/src/providers/krb5/krb5_auth.h @@ -83,7 +83,8 @@ errno_t init_delayed_online_authentication(struct krb5_ctx *krb5_ctx, errno_t init_renew_tgt(struct krb5_ctx *krb5_ctx, struct be_ctx *be_ctx, struct tevent_context *ev, time_t renew_intv); errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, - struct tgt_times *tgtt, struct pam_data *pd); + struct tgt_times *tgtt, struct pam_data *pd, + const char *upn); /* krb5_access.c */ struct tevent_req *krb5_access_send(TALLOC_CTX *mem_ctx, diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c index b973c13..335da42 100644 --- a/src/providers/krb5/krb5_child.c +++ b/src/providers/krb5/krb5_child.c @@ -1046,12 +1046,14 @@ static errno_t renew_tgt_child(int fd, struct krb5_req *kr) if (kr->pd->authtok_type != SSS_AUTHTOK_TYPE_CCFILE) { DEBUG(1, ("Unsupported authtok type for TGT renewal [%d].\n", kr->pd->authtok_type)); + kerr = EINVAL; goto done; } ccname = talloc_strndup(kr, (char *) kr->pd->authtok, kr->pd->authtok_size); if (ccname == NULL) { DEBUG(1, ("talloc_strndup failed.\n")); + kerr = ENOMEM; goto done; } @@ -1064,6 +1066,9 @@ static errno_t renew_tgt_child(int fd, struct krb5_req *kr) kerr = krb5_get_renewed_creds(kr->ctx, kr->creds, kr->princ, ccache, NULL); if (kerr != 0) { KRB5_DEBUG(1, kerr); + if (kerr == KRB5_KDC_UNREACH) { + status = PAM_AUTHINFO_UNAVAIL; + } goto done; } @@ -1085,6 +1090,7 @@ static errno_t renew_tgt_child(int fd, struct krb5_req *kr) ret = become_user(kr->uid, kr->gid); if (ret != EOK) { DEBUG(1, ("become_user failed.\n")); + kerr = ret; goto done; } } @@ -1107,6 +1113,7 @@ static errno_t renew_tgt_child(int fd, struct krb5_req *kr) } status = PAM_SUCCESS; + kerr = 0; done: krb5_free_cred_contents(kr->ctx, kr->creds); @@ -1115,7 +1122,7 @@ done: krb5_cc_close(kr->ctx, ccache); } - ret = sendresponse(fd, 0, status, kr); + ret = sendresponse(fd, kerr, status, kr); if (ret != EOK) { DEBUG(1, ("sendresponse failed.\n")); } @@ -1424,7 +1431,13 @@ static int krb5_child_setup(struct krb5_req *kr, uint32_t offline) kr->child_req = kuserok_child; break; case SSS_CMD_RENEW: - kr->child_req = renew_tgt_child; + if (!offline) { + 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)); diff --git a/src/providers/krb5/krb5_renew_tgt.c b/src/providers/krb5/krb5_renew_tgt.c index be029fd..7d68c3a 100644 --- a/src/providers/krb5/krb5_renew_tgt.c +++ b/src/providers/krb5/krb5_renew_tgt.c @@ -40,6 +40,7 @@ struct renew_tgt_ctx { }; struct renew_data { + const char *ccfile; time_t start_time; time_t lifetime; time_t start_renew_at; @@ -50,6 +51,7 @@ struct auth_data { struct be_ctx *be_ctx; struct krb5_ctx *krb5_ctx; struct pam_data *pd; + struct renew_data *renew_data; hash_table_t *table; hash_key_t key; }; @@ -86,6 +88,10 @@ static void renew_tgt_done(struct tevent_req *req) talloc_free(req); if (ret) { DEBUG(1, ("krb5_auth request failed.\n")); + if (auth_data->renew_data != NULL) { + DEBUG(5, ("Giving back pam data.\n")); + talloc_steal(auth_data->renew_data, auth_data->pd); + } } else { switch (pam_status) { case PAM_SUCCESS: @@ -97,6 +103,10 @@ static void renew_tgt_done(struct tevent_req *req) DEBUG(4, ("Cannot renewed TGT for user [%s] while offline, " "will retry later.\n", auth_data->pd->user)); + if (auth_data->renew_data != NULL) { + DEBUG(5, ("Giving back pam data.\n")); + talloc_steal(auth_data->renew_data, auth_data->pd); + } break; default: DEBUG(1, ("Failed to renew TGT for user [%s].\n", @@ -132,17 +142,18 @@ static errno_t renew_all_tgts(struct renew_tgt_ctx *renew_tgt_ctx) for (c = 0; c < count; c++) { renew_data = talloc_get_type(entries[c].value.ptr, struct renew_data); - DEBUG(9, ("Checking [%s] for renewal at [%.24s].\n", entries[c].key.str, + DEBUG(9, ("Checking [%s] for renewal at [%.24s].\n", renew_data->ccfile, ctime(&renew_data->start_renew_at))); if (renew_data->start_renew_at < now) { auth_data = talloc_zero(renew_tgt_ctx, struct auth_data); if (auth_data == NULL) { DEBUG(1, ("talloc_zero failed.\n")); } else { - auth_data->pd = renew_data->pd; + auth_data->pd = talloc_steal(auth_data, renew_data->pd); auth_data->krb5_ctx = renew_tgt_ctx->krb5_ctx; auth_data->be_ctx = renew_tgt_ctx->be_ctx; auth_data->table = renew_tgt_ctx->tgt_table; + auth_data->renew_data = renew_data; auth_data->key.type = entries[c].key.type; auth_data->key.str = talloc_strdup(auth_data, entries[c].key.str); @@ -160,7 +171,7 @@ static errno_t renew_all_tgts(struct renew_tgt_ctx *renew_tgt_ctx) } if (auth_data == NULL || te == NULL) { - DEBUG(1, ("Failed to renew TGT in [%s].\n", entries[c].key.str)); + DEBUG(1, ("Failed to renew TGT in [%s].\n", renew_data->ccfile)); ret = hash_delete(renew_tgt_ctx->tgt_table, &entries[c].key); if (ret != HASH_SUCCESS) { DEBUG(1, ("hash_delete failed.\n")); @@ -242,6 +253,19 @@ static void renew_handler(struct renew_tgt_ctx *renew_tgt_ctx) return; } +static void renew_del_cb(hash_entry_t *entry, hash_destroy_enum type, void *pvt) +{ + struct renew_data *renew_data; + + if (entry->value.type == HASH_VALUE_PTR) { + renew_data = talloc_get_type(entry->value.ptr, struct renew_data); + talloc_zfree(renew_data); + return; + } + + DEBUG(1, ("Unexpected value type [%d].\n", entry->value.type)); +} + errno_t init_renew_tgt(struct krb5_ctx *krb5_ctx, struct be_ctx *be_ctx, struct tevent_context *ev, time_t renew_intv) { @@ -254,8 +278,9 @@ errno_t init_renew_tgt(struct krb5_ctx *krb5_ctx, struct be_ctx *be_ctx, return ENOMEM; } - ret = sss_hash_create(krb5_ctx->renew_tgt_ctx, INITIAL_TGT_TABLE_SIZE, - &krb5_ctx->renew_tgt_ctx->tgt_table); + ret = sss_hash_create_ex(krb5_ctx->renew_tgt_ctx, INITIAL_TGT_TABLE_SIZE, + &krb5_ctx->renew_tgt_ctx->tgt_table, 0, 0, 0, 0, + renew_del_cb, NULL); if (ret != EOK) { DEBUG(1, ("sss_hash_create failed.\n")); goto fail; @@ -287,9 +312,9 @@ fail: } errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, - struct tgt_times *tgtt, struct pam_data *pd) + struct tgt_times *tgtt, struct pam_data *pd, + const char *upn) { - char *key_str = NULL; int ret; hash_key_t key; hash_value_t value; @@ -307,26 +332,34 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, return EINVAL; } - key.type = HASH_KEY_STRING; - if (ccfile[0] == '/') { - key_str = talloc_asprintf(NULL, "FILE:%s", ccfile); - if (key_str == NULL) { - DEBUG(1, ("talloc_asprintf doneed.\n")); - ret = ENOMEM; - goto done; - } - } else { - key_str = talloc_strdup(NULL, ccfile); + if (upn == NULL) { + DEBUG(1, ("Missing user principal name.\n")); + return EINVAL; } - key.str = key_str; + + /* hash_enter copies the content of the hash string, so it is safe to use + * discard_const_p here. */ + key.type = HASH_KEY_STRING; + key.str = discard_const_p(char, upn); renew_data = talloc_zero(krb5_ctx->renew_tgt_ctx, struct renew_data); if (renew_data == NULL) { - DEBUG(1, ("talloc_zero doneed.\n")); + DEBUG(1, ("talloc_zero failed.\n")); ret = ENOMEM; goto done; } + if (ccfile[0] == '/') { + renew_data->ccfile = talloc_asprintf(renew_data, "FILE:%s", ccfile); + if (renew_data->ccfile == NULL) { + DEBUG(1, ("talloc_asprintf failed.\n")); + ret = ENOMEM; + goto done; + } + } else { + renew_data->ccfile = talloc_strdup(renew_data, ccfile); + } + renew_data->start_time = tgtt->starttime; renew_data->lifetime = tgtt->endtime; renew_data->start_renew_at = (time_t) (tgtt->starttime + @@ -334,7 +367,7 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, ret = copy_pam_data(renew_data, pd, &renew_data->pd); if (ret != EOK) { - DEBUG(1, ("copy_pam_data doneed.\n")); + DEBUG(1, ("copy_pam_data failed.\n")); goto done; } @@ -345,7 +378,8 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, } talloc_zfree(renew_data->pd->authtok); - renew_data->pd->authtok = (uint8_t *) talloc_strdup(renew_data->pd, key.str); + renew_data->pd->authtok = (uint8_t *) talloc_strdup(renew_data->pd, + renew_data->ccfile); if (renew_data->pd->authtok == NULL) { DEBUG(1, ("talloc_strdup failed.\n")); ret = ENOMEM; @@ -366,13 +400,12 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile, goto done; } - DEBUG(7, ("Added [%s] for renewal at [%.24s].\n", key_str, + DEBUG(7, ("Added [%s] for renewal at [%.24s].\n", renew_data->ccfile, ctime(&renew_data->start_renew_at))); ret = EOK; done: - talloc_free(key_str); if (ret != EOK) { talloc_free(renew_data); } -- 1.7.3.2
_______________________________________________ sssd-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/sssd-devel
