[RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[Axel Burri]

This patch allows to build distinct binaries for specific btrfs
subcommands, e.g. "btrfs-subvolume-show" which would be identical to
"btrfs subvolume show".


Motivation:

While btrfs-progs offer the all-inclusive "btrfs" command, it gets
pretty cumbersome to restrict privileges to the subcommands [1].
Common approaches are to either setuid root for "/sbin/btrfs" (which
is not recommended at all), or to write sudo rules for each
subcommand.

Separating the subcommands into distinct binaries makes it easy to set
elevated privileges using capabilities(7) or setuid. A typical use
case where this is needed is when it comes to automated scripts,
e.g. btrbk [2] [3] creating snapshots and send/receive them via ssh.


Description:

Patch 1 adds a template as well as a generator shell script for the
splitted subcommands.

Patch 2 adds the generated subcommand source files.

Patch 3-5 adds a "install-splitcmd-setcap" make target, with different
approaches (either hardcoded in Makefile, or more generically by
including "Makefile.install_setcap" generated by "splitcmd-gen.sh").


Open Questions:

1. "make install-splitcmd-setcap" installs the binaries with hardcoded
group "btrfs". This needs to be configurable (how?). Another approach
would be to not set the group at all, and leave this to the user or
distro packaging script.

2. Instead of the "install-splitcmd-setcap" make target, we could
introduce a "configure --enable-splitted-subcommands" option, which
would simply add all splitcmd binaries to the "all" and "install"
targets without special treatment, and leave the setcap stuff to the
user or distro packaging script (at least in gentoo, this needs to be
specified using the "fcaps" eclass anyways [5]).


References:

  [1] https://www.spinics.net/lists/linux-btrfs/msg75736.html
  [2] https://github.com/digint/btrbk
  [3] https://github.com/digint/btrfs-progs-btrbk
  [4] https://github.com/digint/btrfs-progs/tree/splitcmd-setcap
  [5] https://dev.tty0.ch/portage/digint-overlay.git (sys-fs/btrfs-progs-btrbk)



Axel Burri (6):
  btrfs-progs: splitcmd-gen.sh: create btrfs-<subcommand> binaries for
    selected subcommands
  btrfs-progs: add btrfs-<subcommand> source files generated by
    splitcmd-gen.sh
  btrfs-progs: Makefile: add "install-splitcmd-setcap" target, installs
    splitcmd binaries with appropriate capabilities
  btrfs-progs: Makefile: include Makefile.install_setcap generated by
    splitcmd-gen.sh
  btrfs-progs: Makefile: move progs_splitcmd variable to
    Makefile.install_setcap
  btrfs-progs: add splitcmd binaries to gitignore

 .gitignore                 |  9 +++++
 Makefile                   | 20 +++++++++++
 Makefile.inc.in            |  1 +
 Makefile.install_setcap    | 12 +++++++
 btrfs-filesystem-usage.c   | 23 +++++++++++++
 btrfs-qgroup-destroy.c     | 23 +++++++++++++
 btrfs-receive.c            | 23 +++++++++++++
 btrfs-send.c               | 23 +++++++++++++
 btrfs-subvolume-delete.c   | 23 +++++++++++++
 btrfs-subvolume-list.c     | 23 +++++++++++++
 btrfs-subvolume-show.c     | 23 +++++++++++++
 btrfs-subvolume-snapshot.c | 23 +++++++++++++
 configure.ac               |  1 +
 splitcmd-gen.sh            | 86 ++++++++++++++++++++++++++++++++++++++++++++++
 splitcmd.c.in              | 17 +++++++++
 15 files changed, 330 insertions(+)
 create mode 100644 Makefile.install_setcap
 create mode 100644 btrfs-filesystem-usage.c
 create mode 100644 btrfs-qgroup-destroy.c
 create mode 100644 btrfs-receive.c
 create mode 100644 btrfs-send.c
 create mode 100644 btrfs-subvolume-delete.c
 create mode 100644 btrfs-subvolume-list.c
 create mode 100644 btrfs-subvolume-show.c
 create mode 100644 btrfs-subvolume-snapshot.c
 create mode 100755 splitcmd-gen.sh
 create mode 100644 splitcmd.c.in

-- 
2.16.4

[RFC PATCH 1/6] btrfs-progs: splitcmd-gen.sh: create btrfs-<subcommand> binaries for selected subcommands

[Axel Burri]

Create separate binaries for each subcommand ("btrfs foo bar").
Least invasive approach, generate c-files for each command:

    # ./splitcmd-gen.sh
    # make V=1 btrfs-subvolume-show
    # make V=1 btrfs-send
    # [...]

Alternative approach: instead of including the c-file, link with obj
in Makefile, e.g.:

    btrfs_subvolume_show_objects = cmds-subvolume.o
    btrfs_send_objects = cmds-send.o
    [...]

This implies adaptions in cmds-subvolume.c (and others):

    -static int cmd_filesystem_show(int argc, char **argv)
    +int cmd_filesystem_show(int argc, char **argv)

If they are defined non-static, we could probably simplify further and
add `-Wl,-eentry` flags (changing entry point from "main" to "entry").

With this, and if handle_command_group() was declared in some library
instead of btrfs.c, we would get rid of generated files completely.

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 splitcmd-gen.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 splitcmd.c.in   | 17 ++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100755 splitcmd-gen.sh
 create mode 100644 splitcmd.c.in

diff --git a/splitcmd-gen.sh b/splitcmd-gen.sh
new file mode 100755
index 00000000..4d2e0509
--- /dev/null
+++ b/splitcmd-gen.sh
@@ -0,0 +1,70 @@
+#!/bin/bash
+
+#
+# Generate c-files for btrfs subcommands defined below
+#
+
+# Notes on linux capabilities:
+#
+# btrfs-subvolume-show, btrfs-subvolume-list, btrfs-send:
+#  - CAP_FOWNER is only needed for O_NOATIME flag in open() system calls
+#  - why CAP_SYS_ADMIN? shouldn't CAP_DAC_READ_SEARCH be enough?
+#
+# btrfs-receive:
+#  - dependent on send-stream (see cmds-receive.c: "send_ops"):
+#    CAP_CHOWN, CAP_MKNOD, CAP_SETFCAP (for "lsetxattr")
+#
+# btrfs-filesystem-usage:
+#  - CAP_SYS_ADMIN is for BTRFS_IOC_TREE_SEARCH and BTRFS_IOC_FS_INFO
+#    in order to provide full level of detail, see btrfs-filesystem(8)
+
+
+makefile_out="Makefile.install_setcap"
+
+splitcmd_list=""
+setcap_lines=""
+
+function gen_splitcmd {
+    local name="$1"
+    local dest="${1}.c"
+    local cfile="$2"
+    local entry="$3"
+    local caps="$4"
+    echo "generating: ${dest} (cfile=${cfile}, entry=${entry})"
+    echo -e "/*\n * ${name}\n *\n * GENERATED BY splitcmd-gen.sh\n */\n" > $dest
+    sed -e "s|@BTRFS_SPLITCMD_CFILE_INCLUDE@|${cfile}|g" \
+        -e "s|@BTRFS_SPLITCMD_ENTRY@|${entry}|g" \
+        splitcmd.c.in >> $dest
+}
+
+gen_splitcmd "btrfs-subvolume-show" \
+             "cmds-subvolume.c" "cmd_subvol_show" \
+             "cap_sys_admin,cap_fowner,cap_dac_read_search"
+
+gen_splitcmd "btrfs-subvolume-list" \
+             "cmds-subvolume.c" "cmd_subvol_list" \
+             "cap_sys_admin,cap_fowner,cap_dac_read_search"
+
+gen_splitcmd "btrfs-subvolume-snapshot" \
+             "cmds-subvolume.c" "cmd_subvol_snapshot" \
+             "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
+
+gen_splitcmd "btrfs-subvolume-delete" \
+             "cmds-subvolume.c" "cmd_subvol_delete" \
+             "cap_sys_admin,cap_dac_override"
+
+gen_splitcmd "btrfs-send" \
+             "cmds-send.c" "cmd_send" \
+             "cap_sys_admin,cap_fowner,cap_dac_read_search"
+
+gen_splitcmd "btrfs-receive" \
+             "cmds-receive.c" "cmd_receive" \
+             "cap_sys_admin,cap_fowner,cap_chown,cap_mknod,cap_setfcap,cap_dac_override,cap_dac_read_search"
+
+gen_splitcmd "btrfs-filesystem-usage" \
+             "cmds-fi-usage.c" "cmd_filesystem_usage" \
+             "cap_sys_admin"
+
+gen_splitcmd "btrfs-qgroup-destroy" \
+             "cmds-qgroup.c" "cmd_qgroup_destroy" \
+             "cap_sys_admin,cap_dac_override"
diff --git a/splitcmd.c.in b/splitcmd.c.in
new file mode 100644
index 00000000..aa07af9a
--- /dev/null
+++ b/splitcmd.c.in
@@ -0,0 +1,17 @@
+#include "@BTRFS_SPLITCMD_CFILE_INCLUDE@"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return @BTRFS_SPLITCMD_ENTRY@(argc, argv);
+}
-- 
2.16.4

