{"thread":{"id":"40271","subject":"[RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","startedAt":"2015-09-04T10:52:45Z","lastAt":"2015-09-08T21:40:19Z","messageCount":12,"participants":["Giuseppe Bilotta","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269409","messageId":"CAOxFTcx9E_FtYD_Jn3+S3j-rwGO+hJSVXUC2S5ZBB6der7dUuw@mail.gmail.com","threadId":"40271","inReplyTo":null,"subject":"[RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2015-09-04T10:52:45Z","receivedAt":"2015-09-04T10:52:45Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Hello all,\n\n\nI'm going to tell a tale of the oddest bug I've ever come across in\nGit and aks for suggestions on how we can improve the user experience\nin a sensible way.\n\nI've been stymied for days trying to solve the following issue, which\nI came across every time I tried to push to a specific machine when\nusing git 2.5 (I'm honestly not sure about which version introduced\nthis, and I haven't runthe necessary bisection due to lack of time,\nsorry).\n\nTrying to push any changes with 2.5 resulted in this kind of failure:\n\nuser@clientmachine:~/some/git/workdir $ git push\nCounting objects: 6, done.\nDelta compression using up to 8 threads.\nCompressing objects: 100% (5/5), done.\nWriting objects: 100% (6/6), 841 bytes | 0 bytes/s, done.\nTotal 6 (delta 2), reused 0 (delta 0)\nfatal: Could not switch to '/home/user/some/git': No such file or directory\nerror: unpack failed: unpack-objects abnormal exit\nfatal: Could not switch to '/home/user/some/git': No such file or directory\nTo git@remote.machine:remote-repo\n! [remote rejected] master -> master (n/a (unpacker error))\nerror: failed to push some refs to 'git@remote.machine:remote-repo'\n\nNotice two things: the messages refer to the worktree updir of the\nCLIENT machine, and even though it's _completely not obvious_ due to\nthe missing 'remote:' lines, the messages actually come from the\nSERVER. The lack of indicator lines _alone_ took me hours of debugging\nbefore I finally understood that they were coming from the other side\n(yes, I woud say that it's a bug), but after this I was even more\nperplexed: why the heck would the SERVER try to switch to the updir of\nthe CLIENT worktree?\n\nI still couldn't do much on the SERVER to debug due to a variety of\nreasons, but I finally had a suspicion: it was almost as if the SERVER\nwas getting the GIT_DIR information from the CLIENT. And why the heck\nwould _that_ be the case?\n\nI then remembered that the server was actually configured to AcceptEnv\nGIT_* in sshd_config, for reasons related to git identity preservation\ndespite single login account (please don't ask). Turning the AcceptEnv\nto a stricter GIT_AUTHOR* and GIT_COMMITTER* solved the issue.\n\nNow, a couple of questions:\n\n1. since when have git internals started exporting GIT_* variables\nrelated to the git dir and worktree location?\n2. is it worth making sure that these don't get propagated via ssh?\n\nRegarding 2., there are actually webpages around suggesting the\nAcceptEnv GIT_* trick, and these setups are _all_ going to break with\nthe latest git, so this is something that might be worth looking into.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"269414","messageId":"20150904125448.GA25501@sigill.intra.peff.net","threadId":"40271","inReplyTo":"CAOxFTcx9E_FtYD_Jn3+S3j-rwGO+hJSVXUC2S5ZBB6der7dUuw@mail.gmail.com","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T12:54:49Z","receivedAt":"2015-09-04T12:54:49Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 12:52:45PM +0200, Giuseppe Bilotta wrote:\n\n> Trying to push any changes with 2.5 resulted in this kind of failure:\n> \n> user@clientmachine:~/some/git/workdir $ git push\n> Counting objects: 6, done.\n> Delta compression using up to 8 threads.\n> Compressing objects: 100% (5/5), done.\n> Writing objects: 100% (6/6), 841 bytes | 0 bytes/s, done.\n> Total 6 (delta 2), reused 0 (delta 0)\n> fatal: Could not switch to '/home/user/some/git': No such file or directory\n> error: unpack failed: unpack-objects abnormal exit\n> fatal: Could not switch to '/home/user/some/git': No such file or directory\n> To git@remote.machine:remote-repo\n> ! [remote rejected] master -> master (n/a (unpacker error))\n> error: failed to push some refs to 'git@remote.machine:remote-repo'\n>\n> Notice two things: the messages refer to the worktree updir of the\n> CLIENT machine, and even though it's _completely not obvious_ due to\n> the missing 'remote:' lines, the messages actually come from the\n> SERVER. The lack of indicator lines _alone_ took me hours of debugging\n> before I finally understood that they were coming from the other side\n\nOlder versions of receive-pack would let unpack-objects output go\nstraight to stderr, but that changed in a22e6f8 (receive-pack: send\npack-processing stderr over sideband, 2012-09-21), which is in git\nv1.7.12.3. What version of git is running on the remote server?\n\nE.g., even without going over ssh, if I do:\n\n  git init\n  echo content >file && git add file && git commit -m foo\n  git init --bare dst.git\n  # force unpacker to fail\n  chmod -w dst.git/objects\n  git push dst.git\n\nI get:\n\n  Counting objects: 3, done.\n  Writing objects: 100% (3/3), 205 bytes | 0 bytes/s, done.\n  Total 3 (delta 0), reused 0 (delta 0)\n  remote: error: insufficient permission for adding an object to repository database ./objects\n  remote: fatal: failed to write object\n  error: unpack failed: unpack-objects abnormal exit\n  To dst.git\n   ! [remote rejected] master -> master (unpacker error)\n  error: failed to push some refs to 'dst.git'\n\nThe \"unpack failed\" line _does_ come from the remote, but comes straight\nfrom receive-pack, not the child unpack-objects. Receive-pack\ndistinguishes between errors which should go to the client and which\njust go to stderr for debugging (remember that not all transports\nactually propagate stderr to the client; ssh is special here). It's\npossible we could switch the \"unpack failed\" to go to the client, but it\nis redundant with the \"unpacker error\" which _does_ go to the client.\n\n> I still couldn't do much on the SERVER to debug due to a variety of\n> reasons, but I finally had a suspicion: it was almost as if the SERVER\n> was getting the GIT_DIR information from the CLIENT. And why the heck\n> would _that_ be the case?\n> \n> I then remembered that the server was actually configured to AcceptEnv\n> GIT_* in sshd_config, for reasons related to git identity preservation\n> despite single login account (please don't ask). Turning the AcceptEnv\n> to a stricter GIT_AUTHOR* and GIT_COMMITTER* solved the issue.\n\nI couldn't reproduce this problem, either during a local push, or across\nan ssh session (where the client has \"SendEnv GIT_*\" and the server has\n\"AcceptEnv GIT_*\").\n\nIn the local case, we explicitly unset GIT_DIR and related variables in\nconnect.c:git_connect. We don't seem to do so for the ssh case, though.\nI can confirm that the variable makes it across to the remote:\n\n  GIT_DIR=$PWD git push \\\n    --receive-pack='echo >&2 GIT_DIR=$GIT_DIR; git-receive-pack' \\\n    remote-host:remote-repo\n\nshows our local $PWD on the remote side (though note that you have to\nexplicitly set $GIT_DIR; git-push does not do so normally).\n\nOn the receiving side, git-receive-pack takes an argument for the repo\npath, and calls enter_repo. That should result in calling set_git_dir(),\nwhich overwrites $GIT_DIR in the environment. AFAICT, it has always done\nso. So I'm not sure how GIT_DIR would leak through, even on an older\nversion of git.\n\n> 1. since when have git internals started exporting GIT_* variables\n> related to the git dir and worktree location?\n\nIt has done so for a long time, though the exact rules for doing so\nchanges from time to time. Browsing \"git log\", I couldn't find any\nrecent changes in this area. It would help if you could bisect the\nproblem, as I can't manage to replicate it.\n\n> 2. is it worth making sure that these don't get propagated via ssh?\n\nIt seems like a reasonable idea for git_connect() to do so in the ssh\ncase, as well as the local case. That _could_ be a regression for\nsomebody who uses an ssh-wrapper whose behavior changes based on some\n$GIT_* variable, but that seems a bit far-fetched.\n\nIt shouldn't be necessary for $GIT_DIR, but it makes sense for other git\nvariables. E.g., with \"AcceptEnv GIT_*\", \"git -c\" config is propagated.\nE.g.:\n\n  # make a syntactically bogus commit\n  commit=$(git cat-file commit HEAD |\n           sed 's/>/>>/' |\n\t   git hash-object -t commit -w --stdin)\n  git update-ref HEAD $commit\n  # confirm that git complains\n  git fsck\n\n  # confirm that it gets blocked by receive.fsckObjects; the \"-c\" is\n  # applied to the receive-pack directly, so we are already in the\n  # \"remote\" repo.\n  git push --receive-pack='git -c receive.fsckobjects=true receive-pack' dst.git\n\n  # now let's try it with a local \"-c\". This doesn't get propagated, as\n  # we clean out any environment-level config before running\n  # receive-pack in the \"remote\". As a result, the push succeeds.\n  git -c receive.fsckobjects=true push dst.git\n\n  # and now the same thing over ssh; this _will_ complain, as we\n  # propagated the config.\n  git -c receive.fsckObjects=true push remote:dst.git\n\n-Peff\n"},{"id":"269420","messageId":"CAOxFTcwYJqFHrBkie19Fm3AqeYmSLRNBpBtOAkiYmfO9UgN-0g@mail.gmail.com","threadId":"40271","inReplyTo":"20150904125448.GA25501@sigill.intra.peff.net","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2015-09-04T14:02:09Z","receivedAt":"2015-09-04T14:02:09Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"Hello,\n\nOn Fri, Sep 4, 2015 at 2:54 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 04, 2015 at 12:52:45PM +0200, Giuseppe Bilotta wrote:\n>\n>> Notice two things: the messages refer to the worktree updir of the\n>> CLIENT machine, and even though it's _completely not obvious_ due to\n>> the missing 'remote:' lines, the messages actually come from the\n>> SERVER. The lack of indicator lines _alone_ took me hours of debugging\n>> before I finally understood that they were coming from the other side\n>\n> Older versions of receive-pack would let unpack-objects output go\n> straight to stderr, but that changed in a22e6f8 (receive-pack: send\n> pack-processing stderr over sideband, 2012-09-21), which is in git\n> v1.7.12.3. What version of git is running on the remote server?\n\nSorry, I forgot to mention this in my origina email, I should have.\n\nThe remote is quite ancient: git version 1.7.0.4\n\n>> I still couldn't do much on the SERVER to debug due to a variety of\n>> reasons, but I finally had a suspicion: it was almost as if the SERVER\n>> was getting the GIT_DIR information from the CLIENT. And why the heck\n>> would _that_ be the case?\n>>\n>> I then remembered that the server was actually configured to AcceptEnv\n>> GIT_* in sshd_config, for reasons related to git identity preservation\n>> despite single login account (please don't ask). Turning the AcceptEnv\n>> to a stricter GIT_AUTHOR* and GIT_COMMITTER* solved the issue.\n>\n> I couldn't reproduce this problem, either during a local push, or across\n> an ssh session (where the client has \"SendEnv GIT_*\" and the server has\n> \"AcceptEnv GIT_*\").\n\n[snip]\n\n> On the receiving side, git-receive-pack takes an argument for the repo\n> path, and calls enter_repo. That should result in calling set_git_dir(),\n> which overwrites $GIT_DIR in the environment. AFAICT, it has always done\n> so. So I'm not sure how GIT_DIR would leak through, even on an older\n> version of git.\n\nHm, aside from the ancient version of git, the other side also has\ngitolite2  running (can you tell it's an ancient installation that\nneeds upgrading?).  However,I've done a quick test by creating an\nempty bare git repository outside of gitolite2, and I'm still getting\nthe same error, so I would exclude gitolite2 interference. In fact,\nyour testing approach gave me the idea to test by prepending env 1>&2\nto the receive pack and this way I noticed that the variable being set\nwas GIT_WORK_TREE. In fact, using --receive-pack 'export -n\nGIT_WORK_TREE ; git-receive-pack' fixes the problem for me.\n\nSo, I've at least been able to circumscribe the problem to: server git\n1.7.0.4, client git 2.5, GIT_WORK_TREE being sent over the ssh\nconnection. I'll see if I can find some time to do some proper\nbisecting next week.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"269423","messageId":"20150904142630.GA11830@sigill.intra.peff.net","threadId":"40271","inReplyTo":"CAOxFTcwYJqFHrBkie19Fm3AqeYmSLRNBpBtOAkiYmfO9UgN-0g@mail.gmail.com","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T14:26:31Z","receivedAt":"2015-09-04T14:26:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 04:02:09PM +0200, Giuseppe Bilotta wrote:\n\n> So, I've at least been able to circumscribe the problem to: server git\n> 1.7.0.4, client git 2.5, GIT_WORK_TREE being sent over the ssh\n> connection. I'll see if I can find some time to do some proper\n> bisecting next week.\n\nThat sounds like progress. I was hoping I could replicate it with that\ninformation, but I still can't:\n\n  LOCAL_GIT=git.v2.5.0\n  REMOTE_GIT=/home/peff/compile/git/bin-wrappers/git\n\n  $ $LOCAL_GIT version\n  git version 2.5.0\n\n  $ ssh git.peff.net $REMOTE_GIT version\n  git version 1.7.0.4\n\n  $ ssh git.peff.net \"rm -rf /tmp/foo.git; $REMOTE_GIT init --bare /tmp/foo.git\"\n  $ GIT_DIR=$PWD/.git GIT_WORK_TREE=$PWD $LOCAL_GIT push \\\n      --receive-pack=\"env | grep GIT >&2; $REMOTE_GIT receive-pack\" \\\n      git.peff.net:/tmp/foo.git HEAD:branch\n\n  GIT_DIR=/home/peff/tmp/.git\n  GIT_WORK_TREE=/home/peff/tmp\n  GIT_PREFIX=\n  Counting objects: 3, done.\n  Writing objects: 100% (3/3), 208 bytes | 0 bytes/s, done.\n  Total 3 (delta 0), reused 0 (delta 0)\n  To git.peff.net:/tmp/foo.git\n   * [new branch]      HEAD -> branch\n\nInterestingly, if I try pushing straight to master, the remote side\ncomplains about trying to push the checked-out branch, even though it's\na bare repo (presumably it's getting confused by the GIT_WORK_TREE\nsetting; so that's _a_ bug, but not the one you are seeing).\n\nSo I'm not sure what is different about my setup and yours.\n\n-Peff\n"},{"id":"269437","messageId":"xmqqpp1yf3qe.fsf@gitster.mtv.corp.google.com","threadId":"40271","inReplyTo":"20150904125448.GA25501@sigill.intra.peff.net","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-04T18:18:01Z","receivedAt":"2015-09-04T18:18:01Z","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> It shouldn't be necessary for $GIT_DIR, but it makes sense for other git\n> variables. E.g., with \"AcceptEnv GIT_*\", \"git -c\" config is propagated.\n> E.g.:\n> ...\n\nJust to make sure I got you correctly, you are saying that \"we\npropagate, but that is not correct. We should stop doing so\", right?\n"},{"id":"269443","messageId":"CAOxFTczPD00rw3nmaYjM=afXez_07p8kM28XY__opLKsVOaWxg@mail.gmail.com","threadId":"40271","inReplyTo":"xmqqpp1yf3qe.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2015-09-04T21:10:52Z","receivedAt":"2015-09-04T21:10:52Z","isPatch":false,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Fri, Sep 4, 2015 at 8:18 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Jeff King <peff@peff.net> writes:\n>\n>> It shouldn't be necessary for $GIT_DIR, but it makes sense for other git\n>> variables. E.g., with \"AcceptEnv GIT_*\", \"git -c\" config is propagated.\n>> E.g.:\n>> ...\n>\n> Just to make sure I got you correctly, you are saying that \"we\n> propagate, but that is not correct. We should stop doing so\", right?\n\nIndeed, we should not propagate them. For what we know so far, at\nleast GIT_DIR and GIT_WORK_TREE should be removed from the ssh\nenvironment, in case the remote server has AcceptEnv GIT_* and an\nolder git version.\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"269449","messageId":"20150904214454.GA18320@sigill.intra.peff.net","threadId":"40271","inReplyTo":"xmqqpp1yf3qe.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC] a tale of Git 2.5, ssh transport and GIT_* environment variables","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T21:44:54Z","receivedAt":"2015-09-04T21:44:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 11:18:01AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > It shouldn't be necessary for $GIT_DIR, but it makes sense for other git\n> > variables. E.g., with \"AcceptEnv GIT_*\", \"git -c\" config is propagated.\n> > E.g.:\n> > ...\n> \n> Just to make sure I got you correctly, you are saying that \"we\n> propagate, but that is not correct. We should stop doing so\", right?\n\nExactly. We do not propagate config over git:// or http:// (because we\ndo not share our environment). Nor do we do so over same-machine\nconnections (because we explicitly clean the environment). So ssh:// is\nthe odd duck.\n\n-Peff\n"},{"id":"269455","messageId":"20150904224008.GA11164@sigill.intra.peff.net","threadId":"40271","inReplyTo":"20150904214454.GA18320@sigill.intra.peff.net","subject":"[PATCH] git_connect: clear GIT_* environment for ssh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-04T22:40:08Z","receivedAt":"2015-09-04T22:40:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 05:44:54PM -0400, Jeff King wrote:\n\n> > Just to make sure I got you correctly, you are saying that \"we\n> > propagate, but that is not correct. We should stop doing so\", right?\n> \n> Exactly. We do not propagate config over git:// or http:// (because we\n> do not share our environment). Nor do we do so over same-machine\n> connections (because we explicitly clean the environment). So ssh:// is\n> the odd duck.\n\nSo here is the patch I would propose. I'm fairly certain it will solve\nGiuseppe's problem, though I think there is something else odd going on\nhere that I don't understand. I'm worried that we're papering over\nanother regression. Giuseppe, if you can still find time to do that\nbisect, it would be helpful.\n\n-- >8 --\nSubject: git_connect: clear GIT_* environment for ssh\n\nWhen we \"switch\" to another local repository to run the server\nside of a fetch or push, we must clear the variables in\nlocal_repo_env so that our local $GIT_DIR, etc, do not\npollute the upload-pack or receive-pack that is executing in\nthe \"remote\" repository.\n\nWe have never done so for ssh connections. For the most\npart, nobody has noticed because ssh will not pass unknown\nenvironment variables by default. However, it is not out of\nthe question for a user to configure ssh to pass along GIT_*\nvariables using SendEnv/AcceptEnv.\n\nWe can demonstrate the problem by using \"git -c\" on a local\ncommand and seeing its impact on a remote repository.  This\nconfig ends up in $GIT_CONFIG_PARAMETERS. In the local case,\nthe config has no impact, but in the ssh transport, it does\n(our test script has a fake ssh that passes through all\nenvironment variables; this isn't normal, but does simulate\none possible setup).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connect.c                     |  4 ++--\n t/t5507-remote-environment.sh | 34 ++++++++++++++++++++++++++++++++++\n 2 files changed, 36 insertions(+), 2 deletions(-)\n create mode 100755 t/t5507-remote-environment.sh\n\ndiff --git a/connect.c b/connect.c\nindex c0144d8..962f990 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -721,6 +721,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tstrbuf_addch(&cmd, ' ');\n \t\tsq_quote_buf(&cmd, path);\n \n+\t\t/* remove repo-local variables from the environment */\n+\t\tconn->env = local_repo_env;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n@@ -778,8 +780,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \t\t\targv_array_push(&conn->args, ssh_host);\n \t\t} else {\n-\t\t\t/* remove repo-local variables from the environment */\n-\t\t\tconn->env = local_repo_env;\n \t\t\tconn->use_shell = 1;\n \t\t}\n \t\targv_array_push(&conn->args, cmd.buf);\ndiff --git a/t/t5507-remote-environment.sh b/t/t5507-remote-environment.sh\nnew file mode 100755\nindex 0000000..e614929\n--- /dev/null\n+++ b/t/t5507-remote-environment.sh\n@@ -0,0 +1,34 @@\n+#!/bin/sh\n+\n+test_description='check environment showed to remote side of transports'\n+. ./test-lib.sh\n+\n+test_expect_success 'set up \"remote\" push situation' '\n+\ttest_commit one &&\n+\tgit config push.default current &&\n+\tgit init remote\n+'\n+\n+test_expect_success 'set up fake ssh' '\n+\tGIT_SSH_COMMAND=\"f() {\n+\t\tcd \\\"\\$TRASH_DIRECTORY\\\" &&\n+\t\teval \\\"\\$2\\\"\n+\t}; f\" &&\n+\texport GIT_SSH_COMMAND &&\n+\texport TRASH_DIRECTORY\n+'\n+\n+# due to receive.denyCurrentBranch=true\n+test_expect_success 'confirm default push fails' '\n+\ttest_must_fail git push remote\n+'\n+\n+test_expect_success 'config does not travel over same-machine push' '\n+\ttest_must_fail git -c receive.denyCurrentBranch=false push remote\n+'\n+\n+test_expect_success 'config does not travel over ssh push' '\n+\ttest_must_fail git -c receive.denyCurrentBranch=false push host:remote\n+'\n+\n+test_done\n-- \n2.5.1.812.ge796bff\n"},{"id":"269489","messageId":"CAOxFTcz9dZRQVnVnt+GtzAiu+GBi7CPE17d7rGo3H0v56MMAZQ@mail.gmail.com","threadId":"40271","inReplyTo":"20150904224008.GA11164@sigill.intra.peff.net","subject":"Re: [PATCH] git_connect: clear GIT_* environment for ssh","fromName":"Giuseppe Bilotta","fromEmail":"giuseppe.bilotta@gmail.com","sentAt":"2015-09-05T13:50:08Z","receivedAt":"2015-09-05T13:50:08Z","isPatch":true,"sender":{"key":"giuseppe.bilotta@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1464?v=4"},"body":"On Sat, Sep 5, 2015 at 12:40 AM, Jeff King <peff@peff.net> wrote:\n> So here is the patch I would propose. I'm fairly certain it will solve\n> Giuseppe's problem, though I think there is something else odd going on\n> here that I don't understand. I'm worried that we're papering over\n> another regression. Giuseppe, if you can still find time to do that\n> bisect, it would be helpful.\n\nSo, as it happens I was able to script the check in a\nnot-too-convoluted way using the same pair of machines where I came\nacross the problem,, and it turns out that the culprit is as obvious\nas one would expect:\n\nd95138e695d99d32dcad528a2a7974f434c51e79 is the first bad commit\ncommit d95138e695d99d32dcad528a2a7974f434c51e79\nAuthor: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nDate:   Fri Jun 26 17:37:35 2015 +0700\n\n    setup: set env $GIT_WORK_TREE when work tree is set, like $GIT_DIR\n\nwhich matches my previous findings in that the problem was\nGIT_WORK_TREE being leaked. Since setting GIT_WORK_TREE when setting\nGIT_DIR is correct, the solution is indeed to clear GIT_* environment\nvariables for ssh, as you propose. I tested your patch and indeed it\nfixes my problem.\n\nI still don't understand why you cannot replicate the issue on your\nside. One thing that is _extremely_ important in reproducing the\nproblem is that the leaked GIT_WORK_TREE point to a non-existent (or\notherwise inaccessible) directory in the server machine. For example,\non my first attempt to reproduce I was trying to use my clone of the\ngit repo, and it wouldn't work because I _did_ have a ~/src/git on\nboth machines, even though I was pushing to a\nremote.machine:/tmp/test.git\n\nWould it be worth looking at the issue server-side too, as this looks\nlike something that might have exploit potential? (At the very least,\nit could be used to test if/which directories the remote user has\naccess to.)\n\n> -- >8 --\n> Subject: git_connect: clear GIT_* environment for ssh\n>\n> When we \"switch\" to another local repository to run the server\n> side of a fetch or push, we must clear the variables in\n> local_repo_env so that our local $GIT_DIR, etc, do not\n> pollute the upload-pack or receive-pack that is executing in\n> the \"remote\" repository.\n\nI think we might want to mention GIT_WORK_TREE explicitly here, since\nthis seems to be the one causing issues.\n\n> We have never done so for ssh connections. For the most\n> part, nobody has noticed because ssh will not pass unknown\n> environment variables by default. However, it is not out of\n> the question for a user to configure ssh to pass along GIT_*\n> variables using SendEnv/AcceptEnv.\n>\n> We can demonstrate the problem by using \"git -c\" on a local\n> command and seeing its impact on a remote repository.  This\n> config ends up in $GIT_CONFIG_PARAMETERS. In the local case,\n> the config has no impact, but in the ssh transport, it does\n> (our test script has a fake ssh that passes through all\n> environment variables; this isn't normal, but does simulate\n> one possible setup).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nThe patch works for me, so aside from the suggested commit message\nchange, I'm all for it.\n\nReviewed-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>\n\n> ---\n>  connect.c                     |  4 ++--\n>  t/t5507-remote-environment.sh | 34 ++++++++++++++++++++++++++++++++++\n>  2 files changed, 36 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t5507-remote-environment.sh\n>\n> diff --git a/connect.c b/connect.c\n> index c0144d8..962f990 100644\n> --- a/connect.c\n> +++ b/connect.c\n> @@ -721,6 +721,8 @@ struct child_process *git_connect(int fd[2], const char *url,\n>                 strbuf_addch(&cmd, ' ');\n>                 sq_quote_buf(&cmd, path);\n>\n> +               /* remove repo-local variables from the environment */\n> +               conn->env = local_repo_env;\n\nA posteriori, this makes a lot of sense too: why single out a single\nprotocol in the environment cleanup?\n\n>                 conn->in = conn->out = -1;\n>                 if (protocol == PROTO_SSH) {\n>                         const char *ssh;\n> @@ -778,8 +780,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n>                         }\n>                         argv_array_push(&conn->args, ssh_host);\n>                 } else {\n> -                       /* remove repo-local variables from the environment */\n> -                       conn->env = local_repo_env;\n>                         conn->use_shell = 1;\n\nOf course we're now left with just conn->use_shell = 1 in the non-ssh\ncase. This could be moved in front of the if, and replaced with\nsomething like conn>use_shell = !(procol == PROTO_SSH), but maybe this\nwould kill legibility. It's just  that a single line i the else clause\nof a large if looks odd.\n\n\n-- \nGiuseppe \"Oblomov\" Bilotta\n"},{"id":"269602","messageId":"20150908083314.GB2991@sigill.intra.peff.net","threadId":"40271","inReplyTo":"CAOxFTcz9dZRQVnVnt+GtzAiu+GBi7CPE17d7rGo3H0v56MMAZQ@mail.gmail.com","subject":"[PATCH] git_connect: clarify conn->use_shell flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T08:33:14Z","receivedAt":"2015-09-08T08:33:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 05, 2015 at 03:50:08PM +0200, Giuseppe Bilotta wrote:\n\n> which matches my previous findings in that the problem was\n> GIT_WORK_TREE being leaked. Since setting GIT_WORK_TREE when setting\n> GIT_DIR is correct, the solution is indeed to clear GIT_* environment\n> variables for ssh, as you propose. I tested your patch and indeed it\n> fixes my problem.\n\nYeah, that makes sense.\n\n> I still don't understand why you cannot replicate the issue on your\n> side. One thing that is _extremely_ important in reproducing the\n> problem is that the leaked GIT_WORK_TREE point to a non-existent (or\n> otherwise inaccessible) directory in the server machine. For example,\n> on my first attempt to reproduce I was trying to use my clone of the\n> git repo, and it wouldn't work because I _did_ have a ~/src/git on\n> both machines, even though I was pushing to a\n> remote.machine:/tmp/test.git\n\nI checked that the path in question is not accessible in my test setup.\nI'm also confused why it doesn't replicate for me.\n\nI noticed that your original report shows receive-pack behaving well,\nbut the child unpack-objects complaining. That process should not care\nabout GIT_WORK_TREE either, though.\n\n> Would it be worth looking at the issue server-side too, as this looks\n> like something that might have exploit potential? (At the very least,\n> it could be used to test if/which directories the remote user has\n> access to.)\n\nI think if anything, the server side should be clearing GIT_DIR,\nGIT_WORK_TREE, etc, when it does enter_repo(). The point of that\nfunction is to enter a repository which is given externally (generally\non the command-line, though in the case of git-daemon, across the\nsocket). Clearing any existing local-repo environment variables seems\nlike it should be part of that.\n\nThat's as easy as:\n\ndiff --git a/path.c b/path.c\nindex 95acbaf..395a75d 100644\n--- a/path.c\n+++ b/path.c\n@@ -383,6 +383,10 @@ const char *enter_repo(const char *path, int strict)\n {\n \tstatic char used_path[PATH_MAX];\n \tstatic char validated_path[PATH_MAX];\n+\tconst char * const *env;\n+\n+\tfor (env = local_repo_env; *env; env++)\n+\t\tunsetenv(*env);\n \n \tif (!path)\n \t\treturn NULL;\n\nbut I suspect it would break some esoteric cases. E.g.:\n\n  git clone -u 'git -c some.option=true upload-pack' foo.git\n\ndoes what you'd expect now. With that patch, upload-pack would clear its\nenv before entering \"foo.git\", and would ignore it.\n\nIt would probably be OK to clean GIT_DIR (since we overwrite it in\nenter_repo anyway) and GIT_WORK_TREE (since upload-pack, etc, should not\ncare about it). But the others are much more confusing (should we clear\nGIT_ALTERNATE_OBJECT_DIRECTORIES? You would not want that to bleed over\nbetween repositories, but setting it manually seems like a legitimate,\nif uncommon, thing to do).\n\nSo I'm hesitant to do a patch like that because it seems like the wrong\nlayer. It's at the interface between the old and new repos that we need\nclean these up. And we are (with my other patch) doing that on the\nclient side. If the server side wants to enforce some protection, that's\na good idea, too. But it should be happening at the ssh layer; i.e.,\ndisabling \"AcceptEnv GIT_*\".\n\n> > -- >8 --\n> > Subject: git_connect: clear GIT_* environment for ssh\n> >\n> > When we \"switch\" to another local repository to run the server\n> > side of a fetch or push, we must clear the variables in\n> > local_repo_env so that our local $GIT_DIR, etc, do not\n> > pollute the upload-pack or receive-pack that is executing in\n> > the \"remote\" repository.\n> \n> I think we might want to mention GIT_WORK_TREE explicitly here, since\n> this seems to be the one causing issues.\n\nHeh. I avoided doing so because I could not convince it to cause an\nissue (and even with that aside, the config thing is wrong by itself,\nand I _could_ test that).\n\n> The patch works for me, so aside from the suggested commit message\n> change, I'm all for it.\n\nThanks for testing/reviewing.\n\n> > @@ -778,8 +780,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n> >                         }\n> >                         argv_array_push(&conn->args, ssh_host);\n> >                 } else {\n> > -                       /* remove repo-local variables from the environment */\n> > -                       conn->env = local_repo_env;\n> >                         conn->use_shell = 1;\n> \n> Of course we're now left with just conn->use_shell = 1 in the non-ssh\n> case. This could be moved in front of the if, and replaced with\n> something like conn>use_shell = !(procol == PROTO_SSH), but maybe this\n> would kill legibility. It's just  that a single line i the else clause\n> of a large if looks odd.\n\nYeah, I noticed that, too, but I wanted to keep the diff minimal. Maybe\nit is worth doing this cleanup on top.\n\n-- >8 --\nSubject: [PATCH] git_connect: clarify conn->use_shell flag\n\nWhen executing user-specified programs, we generally always\nwant to use a shell, for flexibility and consistency. One\nbig exception is executing $GIT_SSH, which for historical\nreasons must not use a shell.\n\nOnce upon a time the logic in git_connect looked like:\n\n  if (protocol == PROTO_SSH) {\n\t  ... setup ssh ...\n  } else {\n\t  ... setup local connection ...\n\t  conn->use_shell = 1;\n  }\n\nBut over time the PROTO_SSH block has grown, and the \"local\"\nblock has shrunk so that it contains only conn->use_shell;\nit's easy to miss at the end of the large block.  Moreover,\nPROTO_SSH now also sometimes sets use_shell, when the new\nGIT_SSH_COMMAND is used.\n\nTo make the flow easier to follow, let's just set\nconn->use_shell when we're setting up the \"conn\" struct, and\nunset it (with a comment) in the historical GIT_SSH case.\n\nNote that for clarity we leave \"use_shell\" on in the case\nthat we fall back to our default \"ssh\" This looks like a\nbehavior change, but in practice run-command.c optimizes\nshell invocations without metacharacters into a straight\nexecve anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connect.c | 17 +++++++++++------\n 1 file changed, 11 insertions(+), 6 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 962f990..6f3f85e 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -723,6 +723,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t/* remove repo-local variables from the environment */\n \t\tconn->env = local_repo_env;\n+\t\tconn->use_shell = 1;\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n@@ -748,15 +749,21 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\tif (ssh) {\n-\t\t\t\tconn->use_shell = 1;\n+\t\t\tif (ssh)\n \t\t\t\tputty = 0;\n-\t\t\t} else {\n+\t\t\telse {\n \t\t\t\tconst char *base;\n \t\t\t\tchar *ssh_dup;\n \n+\t\t\t\t/*\n+\t\t\t\t * GIT_SSH is the no-shell version of\n+\t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n+\t\t\t\t * historical compatibility).\n+\t\t\t\t */\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\tif (!ssh)\n+\t\t\t\tif (ssh)\n+\t\t\t\t\tconn->use_shell = 0;\n+\t\t\t\telse\n \t\t\t\t\tssh = \"ssh\";\n \n \t\t\t\tssh_dup = xstrdup(ssh);\n@@ -779,8 +786,6 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\targv_array_push(&conn->args, port);\n \t\t\t}\n \t\t\targv_array_push(&conn->args, ssh_host);\n-\t\t} else {\n-\t\t\tconn->use_shell = 1;\n \t\t}\n \t\targv_array_push(&conn->args, cmd.buf);\n \n-- \n2.6.0.rc0.358.gaf70435\n"},{"id":"269610","messageId":"xmqqwpw096dq.fsf@gitster.mtv.corp.google.com","threadId":"40271","inReplyTo":"20150908083314.GB2991@sigill.intra.peff.net","subject":"Re: [PATCH] git_connect: clarify conn->use_shell flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-08T17:18:41Z","receivedAt":"2015-09-08T17:18:41Z","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> Subject: [PATCH] git_connect: clarify conn->use_shell flag\n>\n> When executing user-specified programs, we generally always\n> want to use a shell, for flexibility and consistency. One\n> big exception is executing $GIT_SSH, which for historical\n> reasons must not use a shell.\n>\n> Once upon a time the logic in git_connect looked like:\n>\n>   if (protocol == PROTO_SSH) {\n> \t  ... setup ssh ...\n>   } else {\n> \t  ... setup local connection ...\n> \t  conn->use_shell = 1;\n>   }\n>\n> But over time the PROTO_SSH block has grown, and the \"local\"\n> block has shrunk so that it contains only conn->use_shell;\n> it's easy to miss at the end of the large block.  Moreover,\n> PROTO_SSH now also sometimes sets use_shell, when the new\n> GIT_SSH_COMMAND is used.\n>\n> To make the flow easier to follow, let's just set\n> conn->use_shell when we're setting up the \"conn\" struct, and\n> unset it (with a comment) in the historical GIT_SSH case.\n\nMakes perfect sense.  I think another thing that falls into the same\nclass wrt readability is 'putty'; if the code does putty=0 at the\nbeginning of \"various flavours of SSH\", and sets it only when it\nchecks with \"various flavours of plink\" when GIT_SSH_COMMAND is not\nset, the logic would be even clearer, I suspect.\n\n> Note that for clarity we leave \"use_shell\" on in the case\n> that we fall back to our default \"ssh\" This looks like a\n> behavior change, but in practice run-command.c optimizes\n> shell invocations without metacharacters into a straight\n> execve anyway.\n\nHmm, interesting.  I am not sure if that has to be the way, though.\nWouldn't the resulting code become simpler if you do not do that?\n\nThat is, is is what I have in mind on top of your patch.  Did I\nbreak something?\n\n connect.c | 17 ++++++++---------\n 1 file changed, 8 insertions(+), 9 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 6f3f85e..acd39d7 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -727,7 +727,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\tconn->in = conn->out = -1;\n \t\tif (protocol == PROTO_SSH) {\n \t\t\tconst char *ssh;\n-\t\t\tint putty, tortoiseplink = 0;\n+\t\t\tint putty = 0, tortoiseplink = 0;\n \t\t\tchar *ssh_host = hostandport;\n \t\t\tconst char *port = NULL;\n \t\t\tget_host_and_port(&ssh_host, &port);\n@@ -749,9 +749,7 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t}\n \n \t\t\tssh = getenv(\"GIT_SSH_COMMAND\");\n-\t\t\tif (ssh)\n-\t\t\t\tputty = 0;\n-\t\t\telse {\n+\t\t\tif (!ssh) {\n \t\t\t\tconst char *base;\n \t\t\t\tchar *ssh_dup;\n \n@@ -760,10 +758,10 @@ struct child_process *git_connect(int fd[2], const char *url,\n \t\t\t\t * GIT_SSH_COMMAND (and must remain so for\n \t\t\t\t * historical compatibility).\n \t\t\t\t */\n+\t\t\t\tconn->use_shell = 0;\n+\n \t\t\t\tssh = getenv(\"GIT_SSH\");\n-\t\t\t\tif (ssh)\n-\t\t\t\t\tconn->use_shell = 0;\n-\t\t\t\telse\n+\t\t\t\tif (!ssh)\n \t\t\t\t\tssh = \"ssh\";\n \n \t\t\t\tssh_dup = xstrdup(ssh);\n@@ -771,8 +769,9 @@ struct child_process *git_connect(int fd[2], const char *url,\n \n \t\t\t\ttortoiseplink = !strcasecmp(base, \"tortoiseplink\") ||\n \t\t\t\t\t!strcasecmp(base, \"tortoiseplink.exe\");\n-\t\t\t\tputty = !strcasecmp(base, \"plink\") ||\n-\t\t\t\t\t!strcasecmp(base, \"plink.exe\") || tortoiseplink;\n+\t\t\t\tputty = tortoiseplink ||\n+\t\t\t\t\t!strcasecmp(base, \"plink\") ||\n+\t\t\t\t\t!strcasecmp(base, \"plink.exe\");\n \n \t\t\t\tfree(ssh_dup);\n \t\t\t}\n"},{"id":"269628","messageId":"20150908214019.GA24159@sigill.intra.peff.net","threadId":"40271","inReplyTo":"xmqqwpw096dq.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git_connect: clarify conn->use_shell flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T21:40:19Z","receivedAt":"2015-09-08T21:40:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 08, 2015 at 10:18:41AM -0700, Junio C Hamano wrote:\n\n> > To make the flow easier to follow, let's just set\n> > conn->use_shell when we're setting up the \"conn\" struct, and\n> > unset it (with a comment) in the historical GIT_SSH case.\n> \n> Makes perfect sense.  I think another thing that falls into the same\n> class wrt readability is 'putty'; if the code does putty=0 at the\n> beginning of \"various flavours of SSH\", and sets it only when it\n> checks with \"various flavours of plink\" when GIT_SSH_COMMAND is not\n> set, the logic would be even clearer, I suspect.\n\nYeah, I think so.\n\n> > Note that for clarity we leave \"use_shell\" on in the case\n> > that we fall back to our default \"ssh\" This looks like a\n> > behavior change, but in practice run-command.c optimizes\n> > shell invocations without metacharacters into a straight\n> > execve anyway.\n> \n> Hmm, interesting.  I am not sure if that has to be the way, though.\n> Wouldn't the resulting code become simpler if you do not do that?\n\nHeh, I originally wrote it that way, and waffled between sending one or\nthe other.\n\n> That is, is is what I have in mind on top of your patch.  Did I\n> break something?\n> \n>  connect.c | 17 ++++++++---------\n>  1 file changed, 8 insertions(+), 9 deletions(-)\n\nI think both changes are correct, and the result looks nice to read.\nFeel free to squash them in as you apply.\n\n-Peff\n"}]}