* [PATCH 1/4] credential: update description for credential_from_url_gently
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
@ 2020-05-05 1:39 ` Carlo Marcelo Arenas Belón
2020-05-05 1:39 ` [PATCH 2/4] credential: correct order of parameters for credential_match Carlo Marcelo Arenas Belón
` (4 subsequent siblings)
5 siblings, 0 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-05 1:39 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)
changes the implementation for this function to return -1 if protocol is
missing.
Update blurb to match implementation.
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
credential.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/credential.h b/credential.h
index d99ec42b2a..f51703d9ce 100644
--- a/credential.h
+++ b/credential.h
@@ -177,8 +177,8 @@ void credential_write(const struct credential *, FILE *);
* Parse a url into a credential struct, replacing any existing contents.
*
* If the url can't be parsed (e.g., a missing "proto://" component), the
- * resulting credential will be empty but we'll still return success from the
- * "gently" form.
+ * resulting credential will be empty and the function will return an
+ * error (even in the "gently" form).
*
* If we encounter a component which cannot be represented as a credential
* value (e.g., because it contains a newline), the "gently" form will return
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 2/4] credential: correct order of parameters for credential_match
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
2020-05-05 1:39 ` [PATCH 1/4] credential: update description for credential_from_url_gently Carlo Marcelo Arenas Belón
@ 2020-05-05 1:39 ` Carlo Marcelo Arenas Belón
2020-05-05 1:39 ` [PATCH 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
` (3 subsequent siblings)
5 siblings, 0 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-05 1:39 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Since the beginning in 118250728e (credential: apply helper config,
2011-12-10), the declaration for that function used a different order
than the implementation.
All callers use the same order than the implementation, so update
the declaration in credential.h to match.
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
credential.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/credential.h b/credential.h
index f51703d9ce..c0e17e3554 100644
--- a/credential.h
+++ b/credential.h
@@ -189,7 +189,7 @@ void credential_write(const struct credential *, FILE *);
void credential_from_url(struct credential *, const char *url);
int credential_from_url_gently(struct credential *, const char *url, int quiet);
-int credential_match(const struct credential *have,
- const struct credential *want);
+int credential_match(const struct credential *want,
+ const struct credential *have);
#endif /* CREDENTIAL_H */
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH 3/4] credential: update gitcredentials documentation
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
2020-05-05 1:39 ` [PATCH 1/4] credential: update description for credential_from_url_gently Carlo Marcelo Arenas Belón
2020-05-05 1:39 ` [PATCH 2/4] credential: correct order of parameters for credential_match Carlo Marcelo Arenas Belón
@ 2020-05-05 1:39 ` Carlo Marcelo Arenas Belón
2020-05-06 16:21 ` Jeff King
2020-05-05 1:39 ` [PATCH 4/4] credential: document protocol updates Carlo Marcelo Arenas Belón
` (2 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-05 1:39 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Clarify the expected effect of all attributes and how the helpers
are expected to handle them and the context where they operate.
While at it, space the descriptions for clarity, and add a paragraph
mentioning the early termination in the list processing of helpers,
to complement the one about the special "quit" attribute.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Documentation/gitcredentials.txt | 26 ++++++++++++++++++--------
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 1814d2d23c..72faadae9e 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -262,16 +262,26 @@ For a `get` operation, the helper should produce a list of attributes on
stdout in the same format (see linkgit:git-credential[1] for common
attributes). A helper is free to produce a subset, or even no values at
all if it has nothing useful to provide. Any provided attributes will
-overwrite those already known about by Git. If a helper outputs a
-`quit` attribute with a value of `true` or `1`, no further helpers will
-be consulted, nor will the user be prompted (if no credential has been
-provided, the operation will then fail).
+overwrite those already known about by Git's credential subsystem.
+
+While it is possible to override all attributes, well behaving helpers
+should refrain to do so for any attributes other than username and
+password.
+
+If a helper outputs a `quit` attribute with a value of `true` or `1`,
+no further helpers will be consulted, nor will the user be prompted
+(if no credential has been provided, the operation will then fail).
+
+Similarly, no more helpers will be consulted once both username and
+password had been provided.
For a `store` or `erase` operation, the helper's output is ignored.
-If it fails to perform the requested operation, it may complain to
-stderr to inform the user. If it does not support the requested
-operation (e.g., a read-only store), it should silently ignore the
-request.
+
+If a helper fails to perform the requested operation or needs to notify
+the user of a potential issue, it may write to stderr.
+
+If it does not support the requested operation (e.g., a read-only store),
+it should silently ignore the request.
If a helper receives any other operation, it should silently ignore the
request. This leaves room for future operations to be added (older
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 3/4] credential: update gitcredentials documentation
2020-05-05 1:39 ` [PATCH 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
@ 2020-05-06 16:21 ` Jeff King
0 siblings, 0 replies; 29+ messages in thread
From: Jeff King @ 2020-05-06 16:21 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Mon, May 04, 2020 at 06:39:07PM -0700, Carlo Marcelo Arenas Belón wrote:
> Clarify the expected effect of all attributes and how the helpers
> are expected to handle them and the context where they operate.
>
> While at it, space the descriptions for clarity, and add a paragraph
> mentioning the early termination in the list processing of helpers,
> to complement the one about the special "quit" attribute.
Yep, these all make sense. One nit:
> +While it is possible to override all attributes, well behaving helpers
> +should refrain to do so for any attributes other than username and
> +password.
"refrain from doing so" would be more idiomatic English.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 4/4] credential: document protocol updates
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
` (2 preceding siblings ...)
2020-05-05 1:39 ` [PATCH 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
@ 2020-05-05 1:39 ` Carlo Marcelo Arenas Belón
2020-05-06 16:26 ` Jeff King
2020-05-06 16:27 ` [PATCH 0/4] credential: documentation updates for maint Jeff King
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
5 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-05 1:39 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Document protocol changes after CVE-2020-11008, while at it do some
minor improvements for clarity and consitency.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Documentation/git-credential.txt | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 6f0c7ca80f..73a287e634 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -104,7 +104,7 @@ INPUT/OUTPUT FORMAT
credential information in its standard input/output. This information
can correspond either to keys for which `git credential` will obtain
the login/password information (e.g. host, protocol, path), or to the
-actual credential data to be obtained (login/password).
+actual credential data to be obtained (username/password).
The credential is split into a set of named attributes, with one
attribute per line. Each attribute is
@@ -123,7 +123,8 @@ Git understands the following attributes:
`host`::
- The remote hostname for a network credential.
+ The remote hostname for a network credential. This includes
+ the port number if one was specified.
`path`::
@@ -134,7 +135,7 @@ Git understands the following attributes:
`username`::
The credential's username, if we already have one (e.g., from a
- URL, from the user, or from a previously run helper).
+ URL, the configuration, the user, or from a previously run helper).
`password`::
@@ -146,8 +147,11 @@ Git understands the following attributes:
value is parsed as a URL and treated as if its constituent parts
were read (e.g., `url=https://example.com` would behave as if
`protocol=https` and `host=example.com` had been provided). This
- can help callers avoid parsing URLs themselves. Note that any
- components which are missing from the URL (e.g., there is no
- username in the example above) will be set to empty; if you want
- to provide a URL and override some attributes, provide the URL
- attribute first, followed by any overrides.
+ can help callers avoid parsing URLs themselves.
+
+ Note that specifying a protocol is mandatory and if the URL
+ type doesn't require a hostname (like for cert://) then an
+ empty ("") hostname will be generated.
+
+ Components which are missing from the URL (e.g., there is no
+ username in the example above) will be left unset.
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 4/4] credential: document protocol updates
2020-05-05 1:39 ` [PATCH 4/4] credential: document protocol updates Carlo Marcelo Arenas Belón
@ 2020-05-06 16:26 ` Jeff King
0 siblings, 0 replies; 29+ messages in thread
From: Jeff King @ 2020-05-06 16:26 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Mon, May 04, 2020 at 06:39:08PM -0700, Carlo Marcelo Arenas Belón wrote:
> Document protocol changes after CVE-2020-11008, while at it do some
> minor improvements for clarity and consitency.
s/consitency/consistency/s
I think the overall direction is good. A few small comments below.
> `host`::
>
> - The remote hostname for a network credential.
> + The remote hostname for a network credential. This includes
> + the port number if one was specified.
It might be worth giving an example of the syntax, like:
the port number if one was specified (e.g., "example.com:8088").
> @@ -146,8 +147,11 @@ Git understands the following attributes:
> value is parsed as a URL and treated as if its constituent parts
> were read (e.g., `url=https://example.com` would behave as if
> `protocol=https` and `host=example.com` had been provided). This
> - can help callers avoid parsing URLs themselves. Note that any
> - components which are missing from the URL (e.g., there is no
> - username in the example above) will be set to empty; if you want
> - to provide a URL and override some attributes, provide the URL
> - attribute first, followed by any overrides.
> + can help callers avoid parsing URLs themselves.
> +
> + Note that specifying a protocol is mandatory and if the URL
> + type doesn't require a hostname (like for cert://) then an
> + empty ("") hostname will be generated.
So we are losing the bit about overriding. I think that is OK, as we'd
like to avoid suggesting that is a good idea, and we may even remove the
feature in the future.
The word "generated" confused me a bit. Maybe something like:
...and if the URL doesn't specify a hostname (e.g.,
"cert:///path/to/file"), the credential will contain a hostname
attribute whose value is an empty string.
> + Components which are missing from the URL (e.g., there is no
> + username in the example above) will be left unset.
Makes sense.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
` (3 preceding siblings ...)
2020-05-05 1:39 ` [PATCH 4/4] credential: document protocol updates Carlo Marcelo Arenas Belón
@ 2020-05-06 16:27 ` Jeff King
2020-05-06 23:28 ` Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
5 siblings, 1 reply; 29+ messages in thread
From: Jeff King @ 2020-05-06 16:27 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Mon, May 04, 2020 at 06:39:04PM -0700, Carlo Marcelo Arenas Belón wrote:
> in line with Jonathan's wishlist for a 2.26.3[1] release, the
> following changes address minimal code changes related to the latest
> updates as well to documentation changes that would guide helpers to
> adjust to the updated credential subsystem.
I left a few small comments on 3 and 4, but overall these all look like
improvements to me. Thanks.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-06 16:27 ` [PATCH 0/4] credential: documentation updates for maint Jeff King
@ 2020-05-06 23:28 ` Carlo Marcelo Arenas Belón
2020-05-07 20:59 ` Jeff King
0 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 23:28 UTC (permalink / raw)
To: Jeff King; +Cc: git, jrnieder
Peff,
thanks for the feedback. as I mentioned in the cover letter for the reroll
(which I am hoping you could Reviewed-by fully), I am including this extra
patch on top that tries to clarify the format of the values, so that helper
developers could adjust their implementations as needed.
Carlo
--- >8 ---
Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values
Because of the similarity on the names of the keys with what is defined
in RFC3986 is easy to assume the same rules would apply here.
Make sure that the format and encoding is well defined to avoid helper
developers assuming incorrectly.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Documentation/git-credential.txt | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 8d990e92fd..d72e85c874 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -113,9 +113,16 @@ separated by an `=` (equals) sign, followed by a newline.
The key may contain any bytes except `=`, newline, or NUL. The value may
contain any bytes except newline or NUL.
-In both cases, all bytes are treated as-is (i.e., there is no quoting,
-and one cannot transmit a value with newline or NUL in it). The list of
-attributes is terminated by a blank line or end-of-file.
+In both cases, all bytes are treated AS-IS (i.e., there is no quoting,
+and one cannot transmit a value with newline or NUL in it).
+
+Values start inmediately after the '=' separator and could consist of 0
+or more bytes until the required newline delimiter. No assumptions of case
+insensitivity can be made on their contents and if a specific encoding is
+required (e.g. "UTF-8") then the byte contents should be re-encoded before
+use.
+
+The list of attributes is terminated by a blank line or end-of-file.
Git understands the following attributes:
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-06 23:28 ` Carlo Marcelo Arenas Belón
@ 2020-05-07 20:59 ` Jeff King
2020-05-07 21:23 ` Carlo Marcelo Arenas Belón
0 siblings, 1 reply; 29+ messages in thread
From: Jeff King @ 2020-05-07 20:59 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:
> Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values
>
> Because of the similarity on the names of the keys with what is defined
> in RFC3986 is easy to assume the same rules would apply here.
>
> Make sure that the format and encoding is well defined to avoid helper
> developers assuming incorrectly.
I'm not sure this really clarifies anything, because it just says "no
assumptions can be made". Which I guess is a statement, but I'm not sure
what I'd do with it as a helper developer.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-07 20:59 ` Jeff King
@ 2020-05-07 21:23 ` Carlo Marcelo Arenas Belón
2020-05-07 22:17 ` Jeff King
0 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-07 21:23 UTC (permalink / raw)
To: Jeff King; +Cc: git, jrnieder
On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:
> On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:
>
> > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values
> >
> > Because of the similarity on the names of the keys with what is defined
> > in RFC3986 is easy to assume the same rules would apply here.
> >
> > Make sure that the format and encoding is well defined to avoid helper
> > developers assuming incorrectly.
>
> I'm not sure this really clarifies anything, because it just says "no
> assumptions can be made". Which I guess is a statement, but I'm not sure
> what I'd do with it as a helper developer.
not sure what part of the added lines you are referring to but I am happy
to provide some examples of what I would expect to clarify below from
what I'd seen from some helpers that I'd read the code from recently.
as an example, I would expect the helper developer to start checking for
the locale and calling iconv in cases where it is not using utf-8, before
sending it to a storage that requires that (ex: osxkeychain), or utf-16
(maybe in windows).
osxkeychain will probably also check for protocol in a case insensitive
way to make sure it is not ignoring credentials that are not all lowercase
as it does now.
Carlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-07 21:23 ` Carlo Marcelo Arenas Belón
@ 2020-05-07 22:17 ` Jeff King
2020-05-07 23:35 ` Carlo Marcelo Arenas Belón
0 siblings, 1 reply; 29+ messages in thread
From: Jeff King @ 2020-05-07 22:17 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Thu, May 07, 2020 at 02:23:48PM -0700, Carlo Marcelo Arenas Belón wrote:
> On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:
> > On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:
> >
> > > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values
> > >
> > > Because of the similarity on the names of the keys with what is defined
> > > in RFC3986 is easy to assume the same rules would apply here.
> > >
> > > Make sure that the format and encoding is well defined to avoid helper
> > > developers assuming incorrectly.
> >
> > I'm not sure this really clarifies anything, because it just says "no
> > assumptions can be made". Which I guess is a statement, but I'm not sure
> > what I'd do with it as a helper developer.
>
> not sure what part of the added lines you are referring to but I am happy
> to provide some examples of what I would expect to clarify below from
> what I'd seen from some helpers that I'd read the code from recently.
>
> as an example, I would expect the helper developer to start checking for
> the locale and calling iconv in cases where it is not using utf-8, before
> sending it to a storage that requires that (ex: osxkeychain), or utf-16
> (maybe in windows).
>
> osxkeychain will probably also check for protocol in a case insensitive
> way to make sure it is not ignoring credentials that are not all lowercase
> as it does now.
Those things all seem reasonable. I just meant that reading:
No assumptions of case insensitivity can be made on their contents and
if a specific encoding is required (e.g. "UTF-8") then the byte
contents should be re-encoded before use.
didn't point me in a useful direction there. Reading it again, I'm still
not sure if you're trying to say that helpers should match protocols
case-insensitively or not. And TBH, I don't think it matters that much.
It's a quality-of-implementation issue for helpers, and if nobody is
complaining about their behavior, does it really matter? I'd be more
concerned if doing the wrong thing involved a security vulnerability,
but the worst case with case-insensitivity is probably that they _fail_
to match a credential when they should.
Likewise for weird encodings, unless an attacker can somehow come up
with a hostname byte sequence that a helper mistakes for another
legitimate hostname, _and_ that can be used sensibly by git or curl.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH 0/4] credential: documentation updates for maint
2020-05-07 22:17 ` Jeff King
@ 2020-05-07 23:35 ` Carlo Marcelo Arenas Belón
0 siblings, 0 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-07 23:35 UTC (permalink / raw)
To: Jeff King; +Cc: git, jrnieder
On Thu, May 07, 2020 at 06:17:48PM -0400, Jeff King wrote:
> On Thu, May 07, 2020 at 02:23:48PM -0700, Carlo Marcelo Arenas Belón wrote:
> > On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:
> > > On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:
> > >
> > > > Subject: [RFC PATCH 5/4] credential: document encoding assumptions for values
> > > >
> > > > Because of the similarity on the names of the keys with what is defined
> > > > in RFC3986 is easy to assume the same rules would apply here.
> > > >
> > > > Make sure that the format and encoding is well defined to avoid helper
> > > > developers assuming incorrectly.
> > >
> > > I'm not sure this really clarifies anything, because it just says "no
> > > assumptions can be made". Which I guess is a statement, but I'm not sure
> > > what I'd do with it as a helper developer.
> >
> > not sure what part of the added lines you are referring to but I am happy
> > to provide some examples of what I would expect to clarify below from
> > what I'd seen from some helpers that I'd read the code from recently.
> >
> > as an example, I would expect the helper developer to start checking for
> > the locale and calling iconv in cases where it is not using utf-8, before
> > sending it to a storage that requires that (ex: osxkeychain), or utf-16
> > (maybe in windows).
> >
> > osxkeychain will probably also check for protocol in a case insensitive
> > way to make sure it is not ignoring credentials that are not all lowercase
> > as it does now.
>
> Those things all seem reasonable. I just meant that reading:
>
> No assumptions of case insensitivity can be made on their contents and
> if a specific encoding is required (e.g. "UTF-8") then the byte
> contents should be re-encoded before use.
>
> didn't point me in a useful direction there. Reading it again, I'm still
> not sure if you're trying to say that helpers should match protocols
> case-insensitively or not.
What I am trying to clarify is that the URL or parameters they were passed
were not normalized (as it is usually expected for internal use as per
RFC3986), and therefore they can't just do a lowercase check for protocol
hoping it will match regardless of what was input by the user.
they could decide after that to treat credentials for HTTP differently
than the ones for http (like store does) or not, but at least now they
know what to expect.
then again, writing specs is not my forte, so if there is a better way to
express that I am happy to see an alternative.
> And TBH, I don't think it matters that much.
> It's a quality-of-implementation issue for helpers, and if nobody is
> complaining about their behavior, does it really matter?
it matters in principle; deep down any inconsistencies on behaviour
would eventually lead to bugs, and some of them (like the ones that
were raised recently) could have security implications.
one way we could help avoid inconsistent behaviour is by having a spec
that while allowing for a flexible implementation makes sure there is
no ambiguity that could result in mismatching interpretations.
> I'd be more
> concerned if doing the wrong thing involved a security vulnerability,
> but the worst case with case-insensitivity is probably that they _fail_
> to match a credential when they should.
I don't think any of these suggestions is directly related to a security
risk, eventhough as explained earlier has the longterm effect to reduce
the risk of one.
it is also not a final word, as usually having these controls implemented
multiple times is also a risk, and will be better IMHO longterm to do
the normalization once and get rid of them, but leaving them undefined
is IMHO a lost opportunity until then.
Carlo
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 0/4] credential: documentation updates for maint
2020-05-05 1:39 ` [PATCH 0/4] credential: documentation updates for maint Carlo Marcelo Arenas Belón
` (4 preceding siblings ...)
2020-05-06 16:27 ` [PATCH 0/4] credential: documentation updates for maint Jeff King
@ 2020-05-06 21:47 ` Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 1/4] credential: update description for credential_from_url_gently Carlo Marcelo Arenas Belón
` (3 more replies)
5 siblings, 4 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 21:47 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
in line with Jonathan's wishlist[1] for a 2.26.3 release, the
following changes address minimal code changes related to the latest
updates as well to documentation changes that would guide helpers to
adjust to the updated credential subsystem.
patches 1 and 2 are unchanged, patch 3 has all suggestions by Peff
added and hopefully is ready; patch 4 does integrate all suggestions
as well but adds some more changes that I thought were neccesary while
rereading it.
I think this time there are no grammatical issues (and a free online
checker seems to concur), but as a non native English speaker and
ineffective spell checker user, look forward to any suggestions there.
commit message has also been improved based on the feedback received.
not sure how it will look like, but still think that this is the right
time to clarify the issue of encoding, if only so that further changes
could be coordinated more effectively with helper developers (who seem
to be mainly out of tree) so will most likely send an RFC for a 5/4
patch that could be considered for inclusion on a reroll.
[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/
Carlo Marcelo Arenas Belón (4):
credential: update description for credential_from_url_gently
credential: correct order of parameters for credential_match
credential: update gitcredentials documentation
credential: document protocol updates
Documentation/git-credential.txt | 34 ++++++++++++++++++++------------
Documentation/gitcredentials.txt | 25 +++++++++++++++--------
credential.h | 8 ++++----
3 files changed, 42 insertions(+), 25 deletions(-)
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 1/4] credential: update description for credential_from_url_gently
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
@ 2020-05-06 21:47 ` Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 2/4] credential: correct order of parameters for credential_match Carlo Marcelo Arenas Belón
` (2 subsequent siblings)
3 siblings, 0 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 21:47 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)
changes the implementation for this function to return -1 if protocol is
missing.
Update blurb to match implementation.
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
credential.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/credential.h b/credential.h
index d99ec42b2a..f51703d9ce 100644
--- a/credential.h
+++ b/credential.h
@@ -177,8 +177,8 @@ void credential_write(const struct credential *, FILE *);
* Parse a url into a credential struct, replacing any existing contents.
*
* If the url can't be parsed (e.g., a missing "proto://" component), the
- * resulting credential will be empty but we'll still return success from the
- * "gently" form.
+ * resulting credential will be empty and the function will return an
+ * error (even in the "gently" form).
*
* If we encounter a component which cannot be represented as a credential
* value (e.g., because it contains a newline), the "gently" form will return
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 2/4] credential: correct order of parameters for credential_match
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 1/4] credential: update description for credential_from_url_gently Carlo Marcelo Arenas Belón
@ 2020-05-06 21:47 ` Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 4/4] credential: document protocol updates Carlo Marcelo Arenas Belón
3 siblings, 0 replies; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 21:47 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Since the beginning in 118250728e (credential: apply helper config,
2011-12-10), the declaration for that function used a different order
than the implementation.
All callers use the same order than the implementation, so update
the declaration in credential.h to match.
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
credential.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/credential.h b/credential.h
index f51703d9ce..c0e17e3554 100644
--- a/credential.h
+++ b/credential.h
@@ -189,7 +189,7 @@ void credential_write(const struct credential *, FILE *);
void credential_from_url(struct credential *, const char *url);
int credential_from_url_gently(struct credential *, const char *url, int quiet);
-int credential_match(const struct credential *have,
- const struct credential *want);
+int credential_match(const struct credential *want,
+ const struct credential *have);
#endif /* CREDENTIAL_H */
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v2 3/4] credential: update gitcredentials documentation
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 1/4] credential: update description for credential_from_url_gently Carlo Marcelo Arenas Belón
2020-05-06 21:47 ` [PATCH v2 2/4] credential: correct order of parameters for credential_match Carlo Marcelo Arenas Belón
@ 2020-05-06 21:47 ` Carlo Marcelo Arenas Belón
2020-05-07 20:54 ` Jeff King
2020-05-06 21:47 ` [PATCH v2 4/4] credential: document protocol updates Carlo Marcelo Arenas Belón
3 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 21:47 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Clarify the expected effect of all attributes and how the helpers
are expected to handle them and the context where they operate.
While at it, space the descriptions for clarity, and add a paragraph
mentioning the early termination in the list processing of helpers,
to complement the one about the special "quit" attribute.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Documentation/gitcredentials.txt | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/Documentation/gitcredentials.txt b/Documentation/gitcredentials.txt
index 1814d2d23c..05e544ee49 100644
--- a/Documentation/gitcredentials.txt
+++ b/Documentation/gitcredentials.txt
@@ -262,16 +262,25 @@ For a `get` operation, the helper should produce a list of attributes on
stdout in the same format (see linkgit:git-credential[1] for common
attributes). A helper is free to produce a subset, or even no values at
all if it has nothing useful to provide. Any provided attributes will
-overwrite those already known about by Git. If a helper outputs a
-`quit` attribute with a value of `true` or `1`, no further helpers will
-be consulted, nor will the user be prompted (if no credential has been
-provided, the operation will then fail).
+overwrite those already known about by Git's credential subsystem.
+
+While it is possible to override all attributes, well behaving helpers
+should refrain from doing so for anyone other than username and password.
+
+If a helper outputs a `quit` attribute with a value of `true` or `1`,
+no further helpers will be consulted, nor will the user be prompted
+(if no credential has been provided, the operation will then fail).
+
+Similarly, no more helpers will be consulted once both username and
+password had been provided.
For a `store` or `erase` operation, the helper's output is ignored.
-If it fails to perform the requested operation, it may complain to
-stderr to inform the user. If it does not support the requested
-operation (e.g., a read-only store), it should silently ignore the
-request.
+
+If a helper fails to perform the requested operation or needs to notify
+the user of a potential issue, it may write to stderr.
+
+If it does not support the requested operation (e.g., a read-only store),
+it should silently ignore the request.
If a helper receives any other operation, it should silently ignore the
request. This leaves room for future operations to be added (older
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v2 3/4] credential: update gitcredentials documentation
2020-05-06 21:47 ` [PATCH v2 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
@ 2020-05-07 20:54 ` Jeff King
2020-05-07 21:02 ` Junio C Hamano
0 siblings, 1 reply; 29+ messages in thread
From: Jeff King @ 2020-05-07 20:54 UTC (permalink / raw)
To: Carlo Marcelo Arenas Belón; +Cc: git, jrnieder
On Wed, May 06, 2020 at 02:47:25PM -0700, Carlo Marcelo Arenas Belón wrote:
> +While it is possible to override all attributes, well behaving helpers
> +should refrain from doing so for anyone other than username and password.
The earlier versions said "any attributes other than..." which I think
is a bit more precise than "anyone".
Other than that, this looks good to me.
-Peff
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v2 3/4] credential: update gitcredentials documentation
2020-05-07 20:54 ` Jeff King
@ 2020-05-07 21:02 ` Junio C Hamano
0 siblings, 0 replies; 29+ messages in thread
From: Junio C Hamano @ 2020-05-07 21:02 UTC (permalink / raw)
To: Jeff King; +Cc: Carlo Marcelo Arenas Belón, git, jrnieder
Jeff King <peff@peff.net> writes:
> On Wed, May 06, 2020 at 02:47:25PM -0700, Carlo Marcelo Arenas Belón wrote:
>
>> +While it is possible to override all attributes, well behaving helpers
>> +should refrain from doing so for anyone other than username and password.
>
> The earlier versions said "any attributes other than..." which I think
> is a bit more precise than "anyone".
>
> Other than that, this looks good to me.
Will locally tweak; thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v2 4/4] credential: document protocol updates
2020-05-06 21:47 ` [PATCH v2 " Carlo Marcelo Arenas Belón
` (2 preceding siblings ...)
2020-05-06 21:47 ` [PATCH v2 3/4] credential: update gitcredentials documentation Carlo Marcelo Arenas Belón
@ 2020-05-06 21:47 ` Carlo Marcelo Arenas Belón
2020-05-07 20:57 ` Jeff King
3 siblings, 1 reply; 29+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2020-05-06 21:47 UTC (permalink / raw)
To: git; +Cc: peff, jrnieder, Carlo Marcelo Arenas Belón
Document protocol changes after CVE-2020-11008, including the removal of
references to the override of attributes which is no longer recommended
after CVE-2020-5260 and that might be removed in the future.
While at it do some improvements for clarity and consistency.
Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
Documentation/git-credential.txt | 34 ++++++++++++++++++++------------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 6f0c7ca80f..8d990e92fd 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -103,17 +103,20 @@ INPUT/OUTPUT FORMAT
`git credential` reads and/or writes (depending on the action used)
credential information in its standard input/output. This information
can correspond either to keys for which `git credential` will obtain
-the login/password information (e.g. host, protocol, path), or to the
-actual credential data to be obtained (login/password).
+the login information (e.g. host, protocol, path), or to the actual
+credential data to be obtained (username/password).
The credential is split into a set of named attributes, with one
-attribute per line. Each attribute is
-specified by a key-value pair, separated by an `=` (equals) sign,
-followed by a newline. The key may contain any bytes except `=`,
-newline, or NUL. The value may contain any bytes except newline or NUL.
+attribute per line. Each attribute is specified by a key-value pair,
+separated by an `=` (equals) sign, followed by a newline.
+
+The key may contain any bytes except `=`, newline, or NUL. The value may
+contain any bytes except newline or NUL.
+
In both cases, all bytes are treated as-is (i.e., there is no quoting,
and one cannot transmit a value with newline or NUL in it). The list of
attributes is terminated by a blank line or end-of-file.
+
Git understands the following attributes:
`protocol`::
@@ -123,7 +126,8 @@ Git understands the following attributes:
`host`::
- The remote hostname for a network credential.
+ The remote hostname for a network credential. This includes
+ the port number if one was specified (e.g., "example.com:8088").
`path`::
@@ -134,7 +138,7 @@ Git understands the following attributes:
`username`::
The credential's username, if we already have one (e.g., from a
- URL, from the user, or from a previously run helper).
+ URL, the configuration, the user, or from a previously run helper).
`password`::
@@ -146,8 +150,12 @@ Git understands the following attributes:
value is parsed as a URL and treated as if its constituent parts
were read (e.g., `url=https://example.com` would behave as if
`protocol=https` and `host=example.com` had been provided). This
- can help callers avoid parsing URLs themselves. Note that any
- components which are missing from the URL (e.g., there is no
- username in the example above) will be set to empty; if you want
- to provide a URL and override some attributes, provide the URL
- attribute first, followed by any overrides.
+ can help callers avoid parsing URLs themselves.
+
+ Note that specifying a protocol is mandatory and if the URL
+ doesn't specify a hostname (e.g., "cert:///path/to/file") the
+ credential will contain a hostname attribute whose value is an
+ empty string.
+
+ Components which are missing from the URL (e.g., there is no
+ username in the example above) will be left unset.
--
2.26.2.686.gfaf46a9ccd
^ permalink raw reply related [flat|nested] 29+ messages in thread