fix: if not running as root, don't attempt chown#30
fix: if not running as root, don't attempt chown#30dotconfig404 wants to merge 1 commit intoOpenVoxProject:mainfrom
Conversation
|
|
||
| # Ensure the symlink has the same ownership as the source when running | ||
| # with privileges to change ownership. | ||
| if instance.running_as_root? |
There was a problem hiding this comment.
Is this really only called when using the CLI? I didn't dig through the code yet, but I thought starting the openvoxserver also creates the symlink. And the process starts as user puppet, not root. But maybe I am wrong, the the symlink is created by different code.
There was a problem hiding this comment.
Hi, yes, this is CLI-only.
The server also creates a symlink during ca/initialize!, but the server process runs as the puppet user (though it is started as root), so that code path is effectively a no-op in practice. IIRC I had some initialization issues there before, but that’s a separate issue.
While reviewing #32, I initially wondered why the additional FileUtils.chown call sites it guards didn’t fail in my testing. This was because find_user_and_group already falls back to Process.euid/egid when not running as root, so @user/@group effectively encode "safe to chown". That is, if not running as root using @user/@group already points to the UID and GID of the process -> no-op.
forcibly_symlink on the other hand uses source_info.uid/gid directly instead of @user/@group, so it can attempt to chown to a different user and trigger EPERM in rootless containers.
You could make all call sites consistent by switching forcibly_symlink to use @user/@group, but that still leaves find_user_and_group encoding two concerns: "intended owner" and "safe to chown".
That’s why I went with a small refactor instead: make the "should we chown at all?" decision explicit via a helper and apply it consistently. See: #33
Guard all three FileUtils.chown call sites in file_system.rb with the existing running_as_root? check so that openvoxserver-ca no longer crashes when running inside rootless containers (e.g. podman rootless, OpenShift with arbitrary UIDs) where the process lacks CAP_CHOWN. Affected methods: forcibly_symlink, write_file, ensure_dir. In these environments file ownership is typically managed through SGID bits and g=u permission patterns instead of explicit chown calls. Inspired by the approach in OpenVoxProject#30. Signed-off-by: Simon Lauger <simon@lauger.de>
|
Thanks for the fix and the idea with the running_as_root? guard - that's a clean approach and consistent with how the class already handles the root check in find_user_and_group. I noticed the fix only covers forcibly_symlink, but there are two more FileUtils.chown calls in the same file that also fail in rootless containers. I liked the approach and adopted it for all three call sites in #32 (plus added specs). |
Guard all three FileUtils.chown call sites in file_system.rb with the existing running_as_root? check so that openvoxserver-ca no longer crashes when running inside rootless containers (e.g. podman rootless, OpenShift with arbitrary UIDs) where the process lacks CAP_CHOWN. Affected methods: forcibly_symlink, write_file, ensure_dir. In these environments file ownership is typically managed through SGID bits and g=u permission patterns instead of explicit chown calls. Inspired by the approach in OpenVoxProject#30. Signed-off-by: Simon Lauger <simon@lauger.de>
3 participants
Hi!
puppetserver ca setup/puppetserver ca importcurrently fail in non-root/containerized environments when CA directory compatibility symlinks are created.Puppetserver::Ca::Utils::FileSystem.forcibly_symlinkalways callsFileUtils.chown(...)on the symlink target after creating it.When running without
CAP_CHOWN(for example, arbitrary UID containers), this raisesErrno::EPERMand aborts the command.This PR should ensure that ownership is only harmonized when running as root.
Added unit tests for
Puppetserver::Ca::Utils::FileSystem.forcibly_symlink:FileUtils.chownFileUtils.chownwith source uid/gidAlso verified related setup/import specs still pass.
Thanks!
PS: This PR will remove the need for this