Re: [PATCH] credential/libsecret: load secrets explicitly

>
> 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