From: "M Hickford via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jeff King <peff@peff.net>, M Hickford <mirth.hickford@gmail.com>,
M Hickford <mirth.hickford@gmail.com>
Subject: [PATCH v4] credential/libsecret: store new attributes
Date: Wed, 17 May 2023 06:55:40 +0000 [thread overview]
Message-ID: <pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1469.v3.git.git.1683270298313.gitgitgadget@gmail.com>
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
next prev parent reply other threads:[~2023-05-17 6:55 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
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 ` M Hickford via GitGitGadget [this message]
2023-06-16 19:55 ` [PATCH v5] credential/libsecret: store new attributes 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=pull.1469.v4.git.git.1684306540947.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=mirth.hickford@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.