| Message ID | pull.1477.git.git.1679729956620.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | credential/wincred: store password_expiry_utc | expand |
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 >
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
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.
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/
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
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.
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);