[RFC PATCH 2/6] btrfs-progs: add btrfs-<subcommand> source files generated by splitcmd-gen.sh

[Axel Burri]

Another approach would be to generate the splitted commands in the
Makefile on-demand, which is probably not desired.

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 btrfs-filesystem-usage.c   | 23 +++++++++++++++++++++++
 btrfs-qgroup-destroy.c     | 23 +++++++++++++++++++++++
 btrfs-receive.c            | 23 +++++++++++++++++++++++
 btrfs-send.c               | 23 +++++++++++++++++++++++
 btrfs-subvolume-delete.c   | 23 +++++++++++++++++++++++
 btrfs-subvolume-list.c     | 23 +++++++++++++++++++++++
 btrfs-subvolume-show.c     | 23 +++++++++++++++++++++++
 btrfs-subvolume-snapshot.c | 23 +++++++++++++++++++++++
 8 files changed, 184 insertions(+)
 create mode 100644 btrfs-filesystem-usage.c
 create mode 100644 btrfs-qgroup-destroy.c
 create mode 100644 btrfs-receive.c
 create mode 100644 btrfs-send.c
 create mode 100644 btrfs-subvolume-delete.c
 create mode 100644 btrfs-subvolume-list.c
 create mode 100644 btrfs-subvolume-show.c
 create mode 100644 btrfs-subvolume-snapshot.c

diff --git a/btrfs-filesystem-usage.c b/btrfs-filesystem-usage.c
new file mode 100644
index 00000000..e102ebc8
--- /dev/null
+++ b/btrfs-filesystem-usage.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-filesystem-usage
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-fi-usage.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_filesystem_usage(argc, argv);
+}
diff --git a/btrfs-qgroup-destroy.c b/btrfs-qgroup-destroy.c
new file mode 100644
index 00000000..4fb32210
--- /dev/null
+++ b/btrfs-qgroup-destroy.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-qgroup-destroy
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-qgroup.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_qgroup_destroy(argc, argv);
+}
diff --git a/btrfs-receive.c b/btrfs-receive.c
new file mode 100644
index 00000000..9fe94080
--- /dev/null
+++ b/btrfs-receive.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-receive
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-receive.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_receive(argc, argv);
+}
diff --git a/btrfs-send.c b/btrfs-send.c
new file mode 100644
index 00000000..0eff2931
--- /dev/null
+++ b/btrfs-send.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-send
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-send.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_send(argc, argv);
+}
diff --git a/btrfs-subvolume-delete.c b/btrfs-subvolume-delete.c
new file mode 100644
index 00000000..d6018d8b
--- /dev/null
+++ b/btrfs-subvolume-delete.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-subvolume-delete
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-subvolume.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_subvol_delete(argc, argv);
+}
diff --git a/btrfs-subvolume-list.c b/btrfs-subvolume-list.c
new file mode 100644
index 00000000..8529aef5
--- /dev/null
+++ b/btrfs-subvolume-list.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-subvolume-list
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-subvolume.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_subvol_list(argc, argv);
+}
diff --git a/btrfs-subvolume-show.c b/btrfs-subvolume-show.c
new file mode 100644
index 00000000..856f97cf
--- /dev/null
+++ b/btrfs-subvolume-show.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-subvolume-show
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-subvolume.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_subvol_show(argc, argv);
+}
diff --git a/btrfs-subvolume-snapshot.c b/btrfs-subvolume-snapshot.c
new file mode 100644
index 00000000..efdc0142
--- /dev/null
+++ b/btrfs-subvolume-snapshot.c
@@ -0,0 +1,23 @@
+/*
+ * btrfs-subvolume-snapshot
+ *
+ * GENERATED BY splitcmd-gen.sh
+ */
+
+#include "cmds-subvolume.c"
+
+/*
+ * Dummy object: used from second-level command groups (e.g. in
+ * "cmds-subvolume.c"), is never called in splitcmd executables.
+ */
+int handle_command_group(const struct cmd_group *grp, int argc,
+			 char **argv)
+{
+	exit(1);
+}
+
+
+int main(int argc, char **argv)
+{
+	return cmd_subvol_snapshot(argc, argv);
+}
-- 
2.16.4

