Skip to content

fix: skip chown in rootless containers via ensure_ownership helper#33

Open
dotconfig404 wants to merge 1 commit intoOpenVoxProject:mainfrom
dotconfig404:fix/chown-in-rootless-containers
Open

fix: skip chown in rootless containers via ensure_ownership helper#33
dotconfig404 wants to merge 1 commit intoOpenVoxProject:mainfrom
dotconfig404:fix/chown-in-rootless-containers

Conversation

@dotconfig404
Copy link
Copy Markdown

Alternative to #30 and #32 that fixes the same rootless-container issue with a small refactor.

In rootless containers, openvoxserver-ca may run without CAP_CHOWN. In that case, the FileUtils.chown calls in file_system.rb can raise Errno::EPERM, aborting commands such as puppetserver ca setup or puppetserver ca import.

This PR consolidates ownership changes into a single helper: FileSystem.ensure_ownership(path). The helper only performs chown when running as root; otherwise it is a no-op. When running as non-root, the current process has created the path, so ownership is already correct.

Note that find_user_and_group already falls back to the current user when not running as root, so existing call sites effectively perform a no-op chown in that case. forcibly_symlink bypasses this and uses the source UID/GID directly, which is what triggers EPERM in rootless environments.

This PR makes the “should we chown at all?” decision explicit and consistent across all call sites.

Compared with #30 and #32, this keeps the same behavioral fix but centralizes the logic in one place, making it easier to reason about.

This preserves existing behavior when running as root.

Changes

  • remove singleton state from file_system.rb
  • remove find_user_and_group / @user / @group
  • introduce ensure_ownership(path)
  • convert remaining methods to class methods
  • add focused specs for ensure_ownership

end
end
end
end No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the missing newline

@bastelfreak
Copy link
Copy Markdown
Contributor

@dotconfig404 thanks for the PR! Please see the failing the DCO check: https://github.com/OpenVoxProject/openvoxserver-ca/pull/33/checks

@dotconfig404 dotconfig404 force-pushed the fix/chown-in-rootless-containers branch from 921d584 to 95b4b5a Compare April 13, 2026 14:41
Signed-off-by: dotconfig <dotconfig@krutt.org>
@dotconfig404 dotconfig404 force-pushed the fix/chown-in-rootless-containers branch from 95b4b5a to 016db7f Compare April 13, 2026 14:44
@slauger
Copy link
Copy Markdown
Member

slauger commented Apr 13, 2026

Centralizing the ownership logic in ensure_ownership is a much better approach than guarding each call site individually (like #30 and #32 do). The removal of the singleton state and find_user_and_group makes the class easier to reason about.

I'll close #32 in favor of this. LGTM.

@slauger
Copy link
Copy Markdown
Member

slauger commented Apr 13, 2026

@bastelfreak Could you approve the GitHub Actions workflow for this PR?

@bastelfreak
Copy link
Copy Markdown
Contributor

@slauger @sebastianrakel @rwaffen did one of you had a chance to test this?

slauger added a commit to slauger/openvox-operator that referenced this pull request Apr 20, 2026
Replace our sed-based patches for rootless chown issues with the
upstream file_system.rb from OpenVoxProject/openvoxserver-ca#33.

The upstream PR introduces an ensure_ownership helper that skips chown
when not running as root, which covers both the file_system.rb chown
calls and the forcibly_symlink used by symlink_to_old_cadir in setup.rb.

This eliminates the need for both of our previous sed patches.

Signed-off-by: Simon Lauger <simon@lauger.de>
slauger added a commit to slauger/openvox-operator that referenced this pull request Apr 20, 2026
Replace our sed-based patches for rootless chown issues with the
upstream file_system.rb from OpenVoxProject/openvoxserver-ca#33.

The upstream PR introduces an ensure_ownership helper that skips chown
when not running as root, which covers both the file_system.rb chown
calls and the forcibly_symlink used by symlink_to_old_cadir in setup.rb.

This eliminates the need for both of our previous sed patches.

Signed-off-by: Simon Lauger <simon@lauger.de>
@slauger
Copy link
Copy Markdown
Member

slauger commented Apr 20, 2026

I tested this against my operator image build. It replaces the sed-based workarounds. The image build and a puppetserver ca setup both worked fine in a rootless container. LGTM from my side.

slauger added a commit to slauger/openvox-operator that referenced this pull request Apr 20, 2026
Replace our sed-based patches for rootless chown issues with the
upstream file_system.rb from OpenVoxProject/openvoxserver-ca#33.

The upstream PR introduces an ensure_ownership helper that skips chown
when not running as root, which covers both the file_system.rb chown
calls and the forcibly_symlink used by symlink_to_old_cadir in setup.rb.

This eliminates the need for both of our previous sed patches.

Signed-off-by: Simon Lauger <simon@lauger.de>
slauger added a commit to slauger/openvox-operator that referenced this pull request Apr 20, 2026
Replace our sed-based patches for rootless chown issues with the
upstream file_system.rb from OpenVoxProject/openvoxserver-ca#33.

The upstream PR introduces an ensure_ownership helper that skips chown
when not running as root, which covers both the file_system.rb chown
calls and the forcibly_symlink used by symlink_to_old_cadir in setup.rb.

This eliminates the need for both of our previous sed patches.

Signed-off-by: Simon Lauger <simon@lauger.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants