On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
> From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001
>From: Petr Cech<pc...@redhat.com>
>Date: Wed, 7 Oct 2015 08:57:15 -0400
>Subject: [PATCH 10/11] KRB5_CHILD: More restrictive umask
>
>We could use more restrictive umask in krb5_child. I found out that
>there is directory creation, but it is done by create_ccache_dir()
>which has its own umask setup.
>
>Resolves:
>https://fedorahosted.org/sssd/ticket/2424
>---
> src/providers/krb5/krb5_child.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
>index
69b7687188c04498f6ef7c10a1b5ca602daca8ef..be8db23df4660adcb59fcd2677b28ee415cd18d8
100644
>--- a/src/providers/krb5/krb5_child.c
>+++ b/src/providers/krb5/krb5_child.c
>@@ -720,7 +720,7 @@ static krb5_error_code create_ccache(char *ccname,
krb5_creds *creds)
> #endif
>
> /* Set a restrictive umask, just in case we end up creating any file */
>- umask(SSS_DFL_X_UMASK);
>+ umask(SSS_DFL_UMASK);
I think this change is OK, as you say, the directories might need the
executable flag, but then the directory-creating code should make sure
the permissions are more relaxed..
I checked it again. It is OK.
btw I tested both FILE ccache:
krb5_ccname_template =FILE:/tmp/ccache_%p.XXXXXX
the result looked OK to me:
# ll /tmp/ccache_ad...@ipa.test.KDaxgn
-rw-------. 1 admin admins 1041 Oct 12 09:14
/tmp/ccache_ad...@ipa.test.KDaxgn
and DIR ccache:
krb5_ccname_template = DIR:/tmp/ccaches/ccache_%p
also looked good:
# ll -d/tmp/ccaches/
drwx------. 3 admin admins 4096 Oct 12 09:31/tmp/ccaches/
# ll -d/tmp/ccaches/ccache_ad...@ipa.test/
drwx------. 2 admin admins 4096 Oct 12
09:31/tmp/ccaches/ccache_ad...@ipa.test/
# ll /tmp/ccaches/ccache_ad...@ipa.test
-rw-------. 1 admin admins 10 Oct 12 09:31 primary
-rw-------. 1 admin admins 1041 Oct 12 09:31 tktrg2WYD
>
> /* we create a new context here as the main process one may have been
> * opened as root and contain possibly references (even open handles ?)
>--
>2.4.3
>
> From 6085c5ce86e6ba79f29d2c18f6fceca9bab5cecb Mon Sep 17 00:00:00 2001
>From: Petr Cech<pc...@redhat.com>
>Date: Wed, 7 Oct 2015 09:32:12 -0400
>Subject: [PATCH 11/11] UTILS: Removing SSS_DFL_X UMASK constant
077 is still used in sss_unique_file(). So we can either use SSS_DFL_X
umask there or convert to non-executable umask. Either way, I think it's
OK to keep SSS_DFL_X even though it's unused right now for later use.
It's just a constant.
OK, SSS_DFL_X_UMASK is still here, but not used in code.
sss_unique_file is used to generate kdcinfo files, where non-x would be
OK because later we fchmod to 644 anyway:
ret = fchmod(fd, S_IRUSR|S_IWUSR|S_IRGRP|S_IROTH);
..and also used in gpo_cache_store_file() which uses the same pattern..
I rewrote DFL_X to DFL in sss_unique_file().
...then also in sss_unique_filename() which is used to create dummy
keytabs in ipa_server_trusted_dom_setup_1way(), handle_randomized() and
ldap_child_get_tgt_sync(). Now:
- ipa_server_trusted_dom_setup_1way() - safe to change, we only use it
to get a unique filename, the contents are filled with ipa-getkeytab
- handle_randomized() - safe to change, libkrb5 unlinks the unique
file later, so we just really need the filename
- ldap_child_get_tgt_sync() - ditto, only used as input for
krb5_cc_resolve()
The third patch is about redudant constant.
And at the end, there are may uses of umask() in CI tests, which I leave
how they are. They could be test relevant. Maybe I will touch it in some
future patch.
The last umask like constant is 644, which is connected to chmod(),
open(), etc. Do we want to have a constant for it?
Regards
Petr
>From 2613e2f0cf519664136cb2ff2fb6ef30b80b12b2 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Wed, 7 Oct 2015 08:57:15 -0400
Subject: [PATCH 1/3] KRB5_CHILD: More restrictive umask
We could use more restrictive umask in krb5_child. I found out that
there is directory creation, but it is done by create_ccache_dir()
which has its own umask setup.
Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
src/providers/krb5/krb5_child.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/providers/krb5/krb5_child.c b/src/providers/krb5/krb5_child.c
index 69b7687188c04498f6ef7c10a1b5ca602daca8ef..be8db23df4660adcb59fcd2677b28ee415cd18d8 100644
--- a/src/providers/krb5/krb5_child.c
+++ b/src/providers/krb5/krb5_child.c
@@ -720,7 +720,7 @@ static krb5_error_code create_ccache(char *ccname, krb5_creds *creds)
#endif
/* Set a restrictive umask, just in case we end up creating any file */
- umask(SSS_DFL_X_UMASK);
+ umask(SSS_DFL_UMASK);
/* we create a new context here as the main process one may have been
* opened as root and contain possibly references (even open handles ?)
--
2.4.3
>From 94ed2b917c487e8920427b8867e0acabb70f562f Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Wed, 21 Oct 2015 08:06:19 -0400
Subject: [PATCH 2/3] UTIL: More restrictive umask on sss_unique_file()
There is no need to have executable unique_file.
Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
src/util/util.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/util.c b/src/util/util.c
index f0925051a08970b191f61a533cd49dd53ae128e0..11924a3718e55352a73848ea422a7568d42f564a 100644
--- a/src/util/util.c
+++ b/src/util/util.c
@@ -1090,7 +1090,7 @@ int sss_unique_file(TALLOC_CTX *owner,
char *path_tmpl,
errno_t *_err)
{
- return sss_unique_file_ex(owner, path_tmpl, 077, _err);
+ return sss_unique_file_ex(owner, path_tmpl, SSS_DFL_UMASK, _err);
}
errno_t sss_unique_filename(TALLOC_CTX *owner, char *path_tmpl)
--
2.4.3
>From b9f1cf5f2db839c4cd22207784e9a6e0ea58b398 Mon Sep 17 00:00:00 2001
From: Petr Cech <pc...@redhat.com>
Date: Wed, 21 Oct 2015 08:27:37 -0400
Subject: [PATCH 3/3] TOOLS: DFL_UMASK --> SSS_DFL_UMASK
We could use SSS_DFL_UMASK instead of DFL_UMASK.
Resolves:
https://fedorahosted.org/sssd/ticket/2424
---
src/tools/sss_sync_ops.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/tools/sss_sync_ops.c b/src/tools/sss_sync_ops.c
index e6faedcae556b3c55a7d267b429eed46076119c9..5468929b691c6539cdf55f59be3560412e398f21 100644
--- a/src/tools/sss_sync_ops.c
+++ b/src/tools/sss_sync_ops.c
@@ -31,7 +31,6 @@
#define DFL_BASEDIR_VAL "/home"
#define DFL_CREATE_HOMEDIR true
#define DFL_REMOVE_HOMEDIR true
-#define DFL_UMASK 077
#define DFL_SKEL_DIR "/etc/skel"
#define DFL_MAIL_DIR "/var/spool/mail"
@@ -524,7 +523,7 @@ int useradd_defaults(TALLOC_CTX *mem_ctx,
/* umask to create homedirs */
ret = confdb_get_int(confdb,
conf_path, CONFDB_LOCAL_UMASK,
- DFL_UMASK, (int *) &data->umask);
+ SSS_DFL_UMASK, (int *) &data->umask);
if (ret != EOK) {
goto done;
}
--
2.4.3
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