[RFC PATCH 3/6] btrfs-progs: Makefile: add "install-splitcmd-setcap" target, installs splitcmd binaries with appropriate capabilities

[Axel Burri]

Install all $progs_install_splitcmd, and set appropriate linux file
capabilities(7) using setcap(8).

NOTE: while installing, group is hardcoded to "btrfs"! This needs
further discussion.

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 Makefile        | 36 ++++++++++++++++++++++++++++++++++++
 Makefile.inc.in |  1 +
 configure.ac    |  1 +
 3 files changed, 38 insertions(+)

diff --git a/Makefile b/Makefile
index fcfc815a..5a1e2747 100644
--- a/Makefile
+++ b/Makefile
@@ -4,6 +4,7 @@
 #   static      build static bnaries, requires static version of the libraries
 #   test        run the full testsuite
 #   install     install to default location (/usr/local)
+#   install-splitcmd-setcap  install splitcmd binaries, and set linux capabilities
 #   clean       clean built binaries (not the documentation)
 #   clean-all   clean as above, clean docs and generated files
 #
@@ -231,6 +232,30 @@ progs_install =
 progs_build =
 endif
 
+# split-command executables, generated by splitcmd-gen.sh
+progs_splitcmd = btrfs-send \
+	btrfs-receive \
+	btrfs-subvolume-list \
+	btrfs-subvolume-show \
+	btrfs-subvolume-snapshot \
+	btrfs-subvolume-delete \
+	btrfs-filesystem-usage \
+	btrfs-qgroup-destroy
+
+progs_install_splitcmd = $(progs_splitcmd)
+
+INSTALL_SETCAP_FLAGS = -m710 -gbtrfs
+
+# linux capabilities(7) needed; used by "install-splitcmd-setcap-%" below
+btrfs_subvolume_show_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_subvolume_list_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_subvolume_snapshot_fcaps = "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
+btrfs_subvolume_delete_fcaps = "cap_sys_admin,cap_dac_override"
+btrfs_send_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_receive_fcaps = "cap_sys_admin,cap_fowner,cap_chown,cap_mknod,cap_setfcap,cap_dac_override,cap_dac_read_search"
+btrfs_filesystem_usage_fcaps = "cap_sys_admin"
+btrfs_qgroup_destroy_fcaps = "cap_sys_admin,cap_dac_override"
+
 # external libs required by various binaries; for btrfs-foo,
 # specify btrfs_foo_libs = <list of libs>; see $($(subst...)) rules below
 btrfs_convert_cflags = -DBTRFSCONVERT_EXT2=$(BTRFSCONVERT_EXT2)
@@ -318,6 +343,7 @@ endif
 		$($(subst -,_,btrfs-$(@:%/$(notdir $@)=%)-cflags))
 
 all: $(progs_build) $(libs_build) $(BUILDDIRS)
+splitcmd: $(progs_splitcmd)
 ifeq ($(PYTHON_BINDINGS),1)
 all: libbtrfsutil_python
 endif
@@ -618,6 +644,7 @@ clean: $(CLEANDIRS)
 	      $(check_defs) \
 	      $(libs) $(lib_links) \
 	      $(progs_static) \
