[SSSD] Review of umask() in SSSD

Petr Cech pcech at redhat.com
Wed Oct 21 13:19:18 UTC 2015


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-KRB5_CHILD-More-restrictive-umask.patch
Type: text/x-patch
Size: 1161 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151021/eb466aab/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-UTIL-More-restrictive-umask-on-sss_unique_file.patch
Type: text/x-patch
Size: 933 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151021/eb466aab/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TOOLS-DFL_UMASK-SSS_DFL_UMASK.patch
Type: text/x-patch
Size: 1228 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20151021/eb466aab/attachment-0002.bin>


More information about the sssd-devel mailing list