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

[Axel Burri <axel@tty0.ch>]

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.
>