wincred: improve compatibility with windows versions

PATCH In review Karsten Blees 4 participants 15 messages Jun 15, 2016

Share & tools

Karsten Blees

author

2016-06-15 22:55 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

---

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

contrib/credential/wincred/git-credential-wincred.c +53 ~~-126~~

index cbaec5f..3464080 100644
@@ -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,
 	    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);
 }
 
-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]);
 }
 
 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"");
 }
 
 static void get_credential(void)
 {
-	WCHAR *user_buf, *pass_buf;
-	DWORD user_buf_size = 0, pass_buf_size = 0;
-	CREDENTIALW **creds, *cred = NULL;
+	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;

@@ -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);
-}
-
-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");
 }

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

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

@@ -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);
-
 	if (!strcmp(argv[1], "get"))
 		get_credential();
 	else if (!strcmp(argv[1], "store"))

Signature · 15 lines

1.8.0.msysgit.0.4.g4e40dea

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Erik Faye-Lund

2016-06-15 22:55 UTC #1 lore raw

Quoted 17 lines · 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.

Quoted 9 lines

Signed-off-by Karsten Blees <[email protected]>
---
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?

Quoted 4 lines

contrib/credential/wincred/git-credential-wincred.c +0 -0

@@ -67,20 +61,14 @@ typedef struct _CREDENTIALW {
 #define CRED_MAX_ATTRIBUTES 64
 
 typedef BOOL (WINAPI *CredWriteWT)(PCREDENTIALW, DWORD);

contrib/credential/wincred/git-credential-wincred.c typedef struct _CREDENTIALW { · line 69

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +0 -4

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

Quoted 8 lines

contrib/credential/wincred/git-credential-wincred.c +0 -2

 static CredWriteWT CredWriteW;
-static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW;
 static CredEnumerateWT CredEnumerateW;
-static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
 static CredFreeT CredFree;
 static CredDeleteWT CredDeleteW;

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 80

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +1 -2

@@ -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?

Quoted 15 lines

contrib/credential/wincred/git-credential-wincred.c +3 -6

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

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 109

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +2 -1

+       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?

Quoted 13 lines

contrib/credential/wincred/git-credential-wincred.c +1 ~~-11~~

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

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 120

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +7 -1

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

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +1 -4

 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;

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 133

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +8 -0

+       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.

Quoted 29 lines

contrib/credential/wincred/git-credential-wincred.c +5 ~~-16~~

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

contrib/credential/wincred/git-credential-wincred.c static void get_credential(void) · line 197

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +0 ~~-10~~

-       /* 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!

Quoted 46 lines

contrib/credential/wincred/git-credential-wincred.c +4 ~~-27~~

-
-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++;

contrib/credential/wincred/git-credential-wincred.c static void store_credential(void) · line 181

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +0 -7

-       }
-
-       if (path) {
-               write_attr(attrs + cred.AttributeCount, L"git_path", path);
-               cred.AttributeCount++;
-       }
-
        if (!CredWriteW(&cred, 0))
                die("CredWrite failed");
 }

Nice.

Quoted 6 lines

contrib/credential/wincred/git-credential-wincred.c +2 -0

@@ -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 */

contrib/credential/wincred/git-credential-wincred.c static void read_credential(void) · line 223

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +3 -2

+               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?

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +1 -1

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

contrib/credential/wincred/git-credential-wincred.c static void read_credential(void) · line 236

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +2 -3

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

Quoted 19 lines

contrib/credential/wincred/git-credential-wincred.c +6 -7

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

contrib/credential/wincred/git-credential-wincred.c int main(int argc, char *argv[]) · line 348

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +3 -4

+               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.

Signature · 12 lines

*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Karsten Blees

author

2016-06-15 22:55 UTC #2 lore raw

Am 04.01.2013 22:57, schrieb Erik Faye-Lund:

Quoted 23 lines · Karsten Blees

Quoted 22 lines
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 <[email protected]>
---
Hi,

Quoted 9 lines

Quoted 4 lines
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.

Quoted 7 lines

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://[email protected] /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 :-)

Quoted 4 lines

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.

Quoted 13 lines

contrib/credential/wincred/git-credential-wincred.c +0 -4

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

Quoted 4 lines

contrib/credential/wincred/git-credential-wincred.c typedef struct _CREDENTIALW { · line 69
Quoted 1 line
contrib/credential/wincred/git-credential-wincred.c +0 -0
 static HMODULE advapi, credui;
Perhaps get rid of credui also?

Good catch

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +0 -2

 static CredWriteWT CredWriteW;
-static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW;
 static CredEnumerateWT CredEnumerateW;
-static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
 static CredFreeT CredFree;
 static CredDeleteWT CredDeleteW;

Quoted 12 lines

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 80
Quoted 8 lines
contrib/credential/wincred/git-credential-wincred.c +1 -2
@@ -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

Quoted 18 lines

contrib/credential/wincred/git-credential-wincred.c +4 -6

-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!");

Quoted 12 lines

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 109
Quoted 7 lines
contrib/credential/wincred/git-credential-wincred.c +1 -1
        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.

Quoted 19 lines

contrib/credential/wincred/git-credential-wincred.c +6 ~~-12~~

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

Quoted 9 lines

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 120
Quoted 4 lines
contrib/credential/wincred/git-credential-wincred.c +2 -0
+               *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.

Quoted 18 lines

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +1 -4

 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;

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 133

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +8 -0

+       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"");
 }

Quoted 7 lines

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.

Quoted 115 lines

Quoted 29 lines

contrib/credential/wincred/git-credential-wincred.c +5 ~~-16~~

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

contrib/credential/wincred/git-credential-wincred.c static void get_credential(void) · line 197

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +0 ~~-10~~

-       /* 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!

Quoted 46 lines

contrib/credential/wincred/git-credential-wincred.c +4 ~~-27~~

-
-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++;

contrib/credential/wincred/git-credential-wincred.c static void store_credential(void) · line 181

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +0 -7

-       }
-
-       if (path) {
-               write_attr(attrs + cred.AttributeCount, L"git_path", path);
-               cred.AttributeCount++;
-       }
-
        if (!CredWriteW(&cred, 0))
                die("CredWrite failed");
 }

Nice.

Quoted 14 lines

contrib/credential/wincred/git-credential-wincred.c +5 -2

@@ -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';

Quoted 5 lines

contrib/credential/wincred/git-credential-wincred.c static void read_credential(void) · line 223
Quoted 2 lines
contrib/credential/wincred/git-credential-wincred.c +0 -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 :-)

Quoted 20 lines

Quoted 7 lines

contrib/credential/wincred/git-credential-wincred.c +1 -1

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

contrib/credential/wincred/git-credential-wincred.c static void read_credential(void) · line 236

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +2 -3

-                       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

Quoted 33 lines

Quoted 19 lines

contrib/credential/wincred/git-credential-wincred.c +6 -7

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

contrib/credential/wincred/git-credential-wincred.c int main(int argc, char *argv[]) · line 348

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +3 -4

+               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.

Signature · 12 lines

*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Erik Faye-Lund

2016-06-15 22:55 UTC #3 lore raw

Quoted 8 lines · Karsten Blees

Am 04.01.2013 22:57, schrieb Erik Faye-Lund:
Quoted 4 lines
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.

Quoted 5 lines

Quoted 5 lines
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.

Quoted 7 lines

Quoted 2 lines
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://[email protected] /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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 21 lines

Quoted 7 lines

static int match_cred(const CREDENTIALW *cred)
{

contrib/credential/wincred/git-credential-wincred.c +0 -4

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

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 132

Quoted 10 lines

contrib/credential/wincred/git-credential-wincred.c +9 -0

+       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?

Quoted 7 lines

Quoted 4 lines
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 ;)

Quoted 2 lines

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

Karsten Blees

author

2016-06-15 22:55 UTC #4 lore raw

v2 cover improve-wincred-compatibility

Changes since initial version (see attached diff for details):
- split in two patches
- removed unused variables
- improved the dll error message
- changed ?: to if else
- added comments

Also available here:
https://github.com/kblees/git/tree/kb/improve-wincred-compatibility-v2
git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility-v2

Karsten Blees (2):
wincred: accept CRLF on stdin to simplify console usage
wincred: improve compatibility with windows versions

 .../credential/wincred/git-credential-wincred.c    | 206 ++++++++-------------
 1 file changed, 75 insertions(+), 131 deletions(-)

Quoted 1 line

git diff kb/improve-wincred-compatibility..kb/improve-wincred-compatibility-v2

contrib/credential/wincred/git-credential-wincred.c +27 ~~-10~~

index 3464080..dac19ea 100644
@@ -66,7 +66,7 @@ typedef BOOL (WINAPI *CredEnumerateWT)(LPCWSTR, DWORD, DWORD *,
 typedef VOID (WINAPI *CredFreeT)(PVOID);
 typedef BOOL (WINAPI *CredDeleteWT)(LPCWSTR, DWORD, DWORD);
 
-static HMODULE advapi, credui;
+static HMODULE advapi;
 static CredWriteWT CredWriteW;
 static CredEnumerateWT CredEnumerateW;
 static CredFreeT CredFree;

@@ -77,7 +77,7 @@ static void load_cred_funcs(void)
 	/* load DLLs */
 	advapi = LoadLibrary("advapi32.dll");
 	if (!advapi)
-		die("failed to load DLLs");
+		die("failed to load advapi32.dll");
 
 	/* get function pointers */
 	CredWriteW = (CredWriteWT)GetProcAddress(advapi, "CredWriteW");

@@ -107,14 +107,34 @@ static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 	free(buf);
 }
 
