[SSSD] [PATCH] Fixes for automatic ticket renewal

Sumit Bose sbose at redhat.com
Tue Dec 14 09:17:55 UTC 2010


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.

bye,
Sumit
-------------- next part --------------
From e3ee5aebac3bd24a7903454d86dbdf4145b40983 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] 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      |    9 ++++--
 src/providers/krb5/krb5_child.c     |    9 ++++++-
 src/providers/krb5/krb5_renew_tgt.c |   46 ++++++++++++++++++++--------------
 3 files changed, 41 insertions(+), 23 deletions(-)

diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index e6b680e..37af5f0 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -582,8 +582,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 +594,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,
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index b973c13..1b9c0f3 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"));
     }
diff --git a/src/providers/krb5/krb5_renew_tgt.c b/src/providers/krb5/krb5_renew_tgt.c
index be029fd..61fa713 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;
@@ -132,7 +133,7 @@ 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);
@@ -160,7 +161,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"));
@@ -289,10 +290,10 @@ fail:
 errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile,
                                struct tgt_times *tgtt, struct pam_data *pd)
 {
-    char *key_str = NULL;
     int ret;
     hash_key_t key;
     hash_value_t value;
+    hash_value_t old_value = {HASH_VALUE_PTR, {.ptr = NULL}} ;
     struct renew_data *renew_data = NULL;
 
     if (krb5_ctx->renew_tgt_ctx == NULL) {
@@ -308,25 +309,26 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile,
     }
 
     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);
-    }
-    key.str = key_str;
+    key.str = pd->user;
 
     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 +336,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 +347,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;
@@ -359,6 +362,12 @@ errno_t add_tgt_to_renew_table(struct krb5_ctx *krb5_ctx, const char *ccfile,
     value.type = HASH_VALUE_PTR;
     value.ptr = renew_data;
 
+    ret = hash_lookup(krb5_ctx->renew_tgt_ctx->tgt_table, &key, &old_value);
+    if (ret != HASH_SUCCESS && ret != HASH_ERROR_KEY_NOT_FOUND) {
+        DEBUG(1, ("hash_lookup failed [%s].\n", hash_error_string(ret)));
+    }
+    talloc_zfree(old_value.ptr);
+
     ret = hash_enter(krb5_ctx->renew_tgt_ctx->tgt_table, &key, &value);
     if (ret != HASH_SUCCESS) {
         DEBUG(1, ("hash_enter failed.\n"));
@@ -366,13 +375,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