* [PATCH] credential/wincred: store oauth_refresh_token
@ 2023-05-20 7:20 M Hickford via GitGitGadget
2023-06-16 22:30 ` M Hickford
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: M Hickford via GitGitGadget @ 2023-05-20 7:20 UTC (permalink / raw)
To: git; +Cc: M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
a5c7656 (credential: new attribute oauth_refresh_token) introduced
a new confidential credential attribute for helpers to store.
We encode the new attribute in the CredentialBlob, separated by
newline:
hunter2
oauth_refresh_token=xyzzy
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
Alternatives considered: store oauth_refresh_token in a wincred
attribute. This would be insecure because wincred assumes attribute
values to be non-confidential.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/wincred: store oauth_refresh_token
I'm not confident in C and found string manipulation difficult, so
please review carefully.
I tested on Linux, cross compiling with Zig make CC="zig cc -target
x86_64-windows-gnu" and tested make GIT_TEST_CREDENTIAL_HELPER=wincred
t0303-credential-external.sh (with a shell script git-credential-wincred
that wraps wine64 git-credential-wincred.exe "$@")
See also similar patch for credential-libsecret "[PATCH v4]
credential/libsecret: store new attributes"
https://lore.kernel.org/git/pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com/T/#u
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1534
.../wincred/git-credential-wincred.c | 40 ++++++++++++++++---
t/t0303-credential-external.sh | 1 +
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 96f10613aee..dc34283b85e 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
}
static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
- *password_expiry_utc;
+ *password_expiry_utc, *oauth_refresh_token;
static void write_item(const char *what, LPCWSTR wbuf, int wlen)
{
@@ -128,6 +128,11 @@ static void get_credential(void)
DWORD num_creds;
int i;
CREDENTIAL_ATTRIBUTEW *attr;
+ WCHAR *secret;
+ WCHAR *line;
+ WCHAR *remaining_lines;
+ WCHAR *part;
+ WCHAR *remaining_parts;
if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
return;
@@ -137,9 +142,17 @@ static void get_credential(void)
if (match_cred(creds[i])) {
write_item("username", creds[i]->UserName,
creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
- write_item("password",
- (LPCWSTR)creds[i]->CredentialBlob,
- creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ secret = xmalloc(creds[i]->CredentialBlobSize);
+ wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ line = wcstok_s(secret, L"\r\n", &remaining_lines);
+ write_item("password", line, wcslen(line));
+ while(line != NULL) {
+ part = wcstok_s(line, L"=", &remaining_parts);
+ if (!wcscmp(part, L"oauth_refresh_token")) {
+ write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
+ }
+ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
+ }
for (int j = 0; j < creds[i]->AttributeCount; j++) {
attr = creds[i]->Attributes + j;
if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
@@ -148,6 +161,7 @@ static void get_credential(void)
break;
}
}
+ free(secret);
break;
}
@@ -158,16 +172,26 @@ static void store_credential(void)
{
CREDENTIALW cred;
CREDENTIAL_ATTRIBUTEW expiry_attr;
+ WCHAR *secret;
+ int wlen;
if (!wusername || !password)
return;
+ if (oauth_refresh_token) {
+ wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ secret = xmalloc(sizeof(WCHAR) * wlen);
+ _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ } else {
+ secret = _wcsdup(password);
+ }
+
cred.Flags = 0;
cred.Type = CRED_TYPE_GENERIC;
cred.TargetName = target;
cred.Comment = L"saved by git-credential-wincred";
- cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
- cred.CredentialBlob = (LPVOID)password;
+ cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
+ cred.CredentialBlob = (LPVOID)_wcsdup(secret);
cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
cred.AttributeCount = 0;
cred.Attributes = NULL;
@@ -182,6 +206,8 @@ static void store_credential(void)
cred.TargetAlias = NULL;
cred.UserName = wusername;
+ free(secret);
+
if (!CredWriteW(&cred, 0))
die("CredWrite failed");
}
@@ -253,6 +279,8 @@ static void read_credential(void)
password = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "password_expiry_utc"))
password_expiry_utc = utf8_to_utf16_dup(v);
+ else if (!strcmp(buf, "oauth_refresh_token"))
+ oauth_refresh_token = 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
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..51886b8e259 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -45,6 +45,7 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
base-commit: 9e49351c3060e1fa6e0d2de64505b7becf157f28
--
gitgitgadget
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] credential/wincred: store oauth_refresh_token
2023-05-20 7:20 [PATCH] credential/wincred: store oauth_refresh_token M Hickford via GitGitGadget
@ 2023-06-16 22:30 ` M Hickford
[not found] ` <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
2023-11-06 6:16 ` [PATCH v2] " M Hickford via GitGitGadget
2 siblings, 0 replies; 7+ messages in thread
From: M Hickford @ 2023-06-16 22:30 UTC (permalink / raw)
To: M Hickford via GitGitGadget
Cc: git, M Hickford, Matthew John Cheetham, Taylor Blau, Jeff King
On Sat, 20 May 2023 at 08:20, M Hickford via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: M Hickford <mirth.hickford@gmail.com>
>
> a5c7656 (credential: new attribute oauth_refresh_token) introduced
> a new confidential credential attribute for helpers to store.
>
> We encode the new attribute in the CredentialBlob, separated by
> newline:
>
> hunter2
> oauth_refresh_token=xyzzy
>
> This is extensible and backwards compatible. The credential protocol
> already assumes that attribute values do not contain newlines.
>
> Alternatives considered: store oauth_refresh_token in a wincred
> attribute. This would be insecure because wincred assumes attribute
> values to be non-confidential.
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
> credential/wincred: store oauth_refresh_token
>
> I'm not confident in C and found string manipulation difficult, so
> please review carefully.
>
> I tested on Linux, cross compiling with Zig make CC="zig cc -target
> x86_64-windows-gnu" and tested make GIT_TEST_CREDENTIAL_HELPER=wincred
> t0303-credential-external.sh (with a shell script git-credential-wincred
> that wraps wine64 git-credential-wincred.exe "$@")
>
> See also similar patch for credential-libsecret "[PATCH v4]
> credential/libsecret: store new attributes"
> https://lore.kernel.org/git/pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com/T/#u
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1534
>
> .../wincred/git-credential-wincred.c | 40 ++++++++++++++++---
> t/t0303-credential-external.sh | 1 +
> 2 files changed, 35 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> index 96f10613aee..dc34283b85e 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> }
>
> static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> - *password_expiry_utc;
> + *password_expiry_utc, *oauth_refresh_token;
>
> static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> {
> @@ -128,6 +128,11 @@ static void get_credential(void)
> DWORD num_creds;
> int i;
> CREDENTIAL_ATTRIBUTEW *attr;
> + WCHAR *secret;
> + WCHAR *line;
> + WCHAR *remaining_lines;
> + WCHAR *part;
> + WCHAR *remaining_parts;
>
> if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> return;
> @@ -137,9 +142,17 @@ static void get_credential(void)
> if (match_cred(creds[i])) {
> write_item("username", creds[i]->UserName,
> creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> - write_item("password",
> - (LPCWSTR)creds[i]->CredentialBlob,
> - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + secret = xmalloc(creds[i]->CredentialBlobSize);
> + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> + write_item("password", line, wcslen(line));
> + while(line != NULL) {
> + part = wcstok_s(line, L"=", &remaining_parts);
> + if (!wcscmp(part, L"oauth_refresh_token")) {
> + write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
> + }
> + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> + }
> for (int j = 0; j < creds[i]->AttributeCount; j++) {
> attr = creds[i]->Attributes + j;
> if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> @@ -148,6 +161,7 @@ static void get_credential(void)
> break;
> }
> }
> + free(secret);
> break;
> }
>
> @@ -158,16 +172,26 @@ static void store_credential(void)
> {
> CREDENTIALW cred;
> CREDENTIAL_ATTRIBUTEW expiry_attr;
> + WCHAR *secret;
> + int wlen;
>
> if (!wusername || !password)
> return;
>
> + if (oauth_refresh_token) {
> + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + secret = xmalloc(sizeof(WCHAR) * wlen);
> + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + } else {
> + secret = _wcsdup(password);
> + }
> +
> cred.Flags = 0;
> cred.Type = CRED_TYPE_GENERIC;
> cred.TargetName = target;
> cred.Comment = L"saved by git-credential-wincred";
> - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> - cred.CredentialBlob = (LPVOID)password;
> + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> cred.AttributeCount = 0;
> cred.Attributes = NULL;
> @@ -182,6 +206,8 @@ static void store_credential(void)
> cred.TargetAlias = NULL;
> cred.UserName = wusername;
>
> + free(secret);
> +
> if (!CredWriteW(&cred, 0))
> die("CredWrite failed");
> }
> @@ -253,6 +279,8 @@ static void read_credential(void)
> password = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "password_expiry_utc"))
> password_expiry_utc = utf8_to_utf16_dup(v);
> + else if (!strcmp(buf, "oauth_refresh_token"))
> + oauth_refresh_token = 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
> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> index f028fd14182..51886b8e259 100755
> --- a/t/t0303-credential-external.sh
> +++ b/t/t0303-credential-external.sh
> @@ -45,6 +45,7 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
>
> helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> +helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
>
> if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
> say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
>
> base-commit: 9e49351c3060e1fa6e0d2de64505b7becf157f28
> --
> gitgitgadget
Hi. Is anyone familiar enough with Windows CRT to review this patch?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] credential/wincred: store oauth_refresh_token
[not found] ` <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
@ 2023-10-30 8:00 ` M Hickford
0 siblings, 0 replies; 7+ messages in thread
From: M Hickford @ 2023-10-30 8:00 UTC (permalink / raw)
To: M Hickford
Cc: git-for-windows, Taylor Blau, Git Mailing List,
Matthew John Cheetham, Jakub Bereżański, patthoyts
Hi. Is anyone with Windows development experience able to take a look
at this patch to credential-wincred?
https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
https://github.com/gitgitgadget/git/pull/1534
On Thu, 10 Aug 2023 at 20:58, M Hickford <mirth.hickford@gmail.com> wrote:
>
> Hi. A small patch to git-credential-wincred needs review on the Git
> mailing list https://lore.kernel.org/git/pull.1534.git.1684567247339.gitgitgadget@gmail.com/
> . Does anyone here have the expertise to review?
>
> On Sat, 20 May 2023 at 08:20, M Hickford via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > a5c7656 (credential: new attribute oauth_refresh_token) introduced
> > a new confidential credential attribute for helpers to store.
> >
> > We encode the new attribute in the CredentialBlob, separated by
> > newline:
> >
> > hunter2
> > oauth_refresh_token=xyzzy
> >
> > This is extensible and backwards compatible. The credential protocol
> > already assumes that attribute values do not contain newlines.
> >
> > Alternatives considered: store oauth_refresh_token in a wincred
> > attribute. This would be insecure because wincred assumes attribute
> > values to be non-confidential.
> >
> > Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> > ---
> > credential/wincred: store oauth_refresh_token
> >
> > I'm not confident in C and found string manipulation difficult, so
> > please review carefully.
> >
> > I tested on Linux, cross compiling with Zig make CC="zig cc -target
> > x86_64-windows-gnu" and tested make GIT_TEST_CREDENTIAL_HELPER=wincred
> > t0303-credential-external.sh (with a shell script git-credential-wincred
> > that wraps wine64 git-credential-wincred.exe "$@")
> >
> > See also similar patch for credential-libsecret "[PATCH v4]
> > credential/libsecret: store new attributes"
> > https://lore.kernel.org/git/pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com/T/#u
> >
> > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v1
> > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v1
> > Pull-Request: https://github.com/gitgitgadget/git/pull/1534
> >
> > .../wincred/git-credential-wincred.c | 40 ++++++++++++++++---
> > t/t0303-credential-external.sh | 1 +
> > 2 files changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> > index 96f10613aee..dc34283b85e 100644
> > --- a/contrib/credential/wincred/git-credential-wincred.c
> > +++ b/contrib/credential/wincred/git-credential-wincred.c
> > @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> > }
> >
> > static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> > - *password_expiry_utc;
> > + *password_expiry_utc, *oauth_refresh_token;
> >
> > static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> > {
> > @@ -128,6 +128,11 @@ static void get_credential(void)
> > DWORD num_creds;
> > int i;
> > CREDENTIAL_ATTRIBUTEW *attr;
> > + WCHAR *secret;
> > + WCHAR *line;
> > + WCHAR *remaining_lines;
> > + WCHAR *part;
> > + WCHAR *remaining_parts;
> >
> > if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> > return;
> > @@ -137,9 +142,17 @@ static void get_credential(void)
> > if (match_cred(creds[i])) {
> > write_item("username", creds[i]->UserName,
> > creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> > - write_item("password",
> > - (LPCWSTR)creds[i]->CredentialBlob,
> > - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + secret = xmalloc(creds[i]->CredentialBlobSize);
> > + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> > + write_item("password", line, wcslen(line));
> > + while(line != NULL) {
> > + part = wcstok_s(line, L"=", &remaining_parts);
> > + if (!wcscmp(part, L"oauth_refresh_token")) {
> > + write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
> > + }
> > + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> > + }
> > for (int j = 0; j < creds[i]->AttributeCount; j++) {
> > attr = creds[i]->Attributes + j;
> > if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> > @@ -148,6 +161,7 @@ static void get_credential(void)
> > break;
> > }
> > }
> > + free(secret);
> > break;
> > }
> >
> > @@ -158,16 +172,26 @@ static void store_credential(void)
> > {
> > CREDENTIALW cred;
> > CREDENTIAL_ATTRIBUTEW expiry_attr;
> > + WCHAR *secret;
> > + int wlen;
> >
> > if (!wusername || !password)
> > return;
> >
> > + if (oauth_refresh_token) {
> > + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + secret = xmalloc(sizeof(WCHAR) * wlen);
> > + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + } else {
> > + secret = _wcsdup(password);
> > + }
> > +
> > cred.Flags = 0;
> > cred.Type = CRED_TYPE_GENERIC;
> > cred.TargetName = target;
> > cred.Comment = L"saved by git-credential-wincred";
> > - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> > - cred.CredentialBlob = (LPVOID)password;
> > + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> > + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> > cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> > cred.AttributeCount = 0;
> > cred.Attributes = NULL;
> > @@ -182,6 +206,8 @@ static void store_credential(void)
> > cred.TargetAlias = NULL;
> > cred.UserName = wusername;
> >
> > + free(secret);
> > +
> > if (!CredWriteW(&cred, 0))
> > die("CredWrite failed");
> > }
> > @@ -253,6 +279,8 @@ static void read_credential(void)
> > password = utf8_to_utf16_dup(v);
> > else if (!strcmp(buf, "password_expiry_utc"))
> > password_expiry_utc = utf8_to_utf16_dup(v);
> > + else if (!strcmp(buf, "oauth_refresh_token"))
> > + oauth_refresh_token = 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
> > diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> > index f028fd14182..51886b8e259 100755
> > --- a/t/t0303-credential-external.sh
> > +++ b/t/t0303-credential-external.sh
> > @@ -45,6 +45,7 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
> > helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > helper_test "$GIT_TEST_CREDENTIAL_HELPER"
> > +helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
> >
> > if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
> > say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
> >
> > base-commit: 9e49351c3060e1fa6e0d2de64505b7becf157f28
> > --
> > gitgitgadget
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] credential/wincred: store oauth_refresh_token
2023-05-20 7:20 [PATCH] credential/wincred: store oauth_refresh_token M Hickford via GitGitGadget
2023-06-16 22:30 ` M Hickford
[not found] ` <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
@ 2023-11-06 6:16 ` M Hickford via GitGitGadget
2023-11-14 7:40 ` Junio C Hamano
2024-01-28 21:25 ` [PATCH v3] " M Hickford via GitGitGadget
2 siblings, 2 replies; 7+ messages in thread
From: M Hickford via GitGitGadget @ 2023-11-06 6:16 UTC (permalink / raw)
To: git; +Cc: Jeff King, Taylor Blau, Matthew John Cheetham, M Hickford,
M Hickford
From: M Hickford <mirth.hickford@gmail.com>
a5c7656 (credential: new attribute oauth_refresh_token) introduced
a new confidential credential attribute for helpers to store.
We encode the new attribute in the CredentialBlob, separated by
newline:
hunter2
oauth_refresh_token=xyzzy
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
This fixes test "helper (wincred) gets oauth_refresh_token" when
t0303-credential-external.sh is run with
GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
(credential: new attribute oauth_refresh_token, 2023-04-21).
Alternatives considered: store oauth_refresh_token in a wincred
attribute. This would be insecure because wincred assumes attribute
values to be non-confidential.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/wincred: store oauth_refresh_token
Patch v2 fixes a bug when password is empty.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1534
Range-diff vs v1:
1: 46ce22aa3a3 ! 1: 12f092f45b1 credential/wincred: store oauth_refresh_token
@@ Commit message
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
+ This fixes test "helper (wincred) gets oauth_refresh_token" when
+ t0303-credential-external.sh is run with
+ GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
+ (credential: new attribute oauth_refresh_token, 2023-04-21).
+
Alternatives considered: store oauth_refresh_token in a wincred
attribute. This would be insecure because wincred assumes attribute
values to be non-confidential.
@@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(
if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
return;
@@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(void)
- if (match_cred(creds[i])) {
+ if (match_cred(creds[i], 0)) {
write_item("username", creds[i]->UserName,
creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
- write_item("password",
- (LPCWSTR)creds[i]->CredentialBlob,
- creds[i]->CredentialBlobSize / sizeof(WCHAR));
-+ secret = xmalloc(creds[i]->CredentialBlobSize);
-+ wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
-+ line = wcstok_s(secret, L"\r\n", &remaining_lines);
-+ write_item("password", line, wcslen(line));
-+ while(line != NULL) {
-+ part = wcstok_s(line, L"=", &remaining_parts);
-+ if (!wcscmp(part, L"oauth_refresh_token")) {
-+ write_item("oauth_refresh_token", remaining_parts, wcslen(remaining_parts));
++ if (creds[i]->CredentialBlobSize > 0) {
++ secret = xmalloc(creds[i]->CredentialBlobSize);
++ wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
++ line = wcstok_s(secret, L"\r\n", &remaining_lines);
++ write_item("password", line, line ? wcslen(line) : 0);
++ while(line != NULL) {
++ part = wcstok_s(line, L"=", &remaining_parts);
++ if (!wcscmp(part, L"oauth_refresh_token")) {
++ write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
++ }
++ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
+ }
-+ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
++ } else {
++ write_item("password",
++ (LPCWSTR)creds[i]->CredentialBlob,
++ creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ }
for (int j = 0; j < creds[i]->AttributeCount; j++) {
attr = creds[i]->Attributes + j;
@@ contrib/credential/wincred/git-credential-wincred.c: static void read_credential
/*
* Ignore other lines; we don't know what they mean, but
* this future-proofs us when later versions of git do
-
- ## t/t0303-credential-external.sh ##
-@@ t/t0303-credential-external.sh: test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
- helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
-
- helper_test "$GIT_TEST_CREDENTIAL_HELPER"
-+helper_test_oauth_refresh_token "$GIT_TEST_CREDENTIAL_HELPER"
-
- if test -z "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"; then
- say "# skipping timeout tests (GIT_TEST_CREDENTIAL_HELPER_TIMEOUT not set)"
.../wincred/git-credential-wincred.c | 46 ++++++++++++++++---
1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 4cd56c42e24..5c6a7d65d4a 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
}
static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
- *password_expiry_utc;
+ *password_expiry_utc, *oauth_refresh_token;
static void write_item(const char *what, LPCWSTR wbuf, int wlen)
{
@@ -140,6 +140,11 @@ static void get_credential(void)
DWORD num_creds;
int i;
CREDENTIAL_ATTRIBUTEW *attr;
+ WCHAR *secret;
+ WCHAR *line;
+ WCHAR *remaining_lines;
+ WCHAR *part;
+ WCHAR *remaining_parts;
if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
return;
@@ -149,9 +154,23 @@ static void get_credential(void)
if (match_cred(creds[i], 0)) {
write_item("username", creds[i]->UserName,
creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
- write_item("password",
- (LPCWSTR)creds[i]->CredentialBlob,
- creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ if (creds[i]->CredentialBlobSize > 0) {
+ secret = xmalloc(creds[i]->CredentialBlobSize);
+ wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ line = wcstok_s(secret, L"\r\n", &remaining_lines);
+ write_item("password", line, line ? wcslen(line) : 0);
+ while(line != NULL) {
+ part = wcstok_s(line, L"=", &remaining_parts);
+ if (!wcscmp(part, L"oauth_refresh_token")) {
+ write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
+ }
+ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
+ }
+ } else {
+ write_item("password",
+ (LPCWSTR)creds[i]->CredentialBlob,
+ creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ }
for (int j = 0; j < creds[i]->AttributeCount; j++) {
attr = creds[i]->Attributes + j;
if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
@@ -160,6 +179,7 @@ static void get_credential(void)
break;
}
}
+ free(secret);
break;
}
@@ -170,16 +190,26 @@ static void store_credential(void)
{
CREDENTIALW cred;
CREDENTIAL_ATTRIBUTEW expiry_attr;
+ WCHAR *secret;
+ int wlen;
if (!wusername || !password)
return;
+ if (oauth_refresh_token) {
+ wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ secret = xmalloc(sizeof(WCHAR) * wlen);
+ _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ } else {
+ secret = _wcsdup(password);
+ }
+
cred.Flags = 0;
cred.Type = CRED_TYPE_GENERIC;
cred.TargetName = target;
cred.Comment = L"saved by git-credential-wincred";
- cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
- cred.CredentialBlob = (LPVOID)password;
+ cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
+ cred.CredentialBlob = (LPVOID)_wcsdup(secret);
cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
cred.AttributeCount = 0;
cred.Attributes = NULL;
@@ -194,6 +224,8 @@ static void store_credential(void)
cred.TargetAlias = NULL;
cred.UserName = wusername;
+ free(secret);
+
if (!CredWriteW(&cred, 0))
die("CredWrite failed");
}
@@ -265,6 +297,8 @@ static void read_credential(void)
password = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "password_expiry_utc"))
password_expiry_utc = utf8_to_utf16_dup(v);
+ else if (!strcmp(buf, "oauth_refresh_token"))
+ oauth_refresh_token = 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
base-commit: bc5204569f7db44d22477485afd52ea410d83743
--
gitgitgadget
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] credential/wincred: store oauth_refresh_token
2023-11-06 6:16 ` [PATCH v2] " M Hickford via GitGitGadget
@ 2023-11-14 7:40 ` Junio C Hamano
2023-11-24 8:00 ` M Hickford
2024-01-28 21:25 ` [PATCH v3] " M Hickford via GitGitGadget
1 sibling, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2023-11-14 7:40 UTC (permalink / raw)
To: M Hickford via GitGitGadget
Cc: git, Jeff King, Taylor Blau, Matthew John Cheetham, M Hickford
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: M Hickford <mirth.hickford@gmail.com>
>
> a5c7656 (credential: new attribute oauth_refresh_token) introduced
> a new confidential credential attribute for helpers to store.
>
> We encode the new attribute in the CredentialBlob, separated by
> newline:
>
> hunter2
> oauth_refresh_token=xyzzy
>
> This is extensible and backwards compatible. The credential protocol
> already assumes that attribute values do not contain newlines.
>
> This fixes test "helper (wincred) gets oauth_refresh_token" when
> t0303-credential-external.sh is run with
> GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
> (credential: new attribute oauth_refresh_token, 2023-04-21).
>
> Alternatives considered: store oauth_refresh_token in a wincred
> attribute. This would be insecure because wincred assumes attribute
> values to be non-confidential.
Earlier, a5c76569 (credential: new attribute oauth_refresh_token,
2023-04-21) built the "git" side support for the token, and taught
credential-cache to store the necessary information. Then 0ce02e2f
(credential/libsecret: store new attributes, 2023-06-16) was written
for libsecret to support the same interface.
And this one adds corresponding support for wincred. Do I
understand what is going on around this patch correctly?
I do not do Windows, but some people on this list certainly do and
would be capable of giving the patch a necessary nudge ;-)
Thanks.
> .../wincred/git-credential-wincred.c | 46 ++++++++++++++++---
> 1 file changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> index 4cd56c42e24..5c6a7d65d4a 100644
> --- a/contrib/credential/wincred/git-credential-wincred.c
> +++ b/contrib/credential/wincred/git-credential-wincred.c
> @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> }
>
> static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> - *password_expiry_utc;
> + *password_expiry_utc, *oauth_refresh_token;
>
> static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> {
> @@ -140,6 +140,11 @@ static void get_credential(void)
> DWORD num_creds;
> int i;
> CREDENTIAL_ATTRIBUTEW *attr;
> + WCHAR *secret;
> + WCHAR *line;
> + WCHAR *remaining_lines;
> + WCHAR *part;
> + WCHAR *remaining_parts;
>
> if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> return;
> @@ -149,9 +154,23 @@ static void get_credential(void)
> if (match_cred(creds[i], 0)) {
> write_item("username", creds[i]->UserName,
> creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> - write_item("password",
> - (LPCWSTR)creds[i]->CredentialBlob,
> - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + if (creds[i]->CredentialBlobSize > 0) {
> + secret = xmalloc(creds[i]->CredentialBlobSize);
> + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> + write_item("password", line, line ? wcslen(line) : 0);
> + while(line != NULL) {
> + part = wcstok_s(line, L"=", &remaining_parts);
> + if (!wcscmp(part, L"oauth_refresh_token")) {
> + write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
> + }
> + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> + }
> + } else {
> + write_item("password",
> + (LPCWSTR)creds[i]->CredentialBlob,
> + creds[i]->CredentialBlobSize / sizeof(WCHAR));
> + }
> for (int j = 0; j < creds[i]->AttributeCount; j++) {
> attr = creds[i]->Attributes + j;
> if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> @@ -160,6 +179,7 @@ static void get_credential(void)
> break;
> }
> }
> + free(secret);
> break;
> }
>
> @@ -170,16 +190,26 @@ static void store_credential(void)
> {
> CREDENTIALW cred;
> CREDENTIAL_ATTRIBUTEW expiry_attr;
> + WCHAR *secret;
> + int wlen;
>
> if (!wusername || !password)
> return;
>
> + if (oauth_refresh_token) {
> + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + secret = xmalloc(sizeof(WCHAR) * wlen);
> + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> + } else {
> + secret = _wcsdup(password);
> + }
> +
> cred.Flags = 0;
> cred.Type = CRED_TYPE_GENERIC;
> cred.TargetName = target;
> cred.Comment = L"saved by git-credential-wincred";
> - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> - cred.CredentialBlob = (LPVOID)password;
> + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> cred.AttributeCount = 0;
> cred.Attributes = NULL;
> @@ -194,6 +224,8 @@ static void store_credential(void)
> cred.TargetAlias = NULL;
> cred.UserName = wusername;
>
> + free(secret);
> +
> if (!CredWriteW(&cred, 0))
> die("CredWrite failed");
> }
> @@ -265,6 +297,8 @@ static void read_credential(void)
> password = utf8_to_utf16_dup(v);
> else if (!strcmp(buf, "password_expiry_utc"))
> password_expiry_utc = utf8_to_utf16_dup(v);
> + else if (!strcmp(buf, "oauth_refresh_token"))
> + oauth_refresh_token = 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
>
> base-commit: bc5204569f7db44d22477485afd52ea410d83743
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] credential/wincred: store oauth_refresh_token
2023-11-14 7:40 ` Junio C Hamano
@ 2023-11-24 8:00 ` M Hickford
0 siblings, 0 replies; 7+ messages in thread
From: M Hickford @ 2023-11-24 8:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jeff King, M Hickford, M Hickford via GitGitGadget,
Matthew John Cheetham, Taylor Blau, git, patthoyts,
Jakub Bereżański
On Tue, 14 Nov 2023 at 07:41, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: M Hickford <mirth.hickford@gmail.com>
> >
> > a5c7656 (credential: new attribute oauth_refresh_token) introduced
> > a new confidential credential attribute for helpers to store.
> >
> > We encode the new attribute in the CredentialBlob, separated by
> > newline:
> >
> > hunter2
> > oauth_refresh_token=xyzzy
> >
> > This is extensible and backwards compatible. The credential protocol
> > already assumes that attribute values do not contain newlines.
> >
> > This fixes test "helper (wincred) gets oauth_refresh_token" when
> > t0303-credential-external.sh is run with
> > GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
> > (credential: new attribute oauth_refresh_token, 2023-04-21).
> >
> > Alternatives considered: store oauth_refresh_token in a wincred
> > attribute. This would be insecure because wincred assumes attribute
> > values to be non-confidential.
>
> Earlier, a5c76569 (credential: new attribute oauth_refresh_token,
> 2023-04-21) built the "git" side support for the token, and taught
> credential-cache to store the necessary information. Then 0ce02e2f
> (credential/libsecret: store new attributes, 2023-06-16) was written
> for libsecret to support the same interface.
>
> And this one adds corresponding support for wincred. Do I
> understand what is going on around this patch correctly?
Yes, that's right.
>
> I do not do Windows, but some people on this list certainly do and
> would be capable of giving the patch a necessary nudge ;-)
>
> Thanks.
>
> > .../wincred/git-credential-wincred.c | 46 ++++++++++++++++---
> > 1 file changed, 40 insertions(+), 6 deletions(-)
> >
> > diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
> > index 4cd56c42e24..5c6a7d65d4a 100644
> > --- a/contrib/credential/wincred/git-credential-wincred.c
> > +++ b/contrib/credential/wincred/git-credential-wincred.c
> > @@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
> > }
> >
> > static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
> > - *password_expiry_utc;
> > + *password_expiry_utc, *oauth_refresh_token;
> >
> > static void write_item(const char *what, LPCWSTR wbuf, int wlen)
> > {
> > @@ -140,6 +140,11 @@ static void get_credential(void)
> > DWORD num_creds;
> > int i;
> > CREDENTIAL_ATTRIBUTEW *attr;
> > + WCHAR *secret;
> > + WCHAR *line;
> > + WCHAR *remaining_lines;
> > + WCHAR *part;
> > + WCHAR *remaining_parts;
> >
> > if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
> > return;
> > @@ -149,9 +154,23 @@ static void get_credential(void)
> > if (match_cred(creds[i], 0)) {
> > write_item("username", creds[i]->UserName,
> > creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
> > - write_item("password",
> > - (LPCWSTR)creds[i]->CredentialBlob,
> > - creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + if (creds[i]->CredentialBlobSize > 0) {
> > + secret = xmalloc(creds[i]->CredentialBlobSize);
> > + wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + line = wcstok_s(secret, L"\r\n", &remaining_lines);
> > + write_item("password", line, line ? wcslen(line) : 0);
> > + while(line != NULL) {
> > + part = wcstok_s(line, L"=", &remaining_parts);
> > + if (!wcscmp(part, L"oauth_refresh_token")) {
> > + write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
> > + }
> > + line = wcstok_s(NULL, L"\r\n", &remaining_lines);
> > + }
> > + } else {
> > + write_item("password",
> > + (LPCWSTR)creds[i]->CredentialBlob,
> > + creds[i]->CredentialBlobSize / sizeof(WCHAR));
> > + }
> > for (int j = 0; j < creds[i]->AttributeCount; j++) {
> > attr = creds[i]->Attributes + j;
> > if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
> > @@ -160,6 +179,7 @@ static void get_credential(void)
> > break;
> > }
> > }
> > + free(secret);
> > break;
> > }
> >
> > @@ -170,16 +190,26 @@ static void store_credential(void)
> > {
> > CREDENTIALW cred;
> > CREDENTIAL_ATTRIBUTEW expiry_attr;
> > + WCHAR *secret;
> > + int wlen;
> >
> > if (!wusername || !password)
> > return;
> >
> > + if (oauth_refresh_token) {
> > + wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + secret = xmalloc(sizeof(WCHAR) * wlen);
> > + _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
> > + } else {
> > + secret = _wcsdup(password);
> > + }
> > +
> > cred.Flags = 0;
> > cred.Type = CRED_TYPE_GENERIC;
> > cred.TargetName = target;
> > cred.Comment = L"saved by git-credential-wincred";
> > - cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
> > - cred.CredentialBlob = (LPVOID)password;
> > + cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
> > + cred.CredentialBlob = (LPVOID)_wcsdup(secret);
> > cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
> > cred.AttributeCount = 0;
> > cred.Attributes = NULL;
> > @@ -194,6 +224,8 @@ static void store_credential(void)
> > cred.TargetAlias = NULL;
> > cred.UserName = wusername;
> >
> > + free(secret);
> > +
> > if (!CredWriteW(&cred, 0))
> > die("CredWrite failed");
> > }
> > @@ -265,6 +297,8 @@ static void read_credential(void)
> > password = utf8_to_utf16_dup(v);
> > else if (!strcmp(buf, "password_expiry_utc"))
> > password_expiry_utc = utf8_to_utf16_dup(v);
> > + else if (!strcmp(buf, "oauth_refresh_token"))
> > + oauth_refresh_token = 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
> >
> > base-commit: bc5204569f7db44d22477485afd52ea410d83743
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3] credential/wincred: store oauth_refresh_token
2023-11-06 6:16 ` [PATCH v2] " M Hickford via GitGitGadget
2023-11-14 7:40 ` Junio C Hamano
@ 2024-01-28 21:25 ` M Hickford via GitGitGadget
1 sibling, 0 replies; 7+ messages in thread
From: M Hickford via GitGitGadget @ 2024-01-28 21:25 UTC (permalink / raw)
To: git
Cc: Jeff King, Taylor Blau, Matthew John Cheetham, git-for-windows,
Jakub Bereżański, patthoyts, M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
a5c7656 (credential: new attribute oauth_refresh_token) introduced
a new confidential credential attribute and added support to
credential-cache. Later 0ce02e2f (credential/libsecret: store new
attributes, 2023-06-16) added support in credential-libsecret.
To add support in credential-wincred, we encode the new attribute in the
CredentialBlob, separated by newline:
hunter2
oauth_refresh_token=xyzzy
This is extensible and backwards compatible. The credential protocol
already assumes that attribute values do not contain newlines.
This fixes test "helper (wincred) gets oauth_refresh_token" when
t0303-credential-external.sh is run with
GIT_TEST_CREDENTIAL_HELPER=wincred. This test was added in a5c76569e7
(credential: new attribute oauth_refresh_token, 2023-04-21).
Alternatives considered: store oauth_refresh_token in a wincred
attribute. This would be insecure because wincred assumes attribute
values to be non-confidential.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/wincred: store oauth_refresh_token
Patch v3 fixes a maybe-uninitialized warning.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1534%2Fhickford%2Fwincred-refresh-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1534/hickford/wincred-refresh-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1534
Range-diff vs v2:
1: 12f092f45b1 ! 1: a324c3aac41 credential/wincred: store oauth_refresh_token
@@ Commit message
credential/wincred: store oauth_refresh_token
a5c7656 (credential: new attribute oauth_refresh_token) introduced
- a new confidential credential attribute for helpers to store.
+ a new confidential credential attribute and added support to
+ credential-cache. Later 0ce02e2f (credential/libsecret: store new
+ attributes, 2023-06-16) added support in credential-libsecret.
- We encode the new attribute in the CredentialBlob, separated by
- newline:
+ To add support in credential-wincred, we encode the new attribute in the
+ CredentialBlob, separated by newline:
hunter2
oauth_refresh_token=xyzzy
@@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(
+ }
+ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
+ }
++ free(secret);
+ } else {
+ write_item("password",
+ (LPCWSTR)creds[i]->CredentialBlob,
@@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(
for (int j = 0; j < creds[i]->AttributeCount; j++) {
attr = creds[i]->Attributes + j;
if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
-@@ contrib/credential/wincred/git-credential-wincred.c: static void get_credential(void)
- break;
- }
- }
-+ free(secret);
- break;
- }
-
@@ contrib/credential/wincred/git-credential-wincred.c: static void store_credential(void)
{
CREDENTIALW cred;
.../wincred/git-credential-wincred.c | 46 ++++++++++++++++---
1 file changed, 40 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 4cd56c42e24..4be0d58cd89 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -35,7 +35,7 @@ static void *xmalloc(size_t size)
}
static WCHAR *wusername, *password, *protocol, *host, *path, target[1024],
- *password_expiry_utc;
+ *password_expiry_utc, *oauth_refresh_token;
static void write_item(const char *what, LPCWSTR wbuf, int wlen)
{
@@ -140,6 +140,11 @@ static void get_credential(void)
DWORD num_creds;
int i;
CREDENTIAL_ATTRIBUTEW *attr;
+ WCHAR *secret;
+ WCHAR *line;
+ WCHAR *remaining_lines;
+ WCHAR *part;
+ WCHAR *remaining_parts;
if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
return;
@@ -149,9 +154,24 @@ static void get_credential(void)
if (match_cred(creds[i], 0)) {
write_item("username", creds[i]->UserName,
creds[i]->UserName ? wcslen(creds[i]->UserName) : 0);
- write_item("password",
- (LPCWSTR)creds[i]->CredentialBlob,
- creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ if (creds[i]->CredentialBlobSize > 0) {
+ secret = xmalloc(creds[i]->CredentialBlobSize);
+ wcsncpy_s(secret, creds[i]->CredentialBlobSize, (LPCWSTR)creds[i]->CredentialBlob, creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ line = wcstok_s(secret, L"\r\n", &remaining_lines);
+ write_item("password", line, line ? wcslen(line) : 0);
+ while(line != NULL) {
+ part = wcstok_s(line, L"=", &remaining_parts);
+ if (!wcscmp(part, L"oauth_refresh_token")) {
+ write_item("oauth_refresh_token", remaining_parts, remaining_parts ? wcslen(remaining_parts) : 0);
+ }
+ line = wcstok_s(NULL, L"\r\n", &remaining_lines);
+ }
+ free(secret);
+ } else {
+ write_item("password",
+ (LPCWSTR)creds[i]->CredentialBlob,
+ creds[i]->CredentialBlobSize / sizeof(WCHAR));
+ }
for (int j = 0; j < creds[i]->AttributeCount; j++) {
attr = creds[i]->Attributes + j;
if (!wcscmp(attr->Keyword, L"git_password_expiry_utc")) {
@@ -170,16 +190,26 @@ static void store_credential(void)
{
CREDENTIALW cred;
CREDENTIAL_ATTRIBUTEW expiry_attr;
+ WCHAR *secret;
+ int wlen;
if (!wusername || !password)
return;
+ if (oauth_refresh_token) {
+ wlen = _scwprintf(L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ secret = xmalloc(sizeof(WCHAR) * wlen);
+ _snwprintf_s(secret, sizeof(WCHAR) * wlen, wlen, L"%s\r\noauth_refresh_token=%s", password, oauth_refresh_token);
+ } else {
+ secret = _wcsdup(password);
+ }
+
cred.Flags = 0;
cred.Type = CRED_TYPE_GENERIC;
cred.TargetName = target;
cred.Comment = L"saved by git-credential-wincred";
- cred.CredentialBlobSize = (wcslen(password)) * sizeof(WCHAR);
- cred.CredentialBlob = (LPVOID)password;
+ cred.CredentialBlobSize = wcslen(secret) * sizeof(WCHAR);
+ cred.CredentialBlob = (LPVOID)_wcsdup(secret);
cred.Persist = CRED_PERSIST_LOCAL_MACHINE;
cred.AttributeCount = 0;
cred.Attributes = NULL;
@@ -194,6 +224,8 @@ static void store_credential(void)
cred.TargetAlias = NULL;
cred.UserName = wusername;
+ free(secret);
+
if (!CredWriteW(&cred, 0))
die("CredWrite failed");
}
@@ -265,6 +297,8 @@ static void read_credential(void)
password = utf8_to_utf16_dup(v);
else if (!strcmp(buf, "password_expiry_utc"))
password_expiry_utc = utf8_to_utf16_dup(v);
+ else if (!strcmp(buf, "oauth_refresh_token"))
+ oauth_refresh_token = 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
base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
--
gitgitgadget
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-01-28 21:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-20 7:20 [PATCH] credential/wincred: store oauth_refresh_token M Hickford via GitGitGadget
2023-06-16 22:30 ` M Hickford
[not found] ` <CAGJzqskekExeug45Zh-uQ1dpHpKP8HS3obWQyz8B_DzSZiuAwA@mail.gmail.com>
2023-10-30 8:00 ` M Hickford
2023-11-06 6:16 ` [PATCH v2] " M Hickford via GitGitGadget
2023-11-14 7:40 ` Junio C Hamano
2023-11-24 8:00 ` M Hickford
2024-01-28 21:25 ` [PATCH v3] " M Hickford via GitGitGadget
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).