RFC patchcredential: minor documentation fixes

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(-)

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

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

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

On Sun, May 03, 2020 at 02:58:26AM -0400, Jeff King wrote:

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/

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

Sure, feel free to.

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.

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

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

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.

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

On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:

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

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

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

On Mon, May 04, 2020 at 10:44:36AM -0400, Jeff King wrote:

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.

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

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

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

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(-)

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(-)

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(-)

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(-)

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

s/consitency/consistency/s

I think the overall direction is good. A few small comments below.

It might be worth giving an example of the syntax, like:

  the port number if one was specified (e.g., "example.com:8088").

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.

Makes sense.

-Peff

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(-)

On Mon, May 04, 2020 at 06:39:07PM -0700, Carlo Marcelo Arenas Belón wrote:

Yep, these all make sense. One nit:

"refrain from doing so" would be more idiomatic English.

-Peff

On Mon, May 04, 2020 at 06:39:04PM -0700, Carlo Marcelo Arenas Belón wrote:

I left a few small comments on 3 and 4, but overall these all look like improvements to me. Thanks.

-Peff

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(-)

On Wed, May 06, 2020 at 04:28:48PM -0700, Carlo Marcelo Arenas Belón wrote:

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

On Thu, May 07, 2020 at 04:59:09PM -0400, Jeff King wrote:

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

On Thu, May 07, 2020 at 02:23:48PM -0700, Carlo Marcelo Arenas Belón wrote:

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

On Thu, May 07, 2020 at 06:17:48PM -0400, Jeff King wrote:

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.

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

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(-)

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(-)

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(-)

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(-)

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

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

Jeff King <peff@peff.net> writes:

Will locally tweak; thanks.

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(-)

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

Thanks, this version looks good.

-Peff