| Message ID | 20200505013908.4596-1-carenas@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | credential: documentation updates for maint | expand |
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
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:
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
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
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
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