[PATCH] credential/wincred: store password_expiry_utc

[PATCH] credential/wincred: store password_expiry_utc

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78..9be892610d0 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -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

Re: [PATCH] credential/wincred: store password_expiry_utc

[Johannes Schindelin]

Hi M,

On Sat, 25 Mar 2023, M Hickford via GitGitGadget wrote:

> From: M Hickford <mirth.hickford@gmail.com>
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>     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

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

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

				  ^^^^^^

... here. Note how the return value of `wcscmp()` needs to be non-zero to
enter the conditional block? But `wcscmp()` returns 0 if there are no
differences between the two provided strings.

That's not the only bug, though. While the loop iterates over all of the
attributes, the `attr` variable is unchanged, and always points to the
first attribute. You have to access it as `creds[i]->Attributes[j]`,
though, see e.g.
https://github.com/sandboxie-plus/Sandboxie/blob/f2a357f9222b81e7bdc994e5d9824790a1b5d826/Sandboxie/core/dll/cred.c#L324

So with this patch on top of your patch, it works for me:

-- snip --
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 9be892610d0..1aa840e31a0 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -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;
-- snap --

But I have to wonder: why even bother with `git-wincred`? This credential
helper is so ridiculously limited in its capabilities, it does not even
support any host that is remotely close to safe (no 2FA, no OAuth, just
passwords). So I would be just as happy if I weren't asked to spend my
time to review changes to a credential helper I'd much rather see retired
than actively worked on.

Ciao,
Johannes

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

Re: [PATCH] credential/wincred: store password_expiry_utc

[M Hickford]

On Tue, 28 Mar 2023 at 13:14, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> And the reason is...
>
> > @@ -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")) {
>
>                                   ^^^^^^
>
> ... here. Note how the return value of `wcscmp()` needs to be non-zero to
> enter the conditional block? But `wcscmp()` returns 0 if there are no
> differences between the two provided strings.
>
> That's not the only bug, though. While the loop iterates over all of the
> attributes, the `attr` variable is unchanged, and always points to the
> first attribute. You have to access it as `creds[i]->Attributes[j]`,
> though, see e.g.
> https://github.com/sandboxie-plus/Sandboxie/blob/f2a357f9222b81e7bdc994e5d9824790a1b5d826/Sandboxie/core/dll/cred.c#L324
>
> So with this patch on top of your patch, it works for me:
>

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

>
> But I have to wonder: why even bother with `git-wincred`? This credential
> helper is so ridiculously limited in its capabilities, it does not even
> support any host that is remotely close to safe (no 2FA, no OAuth, just
> passwords). So I would be just as happy if I weren't asked to spend my
> time to review changes to a credential helper I'd much rather see retired
> than actively worked on.

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

[PATCH v2] credential/wincred: store password_expiry_utc

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

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 <mirth.hickford@gmail.com>
---
    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 <mirth.hickford@gmail.com>
      
       ## 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(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78..7b4e7fae675 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -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

Re: [PATCH v2] credential/wincred: store password_expiry_utc

[Junio C Hamano]

"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> 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

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.

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

I cannot be one to help testing but ...

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

... this is a nice one.  Logically it may belong to a separate
topic, but it is small and obvious enough that it is OK to do as a
"while at it" clean-up.

Re: [PATCH v2] credential/wincred: store password_expiry_utc

[M Hickford]

On Thu, 30 Mar 2023 at 20:20, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > 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
>
> 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.
>

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

[PATCH v3] credential/wincred: store password_expiry_utc

[M Hickford via GitGitGadget]

From: M Hickford <mirth.hickford@gmail.com>

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.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    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.
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      


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

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index ead6e267c78..7b4e7fae675 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -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

Re: [PATCH] credential/wincred: store password_expiry_utc

[Junio C Hamano]

M Hickford <mirth.hickford@gmail.com> writes:

> Thanks Johannes for the review and the fix. I'll include it in any patch v2.
>
>> But I have to wonder: why even bother with `git-wincred`? This credential
>> helper is so ridiculously limited in its capabilities, it does not even
>> support any host that is remotely close to safe (no 2FA, no OAuth, just
>> passwords). So I would be just as happy if I weren't asked to spend my
>> time to review changes to a credential helper I'd much rather see retired
>> than actively worked on.
>
> 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).

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.

Re: [PATCH] credential/wincred: store password_expiry_utc

[M Hickford]

On Mon, 1 May 2023 at 23:25, Junio C Hamano <gitster@pobox.com> wrote:
>
> M Hickford <mirth.hickford@gmail.com> writes:
>
> > Thanks Johannes for the review and the fix. I'll include it in any patch v2.
> >
> >> But I have to wonder: why even bother with `git-wincred`? This credential
> >> helper is so ridiculously limited in its capabilities, it does not even
> >> support any host that is remotely close to safe (no 2FA, no OAuth, just
> >> passwords). So I would be just as happy if I weren't asked to spend my
> >> time to review changes to a credential helper I'd much rather see retired
> >> than actively worked on.
> >
> > 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).
>
> 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/

Re: [PATCH] credential/wincred: store password_expiry_utc

[Johannes Sixt]

Am 02.05.23 um 00:25 schrieb Junio C Hamano:
> I am wondering if the lack
> of comments is a good news or no interest.

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

-- Hannes

Re: [PATCH] credential/wincred: store password_expiry_utc

[Felipe Contreras]

Johannes Sixt wrote:
> Am 02.05.23 um 00:25 schrieb Junio C Hamano:
> > I am wondering if the lack
> > of comments is a good news or no interest.
> 
> So far, I have totally negated the existence of credential helpers. I
> can't offer any help in this regard, I am afraid.

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

-- 
Felipe Contreras