threads / patch / 47075

patch, 2 partswincred: learn to handle "empty credentials"

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

## tl;dr

5 messages between Oct 30, 2017 and Oct 31, 2017. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Johannes Schindelin· Oct 30, 2017, 17:20 UTC · lore

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
Johannes Schindelin· Oct 30, 2017, 17:20 UTC · re: Johannes Schindelin · lore

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

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(+)
Show changes to t/lib-credential.sh +19 −0
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
Jeff King· Oct 30, 2017, 18:27 UTC · re: Johannes Schindelin · lore

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

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
Johannes Schindelin· Oct 31, 2017, 17:16 UTC · re: Jeff King · lore

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

Hi Peff,
On Mon, 30 Oct 2017, Jeff King wrote:
Show 20 quoted lines
> 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

Johannes Schindelin· Oct 30, 2017, 17:20 UTC · re: Johannes Schindelin · lore

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

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(-)
Show changes to contrib/credential/wincred/git-credential-wincred.c +8 −2
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

← back to recent threads