FFe: create zfs dataset for each user automatically

Bug #1842902 reported by Didier Roche-Tolomelli
22
This bug affects 2 people
Affects Status Importance Assigned to Milestone
shadow (Ubuntu)
Fix Released
Undecided
Unassigned
zsys (Ubuntu)
Fix Released
Undecided
Unassigned

Bug Description

Part of the zsys spec is creating/associating one user dataset for each HOME user.

As zsys is an official experimentation for 19.10, we would like to include this feature in a safe way, and reachable for any tool creating users (adduser, gnome-control-center, ubiquity…). Those are using useradd under the scene.

For this, the proposed implementation:
- patch useradd trying to execute "zsys useradd create USER HOMEDIR". If zsys isn't present or zsys returns a status code != 0 (which will be the case if the running system isn't a zsys one: pure zfs or non zfs like / on ext4), it will fallback to mkdir. Then the code does the usual chmod()
- patch usermod, trying as well to execute "zsys useradd rename-home OLDHOME NEWHOME". Same failing reason (not a zsys system, not installed, OLDHOME isn't a zsys handled datasets) and fallback to rename(). Then the code does the usual chmod().

Tested with and without zsys installed, the code does what we expect.

I'm attaching the shadow (useradd/usermod) patches, as you can see it's very minimal.
A new ZSYS release will be needed (https://github.com/ubuntu/zsys). As you can see, there are quite some commits since the last release, but it's all baked (as usual) by a huge suite of tests (in ZFS and machine layers) with corner cases tested and such. I'm confident on that change.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :
Revision history for this message
Alex Murray (alexmurray) wrote :

Didier - could you please add some checks on the return values from the various open/dup2/execvl syscalls? Whilst currently I can't see a huge problem if these silently fail (open returns -1, dup2 then fails, or if dup2 fails anyway - then the only consequence is stdout/stderr is not silenced) I think it would be better to be more defensive (or if not, at least add a comment explaining why NOT checking for failures is not a problem).

Also instead of the strrstr() call (which again is unchecked but since this is running on a known string is unlikely to fail) - why not either just use zsys+6 since this is a fixed string OR just const char *pname = "zsys"; ?

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Hey Alex! Thanks for looking at it.

Sure, I've added some checks for open/dup2 (and exiting the child).

On execl, I've kept the similar logic than the rest of the code, meaning:
- don't check for execl return value.
- if something bad happen, execl returns.
- we go on the next line which is a perror() + exit.

On the strrstr(), I found it more clear than the offset and future-proof than hardcoding *pname. I don't have a strong opinion though TBH, so I have just hardcoded "zsys" directly as you suggested.

I've attached a new version of the patch.

Revision history for this message
Alex Murray (alexmurray) wrote :

Thanks Didier, looks great :)

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

attaching shadow build logs

Revision history for this message
Łukasz Zemczak (sil2100) wrote :

Thanks Didier and Alex! This looks promising. I'm generally +1 on this, especially that the shadow part of the feature doesn't have to be blocked on zsys - if all works correctly that is.

For the zsys part, I assume that once we have that we'd like to pull in zsys to main. Did you reach out to the MIR team and security team about it already? I wouldn't want us to do a last minute push of zsys right before release without proper review of all the bits and pieces.

All in all, I'm formally approving the shadow FFe and, with some fear, the zsys FFe as well. Be sure to reach out to the MIR team as early as possible. If zsys isn't main-ready before beta, well, I wouldn't want it to be part of our official images.

Changed in shadow (Ubuntu):
status: New → Triaged
Changed in zsys (Ubuntu):
status: New → Triaged
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Thanks a lot Łukasz! The MIR is available at https://bugs.launchpad.net/ubuntu/+source/zsys/+bug/1839271, the MIR part has been acked, it's pending a security review for now.

Anyway, let's proceed with this so that we can test it early in the wild, thanks again!

Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package zsys - 0.2

---------------
zsys (0.2) eoan; urgency=medium

  * Add userdata hidden subcommand for creating and renaming user datasets.
    (LP: #1842902)
  * Build-dep on grub2-common to not produce s390x build for now until
    we support an alternative bootloader.

 -- Didier Roche <email address hidden> Mon, 09 Sep 2019 15:13:26 +0200

Changed in zsys (Ubuntu):
status: Triaged → Fix Released
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package shadow - 1:4.5-1.1ubuntu4

---------------
shadow (1:4.5-1.1ubuntu4) eoan; urgency=medium

  * debian/patches/1015_add_zsys_support.patch:
    - Call zsys to handle home directory if available.
    We call zsys to handle dataset creation for zsys system in a separate
    home dataset for each user on the system.
    This allows one to handle user dataset outside of /home and also renaming.
    We don't support yet deletion, as removing the dataset would remove as
    well every snapshot of the history, and so, revert to previous version
    will result in user created, but no home directory, which is unwanted.
    (LP: #1842902)

 -- Didier Roche <email address hidden> Thu, 29 Aug 2019 15:00:07 +0200

Changed in shadow (Ubuntu):
status: Triaged → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.