install: Read --root-ssh-authorized-keys before changing mounts - #2476
ericcurtin wants to merge 1 commit into
Conversation
Johan-Liebert1
left a comment
Johan-Liebert1
left a comment
There was a problem hiding this comment.
Logic looks good to me. Could you please sign off your commit as we require a Signed-off-by: <email> line at the end of commit messages
| }); | ||
| } | ||
| let contents = std::fs::read_to_string(p).with_context(|| format!("Reading {p}"))?; | ||
| bootc_utils::reexec::set_reexec_env(ROOT_SSH_AUTHORIZED_KEYS_ENV, &contents); |
There was a problem hiding this comment.
We should make sure it's UTF-8 compatible here itself
There was a problem hiding this comment.
Switched to std::env::var(), so the value is a String (UTF-8 enforced by the type) and a non-UTF-8 value is a proper error. The value we write is also a String from read_to_string, so it is UTF-8 by construction.
|
|
||
| /// Additional environment variables to pass along when we re-execute ourself; | ||
| /// see [`set_reexec_env`]. | ||
| static REEXEC_ENV: Mutex<Vec<(OsString, OsString)>> = Mutex::new(Vec::new()); |
There was a problem hiding this comment.
We only ever call this from one thread so I'm sure if the mutex is required. Also, not a fan of this being a global var. We should only ever re-exec when we are installing, so putting this in prepare_install would make sense and passing in the vector of env vars to reexec_with_guardenv as an extra_args param?
There was a problem hiding this comment.
We need a mutex or equiv around any static, no problem with that from my PoV.
Also, not a fan of this being a global var.
Yes, but doing it differently would require threading this state from the install code into the lsm code...doable but ugly in a different way.
BTW I would generalize this slightly and e.g.:
- Define a struct we can serialize to JSON of stuff we need to save between re-exec
- In the install path, gather that state before we re-exec
- Serialize it to a memfd
- Set an env var
_BOOTC_INSTALL_REEXEC_STATE - Deserialize it early in the install path
There was a problem hiding this comment.
Kept the global per Colin (threading it from install into lsm was the alternative), but folded the env application together with the argv/argv0 setup into a single prepare_reexec() used by both re-exec sites, so the duplication there is gone too, plus a unit test.
On the memfd/JSON generalization: set_reexec_env is already generic over key/value, and the only payload today is a small text file, so I left the env approach for now. Happy to move to a memfd-backed struct if we grow more state to carry across re-exec (or if key files near the 128KiB per-env-string limit turn out to be a real concern).
ede527b to
189ab25
Compare
| // TODO use https://github.com/ostreedev/ostree-rs-ext/pull/493/commits/afc1837ff383681b947de30c0cefc70080a4f87a | ||
| const BASE_IMAGE_PREFIX: &str = "ostree/container/baseimage/bootc"; | ||
|
|
||
| // Match podman's default registry retry policy. A failed attempt has to rebuild |
There was a problem hiding this comment.
It looks like you reverted other changes
There was a problem hiding this comment.
Sorry about that, a bad rebase on my side (the squash was staged against a stale main). Fixed in 8c0cbb8: the diff against main is back to just the 5 intended files (Justfile, install.rs, lsm.rs, tests-integration/install.rs, reexec.rs).
c53e709 to
8c0cbb8
Compare
prepare_install() mounts a tmpfs over /tmp and mirrors the host's /var/tmp before reading the --root-ssh-authorized-keys file, so a file bind mounted into the install container under /tmp was hidden and the install failed with "No such file or directory" even though the file was visible in the container. Read the file before touching mounts. Since we may re-exec afterwards (unshare, SELinux install_t) and run prepare_install() again with the mounts in place, carry the content to the child via the environment using a small bootc_utils::reexec helper shared by both re-exec sites. Mount the key file under /tmp in the integration test to cover this. Also make `just package` pull the base image with retries; the CI package job failed on a transient quay.io EOF during `podman build`. Generated-by: AI Signed-off-by: Eric Curtin <eric.curtin@docker.com>
8c0cbb8 to
d34b57b
Compare
|
The three |
This branch has not been deployed
3 participants
Problem
prepare_install()mounts a tmpfs over/tmpand mirrors the host's/var/tmp, and only afterwards reads the--root-ssh-authorized-keysfile. A file bind mounted into the install container under/tmpis hidden by then:The file is visible via
podman run ... cat /tmp/authorized_keys, so this is confusing.Change
prepare_install()may re-exec (unshare, SELinuxinstall_t) and run again with the tmpfs in place, so carry the content to the child via the environment with a smallbootc_utils::reexechelper used by both re-exec sites. This avoids mutating our own environment, which is not thread safe./tmpto cover this.just packagepulls the base image with retries (fedora-46 CI job failed on a quay.io EOF).Testing
Fedora 44 aarch64, SELinux enforcing (exercises the
install_tre-exec), key file bind mounted at/tmp/authorized_keys:install to-disk --via-loopback ... --root-ssh-authorized-keys /tmp/authorized_keysfails as above./etc/tmpfiles.d/bootc-root-ssh.confcontains the keys.cargo test -p bootc-lib --lib,cargo test -p bootc-internal-utils --lib,cargo fmt --check,make validateclippy/rustdoc pass.Generated-by: AI
I hit this on real hardware, reviewed the change and tested it end to end as above.