+	      $(progs_splitcmd) \
 	      libbtrfsutil/*.o libbtrfsutil/*.o.d
 ifeq ($(PYTHON_BINDINGS),1)
 	$(Q)cd libbtrfsutil/python; \
@@ -678,6 +705,15 @@ install-static: $(progs_static) $(INSTALLDIRS)
 	# btrfsck is a link to btrfs in the src tree, make it so for installed file as well
 	$(LN_S) -f btrfs.static $(DESTDIR)$(bindir)/btrfsck.static
 
+# install split-command binary, and set linux capabilities(7) defined
+# in btrfs_*_fcaps above, using setcap(8)
+install-splitcmd-setcap-%: %
+	@echo $(INSTALL) -m755 -d $(DESTDIR)$(bindir)
+	@echo $(INSTALL) $(INSTALL_SETCAP_FLAGS) $< $(DESTDIR)$(bindir)
+	@echo $(SETCAP) $($(subst -,_,$<)_fcaps)+ep $(DESTDIR)$(bindir)/$<
+
+install-splitcmd-setcap: $(progs_install_splitcmd) $(patsubst %,install-splitcmd-setcap-%,$(progs_install_splitcmd))
+
 $(INSTALLDIRS):
 	@echo "Making install in $(patsubst install-%,%,$@)"
 	$(Q)$(MAKE) $(MAKEOPTS) -C $(patsubst install-%,%,$@) install
diff --git a/Makefile.inc.in b/Makefile.inc.in
index a86c528e..567e4e6f 100644
--- a/Makefile.inc.in
+++ b/Makefile.inc.in
@@ -10,6 +10,7 @@ AR = @AR@
 RM = @RM@
 RMDIR = @RMDIR@
 INSTALL = @INSTALL@
+SETCAP = @SETCAP@
 DISABLE_DOCUMENTATION = @DISABLE_DOCUMENTATION@
 DISABLE_BTRFSCONVERT = @DISABLE_BTRFSCONVERT@
 BUILD_PROGRAMS = @BUILD_PROGRAMS@
diff --git a/configure.ac b/configure.ac
index df02f206..fefbfd9c 100644
--- a/configure.ac
+++ b/configure.ac
@@ -39,6 +39,7 @@ AC_PROG_LN_S
 AC_CHECK_TOOL([AR], [ar])
 AC_PATH_PROG([RM], [rm], [rm])
 AC_PATH_PROG([RMDIR], [rmdir], [rmdir])
+AC_PATH_PROG([SETCAP], [setcap], [setcap])
 
 
 AC_CHECK_FUNCS([openat], [],
-- 
2.16.4

[RFC PATCH 4/6] btrfs-progs: Makefile: include Makefile.install_setcap generated by splitcmd-gen.sh

[Axel Burri]

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 Makefile                | 11 ++---------
 Makefile.install_setcap | 10 ++++++++++
 splitcmd-gen.sh         |  5 +++++
 3 files changed, 17 insertions(+), 9 deletions(-)
 create mode 100644 Makefile.install_setcap

diff --git a/Makefile b/Makefile
index 5a1e2747..acf5a677 100644
--- a/Makefile
+++ b/Makefile
@@ -246,15 +246,8 @@ progs_install_splitcmd = $(progs_splitcmd)
 
 INSTALL_SETCAP_FLAGS = -m710 -gbtrfs
 
-# linux capabilities(7) needed; used by "install-splitcmd-setcap-%" below
-btrfs_subvolume_show_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
-btrfs_subvolume_list_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
-btrfs_subvolume_snapshot_fcaps = "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
-btrfs_subvolume_delete_fcaps = "cap_sys_admin,cap_dac_override"
-btrfs_send_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
-btrfs_receive_fcaps = "cap_sys_admin,cap_fowner,cap_chown,cap_mknod,cap_setfcap,cap_dac_override,cap_dac_read_search"
-btrfs_filesystem_usage_fcaps = "cap_sys_admin"
-btrfs_qgroup_destroy_fcaps = "cap_sys_admin,cap_dac_override"
+# defines btrfs_*_caps; used by "install-splitcmd-setcap-%" below
+include Makefile.install_setcap
 
 # external libs required by various binaries; for btrfs-foo,
 # specify btrfs_foo_libs = <list of libs>; see $($(subst...)) rules below
diff --git a/Makefile.install_setcap b/Makefile.install_setcap
new file mode 100644
index 00000000..7705db74
--- /dev/null
+++ b/Makefile.install_setcap
@@ -0,0 +1,10 @@
+# capabilities(7) for splitcmd executables
+
+btrfs_subvolume_show_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_subvolume_list_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_subvolume_snapshot_fcaps = "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
+btrfs_subvolume_delete_fcaps = "cap_sys_admin,cap_dac_override"
+btrfs_send_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
+btrfs_receive_fcaps = "cap_sys_admin,cap_fowner,cap_chown,cap_mknod,cap_setfcap,cap_dac_override,cap_dac_read_search"
+btrfs_filesystem_usage_fcaps = "cap_sys_admin"
+btrfs_qgroup_destroy_fcaps = "cap_sys_admin,cap_dac_override"
diff --git a/splitcmd-gen.sh b/splitcmd-gen.sh
index 4d2e0509..d34c5cbd 100755
--- a/splitcmd-gen.sh
+++ b/splitcmd-gen.sh
@@ -35,8 +35,13 @@ function gen_splitcmd {
     sed -e "s|@BTRFS_SPLITCMD_CFILE_INCLUDE@|${cfile}|g" \
         -e "s|@BTRFS_SPLITCMD_ENTRY@|${entry}|g" \
         splitcmd.c.in >> $dest
+
+    echo "${name//-/_}_fcaps = \"${caps}\"" >> $makefile_out
 }
 
+echo "generating: ${makefile_out}"
+echo -e "# capabilities(7) for splitcmd executables\n" > $makefile_out
+
 gen_splitcmd "btrfs-subvolume-show" \
              "cmds-subvolume.c" "cmd_subvol_show" \
              "cap_sys_admin,cap_fowner,cap_dac_read_search"
-- 
2.16.4

[RFC PATCH 5/6] btrfs-progs: Makefile: move progs_splitcmd variable to Makefile.install_setcap

[Axel Burri]

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 Makefile                | 17 ++++-------------
 Makefile.install_setcap |  2 ++
 splitcmd-gen.sh         | 17 ++++++++++++++---
 3 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/Makefile b/Makefile
index acf5a677..3f16434c 100644
--- a/Makefile
+++ b/Makefile
@@ -232,22 +232,13 @@ progs_install =
 progs_build =
 endif
 
-# split-command executables, generated by splitcmd-gen.sh
-progs_splitcmd = btrfs-send \
-	btrfs-receive \
-	btrfs-subvolume-list \
-	btrfs-subvolume-show \
-	btrfs-subvolume-snapshot \
-	btrfs-subvolume-delete \
-	btrfs-filesystem-usage \
-	btrfs-qgroup-destroy
-
-progs_install_splitcmd = $(progs_splitcmd)
+# defines "progs_splitcmd" as well as "btrfs_*_fcaps" variables
+# used by "install-splitcmd-setcap-%" below
+include Makefile.install_setcap
 
 INSTALL_SETCAP_FLAGS = -m710 -gbtrfs
 
-# defines btrfs_*_caps; used by "install-splitcmd-setcap-%" below
-include Makefile.install_setcap
+progs_install_splitcmd = $(progs_splitcmd)
 
 # external libs required by various binaries; for btrfs-foo,
 # specify btrfs_foo_libs = <list of libs>; see $($(subst...)) rules below
diff --git a/Makefile.install_setcap b/Makefile.install_setcap
index 7705db74..e1efb686 100644
--- a/Makefile.install_setcap
+++ b/Makefile.install_setcap
@@ -1,5 +1,7 @@
 # capabilities(7) for splitcmd executables
 
+progs_splitcmd = btrfs-subvolume-show btrfs-subvolume-list btrfs-subvolume-snapshot btrfs-subvolume-delete btrfs-send btrfs-receive btrfs-filesystem-usage btrfs-qgroup-destroy
+
 btrfs_subvolume_show_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
 btrfs_subvolume_list_fcaps = "cap_sys_admin,cap_fowner,cap_dac_read_search"
 btrfs_subvolume_snapshot_fcaps = "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
diff --git a/splitcmd-gen.sh b/splitcmd-gen.sh
index d34c5cbd..7492594f 100755
--- a/splitcmd-gen.sh
+++ b/splitcmd-gen.sh
@@ -36,12 +36,14 @@ function gen_splitcmd {
         -e "s|@BTRFS_SPLITCMD_ENTRY@|${entry}|g" \
         splitcmd.c.in >> $dest
 
-    echo "${name//-/_}_fcaps = \"${caps}\"" >> $makefile_out
+    splitcmd_list="${splitcmd_list} ${name}";
+    setcap_lines="${setcap_lines}\n${name//-/_}_fcaps = \"${caps}\""
 }
 
-echo "generating: ${makefile_out}"
-echo -e "# capabilities(7) for splitcmd executables\n" > $makefile_out
 
+#
+# generate c-files
+#
 gen_splitcmd "btrfs-subvolume-show" \
              "cmds-subvolume.c" "cmd_subvol_show" \
              "cap_sys_admin,cap_fowner,cap_dac_read_search"
@@ -73,3 +75,12 @@ gen_splitcmd "btrfs-filesystem-usage" \
 gen_splitcmd "btrfs-qgroup-destroy" \
              "cmds-qgroup.c" "cmd_qgroup_destroy" \
              "cap_sys_admin,cap_dac_override"
+
+
+#
+# generate Makefile includes
+#
+echo "generating: ${makefile_out}"
+echo -e "# capabilities(7) for splitcmd executables" > $makefile_out
+echo -e "\nprogs_splitcmd =$splitcmd_list" >> $makefile_out
+echo -e "${setcap_lines}" >> $makefile_out
-- 
2.16.4

[RFC PATCH 6/6] btrfs-progs: add splitcmd binaries to gitignore

[Axel Burri]

Signed-off-by: Axel Burri <axel@tty0.ch>
---
 .gitignore | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/.gitignore b/.gitignore
index 144ebb3b..299019e0 100644
--- a/.gitignore
+++ b/.gitignore
@@ -42,6 +42,15 @@ library-test-static
 /fssum
 testsuite-id
 
+btrfs-filesystem-usage
+btrfs-qgroup-destroy
+btrfs-receive
+btrfs-send
+btrfs-subvolume-delete
+btrfs-subvolume-list
+btrfs-subvolume-show
+btrfs-subvolume-snapshot
+
 /tests/*-tests-results.txt
 /tests/test-console.txt
 /tests/test.img
-- 
2.16.4

Re: [RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[Austin S. Hemmelgarn]

On 2018-08-29 13:24, Axel Burri wrote:
> This patch allows to build distinct binaries for specific btrfs
> subcommands, e.g. "btrfs-subvolume-show" which would be identical to
> "btrfs subvolume show".
> 
> 
> Motivation:
> 
> While btrfs-progs offer the all-inclusive "btrfs" command, it gets
> pretty cumbersome to restrict privileges to the subcommands [1].
> Common approaches are to either setuid root for "/sbin/btrfs" (which
> is not recommended at all), or to write sudo rules for each
> subcommand.
> 
> Separating the subcommands into distinct binaries makes it easy to set
> elevated privileges using capabilities(7) or setuid. A typical use
> case where this is needed is when it comes to automated scripts,
> e.g. btrbk [2] [3] creating snapshots and send/receive them via ssh.
Let me start by saying I think this is a great idea to have as an 
option, and that the motivation is a particularly good one.

I've posted my opinions on your two open questions below, but there's 
two other comments I'd like to make:

* Is there some particular reason that this only includes the commands 
it does, and _hard codes_ which ones it works with?  if we just do 
everything instead of only the stuff we think needs certain 
capabilities, then we can auto-generate the list of commands to be 
processed based on function names in the C files, and it will 
automatically pick up any newly added commands.  At the very least, it 
could still parse through the C files and look for tags in the comments 
for the functions to indicate which ones need to be processed this way. 
Either case will make it significantly easier to add new commands, and 
would also better justify the overhead of shipping all the files 
pre-generated (because there would be much more involved in 
pre-generating them).

* While not essential, it would be really neat to have the `btrfs` 
command detect if an associated binary exists for whatever command was 
just invoked, and automatically exec that (possibly with some 
verification) instead of calling the command directly so that desired 
permissions are enforced.  This would mitigate the need for users to 
remember different command names depending on execution context.
> 
> 
> Description:
> 
> Patch 1 adds a template as well as a generator shell script for the
> splitted subcommands.
> 
> Patch 2 adds the generated subcommand source files.
> 
> Patch 3-5 adds a "install-splitcmd-setcap" make target, with different
> approaches (either hardcoded in Makefile, or more generically by
> including "Makefile.install_setcap" generated by "splitcmd-gen.sh").
> 
> 
> Open Questions:
> 
> 1. "make install-splitcmd-setcap" installs the binaries with hardcoded
> group "btrfs". This needs to be configurable (how?). Another approach
> would be to not set the group at all, and leave this to the user or
> distro packaging script.
Leave it to the user or distro.  It's likely to end up standardized on 
the name 'btrfs', but it should be agnostic of that.
> 
> 2. Instead of the "install-splitcmd-setcap" make target, we could
> introduce a "configure --enable-splitted-subcommands" option, which
> would simply add all splitcmd binaries to the "all" and "install"
> targets without special treatment, and leave the setcap stuff to the
> user or distro packaging script (at least in gentoo, this needs to be
> specified using the "fcaps" eclass anyways [5]).
A bit of a nitpick, but 'split' is the proper past tense of the word 
'split', it's one of those exceptions that English has all over the 
place.  Even aside from that though, I think `separate` sounds more 
natural for the configure option, or better yet, just make it 
`--enable-fscaps` like most other packages do.

That aside, I think having a configure option is the best way to do 
this, it makes it very easy for distro build systems to handle it 
because this is what they're used to doing anyway.  It also makes it a 
bit easier on the user, because it just becomes `make` to build 
whichever version you want installed.

Re: [RFC PATCH 1/6] btrfs-progs: splitcmd-gen.sh: create btrfs-<subcommand> binaries for selected subcommands

[Misono Tomohiro]

On 2018/08/30 2:24, Axel Burri wrote:
> Create separate binaries for each subcommand ("btrfs foo bar").
> Least invasive approach, generate c-files for each command:
> 
>     # ./splitcmd-gen.sh
>     # make V=1 btrfs-subvolume-show
>     # make V=1 btrfs-send
>     # [...]
> 
> Alternative approach: instead of including the c-file, link with obj
> in Makefile, e.g.:
> 
>     btrfs_subvolume_show_objects = cmds-subvolume.o
>     btrfs_send_objects = cmds-send.o
>     [...]
> 
> This implies adaptions in cmds-subvolume.c (and others):
> 
>     -static int cmd_filesystem_show(int argc, char **argv)
>     +int cmd_filesystem_show(int argc, char **argv)
> 
> If they are defined non-static, we could probably simplify further and
> add `-Wl,-eentry` flags (changing entry point from "main" to "entry").
> 
> With this, and if handle_command_group() was declared in some library
> instead of btrfs.c, we would get rid of generated files completely.
> 
> Signed-off-by: Axel Burri <axel@tty0.ch>
> ---
>  splitcmd-gen.sh | 70 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  splitcmd.c.in   | 17 ++++++++++++++
>  2 files changed, 87 insertions(+)
>  create mode 100755 splitcmd-gen.sh
>  create mode 100644 splitcmd.c.in
> 
> diff --git a/splitcmd-gen.sh b/splitcmd-gen.sh
> new file mode 100755
> index 00000000..4d2e0509
> --- /dev/null
> +++ b/splitcmd-gen.sh
> @@ -0,0 +1,70 @@
> +#!/bin/bash
> +
> +#
> +# Generate c-files for btrfs subcommands defined below
> +#
> +
> +# Notes on linux capabilities:
> +#
> +# btrfs-subvolume-show, btrfs-subvolume-list, btrfs-send:
> +#  - CAP_FOWNER is only needed for O_NOATIME flag in open() system calls
> +#  - why CAP_SYS_ADMIN? shouldn't CAP_DAC_READ_SEARCH be enough?

Hello,

Not directly related this series and just FYI,
I'm working to allow sub show/list to non-privileged user as long
as he can access to the subvolume:
  https://www.spinics.net/lists/linux-btrfs/msg79285.html

Hopefully this will be merged to master in near future
(any comments from user/dev is welcome).

Thanks,
Misono

> +#
> +# btrfs-receive:
> +#  - dependent on send-stream (see cmds-receive.c: "send_ops"):
> +#    CAP_CHOWN, CAP_MKNOD, CAP_SETFCAP (for "lsetxattr")
> +#
> +# btrfs-filesystem-usage:
> +#  - CAP_SYS_ADMIN is for BTRFS_IOC_TREE_SEARCH and BTRFS_IOC_FS_INFO
> +#    in order to provide full level of detail, see btrfs-filesystem(8)
> +
> +
> +makefile_out="Makefile.install_setcap"
> +
> +splitcmd_list=""
> +setcap_lines=""
> +
> +function gen_splitcmd {
> +    local name="$1"
> +    local dest="${1}.c"
> +    local cfile="$2"
> +    local entry="$3"
> +    local caps="$4"
> +    echo "generating: ${dest} (cfile=${cfile}, entry=${entry})"
> +    echo -e "/*\n * ${name}\n *\n * GENERATED BY splitcmd-gen.sh\n */\n" > $dest
> +    sed -e "s|@BTRFS_SPLITCMD_CFILE_INCLUDE@|${cfile}|g" \
> +        -e "s|@BTRFS_SPLITCMD_ENTRY@|${entry}|g" \
> +        splitcmd.c.in >> $dest
> +}
> +
> +gen_splitcmd "btrfs-subvolume-show" \
> +             "cmds-subvolume.c" "cmd_subvol_show" \
> +             "cap_sys_admin,cap_fowner,cap_dac_read_search"
> +
> +gen_splitcmd "btrfs-subvolume-list" \
> +             "cmds-subvolume.c" "cmd_subvol_list" \
> +             "cap_sys_admin,cap_fowner,cap_dac_read_search"
> +
> +gen_splitcmd "btrfs-subvolume-snapshot" \
> +             "cmds-subvolume.c" "cmd_subvol_snapshot" \
> +             "cap_sys_admin,cap_fowner,cap_dac_override,cap_dac_read_search"
> +
> +gen_splitcmd "btrfs-subvolume-delete" \
> +             "cmds-subvolume.c" "cmd_subvol_delete" \
> +             "cap_sys_admin,cap_dac_override"
> +
> +gen_splitcmd "btrfs-send" \
> +             "cmds-send.c" "cmd_send" \
> +             "cap_sys_admin,cap_fowner,cap_dac_read_search"
> +
> +gen_splitcmd "btrfs-receive" \
> +             "cmds-receive.c" "cmd_receive" \
> +             "cap_sys_admin,cap_fowner,cap_chown,cap_mknod,cap_setfcap,cap_dac_override,cap_dac_read_search"
> +
> +gen_splitcmd "btrfs-filesystem-usage" \
> +             "cmds-fi-usage.c" "cmd_filesystem_usage" \
> +             "cap_sys_admin"
> +
> +gen_splitcmd "btrfs-qgroup-destroy" \
> +             "cmds-qgroup.c" "cmd_qgroup_destroy" \
> +             "cap_sys_admin,cap_dac_override"
> diff --git a/splitcmd.c.in b/splitcmd.c.in
> new file mode 100644
> index 00000000..aa07af9a
> --- /dev/null
> +++ b/splitcmd.c.in
> @@ -0,0 +1,17 @@
> +#include "@BTRFS_SPLITCMD_CFILE_INCLUDE@"
> +
> +/*
> + * Dummy object: used from second-level command groups (e.g. in
> + * "cmds-subvolume.c"), is never called in splitcmd executables.
> + */
> +int handle_command_group(const struct cmd_group *grp, int argc,
> +			 char **argv)
> +{
> +	exit(1);
> +}
> +
> +
> +int main(int argc, char **argv)
> +{
> +	return @BTRFS_SPLITCMD_ENTRY@(argc, argv);
> +}
> 

