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

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

On Mon, 2020-10-26 at 10:48 -0700, Eric Biggers wrote:
> On Mon, Oct 26, 2020 at 11:40:07AM +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[])
> > +{
> > +	struct filedes file = { .fd = -1 };
> > +	u8 *salt = NULL;
> > +	struct libfsverity_merkle_tree_params tree_params = { .version = 1 };
> > +	struct libfsverity_digest *digest = NULL;
> > +	struct fsverity_signed_digest *d = NULL;
> > +	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + sizeof(struct fsverity_signed_digest) * 2 + 1];
> > +	bool compact = false, for_builtin_sig = false;
> > +	int status;
> > +	int c;
> > +
> > +	while ((c = getopt_long(argc, argv, "", longopts, NULL)) != -1) {
> > +		switch (c) {
> > +		case OPT_HASH_ALG:
> > +			if (!parse_hash_alg_option(optarg,
> > +						   &tree_params.hash_algorithm))
> > +				goto out_usage;
> > +			break;
> > +		case OPT_BLOCK_SIZE:
> > +			if (!parse_block_size_option(optarg,
> > +						     &tree_params.block_size))
> > +				goto out_usage;
> > +			break;
> > +		case OPT_SALT:
> > +			if (!parse_salt_option(optarg, &salt,
> > +					       &tree_params.salt_size))
> > +				goto out_usage;
> > +			tree_params.salt = salt;
> > +			break;
> > +		case OPT_COMPACT:
> > +			compact = true;
> > +			break;
> > +		case OPT_FOR_BUILTIN_SIG:
> > +			for_builtin_sig = true;
> > +			break;
> > +		default:
> > +			goto out_usage;
> > +		}
> > +	}
> > +
> > +	argv += optind;
> > +	argc -= optind;
> > +
> > +	if (argc != 1)
> > +		goto out_usage;
> 
> I think this should allow specifying multiple files, like 'fsverity measure'
> does.  'fsverity measure' is intended to behave like the sha256sum program.

Added in v3.

> > +	/* The kernel expects more than the digest as the signed payload */
> > +	if (for_builtin_sig) {
> > +		d = xzalloc(sizeof(*d) + digest->digest_size);
> > +		if (!d)
> > +			goto out_err;
> 
> No need to check the return value of xzalloc(), since it exits on error.

Removed in v3.

> > +	if (compact)
> > +		printf("%s", digest_hex);
> > +	else
> > +		printf("File '%s' (%s:%s)\n", argv[0],
> > +			   libfsverity_get_hash_name(tree_params.hash_algorithm),
> > +			   digest_hex);
> 
> Please make the output in the !compact case match 'fsverity measure':
> 
> 	printf("%s:%s %s\n",
> 	       libfsverity_get_hash_name(tree_params.hash_algorithm),
> 	       digest_hex, argv[i]);
> 
> - Eric

Done with v3, sorry got confused with the 'sign' output.

-- 
Kind regards,
Luca Boccassi