| Message ID | pull.1469.v2.git.git.1679729764851.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] credential/libsecret: support password_expiry_utc | expand |
"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.
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. >
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