[Buildroot] [PATCH 1/3] fs: apply permissions late

[Yann E. MORIN <yann.morin.1998@free.fr>]

Arnout, All,

On 2018-11-08 00:43 +0100, Arnout Vandecappelle spake thusly:
> On 27/10/18 09:45, Yann E. MORIN wrote:
> > The combination of fakeroot, tar, and capabilities is broken, because
> > fakeroot currently badly handles capabilities, which are currently
> > simply ignored.
> 
>  "simply ignored" can't be the case, otherwise it still wouldn't work after this
> patch.

No, because fakeroot really badly handles capabilties, as demonstrated
in another mail in that thread. And yes, capabilties *are* currently
simply ignored, and that's on purpose (not that changing would fix it
anyway, and I'm not even totally sure this is related, and neither am I
really sure to understand what it even really means), added in
c994c3db1f7d (fakeroot: disable capabilities):

    https://git.buildroot.org/buildroot/tree/package/fakeroot/fakeroot.mk#n12

    # Force capabilities detection off
    # For now these are process capabilities (faked) rather than file
    # so they're of no real use
    HOST_FAKEROOT_CONF_ENV = \
        ac_cv_header_sys_capability_h=no \
        ac_cv_func_capset=no

[--SNIP--]
> > As described in #11216, asking tar to explicitly store and restore
> > capabilities ends up with a failling build, when tar actually tries to
> > restore the capabilities. Adding support for capabilities to fakeroot
> > (by adding host-libcap as dependency) does not fix the problem.
> 
>  Just to be clear: with this patch, the 'tar' filesystem does still work because
> the creation of the tarball works OK, it's just the extraction that goes wrong,
> right?

Oh, you poinpoint an issue: the tar filesystem must be also modified to
add --xattrs-include='*' otherwise it would not contain any xattrs, and
even less so, any capabilty.

So, with this patch, the 'tar' filesystem is not fixed, but this is not
a regression either, as it did not have capabilties, and it still does
not.

Now, should we fix the tar filesystem, a tarball of the rootfs is
supposed to be extracted by root, the real root, not a fake root, and
as Peter demonstrated, there is no problem with capabilities in a
tarball that is extracted by root.

> > Capabilities are stored in the extended attribute security.capabilty.
> > It turns out that tar does have special handling when extracting and
> > restoring that extended attribute, and that fails miserably when running
> > under fakeroot...
> > 
> > We fix that by offloading the permissions handling down to individual
> > filesystems.
> > 
> > This needs a split of the makedevs call, with the current and first one
> > now only responsible for creating the pseudo devices, while the new,
> > second call does only set the permissions.
> 
>  Why? Is that just to reduce the impact on post-fakeroot scripts?

The idea of the common, intermediate tarball was to include all the
common actions, to avoid repeating them for each filesystem.

Since the creating of device nodes in /dev is not impacted by the
capabilties, I left that in the common part, and only moved the
offending part (setting permissions, which includes capabilties) to the
per-filesystem actions.

It had about nothing to do with the post-fakeroot scripts that I had
thought about.

>  I'd rather move the entire makedevs call to the per-filesystem phase.

I'm still not convinced, as this is not needed (capabilties don't make
sense on device nodes), except if we were to entirely drop the
intemediate tarball altogether.

> > Fixes: #11216
> > 
> > This changes the order of steps, and post-fakeroot scripts are now
> > called before the permissions are set. This could mean breaking existing
> > setups, but more probably, this woudl sovle some, where files created in
> > post-fakeroot scripts can now see their permissions appropriately set.
> 
>  Since extracting xattrs doesn't work correctly, this is not true.i

Sorry, re-reading this, I see where you choked on my explanations. The
underlying reasoning I had, was that files created in a post-fakeroot
scripts could have appropriate capabilties by providing a permissions
table, which is _now_ applied after those files are created.

So, before this patch, capabilities created in a post-fakeroot scripts
were not working, and there was no way to set capabilties on those
files. With this patch, they are not working either, but there is a
workaround by providing a permissions table.

> And normally
> in a post-fakeroot script, you would do any permission setting in the script
> itself and not use the permissions table.

Except that was not working so far, and nobody complained, so nobody was
doing that anyway.

> So I think this statement is utterly
> wrong.

s/utterly//  makes it easier to read, thanks. And yes, it was wrong,
except it did not properly convey my thinking. I hope the above
explanations make it a bit less wrong.

> That said, I doubt that post-build scripts would be affected by the
> permissions tables anyway.

> > This also slightly breaks the idea behind the intermediate image, which
> > was supposed to gather all actions common to all filesystems, so that
> > they are not repeated. Still, most actions are still created only once,
> > and moving just this is purely a practical and pragmatic workaround.
> 
>  Still, I'd prefer to fix fakeroot :-)

Then, please be my guest! ;-) I already fixed something in fakeroot, and
I dread working on that code base any more...

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'