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

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

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

Hi,

I am looking at what it will take to add support for fsverity
signatures to rpm, similar to how rpm supports IMA signatures.

In order to do so, it makes sense to split the fsverity util into a
shared library and the command line tool, so the core functions can be
used from other applciations. Alternatively I will have to copy over a
good chunk of the code into rpm, which makes it nasty to support long
term.

This is a first stab at doing that, and I'd like to get some feedback
on the approach.

I basically split it into four functions:

fsverity_cmd_gen_digest(): Build the digest, but do not sign it
fsverity_cmd_sign():       Sign the digest structure
fsverity_cmd_measure():    Measure a file, basically 'fsverity measure'
fsverity_cmd_enable():     Enable verity on a file, basically 'fsverity enable'

If we can agree on the approach, then I am happy to deal with the full
libtoolification etc.

Jes


Jes Sorensen (7):
  Build basic shared library
  Restructure fsverity_cmd_sign for shared libraries
  Make fsverity_cmd_measure() a library function
  Make fsverity_cmd_enable a library call()
  Rename commands.h to fsverity.h
  Move cmdline helper functions to fsverity.c
  cmd_sign: fsverity_cmd_sign() into two functions

 Makefile      |  18 ++-
 cmd_enable.c  | 133 +------------------
 cmd_measure.c |  51 ++------
 cmd_sign.c    | 168 ++++++------------------
 commands.h    |  24 ----
 fsverity.c    | 345 +++++++++++++++++++++++++++++++++++++++++++++++---
 fsverity.h    |  38 ++++++
 util.c        |  13 ++
 8 files changed, 446 insertions(+), 344 deletions(-)
 delete mode 100644 commands.h
 create mode 100644 fsverity.h

-- 
2.24.1

[PATCH 1/7] Build basic shared library

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 Makefile | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/Makefile b/Makefile
index b9c09b9..006fc60 100644
--- a/Makefile
+++ b/Makefile
@@ -1,20 +1,32 @@
 EXE := fsverity
+LIB := libfsverity.so
 CFLAGS := -O2 -Wall
 CPPFLAGS := -D_FILE_OFFSET_BITS=64
 LDLIBS := -lcrypto
 DESTDIR := /usr/local
+LIBDIR := /usr/lib64
 SRC := $(wildcard *.c)
-OBJ := $(SRC:.c=.o)
+OBJ := fsverity.o
+SSRC := hash_algs.c cmd_sign.c cmd_enable.c cmd_measure.c util.c
+SOBJ := hash_algs.so cmd_sign.so cmd_enable.so cmd_measure.so util.so
 HDRS := $(wildcard *.h)
 
 all:$(EXE)
 
-$(EXE):$(OBJ)
+$(EXE):$(OBJ) $(LIB)
+	$(CC) -o $@ $< -L . -l fsverity
 
 $(OBJ): %.o: %.c $(HDRS)
+	$(CC) -c $(CFLAGS) $(CPPFLAGS) $< -o $@
+
+$(SOBJ): %.so: %.c $(HDRS)
+	$(CC) -c -fPIC $(CFLAGS) $(CPPFLAGS) $< -o $@
+
+libfsverity.so: $(SOBJ)
+	$(CC) $(LDLIBS) -shared -o libfsverity.so $(SOBJ)
 
 clean:
-	rm -f $(EXE) $(OBJ)
+	rm -f $(EXE) $(OBJ) $(SOBJ) $(LIB)
 
 install:all
 	install -Dm755 -t $(DESTDIR)/bin $(EXE)
-- 
2.24.1

[PATCH 2/7] Restructure fsverity_cmd_sign for shared libraries

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

This moves the command line parsing, error reporting etc. to the command
line tool, and turns fsverity_cmd_sign() into a call returning the
digest and the signature.

It is the responsibility of the caller to decide what to do with the
returned objects.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c | 132 ++++++-----------------------------------------------
 commands.h |  23 +++++++++-
 fsverity.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 163 insertions(+), 121 deletions(-)

diff --git a/cmd_sign.c b/cmd_sign.c
index dcb37ce..2d3fa54 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -8,7 +8,6 @@
  */
 
 #include <fcntl.h>
-#include <getopt.h>
 #include <limits.h>
 #include <openssl/bio.h>
 #include <openssl/err.h>
@@ -38,19 +37,6 @@ struct fsverity_descriptor {
 	__u8 signature[];	/* optional PKCS#7 signature */
 };
 
-/*
- * Format in which verity file measurements are signed.  This is the same as
- * 'struct fsverity_digest', except here some magic bytes are prepended to
- * provide some context about what is being signed in case the same key is used
- * for non-fsverity purposes, and here the fields have fixed endianness.
- */
-struct fsverity_signed_digest {
-	char magic[8];			/* must be "FSVerity" */
-	__le16 digest_algorithm;
-	__le16 digest_size;
-	__u8 digest[];
-};
-
 static void __printf(1, 2) __cold
 error_msg_openssl(const char *format, ...)
 {
@@ -340,18 +326,6 @@ out:
 	return ok;
 }
 
-static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
-{
-	struct filedes file;
-	bool ok;
-
-	if (!open_file(&file, filename, O_WRONLY|O_CREAT|O_TRUNC, 0644))
-		return false;
-	ok = full_write(&file, sig, sig_size);
-	ok &= filedes_close(&file);
-	return ok;
-}
-
 #define FS_VERITY_MAX_LEVELS	64
 
 struct block_buffer {
@@ -507,93 +481,27 @@ out:
 	return ok;
 }
 
-enum {
-	OPT_HASH_ALG,
-	OPT_BLOCK_SIZE,
-	OPT_SALT,
-	OPT_KEY,
-	OPT_CERT,
-};
-
-static const struct option longopts[] = {
-	{"hash-alg",	required_argument, NULL, OPT_HASH_ALG},
-	{"block-size",	required_argument, NULL, OPT_BLOCK_SIZE},
-	{"salt",	required_argument, NULL, OPT_SALT},
-	{"key",		required_argument, NULL, OPT_KEY},
-	{"cert",	required_argument, NULL, OPT_CERT},
-	{NULL, 0, NULL, 0}
-};
-
 /* Sign a file for fs-verity by computing its measurement, then signing it. */
