c/snap-bootstrap/initramfs_systemd_mount: add support for dm-verity options #14178
Conversation
|
Hey @sespiros can you please link this to a Jira card if one exists. |
Added the link to the description. |
and address comments in the test suite
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #14178 +/- ##
==========================================
- Coverage 78.73% 78.72% -0.01%
==========================================
Files 1055 1061 +6
Lines 138275 139831 +1556
==========================================
+ Hits 108866 110080 +1214
- Misses 22588 22870 +282
- Partials 6821 6881 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| } | ||
| if opts.VerityHashDevice != "" && opts.VerityRootHash != "" { | ||
| options = append(options, fmt.Sprintf("verity.roothash=%s", opts.VerityRootHash)) | ||
| options = append(options, fmt.Sprintf("verity.hashdevice=%s", opts.VerityHashDevice)) |
There was a problem hiding this comment.
Do we need to possibly escape the hash device path? Is there any convention about this? What if the device has a space or a comma in the name?
There was a problem hiding this comment.
Thanks for catching this. I had already done escaping for the comma in the overlayfs PR (9850c03) so I added the same here.
I added an extra test case for this but I have it commented out for now as the test code will need to use SplitMountOptions() (introduced in f1171b9 and later moved in 3477400) to properly split the mount options. The code for SplitMountOptions() will be introduced as part of the overlayfs PR, so once that one is merged I will uncomment the test and remove the TODO comment.
Spaces don't seem to need escaping. These are options that get passed to systemd-mount when it's ran using Go's Exec so I think the shell escapes those for us. This is some example code I used to test this:
# First create the necessary folders under tmp
mkdir -p /tmp/overlay/lower\
mkdir -p /tmp/overlay/upper
mkdir -p /tmp/overlay/workfunc (s *doSystemdMountSuite) TestSpace(c *C) {
dirWithSpace := "/tmp/overlay/lower "
args := []string{dirWithSpace, "/tmp/overlay/merged"}
args = append(args, "--type=overlay")
var options []string
lowerdir := dirWithSpace
upperdir := "/tmp/overlay/upper"
workdir := "/tmp/overlay/work"
options = append(options, fmt.Sprintf("lowerdir=%s", strings.ReplaceAll(lowerdir, ",", `\,`)))
options = append(options, fmt.Sprintf("upperdir=%s", strings.ReplaceAll(upperdir, ",", `\,`)))
options = append(options, fmt.Sprintf("workdir=%s", strings.ReplaceAll(workdir, ",", `\,`)))
args = append(args, "--options="+strings.Join(options, ","))
cmd := exec.Command("systemd-mount", args...)
// Run the command and capture the output
output, err := cmd.Output()
// Check for errors
if err != nil {
log.Fatalf("Error running command: %v", err)
}
// Print the output
fmt.Printf("Command output: %s\n", output)
}There was a problem hiding this comment.
Spaces do need escaping. Look at the issues with p9 file system. If you don't escape spaces then /proc/self/mountinfo cannot be parsed correctly, as it has optional fields and options (the naming is beyond our control) and both are separated by spaces from each other (any number of optional fields and the singular "options" field).
There was a problem hiding this comment.
Interesting! Do we support this in snapd currently? I guess I could perform the same escaping for spaces as well in addition to the commas (and I guess I would also need to do that in the overlay PR). I am not aware of a more exact convention about this.
There was a problem hiding this comment.
I hope we don't have to use spaces but the point is that the options field is quite fragile so we should employ some escape logic for it. My advice is to look at the source code carefully: https://elixir.bootlin.com/linux/v6.10.9/source/fs/overlayfs/params.c#L189
The unfortunate reality is that it is a mess and literally every routine does something custom. In some places spaces are escaped, in others they are not.
There was a problem hiding this comment.
Added a commit for this to both this and the overlay PR.
| } | ||
| if opts.VerityHashDevice != "" && opts.VerityRootHash != "" { | ||
| options = append(options, fmt.Sprintf("verity.roothash=%s", opts.VerityRootHash)) | ||
| options = append(options, fmt.Sprintf("verity.hashdevice=%s", strings.ReplaceAll(opts.VerityHashDevice, ",", `\,`))) |
There was a problem hiding this comment.
I think it is very important to have a clear picture of who is parsing this and how it is done. I've done by best to figure out how this is used and this is what I think happens:
- options are given to systemd-mount from systemd
- then options are given to libmount from util-linux in
libmount/src/hook_veritydev.c - then some if this uses cryptsetup to setup the device
As far as I can see, there's no escaping anywhere. So after the mount options are parsed by libmount, all paths are verbatim. I'm trying to see if libmount has some escape logic.
ernestl
added
the
Azure CVM/rootfs integrity
| options = append(options, fmt.Sprintf("verity.roothash=%s", opts.VerityRootHash)) | ||
|
|
||
| escaper := strings.NewReplacer(",", `\,`, " ", `\ `) | ||
| options = append(options, fmt.Sprintf("verity.hashdevice=%s", escaper.Replace(opts.VerityHashDevice))) |
There was a problem hiding this comment.
Would that work to use "verity.hashdevice=\"%s\""
There was a problem hiding this comment.
- I reverted the escaping logic.
- I escaped the hash device using double quotes.
- I forbid backslashes, commas, spaces, colons, double quotes and spaces from appearing in paths used for dm-verity hash devices.
- added a test.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #14178 +/- ##
==========================================
+ Coverage 78.95% 79.03% +0.08%
==========================================
Files 1084 1087 +3
Lines 146638 147700 +1062
==========================================
+ Hits 115773 116738 +965
- Misses 23667 23732 +65
- Partials 7198 7230 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
double quoting the options to systemd-mount resulted in errors for the overlayfs case such as: localhost kernel: overlayfs: failed to resolve '"/run/mnt/foo"': -2 which is possibly due to an older util-linux being used. In the same spirit, since we now forbid many characters, we can safely remove the extra double quotes.
| } | ||
|
|
||
| if strings.ContainsAny(opts.VerityHashDevice, forbiddenChars) { | ||
| return fmt.Errorf("cannot mount %q at %q: dm-verity hash device path contains forbidden characters. %q contains one of \"%s\".", what, where, opts.VerityHashDevice, forbiddenChars) |
There was a problem hiding this comment.
| return fmt.Errorf("cannot mount %q at %q: dm-verity hash device path contains forbidden characters. %q contains one of \"%s\".", what, where, opts.VerityHashDevice, forbiddenChars) | |
| return fmt.Errorf("cannot mount %q at %q: dm-verity hash device path contains forbidden characters. %q contains one of %q".", what, where, opts.VerityHashDevice, forbiddenChars) |
There was a problem hiding this comment.
Thanks! Pushed the change.
|
Everyone contributing to this PR have now signed the CLA. Thanks! |
* master: (486 commits) o/snapstate, daemon: respect lanes that are passed to snapstate.InstallComponents, snapstate.InstallComponentPath (canonical#14695) cmd/snap-bootstrap: support for noexec and nodev options in preparation of tightening mount options for ubuntu core partitions o/hookstate: allow snapctl model command for kernel snaps many: complete various component cross-checks (canonical#14634) github: only check formatting when using latest/stable Go tests/main/auto-refresh-backoff: fix sorting of changes from the state (canonical#14691) store: place downloaded file in cache after rebuilding from delta (canonical#14680) interfaces/desktop: Improve launch entry and systray integration with session (canonical#14132) interfaces: desktop session improvements to shutdown,{system,upower}-observe and desktop (canonical#13947) github: run make distcheck packaging: symlink fedora-latest to fedora packaging github: run unit tests on fedora:latest s/snapenv, snapdtool: fix tests that depend on user's SNAP_REEXEC (canonical#14681) store: refactor and improve store cache (canonical#14524) many: registry API creates transaction commit change (canonical#14675) tests/main/nvidia-files: set ubuntu-20.04-64/550-server to broken packaging in table of expectations (canonical#14679) c/snap-mgmt: unmount components before unmounting snaps (canonical#14664) .github/workflows/coverity-scan.yaml: add github action for Coverity (canonical#14654) data/selinux: remove timedatex (canonical#14670) c/snap-bootstrap: import users from hybrid system into recovery system (canonical#14521) ...
sespiros
force-pushed
the
add-verity-options
branch
from
7957fda to
4ff8dd5
Compare
alfonsosanchezbeato
added
the
Run nested
* master: (44 commits) wrappers: do not reload activation units (canonical#14724) gadget/install: support for no{exec,dev,suid} mount flags interfaces/builtin/mount_control: add support for nfs mounts (canonical#14694) tests: use gojq - part 1 (canonical#14686) interfaces/desktop-legacy: allow DBus access to com.canonical.dbusmenu interfaces/builtin/fwupd.go: allow access to nvmem for thunderbolt plugin tests: removing uc16 executions (canonical#14575) tests: Added arm github runner to build snapd (canonical#14504) tests: no need to run spread when there are not tests matching the filter (canonical#14728) tests/lib/tools/store-state: exit on errors, update relevant tests (canonical#14725) tests: udpate the github workflow to run tests suggested by spread-filter tool (canonical#14519) testtime: add mockable timers for use in tests (canonical#14672) interface/screen_inhibit_control: Improve screen inhibit control for use on core (canonical#14134) tests: use images with 20G disk in openstack (canonical#14720) i/builtin: allow @ in custom-device filepaths (canonical#14651) tests: refactor test-snapd-desktop-layout-with-content tests: fix broken app definition tests: capitalize sentences in comments tests: wrap very long shell line tests: fix raciness in async startup and sync install ...
That is for https://warthogs.atlassian.net/browse/FR-8351