store: Centralize composefs directory creation with mode 0700#2006
store: Centralize composefs directory creation with mode 0700#2006cgwalters merged 1 commit intobootc-dev:mainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses a potential security vulnerability by centralizing the creation of the composefs directory and ensuring it is created with restrictive 0700 permissions. The introduction of the ensure_composefs_dir helper function is a good refactoring that improves code clarity and maintainability by removing duplicated logic. The addition of a unit test to verify the directory permissions and the function's idempotency is also a great inclusion. Overall, this is a solid improvement to the codebase.
jeckersb
force-pushed
the
composefs-mode
branch
from
23e050f to
f732be6
Compare
The install-time composefs directory creation in repo.rs used create_dir_all() which relies on the process umask for permissions, potentially creating /sysroot/composefs with overly permissive modes and leaking information. Centralize the directory creation into a new ensure_composefs_dir() helper in store/mod.rs that explicitly sets mode 0700. Both the install-time path (repo.rs) and the runtime lazy-init path (Storage::get_ensure_composefs) now use this single helper. The helper also always updates permissions on existing directories, so systems installed with an older version of bootc will have their composefs directory permissions corrected on upgrade. Also removes #[allow(dead_code)] from COMPOSEFS_MODE since it is now actively used, and adds unit tests verifying the directory permissions, idempotency, and correction of pre-existing wrong permissions. Assisted-by: OpenCode (claude-opus-4-6) Signed-off-by: John Eckersberg <jeckersb@redhat.com>
jeckersb
force-pushed
the
composefs-mode
branch
from
f732be6 to
8b33259
Compare
| .context("Creating composefs directory")?; | ||
| // Always update permissions, in case the directory pre-existed | ||
| // with incorrect mode (e.g. from an older version of bootc). | ||
| physical_root |
There was a problem hiding this comment.
Minor nit but I'd prefer if we only called chmod() if we had work to do. It may not matter...but I think it might still dirty the inode even in the no-op case, causing write traffic.
Also important: At some point in the future we will need to properly handle "physically" readonly rootfs (think ISO), so we don't want to introduce new places where we unconditionally try writes.
There was a problem hiding this comment.
These things can be followups
| } | ||
|
|
||
| #[test] | ||
| fn test_ensure_composefs_dir_fixes_existing() -> Result<()> { |
There was a problem hiding this comment.
The unit test is fine and cheap, but I think this one also wants to be integration tested.
2 participants
The install-time composefs directory creation in repo.rs used
create_dir_all() which relies on the process umask for permissions,
potentially creating /sysroot/composefs with overly permissive modes
and leaking information.
Centralize the directory creation into a new ensure_composefs_dir()
helper in store/mod.rs that explicitly sets mode 0700. Both the
install-time path (repo.rs) and the runtime lazy-init path
(Storage::get_ensure_composefs) now use this single helper.
Also removes #[allow(dead_code)] from COMPOSEFS_MODE since it is now
actively used, and adds a unit test verifying the directory permissions
and idempotency.
Assisted-by: OpenCode (claude-opus-4-6)
Signed-off-by: John Eckersberg jeckersb@redhat.com