-int fsverity_cmd_sign(const struct fsverity_command *cmd,
-		      int argc, char *argv[])
+int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
+		      u32 block_size, u8 *salt, u32 salt_size,
+		      const char *keyfile, const char *certfile,
+		      struct fsverity_signed_digest **retdigest,
+		      u8 **sig, u32 *sig_size)
 {
-	const struct fsverity_hash_alg *hash_alg = NULL;
-	u32 block_size = 0;
-	u8 *salt = NULL;
-	u32 salt_size = 0;
-	const char *keyfile = NULL;
-	const char *certfile = NULL;
 	struct fsverity_signed_digest *digest = NULL;
-	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
-	u8 *sig = NULL;
-	u32 sig_size;
 	int status;
-	int c;
-
-	while ((c = getopt_long(argc, argv, "", longopts, NULL)) != -1) {
-		switch (c) {
-		case OPT_HASH_ALG:
-			if (hash_alg != NULL) {
-				error_msg("--hash-alg can only be specified once");
-				goto out_usage;
-			}
-			hash_alg = find_hash_alg_by_name(optarg);
-			if (hash_alg == NULL)
-				goto out_usage;
-			break;
-		case OPT_BLOCK_SIZE:
-			if (!parse_block_size_option(optarg, &block_size))
-				goto out_usage;
-			break;
-		case OPT_SALT:
-			if (!parse_salt_option(optarg, &salt, &salt_size))
-				goto out_usage;
-			break;
-		case OPT_KEY:
-			if (keyfile != NULL) {
-				error_msg("--key can only be specified once");
-				goto out_usage;
-			}
-			keyfile = optarg;
-			break;
-		case OPT_CERT:
-			if (certfile != NULL) {
-				error_msg("--cert can only be specified once");
-				goto out_usage;
-			}
-			certfile = optarg;
-			break;
-		default:
-			goto out_usage;
-		}
-	}
 
-	argv += optind;
-	argc -= optind;
-
-	if (argc != 2)
-		goto out_usage;
-
-	if (hash_alg == NULL)
-		hash_alg = &fsverity_hash_algs[FS_VERITY_HASH_ALG_DEFAULT];
+	if (hash_alg == NULL) {
+		status = -EINVAL;
+		goto out;
+	}
 
 	if (block_size == 0)
 		block_size = get_default_block_size();
 
 	if (keyfile == NULL) {
-		error_msg("Missing --key argument");
-		goto out_usage;
+		status = -EINVAL;
+		goto out;
 	}
 	if (certfile == NULL)
 		certfile = keyfile;
@@ -603,33 +511,21 @@ int fsverity_cmd_sign(const struct fsverity_command *cmd,
 	digest->digest_algorithm = cpu_to_le16(hash_alg - fsverity_hash_algs);
 	digest->digest_size = cpu_to_le16(hash_alg->digest_size);
 
-	if (!compute_file_measurement(argv[0], hash_alg, block_size,
+	if (!compute_file_measurement(filename, hash_alg, block_size,
 				      salt, salt_size, digest->digest))
 		goto out_err;
 
 	if (!sign_data(digest, sizeof(*digest) + hash_alg->digest_size,
-		       keyfile, certfile, hash_alg, &sig, &sig_size))
-		goto out_err;
-
-	if (!write_signature(argv[1], sig, sig_size))
+		       keyfile, certfile, hash_alg, sig, sig_size))
 		goto out_err;
 
-	bin2hex(digest->digest, hash_alg->digest_size, digest_hex);
-	printf("Signed file '%s' (%s:%s)\n", argv[0], hash_alg->name,
-	       digest_hex);
+	*retdigest = digest;
 	status = 0;
 out:
-	free(salt);
-	free(digest);
-	free(sig);
 	return status;
 
 out_err:
 	status = 1;
 	goto out;
 
-out_usage:
-	usage(cmd, stderr);
-	status = 2;
-	goto out;
 }
diff --git a/commands.h b/commands.h
index 98f9745..c38fcea 100644
--- a/commands.h
+++ b/commands.h
@@ -5,17 +5,36 @@
 #include <stdio.h>
 
 #include "util.h"
+#include "hash_algs.h"
+#include "fsverity_uapi.h"
 
 struct fsverity_command;
 
+/*
+ * Format in which verity file measurements are signed.  This is the same as
+ * 'struct fsverity_digest', except here some magic bytes are prepended to
+ * provide some context about what is being signed in case the same key is used
+ * for non-fsverity purposes, and here the fields have fixed endianness.
+ */
+struct fsverity_signed_digest {
+	char magic[8];			/* must be "FSVerity" */
+	__le16 digest_algorithm;
+	__le16 digest_size;
+	__u8 digest[];
+};
+
+
 void usage(const struct fsverity_command *cmd, FILE *fp);
 
 int fsverity_cmd_enable(const struct fsverity_command *cmd,
 			int argc, char *argv[]);
 int fsverity_cmd_measure(const struct fsverity_command *cmd,
 			 int argc, char *argv[]);
-int fsverity_cmd_sign(const struct fsverity_command *cmd,
-		      int argc, char *argv[]);
+int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
+		      u32 block_size, u8 *salt, u32 salt_size,
+		      const char *keyfile, const char *certfile,
+		      struct fsverity_signed_digest **retdigest,
+		      u8 **sig, u32 *sig_size);
 
 bool parse_block_size_option(const char *arg, u32 *size_ptr);
 u32 get_default_block_size(void);
diff --git a/fsverity.c b/fsverity.c
index 9a44df1..6246031 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -7,14 +7,141 @@
  * Written by Eric Biggers.
  */
 
+#include <fcntl.h>
 #include <limits.h>
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
+#include <getopt.h>
+#include <errno.h>
 
 #include "commands.h"
 #include "hash_algs.h"
 
