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<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 > > > > > >_______________________________________________ > >sssd-devel mailing list > >sssd-devel@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 _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel