Re: [PATCH 0/7] Split fsverity-utils into a shared library

[Jes Sorensen <jes.sorensen@gmail.com>]

Hi Eric,

On 2/14/20 3:35 PM, Eric Biggers wrote:
> Well, this might be a legitimate use case then.  We need to define the library
> interface as simply as possible, though, so that we can maintain this code in
> the future without breaking users.  I suggest starting with something along the
> lines of:
> 
> #ifndef _LIBFSVERITY_H
> #define _LIBFSVERITY_H
> 
> #include <stddef.h>
> #include <stdint.h>
> 
> #define FS_VERITY_HASH_ALG_SHA256       1
> #define FS_VERITY_HASH_ALG_SHA512       2
> 
> struct libfsverity_merkle_tree_params {
> 	uint32_t version;
> 	uint32_t hash_algorithm;
> 	uint32_t block_size;
> 	uint32_t salt_size;
> 	const uint8_t *salt;
> 	size_t reserved[11];
> };
> 
> struct libfsverity_digest {
> 	uint16_t digest_algorithm;
> 	uint16_t digest_size;
> 	uint8_t digest[];
> };
> 
> struct libfsverity_signature_params {
> 	const char *keyfile;
> 	const char *certfile;
> 	size_t reserved[11];
> };

This looks reasonable to me - I would do the reserved fields as void *
or uint32_t, but that is a detail.

> int libfsverity_compute_digest(int fd,
> 			       const struct libfsverity_merkle_tree_params *params,
> 			       struct libfsverity_digest **digest_ret);
> 
> int libfsverity_sign_digest(const struct libfsverity_digest *digest,
> 			    const struct libfsverity_signature_params *sig_params,
> 			    void **sig_ret, size_t *sig_size_ret);
> 
> #endif /* _LIBFSVERITY_H */

Looks good too, I deliberately named the functions as fsverity, but
happy to prepend them with 'lib'. Didn't want to have a clash with
'sign_hash' as a function is actually named in a related library.

> I.e.:
> 
> - The stuff in util.h and hash_algs.h isn't exposed to library users.
> - Then names of all library functions and structs are appropriately prefixed
>   and avoid collisions with the kernel header.
> - Only signing functionality is included.
> - There are reserved fields, so we can add more parameters later.

I was debating whether to expect the library to do the open or have the
caller be responsible for that. Given we have to play the song and dance
with the signing key and certificate filenames, it's a little quirky,
but we're passing those to libopenssl so no way to really get around it.

> Before committing to any stable API, it would also be helpful to see the RPM
> patches to see what it actually does.

Absolutely, I wanted to have us agree on the strategy first before
taking it to the next step.

I'll take a stab at this.

> We'd also need to follow shared library best practices like compiling with
> -fvisibility=hidden and marking the API functions explicitly with
> __attribute__((visibility("default"))), and setting the 'soname' like
> -Wl,-soname=libfsverity.so.0.
> 
> Also, is the GPLv2+ license okay for the use case?

Personally I only care about linking it into rpm, which is GPL v2, so
from my perspective, that is sufficient. I am also fine making it LGPL,
but given it's your code I am stealing, I cannot make that call.

Cheers,
Jes