{"thread":{"id":"47974","subject":"[PATCH] http.c: shell command evaluation for extraheader","startedAt":"2018-03-04T08:40:37Z","lastAt":"2018-04-28T12:24:51Z","messageCount":4,"participants":["Colin Arnott","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"340976","messageId":"CngnTUCx1pVSyKUzINZV972DlD_JMH53F-L2PZtx0_RKMiwXYY4f2UzDPeJaC5fL-6oJBXAxQ5f0OC76I_fagEWxch5csI716N7bQDh3gR8=@urandom.co.uk","threadId":"47974","inReplyTo":null,"subject":"[PATCH] http.c: shell command evaluation for extraheader","fromName":"Colin Arnott","fromEmail":"colin@urandom.co.uk","sentAt":"2018-03-04T08:40:27Z","receivedAt":"2018-03-04T08:40:37Z","isPatch":true,"sender":{"key":"colin@urandom.co.uk","avatar":null},"body":"The http.extraHeader config parameter currently only supports storing\nconstant values. There are two main use cases where this fails:\n\n  0. Sensitive payloads: frequently this config parameter is used to pass\n     authentication credentials in place of or in addition to the\n     Authorization header, however since this value is required to be in\n     the clear this can create security issues.\n\n  1. Mutating headers: some headers, especially new authentication\n     schemes, leverage short lived tokens that change over time.\n\nThere do exist solutions with current tools for these use cases, however\nnone are optimal:\n\n  0. Shell alias: by aliasing over git with a call to git that includes the\n     config directive and evaluates the header value inline, you can\n     fake the desired mutability:\n       `alias git='git -c http.extraHeader=\"$(gpg -d crypt.gpg)\"'`\n     This presents two problems:\n     a. aliasing over commands can be confusing to new users, since git\n        config information is stored in shell configs\n     b. this solution scales only to your shell, not all shells\n\n  1. Global hook: you could implement a hook that writes the config\n     entry before fetch / pull actions, so that it is up to date, but\n     this does nothing to secure it.\n\n  2. git-credential-helper: the credential helper interface already\n     supports shelling out to arbitrary binaries or scripts, however\n     this interface can only be used to populate the Authorization\n     header.\n\nThe optimal solution involves extending the current implementation of\nhttp.extraHeader parsing to allow for arbitrary shell command execution.\nThere seem to be two paradigms for such features:\n\n  0. Overloading with '!' prefixes: seen in alias.* and credential.helper\n\n  1. New \"Cmd\" suffix parameters: seen in sendemail.toCmd sendemail.ccCmd\n\nWhile the latter may be more clear without documentation, the addition\nof a new config parameter seemed more complex for the codebase. As such,\nnew documentation is included.\n\nSeveral edge cases came up during implementation and the following\ndesign decisions were made:\n\n  0. Stdin and stderr for the child_process are exposed to the user:\n     this allows commands that print status information via stderr, and\n     accept input to function. The use case considered is text input for\n     decryption and error handling that is out of scope for git.\n\n  1. Failure to exec: if either the file does not exist, or any other\n     exec related failure occurs, no error is presented to the user,\n     and the header is not included\n\n  2. Non-zero return code: if the child_process returns a non-zero\n     value, no error is presented to the user, the return value is\n     consumed, and the header is not included in the request.\n\n  3. Headers starting with the '!' character require a shell command to\n     create: because no escaping syntax was implemented, the following\n     is required for such headers: \"!printf '!magic: abra'\"\n\nSigned-off-by: Colin Arnott <colin@urandom.co.uk>\n---\n Documentation/config.txt    |  7 +++++++\n http.c                      | 20 ++++++++++++++++++++\n t/t5551-http-fetch-smart.sh |  6 ++++--\n 3 files changed, 31 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f57e9cf10..4b2171d60 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1918,6 +1918,13 @@ http.extraHeader::\n \tmore than one such entry exists, all of them are added as extra\n \theaders.  To allow overriding the settings inherited from the system\n \tconfig, an empty value will reset the extra headers to the empty list.\n+\tIf the value is prefixed with an exclamation point, it will\n+\tbe treated as a shell command.  For example, defining\n+\t\"http.extraHeader = !gpg -d < secure_header.gpg\", will pass the\n+\tdecrypted header, if the command does not exec cleanly or has a\n+\tnon-zero return value, no header will be added.  Note that shell\n+\tcommands will be executed from the top-level directory of a\n+\trepository, which may not necessarily be the current directory.\n \n http.cookieFile::\n \tThe pathname of a file containing previously stored cookie lines,\ndiff --git a/http.c b/http.c\nindex 31755023a..11103df41 100644\n--- a/http.c\n+++ b/http.c\n@@ -380,6 +380,26 @@ static int http_options(const char *var, const char *value, void *cb)\n \t\t} else if (!*value) {\n \t\t\tcurl_slist_free_all(extra_http_headers);\n \t\t\textra_http_headers = NULL;\n+\t\t} else if (value[0] == '!') {\n+\t\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\t\tcp.git_cmd = 0;\n+\t\t\tcp.in = 0;\n+\t\t\tcp.out = -1;\n+\t\t\tcp.err = 0;\n+\t\t\tcp.use_shell = 1;\n+\t\t\targv_array_push(&cp.args, value + 1);\n+\t\t\tif (!start_command(&cp)) {\n+\t\t\t\tstruct strbuf output;\n+\t\t\t\tstrbuf_init(&output, 0);\n+\t\t\t\tstrbuf_read(&output, cp.out, 0);\n+\t\t\t\tclose(cp.out);\n+\t\t\t\tcp.out = -1;\n+\t\t\t\tif (!finish_command(&cp)) {\n+\t\t\t\t\textra_http_headers =\n+\t\t\t\t\t\tcurl_slist_append(extra_http_headers, output.buf);\n+\t\t\t\t}\n+\t\t\t\tstrbuf_release(&output);\n+\t\t\t}\n \t\t} else {\n \t\t\textra_http_headers =\n \t\t\t\tcurl_slist_append(extra_http_headers, value);\ndiff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh\nindex f5721b4a5..039afc76a 100755\n--- a/t/t5551-http-fetch-smart.sh\n+++ b/t/t5551-http-fetch-smart.sh\n@@ -351,7 +351,9 @@ test_expect_success 'custom http headers' '\n \ttest_must_fail git -c http.extraheader=\"x-magic-two: cadabra\" \\\n \t\tfetch \"$HTTPD_URL/smart_headers/repo.git\" &&\n \tgit -c http.extraheader=\"x-magic-one: abra\" \\\n-\t    -c http.extraheader=\"x-magic-two: cadabra\" \\\n+\t    -c http.extraheader=\"!printf \\\"x-magic-two: cadabra\\\"\" \\\n+\t    -c http.extraheader=\"!printf \\\"x-magic-three: alakazam; exit 2\\\"\" \\\n+\t    -c http.extraheader=\"!shellcommanddoesnotexist\" \\\n \t    fetch \"$HTTPD_URL/smart_headers/repo.git\" &&\n \tgit update-index --add --cacheinfo 160000,$(git rev-parse HEAD),sub &&\n \tgit config -f .gitmodules submodule.sub.path sub &&\n@@ -360,7 +362,7 @@ test_expect_success 'custom http headers' '\n \tgit submodule init sub &&\n \ttest_must_fail git submodule update sub &&\n \tgit -c http.extraheader=\"x-magic-one: abra\" \\\n-\t    -c http.extraheader=\"x-magic-two: cadabra\" \\\n+\t    -c http.extraheader=\"!printf \\\"x-magic-two: cadabra\\\"\" \\\n \t\tsubmodule update sub\n '\n \n-- \n2.16.2\n\n\n"},{"id":"341018","messageId":"nycvar.QRO.7.76.6.1803051437000.20700@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","threadId":"47974","inReplyTo":"CngnTUCx1pVSyKUzINZV972DlD_JMH53F-L2PZtx0_RKMiwXYY4f2UzDPeJaC5fL-6oJBXAxQ5f0OC76I_fagEWxch5csI716N7bQDh3gR8=@urandom.co.uk","subject":"Re: [PATCH] http.c: shell command evaluation for extraheader","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-03-05T13:47:48Z","receivedAt":"2018-03-05T13:47:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Colin,\n\nOn Sun, 4 Mar 2018, Colin Arnott wrote:\n\n> The http.extraHeader config parameter currently only supports storing\n> constant values. There are two main use cases where this fails:\n> \n>   0. Sensitive payloads: frequently this config parameter is used to pass\n>      authentication credentials in place of or in addition to the\n>      Authorization header, however since this value is required to be in\n>      the clear this can create security issues.\n> \n>   1. Mutating headers: some headers, especially new authentication\n>      schemes, leverage short lived tokens that change over time.\n> \n> There do exist solutions with current tools for these use cases, however\n> none are optimal:\n> \n>   0. Shell alias: by aliasing over git with a call to git that includes the\n>      config directive and evaluates the header value inline, you can\n>      fake the desired mutability:\n>        `alias git='git -c http.extraHeader=\"$(gpg -d crypt.gpg)\"'`\n>      This presents two problems:\n>      a. aliasing over commands can be confusing to new users, since git\n>         config information is stored in shell configs\n>      b. this solution scales only to your shell, not all shells\n> \n>   1. Global hook: you could implement a hook that writes the config\n>      entry before fetch / pull actions, so that it is up to date, but\n>      this does nothing to secure it.\n> \n>   2. git-credential-helper: the credential helper interface already\n>      supports shelling out to arbitrary binaries or scripts, however\n>      this interface can only be used to populate the Authorization\n>      header.\n\nAs the credential-helper is already intended for sensitive data, and as it\nalready allows to interact with a helper, I would strongly assume that it\nwould make more sense to try to extend that feature (instead of the simple\nextraHeader one).\n\nThis would also help alleviate all the quoting/dequoting issues involved\nwith shell scripting.\n\nBesides, the http.extraHeader feature was designed to accommodate all\nkinds of extra headers, not only authentication ones (and indeed, the\nauthentication was only intended for use in build agents, where both\nenvironment and logging can be controlled rather tightly).\n\nI also see that in your implementation, only the extraHeader value is\nevaluated, without any access to the rest of the metadata (such as URL,\nand optionally specified user).\n\nIt would probably get a little more complicated than a shell script to\nwrite a credential-helper that will always be asked to generate an\nauthentication, but I think even a moderate-level Perl script could be\nused for that, and it *would* know the URL and user for which the\ncredentials are intended...\n\nCiao,\nJohannes\n"},{"id":"341097","messageId":"-mpHjLv0pGYld30rBagku1GYfPM2FqZQubWD4rHt2K_uijQg0ehMjeBnZgr4K77zc2E4HAlm3eqNtP7-lXzhR8o7udP0TdNHMJK7WRauzmk=@urandom.co.uk","threadId":"47974","inReplyTo":"nycvar.QRO.7.76.6.1803051437000.20700@ZVAVAG-6OXH6DA.rhebcr.pbec.zvpebfbsg.pbz","subject":"Re: [PATCH] http.c: shell command evaluation for extraheader","fromName":"Colin Arnott","fromEmail":"colin@urandom.co.uk","sentAt":"2018-03-06T06:09:58Z","receivedAt":"2018-03-06T06:10:14Z","isPatch":true,"sender":{"key":"colin@urandom.co.uk","avatar":null},"body":"Johannes,\n\nOn March 5, 2018 1:47 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n\n> As the credential-helper is already intended for sensitive data, and as it\n> already allows to interact with a helper, I would strongly assume that it\n> would make more sense to try to extend that feature (instead of the simple\n> extraHeader one).\n\nTo confirm you are suggesting that the credential struct, defined in credential.h, be extended to include a headers array, like so:\n--- a/credential.h\n+++ b/credential.h\n@@ -15,6 +15,7 @@ struct credential {\n        char *protocol;\n        char *host;\n        char *path;\n+       char **headers\n };\n \n #define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n\n> This would also help alleviate all the quoting/dequoting issues involved\n> with shell scripting.\n> \n> Besides, the http.extraHeader feature was designed to accommodate all\n> kinds of extra headers, not only authentication ones (and indeed, the\n> authentication was only intended for use in build agents, where both\n> environment and logging can be controlled rather tightly).\n\nI realise that my examples are scoped for auth, but I can conceive of other mutating headers that are not explicitly authentication related, and could benefit from shell execution before fetch, pull, push actions.\n\n> I also see that in your implementation, only the extraHeader value is\n> evaluated, without any access to the rest of the metadata (such as URL,\n> and optionally specified user).\n>\n> It would probably get a little more complicated than a shell script to\n> write a credential-helper that will always be asked to generate an\n> authentication, but I think even a moderate-level Perl script could be\n> used for that, and it would know the URL and user for which the\n> credentials are intended...\n\nYou are correct; the scope provided by http.<url>.* is enough to meet my use cases, however I agree the lack of access to metadata limits what can be done within in the context of the shell, and makes the case for a credential-helper implementation stronger. I think there is something to be said about the simplicity and user-friendliness of allowing shell scripts for semi-complex config options, but authentication is a task that should be handled well and centrally, thus extending the credential-api makes sense.\n\n​Without Wax,\nColin Arnott​\n"},{"id":"346033","messageId":"nycvar.QRO.7.76.6.1804281421430.79@tvgsbejvaqbjf.bet","threadId":"47974","inReplyTo":"-mpHjLv0pGYld30rBagku1GYfPM2FqZQubWD4rHt2K_uijQg0ehMjeBnZgr4K77zc2E4HAlm3eqNtP7-lXzhR8o7udP0TdNHMJK7WRauzmk=@urandom.co.uk","subject":"Re: [PATCH] http.c: shell command evaluation for extraheader","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2018-04-28T12:24:40Z","receivedAt":"2018-04-28T12:24:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Colin,\n\nOn Tue, 6 Mar 2018, Colin Arnott wrote:\n\n> On March 5, 2018 1:47 PM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:\n> \n> > As the credential-helper is already intended for sensitive data, and\n> > as it already allows to interact with a helper, I would strongly\n> > assume that it would make more sense to try to extend that feature\n> > (instead of the simple extraHeader one).\n> \n> To confirm you are suggesting that the credential struct, defined in credential.h, be extended to include a headers array, like so:\n> --- a/credential.h\n> +++ b/credential.h\n> @@ -15,6 +15,7 @@ struct credential {\n>         char *protocol;\n>         char *host;\n>         char *path;\n> +       char **headers\n>  };\n>  \n>  #define CREDENTIAL_INIT { STRING_LIST_INIT_DUP }\n\nI think that was what I had in mind, yes.\n\n> > This would also help alleviate all the quoting/dequoting issues involved\n> > with shell scripting.\n> > \n> > Besides, the http.extraHeader feature was designed to accommodate all\n> > kinds of extra headers, not only authentication ones (and indeed, the\n> > authentication was only intended for use in build agents, where both\n> > environment and logging can be controlled rather tightly).\n> \n> I realise that my examples are scoped for auth, but I can conceive of\n> other mutating headers that are not explicitly authentication related,\n> and could benefit from shell execution before fetch, pull, push actions.\n\nI can conceive of yet another use case that would benefit from shell\nexecution in these scenarios: attacks. That is why I am extremely hesitant\nto go that route.\n\nBut then, I am not the maintainer of Git. If you can convince him, you're\ngood to go.\n\n> > I also see that in your implementation, only the extraHeader value is\n> > evaluated, without any access to the rest of the metadata (such as URL,\n> > and optionally specified user).\n> >\n> > It would probably get a little more complicated than a shell script to\n> > write a credential-helper that will always be asked to generate an\n> > authentication, but I think even a moderate-level Perl script could be\n> > used for that, and it would know the URL and user for which the\n> > credentials are intended...\n> \n> You are correct; the scope provided by http.<url>.* is enough to meet my\n> use cases, however I agree the lack of access to metadata limits what\n> can be done within in the context of the shell, and makes the case for a\n> credential-helper implementation stronger. I think there is something to\n> be said about the simplicity and user-friendliness of allowing shell\n> scripts for semi-complex config options, but authentication is a task\n> that should be handled well and centrally, thus extending the\n> credential-api makes sense.\n\nYes, I agree.\n\nCiao,\nJohannes\n"}]}