+enum {
+	OPT_HASH_ALG,
+	OPT_BLOCK_SIZE,
+	OPT_SALT,
+	OPT_KEY,
+	OPT_CERT,
+};
+
+static const struct option longopts[] = {
+	{"hash-alg",	required_argument, NULL, OPT_HASH_ALG},
+	{"block-size",	required_argument, NULL, OPT_BLOCK_SIZE},
+	{"salt",	required_argument, NULL, OPT_SALT},
+	{"key",		required_argument, NULL, OPT_KEY},
+	{"cert",	required_argument, NULL, OPT_CERT},
+	{NULL, 0, NULL, 0}
+};
+
+static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
+{
+	struct filedes file;
+	bool ok;
+
+	if (!open_file(&file, filename, O_WRONLY|O_CREAT|O_TRUNC, 0644))
+		return false;
+	ok = full_write(&file, sig, sig_size);
+	ok &= filedes_close(&file);
+	return ok;
+}
+
+int wrap_cmd_sign(const struct fsverity_command *cmd, int argc, char *argv[])
+{
+	struct fsverity_signed_digest *digest = NULL;
+	u8 *sig = NULL;
+	u32 sig_size;
+	const struct fsverity_hash_alg *hash_alg = NULL;
+	u32 block_size = 0;
+	u8 *salt = NULL;
+	u32 salt_size = 0;
+	const char *keyfile = NULL;
+	const char *certfile = NULL;
+	int c, status;
+	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
+
+	while ((c = getopt_long(argc, argv, "", longopts, NULL)) != -1) {
+		switch (c) {
+		case OPT_HASH_ALG:
+			if (hash_alg != NULL) {
+				error_msg("--hash-alg can only be specified once");
+				goto out_usage;
+			}
+			hash_alg = find_hash_alg_by_name(optarg);
+			if (hash_alg == NULL)
+				goto out_usage;
+			break;
+		case OPT_BLOCK_SIZE:
+			if (!parse_block_size_option(optarg, &block_size))
+				goto out_usage;
+			break;
+		case OPT_SALT:
+			if (!parse_salt_option(optarg, &salt, &salt_size))
+				goto out_usage;
+			break;
+		case OPT_KEY:
+			if (keyfile != NULL) {
+				error_msg("--key can only be specified once");
+				goto out_usage;
+			}
+			keyfile = optarg;
+			break;
+		case OPT_CERT:
+			if (certfile != NULL) {
+				error_msg("--cert can only be specified once");
+				goto out_usage;
+			}
+			certfile = optarg;
+			break;
+		default:
+			goto out_usage;
+		}
+	}
+
+	if (keyfile == NULL) {
+		status = -EINVAL;
+		error_msg("Missing --key argument");
+		goto out_usage;
+	}
+
+	argv += optind;
+	argc -= optind;
+
+	if (hash_alg == NULL)
+		hash_alg = &fsverity_hash_algs[FS_VERITY_HASH_ALG_DEFAULT];
+
+	if (argc != 2)
+		goto out_usage;
+
+	status = fsverity_cmd_sign(argv[0], hash_alg, block_size, salt, salt_size,
+				   keyfile, certfile, &digest, &sig, &sig_size);
+	if (status == -EINVAL)
+		goto out_usage;
+	if (status != 0)
+		goto out;
+
+	if (!write_signature(argv[1], sig, sig_size)) {
+		status = -EIO;
+		goto out;
+	}
+
+	bin2hex(digest->digest, hash_alg->digest_size, digest_hex);
+	printf("Signed file '%s' (%s:%s)\n", argv[0], hash_alg->name,
+	       digest_hex);
+
+ out:
+	free(salt);
+	free(digest);
+	free(sig);
+	return status;
+
+ out_usage:
+	usage(cmd, stderr);
+	status = 2;
+	goto out;
+}
+
 static const struct fsverity_command {
 	const char *name;
 	int (*func)(const struct fsverity_command *cmd, int argc, char *argv[]);
@@ -38,7 +165,7 @@ static const struct fsverity_command {
 "    fsverity measure FILE...\n"
 	}, {
 		.name = "sign",
-		.func = fsverity_cmd_sign,
+		.func = wrap_cmd_sign,
 		.short_desc = "Sign a file for fs-verity",
 		.usage_str =
 "    fsverity sign FILE OUT_SIGFILE --key=KEYFILE\n"
-- 
2.24.1

[PATCH 3/7] Make fsverity_cmd_measure() a library function

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

This splits the cmdline option parsing into wrap_cmd_measure() and
fsverity_cmd_measure() is just the basic call to the ioctl.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_measure.c | 49 +++++++++----------------------------------------
 commands.h    |  3 +--
 fsverity.c    | 50 +++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 59 insertions(+), 43 deletions(-)

diff --git a/cmd_measure.c b/cmd_measure.c
index 574e3ca..fc3108d 100644
--- a/cmd_measure.c
+++ b/cmd_measure.c
@@ -13,50 +13,24 @@
 
 #include "commands.h"
 #include "fsverity_uapi.h"
-#include "hash_algs.h"
 
 /* Display the measurement of the given verity file(s). */
-int fsverity_cmd_measure(const struct fsverity_command *cmd,
-			 int argc, char *argv[])
+int fsverity_cmd_measure(char *filename, struct fsverity_digest *d)
 {
-	struct fsverity_digest *d = NULL;
 	struct filedes file;
-	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
-	const struct fsverity_hash_alg *hash_alg;
-	char _hash_alg_name[32];
-	const char *hash_alg_name;
 	int status;
-	int i;
 
-	if (argc < 2)
-		goto out_usage;
+	if (!open_file(&file, filename, O_RDONLY, 0))
+		goto out_err;
 
-	d = xzalloc(sizeof(*d) + FS_VERITY_MAX_DIGEST_SIZE);
-
-	for (i = 1; i < argc; i++) {
-		d->digest_size = FS_VERITY_MAX_DIGEST_SIZE;
-
-		if (!open_file(&file, argv[i], O_RDONLY, 0))
-			goto out_err;
-		if (ioctl(file.fd, FS_IOC_MEASURE_VERITY, d) != 0) {
-			error_msg_errno("FS_IOC_MEASURE_VERITY failed on '%s'",
-					file.name);
-			filedes_close(&file);
-			goto out_err;
-		}
+	if (ioctl(file.fd, FS_IOC_MEASURE_VERITY, d) != 0) {
+		error_msg_errno("FS_IOC_MEASURE_VERITY failed on '%s'",
+				file.name);
 		filedes_close(&file);
-
-		ASSERT(d->digest_size <= FS_VERITY_MAX_DIGEST_SIZE);
-		bin2hex(d->digest, d->digest_size, digest_hex);
-		hash_alg = find_hash_alg_by_num(d->digest_algorithm);
-		if (hash_alg) {
-			hash_alg_name = hash_alg->name;
-		} else {
-			sprintf(_hash_alg_name, "ALG_%u", d->digest_algorithm);
-			hash_alg_name = _hash_alg_name;
-		}
-		printf("%s:%s %s\n", hash_alg_name, digest_hex, argv[i]);
+		goto out_err;
 	}
+	filedes_close(&file);
+
 	status = 0;
 out:
 	free(d);
@@ -65,9 +39,4 @@ out:
 out_err:
 	status = 1;
 	goto out;
-
-out_usage:
-	usage(cmd, stderr);
-	status = 2;
-	goto out;
 }
diff --git a/commands.h b/commands.h
index c38fcea..3e07f3d 100644
--- a/commands.h
+++ b/commands.h
@@ -28,8 +28,7 @@ void usage(const struct fsverity_command *cmd, FILE *fp);
 
 int fsverity_cmd_enable(const struct fsverity_command *cmd,
 			int argc, char *argv[]);
-int fsverity_cmd_measure(const struct fsverity_command *cmd,
-			 int argc, char *argv[]);
+int fsverity_cmd_measure(char *filename, struct fsverity_digest *d);
 int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 		      u32 block_size, u8 *salt, u32 salt_size,
 		      const char *keyfile, const char *certfile,
diff --git a/fsverity.c b/fsverity.c
index 6246031..49eca14 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -142,6 +142,54 @@ int wrap_cmd_sign(const struct fsverity_command *cmd, int argc, char *argv[])
 	goto out;
 }
 
+int wrap_cmd_measure(const struct fsverity_command *cmd,
+		     int argc, char *argv[])
+{
+	struct fsverity_digest *d = NULL;
+	char digest_hex[FS_VERITY_MAX_DIGEST_SIZE * 2 + 1];
+	const struct fsverity_hash_alg *hash_alg;
+	char _hash_alg_name[32];
+	const char *hash_alg_name;
+	int status;
+	int i;
+
+	if (argc < 2)
+		goto out_usage;
+
+	d = xzalloc(sizeof(*d) + FS_VERITY_MAX_DIGEST_SIZE);
+
+	for (i = 1; i < argc; i++) {
+		d->digest_size = FS_VERITY_MAX_DIGEST_SIZE;
+
+		status = fsverity_cmd_measure(argv[i], d);
+		if (status)
+			goto out_err;
+
+		ASSERT(d->digest_size <= FS_VERITY_MAX_DIGEST_SIZE);
+		bin2hex(d->digest, d->digest_size, digest_hex);
+		hash_alg = find_hash_alg_by_num(d->digest_algorithm);
+		if (hash_alg) {
+			hash_alg_name = hash_alg->name;
+		} else {
+			sprintf(_hash_alg_name, "ALG_%u", d->digest_algorithm);
+			hash_alg_name = _hash_alg_name;
+		}
+		printf("%s:%s %s\n", hash_alg_name, digest_hex, argv[i]);
+	}
+out:
+	free(d);
+	return status;
+
+out_err:
+	status = 1;
+	goto out;
+
+out_usage:
+	usage(cmd, stderr);
+	status = 2;
+	goto out;
+}
+
 static const struct fsverity_command {
 	const char *name;
 	int (*func)(const struct fsverity_command *cmd, int argc, char *argv[]);
@@ -158,7 +206,7 @@ static const struct fsverity_command {
 "               [--signature=SIGFILE]\n"
 	}, {
 		.name = "measure",
-		.func = fsverity_cmd_measure,
+		.func = wrap_cmd_measure,
 		.short_desc =
 "Display the measurement of the given verity file(s)",
 		.usage_str =
-- 
2.24.1

[PATCH 4/7] Make fsverity_cmd_enable a library call()

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

Split the parsing of command line arguments from the actual cmd call,
which allows this to be called by other users.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_enable.c | 131 ++----------------------------------------------
 commands.h   |   3 +-
 fsverity.c   | 137 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 140 insertions(+), 131 deletions(-)

diff --git a/cmd_enable.c b/cmd_enable.c
index 1646299..8c55722 100644
--- a/cmd_enable.c
+++ b/cmd_enable.c
@@ -18,137 +18,17 @@
 #include "fsverity_uapi.h"
 #include "hash_algs.h"
 
-static bool parse_hash_alg_option(const char *arg, u32 *alg_ptr)
-{
-	char *end;
-	unsigned long n = strtoul(arg, &end, 10);
-	const struct fsverity_hash_alg *alg;
-
-	if (*alg_ptr != 0) {
-		error_msg("--hash-alg can only be specified once");
-		return false;
-	}
-
-	/* Specified by number? */
-	if (n > 0 && n < INT32_MAX && *end == '\0') {
-		*alg_ptr = n;
-		return true;
-	}
-
-	/* Specified by name? */
-	alg = find_hash_alg_by_name(arg);
-	if (alg != NULL) {
-		*alg_ptr = alg - fsverity_hash_algs;
-		return true;
-	}
-	return false;
-}
-
-static bool read_signature(const char *filename, u8 **sig_ret,
-			   u32 *sig_size_ret)
-{
-	struct filedes file = { .fd = -1 };
-	u64 file_size;
-	u8 *sig = NULL;
-	bool ok = false;
-
-	if (!open_file(&file, filename, O_RDONLY, 0))
-		goto out;
-	if (!get_file_size(&file, &file_size))
-		goto out;
-	if (file_size <= 0) {
-		error_msg("signature file '%s' is empty", filename);
-		goto out;
-	}
-	if (file_size > 1000000) {
-		error_msg("signature file '%s' is too large", filename);
-		goto out;
-	}
-	sig = xmalloc(file_size);
-	if (!full_read(&file, sig, file_size))
-		goto out;
-	*sig_ret = sig;
-	*sig_size_ret = file_size;
-	sig = NULL;
-	ok = true;
-out:
-	filedes_close(&file);
-	free(sig);
-	return ok;
-}
-
-enum {
-	OPT_HASH_ALG,
-	OPT_BLOCK_SIZE,
-	OPT_SALT,
-	OPT_SIGNATURE,
-};
-
-static const struct option longopts[] = {
-	{"hash-alg",	required_argument, NULL, OPT_HASH_ALG},
-	{"block-size",	required_argument, NULL, OPT_BLOCK_SIZE},
-	{"salt",	required_argument, NULL, OPT_SALT},
-	{"signature",	required_argument, NULL, OPT_SIGNATURE},
-	{NULL, 0, NULL, 0}
-};
-
 /* Enable fs-verity on a file. */
-int fsverity_cmd_enable(const struct fsverity_command *cmd,
-			int argc, char *argv[])
+int fsverity_cmd_enable(char *filename, struct fsverity_enable_arg *arg)
 {
-	struct fsverity_enable_arg arg = { .version = 1 };
 	u8 *salt = NULL;
 	u8 *sig = NULL;
 	struct filedes file;
 	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, &arg.hash_algorithm))
-				goto out_usage;
-			break;
-		case OPT_BLOCK_SIZE:
-			if (!parse_block_size_option(optarg, &arg.block_size))
-				goto out_usage;
-			break;
-		case OPT_SALT:
-			if (!parse_salt_option(optarg, &salt, &arg.salt_size))
-				goto out_usage;
-			arg.salt_ptr = (uintptr_t)salt;
-			break;
-		case OPT_SIGNATURE:
-			if (sig != NULL) {
-				error_msg("--signature can only be specified once");
-				goto out_usage;
-			}
-			if (!read_signature(optarg, &sig, &arg.sig_size))
-				goto out_err;
-			arg.sig_ptr = (uintptr_t)sig;
-			break;
-		default:
-			goto out_usage;
-		}
-	}
 
-	argv += optind;
-	argc -= optind;
-
-	if (argc != 1)
-		goto out_usage;
-
-	if (arg.hash_algorithm == 0)
-		arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
-
-	if (arg.block_size == 0)
-		arg.block_size = get_default_block_size();
-
-	if (!open_file(&file, argv[0], O_RDONLY, 0))
+	if (!open_file(&file, filename, O_RDONLY, 0))
 		goto out_err;
-	if (ioctl(file.fd, FS_IOC_ENABLE_VERITY, &arg) != 0) {
-		error_msg_errno("FS_IOC_ENABLE_VERITY failed on '%s'",
-				file.name);
+	if (ioctl(file.fd, FS_IOC_ENABLE_VERITY, arg) != 0) {
 		filedes_close(&file);
 		goto out_err;
 	}
@@ -164,9 +44,4 @@ out:
 out_err:
 	status = 1;
 	goto out;
-
-out_usage:
-	usage(cmd, stderr);
-	status = 2;
-	goto out;
 }
diff --git a/commands.h b/commands.h
index 3e07f3d..e490c25 100644
--- a/commands.h
+++ b/commands.h
@@ -26,8 +26,7 @@ struct fsverity_signed_digest {
 
 void usage(const struct fsverity_command *cmd, FILE *fp);
 
-int fsverity_cmd_enable(const struct fsverity_command *cmd,
-			int argc, char *argv[]);
+int fsverity_cmd_enable(char *filename, struct fsverity_enable_arg *arg);
 int fsverity_cmd_measure(char *filename, struct fsverity_digest *d);
 int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 		      u32 block_size, u8 *salt, u32 salt_size,
diff --git a/fsverity.c b/fsverity.c
index 49eca14..b4e67a2 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -24,6 +24,7 @@ enum {
 	OPT_SALT,
 	OPT_KEY,
 	OPT_CERT,
+	OPT_SIGNATURE,
 };
 
 static const struct option longopts[] = {
@@ -35,6 +36,14 @@ static const struct option longopts[] = {
 	{NULL, 0, NULL, 0}
 };
 
+static const struct option enable_longopts[] = {
+	{"hash-alg",	required_argument, NULL, OPT_HASH_ALG},
+	{"block-size",	required_argument, NULL, OPT_BLOCK_SIZE},
+	{"salt",	required_argument, NULL, OPT_SALT},
+	{"signature",	required_argument, NULL, OPT_SIGNATURE},
+	{NULL, 0, NULL, 0}
+};
+
 static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
 {
 	struct filedes file;
@@ -47,6 +56,65 @@ static bool write_signature(const char *filename, const u8 *sig, u32 sig_size)
 	return ok;
 }
 
+static bool read_signature(const char *filename, u8 **sig_ret,
+			   u32 *sig_size_ret)
+{
+	struct filedes file = { .fd = -1 };
+	u64 file_size;
+	u8 *sig = NULL;
+	bool ok = false;
+
+	if (!open_file(&file, filename, O_RDONLY, 0))
+		goto out;
+	if (!get_file_size(&file, &file_size))
+		goto out;
+	if (file_size <= 0) {
+		error_msg("signature file '%s' is empty", filename);
+		goto out;
+	}
+	if (file_size > 1000000) {
+		error_msg("signature file '%s' is too large", filename);
+		goto out;
+	}
+	sig = xmalloc(file_size);
+	if (!full_read(&file, sig, file_size))
+		goto out;
+	*sig_ret = sig;
+	*sig_size_ret = file_size;
+	sig = NULL;
+	ok = true;
+out:
+	filedes_close(&file);
+	free(sig);
+	return ok;
+}
+
+static bool parse_hash_alg_option(const char *arg, u32 *alg_ptr)
+{
+	char *end;
+	unsigned long n = strtoul(arg, &end, 10);
+	const struct fsverity_hash_alg *alg;
+
+	if (*alg_ptr != 0) {
+		error_msg("--hash-alg can only be specified once");
+		return false;
+	}
+
+	/* Specified by number? */
+	if (n > 0 && n < INT32_MAX && *end == '\0') {
+		*alg_ptr = n;
+		return true;
+	}
+
+	/* Specified by name? */
+	alg = find_hash_alg_by_name(arg);
+	if (alg != NULL) {
+		*alg_ptr = alg - fsverity_hash_algs;
+		return true;
+	}
+	return false;
+}
+
 int wrap_cmd_sign(const struct fsverity_command *cmd, int argc, char *argv[])
 {
 	struct fsverity_signed_digest *digest = NULL;
@@ -190,6 +258,73 @@ out_usage:
 	goto out;
 }
 
+int wrap_cmd_enable(const struct fsverity_command *cmd,
+		    int argc, char *argv[])
+{
+	struct fsverity_enable_arg arg = { .version = 1 };
+	u8 *salt = NULL;
+	u8 *sig = NULL;
+	int status;
+	int c;
+
+	while ((c = getopt_long(argc, argv, "", enable_longopts, NULL)) != -1) {
+		switch (c) {
+		case OPT_HASH_ALG:
+			if (!parse_hash_alg_option(optarg, &arg.hash_algorithm))
+				goto out_usage;
+			break;
+		case OPT_BLOCK_SIZE:
+			if (!parse_block_size_option(optarg, &arg.block_size))
+				goto out_usage;
+			break;
+		case OPT_SALT:
+			if (!parse_salt_option(optarg, &salt, &arg.salt_size))
+				goto out_usage;
+			arg.salt_ptr = (uintptr_t)salt;
+			break;
+		case OPT_SIGNATURE:
+			if (sig != NULL) {
+				error_msg("--signature can only be specified once");
+				goto out_usage;
+			}
+			if (!read_signature(optarg, &sig, &arg.sig_size)) {
+				error_msg("unable to read signature file %s",
+					  optarg);
+				status = 1;
+				goto out;
+			}
+			arg.sig_ptr = (uintptr_t)sig;
+			break;
+		default:
+			goto out_usage;
+		}
+	}
+
+	argv += optind;
+	argc -= optind;
+
+	if (argc != 1)
+		goto out_usage;
+
+	if (arg.hash_algorithm == 0)
+		arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
+
+	if (arg.block_size == 0)
+		arg.block_size = get_default_block_size();
+
+	status = fsverity_cmd_enable(argv[0], &arg);
+
+out:
+	free(salt);
+	free(sig);
+	return status;
+
+out_usage:
+	usage(cmd, stderr);
+	status = 2;
+	goto out;
+}
+
 static const struct fsverity_command {
 	const char *name;
 	int (*func)(const struct fsverity_command *cmd, int argc, char *argv[]);
@@ -198,7 +333,7 @@ static const struct fsverity_command {
 } fsverity_commands[] = {
 	{
 		.name = "enable",
-		.func = fsverity_cmd_enable,
+		.func = wrap_cmd_enable,
 		.short_desc = "Enable fs-verity on a file",
 		.usage_str =
 "    fsverity enable FILE\n"
-- 
2.24.1

[PATCH 5/7] Rename commands.h to fsverity.h

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

This is a more appropriate name to provide the API for the shared library

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_enable.c             | 2 +-
 cmd_measure.c            | 2 +-
 cmd_sign.c               | 2 +-
 fsverity.c               | 2 +-
 commands.h => fsverity.h | 0
 5 files changed, 4 insertions(+), 4 deletions(-)
 rename commands.h => fsverity.h (100%)

diff --git a/cmd_enable.c b/cmd_enable.c
index 8c55722..732f3f6 100644
--- a/cmd_enable.c
+++ b/cmd_enable.c
@@ -14,7 +14,7 @@
 #include <string.h>
 #include <sys/ioctl.h>
 
-#include "commands.h"
+#include "fsverity.h"
 #include "fsverity_uapi.h"
 #include "hash_algs.h"
 
diff --git a/cmd_measure.c b/cmd_measure.c
index fc3108d..3cd313e 100644
--- a/cmd_measure.c
+++ b/cmd_measure.c
@@ -11,7 +11,7 @@
 #include <stdlib.h>
 #include <sys/ioctl.h>
 
-#include "commands.h"
+#include "fsverity.h"
 #include "fsverity_uapi.h"
 
 /* Display the measurement of the given verity file(s). */
diff --git a/cmd_sign.c b/cmd_sign.c
index 2d3fa54..42779f2 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -16,7 +16,7 @@
 #include <stdlib.h>
 #include <string.h>
 
-#include "commands.h"
+#include "fsverity.h"
 #include "fsverity_uapi.h"
 #include "hash_algs.h"
 
diff --git a/fsverity.c b/fsverity.c
index b4e67a2..f0e94bf 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -15,7 +15,7 @@
 #include <getopt.h>
 #include <errno.h>
 
-#include "commands.h"
+#include "fsverity.h"
 #include "hash_algs.h"
 
 enum {
diff --git a/commands.h b/fsverity.h
similarity index 100%
rename from commands.h
rename to fsverity.h
-- 
2.24.1

[PATCH 6/7] Move cmdline helper functions to fsverity.c

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

There is no need for these to live in the shared library. In addition
move get_default_block_size() to the library and rename it appropriately.

Signed-off-by: Jes Sorensen <jsorensen@fb.com>
---
 cmd_sign.c |  2 +-
 fsverity.c | 25 +++++++++----------------
 fsverity.h |  8 +-------
 util.c     | 13 +++++++++++++
 4 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/cmd_sign.c b/cmd_sign.c
index 42779f2..a0bd168 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -497,7 +497,7 @@ int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 	}
 
 	if (block_size == 0)
-		block_size = get_default_block_size();
+		block_size = fsverity_get_default_block_size();
 
 	if (keyfile == NULL) {
 		status = -EINVAL;
diff --git a/fsverity.c b/fsverity.c
index f0e94bf..45bf0cc 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -18,6 +18,12 @@
 #include "fsverity.h"
 #include "hash_algs.h"
 
+struct fsverity_command;
+
+static bool parse_block_size_option(const char *arg, u32 *size_ptr);
+static bool parse_salt_option(const char *arg, u8 **salt_ptr, u32 *salt_size_ptr);
+static void usage(const struct fsverity_command *cmd, FILE *fp);
+
 enum {
 	OPT_HASH_ALG,
 	OPT_BLOCK_SIZE,
@@ -310,7 +316,7 @@ int wrap_cmd_enable(const struct fsverity_command *cmd,
 		arg.hash_algorithm = FS_VERITY_HASH_ALG_DEFAULT;
 
 	if (arg.block_size == 0)
-		arg.block_size = get_default_block_size();
+		arg.block_size = fsverity_get_default_block_size();
 
 	status = fsverity_cmd_enable(argv[0], &arg);
 
@@ -437,7 +443,7 @@ static const struct fsverity_command *find_command(const char *name)
 	return NULL;
 }
 
-bool parse_block_size_option(const char *arg, u32 *size_ptr)
+static bool parse_block_size_option(const char *arg, u32 *size_ptr)
 {
 	char *end;
 	unsigned long n = strtoul(arg, &end, 10);
@@ -455,7 +461,7 @@ bool parse_block_size_option(const char *arg, u32 *size_ptr)
 	return true;
 }
 
-bool parse_salt_option(const char *arg, u8 **salt_ptr, u32 *salt_size_ptr)
+static bool parse_salt_option(const char *arg, u8 **salt_ptr, u32 *salt_size_ptr)
 {
 	if (*salt_ptr != NULL) {
 		error_msg("--salt can only be specified once");
@@ -470,19 +476,6 @@ bool parse_salt_option(const char *arg, u8 **salt_ptr, u32 *salt_size_ptr)
 	return true;
 }
 
-u32 get_default_block_size(void)
-{
-	long n = sysconf(_SC_PAGESIZE);
-
-	if (n <= 0 || n >= INT_MAX || !is_power_of_2(n)) {
-		fprintf(stderr,
-			"Warning: invalid _SC_PAGESIZE (%ld).  Assuming 4K blocks.\n",
-			n);
-		return 4096;
-	}
-	return n;
-}
-
 int main(int argc, char *argv[])
 {
 	const struct fsverity_command *cmd;
diff --git a/fsverity.h b/fsverity.h
index e490c25..bb2f337 100644
--- a/fsverity.h
+++ b/fsverity.h
@@ -8,8 +8,6 @@
 #include "hash_algs.h"
 #include "fsverity_uapi.h"
 
-struct fsverity_command;
-
 /*
  * Format in which verity file measurements are signed.  This is the same as
  * 'struct fsverity_digest', except here some magic bytes are prepended to
@@ -24,7 +22,7 @@ struct fsverity_signed_digest {
 };
 
 
-void usage(const struct fsverity_command *cmd, FILE *fp);
+u32 fsverity_get_default_block_size(void);
 
 int fsverity_cmd_enable(char *filename, struct fsverity_enable_arg *arg);
 int fsverity_cmd_measure(char *filename, struct fsverity_digest *d);
@@ -34,8 +32,4 @@ int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 		      struct fsverity_signed_digest **retdigest,
 		      u8 **sig, u32 *sig_size);
 
-bool parse_block_size_option(const char *arg, u32 *size_ptr);
-u32 get_default_block_size(void);
-bool parse_salt_option(const char *arg, u8 **salt_ptr, u32 *salt_size_ptr);
-
 #endif /* COMMANDS_H */
diff --git a/util.c b/util.c
index 2218f2e..e4ccd2a 100644
--- a/util.c
+++ b/util.c
@@ -213,3 +213,16 @@ void bin2hex(const u8 *bin, size_t bin_len, char *hex)
 	}
 	*hex = '\0';
 }
+
+u32 fsverity_get_default_block_size(void)
+{
+	long n = sysconf(_SC_PAGESIZE);
+
+	if (n <= 0 || n >= INT_MAX || !is_power_of_2(n)) {
+		fprintf(stderr,
+			"Warning: invalid _SC_PAGESIZE (%ld).  Assuming 4K blocks.\n",
+			n);
+		return 4096;
+	}
+	return n;
+}
-- 
2.24.1

[PATCH 7/7] cmd_sign: fsverity_cmd_sign() into two functions

[Jes Sorensen]

From: Jes Sorensen <jsorensen@fb.com>

This splits cmd_sign() into a gen_digest() and a sign_digest()
function, and fixes fsverity.c to use them appropriately.
---
 cmd_sign.c | 50 +++++++++++++++++++++++++++++++++-----------------
 fsverity.c |  8 ++++++--
 fsverity.h | 13 ++++++++-----
 3 files changed, 47 insertions(+), 24 deletions(-)

diff --git a/cmd_sign.c b/cmd_sign.c
index a0bd168..ba68243 100644
--- a/cmd_sign.c
+++ b/cmd_sign.c
@@ -481,12 +481,11 @@ out:
 	return ok;
 }
 
-/* Sign a file for fs-verity by computing its measurement, then signing it. */
-int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
-		      u32 block_size, u8 *salt, u32 salt_size,
-		      const char *keyfile, const char *certfile,
-		      struct fsverity_signed_digest **retdigest,
-		      u8 **sig, u32 *sig_size)
+/* Generate the fsverity digest computing its measurement. */
+int fsverity_cmd_gen_digest(char *filename,
+			    const struct fsverity_hash_alg *hash_alg,
+			    u32 block_size, u8 *salt, u32 salt_size,
+			    struct fsverity_signed_digest **retdigest)
 {
 	struct fsverity_signed_digest *digest = NULL;
 	int status;
@@ -499,13 +498,6 @@ int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 	if (block_size == 0)
 		block_size = fsverity_get_default_block_size();
 
-	if (keyfile == NULL) {
-		status = -EINVAL;
-		goto out;
-	}
-	if (certfile == NULL)
-		certfile = keyfile;
-
 	digest = xzalloc(sizeof(*digest) + hash_alg->digest_size);
 	memcpy(digest->magic, "FSVerity", 8);
 	digest->digest_algorithm = cpu_to_le16(hash_alg - fsverity_hash_algs);
@@ -515,10 +507,6 @@ int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
 				      salt, salt_size, digest->digest))
 		goto out_err;
 
-	if (!sign_data(digest, sizeof(*digest) + hash_alg->digest_size,
-		       keyfile, certfile, hash_alg, sig, sig_size))
-		goto out_err;
-
 	*retdigest = digest;
 	status = 0;
 out:
@@ -529,3 +517,31 @@ out_err:
 	goto out;
 
 }
+
+/* Sign a pre-generated fsverity_signed_digest structure */
+int fsverity_cmd_sign_digest(struct fsverity_signed_digest *digest,
+			     const struct fsverity_hash_alg *hash_alg,
+			     const char *keyfile, const char *certfile,
+			     u8 **sig, u32 *sig_size)
+{
+	int status;
+
+	if (keyfile == NULL) {
+		status = -EINVAL;
+		goto out;
+	}
+	if (certfile == NULL)
+		certfile = keyfile;
+
+	if (!sign_data(digest, sizeof(*digest) + hash_alg->digest_size,
+		       keyfile, certfile, hash_alg, sig, sig_size))
+		goto out_err;
+
+	status = 0;
+ out:
+	return status;
+
+ out_err:
+	status = 1;
+	goto out;
+}
diff --git a/fsverity.c b/fsverity.c
index 45bf0cc..3fcafcb 100644
--- a/fsverity.c
+++ b/fsverity.c
@@ -188,8 +188,12 @@ int wrap_cmd_sign(const struct fsverity_command *cmd, int argc, char *argv[])
 	if (argc != 2)
 		goto out_usage;
 
-	status = fsverity_cmd_sign(argv[0], hash_alg, block_size, salt, salt_size,
-				   keyfile, certfile, &digest, &sig, &sig_size);
+	status = fsverity_cmd_gen_digest(argv[0], hash_alg, block_size,
+					 salt, salt_size, &digest);
+	if (status)
+		goto out_usage;
+	status = fsverity_cmd_sign_digest(digest, hash_alg, keyfile, certfile,
+					  &sig, &sig_size);
 	if (status == -EINVAL)
 		goto out_usage;
 	if (status != 0)
diff --git a/fsverity.h b/fsverity.h
index bb2f337..695bdac 100644
--- a/fsverity.h
+++ b/fsverity.h
@@ -26,10 +26,13 @@ u32 fsverity_get_default_block_size(void);
 
 int fsverity_cmd_enable(char *filename, struct fsverity_enable_arg *arg);
 int fsverity_cmd_measure(char *filename, struct fsverity_digest *d);
-int fsverity_cmd_sign(char *filename, const struct fsverity_hash_alg *hash_alg,
-		      u32 block_size, u8 *salt, u32 salt_size,
-		      const char *keyfile, const char *certfile,
-		      struct fsverity_signed_digest **retdigest,
-		      u8 **sig, u32 *sig_size);
+int fsverity_cmd_gen_digest(char *filename,
+			    const struct fsverity_hash_alg *hash_alg,
+			    u32 block_size, u8 *salt, u32 salt_size,
+			    struct fsverity_signed_digest **retdigest);
+int fsverity_cmd_sign_digest(struct fsverity_signed_digest *digest,
+			     const struct fsverity_hash_alg *hash_alg,
+			     const char *keyfile, const char *certfile,
+			     u8 **sig, u32 *sig_size);
 
 #endif /* COMMANDS_H */
-- 
2.24.1

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

[Eric Biggers]

Hi Jes,

On Mon, Feb 10, 2020 at 07:00:30PM -0500, Jes Sorensen wrote:
> From: Jes Sorensen <jsorensen@fb.com>
> 
> Hi,
> 
> I am looking at what it will take to add support for fsverity
> signatures to rpm, similar to how rpm supports IMA signatures.
> 
> In order to do so, it makes sense to split the fsverity util into a
> shared library and the command line tool, so the core functions can be
> used from other applciations. Alternatively I will have to copy over a
> good chunk of the code into rpm, which makes it nasty to support long
> term.
> 
> This is a first stab at doing that, and I'd like to get some feedback
> on the approach.
> 
> I basically split it into four functions:
> 
> fsverity_cmd_gen_digest(): Build the digest, but do not sign it
> fsverity_cmd_sign():       Sign the digest structure
> fsverity_cmd_measure():    Measure a file, basically 'fsverity measure'
> fsverity_cmd_enable():     Enable verity on a file, basically 'fsverity enable'
> 
> If we can agree on the approach, then I am happy to deal with the full
> libtoolification etc.
> 

Before we do all this work, can you take a step back and explain the use case so
that we can be sure it's really worthwhile?

fsverity_cmd_enable() and fsverity_cmd_measure() would just be trivial wrappers
around the FS_IOC_ENABLE_VERITY and FS_IOC_MEASURE_VERITY ioctls, so they don't
need a library.  [Aside: I'd suggest calling these fsverity_enable() and
fsverity_measure(), and leaving "cmd" for the command-line wrappers.] 

That leaves signing as the only real point of the library.  But do you actually
need to be able to *sign* the files via the rpm binary, or do you just need to
be able to install already-created signatures?  I.e., can the signatures instead
just be created with 'fsverity sign' when building the RPMs?

Separately, before you start building something around fs-verity's builtin
signature verification support, have you also considered adding support for
fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
signature mechanism.  The IMA maintainer has been expressed interested in that.
If rpm already supports IMA signatures, maybe that way would be a better fit?

- Eric

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

[Jes Sorensen]

On 2/11/20 2:22 PM, Eric Biggers wrote:
> Hi Jes,
> 
> On Mon, Feb 10, 2020 at 07:00:30PM -0500, Jes Sorensen wrote:
>> From: Jes Sorensen <jsorensen@fb.com>
>> If we can agree on the approach, then I am happy to deal with the full
>> libtoolification etc.
> 
> Before we do all this work, can you take a step back and explain the use case so
> that we can be sure it's really worthwhile?
> 
> fsverity_cmd_enable() and fsverity_cmd_measure() would just be trivial wrappers
> around the FS_IOC_ENABLE_VERITY and FS_IOC_MEASURE_VERITY ioctls, so they don't
> need a library.  [Aside: I'd suggest calling these fsverity_enable() and
> fsverity_measure(), and leaving "cmd" for the command-line wrappers.] 
> 
> That leaves signing as the only real point of the library.  But do you actually
> need to be able to *sign* the files via the rpm binary, or do you just need to
> be able to install already-created signatures?  I.e., can the signatures instead
> just be created with 'fsverity sign' when building the RPMs?

So I basically want to be able to carry verity signatures in RPM as RPM
internal data, similar to how it supports IMA signatures. I want to be
able to install those without relying on post-install scripts and
signature files being distributed as actual files that gets installed,
just to have to remove them. This is how IMA support is integrated into
RPM as well.

Right now the RPM approach for signatures involves two steps, a build
digest phase, and a sign the digest phase.

The reason I included enable and measure was for completeness. I don't
care wildly about those.

> Separately, before you start building something around fs-verity's builtin
> signature verification support, have you also considered adding support for
> fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
> signature mechanism.  The IMA maintainer has been expressed interested in that.
> If rpm already supports IMA signatures, maybe that way would be a better fit?

I looked at IMA and it is overly complex. It is not obvious to me how
you would get around that without the full complexity of IMA? The beauty
of fsverity's approach is the simplicity of relying on just the kernel
keyring for validation of the signature. If you have explicit
suggestions, I am certainly happy to look at it.

Thanks,
Jes

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

[Eric Biggers]

On Tue, Feb 11, 2020 at 05:09:22PM -0500, Jes Sorensen wrote:
> On 2/11/20 2:22 PM, Eric Biggers wrote:
> > Hi Jes,
> > 
> > On Mon, Feb 10, 2020 at 07:00:30PM -0500, Jes Sorensen wrote:
> >> From: Jes Sorensen <jsorensen@fb.com>
> >> If we can agree on the approach, then I am happy to deal with the full
> >> libtoolification etc.
> > 
> > Before we do all this work, can you take a step back and explain the use case so
> > that we can be sure it's really worthwhile?
> > 
> > fsverity_cmd_enable() and fsverity_cmd_measure() would just be trivial wrappers
> > around the FS_IOC_ENABLE_VERITY and FS_IOC_MEASURE_VERITY ioctls, so they don't
> > need a library.  [Aside: I'd suggest calling these fsverity_enable() and
> > fsverity_measure(), and leaving "cmd" for the command-line wrappers.] 
> > 
> > That leaves signing as the only real point of the library.  But do you actually
> > need to be able to *sign* the files via the rpm binary, or do you just need to
> > be able to install already-created signatures?  I.e., can the signatures instead
> > just be created with 'fsverity sign' when building the RPMs?
> 
> So I basically want to be able to carry verity signatures in RPM as RPM
> internal data, similar to how it supports IMA signatures. I want to be
> able to install those without relying on post-install scripts and
> signature files being distributed as actual files that gets installed,
> just to have to remove them. This is how IMA support is integrated into
> RPM as well.
> 
> Right now the RPM approach for signatures involves two steps, a build
> digest phase, and a sign the digest phase.
> 
> The reason I included enable and measure was for completeness. I don't
> care wildly about those.

So the signing happens when the RPM is built, not when it's installed?  Are you
sure you actually need a library and not just 'fsverity sign' called from a
build script?

> 
> > Separately, before you start building something around fs-verity's builtin
> > signature verification support, have you also considered adding support for
> > fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
> > signature mechanism.  The IMA maintainer has been expressed interested in that.
> > If rpm already supports IMA signatures, maybe that way would be a better fit?
> 
> I looked at IMA and it is overly complex. It is not obvious to me how
> you would get around that without the full complexity of IMA? The beauty
> of fsverity's approach is the simplicity of relying on just the kernel
> keyring for validation of the signature. If you have explicit
> suggestions, I am certainly happy to look at it.

fs-verity's builtin signature verification feature is simple, but does it
actually do what you need?  Note that unlike IMA, it doesn't provide an
in-kernel policy about which files have to have signatures and which don't.
I.e., to get any authenticity guarantee, before using any files that are
supposed to be protected by fs-verity, userspace has to manually check whether
the fs-verity bit is actually set.  Is that part of your design?

- Eric

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

[Jes Sorensen]

On 2/11/20 6:14 PM, Eric Biggers wrote:
> On Tue, Feb 11, 2020 at 05:09:22PM -0500, Jes Sorensen wrote:
>> On 2/11/20 2:22 PM, Eric Biggers wrote:
>>> Hi Jes,
>> So I basically want to be able to carry verity signatures in RPM as RPM
>> internal data, similar to how it supports IMA signatures. I want to be
>> able to install those without relying on post-install scripts and
>> signature files being distributed as actual files that gets installed,
>> just to have to remove them. This is how IMA support is integrated into
>> RPM as well.
>>
>> Right now the RPM approach for signatures involves two steps, a build
>> digest phase, and a sign the digest phase.
>>
>> The reason I included enable and measure was for completeness. I don't
>> care wildly about those.
> 
> So the signing happens when the RPM is built, not when it's installed?  Are you
> sure you actually need a library and not just 'fsverity sign' called from a
> build script?

So the way RPM is handling these is to calculate the digest in one
place, and sign it in another. Basically the signing is a second step,
post build, using the rpmsign command. Shelling out is not a good fit
for this model.

>>> Separately, before you start building something around fs-verity's builtin
>>> signature verification support, have you also considered adding support for
>>> fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
>>> signature mechanism.  The IMA maintainer has been expressed interested in that.
>>> If rpm already supports IMA signatures, maybe that way would be a better fit?
>>
>> I looked at IMA and it is overly complex. It is not obvious to me how
>> you would get around that without the full complexity of IMA? The beauty
>> of fsverity's approach is the simplicity of relying on just the kernel
>> keyring for validation of the signature. If you have explicit
>> suggestions, I am certainly happy to look at it.
> 
> fs-verity's builtin signature verification feature is simple, but does it
> actually do what you need?  Note that unlike IMA, it doesn't provide an
> in-kernel policy about which files have to have signatures and which don't.
> I.e., to get any authenticity guarantee, before using any files that are
> supposed to be protected by fs-verity, userspace has to manually check whether
> the fs-verity bit is actually set.  Is that part of your design?

Totally aware of this, and it fits the model I am looking at.

Jes

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

[Eric Biggers]

Hi Jes,

On Tue, Feb 11, 2020 at 06:35:45PM -0500, Jes Sorensen wrote:
> On 2/11/20 6:14 PM, Eric Biggers wrote:
> > On Tue, Feb 11, 2020 at 05:09:22PM -0500, Jes Sorensen wrote:
> >> On 2/11/20 2:22 PM, Eric Biggers wrote:
> >>> Hi Jes,
> >> So I basically want to be able to carry verity signatures in RPM as RPM
> >> internal data, similar to how it supports IMA signatures. I want to be
> >> able to install those without relying on post-install scripts and
> >> signature files being distributed as actual files that gets installed,
> >> just to have to remove them. This is how IMA support is integrated into
> >> RPM as well.
> >>
> >> Right now the RPM approach for signatures involves two steps, a build
> >> digest phase, and a sign the digest phase.
> >>
> >> The reason I included enable and measure was for completeness. I don't
> >> care wildly about those.
> > 
> > So the signing happens when the RPM is built, not when it's installed?  Are you
> > sure you actually need a library and not just 'fsverity sign' called from a
> > build script?
> 
> So the way RPM is handling these is to calculate the digest in one
> place, and sign it in another. Basically the signing is a second step,
> post build, using the rpmsign command. Shelling out is not a good fit
> for this model.
> 
> >>> Separately, before you start building something around fs-verity's builtin
> >>> signature verification support, have you also considered adding support for
> >>> fs-verity to IMA?  I.e., using the fs-verity hashing mechanism with the IMA
> >>> signature mechanism.  The IMA maintainer has been expressed interested in that.
> >>> If rpm already supports IMA signatures, maybe that way would be a better fit?
> >>
> >> I looked at IMA and it is overly complex. It is not obvious to me how
> >> you would get around that without the full complexity of IMA? The beauty
> >> of fsverity's approach is the simplicity of relying on just the kernel
> >> keyring for validation of the signature. If you have explicit
> >> suggestions, I am certainly happy to look at it.
> > 
> > fs-verity's builtin signature verification feature is simple, but does it
> > actually do what you need?  Note that unlike IMA, it doesn't provide an
> > in-kernel policy about which files have to have signatures and which don't.
> > I.e., to get any authenticity guarantee, before using any files that are
> > supposed to be protected by fs-verity, userspace has to manually check whether
> > the fs-verity bit is actually set.  Is that part of your design?
> 
> Totally aware of this, and it fits the model I am looking at.
> 

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];
};

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 */



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.

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

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?

- Eric

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

[Jes Sorensen]

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

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

[Eric Biggers]

On Wed, Feb 19, 2020 at 06:49:07PM -0500, Jes Sorensen wrote:
> > 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.
> 

Hi Jes, I'd like to revisit this, as I'm concerned about future use cases where
software under other licenses (e.g. LGPL, MIT, or Apache 2.0) might want to use
libfsverity -- especially if libfsverity grows more functionality.

Also, fsverity-utils links to OpenSSL, which some people (e.g. Debian) consider
to be incompatible with GPLv2.

We think the MIT license (https://opensource.org/licenses/MIT) would offer the
most flexibility.  Are you okay with changing the license of fsverity-utils to
MIT?  If so, I'll send a patch and you can give an Acked-by on it.

Thanks!

- Eric

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

[Jes Sorensen]

On 7/30/20 1:52 PM, Eric Biggers wrote:
> On Wed, Feb 19, 2020 at 06:49:07PM -0500, Jes Sorensen wrote:
>>> 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.
>>
> 
> Hi Jes, I'd like to revisit this, as I'm concerned about future use cases where
> software under other licenses (e.g. LGPL, MIT, or Apache 2.0) might want to use
> libfsverity -- especially if libfsverity grows more functionality.
> 
> Also, fsverity-utils links to OpenSSL, which some people (e.g. Debian) consider
> to be incompatible with GPLv2.
> 
> We think the MIT license (https://opensource.org/licenses/MIT) would offer the
> most flexibility.  Are you okay with changing the license of fsverity-utils to
> MIT?  If so, I'll send a patch and you can give an Acked-by on it.
> 
> Thanks!
> 
> - Eric

Hi Eric,

I went back through my patches to make sure I didn't reuse code from
other GPL projects. I don't see anything that looks like it was reused
except from fsverity-utils itself, so it should be fine.

I think it's fair to relax the license so other projects can link to it.
I would prefer we use the LGPL rather than the MIT license though?

CC'ing Chris Mason as well, since he has the auth to ack it on behalf of
the company.

Cheers,
Jes

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

[Chris Mason]

On 31 Jul 2020, at 13:40, Jes Sorensen wrote:

> On 7/30/20 1:52 PM, Eric Biggers wrote:
>> On Wed, Feb 19, 2020 at 06:49:07PM -0500, Jes Sorensen wrote:
>>>> 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.
>>>
>>
>> Hi Jes, I'd like to revisit this, as I'm concerned about future use 
>> cases where
>> software under other licenses (e.g. LGPL, MIT, or Apache 2.0) might 
>> want to use
>> libfsverity -- especially if libfsverity grows more functionality.
>>
>> Also, fsverity-utils links to OpenSSL, which some people (e.g. 
>> Debian) consider
>> to be incompatible with GPLv2.
>>
>> We think the MIT license would offer the
>> most flexibility.  Are you okay with changing the license of 
>> fsverity-utils to
>> MIT?  If so, I'll send a patch and you can give an Acked-by on it.
>>
>> Thanks!
>>
>> - Eric
>
> Hi Eric,
>
> I went back through my patches to make sure I didn't reuse code from
> other GPL projects. I don't see anything that looks like it was reused
> except from fsverity-utils itself, so it should be fine.
>
> I think it's fair to relax the license so other projects can link to 
> it.
> I would prefer we use the LGPL rather than the MIT license though?
>
> CC'ing Chris Mason as well, since he has the auth to ack it on behalf 
> of
> the company.

MIT, BSD, LGPL are Signed-off-by: Chris Mason <clm@fb.com>

We’re flexible, the goal is just to fit into the rest of fsverity 
overall.

-chris

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

[Eric Biggers]

On Fri, Jul 31, 2020 at 01:47:36PM -0400, Chris Mason wrote:
> On 31 Jul 2020, at 13:40, Jes Sorensen wrote:
> 
> > On 7/30/20 1:52 PM, Eric Biggers wrote:
> > > On Wed, Feb 19, 2020 at 06:49:07PM -0500, Jes Sorensen wrote:
> > > > > 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.
> > > > 
> > > 
> > > Hi Jes, I'd like to revisit this, as I'm concerned about future use
> > > cases where
> > > software under other licenses (e.g. LGPL, MIT, or Apache 2.0) might
> > > want to use
> > > libfsverity -- especially if libfsverity grows more functionality.
> > > 
> > > Also, fsverity-utils links to OpenSSL, which some people (e.g.
> > > Debian) consider
> > > to be incompatible with GPLv2.
> > > 
> > > We think the MIT license would offer the
> > > most flexibility.  Are you okay with changing the license of
> > > fsverity-utils to
> > > MIT?  If so, I'll send a patch and you can give an Acked-by on it.
> > > 
> > > Thanks!
> > > 
> > > - Eric
> > 
> > Hi Eric,
> > 
> > I went back through my patches to make sure I didn't reuse code from
> > other GPL projects. I don't see anything that looks like it was reused
> > except from fsverity-utils itself, so it should be fine.
> > 
> > I think it's fair to relax the license so other projects can link to it.
> > I would prefer we use the LGPL rather than the MIT license though?
> > 
> > CC'ing Chris Mason as well, since he has the auth to ack it on behalf of
> > the company.
> 
> MIT, BSD, LGPL are Signed-off-by: Chris Mason <clm@fb.com>
> 
> We’re flexible, the goal is just to fit into the rest of fsverity overall.
> 
> -chris

Thanks Chris and Jes.

At least on Google's side, a permissive license generally makes things easier
for people -- even though in practice we'll be upstreaming all changes anyway.
Since fsverity-utils is only a small project and is unlikely to be customized
much by people (as it's closely tied to the upstream kernel support), for now
I'd rather not create problems for users or cause duplication of effort.

If it were a larger project, or something people would be more likely to
customize, the case for LGPL would be stronger IMO.

There are also OpenSSL linking exceptions out there even for the LGPL (!), so
I'm not sure everyone agrees that one isn't needed...  I'd like to avoid wasting
time on any such issues and just write code :-)

Note that we can always choose to move to LGPL later, but LGPL => MIT won't be
possible (since in line with kernel community norms, for fsverity-utils we're
only requiring the DCO, not a CLA).  I think we shouldn't go down a one-way
street too early.

I've send out a patch to change the license.  Can you two explicitly give
Acked-by on the patch itself?  Thanks!

- Eric