{"thread":{"id":"47075","subject":"[PATCH 0/2] wincred: learn to handle \"empty credentials\"","startedAt":"2017-10-30T17:20:25Z","lastAt":"2017-10-31T17:16:47Z","messageCount":5,"participants":["Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"331373","messageId":"cover.1509383993.git.johannes.schindelin@gmx.de","threadId":"47075","inReplyTo":null,"subject":"[PATCH 0/2] wincred: learn to handle \"empty credentials\"","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-30T17:20:04Z","receivedAt":"2017-10-30T17:20:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"As we learned some time ago, NTLM authentication happens by passing\n\"empty credentials\", i.e. 0-length usernames and passwords.\n\nHowever, when saved in the Windows Credential Manager, such usernames\nand passwords come back as null when reading the credential. Let's\nhandle this.\n\nThis patch series is a tender four years old and has been simmering in\nGit for Windows since version v1.8.4, so it is most likely mature enough\n(even at that young age) to enter core Git.\n\nNote: these days, Git for Windows prefers to use the Git Credential\nManager for Windows instead, but the wincred code is still carried in\nGit's contrib/ and should be fixed there, too.\n\n\nJakub Bereżański (2):\n t0302: check helper can handle empty credentials\n wincred: handle empty username/password correctly\n\n contrib/credential/wincred/git-credential-wincred.c | 10 ++++++++--\n t/lib-credential.sh | 19 +++++++++++++++++++\n 2 files changed, 27 insertions(+), 2 deletions(-)\n\n\nbase-commit: cb5918aa0d50f50e83787f65c2ddc3dcb10159fe\nPublished-As: https://github.com/dscho/git/releases/tag/jberezanski/wincred-sso-r2-v1\nFetch-It-Via: git fetch https://github.com/dscho/git jberezanski/wincred-sso-r2-v1\n-- \n2.15.0.windows.1\n"},{"id":"331374","messageId":"e055913ec27fc3feb3f017747560f454f6817a0b.1509383993.git.johannes.schindelin@gmx.de","threadId":"47075","inReplyTo":"cover.1509383993.git.johannes.schindelin@gmx.de","subject":"[PATCH 1/2] t0302: check helper can handle empty credentials","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-30T17:20:12Z","receivedAt":"2017-10-30T17:21:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= \n\nMake sure the helper does not crash when blank username and password is\nprovided. If the helper can save such credentials, it should be able to\nread them back.\n\nSigned-off-by: Jakub Bereżański \n---\n t/lib-credential.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/lib-credential.sh b/t/lib-credential.sh\nindex d8e41f7ddd1..937b831ea67 100755\n--- a/t/lib-credential.sh\n+++ b/t/lib-credential.sh\n@@ -44,6 +44,7 @@ helper_test_clean() {\n \treject $1 https example.com user2\n \treject $1 http path.tld user\n \treject $1 https timeout.tld user\n+\treject $1 https sso.tld\n }\n \n reject() {\n@@ -250,6 +251,24 @@ helper_test() {\n \t\tpassword=pass2\n \t\tEOF\n \t'\n+\n+\ttest_expect_success \"helper ($HELPER) can store empty username\" '\n+\t\tcheck approve $HELPER <<-\\EOF &&\n+\t\tprotocol=https\n+\t\thost=sso.tld\n+\t\tusername=\n+\t\tpassword=\n+\t\tEOF\n+\t\tcheck fill $HELPER <<-\\EOF\n+\t\tprotocol=https\n+\t\thost=sso.tld\n+\t\t--\n+\t\tprotocol=https\n+\t\thost=sso.tld\n+\t\tusername=\n+\t\tpassword=\n+\t\tEOF\n+\t'\n }\n \n helper_test_timeout() {\n-- \n2.15.0.windows.1\n\n"},{"id":"331376","messageId":"71cf372b57f88784ca467840512dac2e9e01767b.1509383993.git.johannes.schindelin@gmx.de","threadId":"47075","inReplyTo":"cover.1509383993.git.johannes.schindelin@gmx.de","subject":"[PATCH 2/2] wincred: handle empty username/password correctly","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-30T17:20:44Z","receivedAt":"2017-10-30T17:21:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: =?UTF-8?q?Jakub=20Bere=C5=BCa=C5=84ski?= \n\nEmpty (length 0) usernames and/or passwords, when saved in the Windows\nCredential Manager, come back as null when reading the credential.\n\nOne use case for such empty credentials is with NTLM authentication, where\nempty username and password instruct libcurl to authenticate using the\ncredentials of the currently logged-on user (single sign-on).\n\nWhen locating the relevant credentials, make empty username match null.\nWhen outputting the credentials, handle nulls correctly.\n\nSigned-off-by: Jakub Bereżański \n---\n contrib/credential/wincred/git-credential-wincred.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c\nindex 006134043a4..86518cd93d9 100644\n--- a/contrib/credential/wincred/git-credential-wincred.c\n+++ b/contrib/credential/wincred/git-credential-wincred.c\n@@ -94,6 +94,12 @@ static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];\n static void write_item(const char *what, LPCWSTR wbuf, int wlen)\n {\n \tchar *buf;\n+\n+\tif (!wbuf || !wlen) {\n+\t\tprintf(\"%s=\\n\", what);\n+\t\treturn;\n+\t}\n+\n \tint len = WideCharToMultiByte(CP_UTF8, 0, wbuf, wlen, NULL, 0, NULL,\n \t FALSE);\n \tbuf = xmalloc(len);\n@@ -160,7 +166,7 @@ static int match_part_last(LPCWSTR *ptarget, LPCWSTR want, LPCWSTR delim)\n static int match_cred(const CREDENTIALW *cred)\n {\n \tLPCWSTR target = cred->TargetName;\n-\tif (wusername && wcscmp(wusername, cred->UserName))\n+\tif (wusername && wcscmp(wusername, cred->UserName ? cred->UserName : L\"\"))\n \t\treturn 0;\n \n \treturn match_part(&target, L\"git\", L\":\") &&\n@@ -183,7 +189,7 @@ static void get_credential(void)\n \tfor (i = 0; i < num_creds; ++i)\n \t\tif (match_cred(creds[i])) {\n \t\t\twrite_item(\"username\", creds[i]->UserName,\n-\t\t\t\twcslen(creds[i]->UserName));\n+\t\t\t\tcreds[i]->UserName ? wcslen(creds[i]->UserName) : 0);\n \t\t\twrite_item(\"password\",\n \t\t\t\t(LPCWSTR)creds[i]->CredentialBlob,\n \t\t\t\tcreds[i]->CredentialBlobSize / sizeof(WCHAR));\n-- \n2.15.0.windows.1"},{"id":"331401","messageId":"20171030182718.oyzhjffd6jd4wslu@sigill.intra.peff.net","threadId":"47075","inReplyTo":"e055913ec27fc3feb3f017747560f454f6817a0b.1509383993.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH 1/2] t0302: check helper can handle empty credentials","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-30T18:27:18Z","receivedAt":"2017-10-30T18:27:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 30, 2017 at 06:20:12PM +0100, Johannes Schindelin wrote:\n\n> Subject: Re: [PATCH 1/2] t0302: check helper can handle empty credentials\n\nI guess we really care about t0303 here (which tests external helpers).\nThis patch adds the test to lib-credential, so it hits the \"cache\" and\n\"store\" helpers, too. Which seems to pass, so I guess that's OK (I have\nto admit that as the author of those tools, I wasn't sure how they'd\nreact).\n\n> Make sure the helper does not crash when blank username and password is\n> provided. If the helper can save such credentials, it should be able to\n> read them back.\n\nI worry that some third-party helpers might not be able to represent\nthis case and would fail the test. This has been around for years no\nWindows, but probably hasn't ever been run with osxkeychain or\nlibsecret. I'd be OK with taking this as-is, though, and waiting to see\nif anybody complains. At that point we'll know if the right solution is\nenhancing that helper, or providing a way to optionally skip this test.\n\n(Though I have no idea if anybody actually runs t0303 against\ncustom-built helpers in the first place. The process is pretty manual\nfor now, though the Makefiles in contrib/credential could probably at\nleast provide a \"make test\").\n\n-Peff\n"},{"id":"331485","messageId":"alpine.DEB.2.21.1.1710311814260.6482@virtualbox","threadId":"47075","inReplyTo":"20171030182718.oyzhjffd6jd4wslu@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] t0302: check helper can handle empty credentials","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-10-31T17:16:17Z","receivedAt":"2017-10-31T17:16:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Mon, 30 Oct 2017, Jeff King wrote:\n\n> On Mon, Oct 30, 2017 at 06:20:12PM +0100, Johannes Schindelin wrote:\n> \n> > Subject: Re: [PATCH 1/2] t0302: check helper can handle empty credentials\n> \n> I guess we really care about t0303 here (which tests external helpers).\n> This patch adds the test to lib-credential, so it hits the \"cache\" and\n> \"store\" helpers, too. Which seems to pass, so I guess that's OK (I have\n> to admit that as the author of those tools, I wasn't sure how they'd\n> react).\n> \n> > Make sure the helper does not crash when blank username and password is\n> > provided. If the helper can save such credentials, it should be able to\n> > read them back.\n> \n> I worry that some third-party helpers might not be able to represent\n> this case and would fail the test. This has been around for years no\n> Windows, but probably hasn't ever been run with osxkeychain or\n> libsecret. I'd be OK with taking this as-is, though, and waiting to see\n> if anybody complains. At that point we'll know if the right solution is\n> enhancing that helper, or providing a way to optionally skip this test.\n\nOkay. If you change your mind, please let me know, I would try to set\naside some time to adjust the patch in that event.\n\n> (Though I have no idea if anybody actually runs t0303 against\n> custom-built helpers in the first place. The process is pretty manual\n> for now, though the Makefiles in contrib/credential could probably at\n> least provide a \"make test\").\n\nRight... I am not aware of any attempts to run those tests against Git\nCredential Manager for Windows, for example...\n\nCiao,\nDscho\n"}]}