# Mark remote `gc --auto` error messages 9 messages from 2016-06-02 to 2016-06-05. Participants: Lukas Fleischer, Junio C Hamano, Jeff King. Thread: https://gitlist.dev/t/42511 ## Lukas Fleischer, 2016-06-02 19:05 Subject: Mark remote `gc --auto` error messages Message-ID: <146489432847.688.11121862368709034386@typhoon> URL: https://gitlist.dev/e/146489432847.688.11121862368709034386%40typhoon ``` When running `git push`, it might occur that error messages are transferred from the server to the client. While most messages (those explicitly sent on sideband 2) are prefixed with "remote:", it seems that error messages printed during the automatic householding performed by git-gc(1) are displayed without any additional decoration. Thus, such messages can easily be misinterpreted as git-gc failing locally, see [1] for an actual example of where that happened. Do we want anything like the following patch (completely untested)? -- 8< -- diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a744437..15c323a 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) const char *argv_gc_auto[] = { "gc", "--auto", "--quiet", NULL, }; - int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR; + struct child_process proc = CHILD_PROCESS_INIT; + + proc.no_stdin = 1; + proc.stdout_to_stderr = 1; + proc.err = use_sideband ? -1 : 0; + proc.git_cmd = 1; + proc.argv = argv_gc_auto; + close_all_packs(); - run_command_v_opt(argv_gc_auto, opt); + if (!start_command(&proc)) { + if (use_sideband) + copy_to_sideband(proc.err, -1, NULL); + finish_command(&proc); + } } if (auto_update_server_info) update_server_info(0); -- 8< -- More generally, do we care about making *all* "remote" strings easily distinguishable from "local" strings? Even though it is unlikely to use this for an actual attack, it seems that a malicious server can currently trick a user into performing an action by printing a message that looks like something coming from "local" Git. Prefixing every server message by "remote:" might look a bit ugly but maybe we can simply use a different color instead and fall back to the prefix on terminals without color support. Opinions? [1] https://lists.archlinux.org/pipermail/aur-general/2016-June/032340.html ``` ## Junio C Hamano, 2016-06-02 19:33 Subject: Re: Mark remote `gc --auto` error messages Message-ID: URL: https://gitlist.dev/e/xmqqinxrtmgi.fsf%40gitster.mtv.corp.google.com In-Reply-To: <146489432847.688.11121862368709034386@typhoon> ``` Lukas Fleischer writes: > When running `git push`, it might occur that error messages are > transferred from the server to the client. While most messages (those > explicitly sent on sideband 2) are prefixed with "remote:", it seems > that error messages printed during the automatic householding performed > by git-gc(1) are displayed without any additional decoration. Thus, such > messages can easily be misinterpreted as git-gc failing locally, see [1] > for an actual example of where that happened. Sounds like a sensible goal to me. ``` ## Lukas Fleischer, 2016-06-02 20:06 Subject: Re: Mark remote `gc --auto` error messages Message-ID: <146489800609.1944.4398103814754920753@typhoon.lan> URL: https://gitlist.dev/e/146489800609.1944.4398103814754920753%40typhoon.lan In-Reply-To: ``` On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote: > Lukas Fleischer writes: > > > When running `git push`, it might occur that error messages are > > transferred from the server to the client. While most messages (those > > explicitly sent on sideband 2) are prefixed with "remote:", it seems > > that error messages printed during the automatic householding performed > > by git-gc(1) are displayed without any additional decoration. Thus, such > > messages can easily be misinterpreted as git-gc failing locally, see [1] > > for an actual example of where that happened. > > Sounds like a sensible goal to me. What exactly are you referring to (you only quoted the introduction)? Do you think we should fix the git-gc issue but keep the general behavior of printing messages unaltered? Do you think it would be worthwhile to make server messages distinguishable in general? ``` ## Junio C Hamano, 2016-06-02 20:14 Subject: Re: Mark remote `gc --auto` error messages Message-ID: URL: https://gitlist.dev/e/CAPc5daXVx1%3DptsKJEfEzXbjCNvwYxjAPyp_pob9CeR%2BQr3tG_g%40mail.gmail.com In-Reply-To: <146489800609.1944.4398103814754920753@typhoon.lan> ``` On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer wrote: > On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote: >> Lukas Fleischer writes: >> >> > When running `git push`, it might occur that error messages are >> > transferred from the server to the client. While most messages (those >> > explicitly sent on sideband 2) are prefixed with "remote:", it seems >> > that error messages printed during the automatic householding performed >> > by git-gc(1) are displayed without any additional decoration. Thus, such >> > messages can easily be misinterpreted as git-gc failing locally, see [1] >> > for an actual example of where that happened. >> >> Sounds like a sensible goal to me. > > What exactly are you referring to (you only quoted the introduction)? > Do you think we should fix the git-gc issue but keep the general > behavior of printing messages unaltered? Do you think it would be > worthwhile to make server messages distinguishable in general? The latter, which I think was what your implementation was attempting to do if I read it correctly. ``` ## Jeff King, 2016-06-02 21:48 Subject: Re: Mark remote `gc --auto` error messages Message-ID: <20160602214834.GB13356@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160602214834.GB13356%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote: > On Thu, Jun 2, 2016 at 1:06 PM, Lukas Fleischer wrote: > > On Thu, 02 Jun 2016 at 21:33:33, Junio C Hamano wrote: > >> Lukas Fleischer writes: > >> > >> > When running `git push`, it might occur that error messages are > >> > transferred from the server to the client. While most messages (those > >> > explicitly sent on sideband 2) are prefixed with "remote:", it seems > >> > that error messages printed during the automatic householding performed > >> > by git-gc(1) are displayed without any additional decoration. Thus, such > >> > messages can easily be misinterpreted as git-gc failing locally, see [1] > >> > for an actual example of where that happened. > >> > >> Sounds like a sensible goal to me. > > > > What exactly are you referring to (you only quoted the introduction)? > > Do you think we should fix the git-gc issue but keep the general > > behavior of printing messages unaltered? Do you think it would be > > worthwhile to make server messages distinguishable in general? > > The latter, which I think was what your implementation was attempting to do > if I read it correctly. I think the implementation is doing much more, but it is probably a good thing. Right now we do not send auto-gc output over the sideband, and its stderr goes to receive-pack's stderr. But that is a different place for different protocols. For git-over-https, it is probably apache's error log, or /dev/null if the server admin configured it. For ssh, it may be back over the ssh stderr channel, or it may go to a log or nowhere if the server admin intercepts receive-pack and redirects it. So the greater question is not "should this output be marked" but "should auto-gc data go over the sideband so that all clients see it (and any server-side stderr does not)". And I think the answer is probably yes. And that fixes the "remote: " thing as a side effect. If it were no, then this is not the right solution, and the solution is to swap out copy_to_sideband() for something that copies to stderr with "remote: " prepended, or something. -Peff ``` ## Jeff King, 2016-06-02 21:53 Subject: Re: Mark remote `gc --auto` error messages Message-ID: <20160602215317.GC13356@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160602215317.GC13356%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Jun 02, 2016 at 01:14:02PM -0700, Junio C Hamano wrote: > > What exactly are you referring to (you only quoted the introduction)? > > Do you think we should fix the git-gc issue but keep the general > > behavior of printing messages unaltered? Do you think it would be > > worthwhile to make server messages distinguishable in general? > > The latter, which I think was what your implementation was attempting to do > if I read it correctly. And btw, I don't think this patch fixes the general case. E.g., if receive-pack hits any of its die("BUG") lines, they will not be prefixed. Most clients wouldn't see them, but ssh ones would. To fix that you'd have to do a whole async process wrapping `receive-pack` that just reads its stdout and stderr and muxes it back over the sideband. But I can think of two roadblocks there: - I think the original design of receive-pack was _not_ to share all of stderr with the user, because it might contain secret-ish server-side things. That's why we have rp_error() which copies to the sideband. I don't know how useful that is in practice. We copy the stderr wholesale from sub-processes like index-pack, so things like file paths are likely to get leaked there. - the implementation is a bit tricky, because the die() will take down the mux thread, too. -Peff ``` ## Junio C Hamano, 2016-06-02 21:59 Subject: Re: Mark remote `gc --auto` error messages Message-ID: URL: https://gitlist.dev/e/xmqqmvn3s148.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20160602214834.GB13356@sigill.intra.peff.net> ``` Jeff King writes: > So the greater question is not "should this output be marked" but > "should auto-gc data go over the sideband so that all clients see it > (and any server-side stderr does not)". And I think the answer is > probably yes. And that fixes the "remote: " thing as a side effect. Thanks for stating this a lot more clearly than I could, and I agree that sending this to the other side regardless of the protocol is the right thing. I somehow doubt that server operators would check Apache logs to decide when to do a proper GC, so I do not consider it a true loss ;-) ``` ## Jeff King, 2016-06-02 22:04 Subject: Re: Mark remote `gc --auto` error messages Message-ID: <20160602220454.GA17202@sigill.intra.peff.net> URL: https://gitlist.dev/e/20160602220454.GA17202%40sigill.intra.peff.net In-Reply-To: ``` On Thu, Jun 02, 2016 at 02:59:51PM -0700, Junio C Hamano wrote: > Jeff King writes: > > > So the greater question is not "should this output be marked" but > > "should auto-gc data go over the sideband so that all clients see it > > (and any server-side stderr does not)". And I think the answer is > > probably yes. And that fixes the "remote: " thing as a side effect. > > Thanks for stating this a lot more clearly than I could, and I agree > that sending this to the other side regardless of the protocol is > the right thing. I somehow doubt that server operators would check > Apache logs to decide when to do a proper GC, so I do not consider > it a true loss ;-) I definitely agree. I'd wonder more about "would they want their users to see these details". I dunno. I am only intimately familiar with one git hosting site, and we turn off auto-gc completely. -Peff ``` ## Lukas Fleischer, 2016-06-05 09:36 Subject: [PATCH] receive-pack: send auto-gc output over sideband 2 Message-ID: <20160605093638.16533-1-lfleischer@lfos.de> URL: https://gitlist.dev/e/20160605093638.16533-1-lfleischer%40lfos.de In-Reply-To: <146489432847.688.11121862368709034386@typhoon> ``` Redirect auto-gc output to the sideband such that it is visible to all clients. As a side effect, all auto-gc error messages are now prefixed with "remote: " before being printed to stderr on the client-side which makes it easier to understand that those error messages originate from the server. Signed-off-by: Lukas Fleischer --- builtin/receive-pack.c | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index a744437..15c323a 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1775,9 +1775,20 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) const char *argv_gc_auto[] = { "gc", "--auto", "--quiet", NULL, }; - int opt = RUN_GIT_CMD | RUN_COMMAND_STDOUT_TO_STDERR; + struct child_process proc = CHILD_PROCESS_INIT; + + proc.no_stdin = 1; + proc.stdout_to_stderr = 1; + proc.err = use_sideband ? -1 : 0; + proc.git_cmd = 1; + proc.argv = argv_gc_auto; + close_all_packs(); - run_command_v_opt(argv_gc_auto, opt); + if (!start_command(&proc)) { + if (use_sideband) + copy_to_sideband(proc.err, -1, NULL); + finish_command(&proc); + } } if (auto_update_server_info) update_server_info(0); -- 2.8.3 ```