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. > 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. 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). 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). > 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. > 2) Encoding of passwords. I'm assuming UTF-8, as mixing encodings > here would be insane :P Same as above. > 3) How to match credentials for look-up when not supplying all > attributes. I've implemented a kind-of dodgy matching; I use a > string that packs all supplied attributes. This seems to work, > but I'm a bit worried about what happens when people try to mix > credentials saved with credential.useHttpPath=true with those > saved with credential.useHttpPath=false. See how credential_match in credential.c does it. For a get, the input to the helper is basically a template saying "this is what I want to match". If you have more information in your database than is in the template, that's OK; anything the template doesn't specify should not be considered a restriction. So if you get: protocol=http host=example.com you should match an entry previously stored for: protocol=http host=example.com path=foo.git because what is being asked is a more generic form (if you had two entries, one for foo.git and one for bar.git, it's unclear which should be chosen; my implementations just pick the first match they encounter). The same thing happens for usernames. You might get asked for a credential for a specific username, or you might get asked for a credential of any username at that host. It's nice in the latter case to be able to respond with a previously-stored credential from the former. But note that none of this advanced matching is strictly necessary. It's purely a quality-of-implementation issue in the helper, and that's why I left the details out of the documentation (also, I hoped many helpers could just rely on OS-provided libraries to do the heavy lifting of the match; that's how it is for OS X, but it seems Windows doesn't have anything equivalent). So I'm just describing here what git does for credential-store, credential-cache, and for the internal config matching. You could do a lot less in the helper. For example, I keep my passwords in a password wallet that is backed by GPG-encrypted storage, and my config looks like: [credential "https://github.com"] username = peff helper = "!f() { test $1 = get && echo password=`pass -n github.web.password`; }; f" [credential] helper = cache So note that the helper is totally dumb and relies on the internal config-matching to restrict when it gets used. Likewise, you could be a lot smarter. You could conflate http and https protocol entries according to some internal policy. You could prefix-match paths so that a stored credential for "http://example.com/foo" would be applied to a request for "http://example.com/foo/bar.git". In my helpers, I drew the line where I thought it was reasonable, but there's no reason other helpers have to follow the exact same rules. > 4) How to match credentials for deletion when not supplying > all attributes. It seems from the test-suite that a credential is > expected to be deleted even if the username is not given. Yeah, that's what I would expect. The input is a template, so delete anything that matches the template. > But what happens in such a case when there's two different credentials > for one domain, with different username? The OSX-helper seems to only > delete one of them (but I'm not entirely sure which one, due to > lacking documentation from Apple's side). I'd expect them both to be deleted if no username is given (and I'm pretty sure that's what the stock helpers will do). It may be that the OS X helper does not (I don't recall testing it explicitly). You could consider it a bug, or merely a different design decision. When git deletes a credential, it should provide the username (because it only does so after trying to use the credential and getting a rejection). Again, I intentionally left corner cases like this unspecified, because I didn't want to force helpers to behave one way or the other (especially when git itself doesn't care either way). I hoped that helper writers would then be free to do whatever behavior made sense to them and to their implementation. But from the writer's perspective, I guess it looks like the behavior is simply under-specified. :) > The OSX-helper also have a comment saying "Require at least a > protocol and host for removal, which is what git will give us; if > you want to do something more fancy, use the Keychain manager". > This comment puzzles me a bit; won't git also give us path in case > of credential.useHttpPath=true? Yes; git will provide _at least_ the protocol and host. That code and comment came out of discussion with Junio, where we realized that an "empty" template would cause everything to be deleted. So doing this: git credential-foo erase </dev/null would delete everything! Which maybe is something you would want to do, but it seemed too easy to accidentally trigger this destructive behavior. So I require some input as a sanity-check, and if you want to do something like "delete all entries", you will have to use the separate keychain manager program. Does that make more sense? > 5) How overwriting credentials work. If there's a credential where > all attributes except for the password match, will "store" just > overwrite it? Should it complain? In the stock implementations, we will overwrite. I don't think it matters to git, though. If the helper gives us a credential that works, we will not bother parroting it back[1]. If we get one that does not work, we will erase it. [1] That was my intent, anyway. But I think it may parrot it back, anyway. We should probably set the "approved" flag on a credential given to us by a helper, as there is not much point in asking them to store it again afterwards. > I'm guessing that a lot of these questions fall in the "who cares" > category. For instance, git probably never tries to create a > credential it already has stored. But at least saying "This cannot > happen" in the documentation makes it a lot easier to write a > credential-helper. The problem is that it is not "this cannot happen", but rather "git does not currently do this". I wanted to specify as little as possible so that future versions could change those corner cases if they wanted to (or somebody could write a more advanced tool to interact with the helpers). But maybe we should be more explicit about that in the api-credential document. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html