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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

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