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

On Fri, Mar 23, 2012 at 10:10 PM, Jeff King [off-list ref] wrote:

On Tue, Mar 20, 2012 at 12:06:54AM +0100, Erik Faye-Lund wrote:

quoted

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

quoted

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.

quoted

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

quoted

 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.

quoted

 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.