Re: [PATCH/RFC] contrib: add win32 credential-helper

[Erik Faye-Lund <kusmabite@gmail.com>]

On Fri, Mar 23, 2012 at 10:10 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Mar 20, 2012 at 12:06:54AM +0100, Erik Faye-Lund wrote:
>
>> This one pretty much sucks. Mem-leaks and a sketchy deletion-filter.
>
> Thanks for moving forward on this. I'm sorry I can't be much help on the
> Windows-specific knowledge, but I'll answer what I can.
>

Thanks for following up :)

>> Currently uses "::" as an attribute-separator, but this is not robust
>> without encoding if the attribute values themselves contains "::".
>
> Yeah. Can you store arbitrary bytes? If so, NUL would be a good
> terminator.

No. TargetName is an LPWSTR, which is supposed to be zero-terminated.
There's no way of specifying it's length directly.

> Otherwise, newline is a reasonable choice, as the protocol
> already can't communicate usernames/passwords with newlines (a
> limitation that I accepted to make the protocol much simpler for
> scripting use).
>

This works, but it causes Windows 7's credential manager to glitch in
rendering the credential (adding all the newlines to the end of the
line, and stretching an icon - yuck), which is also a bit unfortunate.

So I'm thinking that escaping the string needs to be done. It can't be
that big of a deal ;)

> That being said, I realized when writing credential-store that you can
> encode all of the components in a URL, like:
>
>  proto://user:password@host/path
>
> You do have to get the URL encoding/decoding right, of course, but since
> it is a standard format, you may have library or OS support (for the
> stock helpers, I was able to cheat and just link against git's code).
>

Yeah, I guess UrlCanonicalize from WinAPI or something along those
lines can be used.

So far I've simply tried to put the stuff in an order that made simple
string-compare sufficient to pass the test. It seems I need to
implement proper matching instead.

>> I'm not really sure how to make it less sucky in some regards, part
>> of this I blame on lacking documentation of the credential-helper
>> prococol :P
>
> Heh. OK, I'll take the blame. :)
>
>>  1) Encoding of usernames. I'm assuming this is supposed to be
>>  UTF-8, because SecKeychainFindInternetPassword which is used by
>>  the OSX-helper is documented to take accountName as UTF-8.
>
> Like many other parts of git, we treat the data as binary goo as much as
> possible. So git hands the helper whatever bytes the user provided, and
> ships off whatever bytes are provided by the helper over http without
> any further processing. The only two exceptions are:
>
>  1. Fields cannot contain NUL (which means wide encodings like utf-16
>     are pretty much out).
>
>  2. Fields cannot contain newline (which effectively means that the
>     encoding needs to be some ascii superset like utf8 or latin1).
>
> In practice, I would expect most usernames and passwords only contain
> ascii bytes, if only because charset issues between the client and
> server would lead to insanity.
>

ASCII unsernames might be common in the UNIX-world, but in the Windows
world this is very much not the case. These functions can be used for
all kinds of services, so I don't think assuming ASCII makes much
sense.

And since OSX documents the encoding, I'm guessing that non-ASCII
usernames isn't entirely unheard of there either.

In general, I think the whole "let's try to get away with not
specifying encoding" is a bit dangerous. Without knowing what encoding
the input and output is, the helpers are pretty much useless for
people. Sure, saying "at least ASCII" helps, but it just takes us
halfway there. And, I think UTF-8 is the least insane option here.
After all, this is an internal protocol; if the credential helper
needs to store something else (like we do for Windows), we can
convert the string. Likewise, if the network protocol the caller is
going to pass this to assumes something else, convert.

>>  2) Encoding of passwords. I'm assuming UTF-8, as mixing encodings
>>  here would be insane :P
>
> Same as above.
>

Again, my objection is pretty much the same as above, modulo the
comment about OSX.

Binary blob password doesn't make sense; human beings have
text-strings as passwords. They don't remember binary blobs.

About the rest of my questions: I think what you're saying pretty much
makes sense. I think some details could be mentioned in
Documentation/technical/api-credentials.txt, and I'll have a look at
adding what I think makes sense after reading through your mail a bit
closer.

Luckily, I'll have some coding time this Easter, so hopefully I'll be
able to finish up a new version soon-ish.