credential/wincred: store password_expiry_utc

From: M Hickford <[email protected]>

---
credential/wincred: store password_expiry_utc

Help wanted from a Windows user to test. I tried testing on Linux with Wine after cross-compiling [1] but Wine has incomplete support for wincred.h [2]. To test:

cd contrib/credential/wincred/
make
echo host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000 | ./git-credential-wincred.exe store echo host=example.com\nprotocol=https | ./git-credential-wincred.exe get

Output of second command should include line password_expiry_utc=2000

[1] make CC="zig cc -target x86_64-windows-gnu" [2]
https://github.com/wine-mirror/wine/blob/bf9d15e3b1a29f73fedda0c34547a9b29d5e2789/dlls/advapi32/cred.c#L181-L186

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1477%2Fhickford%2Fwincred-expiry-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1477/hickford/wincred-expiry-v1
Pull-Request: https://github.com/git/git/pull/1477

 .../wincred/git-credential-wincred.c          | 26 +++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

index ead6e267c78..9be892610d0 100644
@@ -91,7 +91,8 @@ static void load_cred_funcs(void)
 		die("failed to load functions");
 }
 
-static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
+static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
+	*password_expiry_utc;
 
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {

@@ -183,6 +184,7 @@ static void get_credential(void)
 	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;
+	CREDENTIAL_ATTRIBUTEW *attr;
 
 	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
 		return;

@@ -195,6 +197,15 @@ static void get_credential(void)
 			write_item("password",
 				(LPCWSTR)creds[i]->CredentialBlob,
 				creds[i]->CredentialBlobSize / sizeof(WCHAR));
+			attr = creds[i]->Attributes;
+			for (int j = 0; j < creds[i]->AttributeCount; j++) {
+				if (wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
+					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
+					attr->ValueSize / sizeof(WCHAR));
+					break;
+				}
+				attr++;
+			}
 			break;
 		}
 

@@ -204,6 +215,7 @@ static void get_credential(void)
 static void store_credential(void)
 {
 	CREDENTIALW cred;
+	CREDENTIAL_ATTRIBUTEW expiry_attr;
 
 	if (!wusername || !password)
 		return;

@@ -217,6 +229,14 @@ static void store_credential(void)
 	cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
 	cred.AttributeCount = 0;
 	cred.Attributes = NULL;
+	if (password_expiry_utc != NULL) {
+		expiry_attr.Keyword = L"git_password_expiry_utc";
+		expiry_attr.Value = (LPVOID)password_expiry_utc;
+		expiry_attr.ValueSize = (wcslen(password_expiry_utc)) * sizeof(WCHAR);
+		expiry_attr.Flags = 0;
+		cred.Attributes = &expiry_attr;
+		cred.AttributeCount = 1;
+	}
 	cred.TargetAlias = NULL;
 	cred.UserName = wusername;
 

@@ -278,6 +298,8 @@ static void read_credential(void)
 			wusername = utf8_to_utf16_dup(v);
 		} else if (!strcmp(buf, "password"))
 			password = utf8_to_utf16_dup(v);
+		else if (!strcmp(buf, "password_expiry_utc"))
+			password_expiry_utc = utf8_to_utf16_dup(v);
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do

@@ -292,7 +314,7 @@ int main(int argc, char *argv[])
 	    "usage: git credential-wincred <get|store|erase>\n";
 
 	if (!argv[1])
-		die(usage);
+		die("%s", usage);
 
 	/* git use binary pipes to avoid CRLF-issues */
 	_setmode(_fileno(stdin), _O_BINARY);

base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e

gitgitgadget

Hi M,

Sadly, no, it's empty:

	$ cd contrib/credential/wincred/
	$ make
	cc     git-credential-wincred.c   -o git-credential-wincred.exe
	$ echo host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000 | ./git-credential-wincred.exe store
	$ echo host=example.com\nprotocol=https | ./git-credential-wincred.exe get

The reason is that echo does not interpret the escaped n, it does not even get the backslash because Bash eats it first.

But even with printf it does not work:

	$ printf 'host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000\n' | ./git-credential-wincred.exe store
	$ printf 'host=example.com\nprotocol=https\n' | ./git-credential-wincred.exe get                                        username=tim
	password=xyzzy

