Make fs-verity optional #147
Conversation
croissanne
force-pushed
the
optional-verity-v2
branch
2 times, most recently
from
dda89ee to
1e95c9f
Compare
If a repository is set to insecure, it will files that have fs-verity disabled. Specifically it will continue as normal on `MeasureVerityError:VerityMissing`. A repository will still run with fs-verity enabled if it is able to measure the verity of an image when it is opened, even if the repository was set to insecure. Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
Avoids the need to support a mountpoint when mounting root. Signed-off-by: Sanne Raymaekers <[email protected]>
Looks for a "?" prepended to the `composefs=` cmdline parameter. If it is prepended, it will allow a composefs image without fs-verity. Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
Allows cfsctl operations with fs-verity disabled. Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
Ideally this wouldn't be duplicated with composefs-setup-root. Signed-off-by: Sanne Raymaekers <[email protected]>
The nofsverity test needs the repart config, but not the fix-verity part. Signed-off-by: Sanne Raymaekers <[email protected]>
Signed-off-by: Sanne Raymaekers <[email protected]>
croissanne
force-pushed
the
optional-verity-v2
branch
from
1e95c9f to
752ca83
Compare
| @@ -58,6 +59,7 @@ pub struct Repository<ObjectID: FsVerityHashValue> { | |||
| repository: OwnedFd, | |||
| objects: OnceCell<OwnedFd>, | |||
| _data: std::marker::PhantomData<ObjectID>, | |||
| insecure: bool, | |||
There was a problem hiding this comment.
Probably move that above the PhantomData...
| @@ -86,6 +88,7 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> { | |||
| repository, | |||
| objects: OnceCell::new(), | |||
| _data: std::marker::PhantomData, | |||
| insecure: false, | |||
| @@ -127,7 +130,9 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> { | |||
| Ok(fd) => { | |||
| // measure the existing file to ensure that it's correct | |||
| // TODO: try to replace file if it's broken? | |||
| ensure_verity_equal(fd, &id)?; | |||
| if !self.insecure { | |||
There was a problem hiding this comment.
As a general rule I think we always want to attempt to measure fs-verity even in insecure mode, but allow that it fails with "not supported" errors.
crates/composefs/src/repository.rs
Outdated
| @@ -151,8 +156,10 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> { | |||
| )?; | |||
| drop(file); | |||
|
|
|||
| enable_verity::<ObjectID>(&ro_fd).context("Enabling verity digest")?; | |||
| ensure_verity_equal(&ro_fd, &id).context("Double-checking verity digest")?; | |||
| if !self.insecure { | |||
There was a problem hiding this comment.
I think we also want to try opportunistically to enable it...
| @@ -175,10 +182,17 @@ impl<ObjectID: FsVerityHashValue> Repository<ObjectID> { | |||
|
|
|||
| fn open_with_verity(&self, filename: &str, expected_verity: &ObjectID) -> Result<OwnedFd> { | |||
| let fd = self.openat(filename, OFlags::RDONLY)?; | |||
| ensure_verity_equal(&fd, expected_verity)?; | |||
| if !self.insecure { | |||
|
|
||
| set -eux | ||
|
|
||
| output="$1" | ||
| FIX_VERITY=0 |
There was a problem hiding this comment.
The user should be able to specify FIX_VERITY=1 from the outside...
| while [ $# -gt 0 ]; do | ||
| case "$1" in | ||
| #CMDSTART | ||
| -fv|--fix-verity) # |
There was a problem hiding this comment.
Please don't do two-character 'short' opts...
| @@ -9,7 +9,7 @@ chcon system_u:object_r:etc_t:s0 tmp/sysroot/state/*/etc/* | |||
|
|
|||
| definitions="${0%/*}/repart.d" | |||
|
|
|||
| if [ "${FIX_VERITY:-}" = '1' ]; then | |||
| if [ "$SETUP_REPART" = '1' ]; then | |||
There was a problem hiding this comment.
This variable is hilariously poorly named 🤣
There was a problem hiding this comment.
It might be interesting to make this setup a bit more 'neutral' by dynamically generating the repart.d directory in the tmp/ dir on each run using some sort of a heredoc-based approach... It's only two small files and it would be a bit cleaner than sed-hacking, I guess.
| FIX_VERITY=0 | ||
| SETUP_REPART=0 | ||
| FS_FORMAT=ext4 | ||
| parsecmd() { |
There was a problem hiding this comment.
This commandline interface thing seems a bit overkill to be honest...
It might be nicer to just be able to set a couple of extra envvars to modify the test environment, from outside:
- an envvar to disable fs-verity complete, impacting both systemd-repart and the fix-verity script
- an envvar to let choosing a different type for the rootfs (xfs, etc.)
Maybe we could avoid having a whole separate test scenario...
| ${PODMAN_BUILD} \ | ||
| --iidfile=tmp/final.iid \ | ||
| --build-context=base="container-image://${BASE_ID}" \ | ||
| --build-arg=COMPOSEFS_FSVERITY="?${BASE_IMAGE_FSVERITY}" \ |
There was a problem hiding this comment.
It seems like just about the only thing changed is that you add the ? here and the -sr below when building the image, ya? I think that's probably not worth a whole separate scenario....
There was a problem hiding this comment.
It might also be interesting to try building an image without fs-verity but with the composefs= cmdline specified without the ?, ie: to make sure that it fails as expected.
?is prepended to composefs= cmdline, set repository to insecure--insecureto cfsctl#87