{"thread":{"id":"53382","subject":"[RFC PATCH] credential: minor documentation fixes","startedAt":"2020-05-03T06:40:18Z","lastAt":"2020-05-07T23:35:51Z","messageCount":29,"participants":["Carlo Marcelo Arenas Belón","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396856","messageId":"20200503063423.83152-1-carenas@gmail.com","threadId":"53382","inReplyTo":null,"subject":"[RFC PATCH] credential: minor documentation fixes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-03T06:34:23Z","receivedAt":"2020-05-03T06:40:18Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)\nchanges the implementation for creential_from_url_gently to retun -1 if\nprotocol is missing, but didn't update this blurb.\n\nthe order of parameters used in credential_match was inconsistent\nbetween credential.c and credential.h as well, so update both to\nmatch the current implementation.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex d99ec42b2a..c0e17e3554 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -177,8 +177,8 @@ void credential_write(const struct credential *, FILE *);\n * Parse a url into a credential struct, replacing any existing contents.\n *\n * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n- * resulting credential will be empty but we'll still return success from the\n- * \"gently\" form.\n+ * resulting credential will be empty and the function will return an\n+ * error (even in the \"gently\" form).\n *\n * If we encounter a component which cannot be represented as a credential\n * value (e.g., because it contains a newline), the \"gently\" form will return\n@@ -189,7 +189,7 @@ void credential_write(const struct credential *, FILE *);\n void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n-int credential_match(const struct credential *have,\n-\t\t const struct credential *want);\n+int credential_match(const struct credential *want,\n+\t\t const struct credential *have);\n \n #endif /* CREDENTIAL_H */\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"396858","messageId":"20200503065826.GB1829906@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200503063423.83152-1-carenas@gmail.com","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-03T06:58:26Z","receivedAt":"2020-05-03T06:58:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)\n> changes the implementation for creential_from_url_gently to retun -1 if\n> protocol is missing, but didn't update this blurb.\n\nYeah, this makes sense. s/creential/credential/.\n\n> the order of parameters used in credential_match was inconsistent\n> between credential.c and credential.h as well, so update both to\n> match the current implementation.\n\nYes, looks like this has been wrong since the beginning in 118250728e\n(credential: apply helper config, 2011-12-10). I checked the callers to\nmake sure none of them had gotten it backwards, but they all look right\n(and just the declaration is wrong).\n\n-Peff\n"},{"id":"396890","messageId":"20200504074520.GB86805@Carlos-MBP","threadId":"53382","inReplyTo":"20200503065826.GB1829906@coredump.intra.peff.net","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-04T07:45:20Z","receivedAt":"2020-05-04T07:45:25Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:\n> On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:\n> \n> > the order of parameters used in credential_match was inconsistent\n> > between credential.c and credential.h as well, so update both to\n> > match the current implementation.\n> \n> Yes, looks like this has been wrong since the beginning in 118250728e\n> (credential: apply helper config, 2011-12-10). I checked the callers to\n> make sure none of them had gotten it backwards, but they all look right\n> (and just the declaration is wrong).\n\nthanks for checking, will include this (and your typo fix) in the\nsubmission; should I add your \"Reviewed-by\" then?\n\nwas also curious about the other documentation updates mentioned[1] by\nJonathan, and that I was hoping this patch will be included with.\n\nsome things that I think might need clarification (or maybe even code changes\nafter agreed on) are :\n\n* the meaning of \"exactly\" for matching protocol and hostname in the URL\n since 06 are both case insensitive per RFC3986 and we have been\n ambiguous on that, leading to some helpers assuming case or encoding.\n* the rules for how helper matching are expected to be ordered, specially\n considering the recent adding of wildcard matching and the revival of\n partial matching, and the fact that the order is relevant for both\n discovery of credentials and which helpers are used.\n* the use of hostname as a key, since the addition of cert:// that has none\n and uses path instead (had emailed the author privately for clarification,\n but hadn't heard yet) and the effect that has on which fields are expected\n and which values are valid.\n* the role of overrides (ex: the documented example of passing URL and later\n updating it seems useful, eventhough I am not aware if being used)\n* clarification on which fields can be updated by the helper; currently I\n don't think we allow overrides for protocol and host and assume all others\n but the documentation doesn't clarify, and that might be a problem for\n cert:// where file is more relevant.\n\nCarlo\n\n[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/\n"},{"id":"396904","messageId":"20200504144436.GA9893@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200504074520.GB86805@Carlos-MBP","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T14:44:36Z","receivedAt":"2020-05-04T14:44:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:\n> > On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:\n> > \n> > > the order of parameters used in credential_match was inconsistent\n> > > between credential.c and credential.h as well, so update both to\n> > > match the current implementation.\n> > \n> > Yes, looks like this has been wrong since the beginning in 118250728e\n> > (credential: apply helper config, 2011-12-10). I checked the callers to\n> > make sure none of them had gotten it backwards, but they all look right\n> > (and just the declaration is wrong).\n> \n> thanks for checking, will include this (and your typo fix) in the\n> submission; should I add your \"Reviewed-by\" then?\n\nSure, feel free to.\n\n> some things that I think might need clarification (or maybe even code changes\n> after agreed on) are :\n> \n> * the meaning of \"exactly\" for matching protocol and hostname in the URL\n> since 06 are both case insensitive per RFC3986 and we have been\n> ambiguous on that, leading to some helpers assuming case or encoding.\n\nYeah, IIRC we discussed case-sensitivity at the time and went with the\nstricter behavior in the name of safety over convenience. And I don't\nthink anybody has complained since then. So I'm not really _opposed_ to\nloosening it to match the URL, but perhaps a maintenance release is not\nthe best time to do so.\n\n> * the rules for how helper matching are expected to be ordered, specially\n> considering the recent adding of wildcard matching and the revival of\n> partial matching, and the fact that the order is relevant for both\n> discovery of credentials and which helpers are used.\n\nYes, this could be better documented. I think the current rules are\nprobably not ideal, especially when you mix credential.*.helper and\ncredential.helper. So some fixes are possible there, but I think they\nmight be best added as new feature (e.g., a new config like\ncredential.*.helperOrder that says whether and how to use\nnon-url-specific helpers; or something like that).\n\n> * the use of hostname as a key, since the addition of cert:// that has none\n> and uses path instead (had emailed the author privately for clarification,\n> but hadn't heard yet) and the effect that has on which fields are expected\n> and which values are valid.\n\nYeah, there could be more discussion in gitcredentials(7) there.\n\n> * the role of overrides (ex: the documented example of passing URL and later\n> updating it seems useful, eventhough I am not aware if being used)\n\nI'd be OK to see this feature removed. I have used it for various\ndebugging or experimenting scenarios, but Git in general would never\npass anything except a broken-down set of fields, and only one of each\nfield.\n\n> * clarification on which fields can be updated by the helper; currently I\n> don't think we allow overrides for protocol and host and assume all others\n> but the documentation doesn't clarify, and that might be a problem for\n> cert:// where file is more relevant.\n\nI think we do allow a helper to transform a credential in any way:\n\n echo url=https://example.com/ |\n git \\\n -c credential.helper='!sed >&2 s/^/one:/; echo host=other.example.com;:' \\\n -c credential.helper='!sed >&2 s/^/two:/;:' \\\n credential fill\n\nproduces:\n\n one:protocol=https\n one:host=example.com\n two:protocol=https\n two:host=other.example.com\n Username for 'https://other.example.com': \n\nSo after the first helper runs, subsequent helpers (and the internal\nterminal prompt) will consider the modified hostname.\n\nNow whether that's a sane feature or not, I'm not sure.\n\n-Peff\n"},{"id":"396910","messageId":"20200504153957.GC86805@Carlos-MBP","threadId":"53382","inReplyTo":"20200504144436.GA9893@coredump.intra.peff.net","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-04T15:39:57Z","receivedAt":"2020-05-04T15:40:01Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:\n> > \n> > * the meaning of \"exactly\" for matching protocol and hostname in the URL\n> > since 06 are both case insensitive per RFC3986 and we have been\n> > ambiguous on that, leading to some helpers assuming case or encoding.\n> \n> Yeah, IIRC we discussed case-sensitivity at the time and went with the\n> stricter behavior in the name of safety over convenience. And I don't\n> think anybody has complained since then. So I'm not really _opposed_ to\n> loosening it to match the URL, but perhaps a maintenance release is not\n> the best time to do so.\n\nagree, but I was talking not in the context of a feature, but on how we\nare to define the interaction with helpers (which was meant to be part of\nthis maintenance release).\n\ncurrently (since it is undefined) a naive helper could do a caseless match\nby assuming we really meant url as defined by RFC3986, and therefore affect\nthe wrong credential by the operation.\n\nindeed; our own code might get confused so maybe something like (not fully\ntested and likely to need some test coverage) will make sense then as part\nof this maintenance release, with some additional mention that clarifies\nwe REALLY meant \"exactly\" so that helpers can be updated?\n\nCarlo\n---\ndiff --git a/credential.c b/credential.c\nindex 108d9e183a..d2c879a9b3 100644\n--- a/credential.c\n+++ b/credential.c\n@@ -70,7 +70,7 @@ static int proto_is_http(const char *s)\n {\n \tif (!s)\n \t\treturn 0;\n-\treturn !strcmp(s, \"https\") || !strcmp(s, \"http\");\n+\treturn !strcasecmp(s, \"https\") || !strcasecmp(s, \"http\");\n }\n \n static void credential_describe(struct credential *c, struct strbuf *out);\ndiff --git a/fsck.c b/fsck.c\nindex 73f30773f2..d779acdae8 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -997,10 +997,10 @@ static int url_to_curl_url(const char *url, const char **out)\n \t skip_prefix(url, \"ftp::\", out) ||\n \t skip_prefix(url, \"ftps::\", out))\n \t\treturn 1;\n-\tif (starts_with(url, \"http://\") ||\n-\t starts_with(url, \"https://\") ||\n-\t starts_with(url, \"ftp://\") ||\n-\t starts_with(url, \"ftps://\")) {\n+\tif (istarts_with(url, \"http://\") ||\n+\t istarts_with(url, \"https://\") ||\n+\t istarts_with(url, \"ftp://\") ||\n+\t istarts_with(url, \"ftps://\")) {\n \t\t*out = url;\n \t\treturn 1;\n \t}\n"},{"id":"396917","messageId":"20200504155837.GD86805@Carlos-MBP","threadId":"53382","inReplyTo":"20200504144436.GA9893@coredump.intra.peff.net","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-04T15:58:37Z","receivedAt":"2020-05-04T15:58:47Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:\n> On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:\n> > * clarification on which fields can be updated by the helper; currently I\n> > don't think we allow overrides for protocol and host and assume all others\n> > but the documentation doesn't clarify, and that might be a problem for\n> > cert:// where file is more relevant.\n> \n> I think we do allow a helper to transform a credential in any way:\n> \n> echo url=https://example.com/ |\n> git \\\n> -c credential.helper='!sed >&2 s/^/one:/; echo host=other.example.com;:' \\\n> -c credential.helper='!sed >&2 s/^/two:/;:' \\\n> credential fill\n> \n> produces:\n> \n> one:protocol=https\n> one:host=example.com\n> two:protocol=https\n> two:host=other.example.com\n> Username for 'https://other.example.com': \n> \n> So after the first helper runs, subsequent helpers (and the internal\n> terminal prompt) will consider the modified hostname.\n\ncorrect, and that is because the specification (and the current code) does\nsay that is a valid operation.\n\nwhat I meant is that we don't REALLY allow those modification to take place\nsince the credential helper suggestions can't really affect what git assumes\nis the right host or protocol (or FWIW path), unlike username/password and\nso they don't seem worth having IMHO.\n\n> Now whether that's a sane feature or not, I'm not sure.\n\nwould restricting the output of the credential helpers to username and\npassword make more sense in this context then?\n\nobviously this change will required both the protocol (definition and\nimplementation) to change an changes on ALL helpers but I thought that was\nthe objective of that documentation suggestion in Jonathan's original email.\n\nI agree though the discussion of if that is something we should do in a\nmaintenance release or not, is worth having; but I suspect it could be\nimplemented with reasonable enough backward compatibility to be considered.\n\nCarlo\n\nCC adding the author of cert:// too easy on that part of the discussion\n"},{"id":"396921","messageId":"20200504161048.GC12842@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200504153957.GC86805@Carlos-MBP","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T16:10:48Z","receivedAt":"2020-05-04T16:10:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 08:39:57AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:\n> > On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:\n> > > \n> > > * the meaning of \"exactly\" for matching protocol and hostname in the URL\n> > > since 06 are both case insensitive per RFC3986 and we have been\n> > > ambiguous on that, leading to some helpers assuming case or encoding.\n> > \n> > Yeah, IIRC we discussed case-sensitivity at the time and went with the\n> > stricter behavior in the name of safety over convenience. And I don't\n> > think anybody has complained since then. So I'm not really _opposed_ to\n> > loosening it to match the URL, but perhaps a maintenance release is not\n> > the best time to do so.\n> \n> agree, but I was talking not in the context of a feature, but on how we\n> are to define the interaction with helpers (which was meant to be part of\n> this maintenance release).\n> \n> currently (since it is undefined) a naive helper could do a caseless match\n> by assuming we really meant url as defined by RFC3986, and therefore affect\n> the wrong credential by the operation.\n\nRight, I understand. But if helpers are doing case-insensitive matches,\nI don't think that's a big deal security-wise. And if we're not for our\nhelpers, that's erring on the conservative side, but if nobody is\ncomplaining about it, I don't think it's urgent.\n\n-Peff\n"},{"id":"396922","messageId":"20200504161342.GD12842@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200504155837.GD86805@Carlos-MBP","subject":"Re: [RFC PATCH] credential: minor documentation fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-04T16:13:42Z","receivedAt":"2020-05-04T16:13:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 08:58:37AM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> what I meant is that we don't REALLY allow those modification to take place\n> since the credential helper suggestions can't really affect what git assumes\n> is the right host or protocol (or FWIW path), unlike username/password and\n> so they don't seem worth having IMHO.\n\nI guess it depends on your definition of REALLY. Sure, we won't access\na different host via curl (or a different protocol!), but I wouldn't\nexpect that. This is the credential subsystem.\n\nIf you're proposing to document it more clearly, I'm fine with that.\n\nAnd if you want to tighten it up in a major release to ignore such input\nfrom helpers, I'm fine with that, too. But I'd be worried pushing it out\nin a maintenance release and finding out that somebody _is_ depending on\nit somehow.\n\n-Peff\n"},{"id":"397038","messageId":"20200505013908.4596-2-carenas@gmail.com","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"[PATCH 1/4] credential: update description for credential_from_url_gently","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T01:39:05Z","receivedAt":"2020-05-05T01:39:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)\nchanges the implementation for this function to return -1 if protocol is\nmissing.\n\nUpdate blurb to match implementation.\n\nReviewed-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex d99ec42b2a..f51703d9ce 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -177,8 +177,8 @@ void credential_write(const struct credential *, FILE *);\n * Parse a url into a credential struct, replacing any existing contents.\n *\n * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n- * resulting credential will be empty but we'll still return success from the\n- * \"gently\" form.\n+ * resulting credential will be empty and the function will return an\n+ * error (even in the \"gently\" form).\n *\n * If we encounter a component which cannot be represented as a credential\n * value (e.g., because it contains a newline), the \"gently\" form will return\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397039","messageId":"20200505013908.4596-1-carenas@gmail.com","threadId":"53382","inReplyTo":"20200503063423.83152-1-carenas@gmail.com","subject":"[PATCH 0/4] credential: documentation updates for maint","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T01:39:04Z","receivedAt":"2020-05-05T01:39:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"in line with Jonathan's wishlist for a 2.26.3[1] release, the\nfollowing changes address minimal code changes related to the latest\nupdates as well to documentation changes that would guide helpers to\nadjust to the updated credential subsystem.\n\n[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/\n\nCarlo Marcelo Arenas Belón (4):\n credential: update description for credential_from_url_gently\n credential: correct order of parameters for credential_match\n credential: update gitcredentials documentation\n credential: document protocol updates\n\n Documentation/git-credential.txt | 20 ++++++++++++--------\n Documentation/gitcredentials.txt | 26 ++++++++++++++++++--------\n credential.h | 8 ++++----\n 3 files changed, 34 insertions(+), 20 deletions(-)\n\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397040","messageId":"20200505013908.4596-3-carenas@gmail.com","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"[PATCH 2/4] credential: correct order of parameters for credential_match","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T01:39:06Z","receivedAt":"2020-05-05T01:39:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Since the beginning in 118250728e (credential: apply helper config,\n2011-12-10), the declaration for that function used a different order\nthan the implementation.\n\nAll callers use the same order than the implementation, so update\nthe declaration in credential.h to match.\n\nReviewed-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex f51703d9ce..c0e17e3554 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -189,7 +189,7 @@ void credential_write(const struct credential *, FILE *);\n void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n-int credential_match(const struct credential *have,\n-\t\t const struct credential *want);\n+int credential_match(const struct credential *want,\n+\t\t const struct credential *have);\n \n #endif /* CREDENTIAL_H */\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397041","messageId":"20200505013908.4596-5-carenas@gmail.com","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"[PATCH 4/4] credential: document protocol updates","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T01:39:08Z","receivedAt":"2020-05-05T01:40:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Document protocol changes after CVE-2020-11008, while at it do some\nminor improvements for clarity and consitency.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/git-credential.txt | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 6f0c7ca80f..73a287e634 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -104,7 +104,7 @@ INPUT/OUTPUT FORMAT\n credential information in its standard input/output. This information\n can correspond either to keys for which `git credential` will obtain\n the login/password information (e.g. host, protocol, path), or to the\n-actual credential data to be obtained (login/password).\n+actual credential data to be obtained (username/password).\n \n The credential is split into a set of named attributes, with one\n attribute per line. Each attribute is\n@@ -123,7 +123,8 @@ Git understands the following attributes:\n \n `host`::\n \n-\tThe remote hostname for a network credential.\n+\tThe remote hostname for a network credential. This includes\n+\tthe port number if one was specified.\n \n `path`::\n \n@@ -134,7 +135,7 @@ Git understands the following attributes:\n `username`::\n \n \tThe credential's username, if we already have one (e.g., from a\n-\tURL, from the user, or from a previously run helper).\n+\tURL, the configuration, the user, or from a previously run helper).\n \n `password`::\n \n@@ -146,8 +147,11 @@ Git understands the following attributes:\n \tvalue is parsed as a URL and treated as if its constituent parts\n \twere read (e.g., `url=https://example.com` would behave as if\n \t`protocol=https` and `host=example.com` had been provided). This\n-\tcan help callers avoid parsing URLs themselves. Note that any\n-\tcomponents which are missing from the URL (e.g., there is no\n-\tusername in the example above) will be set to empty; if you want\n-\tto provide a URL and override some attributes, provide the URL\n-\tattribute first, followed by any overrides.\n+\tcan help callers avoid parsing URLs themselves.\n+\n+\tNote that specifying a protocol is mandatory and if the URL\n+\ttype doesn't require a hostname (like for cert://) then an\n+\tempty (\"\") hostname will be generated.\n+\n+\tComponents which are missing from the URL (e.g., there is no\n+\tusername in the example above) will be left unset.\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397042","messageId":"20200505013908.4596-4-carenas@gmail.com","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"[PATCH 3/4] credential: update gitcredentials documentation","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-05T01:39:07Z","receivedAt":"2020-05-05T01:40:09Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Clarify the expected effect of all attributes and how the helpers\nare expected to handle them and the context where they operate.\n\nWhile at it, space the descriptions for clarity, and add a paragraph\nmentioning the early termination in the list processing of helpers,\nto complement the one about the special \"quit\" attribute.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/gitcredentials.txt | 26 ++++++++++++++++++--------\n 1 file changed, 18 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 1814d2d23c..72faadae9e 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -262,16 +262,26 @@ For a `get` operation, the helper should produce a list of attributes on\n stdout in the same format (see linkgit:git-credential[1] for common\n attributes). A helper is free to produce a subset, or even no values at\n all if it has nothing useful to provide. Any provided attributes will\n-overwrite those already known about by Git. If a helper outputs a\n-`quit` attribute with a value of `true` or `1`, no further helpers will\n-be consulted, nor will the user be prompted (if no credential has been\n-provided, the operation will then fail).\n+overwrite those already known about by Git's credential subsystem.\n+\n+While it is possible to override all attributes, well behaving helpers\n+should refrain to do so for any attributes other than username and\n+password.\n+\n+If a helper outputs a `quit` attribute with a value of `true` or `1`,\n+no further helpers will be consulted, nor will the user be prompted\n+(if no credential has been provided, the operation will then fail).\n+\n+Similarly, no more helpers will be consulted once both username and\n+password had been provided.\n \n For a `store` or `erase` operation, the helper's output is ignored.\n-If it fails to perform the requested operation, it may complain to\n-stderr to inform the user. If it does not support the requested\n-operation (e.g., a read-only store), it should silently ignore the\n-request.\n+\n+If a helper fails to perform the requested operation or needs to notify\n+the user of a potential issue, it may write to stderr.\n+\n+If it does not support the requested operation (e.g., a read-only store),\n+it should silently ignore the request.\n \n If a helper receives any other operation, it should silently ignore the\n request. This leaves room for future operations to be added (older\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397196","messageId":"20200506162153.GA1841928@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200505013908.4596-4-carenas@gmail.com","subject":"Re: [PATCH 3/4] credential: update gitcredentials documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-06T16:21:53Z","receivedAt":"2020-05-06T16:21:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 06:39:07PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> Clarify the expected effect of all attributes and how the helpers\n> are expected to handle them and the context where they operate.\n> \n> While at it, space the descriptions for clarity, and add a paragraph\n> mentioning the early termination in the list processing of helpers,\n> to complement the one about the special \"quit\" attribute.\n\nYep, these all make sense. One nit:\n\n> +While it is possible to override all attributes, well behaving helpers\n> +should refrain to do so for any attributes other than username and\n> +password.\n\n\"refrain from doing so\" would be more idiomatic English.\n\n-Peff\n"},{"id":"397197","messageId":"20200506162648.GA1275237@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200505013908.4596-5-carenas@gmail.com","subject":"Re: [PATCH 4/4] credential: document protocol updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-06T16:26:48Z","receivedAt":"2020-05-06T16:26:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 06:39:08PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> Document protocol changes after CVE-2020-11008, while at it do some\n> minor improvements for clarity and consitency.\n\ns/consitency/consistency/s\n\nI think the overall direction is good. A few small comments below.\n\n> `host`::\n> \n> -\tThe remote hostname for a network credential.\n> +\tThe remote hostname for a network credential. This includes\n> +\tthe port number if one was specified.\n\nIt might be worth giving an example of the syntax, like:\n\n the port number if one was specified (e.g., \"example.com:8088\").\n\n> @@ -146,8 +147,11 @@ Git understands the following attributes:\n> \tvalue is parsed as a URL and treated as if its constituent parts\n> \twere read (e.g., `url=https://example.com` would behave as if\n> \t`protocol=https` and `host=example.com` had been provided). This\n> -\tcan help callers avoid parsing URLs themselves. Note that any\n> -\tcomponents which are missing from the URL (e.g., there is no\n> -\tusername in the example above) will be set to empty; if you want\n> -\tto provide a URL and override some attributes, provide the URL\n> -\tattribute first, followed by any overrides.\n> +\tcan help callers avoid parsing URLs themselves.\n> +\n> +\tNote that specifying a protocol is mandatory and if the URL\n> +\ttype doesn't require a hostname (like for cert://) then an\n> +\tempty (\"\") hostname will be generated.\n\nSo we are losing the bit about overriding. I think that is OK, as we'd\nlike to avoid suggesting that is a good idea, and we may even remove the\nfeature in the future.\n\nThe word \"generated\" confused me a bit. Maybe something like:\n\n ...and if the URL doesn't specify a hostname (e.g.,\n \"cert:///path/to/file\"), the credential will contain a hostname\n attribute whose value is an empty string.\n\n> +\tComponents which are missing from the URL (e.g., there is no\n> +\tusername in the example above) will be left unset.\n\nMakes sense.\n\n-Peff\n"},{"id":"397199","messageId":"20200506162712.GB1275237@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-06T16:27:12Z","receivedAt":"2020-05-06T16:27:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 04, 2020 at 06:39:04PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> in line with Jonathan's wishlist for a 2.26.3[1] release, the\n> following changes address minimal code changes related to the latest\n> updates as well to documentation changes that would guide helpers to\n> adjust to the updated credential subsystem.\n\nI left a few small comments on 3 and 4, but overall these all look like\nimprovements to me. Thanks.\n\n-Peff\n"},{"id":"397241","messageId":"20200506214726.81854-1-carenas@gmail.com","threadId":"53382","inReplyTo":"20200505013908.4596-1-carenas@gmail.com","subject":"[PATCH v2 0/4] credential: documentation updates for maint","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T21:47:22Z","receivedAt":"2020-05-06T21:48:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"in line with Jonathan's wishlist[1] for a 2.26.3 release, the\nfollowing changes address minimal code changes related to the latest\nupdates as well to documentation changes that would guide helpers to\nadjust to the updated credential subsystem.\n\npatches 1 and 2 are unchanged, patch 3 has all suggestions by Peff\nadded and hopefully is ready; patch 4 does integrate all suggestions\nas well but adds some more changes that I thought were neccesary while\nrereading it.\n\nI think this time there are no grammatical issues (and a free online\nchecker seems to concur), but as a non native English speaker and\nineffective spell checker user, look forward to any suggestions there.\n\ncommit message has also been improved based on the feedback received.\n\nnot sure how it will look like, but still think that this is the right\ntime to clarify the issue of encoding, if only so that further changes\ncould be coordinated more effectively with helper developers (who seem\nto be mainly out of tree) so will most likely send an RFC for a 5/4\npatch that could be considered for inclusion on a reroll.\n\n[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/\n\nCarlo Marcelo Arenas Belón (4):\n credential: update description for credential_from_url_gently\n credential: correct order of parameters for credential_match\n credential: update gitcredentials documentation\n credential: document protocol updates\n\n Documentation/git-credential.txt | 34 ++++++++++++++++++++------------\n Documentation/gitcredentials.txt | 25 +++++++++++++++--------\n credential.h | 8 ++++----\n 3 files changed, 42 insertions(+), 25 deletions(-)\n\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397242","messageId":"20200506214726.81854-2-carenas@gmail.com","threadId":"53382","inReplyTo":"20200506214726.81854-1-carenas@gmail.com","subject":"[PATCH v2 1/4] credential: update description for credential_from_url_gently","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T21:47:23Z","receivedAt":"2020-05-06T21:48:40Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)\nchanges the implementation for this function to return -1 if protocol is\nmissing.\n\nUpdate blurb to match implementation.\n\nReviewed-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex d99ec42b2a..f51703d9ce 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -177,8 +177,8 @@ void credential_write(const struct credential *, FILE *);\n * Parse a url into a credential struct, replacing any existing contents.\n *\n * If the url can't be parsed (e.g., a missing \"proto://\" component), the\n- * resulting credential will be empty but we'll still return success from the\n- * \"gently\" form.\n+ * resulting credential will be empty and the function will return an\n+ * error (even in the \"gently\" form).\n *\n * If we encounter a component which cannot be represented as a credential\n * value (e.g., because it contains a newline), the \"gently\" form will return\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397243","messageId":"20200506214726.81854-3-carenas@gmail.com","threadId":"53382","inReplyTo":"20200506214726.81854-1-carenas@gmail.com","subject":"[PATCH v2 2/4] credential: correct order of parameters for credential_match","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T21:47:24Z","receivedAt":"2020-05-06T21:48:42Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Since the beginning in 118250728e (credential: apply helper config,\n2011-12-10), the declaration for that function used a different order\nthan the implementation.\n\nAll callers use the same order than the implementation, so update\nthe declaration in credential.h to match.\n\nReviewed-by: Jeff King \nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n credential.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/credential.h b/credential.h\nindex f51703d9ce..c0e17e3554 100644\n--- a/credential.h\n+++ b/credential.h\n@@ -189,7 +189,7 @@ void credential_write(const struct credential *, FILE *);\n void credential_from_url(struct credential *, const char *url);\n int credential_from_url_gently(struct credential *, const char *url, int quiet);\n \n-int credential_match(const struct credential *have,\n-\t\t const struct credential *want);\n+int credential_match(const struct credential *want,\n+\t\t const struct credential *have);\n \n #endif /* CREDENTIAL_H */\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397244","messageId":"20200506214726.81854-4-carenas@gmail.com","threadId":"53382","inReplyTo":"20200506214726.81854-1-carenas@gmail.com","subject":"[PATCH v2 3/4] credential: update gitcredentials documentation","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T21:47:25Z","receivedAt":"2020-05-06T21:48:45Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Clarify the expected effect of all attributes and how the helpers\nare expected to handle them and the context where they operate.\n\nWhile at it, space the descriptions for clarity, and add a paragraph\nmentioning the early termination in the list processing of helpers,\nto complement the one about the special \"quit\" attribute.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/gitcredentials.txt | 25 +++++++++++++++++--------\n 1 file changed, 17 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt\nindex 1814d2d23c..05e544ee49 100644\n--- a/Documentation/gitcredentials.txt\n+++ b/Documentation/gitcredentials.txt\n@@ -262,16 +262,25 @@ For a `get` operation, the helper should produce a list of attributes on\n stdout in the same format (see linkgit:git-credential[1] for common\n attributes). A helper is free to produce a subset, or even no values at\n all if it has nothing useful to provide. Any provided attributes will\n-overwrite those already known about by Git. If a helper outputs a\n-`quit` attribute with a value of `true` or `1`, no further helpers will\n-be consulted, nor will the user be prompted (if no credential has been\n-provided, the operation will then fail).\n+overwrite those already known about by Git's credential subsystem.\n+\n+While it is possible to override all attributes, well behaving helpers\n+should refrain from doing so for anyone other than username and password.\n+\n+If a helper outputs a `quit` attribute with a value of `true` or `1`,\n+no further helpers will be consulted, nor will the user be prompted\n+(if no credential has been provided, the operation will then fail).\n+\n+Similarly, no more helpers will be consulted once both username and\n+password had been provided.\n \n For a `store` or `erase` operation, the helper's output is ignored.\n-If it fails to perform the requested operation, it may complain to\n-stderr to inform the user. If it does not support the requested\n-operation (e.g., a read-only store), it should silently ignore the\n-request.\n+\n+If a helper fails to perform the requested operation or needs to notify\n+the user of a potential issue, it may write to stderr.\n+\n+If it does not support the requested operation (e.g., a read-only store),\n+it should silently ignore the request.\n \n If a helper receives any other operation, it should silently ignore the\n request. This leaves room for future operations to be added (older\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397245","messageId":"20200506214726.81854-5-carenas@gmail.com","threadId":"53382","inReplyTo":"20200506214726.81854-1-carenas@gmail.com","subject":"[PATCH v2 4/4] credential: document protocol updates","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T21:47:26Z","receivedAt":"2020-05-06T21:48:46Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Document protocol changes after CVE-2020-11008, including the removal of\nreferences to the override of attributes which is no longer recommended\nafter CVE-2020-5260 and that might be removed in the future.\n\nWhile at it do some improvements for clarity and consistency.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/git-credential.txt | 34 ++++++++++++++++++++------------\n 1 file changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 6f0c7ca80f..8d990e92fd 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -103,17 +103,20 @@ INPUT/OUTPUT FORMAT\n `git credential` reads and/or writes (depending on the action used)\n credential information in its standard input/output. This information\n can correspond either to keys for which `git credential` will obtain\n-the login/password information (e.g. host, protocol, path), or to the\n-actual credential data to be obtained (login/password).\n+the login information (e.g. host, protocol, path), or to the actual\n+credential data to be obtained (username/password).\n \n The credential is split into a set of named attributes, with one\n-attribute per line. Each attribute is\n-specified by a key-value pair, separated by an `=` (equals) sign,\n-followed by a newline. The key may contain any bytes except `=`,\n-newline, or NUL. The value may contain any bytes except newline or NUL.\n+attribute per line. Each attribute is specified by a key-value pair,\n+separated by an `=` (equals) sign, followed by a newline.\n+\n+The key may contain any bytes except `=`, newline, or NUL. The value may\n+contain any bytes except newline or NUL.\n+\n In both cases, all bytes are treated as-is (i.e., there is no quoting,\n and one cannot transmit a value with newline or NUL in it). The list of\n attributes is terminated by a blank line or end-of-file.\n+\n Git understands the following attributes:\n \n `protocol`::\n@@ -123,7 +126,8 @@ Git understands the following attributes:\n \n `host`::\n \n-\tThe remote hostname for a network credential.\n+\tThe remote hostname for a network credential. This includes\n+\tthe port number if one was specified (e.g., \"example.com:8088\").\n \n `path`::\n \n@@ -134,7 +138,7 @@ Git understands the following attributes:\n `username`::\n \n \tThe credential's username, if we already have one (e.g., from a\n-\tURL, from the user, or from a previously run helper).\n+\tURL, the configuration, the user, or from a previously run helper).\n \n `password`::\n \n@@ -146,8 +150,12 @@ Git understands the following attributes:\n \tvalue is parsed as a URL and treated as if its constituent parts\n \twere read (e.g., `url=https://example.com` would behave as if\n \t`protocol=https` and `host=example.com` had been provided). This\n-\tcan help callers avoid parsing URLs themselves. Note that any\n-\tcomponents which are missing from the URL (e.g., there is no\n-\tusername in the example above) will be set to empty; if you want\n-\tto provide a URL and override some attributes, provide the URL\n-\tattribute first, followed by any overrides.\n+\tcan help callers avoid parsing URLs themselves.\n+\n+\tNote that specifying a protocol is mandatory and if the URL\n+\tdoesn't specify a hostname (e.g., \"cert:///path/to/file\") the\n+\tcredential will contain a hostname attribute whose value is an\n+\tempty string.\n+\n+\tComponents which are missing from the URL (e.g., there is no\n+\tusername in the example above) will be left unset.\n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397251","messageId":"20200506232848.GB75901@Carlos-MBP","threadId":"53382","inReplyTo":"20200506162712.GB1275237@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-06T23:28:48Z","receivedAt":"2020-05-06T23:28:53Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Peff,\n\nthanks for the feedback. as I mentioned in the cover letter for the reroll\n(which I am hoping you could Reviewed-by fully), I am including this extra\npatch on top that tries to clarify the format of the values, so that helper\ndevelopers could adjust their implementations as needed.\n\nCarlo\n--- >8 ---\nSubject: [RFC PATCH 5/4] credential: document encoding assumptions for values\n\nBecause of the similarity on the names of the keys with what is defined\nin RFC3986 is easy to assume the same rules would apply here.\n\nMake sure that the format and encoding is well defined to avoid helper\ndevelopers assuming incorrectly.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \n---\n Documentation/git-credential.txt | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt\nindex 8d990e92fd..d72e85c874 100644\n--- a/Documentation/git-credential.txt\n+++ b/Documentation/git-credential.txt\n@@ -113,9 +113,16 @@ separated by an `=` (equals) sign, followed by a newline.\n The key may contain any bytes except `=`, newline, or NUL. The value may\n contain any bytes except newline or NUL.\n \n-In both cases, all bytes are treated as-is (i.e., there is no quoting,\n-and one cannot transmit a value with newline or NUL in it). The list of\n-attributes is terminated by a blank line or end-of-file.\n+In both cases, all bytes are treated AS-IS (i.e., there is no quoting,\n+and one cannot transmit a value with newline or NUL in it).\n+\n+Values start inmediately after the '=' separator and could consist of 0\n+or more bytes until the required newline delimiter. No assumptions of case\n+insensitivity can be made on their contents and if a specific encoding is\n+required (e.g. \"UTF-8\") then the byte contents should be re-encoded before\n+use.\n+\n+The list of attributes is terminated by a blank line or end-of-file.\n \n Git understands the following attributes:\n \n-- \n2.26.2.686.gfaf46a9ccd\n\n"},{"id":"397353","messageId":"20200507205435.GA39802@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200506214726.81854-4-carenas@gmail.com","subject":"Re: [PATCH v2 3/4] credential: update gitcredentials documentation","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:54:35Z","receivedAt":"2020-05-07T20:54:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 06, 2020 at 02:47:25PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> +While it is possible to override all attributes, well behaving helpers\n> +should refrain from doing so for anyone other than username and password.\n\nThe earlier versions said \"any attributes other than...\" which I think\nis a bit more precise than \"anyone\".\n\nOther than that, this looks good to me.\n\n-Peff\n"},{"id":"397354","messageId":"20200507205752.GB39802@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200506214726.81854-5-carenas@gmail.com","subject":"Re: [PATCH v2 4/4] credential: document protocol updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:57:52Z","receivedAt":"2020-05-07T20:57:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 06, 2020 at 02:47:26PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> Document protocol changes after CVE-2020-11008, including the removal of\n> references to the override of attributes which is no longer recommended\n> after CVE-2020-5260 and that might be removed in the future.\n> \n> While at it do some improvements for clarity and consistency.\n\nThanks, this version looks good.\n\n-Peff\n"},{"id":"397355","messageId":"20200507205909.GA38308@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200506232848.GB75901@Carlos-MBP","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T20:59:09Z","receivedAt":"2020-05-07T20:59:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values\n> \n> Because of the similarity on the names of the keys with what is defined\n> in RFC3986 is easy to assume the same rules would apply here.\n> \n> Make sure that the format and encoding is well defined to avoid helper\n> developers assuming incorrectly.\n\nI'm not sure this really clarifies anything, because it just says \"no\nassumptions can be made\". Which I guess is a statement, but I'm not sure\nwhat I'd do with it as a helper developer.\n\n-Peff\n"},{"id":"397357","messageId":"xmqqsggb31d1.fsf@gitster.c.googlers.com","threadId":"53382","inReplyTo":"20200507205435.GA39802@coredump.intra.peff.net","subject":"Re: [PATCH v2 3/4] credential: update gitcredentials documentation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-07T21:02:18Z","receivedAt":"2020-05-07T21:02:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Wed, May 06, 2020 at 02:47:25PM -0700, Carlo Marcelo Arenas Belón wrote:\n>\n>> +While it is possible to override all attributes, well behaving helpers\n>> +should refrain from doing so for anyone other than username and password.\n>\n> The earlier versions said \"any attributes other than...\" which I think\n> is a bit more precise than \"anyone\".\n>\n> Other than that, this looks good to me.\n\nWill locally tweak; thanks.\n"},{"id":"397359","messageId":"20200507212348.GA19917@Carlos-MBP","threadId":"53382","inReplyTo":"20200507205909.GA38308@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-07T21:23:48Z","receivedAt":"2020-05-07T21:23:56Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:\n> On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n> \n> > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values\n> > \n> > Because of the similarity on the names of the keys with what is defined\n> > in RFC3986 is easy to assume the same rules would apply here.\n> > \n> > Make sure that the format and encoding is well defined to avoid helper\n> > developers assuming incorrectly.\n> \n> I'm not sure this really clarifies anything, because it just says \"no\n> assumptions can be made\". Which I guess is a statement, but I'm not sure\n> what I'd do with it as a helper developer.\n\nnot sure what part of the added lines you are referring to but I am happy\nto provide some examples of what I would expect to clarify below from\nwhat I'd seen from some helpers that I'd read the code from recently.\n\nas an example, I would expect the helper developer to start checking for\nthe locale and calling iconv in cases where it is not using utf-8, before\nsending it to a storage that requires that (ex: osxkeychain), or utf-16\n(maybe in windows).\n\nosxkeychain will probably also check for protocol in a case insensitive\nway to make sure it is not ignoring credentials that are not all lowercase\nas it does now.\n\nCarlo\n"},{"id":"397363","messageId":"20200507221748.GA41996@coredump.intra.peff.net","threadId":"53382","inReplyTo":"20200507212348.GA19917@Carlos-MBP","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-07T22:17:48Z","receivedAt":"2020-05-07T22:24:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 07, 2020 at 02:23:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n\n> On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:\n> > On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n> > \n> > > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values\n> > > \n> > > Because of the similarity on the names of the keys with what is defined\n> > > in RFC3986 is easy to assume the same rules would apply here.\n> > > \n> > > Make sure that the format and encoding is well defined to avoid helper\n> > > developers assuming incorrectly.\n> > \n> > I'm not sure this really clarifies anything, because it just says \"no\n> > assumptions can be made\". Which I guess is a statement, but I'm not sure\n> > what I'd do with it as a helper developer.\n> \n> not sure what part of the added lines you are referring to but I am happy\n> to provide some examples of what I would expect to clarify below from\n> what I'd seen from some helpers that I'd read the code from recently.\n> \n> as an example, I would expect the helper developer to start checking for\n> the locale and calling iconv in cases where it is not using utf-8, before\n> sending it to a storage that requires that (ex: osxkeychain), or utf-16\n> (maybe in windows).\n> \n> osxkeychain will probably also check for protocol in a case insensitive\n> way to make sure it is not ignoring credentials that are not all lowercase\n> as it does now.\n\nThose things all seem reasonable. I just meant that reading:\n\n No assumptions of case insensitivity can be made on their contents and\n if a specific encoding is required (e.g. \"UTF-8\") then the byte\n contents should be re-encoded before use.\n\ndidn't point me in a useful direction there. Reading it again, I'm still\nnot sure if you're trying to say that helpers should match protocols\ncase-insensitively or not. And TBH, I don't think it matters that much.\nIt's a quality-of-implementation issue for helpers, and if nobody is\ncomplaining about their behavior, does it really matter? I'd be more\nconcerned if doing the wrong thing involved a security vulnerability,\nbut the worst case with case-insensitivity is probably that they _fail_\nto match a credential when they should.\n\nLikewise for weird encodings, unless an attacker can somehow come up\nwith a hostname byte sequence that a helper mistakes for another\nlegitimate hostname, _and_ that can be used sensibly by git or curl.\n\n-Peff\n"},{"id":"397376","messageId":"20200507233546.GB19917@Carlos-MBP","threadId":"53382","inReplyTo":"20200507221748.GA41996@coredump.intra.peff.net","subject":"Re: [PATCH 0/4] credential: documentation updates for maint","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2020-05-07T23:35:46Z","receivedAt":"2020-05-07T23:35:51Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Thu, May 07, 2020 at 06:17:48PM -0400, Jeff King wrote:\n> On Thu, May 07, 2020 at 02:23:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n> > On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:\n> > > On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:\n> > > \n> > > > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values\n> > > > \n> > > > Because of the similarity on the names of the keys with what is defined\n> > > > in RFC3986 is easy to assume the same rules would apply here.\n> > > > \n> > > > Make sure that the format and encoding is well defined to avoid helper\n> > > > developers assuming incorrectly.\n> > > \n> > > I'm not sure this really clarifies anything, because it just says \"no\n> > > assumptions can be made\". Which I guess is a statement, but I'm not sure\n> > > what I'd do with it as a helper developer.\n> > \n> > not sure what part of the added lines you are referring to but I am happy\n> > to provide some examples of what I would expect to clarify below from\n> > what I'd seen from some helpers that I'd read the code from recently.\n> > \n> > as an example, I would expect the helper developer to start checking for\n> > the locale and calling iconv in cases where it is not using utf-8, before\n> > sending it to a storage that requires that (ex: osxkeychain), or utf-16\n> > (maybe in windows).\n> > \n> > osxkeychain will probably also check for protocol in a case insensitive\n> > way to make sure it is not ignoring credentials that are not all lowercase\n> > as it does now.\n> \n> Those things all seem reasonable. I just meant that reading:\n> \n> No assumptions of case insensitivity can be made on their contents and\n> if a specific encoding is required (e.g. \"UTF-8\") then the byte\n> contents should be re-encoded before use.\n> \n> didn't point me in a useful direction there. Reading it again, I'm still\n> not sure if you're trying to say that helpers should match protocols\n> case-insensitively or not.\n\nWhat I am trying to clarify is that the URL or parameters they were passed\nwere not normalized (as it is usually expected for internal use as per\nRFC3986), and therefore they can't just do a lowercase check for protocol\nhoping it will match regardless of what was input by the user.\n\nthey could decide after that to treat credentials for HTTP differently\nthan the ones for http (like store does) or not, but at least now they\nknow what to expect.\n\nthen again, writing specs is not my forte, so if there is a better way to\nexpress that I am happy to see an alternative.\n\n> And TBH, I don't think it matters that much.\n> It's a quality-of-implementation issue for helpers, and if nobody is\n> complaining about their behavior, does it really matter?\n\nit matters in principle; deep down any inconsistencies on behaviour\nwould eventually lead to bugs, and some of them (like the ones that\nwere raised recently) could have security implications.\n\none way we could help avoid inconsistent behaviour is by having a spec\nthat while allowing for a flexible implementation makes sure there is\nno ambiguity that could result in mismatching interpretations.\n\n> I'd be more\n> concerned if doing the wrong thing involved a security vulnerability,\n> but the worst case with case-insensitivity is probably that they _fail_\n> to match a credential when they should.\n\nI don't think any of these suggestions is directly related to a security\nrisk, eventhough as explained earlier has the longterm effect to reduce\nthe risk of one.\n\nit is also not a final word, as usually having these controls implemented\nmultiple times is also a risk, and will be better IMHO longterm to do\nthe normalization once and get rid of them, but leaving them undefined\nis IMHO a lost opportunity until then.\n\nCarlo\n"}]}