Change pot root mountpoint to require root privileges

[grembo]

Many existing pot commands break in strange ways if the user doesn't have
root permissions. Changing the access mode of jails to root-readable only
(0700) makes this worse, as many pot commands read jails/.../pot.conf.
It is very necessary though for these reasons:

1. pot.conf can contain environment variables that contain secrets that
   shouldn't be readable by unprivileged users on the host system.
2. Anything within the jail's filesystem that isn't protected properly is
   exposed to unprivileged users on the host system. One prime example is
   the content of /secrets when using the nomad pot driver - the nomad
   project relies on data_dir being protected with proper permissions and
   uses file mode 0666 for everything within the container, including the
   content of the secrets dir[0]. As these directories will be shared
   with the container using a nullfs mount, it's very important to protect
   the container's root, which is accomplished with this change.

[0] See also: hashicorp/nomad#11900

[pizzamig]

While I agree with the need to improve security, I have few remarks:

• environment variables shouldn't contain secrets in pot.conf. secrets management should be delegated to systems like vault and dynamically rendered by nomad, and pot.conf should stay readable by any users
• instead of protecting jail root, we could limit the protection to the root mountpoint (the m folder inside the pot folder)
  Security works better if we limit the need of being root, not the other way around. That's why there is a function (_is_uid0) that is used only in commands that need root credentials...
  WDYT?

[grembo]

Protecting just the mountpoints would work for me, if it’s done reliably. This would also have the advantage of protecting existing pot installations on update.

The reason I brought up pot.conf is that many potluck images use (or at least used in the past) environment variables to configure the pot, including things like passwords.

For the images I use I changed this to using vault cubbyhole wrapped secrets instead, which I place in a file in a mount-in path with appropriate permissions.

The other reason I changed permissions of /opt/pot/jails is that it’s easy to control - getting all mountpoints right might be a bit more work/complicated, but maybe I’m a bit too pessimistic there.

[grembo]

@pizzamig I updated the review with a draft quality patch (not thoroughly tested and unit tests not adapted) that tries to do two things:

1. Create the pot root mountpoint with the correct permissions whenever it could be created by accident
2. Correct pot root mountpoint permissions whenever it could've been imported with lax permissions (clone existing image, import remote image)

Do you think this is a viable approach?

[grembo]

What's also a bit unclear is how this would work for type multi pots (I'm not using these myself).

[grembo]

@pizzamig It seems like that monitor doesn't for for commands that are run in a subshell (e.g., to capture their output) - as counters are set in the wrong shell. This could be worked around by using a temporary file in monitor.sh.

[pizzamig]

multi: you have a /m as well.
/m is empty, because it will mount a FreeBSD tree there.
However, multi doesn't support images, nomad-pot-driver and all the nice stuff.

monitor is useless if the stub command is executed in a subshell. I constantly forget about it every time I use it.
to avoid complications, I usually only check the side effects of the stub.
A temporary file is the side effect that lives outside a subshell, but I'm not sure it's worth the effort

[grembo]

@pizzamig

monitor is useless if the stub command is executed in a subshell. I constantly forget about it every time I use it.
to avoid complications, I usually only check the side effects of the stub.
A temporary file is the side effect that lives outside a subshell, but I'm not sure it's worth the effort

I implemented a monitor that can trace commands in subshells as well - this helps a lot with anything ZFS, where all calls are stubbed and no side-effects would be visible otherwise. Implemented separately in #220, I rebased this review on top of that one.

[pizzamig]

While the proposed patch is good to land, I would add an entry in the changelog before to commit.
Then, it's good to be merged

[grembo]

@pizzamig Thanks for reviewing