Jeff King [off-list ref] writes:
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. :)
...
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.
Thanks for a nicely done patch series, but I am not sure if I agree
with the analysis and its conclusion.
If adjust_shared_perm is a no-op, how do we ensure that other files
that need to be served by a dumb HTTP server are readable by it? Is
it because we just happen not to use mkstemp() to create them (and
also is it because the pushers do not have umask 007 or stronger to
prevent files from being read by the HTTP server user)?
Is our goal here to give the users this statement?
For shared repository served by dumb HTTP and written by users
who are different from the user that runs the HTTP server, you
need to do nothing special.
If that is the case, shouldn't the rule be something a lot looser
than "we should just respect your umask"? To satisify the above
goal, shouldn't we somehow make it readable by the HTTP user even
when some pusher has a draconian 0077 umask? But that, while still
complying to the promise of "nothing special", would imply we would
have to make everything readable everywhere, whish is an unachievable
goal. We need to somehow be able to say "this repository should be
readable by these people" per-repository basis.
And we have a mechanism exactly designed to do so to defeat
draconian umask individual users have.
It feels to me that the old set-up were "working" by accident, not
by design (I may be mistaken--so correct me if that were the case).
And if that is the case, I do not think it is a good idea to try to
hide the broken configuration under the rug longer. "As long as
everybody writes world-readable files, you do not have to do
anything" will break when the next person with 0xx7 umask setting
pushes, no?
Hello,
On Tue, 06 Jan 2015 02:08:16 -0800
Junio C Hamano [off-list ref] wrote:
Jeff King [off-list ref] writes:
quoted
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. :)
...
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.
Thanks for a nicely done patch series, but I am not sure if I agree
with the analysis and its conclusion.
If adjust_shared_perm is a no-op, how do we ensure that other files
that need to be served by a dumb HTTP server are readable by it?
Just don't make it unreadable on purpose (or by mistake) by git. The
rest is taken care by OS.
Is
it because we just happen not to use mkstemp() to create them (and
also is it because the pushers do not have umask 007 or stronger to
prevent files from being read by the HTTP server user)?
Is our goal here to give the users this statement?
For shared repository served by dumb HTTP and written by users
who are different from the user that runs the HTTP server, you
need to do nothing special.
If that is the case, shouldn't the rule be something a lot looser
than "we should just respect your umask"? To satisify the above
goal, shouldn't we somehow make it readable by the HTTP user even
when some pusher has a draconian 0077 umask?
I would dread such solution. umask is well-known Unix device to control
permissions of created files. If someone sets it to 0077, they want
new files be not accessible by anyone but their owner, period. It
doesn't make sense to work that around. Or at least, it's different
issue from the reported here.
But that, while still
complying to the promise of "nothing special", would imply we would
have to make everything readable everywhere, whish is an unachievable
goal. We need to somehow be able to say "this repository should be
readable by these people" per-repository basis.
And we have a mechanism exactly designed to do so to defeat
draconian umask individual users have.
I'm not sure I understand how this "draconian umask" got into picture
here at all. The original report was "with liberal umask, there're
draconian file permissions". Jeff's patch fixes exactly it. Transposing
"draconian" into "umask" position make it completely different
problem.
It feels to me that the old set-up were "working" by accident, not
by design (I may be mistaken--so correct me if that were the case).
If you mean our setup, I don't see anything wrong with it: we installed
git and apache from our distro, we installed Gerrit from the official
site, we made a cronjob to be run from gerrit user (as the owner of
repositories). Everything worked, as expected. Upgrading to git 2.2.1
broke it, because umask was not followed. What can be wrong here except
not following umask?
And if that is the case, I do not think it is a good idea to try to
hide the broken configuration under the rug longer. "As long as
everybody writes world-readable files, you do not have to do
anything" will break when the next person with 0xx7 umask setting
pushes, no?
Thanks,
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
On Tue, Jan 06, 2015 at 02:08:16AM -0800, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
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. :)
...
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.
Thanks for a nicely done patch series, but I am not sure if I agree
with the analysis and its conclusion.
If adjust_shared_perm is a no-op, how do we ensure that other files
that need to be served by a dumb HTTP server are readable by it? Is
it because we just happen not to use mkstemp() to create them (and
also is it because the pushers do not have umask 007 or stronger to
prevent files from being read by the HTTP server user)?
I think there are two things at play here.
One is that we accidentally tightened the permissions on the info/*
files, and that is a regression that should be fixed regardless. So the
patch series is doing the right thing, even if the commit message is up
for debate. And I think you agree with that, from what you've written.
As for "should it work", I would tend to say yes. As long as "work" is
"respect your umask". Git has no reason to do anything other than "0666
& umask"[1] when creating new files. The umask is the traditional way to
configure the permissions on files you create, and git should follow it,
unless it happens to know a particular file is sensitive (and I cannot
really think of any that are, aside from a few related to credential
storage).
So if you do not have "0004" in your umask, everything git creates should
be world-readable, and other users should be able to access it.
Grepping around, there are a few other calls to mkstemp (and not
mkstemp_mode). But they are all for true temporary files (which will be
read by subprocesses of the current process), and not files which we
expect to live on in the repo. So I think we are mostly following that
rule already.
There are a couple spots where we use 0600 explicitly, for no good
reason (e.g., some BISECT_* files, which I guess might be left in the
filesystem for later processes to read).
[1] We actually use 0444 for object and packfile creation, but I think
that still follows the same line of reasoning.
Is our goal here to give the users this statement?
For shared repository served by dumb HTTP and written by users
who are different from the user that runs the HTTP server, you
need to do nothing special.
No, I don't think so. We should follow the umask, and in most cases that
will just work for serving by another user (and if it _doesn't_, then
perhaps it is because the user with the draconian umask _wanted_ to
prevent other people, including the http user, from reading it).
And as you noted, if you want to override that umask, we already have
core.sharedrepository.
So maybe my commit message overstated things. And it should just say "we
should be respecting the umask, because setting a permissive umask is
enough to make dumb http work, and we broke that".
It feels to me that the old set-up were "working" by accident, not
by design (I may be mistaken--so correct me if that were the case).
I do not think it was consciously designed as part of git, but rather
that general good taste and fitting in with Unix traditions made it
work. We follow the umask, and the umask is typically enough to make it
work.
And if that is the case, I do not think it is a good idea to try to
hide the broken configuration under the rug longer. "As long as
everybody writes world-readable files, you do not have to do
anything" will break when the next person with 0xx7 umask setting
pushes, no?
Yes, but it will be the fault of the person with the 0xx7 umask. ;)
-Peff