And the reason is...

				  ^^^^^^

index 9be892610d0..1aa840e31a0 100644
@@ -197,9 +197,9 @@ static void get_credential(void)
 			write_item("password",
 				(LPCWSTR)creds[i]->CredentialBlob,
 				creds[i]->CredentialBlobSize / sizeof(WCHAR));
-			attr = creds[i]->Attributes;
 			for (int j = 0; j < creds[i]->AttributeCount; j++) {
-				if (wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
+				attr = creds[i]->Attributes + j;
+				if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
 					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
 					attr->ValueSize / sizeof(WCHAR));
 					break;

Thanks Johannes for the review and the fix. I'll include it in any patch v2.

git-credential-wincred has the same capabilities as popular helpers git-credential-cache, git-credential-store, git-credential-osxkeychain and git-credential-libsecret. Any of which can store OAuth credentials generated by a helper such as git-credential-oauth [1]. This is compatible with 2FA (any 2FA happens in browser). Example config:

[credential]
helper = wincred
helper = oauth

This patch to store password_expiry_utc is necessary to avoid Git trying to use OAuth credentials beyond expiry. See
https://github.com/git/git/commit/d208bfdfef97a1e8fb746763b5057e0ad91e283b
for background (I'll add to commit message v2).

What about Git Credential Manager? GCM has a similar need to store password expiry, see comment
https://github.com/git-ecosystem/git-credential-manager/discussions/1169#discussioncomment-5472096.

I think OAuth is important enough to fix this issue in both git-credential-wincred and GCM. Some users might prefer the above setup over GCM to avoid .NET dependency or if they really like git-credential-oauth.

Note that OAuth expiry issues don't occur authenticating to GitHub because GitHub doesn't populate OAuth expiry.

[1] https://github.com/hickford/git-credential-oauth

From: M Hickford <[email protected]>

This attribute is important when storing OAuth credentials which may expire after as little as one hour. See
https://github.com/git/git/commit/d208bfdfef97a1e8fb746763b5057e0ad91e283b

---
credential/wincred: store password_expiry_utc

Help wanted from a Windows user to test. I tried testing on Linux with Wine after cross-compiling [1] but Wine has incomplete support for wincred.h [2]. To test:

cd contrib/credential/wincred/
make
echo host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000 | ./git-credential-wincred.exe store echo host=example.com\nprotocol=https | ./git-credential-wincred.exe get

Output of second command should include line password_expiry_utc=2000

[1] make CC="zig cc -target x86_64-windows-gnu" [2]
https://github.com/wine-mirror/wine/blob/bf9d15e3b1a29f73fedda0c34547a9b29d5e2789/dlls/advapi32/cred.c#L181-L186

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1477%2Fhickford%2Fwincred-expiry-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1477/hickford/wincred-expiry-v2
Pull-Request: https://github.com/git/git/pull/1477

Range-diff vs v1:

1: 05340715ef2 ! 1: 51a9039bd15 credential/wincred: store password_expiry_utc

     @@ Metadata
       ## Commit message ##
          credential/wincred: store password_expiry_utc
      
     +    This attribute is important when storing OAuth credentials which may
     +    expire after as little as one hour. See
     +    https://github.com/git/git/commit/d208bfdfef97a1e8fb746763b5057e0ad91e283b
     +
          Signed-off-by: M Hickford <[email protected]>
      
       ## contrib/credential/wincred/git-credential-wincred.c ##
     @@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(
       			write_item("password",
       				(LPCWSTR)creds[i]->CredentialBlob,
       				creds[i]->CredentialBlobSize / sizeof(WCHAR));
     -+			attr = creds[i]->Attributes;
      +			for (int j = 0; j < creds[i]->AttributeCount; j++) {
     -+				if (wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
     ++				attr = creds[i]->Attributes + j;
     ++				if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
      +					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
      +					attr->ValueSize / sizeof(WCHAR));
      +					break;
      +				}
     -+				attr++;
      +			}
       			break;
       		}

 .../wincred/git-credential-wincred.c          | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

index ead6e267c78..7b4e7fae675 100644
@@ -91,7 +91,8 @@ static void load_cred_funcs(void)
 		die("failed to load functions");
 }
 
