Re: [PATCH] wincred: improve compatibility with windows versions

2013-01-08 Thread Karsten Blees 
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

2013-01-08 Thread Erik Faye-Lund 
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

2013-01-04 Thread Karsten Blees 
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

2013-01-04 Thread 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.

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