{"thread":{"id":"38284","subject":"git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","startedAt":"2015-01-05T19:07:24Z","lastAt":"2015-01-06T21:47:10Z","messageCount":14,"participants":["Paul Sokolovsky","Torsten Bögershausen","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"254297","messageId":"20150105210724.032e9718@x230","threadId":"38284","inReplyTo":null,"subject":"git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Paul Sokolovsky","fromEmail":"paul.sokolovsky@linaro.org","sentAt":"2015-01-05T19:07:24Z","receivedAt":"2015-01-05T19:07:24Z","isPatch":false,"sender":{"key":"paul.sokolovsky@linaro.org","avatar":null},"body":"Hello,\n\nWe recently upgraded to git 2.2.1 from 2.1.x and faced issue with\naccessing repositories over dump HTTP protocol. In our setting,\nrepositories are managed by Gerrit, so owned by Gerrit daemon user,\nbut we also offer anon access via smart and dumb HTTP protocols. For the\nlatter, we of course rely on \"git update-server-info\" being run.\n\nSo, after the upgrade, users started to report that accessing\ninfo/refs file of a repo, as required for HTTP dump protocol, leads to\n403 Forbidden HTTP error. We traced that to 0600 filesystem permissions\nfor such files (for objects/info/packs too) (owner is gerrit user, to\nremind). After resetting permissions to 0644, they get back to 0600\nafter some time (we have a cronjob in addition to a hook to run \"git\nupdate-server-info\"). umask is permissive when running cronjob (0002).\n\n\nI traced the issue to:\nhttps://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec\n\nIt says: \"Let's instead switch to using a unique tempfile via mkstemp.\"\nReading man mkstemp: \"The  file  is  created  with permissions 0600\".\nSo, that's it. The patch above contains call to adjust_shared_perm(),\nbut apparently it doesn't promote restrictive msktemp permissions to\nsomething more accessible.\n\nHope this issue can be addressed.\n\n\nThanks,\nPaul\n\nLinaro.org | Open source software for ARM SoCs\nFollow Linaro: http://www.facebook.com/pages/Linaro\nhttp://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog\n"},{"id":"254310","messageId":"54AB0ED0.3000400@web.de","threadId":"38284","inReplyTo":"20150105210724.032e9718@x230","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2015-01-05T22:23:12Z","receivedAt":"2015-01-05T22:23:12Z","isPatch":false,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2015-01-05 20.07, Paul Sokolovsky wrote:\n> Hello,\n> \n> We recently upgraded to git 2.2.1 from 2.1.x and faced issue with\n> accessing repositories over dump HTTP protocol. In our setting,\n> repositories are managed by Gerrit, so owned by Gerrit daemon user,\n> but we also offer anon access via smart and dumb HTTP protocols. For the\n> latter, we of course rely on \"git update-server-info\" being run.\n> \n> So, after the upgrade, users started to report that accessing\n> info/refs file of a repo, as required for HTTP dump protocol, leads to\n> 403 Forbidden HTTP error. We traced that to 0600 filesystem permissions\n> for such files (for objects/info/packs too) (owner is gerrit user, to\n> remind). After resetting permissions to 0644, they get back to 0600\n> after some time (we have a cronjob in addition to a hook to run \"git\n> update-server-info\"). umask is permissive when running cronjob (0002).\n> \n> \n> I traced the issue to:\n> https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec\n> \n> It says: \"Let's instead switch to using a unique tempfile via mkstemp.\"\n> Reading man mkstemp: \"The  file  is  created  with permissions 0600\".\n> So, that's it. The patch above contains call to adjust_shared_perm(),\n> but apparently it doesn't promote restrictive msktemp permissions to\n> something more accessible.\n> \n> Hope this issue can be addressed.\n> \n> \n> Thanks,\n> Paul\nDoes \ngit config core.sharedRepository 0644 \nhelp?\n\nUnless the the repo is configured as shared, \nadjust_shared_perm() will not widen the access bits:\n\nhttp://git-htmldocs.googlecode.com/git/git-config.html\n"},{"id":"254314","messageId":"20150106034702.GA11503@peff.net","threadId":"38284","inReplyTo":"20150105210724.032e9718@x230","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T03:47:02Z","receivedAt":"2015-01-06T03:47:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 05, 2015 at 09:07:24PM +0200, Paul Sokolovsky wrote:\n\n> So, after the upgrade, users started to report that accessing\n> info/refs file of a repo, as required for HTTP dump protocol, leads to\n> 403 Forbidden HTTP error. We traced that to 0600 filesystem permissions\n> for such files (for objects/info/packs too) (owner is gerrit user, to\n> remind). After resetting permissions to 0644, they get back to 0600\n> after some time (we have a cronjob in addition to a hook to run \"git\n> update-server-info\"). umask is permissive when running cronjob (0002).\n> \n> I traced the issue to:\n> https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec\n\nYeah, I didn't consider the mode impact of using mkstemp. That is\ndefinitely a regression that should be fixed. Though of course if you\nreally do want 0644, you should set your umask to 0022. :)\n\n> It says: \"Let's instead switch to using a unique tempfile via mkstemp.\"\n> Reading man mkstemp: \"The  file  is  created  with permissions 0600\".\n> So, that's it. The patch above contains call to adjust_shared_perm(),\n> but apparently it doesn't promote restrictive msktemp permissions to\n> something more accessible.\n\nIf you haven't set core.sharedrepository, then adjust_shared_perm is a\nnoop. But you shouldn't have to do that. Git should just respect your\numask in this case.\n\n> Hope this issue can be addressed.\n\nPatches to follow. Thanks for the report.\n\n  [1/2]: t1301: set umask in reflog sharedrepository=group test\n  [2/2]: update-server-info: create info/* with mode 0666\n\n-Peff\n"},{"id":"254315","messageId":"20150106034942.GA20087@peff.net","threadId":"38284","inReplyTo":"20150106034702.GA11503@peff.net","subject":"[PATCH 1/2] t1301: set umask in reflog sharedrepository=group test","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T03:49:43Z","receivedAt":"2015-01-06T03:49:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The t1301 script sets the umask globally before many of the\ntests. Most of the tests that care about the umask then set\nit explicitly at the start of the test. However, one test\ndoes not, and relies on the 077 umask setting from earlier\ntests. This is fragile and can break if another test is\nadded in between. Let's be more explicit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI suspect the world would be a better place if t1301 did all of its\numask setting in subshells, as it may also affect things like writing\nout the test results. But nobody has complained, so I'm not inclined to\nspend a lot of time futzing with it.\n\nThis is enough to protect the test I'm about to add in the next patch,\nso it's not worse than the status quo.\n\n t/t1301-shared-repo.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex de42d21..86ed901 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -112,6 +112,7 @@ do\n done\n \n test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n+\tumask 077 &&\n \tgit config core.sharedRepository group &&\n \tgit reflog expire --all &&\n \tactual=\"$(ls -l .git/logs/refs/heads/master)\" &&\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254316","messageId":"20150106035048.GB20087@peff.net","threadId":"38284","inReplyTo":"20150106034702.GA11503@peff.net","subject":"[PATCH 2/2] update-server-info: create info/* with mode 0666","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T03:50:49Z","receivedAt":"2015-01-06T03:50:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to d38379e (make update-server-info more robust,\n2014-09-13), we used a straight \"fopen\" to create the\ninfo/refs and objects/info/packs files, which creates the\nfile using mode 0666 (less the default umask).\n\nIn d38379e, we switched to creating the file with mkstemp\nto get a unique filename. But mkstemp also uses the more\nrestrictive 0600 mode to create the file. This was an\nunintended side effect that we did not want, and causes\nproblems when the repository is served by a different user\nthan the one running update-server-info (it is no longer\nreadable by a dumb http server running as `www`, for\nexample).\n\nWe can fix this by using git_mkstemp_mode and specifying\n0666.  Note that we could also say \"just use\ncore.sharedrepository\", as we do call adjust_shared_perm\non the result before renaming it into place.  But that is\nnot very friendly. The shared-repo config is usually about\nmaking things _writable_ for other users. Until d38379e,\nthere was no explicit config needed to serve an otherwise\nreadable repository, and we should consider it a\nregression.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n server-info.c          |  2 +-\n t/t1301-shared-repo.sh | 10 ++++++++++\n 2 files changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/server-info.c b/server-info.c\nindex 31f4a74..34b0253 100644\n--- a/server-info.c\n+++ b/server-info.c\n@@ -17,7 +17,7 @@ static int update_info_file(char *path, int (*generate)(FILE *))\n \tFILE *fp = NULL;\n \n \tsafe_create_leading_directories(path);\n-\tfd = mkstemp(tmp);\n+\tfd = git_mkstemp_mode(tmp, 0666);\n \tif (fd < 0)\n \t\tgoto out;\n \tfp = fdopen(fd, \"w\");\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex 86ed901..feff55e 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -111,6 +111,16 @@ do\n \n done\n \n+test_expect_success POSIXPERM 'info/refs is readable in unshared repo' '\n+\trm -f .git/info/refs &&\n+\ttest_unconfig core.sharedrepository &&\n+\tumask 002 &&\n+\tgit update-server-info &&\n+\techo \"-rw-rw-r--\" >expect &&\n+\tmodebits .git/info/refs >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n \tumask 077 &&\n \tgit config core.sharedRepository group &&\n-- \n2.2.1.425.g441bb3c\n"},{"id":"254320","messageId":"xmqqd26sql0v.fsf@gitster.dls.corp.google.com","threadId":"38284","inReplyTo":"20150106034702.GA11503@peff.net","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T10:08:16Z","receivedAt":"2015-01-06T10:08:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Yeah, I didn't consider the mode impact of using mkstemp. That is\n> definitely a regression that should be fixed. Though of course if you\n> really do want 0644, you should set your umask to 0022. :)\n> ...\n> If you haven't set core.sharedrepository, then adjust_shared_perm is a\n> noop. But you shouldn't have to do that. Git should just respect your\n> umask in this case.\n\nThanks for a nicely done patch series, but I am not sure if I agree\nwith the analysis and its conclusion.\n\nIf adjust_shared_perm is a no-op, how do we ensure that other files\nthat need to be served by a dumb HTTP server are readable by it?  Is\nit because we just happen not to use mkstemp() to create them (and\nalso is it because the pushers do not have umask 007 or stronger to\nprevent files from being read by the HTTP server user)?\n\nIs our goal here to give the users this statement?\n\n    For shared repository served by dumb HTTP and written by users\n    who are different from the user that runs the HTTP server, you\n    need to do nothing special.\n\nIf that is the case, shouldn't the rule be something a lot looser\nthan \"we should just respect your umask\"?  To satisify the above\ngoal, shouldn't we somehow make it readable by the HTTP user even\nwhen some pusher has a draconian 0077 umask?  But that, while still\ncomplying to the promise of \"nothing special\", would imply we would\nhave to make everything readable everywhere, whish is an unachievable\ngoal.  We need to somehow be able to say \"this repository should be\nreadable by these people\" per-repository basis.\n\nAnd we have a mechanism exactly designed to do so to defeat\ndraconian umask individual users have.\n\nIt feels to me that the old set-up were \"working\" by accident, not\nby design (I may be mistaken--so correct me if that were the case).\nAnd if that is the case, I do not think it is a good idea to try to\nhide the broken configuration under the rug longer.  \"As long as\neverybody writes world-readable files, you do not have to do\nanything\" will break when the next person with 0xx7 umask setting\npushes, no?\n"},{"id":"254330","messageId":"20150106141211.2ad83df4@x230","threadId":"38284","inReplyTo":"20150106034702.GA11503@peff.net","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Paul Sokolovsky","fromEmail":"paul.sokolovsky@linaro.org","sentAt":"2015-01-06T12:12:11Z","receivedAt":"2015-01-06T12:12:11Z","isPatch":false,"sender":{"key":"paul.sokolovsky@linaro.org","avatar":null},"body":"Hello,\n\nOn Mon, 5 Jan 2015 22:47:02 -0500\nJeff King <peff@peff.net> wrote:\n\n> On Mon, Jan 05, 2015 at 09:07:24PM +0200, Paul Sokolovsky wrote:\n> \n> > So, after the upgrade, users started to report that accessing\n> > info/refs file of a repo, as required for HTTP dump protocol, leads\n> > to 403 Forbidden HTTP error. We traced that to 0600 filesystem\n> > permissions for such files (for objects/info/packs too) (owner is\n> > gerrit user, to remind). After resetting permissions to 0644, they\n> > get back to 0600 after some time (we have a cronjob in addition to\n> > a hook to run \"git update-server-info\"). umask is permissive when\n> > running cronjob (0002).\n> > \n> > I traced the issue to:\n> > https://github.com/git/git/commit/d38379ece9216735ecc0ffd76c4c4e3da217daec\n> \n> Yeah, I didn't consider the mode impact of using mkstemp. That is\n> definitely a regression that should be fixed. Though of course if you\n> really do want 0644, you should set your umask to 0022. :)\n\nWell, group permissions are ok - we just need it to be world-readable,\nand that's not random, but complies with hosting requirements - our\nrepos are public otherwise.\n\n> > It says: \"Let's instead switch to using a unique tempfile via\n> > mkstemp.\" Reading man mkstemp: \"The  file  is  created  with\n> > permissions 0600\". So, that's it. The patch above contains call to\n> > adjust_shared_perm(), but apparently it doesn't promote restrictive\n> > msktemp permissions to something more accessible.\n> \n> If you haven't set core.sharedrepository, then adjust_shared_perm is a\n> noop. But you shouldn't have to do that. Git should just respect your\n> umask in this case.\n\nMy reference to adjust_shared_perm() was because I initially wanted to\nwrite \"apparently, it makes sense to do chmod after mkstemp()\", but I\nspotted that there's adjust_shared_perm() already, which does some\nshuffling of permissions.\n\n> > Hope this issue can be addressed.\n> \n> Patches to follow. Thanks for the report.\n> \n>   [1/2]: t1301: set umask in reflog sharedrepository=group test\n>   [2/2]: update-server-info: create info/* with mode 0666\n\nThanks much for the prompt reply and patches!\n\n> \n> -Peff\n\n\n\n-- \nBest Regards,\nPaul\n\nLinaro.org | Open source software for ARM SoCs\nFollow Linaro: http://www.facebook.com/pages/Linaro\nhttp://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog\n"},{"id":"254332","messageId":"20150106144322.61d7ff89@x230","threadId":"38284","inReplyTo":"xmqqd26sql0v.fsf@gitster.dls.corp.google.com","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Paul Sokolovsky","fromEmail":"paul.sokolovsky@linaro.org","sentAt":"2015-01-06T12:43:22Z","receivedAt":"2015-01-06T12:43:22Z","isPatch":false,"sender":{"key":"paul.sokolovsky@linaro.org","avatar":null},"body":"Hello,\n\nOn Tue, 06 Jan 2015 02:08:16 -0800\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Yeah, I didn't consider the mode impact of using mkstemp. That is\n> > definitely a regression that should be fixed. Though of course if\n> > you really do want 0644, you should set your umask to 0022. :)\n> > ...\n> > If you haven't set core.sharedrepository, then adjust_shared_perm\n> > is a noop. But you shouldn't have to do that. Git should just\n> > respect your umask in this case.\n> \n> Thanks for a nicely done patch series, but I am not sure if I agree\n> with the analysis and its conclusion.\n> \n> If adjust_shared_perm is a no-op, how do we ensure that other files\n> that need to be served by a dumb HTTP server are readable by it?\n\nJust don't make it unreadable on purpose (or by mistake) by git. The\nrest is taken care by OS.\n\n>   Is\n> it because we just happen not to use mkstemp() to create them (and\n> also is it because the pushers do not have umask 007 or stronger to\n> prevent files from being read by the HTTP server user)?\n> \n> Is our goal here to give the users this statement?\n> \n>     For shared repository served by dumb HTTP and written by users\n>     who are different from the user that runs the HTTP server, you\n>     need to do nothing special.\n>\n> If that is the case, shouldn't the rule be something a lot looser\n> than \"we should just respect your umask\"?  To satisify the above\n> goal, shouldn't we somehow make it readable by the HTTP user even\n> when some pusher has a draconian 0077 umask?\n\nI would dread such solution. umask is well-known Unix device to control\npermissions of created files. If someone sets it to 0077, they want\nnew files be not accessible by anyone but their owner, period. It\ndoesn't make sense to work that around. Or at least, it's different\nissue from the reported here.\n\n>  But that, while still\n> complying to the promise of \"nothing special\", would imply we would\n> have to make everything readable everywhere, whish is an unachievable\n> goal.  We need to somehow be able to say \"this repository should be\n> readable by these people\" per-repository basis.\n> \n> And we have a mechanism exactly designed to do so to defeat\n> draconian umask individual users have.\n\nI'm not sure I understand how this \"draconian umask\" got into picture\nhere at all. The original report was \"with liberal umask, there're\ndraconian file permissions\". Jeff's patch fixes exactly it. Transposing\n\"draconian\" into \"umask\" position make it completely different\nproblem.\n\n> \n> It feels to me that the old set-up were \"working\" by accident, not\n> by design (I may be mistaken--so correct me if that were the case).\n\nIf you mean our setup, I don't see anything wrong with it: we installed\ngit and apache from our distro, we installed Gerrit from the official\nsite, we made a cronjob to be run from gerrit user (as the owner of\nrepositories). Everything worked, as expected. Upgrading to git 2.2.1\nbroke it, because umask was not followed. What can be wrong here except\nnot following umask?\n\n> And if that is the case, I do not think it is a good idea to try to\n> hide the broken configuration under the rug longer.  \"As long as\n> everybody writes world-readable files, you do not have to do\n> anything\" will break when the next person with 0xx7 umask setting\n> pushes, no?\n\n\n\nThanks,\nPaul\n\nLinaro.org | Open source software for ARM SoCs\nFollow Linaro: http://www.facebook.com/pages/Linaro\nhttp://twitter.com/#!/linaroorg - http://www.linaro.org/linaro-blog\n"},{"id":"254354","messageId":"xmqqlhlfpx57.fsf@gitster.dls.corp.google.com","threadId":"38284","inReplyTo":"xmqqd26sql0v.fsf@gitster.dls.corp.google.com","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T18:44:04Z","receivedAt":"2015-01-06T18:44:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> Yeah, I didn't consider the mode impact of using mkstemp. That is\n>> definitely a regression that should be fixed. Though of course if you\n>> really do want 0644, you should set your umask to 0022. :)\n>> ...\n>> If you haven't set core.sharedrepository, then adjust_shared_perm is a\n>> noop. But you shouldn't have to do that. Git should just respect your\n>> umask in this case.\n>\n> Thanks for a nicely done patch series, but I am not sure if I agree\n> with the analysis and its conclusion.\n>\n> If adjust_shared_perm is a no-op, how do we ensure that other files\n> that need to be served by a dumb HTTP server are readable by it?  Is\n> it because we just happen not to use mkstemp() to create them (and\n> also is it because the pushers do not have umask 007 or stronger to\n> prevent files from being read by the HTTP server user)?\n>\n> Is our goal here to give the users this statement?\n>\n>     For shared repository served by dumb HTTP and written by users\n>     who are different from the user that runs the HTTP server, you\n>     need to do nothing special.\n>\n> If that is the case, shouldn't the rule be something a lot looser\n> than \"we should just respect your umask\"?  To satisify the above\n> goal, shouldn't we somehow make it readable by the HTTP user even\n> when some pusher has a draconian 0077 umask?  But that, while still\n> complying to the promise of \"nothing special\", would imply we would\n> have to make everything readable everywhere, whish is an unachievable\n> goal.  We need to somehow be able to say \"this repository should be\n> readable by these people\" per-repository basis.\n>\n> And we have a mechanism exactly designed to do so to defeat\n> draconian umask individual users have.\n>\n> It feels to me that the old set-up were \"working\" by accident, not\n> by design (I may be mistaken--so correct me if that were the case).\n> And if that is the case, I do not think it is a good idea to try to\n> hide the broken configuration under the rug longer.  \"As long as\n> everybody writes world-readable files, you do not have to do\n> anything\" will break when the next person with 0xx7 umask setting\n> pushes, no?\n\nHaving said all that, I agree that the patch series does the right\nthing in that it stops us from tightening without being told.  It's\njust that the change is not a general solution for \"you shouldn't\nhave to set core.sharedrepository even when people with different\numask settings push into the same repo\".\n"},{"id":"254355","messageId":"xmqqh9w3px0a.fsf@gitster.dls.corp.google.com","threadId":"38284","inReplyTo":"20150106035048.GB20087@peff.net","subject":"Re: [PATCH 2/2] update-server-info: create info/* with mode 0666","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T18:47:01Z","receivedAt":"2015-01-06T18:47:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> +test_expect_success POSIXPERM 'info/refs is readable in unshared repo' '\n> +\trm -f .git/info/refs &&\n> +\ttest_unconfig core.sharedrepository &&\n> +\tumask 002 &&\n> +\tgit update-server-info &&\n> +\techo \"-rw-rw-r--\" >expect &&\n> +\tmodebits .git/info/refs >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nHmm, the label and the test look somewhat out-of-sync.  \"readable as\nlong as umask allows it\" would be more in line with what the fix is\nabout (i.e. I would expect a test with that title to pass even if I\nchanged 'umask 002' to 'umask 007', but that is not what we want in\nthis series).\n\n\n\n>  test_expect_success POSIXPERM 'git reflog expire honors core.sharedRepository' '\n>  \tumask 077 &&\n>  \tgit config core.sharedRepository group &&\n"},{"id":"254363","messageId":"20150106193742.GA28440@peff.net","threadId":"38284","inReplyTo":"xmqqd26sql0v.fsf@gitster.dls.corp.google.com","subject":"Re: git 2.2.x: Unexpected, overstrict file permissions after \"git update-server-info\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T19:37:42Z","receivedAt":"2015-01-06T19:37:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 06, 2015 at 02:08:16AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Yeah, I didn't consider the mode impact of using mkstemp. That is\n> > definitely a regression that should be fixed. Though of course if you\n> > really do want 0644, you should set your umask to 0022. :)\n> > ...\n> > If you haven't set core.sharedrepository, then adjust_shared_perm is a\n> > noop. But you shouldn't have to do that. Git should just respect your\n> > umask in this case.\n> \n> Thanks for a nicely done patch series, but I am not sure if I agree\n> with the analysis and its conclusion.\n> \n> If adjust_shared_perm is a no-op, how do we ensure that other files\n> that need to be served by a dumb HTTP server are readable by it?  Is\n> it because we just happen not to use mkstemp() to create them (and\n> also is it because the pushers do not have umask 007 or stronger to\n> prevent files from being read by the HTTP server user)?\n\nI think there are two things at play here.\n\nOne is that we accidentally tightened the permissions on the info/*\nfiles, and that is a regression that should be fixed regardless. So the\npatch series is doing the right thing, even if the commit message is up\nfor debate. And I think you agree with that, from what you've written.\n\nAs for \"should it work\", I would tend to say yes. As long as \"work\" is\n\"respect your umask\". Git has no reason to do anything other than \"0666\n& umask\"[1] when creating new files. The umask is the traditional way to\nconfigure the permissions on files you create, and git should follow it,\nunless it happens to know a particular file is sensitive (and I cannot\nreally think of any that are, aside from a few related to credential\nstorage).\n\nSo if you do not have \"0004\" in your umask, everything git creates should\nbe world-readable, and other users should be able to access it.\n\nGrepping around, there are a few other calls to mkstemp (and not\nmkstemp_mode). But they are all for true temporary files (which will be\nread by subprocesses of the current process), and not files which we\nexpect to live on in the repo. So I think we are mostly following that\nrule already.\n\nThere are a couple spots where we use 0600 explicitly, for no good\nreason (e.g., some BISECT_* files, which I guess might be left in the\nfilesystem for later processes to read).\n\n[1] We actually use 0444 for object and packfile creation, but I think\n    that still follows the same line of reasoning.\n\n> Is our goal here to give the users this statement?\n> \n>     For shared repository served by dumb HTTP and written by users\n>     who are different from the user that runs the HTTP server, you\n>     need to do nothing special.\n\nNo, I don't think so. We should follow the umask, and in most cases that\nwill just work for serving by another user (and if it _doesn't_, then\nperhaps it is because the user with the draconian umask _wanted_ to\nprevent other people, including the http user, from reading it).\n\nAnd as you noted, if you want to override that umask, we already have\ncore.sharedrepository.\n\nSo maybe my commit message overstated things. And it should just say \"we\nshould be respecting the umask, because setting a permissive umask is\nenough to make dumb http work, and we broke that\".\n\n> It feels to me that the old set-up were \"working\" by accident, not\n> by design (I may be mistaken--so correct me if that were the case).\n\nI do not think it was consciously designed as part of git, but rather\nthat general good taste and fitting in with Unix traditions made it\nwork. We follow the umask, and the umask is typically enough to make it\nwork.\n\n> And if that is the case, I do not think it is a good idea to try to\n> hide the broken configuration under the rug longer.  \"As long as\n> everybody writes world-readable files, you do not have to do\n> anything\" will break when the next person with 0xx7 umask setting\n> pushes, no?\n\nYes, but it will be the fault of the person with the 0xx7 umask. ;)\n\n-Peff\n"},{"id":"254364","messageId":"20150106193950.GB28440@peff.net","threadId":"38284","inReplyTo":"xmqqh9w3px0a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] update-server-info: create info/* with mode 0666","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T19:39:51Z","receivedAt":"2015-01-06T19:39:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 06, 2015 at 10:47:01AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > +test_expect_success POSIXPERM 'info/refs is readable in unshared repo' '\n> > +\trm -f .git/info/refs &&\n> > +\ttest_unconfig core.sharedrepository &&\n> > +\tumask 002 &&\n> > +\tgit update-server-info &&\n> > +\techo \"-rw-rw-r--\" >expect &&\n> > +\tmodebits .git/info/refs >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Hmm, the label and the test look somewhat out-of-sync.  \"readable as\n> long as umask allows it\" would be more in line with what the fix is\n> about (i.e. I would expect a test with that title to pass even if I\n> changed 'umask 002' to 'umask 007', but that is not what we want in\n> this series).\n\nThat is definitely not what the series means to accomplish. I think\nnaming the test \"info/refs respects umask in unshared repo\" is probably\na better title for the test.\n\n-Peff\n"},{"id":"254371","messageId":"xmqqegr7oa9m.fsf@gitster.dls.corp.google.com","threadId":"38284","inReplyTo":"20150106193950.GB28440@peff.net","subject":"Re: [PATCH 2/2] update-server-info: create info/* with mode 0666","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-01-06T21:43:33Z","receivedAt":"2015-01-06T21:43:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jan 06, 2015 at 10:47:01AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > +test_expect_success POSIXPERM 'info/refs is readable in unshared repo' '\n>> > +\trm -f .git/info/refs &&\n>> > +\ttest_unconfig core.sharedrepository &&\n>> > +\tumask 002 &&\n>> > +\tgit update-server-info &&\n>> > +\techo \"-rw-rw-r--\" >expect &&\n>> > +\tmodebits .git/info/refs >actual &&\n>> > +\ttest_cmp expect actual\n>> > +'\n>> \n>> Hmm, the label and the test look somewhat out-of-sync.  \"readable as\n>> long as umask allows it\" would be more in line with what the fix is\n>> about (i.e. I would expect a test with that title to pass even if I\n>> changed 'umask 002' to 'umask 007', but that is not what we want in\n>> this series).\n>\n> That is definitely not what the series means to accomplish. I think\n> naming the test \"info/refs respects umask in unshared repo\" is probably\n> a better title for the test.\n\nThanks for sanity-checking me (I am still somewhat feverish and not\nperforming at 100% level).  Here is what I have locally (but haven't\ngot around to today's integration cycle yet) on top.\n\nSubject: [PATCH] SQUASH???\n\n---\n t/t1301-shared-repo.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t1301-shared-repo.sh b/t/t1301-shared-repo.sh\nindex feff55e..d5eacb0 100755\n--- a/t/t1301-shared-repo.sh\n+++ b/t/t1301-shared-repo.sh\n@@ -111,7 +111,7 @@ do\n \n done\n \n-test_expect_success POSIXPERM 'info/refs is readable in unshared repo' '\n+test_expect_success POSIXPERM 'info/refs is created honoring the umask' '\n \trm -f .git/info/refs &&\n \ttest_unconfig core.sharedrepository &&\n \tumask 002 &&\n-- \n2.2.1-349-g24d7964\n"},{"id":"254372","messageId":"20150106214710.GA457@peff.net","threadId":"38284","inReplyTo":"xmqqegr7oa9m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] update-server-info: create info/* with mode 0666","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-01-06T21:47:10Z","receivedAt":"2015-01-06T21:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 06, 2015 at 01:43:33PM -0800, Junio C Hamano wrote:\n\n> > That is definitely not what the series means to accomplish. I think\n> > naming the test \"info/refs respects umask in unshared repo\" is probably\n> > a better title for the test.\n> \n> Thanks for sanity-checking me (I am still somewhat feverish and not\n> performing at 100% level).  Here is what I have locally (but haven't\n> got around to today's integration cycle yet) on top.\n\nYeah, that looks fine. Do you think we need an update to the explanation\nin the commit message, or does it make sense in light of the\ndiscussion we've had?\n\n-Peff\n"}]}