[SSSD] [PATCH] Fixes for automatic ticket renewal
Sumit Bose
sbose at redhat.com
Mon Dec 20 11:16:26 UTC 2010
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
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From f8d24e44e897bb52454ba2165861034a1d0d2194 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
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
-------------- next part --------------
From 64c13f79c4bb222b7919bdf51aaf519d42147827 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
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
More information about the sssd-devel
mailing list