Re: [PATCH] gpg-interface: lazily initialize and read the configuration

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 9, 2023, 02:24 UTC
Message-ID: <230209.86pmajy3ig.gmgdl@evledraar.gmail.com>
In-Reply-To: <xmqq5ycbpp8a.fsf@gitster.g>

On Wed, Feb 08 2023, Junio C Hamano wrote:

Show 12 quoted lines

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> One thing left un-noted here is that this will defer any errors in the
>> config now until use (or lazy init), so e.g.:
>>
>> 	git -c gpg.mintrustlevel=bad show --show-signature
>>
>> Used to exit with code 128 and an error, but will now (at least for me)
>> proceed to run show successfully.
>
> That one is probably a good thing.  We shouldn't interrupt users for
> a misspelt configuration value that the user is not using.

*nod*, just noting it.

Show 47 quoted lines

>>> @@ -632,6 +644,8 @@ int check_signature(struct signature_check *sigc,
>>>  	struct gpg_format *fmt;
>>>  	int status;
>>>  
>>> +	gpg_interface_lazy_init();
>>> +
>>>  	sigc->result = 'N';
>>>  	sigc->trust_level = -1;
>>
>> This is needed, because we need "configured_min_trust_level" populated.
>
> I specifically did not want anybody to start doing this line of
> analysis, because it will add unnecessary bugs, and also introduce
> maintenance problems.  You may be able to grab the current state of
> the code, but that will get stale and won't be a good guide to keep
> our code robust.
>
>>> @@ -695,11 +709,13 @@ int parse_signature(const char *buf, size_t size, struct strbuf *payload, struct
>>>  
>>>  void set_signing_key(const char *key)
>>>  {
>>> +	gpg_interface_lazy_init();
>>> +
>>>  	free(configured_signing_key);
>>>  	configured_signing_key = xstrdup(key);
>>>  }
>>
>> But this is not, we could say that we're doing it for good measure, but
>> it's hard to imagine a scenario where we would end up actually needing
>> lazy init here. we'll just set a variable here, which...
>
> And especially this one, we must have init or we'll be incorrect, I
> think.  There is a direct set_signing_key() caller (I think in "git
> tag") that does not come from the git_config() callback route.
> Without the lazy initialization, we'd get configured_signing_key
> from the config because early in the start-up sequence of the
> command we would do git_gpg_config() via git_config(), and then try
> to process "-u keyid" by calling this one again.
>
> If you forget to lazily initialize here, configured_signing_key gets
> the keyid obtained via "-u keyid", and then when control reaches the
> real signing function, we'd realize that we still haven't
> initialized ourselves.  And we call lazy init, which finds
> configured keyID, which is very likely different from "-u keyid"
> (the user would not be passing "-u keyid" from the command line to
> override, if that is the same as the configured one), and clobber
> it.

Yeah, I take your general point that it's good to sprinkle the gpg_interface_lazy_init().

In this case I think we'll just barely do the right thing, the only external caller is tag.c, which first does:

	set_signing_key(keyid);

And then:

	sign_buffer(buffer, buffer, get_signing_key());

So we'll (I think):

	1. Get the non-config'd key from before
	2. Call sign_buffer()
	3. Promptly clobber that key
	4. It won't matter because at that point we'll be passing a key
	   as a parma, which overrides the "config"

But yeah, dropping it would mean we end up with the wrong key in the variable afterwards, which even if it's not used is nasty.