Re: [PATCH] credential/wincred: store password_expiry_utc

From: M Hickford <mirth.hickford@gmail.com>
Date: Mar 30, 2023, 05:50 UTC
Message-ID: <CAGJzqs=D8hmcxJKGCcz-NqEQ+QDYgi_aO02fj59kQoHZgiW3OQ@mail.gmail.com>
In-Reply-To: <35e1ebe6-e15b-1712-f030-70ab708740db@gmx.de>

On Tue, 28 Mar 2023 at 13:14, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 24 quoted lines

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

Show 7 quoted lines

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