From: M Hickford <mirth.hickford@gmail.com>
To: M Hickford via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Jeff King <peff@peff.net>,
M Hickford <mirth.hickford@gmail.com>
Subject: Re: [PATCH v3] credential/libsecret: support password_expiry_utc
Date: Mon, 15 May 2023 11:50:01 +0100 [thread overview]
Message-ID: <CAGJzqskMwOJkriH6serqdwAVYi+fftEL8ohJd-suP6v+OxB_bg@mail.gmail.com> (raw)
In-Reply-To: <pull.1469.v3.git.git.1683270298313.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2023-05-15 10:50 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAGJzqskMwOJkriH6serqdwAVYi+fftEL8ohJd-suP6v+OxB_bg@mail.gmail.com \
--to=mirth.hickford@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).