Re: [fsverity-utils PATCH v2] Implement PKCS#11 opaque keys support through OpenSSL pkcs11 engine

[Eric Biggers <ebiggers@kernel.org>]

On Thu, Sep 09, 2021 at 12:20:35AM +0000, Aleksander Adamowski wrote:
> On Wednesday, September 8, 2021 4:48 PM, Eric Biggers wrote:
> > Regarding struct libfsverity_signature_params, I wrote "Please write a comment
> > that clearly explains which parameters must be specified and when.".
> 
> Got it. I assumed that the detailed explanation in the manpage covering the
> same parameters would be sufficient, as repeating it in struct comments would
> make the information redundant and require reformatting that part to multi-line
> comments.
> 
> I can add it to the struct comments, but this will mean I'll need to change
> them to multi-line comments (above each struct member) and add empty lines
> between members (following the same commenting style as in struct
> libfsverity_merkle_tree_params). Are you okay with that change?

The fsverity.1 man page documents the fsverity tool, whereas the comments in
libfsverity.h document the interface to libfsverity.  These are different
things, so the documentation for one doesn't really apply to the other, unless
they explicitly reference each other.  (Which you can do if it would avoid
redundancy.  The point is, if the documentation is somewhere else, people
actually need to be told where to find it.)

It's fine to reformat the comments and code if necessary.

> 
> > Also I mentioned "The !OPENSSL_IS_BORINGSSL case no longer returns an error if
> > sig_params->keyfile or sig_params->certfile is unset".  That wasn't addressed
> > for sig_params->certfile.
> 
> Ah, I see. In my patch V2, after your suggestion, there's a new NULL check for
> certfile in lib/sign_digest.c:87 that I intended as a replacement for the
> previous check in lib/sign_digest.c:337. I think it's a better place for that
> check, as it's in the place of actual use.
> 
> Do you want me to place that check back in the pre-check logic in
> libfsverity_sign_digest()?

Your patch has:

	if (!certfile) {
		libfsverity_error_msg("certfile must be specified");
	}

It needs to be:

	if (!certfile) {
		libfsverity_error_msg("certfile must be specified");
		return -EINVAL;
	}

- Eric