This sounds good! I'm assuming we'll need to keep CAP_SYS_MOUNT.

Also, Ben commented in the CL that he had some trouble with PID namespaces. Commenting here so that we don't forget.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/a3aeae31bd4c8e1e69a5aef54ef46b73d6dc3c72

commit a3aeae31bd4c8e1e69a5aef54ef46b73d6dc3c72
Author: Ben Chan <benchan@chromium.org>
Date: Thu Jun 08 06:45:51 2017

cros-disks: run cros-disks inside new IPC namespace

This CL modifies the cros-disks upstart job to run cros-disks in a more
contained environment with new IPC namespace.

BUG= chromium:726036 
TEST=Tested the following:
1. Verify that cros-disks can be started, stopped, and restarted by upstart:

     stop cros-disks
     status cros-disks   # cros-disks stop/waiting

     start cros-disks
     status cros-disks   # cros-disks start/running, process 24381
                         # where process 24381 is PID of cros-disks

     restart cros-disks
     status cros-disks   # cros-disks start/running, process 24535
                         # where process 24535 is PID of cros-disks

2. Run platform_CrosDisksDBus, platform_CrosDisksFilesystem, and
   platform_CrosDisksFormat tests.
3. Insert FAT / NTFS / exFAT formatted USB drive and verify read/write
   operations on the drive via Files.app.
4. Format a USB drive via Files.app.
5. Open a RAR archive via Files.app.

Change-Id: Iff9604a536f2249ab22ec1f61f356166563704f6
Reviewed-on: https://chromium-review.googlesource.com/522408
Commit-Ready: Ben Chan <benchan@chromium.org>
Tested-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/a3aeae31bd4c8e1e69a5aef54ef46b73d6dc3c72/cros-disks/cros-disks.conf

I did some brief testing of cros-disks using a non-root user, and using CAP_SYS_ADMIN (for mount) and CAP_SYS_MODULE (to add the FUSE module) didn't work -- the platform_CrosDisksFormat test fails:

platform_CrosDisksFormat.vfat   FAIL: FormatCompleted signal not matched on "status": expected={'status': 0, 'path': '/dev/loop6'}, actual={'status': dbus.UInt32(7L), 'path': dbus.String(u'/dev/loop6')}

Not sure what more stuff is needed.