Re: [RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[Axel Burri]

On 29/08/2018 21.02, Austin S. Hemmelgarn wrote:
> On 2018-08-29 13:24, Axel Burri wrote:
>> This patch allows to build distinct binaries for specific btrfs
>> subcommands, e.g. "btrfs-subvolume-show" which would be identical to
>> "btrfs subvolume show".
>>
>>
>> Motivation:
>>
>> While btrfs-progs offer the all-inclusive "btrfs" command, it gets
>> pretty cumbersome to restrict privileges to the subcommands [1].
>> Common approaches are to either setuid root for "/sbin/btrfs" (which
>> is not recommended at all), or to write sudo rules for each
>> subcommand.
>>
>> Separating the subcommands into distinct binaries makes it easy to set
>> elevated privileges using capabilities(7) or setuid. A typical use
>> case where this is needed is when it comes to automated scripts,
>> e.g. btrbk [2] [3] creating snapshots and send/receive them via ssh.
> Let me start by saying I think this is a great idea to have as an
> option, and that the motivation is a particularly good one.
> 
> I've posted my opinions on your two open questions below, but there's
> two other comments I'd like to make:
> 
> * Is there some particular reason that this only includes the commands
> it does, and _hard codes_ which ones it works with?  if we just do
> everything instead of only the stuff we think needs certain
> capabilities, then we can auto-generate the list of commands to be
> processed based on function names in the C files, and it will
> automatically pick up any newly added commands.  At the very least, it
> could still parse through the C files and look for tags in the comments
> for the functions to indicate which ones need to be processed this way.
> Either case will make it significantly easier to add new commands, and
> would also better justify the overhead of shipping all the files
> pre-generated (because there would be much more involved in
> pre-generating them).

It includes the commands that are required by btrbk. It was quite
painful to figure out the required capabilities (reading kernel code and
some trial and error involved), and I did not get around to include
other commands yet.

I like your idea of adding some tags in the C files, I'll try to
implement this, and we'll see what it gets to.

> * While not essential, it would be really neat to have the `btrfs`
> command detect if an associated binary exists for whatever command was
> just invoked, and automatically exec that (possibly with some
> verification) instead of calling the command directly so that desired
> permissions are enforced.  This would mitigate the need for users to
> remember different command names depending on execution context.

Hmm this sounds a bit too magic for me, and would probably be more
confusing than useful. It would mean than running "btrfs" as user would
work when splitted commands are available, and would not work if not.

>>
>>
>> Description:
>>
>> Patch 1 adds a template as well as a generator shell script for the
>> splitted subcommands.
>>
>> Patch 2 adds the generated subcommand source files.
>>
>> Patch 3-5 adds a "install-splitcmd-setcap" make target, with different
>> approaches (either hardcoded in Makefile, or more generically by
>> including "Makefile.install_setcap" generated by "splitcmd-gen.sh").
>>
>>
>> Open Questions:
>>
>> 1. "make install-splitcmd-setcap" installs the binaries with hardcoded
>> group "btrfs". This needs to be configurable (how?). Another approach
>> would be to not set the group at all, and leave this to the user or
>> distro packaging script.
> Leave it to the user or distro.  It's likely to end up standardized on
> the name 'btrfs', but it should be agnostic of that.
>>
>> 2. Instead of the "install-splitcmd-setcap" make target, we could
>> introduce a "configure --enable-splitted-subcommands" option, which
>> would simply add all splitcmd binaries to the "all" and "install"
>> targets without special treatment, and leave the setcap stuff to the
>> user or distro packaging script (at least in gentoo, this needs to be
>> specified using the "fcaps" eclass anyways [5]).
> A bit of a nitpick, but 'split' is the proper past tense of the word
> 'split', it's one of those exceptions that English has all over the
> place.  Even aside from that though, I think `separate` sounds more
> natural for the configure option, or better yet, just make it
> `--enable-fscaps` like most other packages do.
> 
> That aside, I think having a configure option is the best way to do
> this, it makes it very easy for distro build systems to handle it
> because this is what they're used to doing anyway.  It also makes it a
> bit easier on the user, because it just becomes `make` to build
> whichever version you want installed.

Re: [RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[Austin S. Hemmelgarn]

On 2018-08-30 13:13, Axel Burri wrote:
> On 29/08/2018 21.02, Austin S. Hemmelgarn wrote:
>> On 2018-08-29 13:24, Axel Burri wrote:
>>> This patch allows to build distinct binaries for specific btrfs
>>> subcommands, e.g. "btrfs-subvolume-show" which would be identical to
>>> "btrfs subvolume show".
>>>
>>>
>>> Motivation:
>>>
>>> While btrfs-progs offer the all-inclusive "btrfs" command, it gets
>>> pretty cumbersome to restrict privileges to the subcommands [1].
>>> Common approaches are to either setuid root for "/sbin/btrfs" (which
>>> is not recommended at all), or to write sudo rules for each
>>> subcommand.
>>>
>>> Separating the subcommands into distinct binaries makes it easy to set
>>> elevated privileges using capabilities(7) or setuid. A typical use
>>> case where this is needed is when it comes to automated scripts,
>>> e.g. btrbk [2] [3] creating snapshots and send/receive them via ssh.
>> Let me start by saying I think this is a great idea to have as an
>> option, and that the motivation is a particularly good one.
>>
>> I've posted my opinions on your two open questions below, but there's
>> two other comments I'd like to make:
>>
>> * Is there some particular reason that this only includes the commands
>> it does, and _hard codes_ which ones it works with?  if we just do
>> everything instead of only the stuff we think needs certain
>> capabilities, then we can auto-generate the list of commands to be
>> processed based on function names in the C files, and it will
>> automatically pick up any newly added commands.  At the very least, it
>> could still parse through the C files and look for tags in the comments
>> for the functions to indicate which ones need to be processed this way.
>> Either case will make it significantly easier to add new commands, and
>> would also better justify the overhead of shipping all the files
>> pre-generated (because there would be much more involved in
>> pre-generating them).
> 
> It includes the commands that are required by btrbk. It was quite
> painful to figure out the required capabilities (reading kernel code and
> some trial and error involved), and I did not get around to include
> other commands yet.
Yeah, I can imagine that it was not an easy task.  I've actually been 
thinking of writing a script to scan the kernel sources and assemble a 
summary of the permissions checks performed by each system call and 
ioctl so that stuff like this is a bit easier, but that's unfortunately 
way beyond my abilities right now (parsing C and building call graphs is 
not easy no matter what language you're doing it with).
> 
> I like your idea of adding some tags in the C files, I'll try to
> implement this, and we'll see what it gets to.
Something embedded in the comments is likely to be the easiest option in 
terms of making sure it doesn't break the regular build.  Just the 
tagging in general would be useful as documentation though.

It would be kind of neat to have the list of capabilities needed for 
each one auto-generated from what it calls, but that's getting into some 
particularly complex territory that would likely require call graphs to 
properly implement.
> 
>> * While not essential, it would be really neat to have the `btrfs`
>> command detect if an associated binary exists for whatever command was
>> just invoked, and automatically exec that (possibly with some
>> verification) instead of calling the command directly so that desired
>> permissions are enforced.  This would mitigate the need for users to
>> remember different command names depending on execution context.
> 
> Hmm this sounds a bit too magic for me, and would probably be more
> confusing than useful. It would mean than running "btrfs" as user would
> work when splitted commands are available, and would not work if not.
It would also mean scripts would not have to add special handling for 
the case of running as a non-root user and seeing if the split commands 
actually exist or not (and, for that matter, would not have to directly 
depend on having the split commands at all), and that users would not 
need to worry about how to call BTRFS based on who they were running as. 
  Realistically, I'd expect the same error to show if the binary isn't 
available as if it's not executable, so that it just becomes a case of 
'if you see this error, re-run the same thing as root and it should work'.
> 
>>>
>>>
>>> Description:
>>>
>>> Patch 1 adds a template as well as a generator shell script for the
>>> splitted subcommands.
>>>
>>> Patch 2 adds the generated subcommand source files.
>>>
>>> Patch 3-5 adds a "install-splitcmd-setcap" make target, with different
>>> approaches (either hardcoded in Makefile, or more generically by
>>> including "Makefile.install_setcap" generated by "splitcmd-gen.sh").
>>>
>>>
>>> Open Questions:
>>>
>>> 1. "make install-splitcmd-setcap" installs the binaries with hardcoded
>>> group "btrfs". This needs to be configurable (how?). Another approach
>>> would be to not set the group at all, and leave this to the user or
>>> distro packaging script.
>> Leave it to the user or distro.  It's likely to end up standardized on
>> the name 'btrfs', but it should be agnostic of that.
>>>
>>> 2. Instead of the "install-splitcmd-setcap" make target, we could
>>> introduce a "configure --enable-splitted-subcommands" option, which
>>> would simply add all splitcmd binaries to the "all" and "install"
>>> targets without special treatment, and leave the setcap stuff to the
>>> user or distro packaging script (at least in gentoo, this needs to be
>>> specified using the "fcaps" eclass anyways [5]).
>> A bit of a nitpick, but 'split' is the proper past tense of the word
>> 'split', it's one of those exceptions that English has all over the
>> place.  Even aside from that though, I think `separate` sounds more
>> natural for the configure option, or better yet, just make it
>> `--enable-fscaps` like most other packages do.
>>
>> That aside, I think having a configure option is the best way to do
>> this, it makes it very easy for distro build systems to handle it
>> because this is what they're used to doing anyway.  It also makes it a
>> bit easier on the user, because it just becomes `make` to build
>> whichever version you want installed.

Re: [RFC PATCH 0/6] btrfs-progs: build distinct binaries for specific btrfs subcommands

[Axel Burri]

On 30/08/2018 19.23, Austin S. Hemmelgarn wrote:
> On 2018-08-30 13:13, Axel Burri wrote:
>> On 29/08/2018 21.02, Austin S. Hemmelgarn wrote:
>>> On 2018-08-29 13:24, Axel Burri wrote:
>>>> This patch allows to build distinct binaries for specific btrfs
>>>> subcommands, e.g. "btrfs-subvolume-show" which would be identical to
>>>> "btrfs subvolume show".
>>>>
>>>>
>>>> Motivation:
>>>>
>>>> While btrfs-progs offer the all-inclusive "btrfs" command, it gets
>>>> pretty cumbersome to restrict privileges to the subcommands [1].
>>>> Common approaches are to either setuid root for "/sbin/btrfs" (which
>>>> is not recommended at all), or to write sudo rules for each
>>>> subcommand.
>>>>
>>>> Separating the subcommands into distinct binaries makes it easy to set
>>>> elevated privileges using capabilities(7) or setuid. A typical use
>>>> case where this is needed is when it comes to automated scripts,
>>>> e.g. btrbk [2] [3] creating snapshots and send/receive them via ssh.
>>> Let me start by saying I think this is a great idea to have as an
>>> option, and that the motivation is a particularly good one.
>>>
>>> I've posted my opinions on your two open questions below, but there's
>>> two other comments I'd like to make:
>>>
>>> * Is there some particular reason that this only includes the commands
>>> it does, and _hard codes_ which ones it works with?  if we just do
>>> everything instead of only the stuff we think needs certain
>>> capabilities, then we can auto-generate the list of commands to be
>>> processed based on function names in the C files, and it will
>>> automatically pick up any newly added commands.  At the very least, it
>>> could still parse through the C files and look for tags in the comments
>>> for the functions to indicate which ones need to be processed this way.
>>> Either case will make it significantly easier to add new commands, and
>>> would also better justify the overhead of shipping all the files
>>> pre-generated (because there would be much more involved in
>>> pre-generating them).
>>
>> It includes the commands that are required by btrbk. It was quite
>> painful to figure out the required capabilities (reading kernel code and
>> some trial and error involved), and I did not get around to include
>> other commands yet.
> Yeah, I can imagine that it was not an easy task.  I've actually been
> thinking of writing a script to scan the kernel sources and assemble a
> summary of the permissions checks performed by each system call and
> ioctl so that stuff like this is a bit easier, but that's unfortunately
> way beyond my abilities right now (parsing C and building call graphs is
> not easy no matter what language you're doing it with).
>>
>> I like your idea of adding some tags in the C files, I'll try to
>> implement this, and we'll see what it gets to.
> Something embedded in the comments is likely to be the easiest option in
> terms of making sure it doesn't break the regular build.  Just the
> tagging in general would be useful as documentation though.
> 
> It would be kind of neat to have the list of capabilities needed for
> each one auto-generated from what it calls, but that's getting into some
> particularly complex territory that would likely require call graphs to
> properly implement.

After some more iterations I came up with a generic "Makefile only"
version of this patchset. The new version does not need generated
c-files, and matches entry point declarations as well as additional tags
(for fscaps declaration or future enhancements) in all "cmds-*.c" files.

See new thread: "[RFC PATCH v2 0/4] btrfs-progs: build distinct binaries
for specific btrfs subcommands"


>>
>>> * While not essential, it would be really neat to have the `btrfs`
>>> command detect if an associated binary exists for whatever command was
>>> just invoked, and automatically exec that (possibly with some
>>> verification) instead of calling the command directly so that desired
>>> permissions are enforced.  This would mitigate the need for users to
>>> remember different command names depending on execution context.
>>
>> Hmm this sounds a bit too magic for me, and would probably be more
>> confusing than useful. It would mean than running "btrfs" as user would
>> work when splitted commands are available, and would not work if not.
> It would also mean scripts would not have to add special handling for
> the case of running as a non-root user and seeing if the split commands
> actually exist or not (and, for that matter, would not have to directly
> depend on having the split commands at all), and that users would not
> need to worry about how to call BTRFS based on who they were running as.
>  Realistically, I'd expect the same error to show if the binary isn't
> available as if it's not executable, so that it just becomes a case of
> 'if you see this error, re-run the same thing as root and it should work'.
>>
>>>>
>>>>
>>>> Description:
>>>>
>>>> Patch 1 adds a template as well as a generator shell script for the
>>>> splitted subcommands.
>>>>
>>>> Patch 2 adds the generated subcommand source files.
>>>>
>>>> Patch 3-5 adds a "install-splitcmd-setcap" make target, with different
>>>> approaches (either hardcoded in Makefile, or more generically by
>>>> including "Makefile.install_setcap" generated by "splitcmd-gen.sh").
>>>>
>>>>
>>>> Open Questions:
>>>>
>>>> 1. "make install-splitcmd-setcap" installs the binaries with hardcoded
>>>> group "btrfs". This needs to be configurable (how?). Another approach
>>>> would be to not set the group at all, and leave this to the user or
>>>> distro packaging script.
>>> Leave it to the user or distro.  It's likely to end up standardized on
>>> the name 'btrfs', but it should be agnostic of that.
>>>>
>>>> 2. Instead of the "install-splitcmd-setcap" make target, we could
>>>> introduce a "configure --enable-splitted-subcommands" option, which
>>>> would simply add all splitcmd binaries to the "all" and "install"
>>>> targets without special treatment, and leave the setcap stuff to the
>>>> user or distro packaging script (at least in gentoo, this needs to be
>>>> specified using the "fcaps" eclass anyways [5]).
>>> A bit of a nitpick, but 'split' is the proper past tense of the word
>>> 'split', it's one of those exceptions that English has all over the
>>> place.  Even aside from that though, I think `separate` sounds more
>>> natural for the configure option, or better yet, just make it
>>> `--enable-fscaps` like most other packages do.
>>>
>>> That aside, I think having a configure option is the best way to do
>>> this, it makes it very easy for distro build systems to handle it
>>> because this is what they're used to doing anyway.  It also makes it a
>>> bit easier on the user, because it just becomes `make` to build
>>> whichever version you want installed.
>