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

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

On Fri, 2020-10-23 at 21:23 -0700, Eric Biggers wrote:
> On Thu, Oct 22, 2020 at 06:21:55PM +0100, 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;
> > +	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;
> > +		default:
> > +			goto out_usage;
> > +		}
> > +	}
> > +
> > +	argv += optind;
> > +	argc -= optind;
> > +
> > +	if (argc != 1)
> > +		goto out_usage;
> > +
> > +	if (tree_params.hash_algorithm == 0)
> > +		tree_params.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
> > +
> > +	if (tree_params.block_size == 0)
> > +		tree_params.block_size = get_default_block_size();
> > +
> > +	if (!open_file(&file, argv[0], O_RDONLY, 0))
> > +		goto out_err;
> > +
> > +	if (!get_file_size(&file, &tree_params.file_size))
> > +		goto out_err;
> > +
> > +	if (libfsverity_compute_digest(&file, read_callback,
> > +				       &tree_params, &digest) != 0) {
> > +		error_msg("failed to compute digest");
> > +		goto out_err;
> > +	}
> > +
> > +	ASSERT(digest->digest_size <= FS_VERITY_MAX_DIGEST_SIZE);
> > +
> > +	d = xzalloc(sizeof(*d) + digest->digest_size);
> > +	if (!d)
> > +		goto out_err;
> > +	memcpy(d->magic, "FSVerity", 8);
> > +	d->digest_algorithm = cpu_to_le16(digest->digest_algorithm);
> > +	d->digest_size = cpu_to_le16(digest->digest_size);
> > +	memcpy(d->digest, digest->digest, digest->digest_size);
> > +
> > +	bin2hex((const u8 *)d, sizeof(*d) + digest->digest_size, digest_hex);
> > +
> > +	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);
> 
> Can you make this command format its output in the same way as
> 'fsverity measure' by default, and put the 'struct fsverity_signed_digest'
> formatted output behind an option, like --for-builtin-sig?

Sure, done in v2.

> The 'struct fsverity_signed_digest' is specific to the builtin (in-kernel)
> signature support, which isn't the only way to use fs-verity.  The signature
> verification can also be done in userspace, which is more flexible.  (And you
> should consider doing it that way, if you haven't already.  I'm not sure exactly
> what your use case is.)

Our use case is to ultimately add kernel-side policy enforcement of
integrity via the IPE LSM: https://microsoft.github.io/ipe/ so we can't
do without kernel verification. I need the new command as I've got some
teams building and signing on Windows, using whatever native API is
available there, so can't use the 'fsverity sign' command.

> So when possible, I'd like to have the default be the basic fs-verity feature.
> If someone then specifically wants to use the builtin signature support on top
> of that, as opposed to using fs-verity in another way, then they can provide the
> option they need to do that.
> 
> Separately, it would also be nice to share more code with cmd_sign.c, as they
> both have to parse a lot of the same options.  Maybe it doesn't work out,
> though.

I did have a look at this at the beginning, but I was not happy with
how it looked like - both are using APIs from the public library and
are compact enough, so it felt a bit awkward to add yet another shim
layer, and opted to avoid it. If you feel strongly about it let me know
and I can revise.

-- 
Kind regards,
Luca Boccassi