Re: [PATCH] wincred: improve compatibility with windows versions
Am 04.01.2013 22:57, schrieb Erik Faye-Lund: On Fri, Jan 4, 2013 at 9:28 PM, Karsten Blees karsten.bl...@gmail.com 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 bl...@dcon.de --- 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://m...@example.com /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
Re: [PATCH] wincred: improve compatibility with windows versions
On Tue, Jan 8, 2013 at 5:20 PM, Karsten Blees karsten.bl...@gmail.com wrote: Am 04.01.2013 22:57, schrieb Erik Faye-Lund: 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. Thanks for the background-info. 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://m...@example.com /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 :-) Right, thanks for clearing that up. 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. Nah, I don't think it's worth the trouble. It's a bit unfortunate that people might get stale credentials clogging up the system, but I don't really thing this is a big deal. static int match_cred(const CREDENTIALW *cred) { - return (!wusername || !wcscmp(wusername, cred-UserName)) - match_attr(cred, Lgit_protocol, protocol) - match_attr(cred, Lgit_host, host) - match_attr(cred, Lgit_path, path); + LPCWSTR target = cred-TargetName; + if (wusername wcscmp(wusername, cred-UserName)) + return 0; + + return match_part(target, Lgit, 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) I can kind of make up some theoretical reasons, but they are a bit exotic ;) 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. I don't think breaking this is a good idea. It just feels a bit silly, but I see now that other applications does the same duplication. So let's just stick to it, even if it's a bit icky ;) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] wincred: improve compatibility with windows versions
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 bl...@dcon.de --- 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. Cheers, Karsten Also available here: https://github.com/kblees/git/tree/kb/improve-wincred-compatibility git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility --- .../credential/wincred/git-credential-wincred.c| 179 ++--- 1 file changed, 53 insertions(+), 126 deletions(-) diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c index cbaec5f..3464080 100644 --- a/contrib/credential/wincred/git-credential-wincred.c +++ b/contrib/credential/wincred/git-credential-wincred.c @@ -9,6 +9,8 @@ /* common helpers */ +#define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0])) + static void die(const char *err, ...) { char msg[4096]; @@ -30,14 +32,6 @@ static void *xmalloc(size_t size) return ret; } -static char *xstrdup(const char *str) -{ - char *ret = strdup(str); - if (!ret) - die(Out of memory); - return ret; -} - /* MinGW doesn't have wincred.h, so we need to define stuff */ typedef struct _CREDENTIAL_ATTRIBUTEW { @@ -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; 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); /* get function pointers */ CredWriteW = (CredWriteWT)GetProcAddress(advapi, CredWriteW); - CredUnPackAuthenticationBufferW = (CredUnPackAuthenticationBufferWT) - GetProcAddress(credui, CredUnPackAuthenticationBufferW); CredEnumerateW = (CredEnumerateWT)GetProcAddress(advapi, CredEnumerateW); - CredPackAuthenticationBufferW = (CredPackAuthenticationBufferWT) - GetProcAddress(credui, CredPackAuthenticationBufferW); CredFree = (CredFreeT)GetProcAddress(advapi, CredFree); CredDeleteW = (CredDeleteWT)GetProcAddress(advapi, CredDeleteW); - if (!CredWriteW || !CredUnPackAuthenticationBufferW || - !CredEnumerateW || !CredPackAuthenticationBufferW || !CredFree || - !CredDeleteW) + if (!CredWriteW || !CredEnumerateW || !CredFree || !CredDeleteW) die(failed to load functions); } -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,
Re: [PATCH] wincred: improve compatibility with windows versions
On Fri, Jan 4, 2013 at 9:28 PM, Karsten Blees karsten.bl...@gmail.com 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 bl...@dcon.de --- 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. 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? If so, I simply didn't see that as a goal. Why would it be? Compatibility with IE? We already lose that with our git: prefix in the target, no? Perhaps we can lose the git:-prefix, and gain IE-compatibility when the protocol matches? 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? @@ -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? 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? -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? -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) -