{"thread":{"id":"53312","subject":"[PATCH 0/2] credential: handle partial URLs in config settings again (master)","startedAt":"2020-04-24T22:35:53Z","lastAt":"2020-04-24T22:53:39Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"396199","messageId":"pull.620.git.1587767749.gitgitgadget@gmail.com","threadId":"53312","inReplyTo":null,"subject":"[PATCH 0/2] credential: handle partial URLs in config settings again (master)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T22:35:47Z","receivedAt":"2020-04-24T22:35:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"This is a companion patch to https://github.com/gitgitgadget/git/pull/615,\nbut targets maint instead of maint-2.17: There is not really a good way to\nget those patches into maint by merging because the final patch looks quite\na bit different by necessity (after 46fd7b390034 (credential: allow wildcard\npatterns when matching config, 2020-02-20) the urlmatch machinery is\nsupposed to handle credential..* config parsing).\n\nIMHO the easiest strategy would be to apply these patches on top of maint,\nthen merge the other patches on top of maint-2.17 with -s ours.\n\nNote: typically I do not send GitGitGadget PRs before the builds finished,\njust in case that I did something really stupid that breaks plenty of tests.\nHowever, in this case, I figure that y'all want to see them patches early,\nand I can't wait for the build to finish because I need to get some rest,\nlike, right now.\n\nJohannes Schindelin (2):\n credential: optionally allow partial URLs in\n credential_from_url_gently()\n credential: handle `credential..` again\n\n credential.c | 69 ++++++++++++++++++++++++++++++++++++++----\n t/t0300-credentials.sh | 38 +++++++++++++++++++++++\n urlmatch.c | 10 ++++--\n urlmatch.h | 5 +++\n 4 files changed, 113 insertions(+), 9 deletions(-)\n\n\nbase-commit: af6b65d45ef179ed52087e80cb089f6b2349f4ec\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-620%2Fdscho%2Fcredential-config-partial-url-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-620/dscho/credential-config-partial-url-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/620\n-- \ngitgitgadget\n"},{"id":"396200","messageId":"3653df3fc625caa5e39bbadfdde3ccc074260f5c.1587767749.git.gitgitgadget@gmail.com","threadId":"53312","inReplyTo":"pull.620.git.1587767749.gitgitgadget@gmail.com","subject":"[PATCH 1/2] credential: optionally allow partial URLs in credential_from_url_gently()","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T22:35:48Z","receivedAt":"2020-04-24T22:35:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nPrior to the fixes for CVE-2020-11008, we were _very_ lenient in what we\nrequired from a URL in order to parse it into a `struct credential`.\nThat led to serious vulnerabilities.\n\nThere was one call site, though, that really needed that leniency: when\nparsing config settings a la `credential.dev.azure.com.useHTTPPath`.\nSettings like this might be desired when users want to use, say, a given\nuser name on a given host, regardless of the protocol to be used.\n\nIn preparation for fixing that bug, let's refactor the code to\noptionally allow for partial URLs. For the moment, this functionality is\nonly exposed via the now-renamed function `credential_from_url_1()`, but\nit is not used. The intention is to make it easier to verify that this\ncommit does not change the existing behavior unless explicitly allowing\nfor partial URLs.\n\nPlease note that this patch does more than just reinstating a way to\nimitate the behavior before those CVE-2020-11008 fixes: Before that, we\nwould simply ignore URLs without a protocol. In other words,\nmisleadingly, the following setting would be applied to _all_ URLs:\n\n\t[credential \"example.com\"]\n\t\tusername = that-me\n\nThe obvious intention is to match the host name only. With this patch,\nwe allow precisely that: when parsing the URL with non-zero\n`allow_partial_url`, we do not simply return success if there was no\nprotocol, but we simply leave the protocol unset and continue parsing\nthe URL.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 42 ++++++++++++++++++++++++++++++++++++------\n 1 file changed, 36 insertions(+), 6 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a5..b8f693fa288 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -377,8 +377,31 @@ static int check_url_component(const char *url, int quiet,\n \treturn -1;\n }\n \n-int credential_from_url_gently(struct credential *c, const char *url,\n-\t\t\t int quiet)\n+/*\n+ * Potentially-partial URLs can, but do not have to, contain\n+ *\n+ * - a protocol (or scheme) of the form \"://\"\n+ *\n+ * - a host name (the part after the protocol and before the first slash after\n+ * that, if any)\n+ *\n+ * - a user name and potentially a password (as \"[:]@\" part of\n+ * the host name)\n+ *\n+ * - a path (the part after the host name, if any, starting with the slash)\n+ *\n+ * Missing parts will be left unset in `struct credential`. Thus, `https://`\n+ * will have only the `protocol` set, `example.com` only the host name, and\n+ * `/git` only the path.\n+ *\n+ * Note that an empty host name in an otherwise fully-qualified URL (e.g.\n+ * `cert:///path/to/cert.pem`) will be treated as unset if we expect the URL to\n+ * be potentially partial, and only then (otherwise, the empty string is used).\n+ *\n+ * The credential_from_url() function does not allow partial URLs.\n+ */\n+static int credential_from_url_1(struct credential *c, const char *url,\n+\t\t\t\t int allow_partial_url, int quiet)\n {\n \tconst char *at, *colon, *cp, *slash, *host, *proto_end;\n \n@@ -391,12 +414,12 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t * (3) proto://:@/...\n \t */\n \tproto_end = strstr(url, \"://\");\n-\tif (!proto_end || proto_end == url) {\n+\tif (!allow_partial_url && (!proto_end || proto_end == url)) {\n \t\tif (!quiet)\n \t\t\twarning(_(\"url has no scheme: %s\"), url);\n \t\treturn -1;\n \t}\n-\tcp = proto_end + 3;\n+\tcp = proto_end ? proto_end + 3 : url;\n \tat = strchr(cp, '@');\n \tcolon = strchr(cp, ':');\n \tslash = strchrnul(cp, '/');\n@@ -420,8 +443,10 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \t\thost = at + 1;\n \t}\n \n-\tc->protocol = xmemdupz(url, proto_end - url);\n-\tc->host = url_decode_mem(host, slash - host);\n+\tif (proto_end && proto_end - url > 0)\n+\t\tc->protocol = xmemdupz(url, proto_end - url);\n+\tif (!allow_partial_url || slash - host > 0)\n+\t\tc->host = url_decode_mem(host, slash - host);\n \t/* Trim leading and trailing slashes from path */\n \twhile (*slash == '/')\n \t\tslash++;\n@@ -443,6 +468,11 @@ int credential_from_url_gently(struct credential *c, const char *url,\n \treturn 0;\n }\n \n+int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n+{\n+\treturn credential_from_url_1(c, url, 0, quiet);\n+}\n+\n void credential_from_url(struct credential *c, const char *url)\n {\n \tif (credential_from_url_gently(c, url, 0) < 0)\n-- \ngitgitgadget\n\n"},{"id":"396201","messageId":"46ed1596f91222edeed6258a95ae8f68e3b4993f.1587767749.git.gitgitgadget@gmail.com","threadId":"53312","inReplyTo":"pull.620.git.1587767749.gitgitgadget@gmail.com","subject":"[PATCH 2/2] credential: handle `credential..` again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-04-24T22:35:49Z","receivedAt":"2020-04-24T22:35:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn the patches for CVE-2020-11008, the ability to specify credential\nsettings in the config for partial URLs got lost. For example, it used\nto be possible to specify a credential helper for a specific protocol:\n\n\t[credential \"https://\"]\n\t\thelper = my-https-helper\n\nLikewise, it used to be possible to configure settings for a specific\nhost, e.g.:\n\n\t[credential \"dev.azure.com\"]\n\t\tuseHTTPPath = true\n\nLet's reinstate this behavior.\n\nWhile at it, increase the test coverage to document and verify the\nbehavior with a couple other categories of partial URLs.\n\nSigned-off-by: Johannes Schindelin \n---\n credential.c | 27 +++++++++++++++++++++++++++\n t/t0300-credentials.sh | 38 ++++++++++++++++++++++++++++++++++++++\n urlmatch.c | 10 +++++++---\n urlmatch.h | 5 +++++\n 4 files changed, 77 insertions(+), 3 deletions(-)\n\ndiff --git a/credential.c b/credential.c\nindex b8f693fa288..4e7197d97a9 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -37,6 +37,10 @@ int credential_match(const struct credential *want,\n #undef CHECK\n }\n \n+\n+static int credential_from_potentially_partial_url(struct credential *c,\n+\t\t\t\t\t\t const char *url);\n+\n static int credential_config_callback(const char *var, const char *value,\n \t\t\t\t void *data)\n {\n@@ -82,6 +86,22 @@ static int select_all(const struct urlmatch_item *a,\n \treturn 0;\n }\n \n+static int match_partial_url(const char *url, void *cb)\n+{\n+\tstruct credential *c = cb;\n+\tstruct credential want = CREDENTIAL_INIT;\n+\tint matches = 0;\n+\n+\tif (credential_from_potentially_partial_url(&want, url) < 0)\n+\t\twarning(_(\"skipping credential lookup for key: credential.%s\"),\n+\t\t\turl);\n+\telse\n+\t\tmatches = credential_match(&want, c);\n+\tcredential_clear(&want);\n+\n+\treturn matches;\n+}\n+\n static void credential_apply_config(struct credential *c)\n {\n \tchar *normalized_url;\n@@ -101,6 +121,7 @@ static void credential_apply_config(struct credential *c)\n \tconfig.collect_fn = credential_config_callback;\n \tconfig.cascade_fn = NULL;\n \tconfig.select_fn = select_all;\n+\tconfig.fallback_match_fn = match_partial_url;\n \tconfig.cb = c;\n \n \tcredential_format(c, &url);\n@@ -468,6 +489,12 @@ static int credential_from_url_1(struct credential *c, const char *url,\n \treturn 0;\n }\n \n+static int credential_from_potentially_partial_url(struct credential *c,\n+\t\t\t\t\t\t const char *url)\n+{\n+\treturn credential_from_url_1(c, url, 1, 0);\n+}\n+\n int credential_from_url_gently(struct credential *c, const char *url, int quiet)\n {\n \treturn credential_from_url_1(c, url, 0, quiet);\ndiff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh\nindex 5555a1524f1..c6643288ed6 100755\n--- a/t/t0300-credentials.sh\n+++ b/t/t0300-credentials.sh\n@@ -575,4 +575,42 @@ test_expect_success 'credential system refuses to work with missing protocol' '\n \ttest_i18ncmp expect stderr\n '\n \n+test_expect_success 'credential config with partial URLs' '\n+\techo \"echo password=yep\" | write_script git-credential-yep &&\n+\ttest_write_lines url=https://user@example.com/repo.git >stdin &&\n+\tfor partial in \\\n+\t\texample.com \\\n+\t\tuser@example.com \\\n+\t\thttps:// \\\n+\t\thttps://example.com \\\n+\t\thttps://example.com/ \\\n+\t\thttps://user@example.com \\\n+\t\thttps://user@example.com/ \\\n+\t\thttps://example.com/repo.git \\\n+\t\thttps://user@example.com/repo.git \\\n+\t\t/repo.git\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill stdout &&\n+\t\tgrep yep stdout ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tfor partial in \\\n+\t\tdont.use.this \\\n+\t\thttp:// \\\n+\t\t/repo\n+\tdo\n+\t\tgit -c credential.$partial.helper=yep \\\n+\t\t\tcredential fill stdout &&\n+\t\t! grep yep stdout ||\n+\t\treturn 1\n+\tdone &&\n+\n+\tgit -c credential.$partial.helper=yep \\\n+\t\t-c credential.with%0anewline.username=uh-oh \\\n+\t\tcredential fill stdout 2>stderr &&\n+\ttest_i18ngrep \"skipping credential lookup for key\" stderr\n+'\n+\n test_done\ndiff --git a/urlmatch.c b/urlmatch.c\nindex 29272a5c4f4..33a2ccd306b 100644\n--- a/urlmatch.c\n+++ b/urlmatch.c\n@@ -572,10 +572,14 @@ int urlmatch_config_entry(const char *var, const char *value, void *cb)\n \n \t\tconfig_url = xmemdupz(key, dot - key);\n \t\tnorm_url = url_normalize_1(config_url, &norm_info, 1);\n+\t\tif (norm_url)\n+\t\t\tretval = match_urls(url, &norm_info, &matched);\n+\t\telse if (collect->fallback_match_fn)\n+\t\t\tretval = collect->fallback_match_fn(config_url,\n+\t\t\t\t\t\t\t collect->cb);\n+\t\telse\n+\t\t\tretval = 0;\n \t\tfree(config_url);\n-\t\tif (!norm_url)\n-\t\t\treturn 0;\n-\t\tretval = match_urls(url, &norm_info, &matched);\n \t\tfree(norm_url);\n \t\tif (!retval)\n \t\t\treturn 0;\ndiff --git a/urlmatch.h b/urlmatch.h\nindex 2407520731f..6ff42f81b0c 100644\n--- a/urlmatch.h\n+++ b/urlmatch.h\n@@ -59,6 +59,11 @@ struct urlmatch_config {\n \t * specificity rules) than existing.\n \t */\n \tint (*select_fn)(const struct urlmatch_item *found, const struct urlmatch_item *existing);\n+\t/*\n+\t * An optional callback to allow e.g. for partial URLs; it shall\n+\t * return 1 or 0 depending whether `url` matches or not.\n+\t */\n+\tint (*fallback_match_fn)(const char *url, void *cb);\n };\n \n int urlmatch_config_entry(const char *var, const char *value, void *cb);\n-- \ngitgitgadget\n"},{"id":"396202","messageId":"xmqqwo64a42c.fsf@gitster.c.googlers.com","threadId":"53312","inReplyTo":"pull.620.git.1587767749.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] credential: handle partial URLs in config settings again (master)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-24T22:53:31Z","receivedAt":"2020-04-24T22:53:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> This is a companion patch to https://github.com/gitgitgadget/git/pull/615,\n> but targets maint instead of maint-2.17: There is not really a good way to\n> get those patches into maint by merging because the final patch looks quite\n> a bit different by necessity (after 46fd7b390034 (credential: allow wildcard\n> patterns when matching config, 2020-02-20) the urlmatch machinery is\n> supposed to handle credential..* config parsing).\n>\n> IMHO the easiest strategy would be to apply these patches on top of maint,\n> then merge the other patches on top of maint-2.17 with -s ours.\n\nThanks for a quick help.\n"}]}