[SSSD] Review of umask() in SSSD

Lukas Slebodnik lslebodn at redhat.com
Thu Nov 5 15:09:58 UTC 2015


On (05/11/15 12:58), Jakub Hrozek wrote:
>On Thu, Nov 05, 2015 at 12:22:00PM +0100, Petr Cech wrote:
>> On 10/21/2015 03:19 PM, Petr Cech wrote:
>> >On 10/12/2015 11:37 AM, Jakub Hrozek wrote:
>> >>>> From a15acee2495ee12190e711f3344e14c54fc73062 Mon Sep 17 00:00:00 2001
>> >>>>From: Petr Cech<pcech at 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_admin at IPA.TEST.KDaxgn
>> >>     -rw-------. 1 admin admins 1041 Oct 12 09:14
>> >>/tmp/ccache_admin at 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_admin at IPA.TEST/
>> >>     drwx------. 2 admin admins 4096 Oct 12
>> >>09:31/tmp/ccaches/ccache_admin at IPA.TEST/
>> >>     # ll /tmp/ccaches/ccache_admin at 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<pcech at 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
>> >
>> >
>> >_______________________________________________
>> >sssd-devel mailing list
>> >sssd-devel at lists.fedorahosted.org
>> >https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> >
>> bump
>
>I was reviewing the patches this morning :-)
>
>ACK
>
>CI: http://sssd-ci.duckdns.org/logs/job/32/08/summary.html

master:
* 691387507c256e49845d4ca2a7bd558a2b41e4bb
* 56e067109659886408789c936d37c1e86fe46695
* fb75e886c2f203fe8c10e572cd4d8c635941678d

LS


More information about the sssd-devel mailing list