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

Daniel Martí <[email protected]> writes:

> Gentle nudge on this, anything I can do to assist in getting it reviewed?
>
> I still run into the error a few times per week :)
>
> On 8/4/26 11:40 PM, Daniel Martí via GitGitGadget wrote:
>> From: =?UTF-8?q?Daniel=20Mart=C3=AD?= <[email protected]>
>>
>> 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

I do not program Gnome so I am not exactly qualified to review this,
but anyway.

The above makes it sound like we can just request with
secret_service_search_sync() exactly the same way as before (i.e.,
with LOAD_SECRETS), and then check with secret_item_get_secret()
to see if it has secret value in it.  The problem with the current
code is that we do not validate what that secrete value is (iow, we
do not say "ah, NULL, we should not assume we do have secret already
obtained here").

So does the first hunk to drop _LOAD_SECRETS really needed?  Rather,
would it be more straight-forward to do

	item = items->data;

	secret = secret_item_get_secret(item);
	if (!secret &&
            !secret_item_load_secret_sync(item, NULL, &error)) {
		... your error handling here ...
		return EXIT_FAILURE;
	} 
        if (!secret)
		secret = secret_item_get_secret(item);

instead?  I am assuming that it is rare (like, only a few times a
week) to race with other activities that unloads the secret and most
of the time the first secret_service_search_sync() gets everything
needed in a single call.

I am also assuming that this is a race condition that is not very
easy to reliably reproduce in the test, so I wouldn't expect it to
come with a test to ensure that the fix will not regress in the
future (in other words, lack of patch to t/ directory is fine).

Thanks.


>>
>>      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í <[email protected]>
>> ---
>>      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