Re: [PATCH] credential/wincred: store password_expiry_utc

[M Hickford <mirth.hickford@gmail.com>]

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