[PATCH] credential/libsecret: load secrets explicitly

[PATCH] credential/libsecret: load secrets explicitly

[Daniel Martí via GitGitGadget]

From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>

secret_service_search_sync() can return an item whose secret is not
loaded, despite SECRET_SEARCH_LOAD_SECRETS being set: the search
silently discards secret-loading failures, and the GNOME keyring
daemon silently omits from its GetSecrets reply any item that is
locked or that was deleted after the search matched it, e.g. by a
concurrent "credential erase" from another git process.

secret_item_get_secret() then returns NULL, which we pass unchecked
to secret_value_get_text() and secret_value_unref(), producing

    secret_value_get_text: assertion 'value' failed
    secret_value_unref: assertion 'value != NULL' failed

and losing the password even when the secret is still retrievable.

Drop SECRET_SEARCH_LOAD_SECRETS and instead load the secret of the
one item we use with secret_item_load_secret_sync(), which does
report errors. A secret the search would have silently dropped is
now retrieved normally, and a genuinely inaccessible item produces
a useful message instead of assertion spew, with git falling back
to prompting either way. Merely guarding against NULL would avoid
the assertions, but would forfeit a secret that is still available.
The cost is unchanged: the search no longer batch-fetches the
secrets of all matching items, and the explicit load fetches the
one we use.

Signed-off-by: Daniel Martí <mvdan@mvdan.cc>
---
    credential/libsecret: load secrets explicitly

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2372%2Fmvdan%2Flibsecret-null-secret-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2372/mvdan/libsecret-null-secret-v1
Pull-Request: https://github.com/git/git/pull/2372

 .../libsecret/git-credential-libsecret.c           | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index 941b2afd5e..6bbdf2bd45 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -126,7 +126,7 @@ static int keyring_get(struct credential *c)
 	items = secret_service_search_sync(service,
 					   &schema,
 					   attributes,
-					   SECRET_SEARCH_LOAD_SECRETS | SECRET_SEARCH_UNLOCK,
+					   SECRET_SEARCH_UNLOCK,
 					   NULL,
 					   &error);
 	g_hash_table_unref(attributes);
@@ -143,6 +143,18 @@ static int keyring_get(struct credential *c)
 		gchar **parts;
 
 		item = items->data;
+
+		/*
+		 * Load the secret explicitly rather than via
+		 * SECRET_SEARCH_LOAD_SECRETS, which silently discards load
+		 * failures and returns items whose secret is NULL.
+		 */
+		if (!secret_item_load_secret_sync(item, NULL, &error)) {
+			g_critical("could not load secret: %s", error->message);
+			g_error_free(error);
+			g_list_free_full(items, g_object_unref);
+			return EXIT_FAILURE;
+		}
 		secret = secret_item_get_secret(item);
 		attributes = secret_item_get_attributes(item);
 

base-commit: 5b2471720c93ee30e5764a19f3d3b3ae9ec9712a
-- 
gitgitgadget