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.
+
+ /* 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.
+
+ 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.
+ return;
+ }
+
+ talloc_set_destructor((TALLOC_CTX *)password, password_destructor);
+
+ ret = sysdb_cache_password(sysdb, pd->user, password);
+ if (ret) {
+ DEBUG(2, ("Failed to cache password, offline auth may not work."
+ " (%d)[%s]!?\n", ret, strerror(ret)));
+ /* password caching failures are not fatal errors */
+ }
+
+ talloc_zfree(tmp_ctx);
+}
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
