[RFC PATCH] credential: minor documentation fixes

[RFC PATCH] credential: minor documentation fixes

[Carlo Marcelo Arenas Belón]

c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)
changes the implementation for creential_from_url_gently to retun -1 if
protocol is missing, but didn't update this blurb.

the order of parameters used in credential_match was inconsistent
between credential.c and credential.h as well, so update both to
match the current implementation.

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 credential.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/credential.h b/credential.h
index d99ec42b2a..c0e17e3554 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
@@ -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

Re: [RFC PATCH] credential: minor documentation fixes

[Jeff King]

On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:

> c44088ecc4 (credential: treat URL without scheme as invalid, 2020-04-18)
> changes the implementation for creential_from_url_gently to retun -1 if
> protocol is missing, but didn't update this blurb.

Yeah, this makes sense. s/creential/credential/.

> the order of parameters used in credential_match was inconsistent
> between credential.c and credential.h as well, so update both to
> match the current implementation.

Yes, looks like this has been wrong since the beginning in 118250728e
(credential: apply helper config, 2011-12-10). I checked the callers to
make sure none of them had gotten it backwards, but they all look right
(and just the declaration is wrong).

-Peff

Re: [RFC PATCH] credential: minor documentation fixes

[Carlo Marcelo Arenas Belón]

On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:
> On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:
> 
> > the order of parameters used in credential_match was inconsistent
> > between credential.c and credential.h as well, so update both to
> > match the current implementation.
> 
> Yes, looks like this has been wrong since the beginning in 118250728e
> (credential: apply helper config, 2011-12-10). I checked the callers to
> make sure none of them had gotten it backwards, but they all look right
> (and just the declaration is wrong).

thanks for checking, will include this (and your typo fix) in the
submission; should I add your "Reviewed-by" then?

was also curious about the other documentation updates mentioned[1] by
Jonathan, and that I was hoping this patch will be included with.

some things that I think might need clarification (or maybe even code changes
after agreed on) are :

* the meaning of "exactly" for matching protocol and hostname in the URL
  since 06 are both case insensitive per RFC3986 and we have been
  ambiguous on that, leading to some helpers assuming case or encoding.
* the rules for how helper matching are expected to be ordered, specially
  considering the recent adding of wildcard matching and the revival of
  partial matching, and the fact that the order is relevant for both
  discovery of credentials and which helpers are used.
* the use of hostname as a key, since the addition of cert:// that has none
  and uses path instead (had emailed the author privately for clarification,
  but hadn't heard yet) and the effect that has on which fields are expected
  and which values are valid.
* the role of overrides (ex: the documented example of passing URL and later
  updating it seems useful, eventhough I am not aware if being used)
* clarification on which fields can be updated by the helper; currently I
  don't think we allow overrides for protocol and host and assume all others
  but the documentation doesn't clarify, and that might be a problem for
  cert:// where file is more relevant.

Carlo

[1] https://lore.kernel.org/git/20200428055514.GB201501@google.com/

Re: [RFC PATCH] credential: minor documentation fixes

[Jeff King]

On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:

> On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:
> > On Sat, May 02, 2020 at 11:34:23PM -0700, Carlo Marcelo Arenas Belón wrote:
> > 
> > > the order of parameters used in credential_match was inconsistent
> > > between credential.c and credential.h as well, so update both to
> > > match the current implementation.
> > 
> > Yes, looks like this has been wrong since the beginning in 118250728e
> > (credential: apply helper config, 2011-12-10). I checked the callers to
> > make sure none of them had gotten it backwards, but they all look right
> > (and just the declaration is wrong).
> 
> thanks for checking, will include this (and your typo fix) in the
> submission; should I add your "Reviewed-by" then?

Sure, feel free to.

> some things that I think might need clarification (or maybe even code changes
> after agreed on) are :
> 
> * the meaning of "exactly" for matching protocol and hostname in the URL
>   since 06 are both case insensitive per RFC3986 and we have been
>   ambiguous on that, leading to some helpers assuming case or encoding.

Yeah, IIRC we discussed case-sensitivity at the time and went with the
stricter behavior in the name of safety over convenience. And I don't
think anybody has complained since then. So I'm not really _opposed_ to
loosening it to match the URL, but perhaps a maintenance release is not
the best time to do so.

> * the rules for how helper matching are expected to be ordered, specially
>   considering the recent adding of wildcard matching and the revival of
>   partial matching, and the fact that the order is relevant for both
>   discovery of credentials and which helpers are used.

Yes, this could be better documented. I think the current rules are
probably not ideal, especially when you mix credential.*.helper and
credential.helper. So some fixes are possible there, but I think they
might be best added as new feature (e.g., a new config like
credential.*.helperOrder that says whether and how to use
non-url-specific helpers; or something like that).

> * the use of hostname as a key, since the addition of cert:// that has none
>   and uses path instead (had emailed the author privately for clarification,
>   but hadn't heard yet) and the effect that has on which fields are expected
>   and which values are valid.

Yeah, there could be more discussion in gitcredentials(7) there.

> * the role of overrides (ex: the documented example of passing URL and later
>   updating it seems useful, eventhough I am not aware if being used)

I'd be OK to see this feature removed. I have used it for various
debugging or experimenting scenarios, but Git in general would never
pass anything except a broken-down set of fields, and only one of each
field.

> * clarification on which fields can be updated by the helper; currently I
>   don't think we allow overrides for protocol and host and assume all others
>   but the documentation doesn't clarify, and that might be a problem for
>   cert:// where file is more relevant.

I think we do allow a helper to transform a credential in any way:

  echo url=https://example.com/ |
  git \
    -c credential.helper='!sed >&2 s/^/one:/; echo host=other.example.com;:' \
    -c credential.helper='!sed >&2 s/^/two:/;:' \
    credential fill

produces:

  one:protocol=https
  one:host=example.com
  two:protocol=https
  two:host=other.example.com
  Username for 'https://other.example.com': 

So after the first helper runs, subsequent helpers (and the internal
terminal prompt) will consider the modified hostname.

Now whether that's a sane feature or not, I'm not sure.

-Peff

Re: [RFC PATCH] credential: minor documentation fixes

[Carlo Marcelo Arenas Belón]

On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:
> On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:
> > 
> > * the meaning of "exactly" for matching protocol and hostname in the URL
> >   since 06 are both case insensitive per RFC3986 and we have been
> >   ambiguous on that, leading to some helpers assuming case or encoding.
> 
> Yeah, IIRC we discussed case-sensitivity at the time and went with the
> stricter behavior in the name of safety over convenience. And I don't
> think anybody has complained since then. So I'm not really _opposed_ to
> loosening it to match the URL, but perhaps a maintenance release is not
> the best time to do so.

agree, but I was talking not in the context of a feature, but on how we
are to define the interaction with helpers (which was meant to be part of
this maintenance release).

currently (since it is undefined) a naive helper could do a caseless match
by assuming we really meant url as defined by RFC3986, and therefore affect
the wrong credential by the operation.

indeed; our own code might get confused so maybe something like (not fully
tested and likely to need some test coverage) will make sense then as part
of this maintenance release, with some additional mention that clarifies
we REALLY meant "exactly" so that helpers can be updated?

Carlo
---
diff --git a/credential.c b/credential.c
index 108d9e183a..d2c879a9b3 100644
--- a/credential.c
+++ b/credential.c
@@ -70,7 +70,7 @@ static int proto_is_http(const char *s)
 {
 	if (!s)
 		return 0;
-	return !strcmp(s, "https") || !strcmp(s, "http");
+	return !strcasecmp(s, "https") || !strcasecmp(s, "http");
 }
 
 static void credential_describe(struct credential *c, struct strbuf *out);
diff --git a/fsck.c b/fsck.c
index 73f30773f2..d779acdae8 100644
--- a/fsck.c
+++ b/fsck.c
@@ -997,10 +997,10 @@ static int url_to_curl_url(const char *url, const char **out)
 	    skip_prefix(url, "ftp::", out) ||
 	    skip_prefix(url, "ftps::", out))
 		return 1;
-	if (starts_with(url, "http://") ||
-	    starts_with(url, "https://") ||
-	    starts_with(url, "ftp://") ||
-	    starts_with(url, "ftps://")) {
+	if (istarts_with(url, "http://") ||
+	    istarts_with(url, "https://") ||
+	    istarts_with(url, "ftp://") ||
+	    istarts_with(url, "ftps://")) {
 		*out = url;
 		return 1;
 	}

Re: [RFC PATCH] credential: minor documentation fixes

[Jeff King]

On Mon, May 04, 2020 at 08:39:57AM -0700, Carlo Marcelo Arenas Belón wrote:

> On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:
> > On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:
> > > 
> > > * the meaning of "exactly" for matching protocol and hostname in the URL
> > >   since 06 are both case insensitive per RFC3986 and we have been
> > >   ambiguous on that, leading to some helpers assuming case or encoding.
> > 
> > Yeah, IIRC we discussed case-sensitivity at the time and went with the
> > stricter behavior in the name of safety over convenience. And I don't
> > think anybody has complained since then. So I'm not really _opposed_ to
> > loosening it to match the URL, but perhaps a maintenance release is not
> > the best time to do so.
> 
> agree, but I was talking not in the context of a feature, but on how we
> are to define the interaction with helpers (which was meant to be part of
> this maintenance release).
> 
> currently (since it is undefined) a naive helper could do a caseless match
> by assuming we really meant url as defined by RFC3986, and therefore affect
> the wrong credential by the operation.

Right, I understand. But if helpers are doing case-insensitive matches,
I don't think that's a big deal security-wise. And if we're not for our
helpers, that's erring on the conservative side, but if nobody is
complaining about it, I don't think it's urgent.

-Peff

Re: [RFC PATCH] credential: minor documentation fixes

[Carlo Marcelo Arenas Belón]

On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:
> On Mon, May 04, 2020 at 12:45:20AM -0700, Carlo Marcelo Arenas Belón wrote:
> > * clarification on which fields can be updated by the helper; currently I
> >   don't think we allow overrides for protocol and host and assume all others
> >   but the documentation doesn't clarify, and that might be a problem for
> >   cert:// where file is more relevant.
> 
> I think we do allow a helper to transform a credential in any way:
> 
>   echo url=https://example.com/ |
>   git \
>     -c credential.helper='!sed >&2 s/^/one:/; echo host=other.example.com;:' \
>     -c credential.helper='!sed >&2 s/^/two:/;:' \
>     credential fill
> 
> produces:
> 
>   one:protocol=https
>   one:host=example.com
>   two:protocol=https
>   two:host=other.example.com
>   Username for 'https://other.example.com': 
> 
> So after the first helper runs, subsequent helpers (and the internal
> terminal prompt) will consider the modified hostname.

correct, and that is because the specification (and the current code) does
say that is a valid operation.

what I meant is that we don't REALLY allow those modification to take place
since the credential helper suggestions can't really affect what git assumes
is the right host or protocol (or FWIW path), unlike username/password  and
so they don't seem worth having IMHO.

> Now whether that's a sane feature or not, I'm not sure.

would restricting the output of the credential helpers to username and
password make more sense in this context then?

obviously this change will required both the protocol (definition and
implementation) to change an changes on ALL helpers but I thought that was
the objective of that documentation suggestion in Jonathan's original email.

I agree though the discussion of if that is something we should do in a
maintenance release or not, is worth having; but I suspect it could be
implemented with reasonable enough backward compatibility to be considered.

Carlo

CC adding the author of cert:// too easy on that part of the discussion

Re: [RFC PATCH] credential: minor documentation fixes

[Jeff King]

On Mon, May 04, 2020 at 08:58:37AM -0700, Carlo Marcelo Arenas Belón wrote:

> what I meant is that we don't REALLY allow those modification to take place
> since the credential helper suggestions can't really affect what git assumes
> is the right host or protocol (or FWIW path), unlike username/password  and
> so they don't seem worth having IMHO.

I guess it depends on your definition of REALLY.  Sure, we won't access
a different host via curl (or a different protocol!), but I wouldn't
expect that. This is the credential subsystem.

If you're proposing to document it more clearly, I'm fine with that.

And if you want to tighten it up in a major release to ignore such input
from helpers, I'm fine with that, too. But I'd be worried pushing it out
in a maintenance release and finding out that somebody _is_ depending on
it somehow.

-Peff

[PATCH 0/4] credential: documentation updates for maint

[Carlo Marcelo Arenas Belón]

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.

[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 | 20 ++++++++++++--------
 Documentation/gitcredentials.txt | 26 ++++++++++++++++++--------
 credential.h                     |  8 ++++----
 3 files changed, 34 insertions(+), 20 deletions(-)

-- 
2.26.2.686.gfaf46a9ccd

[PATCH 1/4] credential: update description for credential_from_url_gently

[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

[PATCH 2/4] credential: correct order of parameters for credential_match

[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

[PATCH 3/4] credential: update gitcredentials documentation

[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

Re: [PATCH 3/4] credential: update gitcredentials documentation

[Jeff King]

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

[PATCH 4/4] credential: document protocol updates

[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

Re: [PATCH 4/4] credential: document protocol updates

[Jeff King]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Jeff King]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Jeff King]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Jeff King]

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

Re: [PATCH 0/4] credential: documentation updates for maint

[Carlo Marcelo Arenas Belón]

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

[PATCH v2 0/4] credential: documentation updates for maint

[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

[PATCH v2 1/4] credential: update description for credential_from_url_gently

[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

[PATCH v2 2/4] credential: correct order of parameters for credential_match

[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

[PATCH v2 3/4] credential: update gitcredentials documentation

[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

Re: [PATCH v2 3/4] credential: update gitcredentials documentation

[Jeff King]

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

Re: [PATCH v2 3/4] credential: update gitcredentials documentation

[Junio C Hamano]

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.

[PATCH v2 4/4] credential: document protocol updates

[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

Re: [PATCH v2 4/4] credential: document protocol updates

[Jeff King]

On Wed, May 06, 2020 at 02:47:26PM -0700, Carlo Marcelo Arenas Belón wrote:

> 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.

Thanks, this version looks good.

-Peff