Thread (5 messages) 5 messages, 3 authors, 2023-04-03

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help