+/*
+ * Match an (optional) expected string and a delimiter in the target string,
+ * consuming the matched text by updating the target pointer.
+ */
 static int match_part(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
 {
-	LPCWSTR start = *ptarget;
-	LPCWSTR end = *delim ? wcsstr(start, delim) : start + wcslen(start);
-	int len = end ? end - start : wcslen(start);
+	LPCWSTR delim_pos, start = *ptarget;
+	int len;
+
+	/* find start of delimiter (or end-of-string if delim is empty) */
+	if (*delim)
+		delim_pos = wcsstr(start, delim);
+	else
+		delim_pos = start + wcslen(start);
+
+	/*
+	 * match text up to delimiter, or end of string (e.g. the '/' after
+	 * host is optional if not followed by a path)
+	 */
+	if (delim_pos)
+		len = delim_pos - start;
+	else
+		len = wcslen(start);
+
 	/* update ptarget if we either found a delimiter or need a match */
-	if (end || want)
-		*ptarget = end ? end + wcslen(delim) : start + len;
+	if (delim_pos || want)
+		*ptarget = delim_pos ? delim_pos + wcslen(delim) : start + len;
+
 	return !want || (!wcsncmp(want, start, len) && !want[len]);
 }

@@ -157,9 +177,6 @@ static void get_credential(void)
 static void store_credential(void)
 {
 	CREDENTIALW cred;
-	BYTE *auth_buf;
-	DWORD auth_buf_size = 0;
-	CREDENTIAL_ATTRIBUTEW attrs[CRED_MAX_ATTRIBUTES];
 
 	if (!wusername || !password)
 		return;

Signature · 12 lines

*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Erik Faye-Lund

Acked-by on the v2 series

2016-06-15 22:55 UTC #5 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Quoted 11 lines · Karsten Blees

Changes since initial version (see attached diff for details):
- split in two patches
- removed unused variables
- improved the dll error message
- changed ?: to if else
- added comments
Also available here:
https://github.com/kblees/git/tree/kb/improve-wincred-compatibility-v2
git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility-v2

Quoted 8 lines

Karsten Blees (2):
wincred: accept CRLF on stdin to simplify console usage
wincred: improve compatibility with windows versions
 .../credential/wincred/git-credential-wincred.c    | 206 ++++++++-------------
 1 file changed, 75 insertions(+), 131 deletions(-)

Wonderful!

Acked-by Erik Faye-Lund <[email protected]>

Signature · 12 lines

*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Junio C Hamano

2016-06-15 22:56 UTC #6 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Quoted 16 lines · Erik Faye-Lund

Quoted 15 lines · Karsten Blees
Changes since initial version (see attached diff for details):
- split in two patches
- removed unused variables
- improved the dll error message
- changed ?: to if else
- added comments
Also available here:
https://github.com/kblees/git/tree/kb/improve-wincred-compatibility-v2
git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility-v2
Karsten Blees (2):
wincred: accept CRLF on stdin to simplify console usage
wincred: improve compatibility with windows versions

Quoted 7 lines

Quoted 3 lines
 .../credential/wincred/git-credential-wincred.c    | 206 ++++++++-------------
 1 file changed, 75 insertions(+), 131 deletions(-)
Wonderful!
Acked-by Erik Faye-Lund <[email protected]>

I'm in the "marking leftover bits" mode today, and noticed that nothing happened for this topic in my tree. Did msysgit folks expect me to pick this up directly, or did you guys want to feed this series to me (with possibly other changes you worked on outside this list)?

Karsten Blees

author

2016-06-15 22:56 UTC #7 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Am 25.02.2013 07:43, schrieb Junio C Hamano:

Quoted 22 lines

Quoted 20 lines · Erik Faye-Lund
Quoted 18 lines · Karsten Blees
Changes since initial version (see attached diff for details):
- split in two patches
- removed unused variables
- improved the dll error message
- changed ?: to if else
- added comments
Also available here:
https://github.com/kblees/git/tree/kb/improve-wincred-compatibility-v2
git pull git://github.com/kblees/git.git kb/improve-wincred-compatibility-v2
Karsten Blees (2):
wincred: accept CRLF on stdin to simplify console usage
wincred: improve compatibility with windows versions
 .../credential/wincred/git-credential-wincred.c    | 206 ++++++++-------------
 1 file changed, 75 insertions(+), 131 deletions(-)

Quoted 9 lines

Quoted 3 lines
Wonderful!
Acked-by Erik Faye-Lund <[email protected]>
I'm in the "marking leftover bits" mode today, and noticed that nothing happened for this topic in my tree. Did msysgit folks expect me to pick this up directly, or did you guys want to feed this series to me (with possibly other changes you worked on outside this list)?

The second patch changes the credential format in a backward-incompatible way, so I think this should be in git.git, too (better than having two incompatible versions around).

@Pat, Dscho: the rebase-merge script should automatically drop patches found in upstream, correct?

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Junio C Hamano

2016-06-15 22:56 UTC #8 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Quoted 11 lines · Karsten Blees

Am 25.02.2013 07:43, schrieb Junio C Hamano:
Quoted 1 line
Erik Faye-Lund <[email protected]> writes:
...
Quoted 4 lines
I'm in the "marking leftover bits" mode today, and noticed that nothing happened for this topic in my tree. Did msysgit folks expect me to pick this up directly, or did you guys want to feed this series to me (with possibly other changes you worked on outside this list)?
The second patch changes the credential format in a backward-incompatible way, so I think this should be in git.git, too (better than having two incompatible versions around).

I am a bit confused by that comment, as it was my understanding that the "credential format" only refers to what is stored in the Windows specific credential storage and relevant only to users of msysgit (and I expected folks who are using Git natively on Windows to be using msysgit, not git.git), but in any case, of course we would want a single version and format. My question was if msysgit folks want me to take your patch (which I cannot test myself) and then merge my tree to the msysgit tree as part of their regular updates to catch up with 1.8.2 (and future releases), or they want to test your patches in their tree first, and then either throw me a pull request or send me a patch series with Acked-by:.

I can obviously go either way, but if they expect the former (i.e. I first apply and they pull) while I expect the latter (i.e. they first apply and I pull), then nothing will happen, and if we go the other way around, we would end up getting two copies of the same series. The question was primarily to avoid these possibilities.

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Erik Faye-Lund

2016-06-15 22:56 UTC #9 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Quoted 6 lines · Junio C Hamano

My question was if msysgit folks
want me to take your patch (which I cannot test myself) and then merge my tree to the msysgit tree as part of their regular updates to catch up with 1.8.2 (and future releases), or they want to test your patches in their tree first, and then either throw me a pull request or send me a patch series with Acked-by:.

The following changes since commit 4dac0679feaebbf6545daec14480cf6b94cb74ed:

Git 1.8.2-rc1 (2013-02-25 09:03:26 -0800)

are available in the git repository at:

git://github.com/kusma/git.git for-junio

for you to fetch changes up to 8b2d219a3d6db49c8c3c0a5b620af33d6a40a974:

wincred: improve compatibility with windows versions (2013-02-26
17:42:46 +0100)

----------------------------------------------------------------
Karsten Blees (2):
wincred: accept CRLF on stdin to simplify console usage
wincred: improve compatibility with windows versions

 .../credential/wincred/git-credential-wincred.c    | 206 ++++++++-------------
 1 file changed, 75 insertions(+), 131 deletions(-)

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Junio C Hamano

2016-06-15 22:56 UTC #10 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

Thanks.

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Johannes Schindelin

2016-06-15 22:56 UTC #11 lore raw

Re: [PATCH v2 0/2] improve-wincred-compatibility

In reply to Karsten Blees

Hi Karsten,

Quoted 2 lines · Karsten Blees

@Pat, Dscho: the rebase-merge script should automatically drop patches found in upstream, correct?

Yep, that's the idea. Under some circumstances, --cherry will fail to pick up on it and the commits will still be marked for 'pick', but when it comes to them, there will be no changes to be committed.

Ciao,
Dscho

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "msysGit" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/groups/opt_out.

Karsten Blees

author

2016-06-15 22:55 UTC #12 lore raw

PATCH v2 1/2 wincred: accept CRLF on stdin to simplify console usage

In reply to Erik Faye-Lund

The windows credential helper currently only accepts LF on stdin, but bash and cmd.exe both send CRLF. This prevents interactive use in the console.

Change the stdin parser to optionally accept CRLF.

Signed-off-by Karsten Blees <[email protected]>

---
 contrib/credential/wincred/git-credential-wincred.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

contrib/credential/wincred/git-credential-wincred.c +5 -2

index cbaec5f..94d7140 100644
@@ -284,10 +284,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)

Signature · 15 lines

1.8.0.msysgit.0.4.g4e40dea

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Karsten Blees

author

2016-06-15 22:55 UTC #13 lore raw

PATCH v2 2/2 wincred: improve compatibility with windows versions

In reply to Erik Faye-Lund

Remove the unnecessary packing / unpacking of the password, along with the related API definitions, for compatibility with Windows XP.

Credentials created with an old wincred version will have mangled or empty passwords after this change.

Signed-off-by Karsten Blees <[email protected]>

---
 .../credential/wincred/git-credential-wincred.c    | 199 ++++++++-------------
 1 file changed, 70 insertions(+), 129 deletions(-)

contrib/credential/wincred/git-credential-wincred.c +70 ~~-129~~

index 94d7140..dac19ea 100644
@@ -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 HMODULE advapi;
 static CredWriteWT CredWriteW;
-static CredUnPackAuthenticationBufferWT CredUnPackAuthenticationBufferW;
 static CredEnumerateWT CredEnumerateW;
-static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
 static CredFreeT CredFree;
 static CredDeleteWT CredDeleteW;

@@ -88,74 +76,84 @@ static void load_cred_funcs(void)
 {
 	/* load DLLs */
 	advapi = LoadLibrary("advapi32.dll");
-	credui = LoadLibrary("credui.dll");
-	if (!advapi || !credui)
-		die("failed to load DLLs");
+	if (!advapi)
+		die("failed to load advapi32.dll");
 
 	/* 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,
 	    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);
 }
 
-static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword,
-    const char *want)
+/*
+ * Match an (optional) expected string and a delimiter in the target string,
+ * consuming the matched text by updating the target pointer.
+ */
+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 delim_pos, start = *ptarget;
+	int len;
+
+	/* find start of delimiter (or end-of-string if delim is empty) */
+	if (*delim)
+		delim_pos = wcsstr(start, delim);
+	else
+		delim_pos = start + wcslen(start);
+
+	/*
+	 * match text up to delimiter, or end of string (e.g. the '/' after
+	 * host is optional if not followed by a path)
+	 */
+	if (delim_pos)
+		len = delim_pos - start;
+	else
+		len = wcslen(start);
+
+	/* update ptarget if we either found a delimiter or need a match */
+	if (delim_pos || want)
+		*ptarget = delim_pos ? delim_pos + wcslen(delim) : start + len;
+
+	return !want || (!wcsncmp(want, start, len) && !want[len]);
 }
 
 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"");
 }
 
 static void get_credential(void)
 {
-	WCHAR *user_buf, *pass_buf;
-	DWORD user_buf_size = 0, pass_buf_size = 0;
-	CREDENTIALW **creds, *cred = NULL;
+	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;

@@ -165,90 +163,36 @@ 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);
-}
-
-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)
 {
 	CREDENTIALW cred;
-	BYTE *auth_buf;
-	DWORD auth_buf_size = 0;
-	CREDENTIAL_ATTRIBUTEW attrs[CRED_MAX_ATTRIBUTES];
 
 	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");
 }

@@ -298,13 +242,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);

@@ -333,22 +276,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);
-
 	if (!strcmp(argv[1], "get"))
 		get_credential();
 	else if (!strcmp(argv[1], "store"))

Signature · 15 lines

1.8.0.msysgit.0.4.g4e40dea

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

Erik Faye-Lund

2016-06-15 23:02 UTC #14 lore raw

contrib/credential/wincred/git-credential-wincred.c static void load_cred_funcs(void) · line 149

Quoted 13 lines · Karsten Blees

static int match_cred(const CREDENTIALW *cred)
{

contrib/credential/wincred/git-credential-wincred.c +7 -4

-       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"@") &&

This seems to have broken credentials with '@' in the username, which isn't all that unusual these days... :(

Signature · 18 lines

-- 
*** Please reply-to-all at all times ***
*** (do not pretend to know who is subscribed and who is not) ***
*** Please avoid top-posting. ***
The msysGit Wiki is here: https://github.com/msysgit/msysgit/wiki - Github accounts are free.

You received this message because you are subscribed to the Google
Groups "msysGit" group.
To post to this group, send email to [email protected]
To unsubscribe from this group, send email to
[email protected]
For more options, and view previous threads, visit this group at
http://groups.google.com/group/msysgit?hl=en_US?hl=en

--- 
You received this message because you are subscribed to the Google Groups "Git for Windows" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.