[SSSD] [PATCH 1/5] Fix tevent_req style for krb5_auth

Simo Sorce simo at redhat.com
Mon Nov 26 19:11:47 UTC 2012


On Mon, 2012-11-26 at 18:21 +0100, Pavel Březina wrote:
> On 11/24/2012 05:33 AM, Simo Sorce wrote:
> > No functionality changes,
> > just make the code respect the tevent_req style and naming conventions
> > and enhance readability by adding some helper functions.
> > ---
> 
> Good job on refactoring Simo. I have few comments inline.
> 
> > +static errno_t krb5_auth_prepare_ccache_file(struct krb5child_req *kr,
> > +                                             struct be_ctx *be_ctx,
> > +                                             int *pam_status, int *dp_err)
> > +{
> > +    const char *ccname_template;
> > +    bool private_path = false;
> > +    errno_t ret;
> > +
> 
> Can you put a comment here that would describe why we can't use:
> kr->is_offline = be_is_offline(be_ctx);
> 
> > +    if (!kr->is_offline) {
> > +        kr->is_offline = be_is_offline(be_ctx);
> > +    }
> > +    if (kr->is_offline) {
> > +        DEBUG(9, ("Preparing for offline operation.\n"));
> > +    }
> 
> Also I would separate those condition by new line to stress out that 
> they are not related. This way it really reads more like an if-else 
> statement with a typo.

Well it is a good question on why we do this, I don;t really know, I
just copied over what was being done before (I think).

> > +
> > +    /* The ccache file should be (re)created if one of the following conditions
> > +     * 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 and it is not a renewal
> > +     *   request
> > +     *   (!kr->is_offline && !kr->active_ccache && kr->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 && !kr->active_ccache && !kr->valid_tgt)
> > +     */
> > +    if (kr->ccname == NULL ||
> > +        (kr->is_offline && !kr->active_ccache && !kr->valid_tgt) ||
> > +        (!kr->is_offline && !kr->active_ccache && kr->pd->cmd != SSS_CMD_RENEW)) {
> > +            DEBUG(9, ("Recreating  ccache file.\n"));
> > +            ccname_template = dp_opt_get_cstring(kr->krb5_ctx->opts,
> > +                                                 KRB5_CCNAME_TMPL);
> > +            kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> > +                                                be_ctx->domain->case_sensitive,
> > +                                                &private_path);
> > +            if (kr->ccname == NULL) {
> > +                DEBUG(1, ("expand_ccname_template failed.\n"));
> > +                return ENOMEM;
> > +            }
> > +
> > +            if (kr->cc_be == NULL) {
> > +                kr->cc_be = get_cc_be_ops_ccache(kr->ccname);
> > +            }
> > +            if (kr->cc_be == NULL) {
> > +                DEBUG(SSSDBG_CRIT_FAILURE,
> > +                      ("Cannot get operations on new ccache %s\n", kr->ccname));
> > +                return EINVAL;
> > +            }
> 
> I'd prefer to insert this condition inside the first one, because you 
> are testing returned value of get_cc_be_ops_ccache() more then actual 
> content of the variable.

Yeah this makes sense, I'll fix it.

> > +
> > +            ret = kr->cc_be->create(kr->ccname,
> > +                                    kr->krb5_ctx->illegal_path_re,
> > +                                    kr->uid, kr->gid, private_path);
> > +            if (ret != EOK) {
> > +                DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
> > +                return ret;
> > +            }
> > +    } else {
> > +        DEBUG(SSSDBG_MINOR_FAILURE,
> > +              ("Saved ccache %s if of different type than ccache in "
> > +               "configuration file, reusing the old ccache\n",
> > +               kr->old_ccname));
> > +
> > +        kr->cc_be = get_cc_be_ops_ccache(kr->old_ccname);
> > +        if (kr->cc_be == NULL) {
> > +            DEBUG(SSSDBG_CRIT_FAILURE,
> > +                  ("Cannot get operations on saved ccache %s\n",
> > +                   kr->old_ccname));
> > +            return EINVAL;
> > +        }
> > +    }
> > +
> > +    return EOK;
> > +}
> > +
> > +static void krb5_auth_store_creds(struct sysdb_ctx *sysdb, struct pam_data *pd)
> > +{
> > +    TALLOC_CTX *tmp_ctx;
> > +    char *password = NULL;
> > +    int ret = EOK;
> > +
> > +    tmp_ctx = talloc_new(NULL);
> > +    if (!tmp_ctx) {
> > +        DEBUG(0, ("Out of memory when trying to store credentials\n"));
> > +        return;
> > +    }
> > +
> > +    switch(pd->cmd) {
> > +        case SSS_CMD_RENEW:
> > +            /* The authtok is set to the credential cache
> > +             * during renewal. We don't want to save this
> > +             * as the cached password.
> > +             */
> > +            break;
> > +        case SSS_PAM_AUTHENTICATE:
> > +        case SSS_PAM_CHAUTHTOK_PRELIM:
> > +            password = talloc_size(tmp_ctx, pd->authtok_size + 1);
> > +            if (password != NULL) {
> > +                memcpy(password, pd->authtok, pd->authtok_size);
> > +                password[pd->authtok_size] = '\0';
> > +            }
> > +            break;
> > +        case SSS_PAM_CHAUTHTOK:
> > +            password = talloc_size(tmp_ctx, pd->newauthtok_size + 1);
> > +            if (password != NULL) {
> > +                memcpy(password, pd->newauthtok, pd->newauthtok_size);
> > +                password[pd->newauthtok_size] = '\0';
> > +            }
> > +            break;
> > +        default:
> > +            DEBUG(0, ("unsupported PAM command [%d].\n", pd->cmd));
> > +    }
> > +
> > +    if (password == NULL) {
> > +        if (pd->cmd != SSS_CMD_RENEW) {
> > +            DEBUG(0, ("password not available, offline auth may not work.\n"));
> > +            /* password caching failures are not fatal errors */
> > +        }
> 
> You leak tmp_ctx here.

Great catch, I wish we had per-operation memory contexts, instead of
using NULL so much, it reduces the usefulness of talloc by reintroducing
a lot of hand freeing :/

Will fix.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list