-static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
+static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
+	*password_expiry_utc;
 
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {

@@ -183,6 +184,7 @@ static void get_credential(void)
 	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;
+	CREDENTIAL_ATTRIBUTEW *attr;
 
 	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
 		return;

@@ -195,6 +197,14 @@ static void get_credential(void)
 			write_item("password",
 				(LPCWSTR)creds[i]->CredentialBlob,
 				creds[i]->CredentialBlobSize / sizeof(WCHAR));
+			for (int j = 0; j < creds[i]->AttributeCount; j++) {
+				attr = creds[i]->Attributes + j;
+				if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
+					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
+					attr->ValueSize / sizeof(WCHAR));
+					break;
+				}
+			}
 			break;
 		}
 

@@ -204,6 +214,7 @@ static void get_credential(void)
 static void store_credential(void)
 {
 	CREDENTIALW cred;
+	CREDENTIAL_ATTRIBUTEW expiry_attr;
 
 	if (!wusername || !password)
 		return;

@@ -217,6 +228,14 @@ static void store_credential(void)
 	cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
 	cred.AttributeCount = 0;
 	cred.Attributes = NULL;
+	if (password_expiry_utc != NULL) {
+		expiry_attr.Keyword = L"git_password_expiry_utc";
+		expiry_attr.Value = (LPVOID)password_expiry_utc;
+		expiry_attr.ValueSize = (wcslen(password_expiry_utc)) * sizeof(WCHAR);
+		expiry_attr.Flags = 0;
+		cred.Attributes = &expiry_attr;
+		cred.AttributeCount = 1;
+	}
 	cred.TargetAlias = NULL;
 	cred.UserName = wusername;
 

@@ -278,6 +297,8 @@ static void read_credential(void)
 			wusername = utf8_to_utf16_dup(v);
 		} else if (!strcmp(buf, "password"))
 			password = utf8_to_utf16_dup(v);
+		else if (!strcmp(buf, "password_expiry_utc"))
+			password_expiry_utc = utf8_to_utf16_dup(v);
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do

@@ -292,7 +313,7 @@ int main(int argc, char *argv[])
 	    "usage: git credential-wincred <get|store|erase>\n";
 
 	if (!argv[1])
-		die(usage);
+		die("%s", usage);
 
 	/* git use binary pipes to avoid CRLF-issues */
 	_setmode(_fileno(stdin), _O_BINARY);

base-commit: 8d90352acc5c855620042fdcc6092f23a276af6d

gitgitgadget

Readers do not have to visit GitHub at all, and proposed log message shouldn't force them to. Refer to an existing commit in this project like so instead:

... as one hour. d208bfdf (credential: new attribute password_expiry_utc, 2023-02-18) added support for this attribute in general so that individual credential backend like wincred can use it.

I cannot be one to help testing but ...

Thanks Junio for the example. I'll update the commit message in patch v3.

From: M Hickford <[email protected]>

This attribute is important when storing OAuth credentials which may expire after as little as one hour. d208bfdf (credential: new attribute password_expiry_utc, 2023-02-18) added support for this attribute in general so that individual credential backend like wincred can use it.

---
credential/wincred: store password_expiry_utc

Windows users can test with the following commands:

cd contrib/credential/wincred/ make printf 'host=example.com\nprotocol=https\nusername=tim\npassword=xyzzy\npassword_expiry_utc=2000\n' | ./git-credential-wincred.exe store printf 'host=example.com\nprotocol=https\n' | ./git-credential-wincred.exe get

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1477%2Fhickford%2Fwincred-expiry-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1477/hickford/wincred-expiry-v3
Pull-Request: https://github.com/git/git/pull/1477

Range-diff vs v2:

1: 51a9039bd15 ! 1: d2dff063eb4 credential/wincred: store password_expiry_utc
@@ Commit message
credential/wincred: store password_expiry_utc

This attribute is important when storing OAuth credentials which may
- expire after as little as one hour. See
- https://github.com/git/git/commit/d208bfdfef97a1e8fb746763b5057e0ad91e283b
+ expire after as little as one hour. d208bfdf (credential: new attribute
+ password_expiry_utc, 2023-02-18) added support for this attribute in
+ general so that individual credential backend like wincred can use it.

 .../wincred/git-credential-wincred.c          | 25 +++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

