Re: [fsverity-utils PATCH v4] Add digest sub command

[Luca Boccassi <luca.boccassi@gmail.com>]

On Mon, 2020-10-26 at 11:58 -0700, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 06:17:29PM +0000, luca.boccassi@gmail.com wrote:
> > +/* Compute a file's fs-verity measurement, then print it in hex format. */
> > +int fsverity_cmd_digest(const struct fsverity_command *cmd,
> > +		      int argc, char *argv[])
> 
> Since it can be more than one file now:
> 
> /* Compute the fs-verity measurement of the given file(s), for offline signing */

Applied in v5.

> > +	for (int i = 0; i < argc; i++) {
> > +		struct filedes file = { .fd = -1 };
> > +		struct fsverity_signed_digest *d = NULL;
> > +		struct libfsverity_digest *digest = NULL;
> > +		char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + sizeof(struct fsverity_signed_digest) * 2 + 1];
> > +
> > +		if (!open_file(&file, argv[i], O_RDONLY, 0))
> > +			goto out_err;
> > +
> > +		if (!get_file_size(&file, &tree_params.file_size))
> > +			goto out_err;
> 
> 'file' doesn't get closed on error.  Making it back to the outer scope would fix
> that.

Added to out_err in v5 (I had ignored it as cmd_sign does currently).

> > +		if (compact)
> > +			printf("%s\n", digest_hex);
> > +		else
> > +			printf("%s:%s %s\n",
> > +				libfsverity_get_hash_name(tree_params.hash_algorithm),
> > +				digest_hex, argv[i]);
> 
> I don't think the hash algorithm should be printed in the
> '!compact && for_builtin_sig' case, since it's already included in the struct
> that gets hex-encoded.  I.e.
> 
> 		else if (for_builtin_sig)
> 			printf("%s %s\n", digest_hex, argv[i]);

Fixed in v5.

> > diff --git a/programs/fsverity.c b/programs/fsverity.c
> > index 95f6964..c7c4f75 100644
> > --- a/programs/fsverity.c
> > +++ b/programs/fsverity.c
> > @@ -21,6 +21,14 @@ static const struct fsverity_command {
> >  	const char *usage_str;
> >  } fsverity_commands[] = {
> >  	{
> > +		.name = "digest",
> > +		.func = fsverity_cmd_digest,
> > +		.short_desc = "Compute and print hex-encoded fs-verity digest of a file, for offline signing",
> 
> Likewise, since this can now accept multiple files:
> 
> "Compute the fs-verity measurement of the given file(s), for offline signing"
> 
> (I don't think that "printed as hex" needs to be explicitly mentioned here.)

Done in v5.

-- 
Kind regards,
Luca Boccassi