[PATCH] Generate ccache name at the beginning of auth process

[Jan Zelený]

After some complications I finally made the patch solving ticket #533 (different 
ccache files during multiple simultaneous logins of the same user). It is based 
on Simo's idea to determine the ccache file name in advance and count 
references to it.

Jan

[Sumit Bose]

On Sat, Nov 20, 2010 at 12:25:02AM +0100, Jan Zeleny wrote:
> After some complications I finally made the patch solving ticket #533 (different 
> ccache files during multiple simultaneous logins of the same user). It is based 
> on Simo's idea to determine the ccache file name in advance and count 
> references to it.
>
> Jan

I'm not sure if it is worth writing the reference counter and the new
file to the cache. Having them in a hash should be sufficient and
the reference counter does not have to be reseted at startup (which is
missing).

The reference counter is set in krb5_auth_send() but decremented in
krb5_child_done(). This should be moved to krb5_auth_done() before any
error checking to make sure that it is done under all circumstances.

bye,
Sumit
> From 4a7090fc49073ff671024ca6dbc01d654949670a Mon Sep 17 00:00:00 2001
> From: Jan Zeleny 
> Date: Sat, 20 Nov 2010 00:18:37 +0100
> Subject: [PATCH] Generate ccache name at the beginning of auth
>
> In order to avoid situation when user logging to multiple
> terminals simultaneously gets different ccache files in each
> terminal (if random ccache files are used), the ccache file names
> are determined at the very beginning of authorization process.
> Ticket: #533
>
> src/db/sysdb.h                 |    2 +
>  src/providers/krb5/krb5_auth.c |  242 ++++++++++++++++++++++++++++++----------
>  src/providers/krb5/krb5_auth.h |    2 +
>  3 files changed, 189 insertions(+), 57 deletions(-)
>
> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
> index ed100b6..858c837 100644
> --- a/src/db/sysdb.h
> +++ b/src/db/sysdb.h
> @@ -88,6 +88,8 @@
>  #define SYSDB_UUID "uniqueID"
>  #define SYSDB_UPN "userPrincipalName"
>  #define SYSDB_CCACHE_FILE "ccacheFile"
> +#define SYSDB_CCACHE_NFILE "ccacheNFile"
> +#define SYSDB_CCACHE_NFILE_REF "ccacheNFileRef"
>
> #define SYSDB_ORIG_DN "originalDN"
>  #define SYSDB_ORIG_MODSTAMP "originalModifyTimestamp"
> diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
> index 9d5005a..eac595b 100644
> --- a/src/providers/krb5/krb5_auth.c
> +++ b/src/providers/krb5/krb5_auth.c
> @@ -24,6 +24,7 @@
>
> #include <errno.h>
>  #include <sys/time.h>
> +#include <unistd.h>
>
> #include <sys/types.h>
>  #include <sys/wait.h>
> @@ -147,13 +148,15 @@ static int krb5_save_ccname(TALLOC_CTX *mem_ctx,
>                              struct sysdb_ctx *sysdb,
>                              struct sss_domain_info *domain,
>                              const char *name,
>                        const char *ccname)
>                        const char *attr_name,
>                        const char *ccname,
>                        const int refcount)
>
> {
>      TALLOC_CTX *tmpctx;
>      struct sysdb_attrs *attrs;
>      int ret;
> if (name == NULL || ccname == NULL) {
> if (name == NULL || (ccname == NULL && strcmp(attr_name, SYSDB_CCACHE_FILE) == 0)) {
>     DEBUG(1, ("Missing user or ccache name.\n"));
>     return EINVAL;
> }
>
> @@ -169,10 +172,24 @@ static int krb5_save_ccname(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
> ret = sysdb_attrs_add_string(attrs, SYSDB_CCACHE_FILE, ccname);
> if (ret != EOK) {
>    DEBUG(1, ("sysdb_attrs_add_string failed.\n"));
>    goto done;
>
> if (ccname) {
>    ret = sysdb_attrs_add_string(attrs, attr_name, ccname);
>    if (ret != EOK) {
>        DEBUG(1, ("sysdb_attrs_add_string failed.\n"));
>        goto done;
>    }
>
> }
>
> if (strcmp(attr_name, SYSDB_CCACHE_NFILE) == 0) {
>    if (refcount >= 0) {
>        ret = sysdb_attrs_add_long(attrs, SYSDB_CCACHE_NFILE_REF, refcount);
>        if (ret != EOK) {
>            DEBUG(1, ("sysdb_attrs_add_string failed.\n"));
>            goto done;
>        }
>    } else {
>        DEBUG(3, ("New ccache file ref. counter < 0. Skipping ...\n"));
>    }
>
> }
>
> ret = sysdb_transaction_start(sysdb);
>
> @@ -199,6 +216,67 @@ done:
>      return ret;
>  }
>
> +/**
> Decrease reference counter of the potential new ccname
> When the counter is 0, no auth request for given user should be in progress
> and the new ccname candidate will be generated during the next auth request
> */
>
> +static int krb5_ccname_decref(TALLOC_CTX *mem_ctx,
>                          struct sysdb_ctx *sysdb,
>                          struct sss_domain_info *domain,
>                          const char *name)
>
> +{
> int ccache_file_new_ref = 0;
> struct ldb_result *res = NULL;
> const char **attrs = NULL;
> int ret = EOK;
> attrs = talloc_array(mem_ctx, const char *, 2);
> if (attrs == NULL) {
>    return ENOMEM;
> }
> attrs[0] = SYSDB_CCACHE_NFILE_REF;
> attrs[1] = NULL;
> ret = sysdb_get_user_attr(mem_ctx, sysdb, domain, name, attrs, &res);
> if (ret) {
>    DEBUG(5, ("sysdb search for upn of user [%s] failed.\n", name));
>    goto done;
> }
> switch (res->count) {
> case 0:
>    DEBUG(5, ("No attributes for user [%s] found.\n", name));
>    ret = ENOENT;
>    goto done;
> case 1:
>    ccache_file_new_ref = ldb_msg_find_attr_as_int(res->msgs[0],
>                                                   SYSDB_CCACHE_NFILE_REF,
>                                                   0);
>    if (ccache_file_new_ref < 1) {
>        DEBUG(3, ("Something strange happened - cannot decrease ccache refcount.\n"));
>        ret = EINVAL;
>        goto done;
>    }
>    ret = krb5_save_ccname(mem_ctx, sysdb, domain, name, SYSDB_CCACHE_NFILE, NULL,
>                           ccache_file_new_ref-1);
>    break;
> default:
>    DEBUG(1, ("User search for (%s) returned > 1 results!\n", name));
>    ret = EINVAL;
>    goto done;
> }
>
> +done:
> talloc_free(attrs);
> return ret;
>
> +}
>
> static struct krb5_ctx *get_krb5_ctx(struct be_req *be_req)
>  {
>      struct pam_data *pd;
> @@ -302,11 +380,17 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>      struct ldb_result *res;
>      struct krb5child_req *kr = NULL;
>      const char *ccache_file = NULL;
>
> const char *ccache_file_new = NULL;
>
> int ccache_file_new_ref = 0;
>
> bool private_path = false;
>
> char *cc_file_name = NULL;
>
> size_t ccname_len = 0;
> const char *realm;
> krb5_error_code kerr;
> struct tevent_req *req;
> struct tevent_req *subreq;
> int ret;
>
> int fd;
>
> req = tevent_req_create(mem_ctx, &state, struct krb5_auth_state);
> if (req == NULL) {
>
> @@ -352,7 +436,7 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>          goto done;
>      }
> attrs = talloc_array(state, const char *, 6);
> attrs = talloc_array(state, const char *, 8);
> if (attrs == NULL) {
>     ret = ENOMEM;
>     goto done;
>
> @@ -361,9 +445,11 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>      attrs[0] = SYSDB_UPN;
>      attrs[1] = SYSDB_HOMEDIR;
>      attrs[2] = SYSDB_CCACHE_FILE;
> attrs[3] = SYSDB_UIDNUM;
> attrs[4] = SYSDB_GIDNUM;
> attrs[5] = NULL;
>
> attrs[3] = SYSDB_CCACHE_NFILE;
>
> attrs[4] = SYSDB_CCACHE_NFILE_REF;
>
> attrs[5] = SYSDB_UIDNUM;
>
> attrs[6] = SYSDB_GIDNUM;
>
> attrs[7] = NULL;
>
> ret = krb5_setup(state, pd, krb5_ctx, &state->kr);
> if (ret != EOK) {
>
> @@ -425,6 +511,78 @@ struct tevent_req *krb5_auth_send(TALLOC_CTX *mem_ctx,
>              goto done;
>          }
>    ccache_file_new = ldb_msg_find_attr_as_string(res->msgs[0],
>                                                  SYSDB_CCACHE_NFILE,
>                                                  NULL);
>    ccache_file_new_ref = ldb_msg_find_attr_as_int(res->msgs[0],
>                                                   SYSDB_CCACHE_NFILE_REF,
>                                                   0);
>    DEBUG(8, ("New ccache file: %s (%d references)\n", ccache_file_new,
>                                                       ccache_file_new_ref));
>    if (!ccache_file_new || !ccache_file_new_ref) {
>        cc_file_name = expand_ccname_template(kr, kr,
>                                              dp_opt_get_cstring(kr->krb5_ctx->opts,
>                                                                 KRB5_CCNAME_TMPL),
>                                              true, &private_path);
>        if (cc_file_name == NULL) {
>            DEBUG(1, ("expand_ccname_template failed.\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>        ret = create_ccache_dir(kr, cc_file_name,
>                                kr->krb5_ctx->illegal_path_re,
>                                kr->uid, kr->gid, private_path);
>        if (ret != EOK) {
>            DEBUG(1, ("create_ccache_dir failed.\n"));
>            goto done;
>        }
>        /* A little hack here - assigning to const char *
>         * even though we know that we will be modifiyng the content
>         * a few lines below
>         */
>        ccache_file_new = cc_file_name;
>        if (strncmp(cc_file_name, "FILE:", 5) == 0) {
>            cc_file_name = cc_file_name + 5;
>        }
>        if (cc_file_name[0] != '/') {
>            DEBUG(1, ("Ccache filename is not an absolute path.\n"));
>            ret = EINVAL;
>            goto done;
>        }
>        ccname_len = strlen(cc_file_name);
>        if (ccname_len >= 6 && strcmp(cc_file_name + (ccname_len-6), "XXXXXX")==0 ) {
>            fd = mkstemp(cc_file_name);
>            if (fd == -1) {
>                DEBUG(1, ("mkstemp failed [%d][%s].\n", errno, strerror(errno)));
>                ret = errno;
>                goto done;
>            }
>            close(fd);
>            unlink(cc_file_name);
>        }
>        ccache_file_new_ref = 1;
>    } else {
>        ccache_file_new_ref++;
>    }
>    kr->new_ccname = ccache_file_new;
>    kr->new_ccname_ref = ccache_file_new_ref;
>    DEBUG(8, ("Storing new ccache file: %s (%d references)\n",
>              ccache_file_new, ccache_file_new_ref));
>    ret = krb5_save_ccname(kr, state->be_ctx->sysdb,
>                           state->be_ctx->domain,
>                           kr->pd->user, SYSDB_CCACHE_NFILE,
>                           ccache_file_new, ccache_file_new_ref);
>    if (ret) {
>        DEBUG(1, ("krb5_save_ccname_send failed.\n"));
>        goto done;
>    }
>    ccache_file = ldb_msg_find_attr_as_string(res->msgs[0],
>                                              SYSDB_CCACHE_FILE,
>                                              NULL);
>
> @@ -566,7 +724,6 @@ static void krb5_find_ccache_step(struct tevent_req *req)
>      struct krb5child_req *kr = state->kr;
>      struct pam_data *pd = kr->pd;
>      char *msg;
>
> bool private_path = false;
> struct tevent_req *subreq = NULL;
>
> if (!kr->is_offline) {
>
> @@ -588,24 +745,8 @@ static void krb5_find_ccache_step(struct tevent_req *req)
>          (kr->is_offline && !kr->active_ccache_present &&
>              !kr->valid_tgt_present) ||
>          (!kr->is_offline && !kr->active_ccache_present)) {
>        DEBUG(9, ("Recreating  ccache file.\n"));
>        kr->ccname = expand_ccname_template(kr, kr,
>                                      dp_opt_get_cstring(kr->krb5_ctx->opts,
>                                                         KRB5_CCNAME_TMPL),
>                                            true, &private_path);
>        if (kr->ccname == NULL) {
>            DEBUG(1, ("expand_ccname_template failed.\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>        ret = create_ccache_dir(kr, kr->ccname,
>                                kr->krb5_ctx->illegal_path_re,
>                                kr->uid, kr->gid, private_path);
>        if (ret != EOK) {
>            DEBUG(1, ("create_ccache_dir failed.\n"));
>            goto done;
>        }
>        DEBUG(9, ("Using new ccache file.\n"));
>        kr->ccname = kr->new_ccname;
>
> }
>
> if (kr->is_offline) {
>
> @@ -615,6 +756,13 @@ static void krb5_find_ccache_step(struct tevent_req *req)
>              DEBUG(9, ("Valid TGT available or "
>                        "ccache file is already in use.\n"));
>              kr->ccname = kr->old_ccname;
>        kr->new_ccname = NULL;
>        kr->new_ccname_ref = 0;
>        ret = krb5_save_ccname(kr, state->be_ctx->sysdb,
>                               state->be_ctx->domain,
>                               kr->pd->user, SYSDB_CCACHE_NFILE,
>                               kr->new_ccname, kr->new_ccname_ref);
>        msg = talloc_asprintf(pd, "%s=%s", CCACHE_ENV_NAME, kr->ccname);
>        if (msg == NULL) {
>            DEBUG(1, ("talloc_asprintf failed.\n"));
>
> @@ -683,7 +831,6 @@ static void krb5_child_done(struct tevent_req *subreq)
>      int ret;
>      uint8_t *buf = NULL;
>      ssize_t len = -1;
> ssize_t pref_len;
> size_t p;
> int32_t msg_status;
> int32_t msg_type;
>
> @@ -735,22 +882,6 @@ static void krb5_child_done(struct tevent_req *subreq)
>              goto done;
>          }
>    /* We need to save the name of the credential cache file. To find it
>     * we check if the data part of a message starts with
>     * CCACHE_ENV_NAME"=". pref_len also counts the trailing '=' because
>     * sizeof() counts the trailing '\0' of a string. */
>    pref_len = sizeof(CCACHE_ENV_NAME);
>    if (msg_len > pref_len &&
>        strncmp((const char *) &buf[p], CCACHE_ENV_NAME"=", pref_len) == 0) {
>        kr->ccname = talloc_strndup(kr, (char *) &buf[p+pref_len],
>                                    msg_len-pref_len);
>        if (kr->ccname == NULL) {
>            DEBUG(1, ("talloc_strndup failed.\n"));
>            ret = ENOMEM;
>            goto done;
>        }
>    }
>    ret = pam_add_response(pd, msg_type, msg_len, &buf[p]);
>    if (ret != EOK) {
>        /* This is not a fatal error */
>
> @@ -818,16 +949,7 @@ static void krb5_child_done(struct tevent_req *subreq)
>          fo_set_port_status(kr->srv, PORT_WORKING);
>      }
> /* Now only a successful authentication or password change is left.
> *
> * We expect that one of the messages in the received buffer contains
> * the name of the credential cache file. */
> if (kr->ccname == NULL) {
>    DEBUG(1, ("Missing ccache name in child response.\n"));
>    ret = EINVAL;
>    goto done;
> }
> /* Now only a successful authentication or password change is left. */
> if (kr->old_ccname != NULL) {
>     ret = safe_remove_old_ccache_file(kr->old_ccname, kr->ccname);
>     if (ret != EOK) {
>
> @@ -837,12 +959,18 @@ static void krb5_child_done(struct tevent_req *subreq)
>  ret = krb5_save_ccname(state, state->be_ctx->sysdb,
>                         state->be_ctx->domain,
>                       pd->user, kr->ccname);
>                       pd->user, SYSDB_CCACHE_FILE, kr->ccname, 0);
>
> if (ret) {
>        DEBUG(1, ("krb5_save_ccname_send failed.\n"));
>        goto done;
>    }
>
> ret = krb5_ccname_decref(state, state->be_ctx->sysdb, state->be_ctx->domain,
>                         pd->user);
>
> if (ret) {
>    DEBUG(3, ("krb5_ccname_decref failed.\n"));
>
> }
>
> krb5_save_ccname_done(req);
>    return;
>
> diff --git a/src/providers/krb5/krb5_auth.h b/src/providers/krb5/krb5_auth.h
> index 130e85d..ae984f8 100644
> --- a/src/providers/krb5/krb5_auth.h
> +++ b/src/providers/krb5/krb5_auth.h
> @@ -43,6 +43,8 @@ struct krb5child_req {
>  const char *ccname;
>  const char *old_ccname;
> const char *new_ccname;
> int new_ccname_ref;
> const char *homedir;
> const char *upn;
> uid_t uid;
>
> -- 
> 1.7.3.1
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

[Jan Zelený]

Sumit Bose  wrote:
> On Sat, Nov 20, 2010 at 12:25:02AM +0100, Jan Zeleny wrote:
> > After some complications I finally made the patch solving ticket #533
> > (different ccache files during multiple simultaneous logins of the same
> > user). It is based on Simo's idea to determine the ccache file name in
> > advance and count references to it.
> >
> > Jan
>
> I'm not sure if it is worth writing the reference counter and the new
> file to the cache. Having them in a hash should be sufficient and
> the reference counter does not have to be reseted at startup (which is
> missing).
>
> The reference counter is set in krb5_auth_send() but decremented in
> krb5_child_done(). This should be moved to krb5_auth_done() before any
> error checking to make sure that it is done under all circumstances.
>
> bye,
> Sumit

I'm sending updated patch. Both your objections were valid and they are now 
resolved.

Jan

[Sumit Bose]

On Mon, Dec 06, 2010 at 09:25:01AM +0100, Jan Zelený wrote:
> Sumit Bose  wrote:
> > On Sat, Nov 20, 2010 at 12:25:02AM +0100, Jan Zeleny wrote:
> > > After some complications I finally made the patch solving ticket #533
> > > (different ccache files during multiple simultaneous logins of the same
> > > user). It is based on Simo's idea to determine the ccache file name in
> > > advance and count references to it.
> > >
> > > Jan
> >
> > I'm not sure if it is worth writing the reference counter and the new
> > file to the cache. Having them in a hash should be sufficient and
> > the reference counter does not have to be reseted at startup (which is
> > missing).
> >
> > The reference counter is set in krb5_auth_send() but decremented in
> > krb5_child_done(). This should be moved to krb5_auth_done() before any
> > error checking to make sure that it is done under all circumstances.
> >
> > bye,
> > Sumit
>
> I'm sending updated patch. Both your objections were valid and they are now 
> resolved.

Thank you, it looks much better now, but I'm afraid I still have some
comments:

- please rebase to the current master
- I would prefer to have the ccache_names hash to be a member of
  krb5_ctx instead of being a global variable
- please do not use hash_create() directly, but sss_hash_create(). There
  are still some place which uses hash_create_ex(), but I think the should
  be updated to use sss_hash_create() sooner or later, too.
- please use a suitable struct instead of the "ccname:ref_count" string
- it would be nice it you can add some helper functions to initialize
  and increment the ref count value so that the value itself is actually
  hidden from the caller

bye,
Sumit
> Jan