Am 04.01.2013 22:57, schrieb Erik Faye-Lund: > On Fri, Jan 4, 2013 at 9:28 PM, Karsten Blees <karsten.blees@xxxxxxxxx> wrote: >> On WinXP, the windows credential helper doesn't work at all (due to missing >> Cred[Un]PackAuthenticationBuffer APIs). On Win7, the credential format used >> by wincred is incompatible with native Windows tools (such as the control >> panel applet or 'cmdkey.exe /generic'). These Windows tools only set the >> TargetName, UserName and CredentialBlob members of the CREDENTIAL >> structure (where CredentialBlob is the UTF-16-encoded password). >> >> Remove the unnecessary packing / unpacking of the password, along with the >> related API definitions, for compatibility with Windows XP. >> >> Don't use CREDENTIAL_ATTRIBUTEs to identify credentials for compatibility >> with Windows credential manager tools. Parse the protocol, username, host >> and path fields from the credential's target name instead. >> >> While we're at it, optionally accept CRLF (instead of LF only) to simplify >> debugging in bash / cmd. >> >> Signed-off-by: Karsten Blees <blees@xxxxxxx> >> --- >> >> Hi, >> >> I tried the windows credential helper yesterday and found that it doesn't work on XP at all, and doesn't work properly in combination with Win7 credential manager tools. So here's a patch that reduces it to the functionality provided / expected by Windows. >> >> @Erik, Jeff: Please let me know if I'm missing something and the use of Cred[Un]PackAuthenticationBuffer or CREDENTIAL_ATTRIBUTEs actually served a useful purpose. >> > > The only reason why I used Cred[Un]PackAuthenticationBuffer, were that > I wasn't aware that it was possible any other way. I didn't even know > there was a Windows Credential Manager in Windows XP. > I believe the Cred* API was introduced in Win2k. The XP control panel applet supports domain credentials only, but cmdkey.exe from Windows Server 2003 can be used on XP to manage generic credentials. > The credential attributes were because they were convenient, and I'm > not sure I understand what you mean about the Win7 credential manager > tools. I did test my code with it - in fact, it was a very useful tool > for debugging the helper. > > Are you referring to the credentials not *looking* like normal > HTTP-credentials? No, I was referring to creating / editing git credentials with Windows tools manually. For example, changing your password in control panel removes all credential attributes, so the current wincred won't find them any longer...same for git credentials created e.g. via 'cmdkey /generic:git:http://me@xxxxxxxxxxx /user:me /pass:secret'. The 'puzzling' part is that those credentials *look* exactly the same as if created by wincred, but they don't work. And wincred isn't exactly verbose in its error messages :-) > But, if we do any of these changes, does this mean I will lose my > existing credentials? It's probably not a big deal, but it's worth a > mention, isn't it? > Yes, existing stored credentials are lost after this patch. Will add a note to the commit message. We _could_ try to detect the old format, but I don't think it's worth the trouble. >> @@ -67,20 +61,14 @@ typedef struct _CREDENTIALW { >> #define CRED_MAX_ATTRIBUTES 64 >> >> typedef BOOL (WINAPI *CredWriteWT)(PCREDENTIALW, DWORD); >> -typedef BOOL (WINAPI *CredUnPackAuthenticationBufferWT)(DWORD, PVOID, DWORD, >> - LPWSTR, DWORD *, LPWSTR, DWORD *, LPWSTR, DWORD *); >> typedef BOOL (WINAPI *CredEnumerateWT)(LPCWSTR, DWORD, DWORD *, >> PCREDENTIALW **); >> -typedef BOOL (WINAPI *CredPackAuthenticationBufferWT)(DWORD, LPWSTR, LPWSTR, >> - PBYTE, DWORD *); >> typedef VOID (WINAPI *CredFreeT)(PVOID); >> typedef BOOL (WINAPI *CredDeleteWT)(LPCWSTR, DWORD, DWORD); >> >> static HMODULE advapi, credui; > > Perhaps get rid of credui also? > Good catch >> static CredWriteWT CredWriteW; >> -static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW; >> static CredEnumerateWT CredEnumerateW; >> -static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW; >> static CredFreeT CredFree; >> static CredDeleteWT CredDeleteW; >> >> @@ -88,74 +76,64 @@ static void load_cred_funcs(void) >> { >> /* load DLLs */ >> advapi = LoadLibrary("advapi32.dll"); >> - credui = LoadLibrary("credui.dll"); >> - if (!advapi || !credui) >> + if (!advapi) >> die("failed to load DLLs"); > > It's not multiple DLLs any more, so perhaps "failed to load > advapi32.dll" instead? > Certainly >> -static char target_buf[1024]; >> -static char *protocol, *host, *path, *username; >> -static WCHAR *wusername, *password, *target; >> +static WCHAR *wusername, *password, *protocol, *host, *path, target[1024]; >> >> -static void write_item(const char *what, WCHAR *wbuf) >> +static void write_item(const char *what, LPCWSTR wbuf, int wlen) >> { >> char *buf; >> - int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, NULL, 0, NULL, >> + int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL, >> FALSE); >> buf = xmalloc(len); >> >> - if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, buf, len, NULL, FALSE)) >> + if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, buf, len, NULL, FALSE)) >> die("WideCharToMultiByte failed!"); >> >> printf("%s=", what); >> - fwrite(buf, 1, len - 1, stdout); >> + fwrite(buf, 1, len, stdout); >> putchar('\n'); >> free(buf); >> } >> > > When the password-blob is simply UTF-16 encoded, are the > zero-termination not included? That's the reason for this change, > right? > Yes, the Windows tools store the password without trailing \0. >> -static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword, >> - const char *want) >> +static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim) >> { >> - int i; >> - if (!want) >> - return 1; >> - >> - for (i = 0; i < cred->AttributeCount; ++i) >> - if (!wcscmp(cred->Attributes[i].Keyword, keyword)) >> - return !strcmp((const char *)cred->Attributes[i].Value, >> - want); >> - >> - return 0; /* not found */ >> + LPCWSTR start = *ptarget; >> + LPCWSTR end = *delim ? wcsstr(start, delim) : start + wcslen(start); >> + int len = end ? end - start : wcslen(start); >> + /* update ptarget if we either found a delimiter or need a match */ >> + if (end || want) >> + *ptarget = end ? end + wcslen(delim) : start + len; >> + return !want || (!wcsncmp(want, start, len) && !want[len]); >> } >> > > I'm a bit tired now, but I'm having a hard time reading this code. > Right now it seems it's a bit too ternary-expression heavy for my > taste, though. > OK, I'll flesh that out a bit. Perhaps a few more comments wouldn't hurt either. >> static int match_cred(const CREDENTIALW *cred) >> { >> - return (!wusername || !wcscmp(wusername, cred->UserName)) && >> - match_attr(cred, L"git_protocol", protocol) && >> - match_attr(cred, L"git_host", host) && >> - match_attr(cred, L"git_path", path); >> + LPCWSTR target = cred->TargetName; >> + if (wusername && wcscmp(wusername, cred->UserName)) >> + return 0; >> + >> + return match_part(&target, L"git", L":") && >> + match_part(&target, protocol, L"://") && >> + match_part(&target, wusername, L"@") && >> + match_part(&target, host, L"/") && >> + match_part(&target, path, L""); >> } >> > > Ugh, it feels a bit wrong to store and verify the username twice. Do > we really have to? > > The target needs to be unique, even if two different usernames are > stored for the same site under the same conditions. So probably. It > just doesn't feel quite right. > I don't really see why you would need several usernames to connect to the same target. I can imagine different credentials for reading / writing, but then the protocol would usually be different as well, wouldn't it? (e.g. http vs. ssh) One of my interim solutions was to remove the username part from the URL entirely. That enabled me to change credentials in control panel (including the username), and wincred would use them. However, that version failed the 'helper can store multiple users' test, so I ended up with storing / checking username twice. >> @@ -165,44 +143,15 @@ static void get_credential(void) >> /* search for the first credential that matches username */ >> for (i = 0; i < num_creds; ++i) >> if (match_cred(creds[i])) { >> - cred = creds[i]; >> + write_item("username", creds[i]->UserName, >> + wcslen(creds[i]->UserName)); >> + write_item("password", >> + (LPCWSTR)creds[i]->CredentialBlob, >> + creds[i]->CredentialBlobSize / sizeof(WCHAR)); >> break; >> } >> - if (!cred) >> - return; >> - >> - CredUnPackAuthenticationBufferW(0, cred->CredentialBlob, >> - cred->CredentialBlobSize, NULL, &user_buf_size, NULL, NULL, >> - NULL, &pass_buf_size); >> - >> - user_buf = xmalloc(user_buf_size * sizeof(WCHAR)); >> - pass_buf = xmalloc(pass_buf_size * sizeof(WCHAR)); >> - >> - if (!CredUnPackAuthenticationBufferW(0, cred->CredentialBlob, >> - cred->CredentialBlobSize, user_buf, &user_buf_size, NULL, NULL, >> - pass_buf, &pass_buf_size)) >> - die("CredUnPackAuthenticationBuffer failed"); >> >> CredFree(creds); >> - >> - /* zero-terminate (sizes include zero-termination) */ >> - user_buf[user_buf_size - 1] = L'\0'; >> - pass_buf[pass_buf_size - 1] = L'\0'; >> - >> - write_item("username", user_buf); >> - write_item("password", pass_buf); >> - >> - free(user_buf); >> - free(pass_buf); >> -} > > Nice! > >> - >> -static void write_attr(CREDENTIAL_ATTRIBUTEW *attr, const WCHAR *keyword, >> - const char *value) >> -{ >> - attr->Keyword = (LPWSTR)keyword; >> - attr->Flags = 0; >> - attr->ValueSize = strlen(value) + 1; /* store zero-termination */ >> - attr->Value = (LPBYTE)value; >> } >> >> static void store_credential(void) >> @@ -215,40 +164,18 @@ static void store_credential(void) >> if (!wusername || !password) >> return; >> >> - /* query buffer size */ >> - CredPackAuthenticationBufferW(0, wusername, password, >> - NULL, &auth_buf_size); >> - >> - auth_buf = xmalloc(auth_buf_size); >> - >> - if (!CredPackAuthenticationBufferW(0, wusername, password, >> - auth_buf, &auth_buf_size)) >> - die("CredPackAuthenticationBuffer failed"); >> - >> cred.Flags = 0; >> cred.Type = CRED_TYPE_GENERIC; >> cred.TargetName = target; >> cred.Comment = L"saved by git-credential-wincred"; >> - cred.CredentialBlobSize = auth_buf_size; >> - cred.CredentialBlob = auth_buf; >> + cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR); >> + cred.CredentialBlob = (LPVOID)password; >> cred.Persist = CRED_PERSIST_LOCAL_MACHINE; >> - cred.AttributeCount = 1; >> - cred.Attributes = attrs; >> + cred.AttributeCount = 0; >> + cred.Attributes = NULL; >> cred.TargetAlias = NULL; >> cred.UserName = wusername; >> >> - write_attr(attrs, L"git_protocol", protocol); >> - >> - if (host) { >> - write_attr(attrs + cred.AttributeCount, L"git_host", host); >> - cred.AttributeCount++; >> - } >> - >> - if (path) { >> - write_attr(attrs + cred.AttributeCount, L"git_path", path); >> - cred.AttributeCount++; >> - } >> - >> if (!CredWriteW(&cred, 0)) >> die("CredWrite failed"); >> } > > Nice. > >> @@ -284,10 +211,13 @@ static void read_credential(void) >> >> while (fgets(buf, sizeof(buf), stdin)) { >> char *v; >> + int len = strlen(buf); >> + /* strip trailing CR / LF */ >> + while (len && strchr("\r\n", buf[len - 1])) >> + buf[--len] = 0; >> >> - if (!strcmp(buf, "\n")) >> + if (!*buf) >> break; >> - buf[strlen(buf)-1] = '\0'; >> >> v = strchr(buf, '='); >> if (!v) > > I think this part deserves a separate patch, no? > Sigh...I thought I could get away without doing a patch series :-) >> @@ -295,13 +225,12 @@ static void read_credential(void) >> *v++ = '\0'; >> >> if (!strcmp(buf, "protocol")) >> - protocol = xstrdup(v); >> + protocol = utf8_to_utf16_dup(v); >> else if (!strcmp(buf, "host")) >> - host = xstrdup(v); >> + host = utf8_to_utf16_dup(v); >> else if (!strcmp(buf, "path")) >> - path = xstrdup(v); >> + path = utf8_to_utf16_dup(v); >> else if (!strcmp(buf, "username")) { >> - username = xstrdup(v); >> wusername = utf8_to_utf16_dup(v); >> } else if (!strcmp(buf, "password")) >> password = utf8_to_utf16_dup(v); > > So, you need the strings as UTF-16 instead so you can match against them... > Exactly >> @@ -330,22 +259,20 @@ int main(int argc, char *argv[]) >> return 0; >> >> /* prepare 'target', the unique key for the credential */ >> - strncat(target_buf, "git:", sizeof(target_buf)); >> - strncat(target_buf, protocol, sizeof(target_buf)); >> - strncat(target_buf, "://", sizeof(target_buf)); >> - if (username) { >> - strncat(target_buf, username, sizeof(target_buf)); >> - strncat(target_buf, "@", sizeof(target_buf)); >> + wcscpy(target, L"git:"); >> + wcsncat(target, protocol, ARRAY_SIZE(target)); >> + wcsncat(target, L"://", ARRAY_SIZE(target)); >> + if (wusername) { >> + wcsncat(target, wusername, ARRAY_SIZE(target)); >> + wcsncat(target, L"@", ARRAY_SIZE(target)); >> } >> if (host) >> - strncat(target_buf, host, sizeof(target_buf)); >> + wcsncat(target, host, ARRAY_SIZE(target)); >> if (path) { >> - strncat(target_buf, "/", sizeof(target_buf)); >> - strncat(target_buf, path, sizeof(target_buf)); >> + wcsncat(target, L"/", ARRAY_SIZE(target)); >> + wcsncat(target, path, ARRAY_SIZE(target)); >> } >> >> - target = utf8_to_utf16_dup(target_buf); >> - > > ...Which means that you are composing a UTF-16 target directly rather > than ASCII. Looks sane. > -- 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