index ead6e267c78..7b4e7fae675 100644
@@ -91,7 +91,8 @@ static void load_cred_funcs(void)
 		die("failed to load functions");
 }
 
-static WCHAR *wusername, *password, *protocol, *host, *path, target[1024];
+static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
+	*password_expiry_utc;
 
 static void write_item(const char *what, LPCWSTR wbuf, int wlen)
 {

@@ -183,6 +184,7 @@ static void get_credential(void)
 	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;
+	CREDENTIAL_ATTRIBUTEW *attr;
 
 	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
 		return;

@@ -195,6 +197,14 @@ static void get_credential(void)
 			write_item("password",
 				(LPCWSTR)creds[i]->CredentialBlob,
 				creds[i]->CredentialBlobSize / sizeof(WCHAR));
+			for (int j = 0; j < creds[i]->AttributeCount; j++) {
+				attr = creds[i]->Attributes + j;
+				if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
+					write_item("password_expiry_utc", (LPCWSTR)attr->Value,
+					attr->ValueSize / sizeof(WCHAR));
+					break;
+				}
+			}
 			break;
 		}
 

@@ -204,6 +214,7 @@ static void get_credential(void)
 static void store_credential(void)
 {
 	CREDENTIALW cred;
+	CREDENTIAL_ATTRIBUTEW expiry_attr;
 
 	if (!wusername || !password)
 		return;

@@ -217,6 +228,14 @@ static void store_credential(void)
 	cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
 	cred.AttributeCount = 0;
 	cred.Attributes = NULL;
+	if (password_expiry_utc != NULL) {
+		expiry_attr.Keyword = L"git_password_expiry_utc";
+		expiry_attr.Value = (LPVOID)password_expiry_utc;
+		expiry_attr.ValueSize = (wcslen(password_expiry_utc)) * sizeof(WCHAR);
+		expiry_attr.Flags = 0;
+		cred.Attributes = &expiry_attr;
+		cred.AttributeCount = 1;
+	}
 	cred.TargetAlias = NULL;
 	cred.UserName = wusername;
 

@@ -278,6 +297,8 @@ static void read_credential(void)
 			wusername = utf8_to_utf16_dup(v);
 		} else if (!strcmp(buf, "password"))
 			password = utf8_to_utf16_dup(v);
+		else if (!strcmp(buf, "password_expiry_utc"))
+			password_expiry_utc = utf8_to_utf16_dup(v);
 		/*
 		 * Ignore other lines; we don't know what they mean, but
 		 * this future-proofs us when later versions of git do

@@ -292,7 +313,7 @@ int main(int argc, char *argv[])
 	    "usage: git credential-wincred <get|store|erase>\n";
 
 	if (!argv[1])
-		die(usage);
+		die("%s", usage);
 
 	/* git use binary pipes to avoid CRLF-issues */
 	_setmode(_fileno(stdin), _O_BINARY);

base-commit: 8d90352acc5c855620042fdcc6092f23a276af6d

gitgitgadget

So, even though earlier Dscho sounded negative on extending wincred helper, are we now on track of enhancing its capabilities? The v3 is now queued in my tree and nobody who knows Windows seem to have made any comments on either v2 or v3---I am wondering if the lack of comments is a good news or no interest.

Thanks.

Thanks to Johannes's fixes for v1, the latest patch should be correct, but it would be prudent to wait for a Windows user to test.

The utility of storing password_expiry_utc is universal to all credential helpers. The latest commit message references the introduction of this attribute (d208bfdfef97a1e8fb746763b5057e0ad91e283b) for background. I repeat the arguments in [1], I hope they are persuasive.

[1] https://lore.kernel.org/git/CAGJzqs=D8hmcxJKGCcz-NqEQ+QDYgi_aO02fj59kQoHZgiW3OQ@mail.gmail.com/

Am 02.05.23 um 00:25 schrieb Junio C Hamano:

So far, I have totally negated the existence of credential helpers. I can't offer any help in this regard, I am afraid.

-- Hannes

FWIW the same here. I exclusively use ssh with gnome-keyring and that works perfectly fine.

Felipe Contreras