/dev/loop devices are owned by root:disk and are 660 :(

I don't think cros-disks uses loop devices for anything other than testing, right? We should be able to do something here.

afaik, we don't use loop devices at runtime ... we only allow sourcing from real disks

the autotest could chmod 666 all the loopback nodes when it starts and then chmod 660 when it finishes ...

Yeah I'm gonna try something like that.

Yes, we can easily modify VirtualFilesystemImage in third_party/autotest/files/client/cros/cros_disks.py to setup the permissions of the loop device for tests.  Jorge, if you haven't started yet, I can put together a fix.

I gave that a shot (but by modifying the setup() method in the test) and things almost work, but the ntfs test is failing. I think the problem is that the /dev/loop6 device ends up being owned by the owner used for the previous test -- maybe we need to reset that between tests or something.

The other required change is to have cros-disks get all the capabilities and have FUSEMounter not set capabilities, and use ambient caps.

If you have a VirtualFilesystemImage ready to go, that could help! I'll try to upload a WIP CL for cros-disks to show what it would look like.

Actually, /dev/sdX are owned by root:disk and are 660 as well. Jorge, are you trying to run cros-disks under a different user/group?

Oh yes sorry I should have clarified that. As part of the reduced capabilities I was running cros-disks as a non-root user.

Would that user be part of the disk group?  If not, we would need to chgrp /dev/<device> accessible by cros-disks

And more importantly, are we planning to put /dev/sdX and /dev/loopX under different group such that only the former can be accessed by cros-disks?

or add cros-disks user to the disks group

We can do what Mike suggests -- can you confirm that we only use loopback devices for testing?

yes, loop device is only used for testing

BTW, the cros-disks user needs to be in the chronos-access group as well in order to mount files under /home/chronos/u-<hash>/{Downloads, GCache}

I spent the entire afternoon trying to figure out why the ntfs test was failing on my kevin, only to eventually try a non-modified cros-disks and... still seeing the failure. Looks like ntfs has been failing on kevin and kevin-like devices for a while:

https://wmatrix.googleplex.com/unfiltered?hide_missing=True&releases=tot&tests=platform_CrosDisksFilesystem.ntfs

Ben, any ideas? Is this known?

From the test logs, it seems like the failure happened when the test invoked mkfs.ntfs to create a virtual disk image. I couldn't reproduce that on my chell, so mkfs.ntfs could be failing on kevin (which I think is aarch64?). Will need to debug mkfs.ntfs a bit.  Were you able to mount a NTFS-formatted USB on kevin?  That would at least verify that ntfs-3g works on kevin.

I can try that tomorrow.

Filed  issue 739958  for the mkfs.ntfs problem.

Accessing an NTFS-formatted flash drive works on Kevin.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/a61554a0e2e16517ac289796f6ff4052566a7892

commit a61554a0e2e16517ac289796f6ff4052566a7892
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Sat Jul 15 06:22:06 2017

ntfs3g: Don't make the 'ntfs3g' executable setuid.

cros-disks, currently running as root, launches ntfs3g as the ntfs-3g
user with CAP_SYS_ADMIN + CAP_SYS_SETUID + CAP_SYS_SETGID capabilities,
so 'ntfs3g' doesn't need to be a setuid executable. Even if cros-disks
later runs as a unprivileged user, we can run cros-disks and ntfs3g
with sufficient capabilities for ntfs3g to be able to mount things.

In order for non-root cros-disks to work, the 'ntfs3g' executable needs
to have o+x permissions, or the cros-disks user needs to belong to the
ntfs3g group. If 'ntfs3g' is not setuid, we can make it o+x, and we
don't need to modify groups.

BUG= chromium:726036 
TEST=ls -l /build/veyron_minnie/usr/bin/ntfs3g, it's not setuid.
TEST=platform_CrosDisks*
TEST=Plug NTFS-formatted device, see contents on Files app.

Change-Id: If265f689da96022a70809229c474941c90629915
Reviewed-on: https://chromium-review.googlesource.com/568806
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[rename] https://crrev.com/a61554a0e2e16517ac289796f6ff4052566a7892/sys-fs/ntfs3g/ntfs3g-2017.3.23-r5.ebuild
[modify] https://crrev.com/a61554a0e2e16517ac289796f6ff4052566a7892/profiles/targets/chromeos/package.use

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/b7dafc0a4410067859b37f203f3a285be33c1ade

commit b7dafc0a4410067859b37f203f3a285be33c1ade
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Mon Jul 24 15:24:14 2017

security_SuidBinaries: Remove 'ntfs-3g'.

No longer marked setuid after CL:568806.

BUG= chromium:726036 
TEST=Passes on veyron_minnie.
CQ-DEPEND=CL:568806

Change-Id: I641663d51439f934e27f6dbdccbf1dcd90c34caa
Reviewed-on: https://chromium-review.googlesource.com/572281
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/b7dafc0a4410067859b37f203f3a285be33c1ade/client/site_tests/security_SuidBinaries/baseline.suid

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/eclass-overlay/+/62b41d62e6165372c4f6b668222c295ab51011f6

commit 62b41d62e6165372c4f6b668222c295ab51011f6
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Fri Aug 11 17:35:51 2017

Add cros-disks user to disk, chronos-access groups.

-'disk' group is required to access block devices, and loop devices
(for testing).
-'chronos-access' group is required for cros-disks to mount files under
/home/chronos/u-<hash>/{Downloads, GCache}.

Even though this does give the cros-disks user more power, the
comparison should be made against cros-disks running as root,
which already has access to these /dev nodes.

BUG= chromium:726036 
TEST=Build image, check /etc/group.
TEST=security_AccountsBaseline passes on minnie.
TEST=With CL:563738:
TEST=Run platform_CrosDisks* tests.
CQ-DEPEND=CL:571951

Change-Id: I136cf4494bd89ed87c431858bf82d795f25f4e7b
Reviewed-on: https://chromium-review.googlesource.com/571280
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Jorge Lucangeli Obes <jorgelo@chromium.org>

[modify] https://crrev.com/62b41d62e6165372c4f6b668222c295ab51011f6/profiles/base/accounts/group/chronos-access
[modify] https://crrev.com/62b41d62e6165372c4f6b668222c295ab51011f6/profiles/base/accounts/group/disk

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/53dc1ad837f2b196d63f61477904e7f8ef0f168a

commit 53dc1ad837f2b196d63f61477904e7f8ef0f168a
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Fri Aug 11 17:35:51 2017

security_AccountsBaseline: Add cros-disks to new groups.

BUG= chromium:726036 
TEST=Passes on minnie.
CQ-DEPEND=CL:571280

Change-Id: I1cc75acd8378fc7ef27c58a3a8e67d9146585742
Reviewed-on: https://chromium-review.googlesource.com/571951
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/53dc1ad837f2b196d63f61477904e7f8ef0f168a/client/site_tests/security_AccountsBaseline/baseline.group

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/autotest/+/47e8dee3b4d24c2015241d31048278d2ae401483

commit 47e8dee3b4d24c2015241d31048278d2ae401483
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Mon Aug 14 21:57:21 2017

security_SandboxedServices: Update 'disks' baseline.

BUG= chromium:726036 
TEST=Passes on kevin.
CQ-DEPEND=CL:563738

Change-Id: I5e0d3d7bffb82713b4c096395c71ee15dcf89bd4
Reviewed-on: https://chromium-review.googlesource.com/612412
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/47e8dee3b4d24c2015241d31048278d2ae401483/client/site_tests/security_SandboxedServices/baseline

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8d9729befaecbac52677511afd48ff7cc217d834

commit 8d9729befaecbac52677511afd48ff7cc217d834
Author: Jorge Lucangeli Obes <jorgelo@chromium.org>
Date: Mon Aug 14 21:57:21 2017

cros-disks: Don't run as root.

BUG= chromium:726036 
TEST=platform_CrosDisks*, security_SandboxedServices
CQ-DEPEND=CL:571951,CL:612412

Change-Id: Ie0dcb7df042f091d4c17cda72ba22b139bc8bcaa
Reviewed-on: https://chromium-review.googlesource.com/563738
Commit-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/8d9729befaecbac52677511afd48ff7cc217d834/cros-disks/fuse_mounter.cc
[modify] https://crrev.com/8d9729befaecbac52677511afd48ff7cc217d834/cros-disks/org.chromium.CrosDisks.conf
[modify] https://crrev.com/8d9729befaecbac52677511afd48ff7cc217d834/cros-disks/cros-disks.conf

Gonna call this fixed.