Re: git 2.2.x: Unexpected, overstrict file permissions after "git update-server-info"

From: Paul Sokolovsky <paul.sokolovsky@linaro.org>
Date: Jan 6, 2015, 12:12 UTC
Message-ID: <20150106141211.2ad83df4@x230>
In-Reply-To: <20150106034702.GA11503@peff.net>

Hello,

On Mon, 5 Jan 2015 22:47:02 -0500 Jeff King <peff@peff.net> wrote:

Show 17 quoted lines

> On Mon, Jan 05, 2015 at 09:07:24PM +0200, Paul Sokolovsky wrote:
> 
> > So, after the upgrade, users started to report that accessing
> > info/refs file of a repo, as required for HTTP dump protocol, leads
> > to 403 Forbidden HTTP error. We traced that to 0600 filesystem
> > permissions for such files (for objects/info/packs too) (owner is
> > gerrit user, to remind). After resetting permissions to 0644, they
> > get back to 0600 after some time (we have a cronjob in addition to
> > a hook to run "git update-server-info"). umask is permissive when
> > running cronjob (0002).
> > 
> > I traced the issue to:
> > https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec
> 
> Yeah, I didn't consider the mode impact of using mkstemp. That is
> definitely a regression that should be fixed. Though of course if you
> really do want 0644, you should set your umask to 0022. :)

Well, group permissions are ok - we just need it to be world-readable, and that's not random, but complies with hosting requirements - our repos are public otherwise.

Show 9 quoted lines

> > It says: "Let's instead switch to using a unique tempfile via
> > mkstemp." Reading man mkstemp: "The  file  is  created  with
> > permissions 0600". So, that's it. The patch above contains call to
> > adjust_shared_perm(), but apparently it doesn't promote restrictive
> > msktemp permissions to something more accessible.
> 
> If you haven't set core.sharedrepository, then adjust_shared_perm is a
> noop. But you shouldn't have to do that. Git should just respect your
> umask in this case.

My reference to adjust_shared_perm() was because I initially wanted to write "apparently, it makes sense to do chmod after mkstemp()", but I spotted that there's adjust_shared_perm() already, which does some shuffling of permissions.

Show 6 quoted lines

> > Hope this issue can be addressed.
> 
> Patches to follow. Thanks for the report.
> 
>   [1/2]: t1301: set umask in reflog sharedrepository=group test
>   [2/2]: update-server-info: create info/* with mode 0666

Thanks much for the prompt reply and patches!

> 
> -Peff

-- 
Best Regards,
Paul

Linaro.org | Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro
http://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog