[PATCH 0/2] wincred: learn to handle "empty credentials"

[PATCH 0/2] wincred: learn to handle "empty credentials"

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

As we learned some time ago, NTLM authentication happens by passing
"empty credentials", i.e. 0-length usernames and passwords.

However, when saved in the Windows Credential Manager, such usernames
and passwords come back as null when reading the credential. Let's
handle this.

This patch series is a tender four years old and has been simmering in
Git for Windows since version v1.8.4, so it is most likely mature enough
(even at that young age) to enter core Git.

Note: these days, Git for Windows prefers to use the Git Credential
Manager for Windows instead, but the wincred code is still carried in
Git's contrib/ and should be fixed there, too.


Jakub Bereżański (2):
  t0302: check helper can handle empty credentials
  wincred: handle empty username/password correctly

 contrib/credential/wincred/git-credential-wincred.c | 10 ++++++++--
 t/lib-credential.sh                                 | 19 +++++++++++++++++++
 2 files changed, 27 insertions(+), 2 deletions(-)


base-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe
Published-As: https://github.com/dscho/git/releases/tag/jberezanski/wincred-sso-r2-v1
Fetch-It-Via: git fetch https://github.com/dscho/git jberezanski/wincred-sso-r2-v1
-- 
2.15.0.windows.1

[PATCH 1/2] t0302: check helper can handle empty credentials

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]

From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= <kuba@berezanscy.pl>

Make sure the helper does not crash when blank username and password is
provided. If the helper can save such credentials, it should be able to
read them back.

Signed-off-by: Jakub Bereżański <kuba@berezanscy.pl>
---
 t/lib-credential.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index d8e41f7ddd1..937b831ea67 100755
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -44,6 +44,7 @@ helper_test_clean() {
 	reject $1 https example.com user2
 	reject $1 http path.tld user
 	reject $1 https timeout.tld user
+	reject $1 https sso.tld
 }
 
 reject() {
@@ -250,6 +251,24 @@ helper_test() {
 		password=pass2
 		EOF
 	'
+
+	test_expect_success "helper ($HELPER) can store empty username" '
+		check approve $HELPER <<-\EOF &&
+		protocol=https
+		host=sso.tld
+		username=
+		password=
+		EOF
+		check fill $HELPER <<-\EOF
+		protocol=https
+		host=sso.tld
+		--
+		protocol=https
+		host=sso.tld
+		username=
+		password=
+		EOF
+	'
 }
 
 helper_test_timeout() {
-- 
2.15.0.windows.1

[PATCH 2/2] wincred: handle empty username/password correctly

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2174 bytes --]

From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= <kuba@berezanscy.pl>

Empty (length 0) usernames and/or passwords, when saved in the Windows
Credential Manager, come back as null when reading the credential.

One use case for such empty credentials is with NTLM authentication, where
empty username and password instruct libcurl to authenticate using the
credentials of the currently logged-on user (single sign-on).

When locating the relevant credentials, make empty username match null.
When outputting the credentials, handle nulls correctly.

Signed-off-by: Jakub Bereżański <kuba@berezanscy.pl>
---
 contrib/credential/wincred/git-credential-wincred.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 006134043a4..86518cd93d9 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -94,6 +94,12 @@ static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {
 	char *buf;
+
+	if (!wbuf || !wlen) {
+		printf("%s=\n", what);
+		return;
+	}
+
 	int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL,
 	    FALSE);
 	buf = xmalloc(len);
@@ -160,7 +166,7 @@ static int match_part_last(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)
 static int match_cred(const CREDENTIALW *cred)
 {
 	LPCWSTR target = cred->TargetName;
-	if (wusername && wcscmp(wusername, cred->UserName))
+	if (wusername && wcscmp(wusername, cred->UserName ? cred->UserName : L""))
 		return 0;
 
 	return match_part(&target, L"git", L":") &&
@@ -183,7 +189,7 @@ static void get_credential(void)
 	for (i = 0; i < num_creds; ++i)
 		if (match_cred(creds[i])) {
 			write_item("username", creds[i]->UserName,
-				wcslen(creds[i]->UserName));
+				creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
 			write_item("password",
 				(LPCWSTR)creds[i]->CredentialBlob,
 				creds[i]->CredentialBlobSize / sizeof(WCHAR));
-- 
2.15.0.windows.1

Re: [PATCH 1/2] t0302: check helper can handle empty credentials

[Jeff King]

On Mon, Oct 30, 2017 at 06:20:12PM +0100, Johannes Schindelin wrote:

> Subject: Re: [PATCH 1/2] t0302: check helper can handle empty credentials

I guess we really care about t0303 here (which tests external helpers).
This patch adds the test to lib-credential, so it hits the "cache" and
"store" helpers, too. Which seems to pass, so I guess that's OK (I have
to admit that as the author of those tools, I wasn't sure how they'd
react).

> Make sure the helper does not crash when blank username and password is
> provided. If the helper can save such credentials, it should be able to
> read them back.

I worry that some third-party helpers might not be able to represent
this case and would fail the test. This has been around for years no
Windows, but probably hasn't ever been run with osxkeychain or
libsecret. I'd be OK with taking this as-is, though, and waiting to see
if anybody complains. At that point we'll know if the right solution is
enhancing that helper, or providing a way to optionally skip this test.

(Though I have no idea if anybody actually runs t0303 against
custom-built helpers in the first place. The process is pretty manual
for now, though the Makefiles in contrib/credential could probably at
least provide a "make test").

-Peff

Re: [PATCH 1/2] t0302: check helper can handle empty credentials

[Johannes Schindelin]

Hi Peff,

On Mon, 30 Oct 2017, Jeff King wrote:

> On Mon, Oct 30, 2017 at 06:20:12PM +0100, Johannes Schindelin wrote:
> 
> > Subject: Re: [PATCH 1/2] t0302: check helper can handle empty credentials
> 
> I guess we really care about t0303 here (which tests external helpers).
> This patch adds the test to lib-credential, so it hits the "cache" and
> "store" helpers, too. Which seems to pass, so I guess that's OK (I have
> to admit that as the author of those tools, I wasn't sure how they'd
> react).
> 
> > Make sure the helper does not crash when blank username and password is
> > provided. If the helper can save such credentials, it should be able to
> > read them back.
> 
> I worry that some third-party helpers might not be able to represent
> this case and would fail the test. This has been around for years no
> Windows, but probably hasn't ever been run with osxkeychain or
> libsecret. I'd be OK with taking this as-is, though, and waiting to see
> if anybody complains. At that point we'll know if the right solution is
> enhancing that helper, or providing a way to optionally skip this test.

Okay. If you change your mind, please let me know, I would try to set
aside some time to adjust the patch in that event.

> (Though I have no idea if anybody actually runs t0303 against
> custom-built helpers in the first place. The process is pretty manual
> for now, though the Makefiles in contrib/credential could probably at
> least provide a "make test").

Right... I am not aware of any attempts to run those tests against Git
Credential Manager for Windows, for example...

Ciao,
Dscho