* [PATCH] credential/libsecret: support password_expiry_utc
@ 2023-03-14 21:32 M Hickford via GitGitGadget
2023-03-25 7:36 ` [PATCH v2] " M Hickford via GitGitGadget
0 siblings, 1 reply; 11+ messages in thread
From: M Hickford via GitGitGadget @ 2023-03-14 21:32 UTC (permalink / raw)
To: git
Cc: Jeff King, Dennis Kaarsemaker, Mantas Mikulėnas, M Hickford,
M Hickford
From: M Hickford <mirth.hickford@gmail.com>
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/libsecret: store password_expiry_utc
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v1
Pull-Request: https://github.com/git/git/pull/1469
.../libsecret/git-credential-libsecret.c | 42 ++++++++++++++++---
1 file changed, 37 insertions(+), 5 deletions(-)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 2c5d76d789f..3f2b530db79 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,7 @@ struct credential {
char *path;
char *username;
char *password;
+ char *password_expiry_utc;
};
#define CREDENTIAL_INIT { 0 }
@@ -54,6 +55,20 @@ struct credential_operation {
/* ----------------- Secret Service functions ----------------- */
+static const SecretSchema schema = {
+ "org.git.Password",
+ SECRET_SCHEMA_NONE,
+ {
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { NULL, 0 },
+ }
+};
+
static char *make_label(struct credential *c)
{
if (c->port)
@@ -78,6 +93,9 @@ static GHashTable *make_attr_list(struct credential *c)
g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
if (c->path)
g_hash_table_insert(al, "object", g_strdup(c->path));
+ if (c->password_expiry_utc)
+ g_hash_table_insert(al, "password_expiry_utc",
+ g_strdup(c->password_expiry_utc));
return al;
}
@@ -101,9 +119,11 @@ static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
items = secret_service_search_sync(service,
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
- SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
+ // for backwards compatibility
+ SECRET_SCHEMA_DONT_MATCH_NAME,
NULL,
&error);
g_hash_table_unref(attributes);
@@ -128,6 +148,12 @@ static int keyring_get(struct credential *c)
c->username = g_strdup(s);
}
+ s = g_hash_table_lookup(attributes, "password_expiry_utc");
+ if (s) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(s);
+ }
+
s = secret_value_get_text(secret);
if (s) {
g_free(c->password);
@@ -162,7 +188,7 @@ static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
@@ -198,7 +224,7 @@ static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
attributes = make_attr_list(c);
- secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_clearv_sync(&schema,
attributes,
NULL,
&error);
@@ -238,6 +264,7 @@ static void credential_clear(struct credential *c)
g_free(c->path);
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
credential_init(c);
}
@@ -285,6 +312,9 @@ static int credential_read(struct credential *c)
} else if (!strcmp(key, "username")) {
g_free(c->username);
c->username = g_strdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(value);
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
@@ -312,9 +342,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
static void credential_write(const struct credential *c)
{
- /* only write username/password, if set */
+ /* only write username/password/expiry, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
}
static void usage(const char *name)
base-commit: 73876f4861cd3d187a4682290ab75c9dccadbc56
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2] credential/libsecret: support password_expiry_utc
2023-03-14 21:32 [PATCH] credential/libsecret: support password_expiry_utc M Hickford via GitGitGadget
@ 2023-03-25 7:36 ` M Hickford via GitGitGadget
2023-05-04 17:42 ` Junio C Hamano
2023-05-05 7:04 ` [PATCH v3] " M Hickford via GitGitGadget
0 siblings, 2 replies; 11+ messages in thread
From: M Hickford via GitGitGadget @ 2023-03-25 7:36 UTC (permalink / raw)
To: git
Cc: Jeff King, Dennis Kaarsemaker, Mantas Mikulėnas,
Javier Roucher Iglesias, Matthieu Moy, M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/libsecret: store password_expiry_utc
Patch v2 adds tests.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v2
Pull-Request: https://github.com/git/git/pull/1469
Range-diff vs v1:
1: 46ba4863f33 ! 1: 1e27677b6f5 credential/libsecret: support password_expiry_utc
@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_
}
static void usage(const char *name)
+
+ ## t/lib-credential.sh ##
+@@ t/lib-credential.sh: helper_test_clean() {
+ reject $1 https example.com store-user
+ reject $1 https example.com user1
+ reject $1 https example.com user2
++ reject $1 https example.com user3
+ reject $1 http path.tld user
+ reject $1 https timeout.tld user
+ reject $1 https sso.tld
+@@ t/lib-credential.sh: helper_test_timeout() {
+ '
+ }
+
++helper_test_password_expiry_utc() {
++ HELPER=$1
++
++ test_expect_success "helper ($HELPER) stores password_expiry_utc" '
++ check approve $HELPER <<-\EOF
++ protocol=https
++ host=example.com
++ username=user3
++ password=pass
++ password_expiry_utc=9999999999
++ EOF
++ '
++
++ test_expect_success "helper ($HELPER) gets password_expiry_utc" '
++ check fill $HELPER <<-\EOF
++ protocol=https
++ host=example.com
++ username=user3
++ --
++ protocol=https
++ host=example.com
++ username=user3
++ password=pass
++ password_expiry_utc=9999999999
++ --
++ EOF
++ '
++}
++
+ write_script askpass <<\EOF
+ echo >&2 askpass: $*
+ what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
+
+ ## t/t0301-credential-cache.sh ##
+@@ t/t0301-credential-cache.sh: test_atexit 'git credential-cache exit'
+ # test that the daemon works with no special setup
+ helper_test cache
+
++helper_test_password_expiry_utc cache
++
+ test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
+ test_when_finished "
+ git credential-cache exit &&
+
+ ## t/t0303-credential-external.sh ##
+@@ t/t0303-credential-external.sh: else
+ helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
+ fi
+
++helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
++
+ # clean afterwards so that we are good citizens
+ # and don't leave cruft in the helper's storage, which
+ # might be long-term system storage
.../libsecret/git-credential-libsecret.c | 42 ++++++++++++++++---
t/lib-credential.sh | 30 +++++++++++++
t/t0301-credential-cache.sh | 2 +
t/t0303-credential-external.sh | 2 +
4 files changed, 71 insertions(+), 5 deletions(-)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 2c5d76d789f..3f2b530db79 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,7 @@ struct credential {
char *path;
char *username;
char *password;
+ char *password_expiry_utc;
};
#define CREDENTIAL_INIT { 0 }
@@ -54,6 +55,20 @@ struct credential_operation {
/* ----------------- Secret Service functions ----------------- */
+static const SecretSchema schema = {
+ "org.git.Password",
+ SECRET_SCHEMA_NONE,
+ {
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { NULL, 0 },
+ }
+};
+
static char *make_label(struct credential *c)
{
if (c->port)
@@ -78,6 +93,9 @@ static GHashTable *make_attr_list(struct credential *c)
g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
if (c->path)
g_hash_table_insert(al, "object", g_strdup(c->path));
+ if (c->password_expiry_utc)
+ g_hash_table_insert(al, "password_expiry_utc",
+ g_strdup(c->password_expiry_utc));
return al;
}
@@ -101,9 +119,11 @@ static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
items = secret_service_search_sync(service,
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
- SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
+ // for backwards compatibility
+ SECRET_SCHEMA_DONT_MATCH_NAME,
NULL,
&error);
g_hash_table_unref(attributes);
@@ -128,6 +148,12 @@ static int keyring_get(struct credential *c)
c->username = g_strdup(s);
}
+ s = g_hash_table_lookup(attributes, "password_expiry_utc");
+ if (s) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(s);
+ }
+
s = secret_value_get_text(secret);
if (s) {
g_free(c->password);
@@ -162,7 +188,7 @@ static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
@@ -198,7 +224,7 @@ static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
attributes = make_attr_list(c);
- secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_clearv_sync(&schema,
attributes,
NULL,
&error);
@@ -238,6 +264,7 @@ static void credential_clear(struct credential *c)
g_free(c->path);
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
credential_init(c);
}
@@ -285,6 +312,9 @@ static int credential_read(struct credential *c)
} else if (!strcmp(key, "username")) {
g_free(c->username);
c->username = g_strdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(value);
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
@@ -312,9 +342,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
static void credential_write(const struct credential *c)
{
- /* only write username/password, if set */
+ /* only write username/password/expiry, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
}
static void usage(const char *name)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 5ea8bc9f1dc..9ebf7eeae48 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -43,6 +43,7 @@ helper_test_clean() {
reject $1 https example.com store-user
reject $1 https example.com user1
reject $1 https example.com user2
+ reject $1 https example.com user3
reject $1 http path.tld user
reject $1 https timeout.tld user
reject $1 https sso.tld
@@ -298,6 +299,35 @@ helper_test_timeout() {
'
}
+helper_test_password_expiry_utc() {
+ HELPER=$1
+
+ test_expect_success "helper ($HELPER) stores password_expiry_utc" '
+ check approve $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ EOF
+ '
+
+ test_expect_success "helper ($HELPER) gets password_expiry_utc" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ --
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ --
+ EOF
+ '
+}
+
write_script askpass <<\EOF
echo >&2 askpass: $*
what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 698b7159f03..f5ba727e53b 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -30,6 +30,8 @@ test_atexit 'git credential-cache exit'
# test that the daemon works with no special setup
helper_test cache
+helper_test_password_expiry_utc cache
+
test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
test_when_finished "
git credential-cache exit &&
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..f1478680bff 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -52,6 +52,8 @@ else
helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
fi
+helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
+
# clean afterwards so that we are good citizens
# and don't leave cruft in the helper's storage, which
# might be long-term system storage
base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] credential/libsecret: support password_expiry_utc
2023-03-25 7:36 ` [PATCH v2] " M Hickford via GitGitGadget
@ 2023-05-04 17:42 ` Junio C Hamano
2023-05-05 7:00 ` M Hickford
2023-05-05 7:04 ` [PATCH v3] " M Hickford via GitGitGadget
1 sibling, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2023-05-04 17:42 UTC (permalink / raw)
To: M Hickford via GitGitGadget
Cc: git, Jeff King, Dennis Kaarsemaker, Mantas Mikulėnas,
Javier Roucher Iglesias, Matthieu Moy, M Hickford
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: M Hickford <mirth.hickford@gmail.com>
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
> index 2c5d76d789f..3f2b530db79 100644
> --- a/contrib/credential/libsecret/git-credential-libsecret.c
> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
> @@ -39,6 +39,7 @@ struct credential {
> char *path;
> char *username;
> char *password;
> + char *password_expiry_utc;
> };
>
> #define CREDENTIAL_INIT { 0 }
> @@ -54,6 +55,20 @@ struct credential_operation {
>
> /* ----------------- Secret Service functions ----------------- */
>
> +static const SecretSchema schema = {
> + "org.git.Password",
> + SECRET_SCHEMA_NONE,
> + {
> + { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
> + { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
> + { NULL, 0 },
> + }
> +};
We used to use the bog-standard "COMPAT_NETWORK" but now we are
adding an extra element, and that makes it necessary to define our
own? OK.
> static char *make_label(struct credential *c)
> {
> if (c->port)
> @@ -78,6 +93,9 @@ static GHashTable *make_attr_list(struct credential *c)
> g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
> if (c->path)
> g_hash_table_insert(al, "object", g_strdup(c->path));
> + if (c->password_expiry_utc)
> + g_hash_table_insert(al, "password_expiry_utc",
> + g_strdup(c->password_expiry_utc));
>
> return al;
> }
> @@ -101,9 +119,11 @@ static int keyring_get(struct credential *c)
>
> attributes = make_attr_list(c);
> items = secret_service_search_sync(service,
> - SECRET_SCHEMA_COMPAT_NETWORK,
> + &schema,
> attributes,
> - SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> + SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
> + // for backwards compatibility
No // comments please.
> + SECRET_SCHEMA_DONT_MATCH_NAME,
SECRET_SCHEMA_DONT_MATCH_NAME does not seem to be listed as one of
the flags to be used with secret_service_search_sync(),
https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/SecretService.php#secret-service-search-sync
and the only reference to it I found was as a flag to be placed in
the schema.
https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/migrating-schemas.php
https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-SecretSchema.php
But I'll take your word for it.
I found nothing unexpected or surprising in the rest of the patch to
this file. They all looked just a fallout of having to store and
retrieve one extra item from the database together with many other
things we already store and retrieve. Cleanly written.
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 5ea8bc9f1dc..9ebf7eeae48 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -43,6 +43,7 @@ helper_test_clean() {
> reject $1 https example.com store-user
> reject $1 https example.com user1
> reject $1 https example.com user2
> + reject $1 https example.com user3
> reject $1 http path.tld user
> reject $1 https timeout.tld user
> reject $1 https sso.tld
> @@ -298,6 +299,35 @@ helper_test_timeout() {
> '
> }
>
> +helper_test_password_expiry_utc() {
> + HELPER=$1
> +
> + test_expect_success "helper ($HELPER) stores password_expiry_utc" '
> + check approve $HELPER <<-\EOF
> + protocol=https
> + host=example.com
> + username=user3
> + password=pass
> + password_expiry_utc=9999999999
> + EOF
> + '
>
> + test_expect_success "helper ($HELPER) gets password_expiry_utc" '
> + check fill $HELPER <<-\EOF
> + protocol=https
> + host=example.com
> + username=user3
> + --
> + protocol=https
> + host=example.com
> + username=user3
> + password=pass
> + password_expiry_utc=9999999999
> + --
> + EOF
> + '
> +}
> +
Is any random number usable for this test, or is there some
constraints (like, "it cannot be too small to be a timestamp in the
past, because the entry will be expired immediately")? If there is
some constraint like that, is it a good idea to also test that
(like, "make sure an entry with expiry already happened is
rejected")?
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] credential/libsecret: support password_expiry_utc
2023-05-04 17:42 ` Junio C Hamano
@ 2023-05-05 7:00 ` M Hickford
0 siblings, 0 replies; 11+ messages in thread
From: M Hickford @ 2023-05-05 7:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: M Hickford via GitGitGadget, git, Jeff King, Dennis Kaarsemaker,
Mantas Mikulėnas, Javier Roucher Iglesias, Matthieu Moy,
M Hickford
On Thu, 4 May 2023 at 18:42, Junio C Hamano <gitster@pobox.com> wrote:
>
> "M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> We used to use the bog-standard "COMPAT_NETWORK" but now we are
> adding an extra element, and that makes it necessary to define our
> own? OK.
Exactly.
>
> > static char *make_label(struct credential *c)
> > {
> > if (c->port)
> > @@ -78,6 +93,9 @@ static GHashTable *make_attr_list(struct credential *c)
> > g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
> > if (c->path)
> > g_hash_table_insert(al, "object", g_strdup(c->path));
> > + if (c->password_expiry_utc)
> > + g_hash_table_insert(al, "password_expiry_utc",
> > + g_strdup(c->password_expiry_utc));
> >
> > return al;
> > }
> > @@ -101,9 +119,11 @@ static int keyring_get(struct credential *c)
> >
> > attributes = make_attr_list(c);
> > items = secret_service_search_sync(service,
> > - SECRET_SCHEMA_COMPAT_NETWORK,
> > + &schema,
> > attributes,
> > - SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> > + SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
> > + // for backwards compatibility
>
> No // comments please.
I'll amend in v3.
>
> > + SECRET_SCHEMA_DONT_MATCH_NAME,
>
> SECRET_SCHEMA_DONT_MATCH_NAME does not seem to be listed as one of
> the flags to be used with secret_service_search_sync(),
>
> https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/SecretService.php#secret-service-search-sync
>
> and the only reference to it I found was as a flag to be placed in
> the schema.
>
> https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/migrating-schemas.php
> https://www.manpagez.com/html/libsecret-1/libsecret-1-0.18.6/libsecret-SecretSchema.php
>
> But I'll take your word for it.
Good spot. I must have misread the docs. I shall fix in v3 and test
backwards compatibility carefully.
[1] https://gnome.pages.gitlab.gnome.org/libsecret/migrating-libgnome-keyring.html#working-with-schemas
>
> I found nothing unexpected or surprising in the rest of the patch to
> this file. They all looked just a fallout of having to store and
> retrieve one extra item from the database together with many other
> things we already store and retrieve. Cleanly written.
Thanks Junio for the review.
>
> > diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> > index 5ea8bc9f1dc..9ebf7eeae48 100644
> > --- a/t/lib-credential.sh
> > +++ b/t/lib-credential.sh
> > @@ -43,6 +43,7 @@ helper_test_clean() {
> > reject $1 https example.com store-user
> > reject $1 https example.com user1
> > reject $1 https example.com user2
> > + reject $1 https example.com user3
> > reject $1 http path.tld user
> > reject $1 https timeout.tld user
> > reject $1 https sso.tld
> > @@ -298,6 +299,35 @@ helper_test_timeout() {
> > '
> > }
> >
> > +helper_test_password_expiry_utc() {
> > + HELPER=$1
> > +
> > + test_expect_success "helper ($HELPER) stores password_expiry_utc" '
> > + check approve $HELPER <<-\EOF
> > + protocol=https
> > + host=example.com
> > + username=user3
> > + password=pass
> > + password_expiry_utc=9999999999
> > + EOF
> > + '
> >
> > + test_expect_success "helper ($HELPER) gets password_expiry_utc" '
> > + check fill $HELPER <<-\EOF
> > + protocol=https
> > + host=example.com
> > + username=user3
> > + --
> > + protocol=https
> > + host=example.com
> > + username=user3
> > + password=pass
> > + password_expiry_utc=9999999999
> > + --
> > + EOF
> > + '
> > +}
> > +
>
> Is any random number usable for this test, or is there some
> constraints (like, "it cannot be too small to be a timestamp in the
> past, because the entry will be expired immediately")? If there is
> some constraint like that, is it a good idea to also test that
> (like, "make sure an entry with expiry already happened is
> rejected")?
The date has to be in the future otherwise the credential will be
discarded by `credential approve` before it reaches the helper. That
logic is already tested in t/t0300-credentials.sh. There isn't any
expiry logic in the helper itself to test.
>
> Thanks.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] credential/libsecret: support password_expiry_utc
2023-03-25 7:36 ` [PATCH v2] " M Hickford via GitGitGadget
2023-05-04 17:42 ` Junio C Hamano
@ 2023-05-05 7:04 ` M Hickford via GitGitGadget
2023-05-15 10:50 ` M Hickford
2023-05-17 6:55 ` [PATCH v4] credential/libsecret: store new attributes M Hickford via GitGitGadget
1 sibling, 2 replies; 11+ messages in thread
From: M Hickford via GitGitGadget @ 2023-05-05 7:04 UTC (permalink / raw)
To: git; +Cc: Jeff King, M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
introduced this attribute.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/libsecret: store password_expiry_utc
Patch v3 fixes backwards compatibility.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v3
Pull-Request: https://github.com/git/git/pull/1469
Range-diff vs v2:
1: 1e27677b6f5 ! 1: b46594c8897 credential/libsecret: support password_expiry_utc
@@ Metadata
## Commit message ##
credential/libsecret: support password_expiry_utc
+ d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
+ introduced this attribute.
+
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
## contrib/credential/libsecret/git-credential-libsecret.c ##
@@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
+static const SecretSchema schema = {
+ "org.git.Password",
-+ SECRET_SCHEMA_NONE,
++ /* Ignore schema name for backwards compatibility with previous versions */
++ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
-- SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
-+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
-+ // for backwards compatibility
-+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
- &error);
- g_hash_table_unref(attributes);
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
c->username = g_strdup(s);
}
.../libsecret/git-credential-libsecret.c | 39 +++++++++++++++++--
t/lib-credential.sh | 30 ++++++++++++++
t/t0301-credential-cache.sh | 2 +
t/t0303-credential-external.sh | 2 +
4 files changed, 69 insertions(+), 4 deletions(-)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 2c5d76d789f..182f0805c2b 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,7 @@ struct credential {
char *path;
char *username;
char *password;
+ char *password_expiry_utc;
};
#define CREDENTIAL_INIT { 0 }
@@ -54,6 +55,21 @@ struct credential_operation {
/* ----------------- Secret Service functions ----------------- */
+static const SecretSchema schema = {
+ "org.git.Password",
+ /* Ignore schema name for backwards compatibility with previous versions */
+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { NULL, 0 },
+ }
+};
+
static char *make_label(struct credential *c)
{
if (c->port)
@@ -78,6 +94,9 @@ static GHashTable *make_attr_list(struct credential *c)
g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
if (c->path)
g_hash_table_insert(al, "object", g_strdup(c->path));
+ if (c->password_expiry_utc)
+ g_hash_table_insert(al, "password_expiry_utc",
+ g_strdup(c->password_expiry_utc));
return al;
}
@@ -101,7 +120,7 @@ static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
items = secret_service_search_sync(service,
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
@@ -128,6 +147,12 @@ static int keyring_get(struct credential *c)
c->username = g_strdup(s);
}
+ s = g_hash_table_lookup(attributes, "password_expiry_utc");
+ if (s) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(s);
+ }
+
s = secret_value_get_text(secret);
if (s) {
g_free(c->password);
@@ -162,7 +187,7 @@ static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
@@ -198,7 +223,7 @@ static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
attributes = make_attr_list(c);
- secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_clearv_sync(&schema,
attributes,
NULL,
&error);
@@ -238,6 +263,7 @@ static void credential_clear(struct credential *c)
g_free(c->path);
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
credential_init(c);
}
@@ -285,6 +311,9 @@ static int credential_read(struct credential *c)
} else if (!strcmp(key, "username")) {
g_free(c->username);
c->username = g_strdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(value);
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
@@ -312,9 +341,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
static void credential_write(const struct credential *c)
{
- /* only write username/password, if set */
+ /* only write username/password/expiry, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
}
static void usage(const char *name)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index 5ea8bc9f1dc..9ebf7eeae48 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -43,6 +43,7 @@ helper_test_clean() {
reject $1 https example.com store-user
reject $1 https example.com user1
reject $1 https example.com user2
+ reject $1 https example.com user3
reject $1 http path.tld user
reject $1 https timeout.tld user
reject $1 https sso.tld
@@ -298,6 +299,35 @@ helper_test_timeout() {
'
}
+helper_test_password_expiry_utc() {
+ HELPER=$1
+
+ test_expect_success "helper ($HELPER) stores password_expiry_utc" '
+ check approve $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ EOF
+ '
+
+ test_expect_success "helper ($HELPER) gets password_expiry_utc" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ --
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ --
+ EOF
+ '
+}
+
write_script askpass <<\EOF
echo >&2 askpass: $*
what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index 698b7159f03..f5ba727e53b 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -30,6 +30,8 @@ test_atexit 'git credential-cache exit'
# test that the daemon works with no special setup
helper_test cache
+helper_test_password_expiry_utc cache
+
test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
test_when_finished "
git credential-cache exit &&
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..f1478680bff 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -52,6 +52,8 @@ else
helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
fi
+helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
+
# clean afterwards so that we are good citizens
# and don't leave cruft in the helper's storage, which
# might be long-term system storage
base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] credential/libsecret: support password_expiry_utc
2023-05-05 7:04 ` [PATCH v3] " M Hickford via GitGitGadget
@ 2023-05-15 10:50 ` M Hickford
2023-05-15 18:14 ` Junio C Hamano
2023-05-17 6:55 ` [PATCH v4] credential/libsecret: store new attributes M Hickford via GitGitGadget
1 sibling, 1 reply; 11+ messages in thread
From: M Hickford @ 2023-05-15 10:50 UTC (permalink / raw)
To: M Hickford via GitGitGadget; +Cc: git, Jeff King, M Hickford
On Fri, 5 May 2023 at 08:05, M Hickford via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: M Hickford <mirth.hickford@gmail.com>
>
> d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
> introduced this attribute.
>
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
> credential/libsecret: store password_expiry_utc
>
> Patch v3 fixes backwards compatibility.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v3
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v3
> Pull-Request: https://github.com/git/git/pull/1469
>
> Range-diff vs v2:
>
> 1: 1e27677b6f5 ! 1: b46594c8897 credential/libsecret: support password_expiry_utc
> @@ Metadata
> ## Commit message ##
> credential/libsecret: support password_expiry_utc
>
> + d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
> + introduced this attribute.
> +
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
>
> ## contrib/credential/libsecret/git-credential-libsecret.c ##
> @@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
>
> +static const SecretSchema schema = {
> + "org.git.Password",
> -+ SECRET_SCHEMA_NONE,
> ++ /* Ignore schema name for backwards compatibility with previous versions */
> ++ SECRET_SCHEMA_DONT_MATCH_NAME,
> + {
> + { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
> @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
> - SECRET_SCHEMA_COMPAT_NETWORK,
> + &schema,
> attributes,
> -- SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> -+ SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK |
> -+ // for backwards compatibility
> -+ SECRET_SCHEMA_DONT_MATCH_NAME,
> + SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> NULL,
> - &error);
> - g_hash_table_unref(attributes);
> @@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
> c->username = g_strdup(s);
> }
>
>
> .../libsecret/git-credential-libsecret.c | 39 +++++++++++++++++--
> t/lib-credential.sh | 30 ++++++++++++++
> t/t0301-credential-cache.sh | 2 +
> t/t0303-credential-external.sh | 2 +
> 4 files changed, 69 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
> index 2c5d76d789f..182f0805c2b 100644
> --- a/contrib/credential/libsecret/git-credential-libsecret.c
> +++ b/contrib/credential/libsecret/git-credential-libsecret.c
> @@ -39,6 +39,7 @@ struct credential {
> char *path;
> char *username;
> char *password;
> + char *password_expiry_utc;
> };
>
> #define CREDENTIAL_INIT { 0 }
> @@ -54,6 +55,21 @@ struct credential_operation {
>
> /* ----------------- Secret Service functions ----------------- */
>
> +static const SecretSchema schema = {
> + "org.git.Password",
> + /* Ignore schema name for backwards compatibility with previous versions */
> + SECRET_SCHEMA_DONT_MATCH_NAME,
> + {
> + { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
> + { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
> + { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
I've been testing this patch with credential-generating helper
git-credential-helper. It works, but because libsecret overwrites
items if and only if the attributes match exactly, you end up with
many items in the secret store that differ only by expiry date. This
is inelegant, and confusing to users. Please hold this patch, don't
merge to master. A solution might be to store the expiry date as the
secret of a separate item (even though the value is not confidential)
> + { NULL, 0 },
> + }
> +};
> +
> static char *make_label(struct credential *c)
> {
> if (c->port)
> @@ -78,6 +94,9 @@ static GHashTable *make_attr_list(struct credential *c)
> g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
> if (c->path)
> g_hash_table_insert(al, "object", g_strdup(c->path));
> + if (c->password_expiry_utc)
> + g_hash_table_insert(al, "password_expiry_utc",
> + g_strdup(c->password_expiry_utc));
>
> return al;
> }
> @@ -101,7 +120,7 @@ static int keyring_get(struct credential *c)
>
> attributes = make_attr_list(c);
> items = secret_service_search_sync(service,
> - SECRET_SCHEMA_COMPAT_NETWORK,
> + &schema,
> attributes,
> SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
> NULL,
> @@ -128,6 +147,12 @@ static int keyring_get(struct credential *c)
> c->username = g_strdup(s);
> }
>
> + s = g_hash_table_lookup(attributes, "password_expiry_utc");
> + if (s) {
> + g_free(c->password_expiry_utc);
> + c->password_expiry_utc = g_strdup(s);
> + }
> +
> s = secret_value_get_text(secret);
> if (s) {
> g_free(c->password);
> @@ -162,7 +187,7 @@ static int keyring_store(struct credential *c)
>
> label = make_label(c);
> attributes = make_attr_list(c);
> - secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
> + secret_password_storev_sync(&schema,
> attributes,
> NULL,
> label,
> @@ -198,7 +223,7 @@ static int keyring_erase(struct credential *c)
> return EXIT_FAILURE;
>
> attributes = make_attr_list(c);
> - secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
> + secret_password_clearv_sync(&schema,
> attributes,
> NULL,
> &error);
> @@ -238,6 +263,7 @@ static void credential_clear(struct credential *c)
> g_free(c->path);
> g_free(c->username);
> g_free(c->password);
> + g_free(c->password_expiry_utc);
>
> credential_init(c);
> }
> @@ -285,6 +311,9 @@ static int credential_read(struct credential *c)
> } else if (!strcmp(key, "username")) {
> g_free(c->username);
> c->username = g_strdup(value);
> + } else if (!strcmp(key, "password_expiry_utc")) {
> + g_free(c->password_expiry_utc);
> + c->password_expiry_utc = g_strdup(value);
> } else if (!strcmp(key, "password")) {
> g_free(c->password);
> c->password = g_strdup(value);
> @@ -312,9 +341,11 @@ static void credential_write_item(FILE *fp, const char *key, const char *value)
>
> static void credential_write(const struct credential *c)
> {
> - /* only write username/password, if set */
> + /* only write username/password/expiry, if set */
> credential_write_item(stdout, "username", c->username);
> credential_write_item(stdout, "password", c->password);
> + credential_write_item(stdout, "password_expiry_utc",
> + c->password_expiry_utc);
> }
>
> static void usage(const char *name)
> diff --git a/t/lib-credential.sh b/t/lib-credential.sh
> index 5ea8bc9f1dc..9ebf7eeae48 100644
> --- a/t/lib-credential.sh
> +++ b/t/lib-credential.sh
> @@ -43,6 +43,7 @@ helper_test_clean() {
> reject $1 https example.com store-user
> reject $1 https example.com user1
> reject $1 https example.com user2
> + reject $1 https example.com user3
> reject $1 http path.tld user
> reject $1 https timeout.tld user
> reject $1 https sso.tld
> @@ -298,6 +299,35 @@ helper_test_timeout() {
> '
> }
>
> +helper_test_password_expiry_utc() {
> + HELPER=$1
> +
> + test_expect_success "helper ($HELPER) stores password_expiry_utc" '
> + check approve $HELPER <<-\EOF
> + protocol=https
> + host=example.com
> + username=user3
> + password=pass
> + password_expiry_utc=9999999999
> + EOF
> + '
> +
> + test_expect_success "helper ($HELPER) gets password_expiry_utc" '
> + check fill $HELPER <<-\EOF
> + protocol=https
> + host=example.com
> + username=user3
> + --
> + protocol=https
> + host=example.com
> + username=user3
> + password=pass
> + password_expiry_utc=9999999999
> + --
> + EOF
> + '
> +}
> +
> write_script askpass <<\EOF
> echo >&2 askpass: $*
> what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
> diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
> index 698b7159f03..f5ba727e53b 100755
> --- a/t/t0301-credential-cache.sh
> +++ b/t/t0301-credential-cache.sh
> @@ -30,6 +30,8 @@ test_atexit 'git credential-cache exit'
> # test that the daemon works with no special setup
> helper_test cache
>
> +helper_test_password_expiry_utc cache
> +
> test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
> test_when_finished "
> git credential-cache exit &&
> diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
> index f028fd14182..f1478680bff 100755
> --- a/t/t0303-credential-external.sh
> +++ b/t/t0303-credential-external.sh
> @@ -52,6 +52,8 @@ else
> helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
> fi
>
> +helper_test_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
> +
> # clean afterwards so that we are good citizens
> # and don't leave cruft in the helper's storage, which
> # might be long-term system storage
>
> base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
> --
> gitgitgadget
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] credential/libsecret: support password_expiry_utc
2023-05-15 10:50 ` M Hickford
@ 2023-05-15 18:14 ` Junio C Hamano
2023-05-16 8:03 ` M Hickford
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2023-05-15 18:14 UTC (permalink / raw)
To: M Hickford; +Cc: M Hickford via GitGitGadget, git, Jeff King
M Hickford <mirth.hickford@gmail.com> writes:
>> +static const SecretSchema schema = {
>> + "org.git.Password",
>> + /* Ignore schema name for backwards compatibility with previous versions */
>> + SECRET_SCHEMA_DONT_MATCH_NAME,
>> + {
>> + { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
>> + { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
>> + { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
>> + { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
>> + { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
>> + { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
>
> I've been testing this patch with credential-generating helper
> git-credential-helper. It works, but because libsecret overwrites
> items if and only if the attributes match exactly, you end up with
> many items in the secret store that differ only by expiry date. This
> is inelegant, and confusing to users. Please hold this patch, don't
> merge to master. A solution might be to store the expiry date as the
> secret of a separate item (even though the value is not confidential)
Thanks for stopping me. I'll mark the topic as "on hold".
It does sound problematic, but if we think about what is used as
keys and what is used as values, it does make a lot more sense to
store the expiry as part of a value. After all, we are not even
asking "give me the password that will expire in the most distant
future" or anything like that. We consult the database with "who
wants to access what server over which protocol at what port" as the
key and expect we find the suitable authentication material to use.
It would be best if we can treat the expiry date as an additional
attribute of that authentication material.
Do the methods to store and retrieve a password from the keyring
allow us to add such an extra attribute to the password? I have no
idea how the Gnome keyring API works, but is there a way to mark
each entry in the SecretSchemaAttributes as "this is used as a key"
vs "this is used as a value---do not match"? Would thinking along
such a line help?
Another possibility would be to store encoded concatenation of the
real password and expiration timestamp and decode them into two upon
retrieval. If we were the only user of the keystore, that may work,
but if we are sharing the keystore with other applications, it would
be a non-starter.
What do other application do, when using the keyring to store
expirable passwords with services that do let you know the
expiration time of the password? If they just ask the users again
only after finding out that the password did not work, perhaps we
should do the same, without being proactive and notice the expiry
ourselves? That is, instead of failing the access to the server
immediately upon seeing an auth failure, if the authentication
material is know to have expiration time, can we let the application
layer to ask the end-user to provide an refreshed password and try
again? For such a scheme, we do not have to store ever-changing
"password_expiry_utc" and contaminate the keyring with crufts whose
expiry dates are the only difference. Instead we can just have a
Boolean "does this site expire a valid password?" and use it to
behave differently, if desired, from sites for which the passwords
do not expire, perhaps?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] credential/libsecret: support password_expiry_utc
2023-05-15 18:14 ` Junio C Hamano
@ 2023-05-16 8:03 ` M Hickford
2023-05-16 16:10 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: M Hickford @ 2023-05-16 8:03 UTC (permalink / raw)
To: Junio C Hamano; +Cc: M Hickford, M Hickford via GitGitGadget, git, Jeff King
On Mon, 15 May 2023 at 19:15, Junio C Hamano <gitster@pobox.com> wrote:
>
> M Hickford <mirth.hickford@gmail.com> writes:
>
> >> +static const SecretSchema schema = {
> >> + "org.git.Password",
> >> + /* Ignore schema name for backwards compatibility with previous versions */
> >> + SECRET_SCHEMA_DONT_MATCH_NAME,
> >> + {
> >> + { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
> >> + { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
> >> + { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
> >> + { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
> >> + { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
> >> + { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
> >
> > I've been testing this patch with credential-generating helper
> > git-credential-helper. It works, but because libsecret overwrites
> > items if and only if the attributes match exactly, you end up with
> > many items in the secret store that differ only by expiry date. This
> > is inelegant, and confusing to users. Please hold this patch, don't
> > merge to master. A solution might be to store the expiry date as the
> > secret of a separate item (even though the value is not confidential)
>
> Thanks for stopping me. I'll mark the topic as "on hold".
Thanks Junio
>
> It does sound problematic, but if we think about what is used as
> keys and what is used as values, it does make a lot more sense to
> store the expiry as part of a value. After all, we are not even
> asking "give me the password that will expire in the most distant
> future" or anything like that. We consult the database with "who
> wants to access what server over which protocol at what port" as the
> key and expect we find the suitable authentication material to use.
> It would be best if we can treat the expiry date as an additional
> attribute of that authentication material.
>
> Do the methods to store and retrieve a password from the keyring
> allow us to add such an extra attribute to the password? I have no
> idea how the Gnome keyring API works, but is there a way to mark
> each entry in the SecretSchemaAttributes as "this is used as a key"
> vs "this is used as a value---do not match"? Would thinking along
> such a line help?
Unfortunately not. The libsecret docs warn "Attributes are meant to be
used for lookup of items; they’re not designed to be used as a generic
key/value database".
https://gnome.pages.gitlab.gnome.org/libsecret/migrating-libgnome-keyring.html
Interestingly, Windows' wincred API doesn't have this issue, because
it searches by a unique key, defined separately to attributes.
>
> Another possibility would be to store encoded concatenation of the
> real password and expiration timestamp and decode them into two upon
> retrieval. If we were the only user of the keystore, that may work,
> but if we are sharing the keystore with other applications, it would
> be a non-starter.
Yes, I think that would work nicely. A format such as that below would
be backwards compatible (passwords already can't contain newlines) and
self explanatory to any curious user browsing their secret store. I
already have a draft that works much like this. I'll prepare a patch
v4.
7d7b554
password_expiry_utc=1684179877
oauth_refresh_token=be8a9aa3
Is the secret store ever shared with other applications such as a web
browser? If so, sharing is already broken, because popular Git hosts
such as GitHub and GitLab expect different passwords for web login and
Git authentication (OAuth token or personal access token). A solution
could be to introduce our own libsecret schema (as in the current
patch) instead of continuing to use SECRET_SCHEMA_COMPAT_NETWORK
potentially shared with other apps. I'm not sure whether that's
worthwhile in this patch. I defer to you.
AFAIK major web browsers all implement their own password store rather
than use libsecret, though apparently Chromium has a build flag to use
libsecret on Linux.
https://source.chromium.org/search?q=libsecret%20-f:third_party&ss=chromium
>
> What do other application do, when using the keyring to store
> expirable passwords with services that do let you know the
> expiration time of the password? If they just ask the users again
> only after finding out that the password did not work, perhaps we
> should do the same, without being proactive and notice the expiry
> ourselves? That is, instead of failing the access to the server
> immediately upon seeing an auth failure, if the authentication
> material is know to have expiration time, can we let the application
> layer to ask the end-user to provide an refreshed password and try
> again? For such a scheme, we do not have to store ever-changing
> "password_expiry_utc" and contaminate the keyring with crufts whose
> expiry dates are the only difference. Instead we can just have a
> Boolean "does this site expire a valid password?" and use it to
> behave differently, if desired, from sites for which the passwords
> do not expire, perhaps?
>
In the case of HTTP error 401 Unauthorized, Git calls `credential
reject` then exits. An improvement could be to first call `credential
fill` again (this time without password prompt) and retry if a new
credential is returned. A fresh credential if generated is likely to
be valid. This would improve the experience when using a
credential-generating helper together with a storage helper that drops
password_expiry_utc.
Best remains to use a storage helper that stores password_expiry_utc.
This avoids a doomed HTTP request. Storing oauth_refresh_token is also
beneficial, because OAuth refresh is faster than cold OAuth.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3] credential/libsecret: support password_expiry_utc
2023-05-16 8:03 ` M Hickford
@ 2023-05-16 16:10 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2023-05-16 16:10 UTC (permalink / raw)
To: M Hickford; +Cc: M Hickford via GitGitGadget, git, Jeff King
M Hickford <mirth.hickford@gmail.com> writes:
> Yes, I think that would work nicely. A format such as that below would
> be backwards compatible (passwords already can't contain newlines) and
> self explanatory to any curious user browsing their secret store. I
> already have a draft that works much like this. I'll prepare a patch
> v4.
>
> 7d7b554
> password_expiry_utc=1684179877
> oauth_refresh_token=be8a9aa3
>
> Is the secret store ever shared with other applications such as a web
> browser? If so, sharing is already broken, because popular Git hosts
> such as GitHub and GitLab expect different passwords for web login and
> Git authentication (OAuth token or personal access token).
It probably is a good argument. We do not have to worry about
interoperating with browsers and their authentication with Git
hosting sites. And the "newline can be used to add extra pieces of
information" is a good trick ;-)
> A solution
> could be to introduce our own libsecret schema (as in the current
> patch) instead of continuing to use SECRET_SCHEMA_COMPAT_NETWORK
> potentially shared with other apps. I'm not sure whether that's
> worthwhile in this patch. I defer to you.
It may depend on what other Git GUI frontends may want to do. If
there is no precedent and you are the pioneer, then using our own
would be perfectly fine and we can let others also read from us if
they want to; I presume that it would not prevent us from doing so
even if did not use COMPAT_NETWORK (which gnome dev document even
discourages of its use in new applications anyway).
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v4] credential/libsecret: store new attributes
2023-05-05 7:04 ` [PATCH v3] " M Hickford via GitGitGadget
2023-05-15 10:50 ` M Hickford
@ 2023-05-17 6:55 ` M Hickford via GitGitGadget
2023-06-16 19:55 ` [PATCH v5] " M Hickford via GitGitGadget
1 sibling, 1 reply; 11+ messages in thread
From: M Hickford via GitGitGadget @ 2023-05-17 6:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token)
introduced new credential attributes.
libsecret assumes attribute values are non-confidential and
unchanging, so we encode the new attributes in the secret, separated by
newline:
hunter2
password_expiry_utc=1684189401
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 password_expiry_utc in a libsecret
attribute. This has the problem that libsecret creates new items
rather than overwrites when attribute values change.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/libsecret: store password_expiry_utc and oauth_refresh_token
Patch v4 stores password_expiry_utc inside secret instead of in an
attribute.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v4
Pull-Request: https://github.com/git/git/pull/1469
Range-diff vs v3:
1: b46594c8897 ! 1: 07452a10372 credential/libsecret: support password_expiry_utc
@@ Metadata
Author: M Hickford <mirth.hickford@gmail.com>
## Commit message ##
- credential/libsecret: support password_expiry_utc
+ credential/libsecret: store new attributes
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
- introduced this attribute.
+ and a5c76569e7 (credential: new attribute oauth_refresh_token)
+ introduced new credential attributes.
+
+ libsecret assumes attribute values are non-confidential and
+ unchanging, so we encode the new attributes in the secret, separated by
+ newline:
+
+ hunter2
+ password_expiry_utc=1684189401
+ 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 password_expiry_utc in a libsecret
+ attribute. This has the problem that libsecret creates new items
+ rather than overwrites when attribute values change.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
@@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential {
char *username;
char *password;
+ char *password_expiry_utc;
++ char *oauth_refresh_token;
};
#define CREDENTIAL_INIT { 0 }
@@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
+static const SecretSchema schema = {
+ "org.git.Password",
-+ /* Ignore schema name for backwards compatibility with previous versions */
++ /* Ignore schema name during search for backwards compatibility */
+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
++ /*
++ * libsecret assumes attribute values are non-confidential and
++ * unchanging, so we can't include oauth_refresh_token or
++ * password_expiry_utc.
++ */
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
-+ { "password_expiry_utc", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { NULL, 0 },
+ }
+};
@@ contrib/credential/libsecret/git-credential-libsecret.c: struct credential_opera
static char *make_label(struct credential *c)
{
if (c->port)
-@@ contrib/credential/libsecret/git-credential-libsecret.c: static GHashTable *make_attr_list(struct credential *c)
- g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
- if (c->path)
- g_hash_table_insert(al, "object", g_strdup(c->path));
-+ if (c->password_expiry_utc)
-+ g_hash_table_insert(al, "password_expiry_utc",
-+ g_strdup(c->password_expiry_utc));
-
- return al;
- }
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(
SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
- c->username = g_strdup(s);
- }
+ SecretItem *item;
+ SecretValue *secret;
+ const char *s;
++ gchar **parts;
+
+ item = items->data;
+ secret = secret_item_get_secret(item);
+@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_get(struct credential *c)
-+ s = g_hash_table_lookup(attributes, "password_expiry_utc");
-+ if (s) {
-+ g_free(c->password_expiry_utc);
-+ c->password_expiry_utc = g_strdup(s);
-+ }
-+
s = secret_value_get_text(secret);
if (s) {
- g_free(c->password);
+- g_free(c->password);
+- c->password = g_strdup(s);
++ /*
++ * Passwords and other attributes encoded in following format:
++ * hunter2
++ * password_expiry_utc=1684189401
++ * oauth_refresh_token=xyzzy
++ */
++ parts = g_strsplit(s, "\n", 0);
++ if (g_strv_length(parts) >= 1) {
++ g_free(c->password);
++ c->password = g_strdup(parts[0]);
++ }
++ for (int i = 1; i < g_strv_length(parts); i++) {
++ if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
++ g_free(c->password_expiry_utc);
++ c->password_expiry_utc = g_strdup(&parts[i][20]);
++ } else if (g_str_has_prefix(parts[i], "oauth_refresh_token=")) {
++ g_free(c->oauth_refresh_token);
++ c->oauth_refresh_token = g_strdup(&parts[i][20]);
++ }
++ }
++ g_strfreev(parts);
+ }
+
+ g_hash_table_unref(attributes);
+@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_store(struct credential *c)
+ char *label = NULL;
+ GHashTable *attributes = NULL;
+ GError *error = NULL;
++ GString *secret = NULL;
+
+ /*
+ * Sanity check that what we are storing is actually sensible.
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
++ secret = g_string_new(c->password);
++ if (c->password_expiry_utc) {
++ g_string_append_printf(secret, "\npassword_expiry_utc=%s",
++ c->password_expiry_utc);
++ }
++ if (c->oauth_refresh_token) {
++ g_string_append_printf(secret, "\noauth_refresh_token=%s",
++ c->oauth_refresh_token);
++ }
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
+- c->password,
++ secret->str,
+ NULL,
+ &error);
++ g_string_free(secret, TRUE);
+ g_free(label);
+ g_hash_table_unref(attributes);
+
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
++ g_free(c->oauth_refresh_token);
credential_init(c);
}
@@ contrib/credential/libsecret/git-credential-libsecret.c: static int credential_r
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
-@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_write_item(FILE *fp, const char *key, const char *value)
-
- static void credential_write(const struct credential *c)
- {
-- /* only write username/password, if set */
-+ /* only write username/password/expiry, if set */
+ while (*value)
+ *value++ = '\0';
++ } else if (!strcmp(key, "oauth_refresh_token")) {
++ g_free(c->oauth_refresh_token);
++ c->oauth_refresh_token = g_strdup(value);
++ while (*value)
++ *value++ = '\0';
+ }
+ /*
+ * Ignore other lines; we don't know what they mean, but
+@@ contrib/credential/libsecret/git-credential-libsecret.c: static void credential_write(const struct credential *c)
+ /* only write username/password, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
++ credential_write_item(stdout, "oauth_refresh_token",
++ c->oauth_refresh_token);
}
static void usage(const char *name)
@@ t/lib-credential.sh: helper_test_clean() {
reject $1 https example.com user1
reject $1 https example.com user2
+ reject $1 https example.com user3
+ reject $1 https example.com user4
reject $1 http path.tld user
reject $1 https timeout.tld user
- reject $1 https sso.tld
@@ t/lib-credential.sh: helper_test_timeout() {
'
}
@@ t/lib-credential.sh: helper_test_timeout() {
+ '
+}
+
- write_script askpass <<\EOF
- echo >&2 askpass: $*
- what=$(echo $1 | cut -d" " -f1 | tr A-Z a-z | tr -cd a-z)
+ helper_test_oauth_refresh_token() {
+ HELPER=$1
+
## t/t0301-credential-cache.sh ##
@@ t/t0301-credential-cache.sh: test_atexit 'git credential-cache exit'
+
# test that the daemon works with no special setup
helper_test cache
-
+helper_test_password_expiry_utc cache
-+
+ helper_test_oauth_refresh_token cache
+
test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
- test_when_finished "
- git credential-cache exit &&
## t/t0303-credential-external.sh ##
-@@ t/t0303-credential-external.sh: else
- helper_test_timeout "$GIT_TEST_CREDENTIAL_HELPER_TIMEOUT"
- fi
+@@ 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_password_expiry_utc "$GIT_TEST_CREDENTIAL_HELPER"
-+
- # clean afterwards so that we are good citizens
- # and don't leave cruft in the helper's storage, which
- # might be long-term system storage
++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)"
.../libsecret/git-credential-libsecret.c | 78 +++++++++++++++++--
t/lib-credential.sh | 30 +++++++
t/t0301-credential-cache.sh | 1 +
t/t0303-credential-external.sh | 2 +
4 files changed, 105 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index ef681f29d5b..31cf32ad969 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,8 @@ struct credential {
char *path;
char *username;
char *password;
+ char *password_expiry_utc;
+ char *oauth_refresh_token;
};
#define CREDENTIAL_INIT { 0 }
@@ -54,6 +56,25 @@ struct credential_operation {
/* ----------------- Secret Service functions ----------------- */
+static const SecretSchema schema = {
+ "org.git.Password",
+ /* Ignore schema name during search for backwards compatibility */
+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
+ /*
+ * libsecret assumes attribute values are non-confidential and
+ * unchanging, so we can't include oauth_refresh_token or
+ * password_expiry_utc.
+ */
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { NULL, 0 },
+ }
+};
+
static char *make_label(struct credential *c)
{
if (c->port)
@@ -101,7 +122,7 @@ static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
items = secret_service_search_sync(service,
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
@@ -117,6 +138,7 @@ static int keyring_get(struct credential *c)
SecretItem *item;
SecretValue *secret;
const char *s;
+ gchar **parts;
item = items->data;
secret = secret_item_get_secret(item);
@@ -130,8 +152,27 @@ static int keyring_get(struct credential *c)
s = secret_value_get_text(secret);
if (s) {
- g_free(c->password);
- c->password = g_strdup(s);
+ /*
+ * Passwords and other attributes encoded in following format:
+ * hunter2
+ * password_expiry_utc=1684189401
+ * oauth_refresh_token=xyzzy
+ */
+ parts = g_strsplit(s, "\n", 0);
+ if (g_strv_length(parts) >= 1) {
+ g_free(c->password);
+ c->password = g_strdup(parts[0]);
+ }
+ for (int i = 1; i < g_strv_length(parts); i++) {
+ if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(&parts[i][20]);
+ } else if (g_str_has_prefix(parts[i], "oauth_refresh_token=")) {
+ g_free(c->oauth_refresh_token);
+ c->oauth_refresh_token = g_strdup(&parts[i][20]);
+ }
+ }
+ g_strfreev(parts);
}
g_hash_table_unref(attributes);
@@ -148,6 +189,7 @@ static int keyring_store(struct credential *c)
char *label = NULL;
GHashTable *attributes = NULL;
GError *error = NULL;
+ GString *secret = NULL;
/*
* Sanity check that what we are storing is actually sensible.
@@ -162,13 +204,23 @@ static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret = g_string_new(c->password);
+ if (c->password_expiry_utc) {
+ g_string_append_printf(secret, "\npassword_expiry_utc=%s",
+ c->password_expiry_utc);
+ }
+ if (c->oauth_refresh_token) {
+ g_string_append_printf(secret, "\noauth_refresh_token=%s",
+ c->oauth_refresh_token);
+ }
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
- c->password,
+ secret->str,
NULL,
&error);
+ g_string_free(secret, TRUE);
g_free(label);
g_hash_table_unref(attributes);
@@ -198,7 +250,7 @@ static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
attributes = make_attr_list(c);
- secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_clearv_sync(&schema,
attributes,
NULL,
&error);
@@ -238,6 +290,8 @@ static void credential_clear(struct credential *c)
g_free(c->path);
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
+ g_free(c->oauth_refresh_token);
credential_init(c);
}
@@ -284,11 +338,19 @@ static int credential_read(struct credential *c)
} else if (!strcmp(key, "username")) {
g_free(c->username);
c->username = g_strdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(value);
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
while (*value)
*value++ = '\0';
+ } else if (!strcmp(key, "oauth_refresh_token")) {
+ g_free(c->oauth_refresh_token);
+ c->oauth_refresh_token = g_strdup(value);
+ while (*value)
+ *value++ = '\0';
}
/*
* Ignore other lines; we don't know what they mean, but
@@ -314,6 +376,10 @@ static void credential_write(const struct credential *c)
/* only write username/password, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
+ credential_write_item(stdout, "oauth_refresh_token",
+ c->oauth_refresh_token);
}
static void usage(const char *name)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index f1ab92ba35c..4ad498adbc4 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -43,6 +43,7 @@ helper_test_clean() {
reject $1 https example.com store-user
reject $1 https example.com user1
reject $1 https example.com user2
+ reject $1 https example.com user3
reject $1 https example.com user4
reject $1 http path.tld user
reject $1 https timeout.tld user
@@ -328,6 +329,35 @@ helper_test_timeout() {
'
}
+helper_test_password_expiry_utc() {
+ HELPER=$1
+
+ test_expect_success "helper ($HELPER) stores password_expiry_utc" '
+ check approve $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ EOF
+ '
+
+ test_expect_success "helper ($HELPER) gets password_expiry_utc" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user3
+ --
+ protocol=https
+ host=example.com
+ username=user3
+ password=pass
+ password_expiry_utc=9999999999
+ --
+ EOF
+ '
+}
+
helper_test_oauth_refresh_token() {
HELPER=$1
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index c02a3b5969c..8300faadea9 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -29,6 +29,7 @@ test_atexit 'git credential-cache exit'
# test that the daemon works with no special setup
helper_test cache
+helper_test_password_expiry_utc cache
helper_test_oauth_refresh_token cache
test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..095574bfc6e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -45,6 +45,8 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_password_expiry_utc "$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: 0df2c180904f6b709766f9c24669a9d01543f915
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v5] credential/libsecret: store new attributes
2023-05-17 6:55 ` [PATCH v4] credential/libsecret: store new attributes M Hickford via GitGitGadget
@ 2023-06-16 19:55 ` M Hickford via GitGitGadget
0 siblings, 0 replies; 11+ messages in thread
From: M Hickford via GitGitGadget @ 2023-06-16 19:55 UTC (permalink / raw)
To: git; +Cc: Jeff King, M Hickford, M Hickford
From: M Hickford <mirth.hickford@gmail.com>
d208bfd (credential: new attribute password_expiry_utc, 2023-02-18)
and a5c76569e7 (credential: new attribute oauth_refresh_token)
introduced new credential attributes.
libsecret assumes attribute values are non-confidential and
unchanging, so we encode the new attributes in the secret, separated by
newline:
hunter2
password_expiry_utc=1684189401
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 password_expiry_utc in a libsecret
attribute. This has the problem that libsecret creates new items
rather than overwrites when attribute values change.
Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
credential/libsecret: store password_expiry_utc and oauth_refresh_token
Test that storing credential with expiry overwrites any old credential
Rename test users for clarity
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1469%2Fhickford%2Flibsecret-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1469/hickford/libsecret-v5
Pull-Request: https://github.com/git/git/pull/1469
Range-diff vs v4:
1: 07452a10372 ! 1: 38e02c5e24e credential/libsecret: store new attributes
@@ t/lib-credential.sh: helper_test_clean() {
reject $1 https example.com store-user
reject $1 https example.com user1
reject $1 https example.com user2
-+ reject $1 https example.com user3
++ reject $1 https example.com user-expiry
++ reject $1 https example.com user-expiry-overwrite
reject $1 https example.com user4
reject $1 http path.tld user
reject $1 https timeout.tld user
@@ t/lib-credential.sh: helper_test_timeout() {
+ check approve $HELPER <<-\EOF
+ protocol=https
+ host=example.com
-+ username=user3
++ username=user-expiry
+ password=pass
+ password_expiry_utc=9999999999
+ EOF
@@ t/lib-credential.sh: helper_test_timeout() {
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
-+ username=user3
++ username=user-expiry
+ --
+ protocol=https
+ host=example.com
-+ username=user3
++ username=user-expiry
+ password=pass
+ password_expiry_utc=9999999999
+ --
+ EOF
+ '
++
++ test_expect_success "helper ($HELPER) overwrites when password_expiry_utc changes" '
++ check approve $HELPER <<-\EOF &&
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ password=pass1
++ password_expiry_utc=9999999998
++ EOF
++ check approve $HELPER <<-\EOF &&
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ password=pass2
++ password_expiry_utc=9999999999
++ EOF
++ check fill $HELPER <<-\EOF &&
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ --
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ password=pass2
++ password_expiry_utc=9999999999
++ EOF
++ check reject $HELPER <<-\EOF &&
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ password=pass2
++ EOF
++ check fill $HELPER <<-\EOF
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ --
++ protocol=https
++ host=example.com
++ username=user-expiry-overwrite
++ password=askpass-password
++ --
++ askpass: Password for '\''https://user-expiry-overwrite@example.com'\'':
++ EOF
++ '
+}
+
helper_test_oauth_refresh_token() {
.../libsecret/git-credential-libsecret.c | 78 +++++++++++++++++--
t/lib-credential.sh | 77 ++++++++++++++++++
t/t0301-credential-cache.sh | 1 +
t/t0303-credential-external.sh | 2 +
4 files changed, 152 insertions(+), 6 deletions(-)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index ef681f29d5b..31cf32ad969 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -39,6 +39,8 @@ struct credential {
char *path;
char *username;
char *password;
+ char *password_expiry_utc;
+ char *oauth_refresh_token;
};
#define CREDENTIAL_INIT { 0 }
@@ -54,6 +56,25 @@ struct credential_operation {
/* ----------------- Secret Service functions ----------------- */
+static const SecretSchema schema = {
+ "org.git.Password",
+ /* Ignore schema name during search for backwards compatibility */
+ SECRET_SCHEMA_DONT_MATCH_NAME,
+ {
+ /*
+ * libsecret assumes attribute values are non-confidential and
+ * unchanging, so we can't include oauth_refresh_token or
+ * password_expiry_utc.
+ */
+ { "user", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "object", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "protocol", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { "port", SECRET_SCHEMA_ATTRIBUTE_INTEGER },
+ { "server", SECRET_SCHEMA_ATTRIBUTE_STRING },
+ { NULL, 0 },
+ }
+};
+
static char *make_label(struct credential *c)
{
if (c->port)
@@ -101,7 +122,7 @@ static int keyring_get(struct credential *c)
attributes = make_attr_list(c);
items = secret_service_search_sync(service,
- SECRET_SCHEMA_COMPAT_NETWORK,
+ &schema,
attributes,
SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
NULL,
@@ -117,6 +138,7 @@ static int keyring_get(struct credential *c)
SecretItem *item;
SecretValue *secret;
const char *s;
+ gchar **parts;
item = items->data;
secret = secret_item_get_secret(item);
@@ -130,8 +152,27 @@ static int keyring_get(struct credential *c)
s = secret_value_get_text(secret);
if (s) {
- g_free(c->password);
- c->password = g_strdup(s);
+ /*
+ * Passwords and other attributes encoded in following format:
+ * hunter2
+ * password_expiry_utc=1684189401
+ * oauth_refresh_token=xyzzy
+ */
+ parts = g_strsplit(s, "\n", 0);
+ if (g_strv_length(parts) >= 1) {
+ g_free(c->password);
+ c->password = g_strdup(parts[0]);
+ }
+ for (int i = 1; i < g_strv_length(parts); i++) {
+ if (g_str_has_prefix(parts[i], "password_expiry_utc=")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(&parts[i][20]);
+ } else if (g_str_has_prefix(parts[i], "oauth_refresh_token=")) {
+ g_free(c->oauth_refresh_token);
+ c->oauth_refresh_token = g_strdup(&parts[i][20]);
+ }
+ }
+ g_strfreev(parts);
}
g_hash_table_unref(attributes);
@@ -148,6 +189,7 @@ static int keyring_store(struct credential *c)
char *label = NULL;
GHashTable *attributes = NULL;
GError *error = NULL;
+ GString *secret = NULL;
/*
* Sanity check that what we are storing is actually sensible.
@@ -162,13 +204,23 @@ static int keyring_store(struct credential *c)
label = make_label(c);
attributes = make_attr_list(c);
- secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret = g_string_new(c->password);
+ if (c->password_expiry_utc) {
+ g_string_append_printf(secret, "\npassword_expiry_utc=%s",
+ c->password_expiry_utc);
+ }
+ if (c->oauth_refresh_token) {
+ g_string_append_printf(secret, "\noauth_refresh_token=%s",
+ c->oauth_refresh_token);
+ }
+ secret_password_storev_sync(&schema,
attributes,
NULL,
label,
- c->password,
+ secret->str,
NULL,
&error);
+ g_string_free(secret, TRUE);
g_free(label);
g_hash_table_unref(attributes);
@@ -198,7 +250,7 @@ static int keyring_erase(struct credential *c)
return EXIT_FAILURE;
attributes = make_attr_list(c);
- secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+ secret_password_clearv_sync(&schema,
attributes,
NULL,
&error);
@@ -238,6 +290,8 @@ static void credential_clear(struct credential *c)
g_free(c->path);
g_free(c->username);
g_free(c->password);
+ g_free(c->password_expiry_utc);
+ g_free(c->oauth_refresh_token);
credential_init(c);
}
@@ -284,11 +338,19 @@ static int credential_read(struct credential *c)
} else if (!strcmp(key, "username")) {
g_free(c->username);
c->username = g_strdup(value);
+ } else if (!strcmp(key, "password_expiry_utc")) {
+ g_free(c->password_expiry_utc);
+ c->password_expiry_utc = g_strdup(value);
} else if (!strcmp(key, "password")) {
g_free(c->password);
c->password = g_strdup(value);
while (*value)
*value++ = '\0';
+ } else if (!strcmp(key, "oauth_refresh_token")) {
+ g_free(c->oauth_refresh_token);
+ c->oauth_refresh_token = g_strdup(value);
+ while (*value)
+ *value++ = '\0';
}
/*
* Ignore other lines; we don't know what they mean, but
@@ -314,6 +376,10 @@ static void credential_write(const struct credential *c)
/* only write username/password, if set */
credential_write_item(stdout, "username", c->username);
credential_write_item(stdout, "password", c->password);
+ credential_write_item(stdout, "password_expiry_utc",
+ c->password_expiry_utc);
+ credential_write_item(stdout, "oauth_refresh_token",
+ c->oauth_refresh_token);
}
static void usage(const char *name)
diff --git a/t/lib-credential.sh b/t/lib-credential.sh
index f1ab92ba35c..72f52cfedb2 100644
--- a/t/lib-credential.sh
+++ b/t/lib-credential.sh
@@ -43,6 +43,8 @@ helper_test_clean() {
reject $1 https example.com store-user
reject $1 https example.com user1
reject $1 https example.com user2
+ reject $1 https example.com user-expiry
+ reject $1 https example.com user-expiry-overwrite
reject $1 https example.com user4
reject $1 http path.tld user
reject $1 https timeout.tld user
@@ -328,6 +330,81 @@ helper_test_timeout() {
'
}
+helper_test_password_expiry_utc() {
+ HELPER=$1
+
+ test_expect_success "helper ($HELPER) stores password_expiry_utc" '
+ check approve $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user-expiry
+ password=pass
+ password_expiry_utc=9999999999
+ EOF
+ '
+
+ test_expect_success "helper ($HELPER) gets password_expiry_utc" '
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user-expiry
+ --
+ protocol=https
+ host=example.com
+ username=user-expiry
+ password=pass
+ password_expiry_utc=9999999999
+ --
+ EOF
+ '
+
+ test_expect_success "helper ($HELPER) overwrites when password_expiry_utc changes" '
+ check approve $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ password=pass1
+ password_expiry_utc=9999999998
+ EOF
+ check approve $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ password=pass2
+ password_expiry_utc=9999999999
+ EOF
+ check fill $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ --
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ password=pass2
+ password_expiry_utc=9999999999
+ EOF
+ check reject $HELPER <<-\EOF &&
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ password=pass2
+ EOF
+ check fill $HELPER <<-\EOF
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ --
+ protocol=https
+ host=example.com
+ username=user-expiry-overwrite
+ password=askpass-password
+ --
+ askpass: Password for '\''https://user-expiry-overwrite@example.com'\'':
+ EOF
+ '
+}
+
helper_test_oauth_refresh_token() {
HELPER=$1
diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index c02a3b5969c..8300faadea9 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -29,6 +29,7 @@ test_atexit 'git credential-cache exit'
# test that the daemon works with no special setup
helper_test cache
+helper_test_password_expiry_utc cache
helper_test_oauth_refresh_token cache
test_expect_success 'socket defaults to ~/.cache/git/credential/socket' '
diff --git a/t/t0303-credential-external.sh b/t/t0303-credential-external.sh
index f028fd14182..095574bfc6e 100755
--- a/t/t0303-credential-external.sh
+++ b/t/t0303-credential-external.sh
@@ -45,6 +45,8 @@ test -z "$GIT_TEST_CREDENTIAL_HELPER_SETUP" ||
helper_test_clean "$GIT_TEST_CREDENTIAL_HELPER"
helper_test "$GIT_TEST_CREDENTIAL_HELPER"
+helper_test_password_expiry_utc "$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] 11+ messages in thread
end of thread, other threads:[~2023-06-16 19:55 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-14 21:32 [PATCH] credential/libsecret: support password_expiry_utc M Hickford via GitGitGadget
2023-03-25 7:36 ` [PATCH v2] " M Hickford via GitGitGadget
2023-05-04 17:42 ` Junio C Hamano
2023-05-05 7:00 ` M Hickford
2023-05-05 7:04 ` [PATCH v3] " M Hickford via GitGitGadget
2023-05-15 10:50 ` M Hickford
2023-05-15 18:14 ` Junio C Hamano
2023-05-16 8:03 ` M Hickford
2023-05-16 16:10 ` Junio C Hamano
2023-05-17 6:55 ` [PATCH v4] credential/libsecret: store new attributes M Hickford via GitGitGadget
2023-06-16 19:55 ` [PATCH v5] " 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).