[SSSD] [PATCH] Fixes for automatic ticket renewal

Sumit Bose sbose at redhat.com
Fri Dec 17 10:31:16 UTC 2010


On Tue, Dec 14, 2010 at 12:21:39PM -0500, Stephen Gallagher wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 12/14/2010 04:17 AM, Sumit Bose wrote:
> > Hi,
> > 
> > I have found some minor issues with automatic ticket renewal while
> > testing.
> > 
> > If random ccache file names are used the name of the ccache file should
> > be kept if the user is not logged in, but the TGT is still renewable.
> > 
> > If a user logs in and out repeatedly and random names are used a new
> > hash entry is created for every new ticket. The old entries just eat
> > away some memory because the related ccache file is already deleted.
> > Using the user name as the hash key solves this, because currently sssd
> > (and MIT Kerberos) support one ccache per user.
> 
> 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.

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/
> 
> iEYEARECAAYFAk0Hp6IACgkQeiVVYja6o6MzJQCgsQ0P+IhTlV8Ldq2gnKGZ8G79
> 53AAn2v04ALFP+qrJKW3sx3N69CrhDKK
> =aaba
> -----END PGP SIGNATURE-----
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://fedorahosted.org/mailman/listinfo/sssd-devel
-------------- next part --------------
From 8f0bc372a0038b439c704666a31bb026155bfa78 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] Add delete callback to sss_hash_create()

---
 src/providers/ldap/sdap_async_accounts.c |    4 ++--
 src/util/util.c                          |    6 ++++--
 src/util/util.h                          |    4 +++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/src/providers/ldap/sdap_async_accounts.c b/src/providers/ldap/sdap_async_accounts.c
index 004c69a..8c6dcb2 100644
--- a/src/providers/ldap/sdap_async_accounts.c
+++ b/src/providers/ldap/sdap_async_accounts.c
@@ -1565,13 +1565,13 @@ static void sdap_get_groups_process(struct tevent_req *subreq)
         }
 
         /* Prepare hashes for nested user processing */
-        ret = sss_hash_create(state, 32, &state->user_hash);
+        ret = sss_hash_create(state, 32, &state->user_hash, NULL, NULL);
         if (ret != EOK) {
             tevent_req_error(req, ret);
             return;
         }
 
-        ret = sss_hash_create(state, 32, &state->group_hash);
+        ret = sss_hash_create(state, 32, &state->group_hash, NULL, NULL);
         if (ret != EOK) {
             tevent_req_error(req, ret);
             return;
diff --git a/src/util/util.c b/src/util/util.c
index 67f9880..7d0b7d2 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -475,7 +475,9 @@ static void hash_talloc_free(void *ptr, void *pvt)
 
 errno_t sss_hash_create(TALLOC_CTX *mem_ctx,
                         unsigned long count,
-                        hash_table_t **tbl)
+                        hash_table_t **tbl,
+                        hash_delete_callback *delete_callback,
+                        void *delete_private_data)
 {
     errno_t ret;
     hash_table_t *table;
@@ -489,7 +491,7 @@ errno_t sss_hash_create(TALLOC_CTX *mem_ctx,
 
     hret = hash_create_ex(count, &table, 0, 0, 0, 0,
                           hash_talloc, hash_talloc_free,
-                          internal_ctx, NULL, NULL);
+                          internal_ctx, delete_callback, delete_private_data);
     switch (hret) {
     case HASH_SUCCESS:
         /* Steal the table pointer onto the mem_ctx,
diff --git a/src/util/util.h b/src/util/util.h
index e454f8b..79288aa 100644
--- a/src/util/util.h
+++ b/src/util/util.h
@@ -363,7 +363,9 @@ char **parse_args(const char *str);
 
 errno_t sss_hash_create(TALLOC_CTX *mem_ctx,
                         unsigned long count,
-                        hash_table_t **tbl);
+                        hash_table_t **tbl,
+                        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 6945fb46a2367b21fe19f7ea938ad9f09776e5d8 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 username 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 |   77 +++++++++++++++++++++++++----------
 4 files changed, 99 insertions(+), 43 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..988cea4 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)
 {
@@ -255,7 +279,8 @@ errno_t init_renew_tgt(struct krb5_ctx *krb5_ctx, struct be_ctx *be_ctx,
     }
 
     ret = sss_hash_create(krb5_ctx->renew_tgt_ctx, INITIAL_TGT_TABLE_SIZE,
-                          &krb5_ctx->renew_tgt_ctx->tgt_table);
+                          &krb5_ctx->renew_tgt_ctx->tgt_table, 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