On Wed, 2013-09-25 at 17:37 -0400, Simo Sorce wrote:
> On Tue, 2013-09-24 at 12:52 +0200, Jakub Hrozek wrote:
> > On Fri, Sep 20, 2013 at 01:09:34PM -0400, Simo Sorce wrote:
> > > On Mon, 2013-09-09 at 11:22 -0400, Simo Sorce wrote:
> > > > On Mon, 2013-09-09 at 13:41 +0200, Jakub Hrozek wrote:
> > > > > On Mon, Sep 09, 2013 at 01:25:30PM +0200, Sumit Bose wrote:
> > > > > > On Tue, Sep 03, 2013 at 10:52:14PM -0400, Simo Sorce wrote:
> > > > > > > Attached an untested possible alternative for #2071
> > > > > > >
> > > > > > > The other option is the last patch of my previous patchset in the
> > > > > > > thread
> > > > > > > named: "[SSSD] [PATCHES] Simplify credential krb5 cache
> > > > > > > manipulation".
> > > > > > >
> > > > > > > Let's discuss if any of these approaches is ok, or if a third
> > > > > > > way needs
> > > > > > > to be found.
> > > > > > >
> > > > > > > Simo.
> > > > > > >
> > > > > > > --
> > > > > > > Simo Sorce * Red Hat, Inc * New York
> > > > > >
> > > > > > (a short summary of the two patches in discussion)
> > > > > > The difference between the two approaches is that the patch in this
> > > > > > tread just sets the permission on all newly created directories to
> > > > > > "new_dir_mode = 0700" making them private by default. The patch in
> > > > > > the
> > > > > > other thread tries to improve the current scheme where SSSD tries to
> > > > > > figure out which directory which must be created is a public or a
> > > > > > private one and sets the mode accordingly.
> > > > > >
> > > > > > I agree with 'Setting up public directories is the job of the
> > > > > > admin' and
> > > > > > like the fact that the patch is making the SSSD code much simpler.
> > > > > > But I
> > > > > > assume that there are SSSD setups in use which depend on the
> > > > > > feature of
> > > > > > creating the directories with the "right" permissions. I can imagine
> > > > > > automatic installations e.g. with kickstart which does not create
> > > > > > the
> > > > > > needed directories but rely on SSSD to create them or installations
> > > > > > where the credential caches are stored into some RAM based file
> > > > > > system
> > > > > > where directories must be recreated on every restart. I do not
> > > > > > want to
> > > > > > discuss the correctness of those approaches or if the paths used to
> > > > > > store the credential caches are chosen wisely my point is just that
> > > > > > a
> > > > > > change might break existing installations.
> > > > > >
> > > > > > For this reason I would prefer to just improve the existing
> > > > > > behaviour.
> > > > > > But if a majority says that it is time for a change I will not argue
> > > > > > against it.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > >
> > > > > That's a good point.
> > > > >
> > > > > Could we get away with keeping the existing behaviour and printing a
> > > > > log
> > > > > message whenever we create a public directory saying that this feature
> > > > > might go away in a later release?
> > > >
> > > > I do not really think it is a good point.
> > > > A) I really do not think there is any user fo this feature, the only 2
> > > > places where people put stuff today are /tmp (FILE or DIR) and /run/user
> > > > (DIR).
> > > >
> > > > /tmp is already set up correctly (if not many other things will break
> > > > and SSSD should *not* mask the mess by creating the dir and further
> > > > confusing the admin.
> > > > /run/user is a private directory so making it public would be wrong.
> > > >
> > > > I think a change of behavior is in the best interest of our community
> > > > even if it ends up breaking 1 rare install that happened to know of this
> > > > new behavior and intentionally used it.
> > >
> > > Bump and attached find the 2 patches rebased both on top of my latest
> > > patchset sent to the list earlier.
> > >
> > > 0004 tries to maintain behavior, just saner.
> > > 0001 is the option to remove it completely.
> >
> > As agreed yesterday on a call, we're going to remove the option
> > completely. The patch no longer applies cleanly, not even with a 3-way
> > merge. I had to apply it with patch(1), so can you please re-send? One
> > minor comment inline:
>
> It doesn't apply because, it depends on the patch from the other
> patchset that you did not apply ("krb5: Improve ccache name checks")
Ok fixed the issues and rebase before the other patch, should apply
cleanly on master now.
Simo.
--
Simo Sorce * Red Hat, Inc * New York
>From 4ce662e733d258a426483e4c0222b2ab51b5f542 Mon Sep 17 00:00:00 2001
From: Simo Sorce <[email protected]>
Date: Tue, 3 Sep 2013 22:48:02 -0400
Subject: [PATCH] krb5: Remove ability to create public directories
Setting up public directories is the job of the admin, and
current sssd syntax can't express the actual intention of the admin with
regrads to which parts of the path should be public or private.
Resolves:
https://fedorahosted.org/sssd/ticket/2071
---
src/providers/krb5/krb5_auth.c | 7 ++-
src/providers/krb5/krb5_utils.c | 96 ++++++++++++++---------------------------
src/providers/krb5/krb5_utils.h | 6 +--
3 files changed, 38 insertions(+), 71 deletions(-)
diff --git a/src/providers/krb5/krb5_auth.c b/src/providers/krb5/krb5_auth.c
index 52e230ca98ec827654cb0321a071fbaa77f7e4c6..7478d5a5a69976c389fb7357f98017e566c8e163 100644
--- a/src/providers/krb5/krb5_auth.c
+++ b/src/providers/krb5/krb5_auth.c
@@ -292,7 +292,7 @@ static errno_t krb5_auth_prepare_ccache_name(struct krb5child_req *kr,
struct be_ctx *be_ctx)
{
const char *ccname_template;
- bool private_path = false;
+ const char *realm;
errno_t ret;
if (!kr->is_offline) {
@@ -317,8 +317,7 @@ static errno_t krb5_auth_prepare_ccache_name(struct krb5child_req *kr,
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);
+ be_ctx->domain->case_sensitive);
if (kr->ccname == NULL) {
DEBUG(1, ("expand_ccname_template failed.\n"));
return ENOMEM;
@@ -326,7 +325,7 @@ static errno_t krb5_auth_prepare_ccache_name(struct krb5child_req *kr,
ret = sss_krb5_precreate_ccache(kr->ccname,
kr->krb5_ctx->illegal_path_re,
- kr->uid, kr->gid, private_path);
+ kr->uid, kr->gid);
if (ret != EOK) {
DEBUG(SSSDBG_OP_FAILURE, ("ccache creation failed.\n"));
return ret;
diff --git a/src/providers/krb5/krb5_utils.c b/src/providers/krb5/krb5_utils.c
index ba587408c83ad4aa642dddd42dcb1b3b221fa133..3f273e8d795916dbc70eb578b7a65993f3639dab 100644
--- a/src/providers/krb5/krb5_utils.c
+++ b/src/providers/krb5/krb5_utils.c
@@ -203,7 +203,7 @@ done:
char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
const char *template, bool file_mode,
- bool case_sensitive, bool *private_path)
+ bool case_sensitive)
{
char *copy;
char *p;
@@ -217,8 +217,6 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
char action;
bool rerun;
- *private_path = false;
-
if (template == NULL) {
DEBUG(1, ("Missing template.\n"));
return NULL;
@@ -269,7 +267,6 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
result = talloc_asprintf_append(result, "%s%s", p,
name);
- if (!file_mode) *private_path = true;
break;
case 'U':
if (kr->uid <= 0) {
@@ -279,7 +276,6 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
}
result = talloc_asprintf_append(result, "%s%"SPRIuid, p,
kr->uid);
- if (!file_mode) *private_path = true;
break;
case 'p':
if (kr->upn == NULL) {
@@ -288,7 +284,6 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
goto done;
}
result = talloc_asprintf_append(result, "%s%s", p, kr->upn);
- if (!file_mode) *private_path = true;
break;
case '%':
result = talloc_asprintf_append(result, "%s%%", p);
@@ -308,7 +303,6 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
goto done;
}
result = talloc_asprintf_append(result, "%s%s", p, kr->homedir);
- if (!file_mode) *private_path = true;
break;
case 'd':
if (file_mode) {
@@ -320,8 +314,7 @@ char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
}
dummy = expand_ccname_template(tmp_ctx, kr, cache_dir_tmpl,
- false, case_sensitive,
- private_path);
+ false, case_sensitive);
if (dummy == NULL) {
DEBUG(1, ("Expanding credential cache directory "
"template failed.\n"));
@@ -414,41 +407,30 @@ done:
return res;
}
-static errno_t check_parent_stat(bool private_path, struct stat *parent_stat,
+static errno_t check_parent_stat(struct stat *parent_stat,
uid_t uid, gid_t gid)
{
- if (private_path) {
- if (!((parent_stat->st_uid == 0 && parent_stat->st_gid == 0) ||
- parent_stat->st_uid == uid)) {
- DEBUG(1, ("Private directory can only be created below a "
- "directory belonging to root or to "
- "[%"SPRIuid"][%"SPRIgid"].\n", uid, gid));
- return EINVAL;
- }
+ if (!((parent_stat->st_uid == 0 && parent_stat->st_gid == 0) ||
+ parent_stat->st_uid == uid)) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Private directory can only be created below a directory "
+ "belonging to root or to [%"SPRIuid"][%"SPRIgid"].\n",
+ uid, gid));
+ return EINVAL;
+ }
- if (parent_stat->st_uid == uid) {
- if (!(parent_stat->st_mode & S_IXUSR)) {
- DEBUG(1, ("Parent directory does have the search bit set for "
- "the owner.\n"));
- return EINVAL;
- }
- } else {
- if (!(parent_stat->st_mode & S_IXOTH)) {
- DEBUG(1, ("Parent directory does have the search bit set for "
- "others.\n"));
- return EINVAL;
- }
+ if (parent_stat->st_uid == uid) {
+ if (!(parent_stat->st_mode & S_IXUSR)) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Parent directory does not have the search bit set for "
+ "the owner.\n"));
+ return EINVAL;
}
} else {
- if (parent_stat->st_uid != 0 || parent_stat->st_gid != 0) {
- DEBUG(1, ("Public directory cannot be created below a user "
- "directory.\n"));
- return EINVAL;
- }
-
if (!(parent_stat->st_mode & S_IXOTH)) {
- DEBUG(1, ("Parent directory does have the search bit set for "
- "others.\n"));
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ ("Parent directory does not have the search bit set for "
+ "others.\n"));
return EINVAL;
}
}
@@ -559,7 +541,7 @@ check_ccache_re(const char *filename, pcre *illegal_re)
errno_t
create_ccache_dir(const char *ccdirname, pcre *illegal_re,
- uid_t uid, gid_t gid, bool private_path)
+ uid_t uid, gid_t gid)
{
int ret = EFAULT;
struct stat parent_stat;
@@ -598,27 +580,17 @@ create_ccache_dir(const char *ccdirname, pcre *illegal_re,
goto done;
}
- ret = check_parent_stat(private_path, &parent_stat, uid, gid);
+ ret = check_parent_stat(&parent_stat, uid, gid);
if (ret != EOK) {
DEBUG(SSSDBG_MINOR_FAILURE,
- ("check_parent_stat failed for %s directory [%s].\n",
- private_path ? "private" : "public", ccdirname));
+ ("check_parent_stat failed for directory [%s].\n", ccdirname));
goto done;
}
DLIST_FOR_EACH(li, missing_parents) {
DEBUG(SSSDBG_TRACE_INTERNAL,
("Creating directory [%s].\n", li->s));
- if (li->next == NULL) {
- new_dir_mode = private_path ? 0700 : 01777;
- } else {
- if (private_path &&
- parent_stat.st_uid == uid && parent_stat.st_gid == gid) {
- new_dir_mode = 0700;
- } else {
- new_dir_mode = 0755;
- }
- }
+ new_dir_mode = 0700;
old_umask = umask(0000);
ret = mkdir(li->s, new_dir_mode);
@@ -630,16 +602,12 @@ create_ccache_dir(const char *ccdirname, pcre *illegal_re,
strerror(ret)));
goto done;
}
- if (private_path &&
- ((parent_stat.st_uid == uid && parent_stat.st_gid == gid) ||
- li->next == NULL)) {
- ret = chown(li->s, uid, gid);
- if (ret != EOK) {
- ret = errno;
- DEBUG(SSSDBG_MINOR_FAILURE,
- ("chown failed [%d][%s].\n", ret, strerror(ret)));
- goto done;
- }
+ ret = chown(li->s, uid, gid);
+ if (ret != EOK) {
+ ret = errno;
+ DEBUG(SSSDBG_MINOR_FAILURE,
+ ("chown failed [%d][%s].\n", ret, strerror(ret)));
+ goto done;
}
}
@@ -758,7 +726,7 @@ done:
}
errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re,
- uid_t uid, gid_t gid, bool private_path)
+ uid_t uid, gid_t gid)
{
TALLOC_CTX *tmp_ctx = NULL;
const char *filename;
@@ -802,7 +770,7 @@ errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re,
*end = '\0';
} while (*(end+1) == '\0');
- ret = create_ccache_dir(ccdirname, illegal_re, uid, gid, private_path);
+ ret = create_ccache_dir(ccdirname, illegal_re, uid, gid);
done:
talloc_free(tmp_ctx);
return ret;
diff --git a/src/providers/krb5/krb5_utils.h b/src/providers/krb5/krb5_utils.h
index 33cc6112b02af6991ef4aa4f1988dcbe08ed9266..4b1ebb0bb7a9e13d68ee62820f6408d029a2f072 100644
--- a/src/providers/krb5/krb5_utils.h
+++ b/src/providers/krb5/krb5_utils.h
@@ -43,11 +43,11 @@ errno_t check_if_cached_upn_needs_update(struct sysdb_ctx *sysdb,
const char *upn);
errno_t create_ccache_dir(const char *dirname, pcre *illegal_re,
- uid_t uid, gid_t gid, bool private_path);
+ uid_t uid, gid_t gid);
char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
const char *template, bool file_mode,
- bool case_sensitive, bool *private_path);
+ bool case_sensitive);
errno_t become_user(uid_t uid, gid_t gid);
struct sss_creds;
@@ -58,7 +58,7 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
errno_t restore_creds(struct sss_creds *saved_creds);
errno_t sss_krb5_precreate_ccache(const char *ccname, pcre *illegal_re,
- uid_t uid, gid_t gid, bool private_path);
+ uid_t uid, gid_t gid);
errno_t sss_krb5_cc_destroy(const char *ccname, uid_t uid, gid_t gid);
errno_t sss_krb5_check_ccache_princ(uid_t uid, gid_t gid,
const char *ccname, const char *principal);
--
1.8.3.1
_______________________________________________
sssd-devel mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
