Re: [PATCH] credential/wincred: store password_expiry_utc
From: M Hickford <hidden>
Date: 2023-03-30 05:51:14
On Tue, 28 Mar 2023 at 13:14, Johannes Schindelin [off-list ref] wrote:
And the reason is...quoted
@@ -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