Re: [PATCH v6 1/9] ssh signing: preliminary refactoring and clean-up

[Fabian Stelzer <fs@gigacodes.de>]

On 29.07.21 00:32, Jonathan Tan wrote:
> I think this patch set is beyond the "is this a good idea in general"
> phase (in particular, I think that being able to sign Git commits by
> using SSH infrastructure is very useful), so I'll proceed to critiquing
> the commits in more detail.

Thanks for your help.

> 
> Firstly, in commit messages, the left side of the colon is usually the
> name of the subsystem - in this case, "gpg-interface".
> 

The docs call this "name of the component you're working on". Since this 
code does not actually change any gpg functionality (at least it should 
not) i think gpg-interface in the commits might be a bit misleading.


>> To be able to implement new signing formats this commit:
>>   - makes the sigc structure more generic by renaming "gpg_output" to
>>     "output"
>>   - introduces function pointers in the gpg_format structure to call
>>     format specific signing and verification functions
>>   - moves format detection from verify_signed_buffer into the check_signature
>>     api function and calls the format specific verify
>>   - renames and wraps sign_buffer to handle format specific signing logic
>>     as well
> 
> I think that this commit should be further split up - in particular, it
> is hard for reviewers to verify that there is no difference in
> functionality before and after this commit. I already spotted one
> difference - perhaps there are more. For me, splitting the above 4
> points into 4 commits would be an acceptable split.
> 

The rename can of course be easily separated. The others would probably 
require some code in between commits that's not present in the final 
patch result to make the individual commits compile / work. Otherwise 
those would only add unused code with the last commit then actually 
using everything. I don't think that would make things easier to verify, 
would it?


> [snip]
> 
>>   static struct gpg_format gpg_format[] = {
>> -	{ .name = "openpgp", .program = "gpg",
>> -	  .verify_args = openpgp_verify_args,
>> -	  .sigs = openpgp_sigs
>> +	{
>> +		.name = "openpgp",
>> +		.program = "gpg",
>> +		.verify_args = openpgp_verify_args,
>> +		.sigs = openpgp_sigs,
>> +		.verify_signed_buffer = verify_gpg_signed_buffer,
>> +		.sign_buffer = sign_buffer_gpg,
>>   	},
>> -	{ .name = "x509", .program = "gpgsm",
>> -	  .verify_args = x509_verify_args,
>> -	  .sigs = x509_sigs
>> +	{
>> +		.name = "x509",
>> +		.program = "gpgsm",
>> +		.verify_args = x509_verify_args,
>> +		.sigs = x509_sigs,
>> +		.verify_signed_buffer = verify_gpg_signed_buffer,
>> +		.sign_buffer = sign_buffer_gpg,
>>   	},
>>   };
> 
> I think that verify_signed_buffer and sign_buffer should replace
> verify_args and sigs, not be alongside them. In particular, I see from
> later patches that a new entry will be introduced for SSH, and the
> corresponding new "verify" function does not use verify_args or sigs.
> 

I kept the verify_args since i would either have to duplicate the 
verify_gpg_signed_buffer for gpg & gpgsm or have an if within deciding 
what format to use.
Also this is something that we might want to make a configuration option 
in the future and pass to ssh-keygen as well (there are a couple of -O 
options for it users might want)

sigs is still needed for the parse_signed_buffer api function.