{"thread":{"id":"61682","subject":"[PATCH 0/1] support GIT_IGNORE_INSECURE_OWNER environment variable","startedAt":"2024-06-26T12:34:05Z","lastAt":"2024-07-02T00:29:01Z","messageCount":20,"participants":["Florian Schmaus","Phillip Wood","rsbecker@nexbridge.com","Junio C Hamano","phillip.wood123@gmail.com","Jeff King","Johannes Schindelin","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"497684","messageId":"20240626123358.420292-1-flo@geekplace.eu","threadId":"61682","inReplyTo":null,"subject":"[PATCH 0/1] support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Florian Schmaus","fromEmail":"flo@geekplace.eu","sentAt":"2024-06-26T12:33:57Z","receivedAt":"2024-06-26T12:34:05Z","isPatch":true,"sender":{"key":"flo@geekplace.eu","avatar":"https://gravatar.com/avatar/707ae0cbee836b5612304f2135a9c3e65198d9a521d3ad8977708f0ed7251118?d=mp&s=160"},"body":"I could imagine that the safe-directory check was deliberately not\nmade controllable via an environment variable. However, git versions\nwith the safe-directory check gained adoption and we can now see that\nthere is some fallout caused by the check.\n\nA prominent example is that git-daemon cannot export repos when\nrunning under nobody (see https://bugs.gentoo.org/932091). Since the\n'nobody' user typically has no home directory, the suggested fix\n\n git config --global --add safe.directory xxx.git\n\ndoes not work. Likewise, adding it to /etc/gitconfig is also not\nideal, as it applies to every user.\n\nIn Gentoo, the safe-directory check can be completely disabled via a\nUSE-flag (i.e., Gentoo's mechanism for compile-time package\ncustomization). However, I recently suggested to Gentoo's git-package\nmaintainers the following patch, introducing the\nGIT_IGNORE_INSECURE_OWNER environment variable, as an alternative.\n\nBeing able to disable the safe directory check via an environment\nvariable allows for more flexibility, solves the issue described\nabove, and does *not* statically and globally disable the\nsafe-directory check. This was received with a positive response and\nthe patch will likely be applied by Gentoo.\n\nBut downstream patchery is always the second-best option. Therefore,\nI hereby propose the patch to upstream.\n\nFlorian Schmaus (1):\n setup: support GIT_IGNORE_INSECURE_OWNER environment variable\n\n setup.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\n-- \n2.44.2\n\n"},{"id":"497685","messageId":"20240626123358.420292-2-flo@geekplace.eu","threadId":"61682","inReplyTo":"20240626123358.420292-1-flo@geekplace.eu","subject":"[PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Florian Schmaus","fromEmail":"flo@geekplace.eu","sentAt":"2024-06-26T12:33:58Z","receivedAt":"2024-06-26T12:34:05Z","isPatch":true,"sender":{"key":"flo@geekplace.eu","avatar":"https://gravatar.com/avatar/707ae0cbee836b5612304f2135a9c3e65198d9a521d3ad8977708f0ed7251118?d=mp&s=160"},"body":"Sometimes more flexibility to disable/ignore the ownership check, besides\nthe safe.directory configuration option, is required.\n\nFor example, git-daemon running as nobody user, which typically has no\nhome directory. Therefore, we can not add the path to a user-global\nconfiguration and adding the path to the system-wide configuration could\nhave negative security implications.\n\nTherefore, make the check configurable via an environment variable.\n\nIf the environment variable GIT_IGNORE_INSECURE_OWNER is set to true,\nthen ignore potentially insecure ownership of git-related path\ncomponents.\n\nSigned-off-by: Florian Schmaus \n---\n setup.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/setup.c b/setup.c\nindex 3afa6fb09b28..da3f504fb536 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -1278,6 +1278,14 @@ static int ensure_valid_ownership(const char *gitfile,\n \t */\n \tgit_protected_config(safe_directory_cb, &data);\n \n+\tif (data.is_safe)\n+\t\treturn data.is_safe;\n+\n+\tif (git_env_bool(\"GIT_IGNORE_INSECURE_OWNER\", 0)) {\n+\t\twarning(\"ignoring dubious ownership in repository at '%s' (GIT_IGNORE_INSECURE_OWNER set)\", data.path);\n+\t\treturn 1;\n+\t}\n+\n \treturn data.is_safe;\n }\n \n-- \n2.44.2\n\n"},{"id":"497693","messageId":"9e5b0cc6-e28c-4c51-ab48-663c61c00ee3@gmail.com","threadId":"61682","inReplyTo":"20240626123358.420292-2-flo@geekplace.eu","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-26T13:11:32Z","receivedAt":"2024-06-26T13:11:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Florian\n\nOn 26/06/2024 13:33, Florian Schmaus wrote:\n> Sometimes more flexibility to disable/ignore the ownership check, besides\n> the safe.directory configuration option, is required.\n> \n> For example, git-daemon running as nobody user, which typically has no\n> home directory. Therefore, we can not add the path to a user-global\n> configuration and adding the path to the system-wide configuration could\n> have negative security implications.\n> \n> Therefore, make the check configurable via an environment variable.\n\nAn alternative would be to allow safe.directory to be specified on the \ncommand line with \"git -c safe.directory='*' daemon ...\" rather than \nadding a dedicated environment variable. Or you could set $HOME to a \nsuitable directory when running \"git daemon\" and put the user-global \nconfig file there. That directory and config file only need to be \nreadable by the user that \"git daemon\" is running under it can be owned \nby root or whoever else you want.\n\nBest Wishes\n\nPhillip\n\n\n> If the environment variable GIT_IGNORE_INSECURE_OWNER is set to true,\n> then ignore potentially insecure ownership of git-related path\n> components.\n> \n> Signed-off-by: Florian Schmaus \n> ---\n> setup.c | 8 ++++++++\n> 1 file changed, 8 insertions(+)\n> \n> diff --git a/setup.c b/setup.c\n> index 3afa6fb09b28..da3f504fb536 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -1278,6 +1278,14 @@ static int ensure_valid_ownership(const char *gitfile,\n> \t */\n> \tgit_protected_config(safe_directory_cb, &data);\n> \n> +\tif (data.is_safe)\n> +\t\treturn data.is_safe;\n> +\n> +\tif (git_env_bool(\"GIT_IGNORE_INSECURE_OWNER\", 0)) {\n> +\t\twarning(\"ignoring dubious ownership in repository at '%s' (GIT_IGNORE_INSECURE_OWNER set)\", data.path);\n> +\t\treturn 1;\n> +\t}\n> +\n> \treturn data.is_safe;\n> }\n> \n\n"},{"id":"497704","messageId":"046501dac7dc$31d92d60$958b8820$@nexbridge.com","threadId":"61682","inReplyTo":"9e5b0cc6-e28c-4c51-ab48-663c61c00ee3@gmail.com","subject":"RE: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"","fromEmail":"rsbecker@nexbridge.com","sentAt":"2024-06-26T15:19:04Z","receivedAt":"2024-06-26T15:19:26Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On Wednesday, June 26, 2024 9:12 AM, Phillip Wood wrote:\n>On 26/06/2024 13:33, Florian Schmaus wrote:\n>> Sometimes more flexibility to disable/ignore the ownership check,\n>> besides the safe.directory configuration option, is required.\n>>\n>> For example, git-daemon running as nobody user, which typically has no\n>> home directory. Therefore, we can not add the path to a user-global\n>> configuration and adding the path to the system-wide configuration\n>> could have negative security implications.\n>>\n>> Therefore, make the check configurable via an environment variable.\n>\n>An alternative would be to allow safe.directory to be specified on the command line\n>with \"git -c safe.directory='*' daemon ...\" rather than adding a dedicated\n>environment variable. Or you could set $HOME to a suitable directory when\n>running \"git daemon\" and put the user-global config file there. That directory and\n>config file only need to be readable by the user that \"git daemon\" is running under it\n>can be owned by root or whoever else you want.\n\nI agree with this alternative. Our CI build environment already has so many environment variables (not just from git but all dependencies and the CI environment itself) that we are pushing the 56Kb platform limit (not kidding). Reducing dependence on environment variables is, in my opinion, a good and necessary thing.\n\n>> If the environment variable GIT_IGNORE_INSECURE_OWNER is set to true,\n>> then ignore potentially insecure ownership of git-related path\n>> components.\n>>\n>> Signed-off-by: Florian Schmaus \n>> ---\n>> setup.c | 8 ++++++++\n>> 1 file changed, 8 insertions(+)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index 3afa6fb09b28..da3f504fb536 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -1278,6 +1278,14 @@ static int ensure_valid_ownership(const char *gitfile,\n>> \t */\n>> \tgit_protected_config(safe_directory_cb, &data);\n>>\n>> +\tif (data.is_safe)\n>> +\t\treturn data.is_safe;\n>> +\n>> +\tif (git_env_bool(\"GIT_IGNORE_INSECURE_OWNER\", 0)) {\n>> +\t\twarning(\"ignoring dubious ownership in repository at '%s'\n>(GIT_IGNORE_INSECURE_OWNER set)\", data.path);\n>> +\t\treturn 1;\n>> +\t}\n>> +\n>> \treturn data.is_safe;\n>> }\n>>\n\nSincerely\n--Randall\n\n"},{"id":"497706","messageId":"72e42e9f-5b85-4863-8506-c99d658d7596@gmail.com","threadId":"61682","inReplyTo":"9e5b0cc6-e28c-4c51-ab48-663c61c00ee3@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-26T15:26:15Z","receivedAt":"2024-06-26T15:26:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2024 14:11, Phillip Wood wrote:\n> Hi Florian\n> \n> On 26/06/2024 13:33, Florian Schmaus wrote:\n>> Sometimes more flexibility to disable/ignore the ownership check, besides\n>> the safe.directory configuration option, is required.\n>>\n>> For example, git-daemon running as nobody user, which typically has no\n>> home directory. Therefore, we can not add the path to a user-global\n>> configuration and adding the path to the system-wide configuration could\n>> have negative security implications.\n>>\n>> Therefore, make the check configurable via an environment variable.\n> \n> An alternative would be to allow safe.directory to be specified on the \n> command line with \"git -c safe.directory='*' daemon ...\" rather than \n> adding a dedicated environment variable.\n\nTo expand an this a little - a couple of times I've wanted to checkout a \nbare repository that is owned by a different user. It is a pain to have \nto add a new config setting just for a one-off checkout. Being able to \nadjust the config on the command line would be very useful in that case.\n\n> Or you could set $HOME to a \n> suitable directory when running \"git daemon\" and put the user-global \n> config file there. That directory and config file only need to be \n> readable by the user that \"git daemon\" is running under it can be owned \n> by root or whoever else you want.\n\nThe advantage of this approach is that there are no changes needed to \ngit, instead of setting GIT_IGNORE_INSECURE_OWNER one sets HOME to point \nto a suitable config file. I found this useful when I was debugging the \nissues with git-daemon earlier[1]\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/834862fd-b579-438a-b9b3-5246bf27ce8a@gmail.com\n\n\n> Best Wishes\n> \n> Phillip\n> \n> \n>> If the environment variable GIT_IGNORE_INSECURE_OWNER is set to true,\n>> then ignore potentially insecure ownership of git-related path\n>> components.\n>>\n>> Signed-off-by: Florian Schmaus \n>> ---\n>>   setup.c | 8 ++++++++\n>>   1 file changed, 8 insertions(+)\n>>\n>> diff --git a/setup.c b/setup.c\n>> index 3afa6fb09b28..da3f504fb536 100644\n>> --- a/setup.c\n>> +++ b/setup.c\n>> @@ -1278,6 +1278,14 @@ static int ensure_valid_ownership(const char \n>> *gitfile,\n>>        */\n>>       git_protected_config(safe_directory_cb, &data);\n>> +    if (data.is_safe)\n>> +        return data.is_safe;\n>> +\n>> +    if (git_env_bool(\"GIT_IGNORE_INSECURE_OWNER\", 0)) {\n>> +        warning(\"ignoring dubious ownership in repository at '%s' \n>> (GIT_IGNORE_INSECURE_OWNER set)\", data.path);\n>> +        return 1;\n>> +    }\n>> +\n>>       return data.is_safe;\n>>   }\n> \n> \n"},{"id":"497714","messageId":"xmqqa5j71snb.fsf@gitster.g","threadId":"61682","inReplyTo":"72e42e9f-5b85-4863-8506-c99d658d7596@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-26T18:11:04Z","receivedAt":"2024-06-26T18:11:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> To expand an this a little - a couple of times I've wanted to checkout\n> a bare repository that is owned by a different user. It is a pain to\n> have to add a new config setting just for a one-off checkout. Being\n> able to adjust the config on the command line would be very useful in\n> that case.\n\nTrue. As long as it is deemed safe to honor the one-off \"git -c\nsafe.directory=...\" from the command line, for the purpose of this\n\"I who am running this 'git' process hereby declare that I trust\nthis and that repository\", I think it would be the best solution\nfor the \"git daemon\" use case.\n\nAnd it is much better than adding a one-off environment variable.\nAfter all, if your \"git daemon\" user does not have a $HOME set in\nits /etc/passwd entry, you cannot set such an environment variable\nin $HOME/.profile so somewhere in your \"git daemon\" invocation would\nhave to be tweaked to have code snippet that sets and exports it\n*anyway*. You can tweak the \"git\" invocation to add the command\nline tweak \"-c safe.directory=...\" at the place you would have set\nand exported the variable, and using the well understood \"git -c\nvar=val\" mechanism would be more appropriate.\n\n>> Or you could set $HOME to a suitable directory when running \"git\n> ...\n> The advantage of this approach is that there are no changes needed to\n> git, instead of setting GIT_IGNORE_INSECURE_OWNER one sets HOME to\n> point to a suitable config file. I found this useful when I was\n> debugging the issues with git-daemon earlier[1]\n\nYup, that sounds like a workable approach, if \"git -c var=val\"\napproach turns out to be inappropriate for security purposes\nfor whatever reason.\n\nThanks.\n"},{"id":"497719","messageId":"6094695a-6b23-412d-9b97-15a7aec6eaf8@gmail.com","threadId":"61682","inReplyTo":"046501dac7dc$31d92d60$958b8820$@nexbridge.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-26T18:38:43Z","receivedAt":"2024-06-26T18:38:46Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2024 16:19, rsbecker@nexbridge.com wrote:\n> On Wednesday, June 26, 2024 9:12 AM, Phillip Wood wrote:\n>> On 26/06/2024 13:33, Florian Schmaus wrote:\n>> An alternative would be to allow safe.directory to be specified on the command line\n>> with \"git -c safe.directory='*' daemon ...\" rather than adding a dedicated\n>> environment variable. Or you could set $HOME to a suitable directory when\n>> running \"git daemon\" and put the user-global config file there. That directory and\n>> config file only need to be readable by the user that \"git daemon\" is running under it\n>> can be owned by root or whoever else you want.\n> \n> I agree with this alternative. Our CI build environment already has so many environment variables (not just from git but all dependencies and the CI environment itself) that we are pushing the 56Kb platform limit (not kidding). Reducing dependence on environment variables is, in my opinion, a good and necessary thing.\n\n\"git -c\" still ends up setting an environment variable internally to \nensure that the config setting is passed down to any child processes. \nThe advantage is that we're re-using an existing mechanism rather than \nintroducing a new environment variable.\n\nBest Wishes\n\nPhillip\n\n>>> If the environment variable GIT_IGNORE_INSECURE_OWNER is set to true,\n>>> then ignore potentially insecure ownership of git-related path\n>>> components.\n>>>\n>>> Signed-off-by: Florian Schmaus \n>>> ---\n>>> setup.c | 8 ++++++++\n>>> 1 file changed, 8 insertions(+)\n>>>\n>>> diff --git a/setup.c b/setup.c\n>>> index 3afa6fb09b28..da3f504fb536 100644\n>>> --- a/setup.c\n>>> +++ b/setup.c\n>>> @@ -1278,6 +1278,14 @@ static int ensure_valid_ownership(const char *gitfile,\n>>> \t */\n>>> \tgit_protected_config(safe_directory_cb, &data);\n>>>\n>>> +\tif (data.is_safe)\n>>> +\t\treturn data.is_safe;\n>>> +\n>>> +\tif (git_env_bool(\"GIT_IGNORE_INSECURE_OWNER\", 0)) {\n>>> +\t\twarning(\"ignoring dubious ownership in repository at '%s'\n>> (GIT_IGNORE_INSECURE_OWNER set)\", data.path);\n>>> +\t\treturn 1;\n>>> +\t}\n>>> +\n>>> \treturn data.is_safe;\n>>> }\n>>>\n> \n> Sincerely\n> --Randall\n> \n"},{"id":"497723","messageId":"a87a7131-3207-42f2-93c0-e6e9e02a2062@geekplace.eu","threadId":"61682","inReplyTo":"xmqqa5j71snb.fsf@gitster.g","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Florian Schmaus","fromEmail":"flo@geekplace.eu","sentAt":"2024-06-26T19:06:10Z","receivedAt":"2024-06-26T19:06:14Z","isPatch":true,"sender":{"key":"flo@geekplace.eu","avatar":"https://gravatar.com/avatar/707ae0cbee836b5612304f2135a9c3e65198d9a521d3ad8977708f0ed7251118?d=mp&s=160"},"body":"Thanks for all your replies. Much appreciated.\n\nOn 26/06/2024 20.11, Junio C Hamano wrote:\n> Phillip Wood writes:\n>> To expand an this a little - a couple of times I've wanted to checkout\n>> a bare repository that is owned by a different user. It is a pain to\n>> have to add a new config setting just for a one-off checkout. Being\n>> able to adjust the config on the command line would be very useful in\n>> that case.\n> \n> True. As long as it is deemed safe to honor the one-off \"git -c\n> safe.directory=...\" from the command line, for the purpose of this\n> \"I who am running this 'git' process hereby declare that I trust\n> this and that repository\", I think it would be the best solution\n> for the \"git daemon\" use case.\n\nHow does one pass \"-c safe.directory=…\" to git-http-backend?\n\nI currently have an Apache config snippet like\n\nSetEnv GIT_PROJECT_ROOT /var/www/example.org/htdocs/git\nSetEnv GIT_HTTP_EXPORT_ALL\nScriptAlias /git/ /usr/libexec/git-core/git-http-backend/\n\n\n Require all granted\n AcceptPathInfo On\n\n\nto serve git repositories.\n\nGranted, the apache user has a home directory, so I am probably able to \nset save.directory via ~/.gitconfig.\n\nHowever, the point here is that git is often invoked indirectly, with no \ncontrol over the command line arguments that are passed to it. On the \nother hand, one has usually control over the environment variables.\n\nI agree that \"-c safe.directory=…\" is preferable to \nGIT_IGNORE_INSECURE_OWNER. However, sometimes using \"-c \nsafe.directory=…\" is cumbersome and maybe even impossible.\n\nOne alternative to GIT_IGNORE_INSECURE_OWNER would be a generic \nGIT_EXTRA_ARGS environment variable. So one could set\n\nGIT_EXTRA_ARGS=\"-c safe.directory=…\"\n\nNot saying that I like the idea, just pointing out this option.\n\n- Florian\n"},{"id":"497727","messageId":"20240626203705.GB441931@coredump.intra.peff.net","threadId":"61682","inReplyTo":"a87a7131-3207-42f2-93c0-e6e9e02a2062@geekplace.eu","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-06-26T20:37:05Z","receivedAt":"2024-06-26T20:37:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 26, 2024 at 09:06:10PM +0200, Florian Schmaus wrote:\n\n> > True. As long as it is deemed safe to honor the one-off \"git -c\n> > safe.directory=...\" from the command line, for the purpose of this\n> > \"I who am running this 'git' process hereby declare that I trust\n> > this and that repository\", I think it would be the best solution\n> > for the \"git daemon\" use case.\n> \n> How does one pass \"-c safe.directory=…\" to git-http-backend?\n> \n> I currently have an Apache config snippet like\n> \n> SetEnv GIT_PROJECT_ROOT /var/www/example.org/htdocs/git\n> SetEnv GIT_HTTP_EXPORT_ALL\n> ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/\n> \n> \n> Require all granted\n> AcceptPathInfo On\n> \n> \n> to serve git repositories.\n> \n> Granted, the apache user has a home directory, so I am probably able to set\n> save.directory via ~/.gitconfig.\n> \n> However, the point here is that git is often invoked indirectly, with no\n> control over the command line arguments that are passed to it. On the other\n> hand, one has usually control over the environment variables.\n> \n> I agree that \"-c safe.directory=…\" is preferable to\n> GIT_IGNORE_INSECURE_OWNER. However, sometimes using \"-c safe.directory=…\" is\n> cumbersome and maybe even impossible.\n> \n> One alternative to GIT_IGNORE_INSECURE_OWNER would be a generic\n> GIT_EXTRA_ARGS environment variable. So one could set\n> \n> GIT_EXTRA_ARGS=\"-c safe.directory=…\"\n> \n> Not saying that I like the idea, just pointing out this option.\n\nYou can do:\n\n GIT_CONFIG_COUNT=1\n GIT_CONFIG_KEY_0=safe.directory\n GIT_CONFIG_VALUE_0=\"*\"\n\nIt is a bit verbose, but it's a documented interface in git-config(1).\n\nUnder the hood \"git -c\" uses a different, older mechanism, but we've not\ndocumented it nor promised that it will remain stable.\n\n-Peff\n"},{"id":"497743","messageId":"5742e728-a012-4960-a32d-bf3b65c3a2e3@gmail.com","threadId":"61682","inReplyTo":"xmqqa5j71snb.fsf@gitster.g","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-27T09:50:36Z","receivedAt":"2024-06-27T09:50:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 26/06/2024 19:11, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> To expand an this a little - a couple of times I've wanted to checkout\n>> a bare repository that is owned by a different user. It is a pain to\n>> have to add a new config setting just for a one-off checkout. Being\n>> able to adjust the config on the command line would be very useful in\n>> that case.\n> \n> True. As long as it is deemed safe to honor the one-off \"git -c\n> safe.directory=...\" from the command line, for the purpose of this\n> \"I who am running this 'git' process hereby declare that I trust\n> this and that repository\", I think it would be the best solution\n> for the \"git daemon\" use case.\n\nThis actually works already, the behavior was changed in 6061601d9f \n(safe.directory: use git_protected_config(), 2022-07-14). The reason I \nthought it didn't work was that I remember it failing on Debian bullseye \na few months ago but that used an older version of git. There is some \nmore rationale for the change in 779ea9303a7 (Documentation: define \nprotected configuration, 2022-07-14)\n\nBest Wishes\n\nPhillip\n\n> And it is much better than adding a one-off environment variable.\n> After all, if your \"git daemon\" user does not have a $HOME set in\n> its /etc/passwd entry, you cannot set such an environment variable\n> in $HOME/.profile so somewhere in your \"git daemon\" invocation would\n> have to be tweaked to have code snippet that sets and exports it\n> *anyway*. You can tweak the \"git\" invocation to add the command\n> line tweak \"-c safe.directory=...\" at the place you would have set\n> and exported the variable, and using the well understood \"git -c\n> var=val\" mechanism would be more appropriate.\n> \n>>> Or you could set $HOME to a suitable directory when running \"git\n>> ...\n>> The advantage of this approach is that there are no changes needed to\n>> git, instead of setting GIT_IGNORE_INSECURE_OWNER one sets HOME to\n>> point to a suitable config file. I found this useful when I was\n>> debugging the issues with git-daemon earlier[1]\n> \n> Yup, that sounds like a workable approach, if \"git -c var=val\"\n> approach turns out to be inappropriate for security purposes\n> for whatever reason.\n> \n> Thanks.\n"},{"id":"497758","messageId":"xmqqpls2v1zx.fsf@gitster.g","threadId":"61682","inReplyTo":"5742e728-a012-4960-a32d-bf3b65c3a2e3@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-27T15:28:34Z","receivedAt":"2024-06-27T15:28:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 26/06/2024 19:11, Junio C Hamano wrote:\n>> Phillip Wood writes:\n>> \n>>> To expand an this a little - a couple of times I've wanted to checkout\n>>> a bare repository that is owned by a different user. It is a pain to\n>>> have to add a new config setting just for a one-off checkout. Being\n>>> able to adjust the config on the command line would be very useful in\n>>> that case.\n>> True. As long as it is deemed safe to honor the one-off \"git -c\n>> safe.directory=...\" from the command line, for the purpose of this\n>> \"I who am running this 'git' process hereby declare that I trust\n>> this and that repository\", I think it would be the best solution\n>> for the \"git daemon\" use case.\n>\n> This actually works already, the behavior was changed in 6061601d9f\n> (safe.directory: use git_protected_config(), 2022-07-14). The reason I\n> thought it didn't work was that I remember it failing on Debian\n> bullseye a few months ago but that used an older version of git. There\n> is some more rationale for the change in 779ea9303a7 (Documentation:\n> define protected configuration, 2022-07-14)\n\nThanks.\n\nSo, does this more or less conclude the episode about how best to\ndeal with the 2.45.1 regression that Florian's patch in this thread\nstarted? It seems that we already have enough mechanisms to help\nusers tweak their existing set-up, so we may not need code changes,\nbut I am wondering if we want to add a bit of documentation around\nsafe.directory to tell them when it makes sense to set it, what\nvalue(s) they would want to set it to, etc.\n\n * For \"git daemon\" invocations, because we know the command is run\n after chdir to a directory with '.' specified as the repository,\n we recommend to have safe.directory=., either on the command line\n with \"-c var=val\" or in daemon user's ~/.gitconfig, in the\n \"git-daemon\" help page? We could recommend safe.directory=*, but\n they would mean the same thing in the context of running \"git\n daemon\".\n\n We may want to discuss who protects from whom with the\n safe.directory mechanism and git-daemon-export-ok mechanism. The\n former is \"the daemon trusts that repositories won't harm the\n daemon user\", while the latter is \"the repository owner is OK for\n it to be published\".\n\n Also optionally, we may update the code to take the absolute path\n of the repository before passing it to the safe.directory check.\n\n * For \"http-backend\" invocations, we should think about potential\n additions that would help users, similar to what I listed above\n for \"git daemon\".\n\nHaving said all that, I do not think I mind GIT_SAFE_DIRECTORIES\nthat is a \":\" separated list of paths that is honored just like the\nmulti-valued configuration variable safe.directory. Once an\nattacker can influence your environment variables, it already is\ngame over, so trusting it does not make the attack surface any\nworse. As Peff explained, we can trigger the more general \"git -c\nvar=val\" mechanism by exporting a set of environment variables, so\nsuch a specialized environment variable is not strictly needed, but\nit would make writing the \"SetEnv\" directive in apache configuration\n(and similar ones for other HTTP server implementations) slighly\nsimpler and a lot more straight-forward.\n\n\n"},{"id":"497802","messageId":"27cadffb-ca3f-487d-86b7-3508c45c446d@gmail.com","threadId":"61682","inReplyTo":"xmqqpls2v1zx.fsf@gitster.g","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-06-28T09:35:25Z","receivedAt":"2024-06-28T09:35:28Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 27/06/2024 16:28, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 26/06/2024 19:11, Junio C Hamano wrote:\n>>> Phillip Wood writes:\n>>>\n>>>> To expand an this a little - a couple of times I've wanted to checkout\n>>>> a bare repository that is owned by a different user. It is a pain to\n>>>> have to add a new config setting just for a one-off checkout. Being\n>>>> able to adjust the config on the command line would be very useful in\n>>>> that case.\n>>> True. As long as it is deemed safe to honor the one-off \"git -c\n>>> safe.directory=...\" from the command line, for the purpose of this\n>>> \"I who am running this 'git' process hereby declare that I trust\n>>> this and that repository\", I think it would be the best solution\n>>> for the \"git daemon\" use case.\n>>\n>> This actually works already, the behavior was changed in 6061601d9f\n>> (safe.directory: use git_protected_config(), 2022-07-14). The reason I\n>> thought it didn't work was that I remember it failing on Debian\n>> bullseye a few months ago but that used an older version of git. There\n>> is some more rationale for the change in 779ea9303a7 (Documentation:\n>> define protected configuration, 2022-07-14)\n> \n> Thanks.\n> \n> So, does this more or less conclude the episode about how best to\n> deal with the 2.45.1 regression that Florian's patch in this thread\n> started? \n\nI think so yes\n\n> It seems that we already have enough mechanisms to help\n> users tweak their existing set-up, so we may not need code changes,\n> but I am wondering if we want to add a bit of documentation around\n> safe.directory to tell them when it makes sense to set it, what\n> value(s) they would want to set it to, etc.\n> \n> * For \"git daemon\" invocations, because we know the command is run\n> after chdir to a directory with '.' specified as the repository,\n> we recommend to have safe.directory=., either on the command line\n> with \"-c var=val\" or in daemon user's ~/.gitconfig, in the\n> \"git-daemon\" help page? We could recommend safe.directory=*, but\n> they would mean the same thing in the context of running \"git\n> daemon\".\n\nI think we'd be better to fix the safe.directory check as you suggest \nbelow if we can but failing that updating the documentation would \ncertainly help.\n\n> We may want to discuss who protects from whom with the\n> safe.directory mechanism and git-daemon-export-ok mechanism. The\n> former is \"the daemon trusts that repositories won't harm the\n> daemon user\", while the latter is \"the repository owner is OK for\n> it to be published\".\n\nYes that would be helpful\n\n> Also optionally, we may update the code to take the absolute path\n> of the repository before passing it to the safe.directory check.\n\nI think doing this would be more helpful than updating the documentation \nto recommend adding \"safe.directory=.\". If we do this we would also want \nto convert \"//\" -> \"/\" in the config keys as we've been forcing users to \nadd paths like \"/srv/git//my-repo\" if the --base-path argument to \ngit-daemon ended with a \"/\"\n\n> * For \"http-backend\" invocations, we should think about potential\n> additions that would help users, similar to what I listed above\n> for \"git daemon\".\n\nThat sounds sensible.\n\n> Having said all that, I do not think I mind GIT_SAFE_DIRECTORIES\n> that is a \":\" separated list of paths that is honored just like the\n> multi-valued configuration variable safe.directory. Once an\n> attacker can influence your environment variables, it already is\n> game over, so trusting it does not make the attack surface any\n> worse.\n\nIndeed in that case the attacker can influence the path that we read the \nprotected config from by setting $HOME (and do far worse by setting $PATH)\n\n> As Peff explained, we can trigger the more general \"git -c\n> var=val\" mechanism by exporting a set of environment variables, so\n> such a specialized environment variable is not strictly needed, but\n> it would make writing the \"SetEnv\" directive in apache configuration\n> (and similar ones for other HTTP server implementations) slighly\n> simpler and a lot more straight-forward.\n\nYes having to set all the GIT_CONFIG_* variables can be rather confusing\n\nBest Wishes\n\nPhillip\n"},{"id":"497815","messageId":"xmqq7ce96mix.fsf@gitster.g","threadId":"61682","inReplyTo":"27cadffb-ca3f-487d-86b7-3508c45c446d@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-06-28T16:48:54Z","receivedAt":"2024-06-28T16:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> We may want to discuss who protects from whom with the\n>> safe.directory mechanism and git-daemon-export-ok mechanism. The\n>> former is \"the daemon trusts that repositories won't harm the\n>> daemon user\", while the latter is \"the repository owner is OK for\n>> it to be published\".\n>\n> Yes that would be helpful\n\nOK, let's see if somebody volunteers for documentation updates in\nthis area.\n\n> I think doing this would be more helpful than updating the\n> documentation to recommend adding \"safe.directory=.\". If we do this we\n> would also want to convert \"//\" -> \"/\" in the config keys as we've\n> been forcing users to add paths like \"/srv/git//my-repo\" if the\n> --base-path argument to git-daemon ended with a \"/\"\n\nOK, so the idea is to normalize both safe.directory and data->path\n(which might come from either worktree or gitdir) and then look for\na match. path needs to be normalized because it can say '.' and\n'/srv/git//my-repo', and values of safe.directory need to be\nnormalized because the users may have written '.' --- oops, relative\nto what directory do we normalize safe.directory values? That would\nnot work. Let me retry.\n\n - Compare entries of safe.directory with data->path literally\n without normalization, as the user may have written in the\n configuration \"safe.directory=.\", expecting that data->path to be\n '.' (the git-daemon use case).\n\n - Normalize entries of safe.directory and data->path and then\n compare them, turning path=\".\" (the git-daemon use case) into\n \"/srv/git/my-repo\" and a safe.directory entry \"/srv/git//my-repo\"\n user wrote into \"/srv/git/my-repo\", so that they match. \n\nOr we could treat \".\" on safe.directory as a synonym for \"*\"\n(i.e. \"anything goes\"), and compare all other cases only after\nnormalization (which would save the cost of \"literal\" comparison for\nsafe.directory entries that are not \".\")?\n\nI may have missed some corner cases, but either of these would\nprobably work.\n\n>> * For \"http-backend\" invocations, we should think about potential\n>> additions that would help users, similar to what I listed above\n>> for \"git daemon\".\n>\n> That sounds sensible.\n\nOK.\n\n>> Having said all that, I do not think I mind GIT_SAFE_DIRECTORIES\n>> that is a \":\" separated list of paths that is honored just like the\n>> multi-valued configuration variable safe.directory. Once an\n>> attacker can influence your environment variables, it already is\n>> game over, so trusting it does not make the attack surface any\n>> worse.\n>\n> Indeed in that case the attacker can influence the path that we read\n> the protected config from by setting $HOME (and do far worse by\n> setting $PATH)\n> ...\n> Yes having to set all the GIT_CONFIG_* variables can be rather confusing\n\nOK. \n\nSo an independent effort may be to introduce the said environment\nvariable, and have it split at ':' and feed into the same machinery\nused to check paths against safe.directory configuration. It may\nneed a minor refactoring to lift the current comparison logic that\nassumes it is _only_ driven by the git_config() callback, and we\nwould probably want to define how these two sources of whitelist\nentries interact (who overrides what, is \"an empty element clears\nall the previous entries\" still true, etc.).\n\nSo, I think we have three actionable items here.\n\nThanks.\n\n\n"},{"id":"497899","messageId":"6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com","threadId":"61682","inReplyTo":"xmqq7ce96mix.fsf@gitster.g","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2024-07-01T15:24:00Z","receivedAt":"2024-07-01T15:24:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nOn 28/06/2024 17:48, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> I think doing this would be more helpful than updating the\n>> documentation to recommend adding \"safe.directory=.\". If we do this we\n>> would also want to convert \"//\" -> \"/\" in the config keys as we've\n>> been forcing users to add paths like \"/srv/git//my-repo\" if the\n>> --base-path argument to git-daemon ended with a \"/\"\n> \n> OK, so the idea is to normalize both safe.directory and data->path\n> (which might come from either worktree or gitdir) and then look for\n> a match. path needs to be normalized because it can say '.' and\n> '/srv/git//my-repo', and values of safe.directory need to be\n> normalized because the users may have written '.' --- oops, relative\n> to what directory do we normalize safe.directory values? That would\n> not work. Let me retry.\n> \n> - Compare entries of safe.directory with data->path literally\n> without normalization, as the user may have written in the\n> configuration \"safe.directory=.\", expecting that data->path to be\n> '.' (the git-daemon use case).\n\nI'm not sure this is a good idea because it is not clear which directory \nthe user wanted to mark as safe when they added a relative directory to \nsafe.directory. In the case of git-daemon one needs both the absolute \npath to the repository and \".\" to be present in safe.directory so we can \nignore \".\" and match the absolute path.\n\n> - Normalize entries of safe.directory and data->path and then\n> compare them, turning path=\".\" (the git-daemon use case) into\n> \"/srv/git/my-repo\" and a safe.directory entry \"/srv/git//my-repo\"\n> user wrote into \"/srv/git/my-repo\", so that they match.\n\nWe have several of normalization functions available:\n\n - normalize_path_copy() does a textual normalization which cleans up\n \"//\", \"/./\" and \"/../\".\n\n - absolute_pathdup() which prepends the current directory to relative\n paths attempting to use $PWD for the current directory where possible\n but does not expand symbolic links and does not clean up the path\n passed to it.\n\n - real_pathdup() which expands symbolic links\n\nOne way forward would be to clean up the entries in safe.directory with \nnormalize_path_copy() and compare them to the result of normalizing \n$git_dir with absolute_pathdup() followed by normalize_path_copy(). That \nwill ensure that we're always comparing the safe.directory entries \nagainst an absolute path and both sides of the comparison are textually \nnormalized. I'm not sure whether we'd be better to use \nabsolute_pathdup() or real_pathdup() or if we'd be safer comparing the \noutput of both against safe.directory if they give different results. If \nthis sounds reasonable I'll try and put a patch together later this week.\n\n> Or we could treat \".\" on safe.directory as a synonym for \"*\"\n> (i.e. \"anything goes\"), and compare all other cases only after\n> normalization (which would save the cost of \"literal\" comparison for\n> safe.directory entries that are not \".\")?\n\nHaving more than one way to spell \"*\" sounds confusing. Assuming \".\" has \nonly been added as a workaround for the current limitations in our \nsafe.directory comparisons I think we'd be better to ignore it.\n\nBest Wishes\n\nPhillip\n\n> I may have missed some corner cases, but either of these would\n> probably work.\n> \n>>> * For \"http-backend\" invocations, we should think about potential\n>>> additions that would help users, similar to what I listed above\n>>> for \"git daemon\".\n>>\n>> That sounds sensible.\n> \n> OK.\n> \n>>> Having said all that, I do not think I mind GIT_SAFE_DIRECTORIES\n>>> that is a \":\" separated list of paths that is honored just like the\n>>> multi-valued configuration variable safe.directory. Once an\n>>> attacker can influence your environment variables, it already is\n>>> game over, so trusting it does not make the attack surface any\n>>> worse.\n>>\n>> Indeed in that case the attacker can influence the path that we read\n>> the protected config from by setting $HOME (and do far worse by\n>> setting $PATH)\n>> ...\n>> Yes having to set all the GIT_CONFIG_* variables can be rather confusing\n> \n> OK.\n> \n> So an independent effort may be to introduce the said environment\n> variable, and have it split at ':' and feed into the same machinery\n> used to check paths against safe.directory configuration. It may\n> need a minor refactoring to lift the current comparison logic that\n> assumes it is _only_ driven by the git_config() callback, and we\n> would probably want to define how these two sources of whitelist\n> entries interact (who overrides what, is \"an empty element clears\n> all the previous entries\" still true, etc.).\n> \n> So, I think we have three actionable items here.\n> \n> Thanks.\n> \n> \n"},{"id":"497906","messageId":"ae658244-877f-c5cf-8947-83b87b66d01f@gmx.de","threadId":"61682","inReplyTo":"72e42e9f-5b85-4863-8506-c99d658d7596@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-07-01T16:34:10Z","receivedAt":"2024-07-01T16:34:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 26 Jun 2024, Phillip Wood wrote:\n\n> On 26/06/2024 14:11, Phillip Wood wrote:\n> > Hi Florian\n> >\n> > On 26/06/2024 13:33, Florian Schmaus wrote:\n> > > Sometimes more flexibility to disable/ignore the ownership check, besides\n> > > the safe.directory configuration option, is required.\n> > >\n> > > For example, git-daemon running as nobody user, which typically has no\n> > > home directory. Therefore, we can not add the path to a user-global\n> > > configuration and adding the path to the system-wide configuration could\n> > > have negative security implications.\n> > >\n> > > Therefore, make the check configurable via an environment variable.\n> >\n> > An alternative would be to allow safe.directory to be specified on the\n> > command line with \"git -c safe.directory='*' daemon ...\" rather than adding\n> > a dedicated environment variable.\n>\n> To expand an this a little - a couple of times I've wanted to checkout a bare\n> repository that is owned by a different user. It is a pain to have to add a\n> new config setting just for a one-off checkout. Being able to adjust the\n> config on the command line would be very useful in that case.\n\nIt is somewhat surprising that this `-c safe.directory=*` method does\n_not_ work for local clones. To verify, I ran this:\n\n git init --bare other-user.git &&\n sudo chown -R 9999.9999 other-user.git/ &&\n git -c safe.directory=\\* clone other-user.git/\n\nThis will complain about the dubious ownership and suggest to add the\n`safe.directory` setting to the user-wide config, ignoring the\ncommand-line config altogether.\n\nThe reason is to be found in\nhttps://github.com/git/git/blob/v2.45.2/connect.c#L1462-L1464:\n\n\t\t/* remove repo-local variables from the environment */\n\t\tfor (var = local_repo_env; *var; var++)\n\t\t\tstrvec_push(&conn->env, *var);\n\nThe `local_repo_env` array _specifically_ lists `GIT_CONFIG_PARAMETERS` in\nhttps://github.com/git/git/blob/v2.45.2/environment.c#L129 to be removed\nfrom the environment when spawning the `git upload-pack` process.\n\nIt was not originally listed, but added via\nhttps://lore.kernel.org/git/20100824064114.GA20724@burratino/, where the\ncommit message does not really shed light into the question why this was\ndesirable, and there is no discussion in that mail thread about this\naspect of the patch, but at least the added test case reveals the\nintention in some sort of way: The `-c` option allows to specify\n`receive.denyDeletes`, and in the described scenario the idea was that it\nwould only apply to the client side of a local `receive-pack` but not the\n\"remote\" one. As the example above illustrates, that patch might have\nbeen overly focused on one specific, particular scenario.\n\nCiao,\nJohannes\n"},{"id":"497908","messageId":"xmqqtth9xbk7.fsf@gitster.g","threadId":"61682","inReplyTo":"6d5b75a6-639d-429b-bd37-232fc6f475af@gmail.com","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T17:32:40Z","receivedAt":"2024-07-01T17:32:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n>> - Compare entries of safe.directory with data->path literally\n>> without normalization, as the user may have written in the\n>> configuration \"safe.directory=.\", expecting that data->path to be\n>> '.' (the git-daemon use case).\n>\n> I'm not sure this is a good idea because it is not clear which\n> directory the user wanted to mark as safe when they added a relative\n> directory to safe.directory. In the case of git-daemon one needs both\n> the absolute path to the repository and \".\" to be present in\n> safe.directory so we can ignore \".\" and match the absolute path.\n\nIOW, we do not bend over backwards to try to be backward compatible\non this point? I can go with that, especially because it smells\nlike (I haven't thought deeply about it yet, though) that approach\ncan simplify the checks.\n\n>> - Normalize entries of safe.directory and data->path and then\n>> compare them, turning path=\".\" (the git-daemon use case) into\n>> \"/srv/git/my-repo\" and a safe.directory entry \"/srv/git//my-repo\"\n>> user wrote into \"/srv/git/my-repo\", so that they match.\n>\n> We have several of normalization functions available:\n>\n> - normalize_path_copy() does a textual normalization which cleans up\n> \"//\", \"/./\" and \"/../\".\n>\n> - absolute_pathdup() which prepends the current directory to relative\n> paths attempting to use $PWD for the current directory where possible\n> but does not expand symbolic links and does not clean up the path\n> passed to it.\n>\n> - real_pathdup() which expands symbolic links\n>\n> One way forward would be to clean up the entries in safe.directory\n> with normalize_path_copy() and compare them to the result of\n> normalizing $git_dir with absolute_pathdup() followed by\n> normalize_path_copy(). That will ensure that we're always comparing\n> the safe.directory entries against an absolute path and both sides of\n> the comparison are textually normalized. I'm not sure whether we'd be\n> better to use absolute_pathdup() or real_pathdup() or if we'd be safer\n> comparing the output of both against safe.directory if they give\n> different results. If this sounds reasonable I'll try and put a patch\n> together later this week.\n\nHmph. Are runtime-detected (not from $GIT_DIR environment and\nfriends) gitdir and/or worktree paths always relative? If we let\ngetcwd() involved in the process of turning them absolute, these\npaths may already have symbolic links \"expanded\", so we have no\nchoice other than expanding symbolic links before using paths in\nsafe.directory to compare with them.\n\nFor example, the paths from safe.directory come from the end-user,\nand may say things like \"/home/phillip/repos/*\", because phillip and\neverybody else on the system think /home/$USER should be where their\nhome directories ought to be, but that was based on the niceness of\nsysadmins to arrange \"/home/phillip\" to be a symbolic link to\n\"/home1/phillip\" because the \"/home\" partition has already run out\nof space to add more users. When gitdir and/or worktree paths are\ncomputed using getcwd(), they would be paths somewhere under the\n\"/home1/phillip/repos/\" directory. Letting real_pathdup() to deal\nwith the symbolic links may be the only usable way in such a set-up\navailable for us.\n\nBut we will of course make tons more synonymous paths by using\nreal_pathdup() to normalize both the safe.directory entries and the\ngitdir and/or worktree paths---are there security downsides in doing\nso?\n"},{"id":"497910","messageId":"20240701181916.GD3199@coredump.intra.peff.net","threadId":"61682","inReplyTo":"ae658244-877f-c5cf-8947-83b87b66d01f@gmx.de","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-01T18:19:16Z","receivedAt":"2024-07-01T18:19:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 01, 2024 at 06:34:10PM +0200, Johannes Schindelin wrote:\n\n> The `local_repo_env` array _specifically_ lists `GIT_CONFIG_PARAMETERS` in\n> https://github.com/git/git/blob/v2.45.2/environment.c#L129 to be removed\n> from the environment when spawning the `git upload-pack` process.\n> \n> It was not originally listed, but added via\n> https://lore.kernel.org/git/20100824064114.GA20724@burratino/, where the\n> commit message does not really shed light into the question why this was\n> desirable, and there is no discussion in that mail thread about this\n> aspect of the patch, but at least the added test case reveals the\n> intention in some sort of way: The `-c` option allows to specify\n> `receive.denyDeletes`, and in the described scenario the idea was that it\n> would only apply to the client side of a local `receive-pack` but not the\n> \"remote\" one. As the example above illustrates, that patch might have\n> been overly focused on one specific, particular scenario.\n\nOne reason we haven't loosened local_repo_env for the local transport is\nthat it would make it inconsistent with all of the other transports.\nI.e., \"git -c foo.bar=baz fetch\" would still be ignored over ssh, https,\nand so on, because those transports don't pass environment variables.\n\nThere's some more discussion from a similar case that came up a month\nago:\n\n https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"497932","messageId":"xmqqplrwvob1.fsf@gitster.g","threadId":"61682","inReplyTo":"20240701181916.GD3199@coredump.intra.peff.net","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-01T20:40:18Z","receivedAt":"2024-07-01T20:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 01, 2024 at 06:34:10PM +0200, Johannes Schindelin wrote:\n>\n>> The `local_repo_env` array _specifically_ lists `GIT_CONFIG_PARAMETERS` in\n>> https://github.com/git/git/blob/v2.45.2/environment.c#L129 to be removed\n>> from the environment when spawning the `git upload-pack` process.\n>> \n>> It was not originally listed, but added via\n>> https://lore.kernel.org/git/20100824064114.GA20724@burratino/, where the\n>> commit message does not really shed light into the question why this was\n>> desirable, and there is no discussion in that mail thread about this\n>> aspect of the patch, but at least the added test case reveals the\n>> intention in some sort of way: The `-c` option allows to specify\n>> `receive.denyDeletes`, and in the described scenario the idea was that it\n>> would only apply to the client side of a local `receive-pack` but not the\n>> \"remote\" one. As the example above illustrates, that patch might have\n>> been overly focused on one specific, particular scenario.\n>\n> One reason we haven't loosened local_repo_env for the local transport is\n> that it would make it inconsistent with all of the other transports.\n> I.e., \"git -c foo.bar=baz fetch\" would still be ignored over ssh, https,\n> and so on, because those transports don't pass environment variables.\n>\n> There's some more discussion from a similar case that came up a month\n> ago:\n>\n> https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/\n\nThanks. I wonder if there is a way to add this kind of pieces of\ninformation to old commits and discussion threads around it after\nthe fact, and if it helps us (like Dscho who wondered why we decided\nif it is a good idea, and more importantly if we still think it is a\ngood idea and why).\n\n ... and then goes back to see the original discussion thread,\n with the \"bright idea\" that I could just follow up on 14-year\n old discussion thread. Only to find that despite what Dscho\n said, the commit message does say why it is desirable (\"to\n imitate remote transport well\") already.\n\nSo, I guess we do not really need to do such a post-annotation in\nthis particular case, but I think after seeing somebody posting a\nmessage like the one I am responding to and finding it helpful, it\nwould be helpful if somebody can post a message pointing at it as a\nresponse to the old thread that wants a post-annotation.\n\n"},{"id":"497943","messageId":"20240701222541.GC20683@coredump.intra.peff.net","threadId":"61682","inReplyTo":"xmqqplrwvob1.fsf@gitster.g","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-01T22:25:41Z","receivedAt":"2024-07-01T22:25:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Eric for some possible public-inbox wisdom]\n\nOn Mon, Jul 01, 2024 at 01:40:18PM -0700, Junio C Hamano wrote:\n\n> > There's some more discussion from a similar case that came up a month\n> > ago:\n> >\n> > https://lore.kernel.org/git/20240529102307.GF1098944@coredump.intra.peff.net/\n> \n> Thanks. I wonder if there is a way to add this kind of pieces of\n> information to old commits and discussion threads around it after\n> the fact, and if it helps us (like Dscho who wondered why we decided\n> if it is a good idea, and more importantly if we still think it is a\n> good idea and why).\n> \n> ... and then goes back to see the original discussion thread,\n> with the \"bright idea\" that I could just follow up on 14-year\n> old discussion thread. Only to find that despite what Dscho\n> said, the commit message does say why it is desirable (\"to\n> imitate remote transport well\") already.\n> \n> So, I guess we do not really need to do such a post-annotation in\n> this particular case, but I think after seeing somebody posting a\n> message like the one I am responding to and finding it helpful, it\n> would be helpful if somebody can post a message pointing at it as a\n> response to the old thread that wants a post-annotation.\n\nUsually I find myself digging backwards in history, following links to\nold threads. But I guess what you are asking is how would somebody\nlooking at old thread XYZ know that it was mentioned much later.\n\nAnd I think the solution is for the new thread to just link to the old\none by message-id (i.e., the usual lore links). And then searching for\nthat message-id in the archive could turn up the later threads. I don't\nknow how well public-inbox handles that in practice, though:\n\n 1. Do things that look like message-ids get searched for in message\n bodies? I'd think so if you don't explicitly say \"this is a message\n id\".\n\n 2. It's really a multi-element search. If I have a thread with 10\n messages, I'd really like to know of more recent threads that\n linked back to _any_ message in the thread. You'd probably have to\n feed them all manually. But in theory indexing could generate some\n kind of bidirectional \"related\" link.\n\nI don't often do this with message-ids, but I frequently do find other\nreferences by doing a full-text search for commit hashes, or phrases\nfrom commit subjects. I usually do so with my local notmuch archive,\nrather than using public-inbox, but I think you should be able to do\nphrase searches there, too.\n\n-Peff\n"},{"id":"497949","messageId":"20240702001953.M35917@dcvr","threadId":"61682","inReplyTo":"20240701222541.GC20683@coredump.intra.peff.net","subject":"Re: [PATCH] setup: support GIT_IGNORE_INSECURE_OWNER environment variable","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2024-07-02T00:19:52Z","receivedAt":"2024-07-02T00:29:01Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King wrote:\n> On Mon, Jul 01, 2024 at 01:40:18PM -0700, Junio C Hamano wrote:\n> > Thanks. I wonder if there is a way to add this kind of pieces of\n> > information to old commits and discussion threads around it after\n> > the fact, and if it helps us (like Dscho who wondered why we decided\n> > if it is a good idea, and more importantly if we still think it is a\n> > good idea and why).\n> > \n> > ... and then goes back to see the original discussion thread,\n> > with the \"bright idea\" that I could just follow up on 14-year\n> > old discussion thread. Only to find that despite what Dscho\n> > said, the commit message does say why it is desirable (\"to\n> > imitate remote transport well\") already.\n> > \n> > So, I guess we do not really need to do such a post-annotation in\n> > this particular case, but I think after seeing somebody posting a\n> > message like the one I am responding to and finding it helpful, it\n> > would be helpful if somebody can post a message pointing at it as a\n> > response to the old thread that wants a post-annotation.\n\nI think adding code comments referencing commit OIDs and URLs\nwith Message-IDs can help. I've noticed many people (usually in\nother projects) haven't learned to use git (log|blame) :<\n\n> Usually I find myself digging backwards in history, following links to\n> old threads. But I guess what you are asking is how would somebody\n> looking at old thread XYZ know that it was mentioned much later.\n> \n> And I think the solution is for the new thread to just link to the old\n> one by message-id (i.e., the usual lore links). And then searching for\n> that message-id in the archive could turn up the later threads. I don't\n> know how well public-inbox handles that in practice, though:\n> \n> 1. Do things that look like message-ids get searched for in message\n> bodies? I'd think so if you don't explicitly say \"this is a message\n> id\".\n\nYes, encapsulating via double-quotes to turn it into a phrase\nsearch may help if the Message-ID contains dashes and such.\n\n> 2. It's really a multi-element search. If I have a thread with 10\n> messages, I'd really like to know of more recent threads that\n> linked back to _any_ message in the thread. You'd probably have to\n> feed them all manually. But in theory indexing could generate some\n> kind of bidirectional \"related\" link.\n\nYou can also join a bunch of Message-IDs (or any other query)\nvia `OR' elements to ensure you don't miss things.\n\n has a lot more\ndetails on combining things which apply to notmuch, too.\n\n> I don't often do this with message-ids, but I frequently do find other\n> references by doing a full-text search for commit hashes, or phrases\n> from commit subjects. I usually do so with my local notmuch archive,\n> rather than using public-inbox, but I think you should be able to do\n> phrase searches there, too.\n\nYeah. That's fairly easy to do for Junio's git <=> git@vger\nmirror. It's more challenging to make work (and presentable)\nfor hundreds of kernel list archives and linux.git mirrors,\nespecially on my 15 year old hardware, but progress is being\nmade since I'm resorting to using C++ for Xapian :x\n"}]}