Fixes: #27538 copy file with archive argument

[arsenalzp]

Fixes #27538

Checklist

Ensure you have completed the following checklist for your pull request to be reviewed:

• Certify you wrote the patch or otherwise have the right to pass it on as an open-source patch by signing all
  commits. (git commit -s). (If needed, use git commit -s --amend). The author email must match
  the sign-off email address. See CONTRIBUTING.md
  for more information.
• Referenced issues using Fixes: #27538 in commit message (if applicable)
• Tests have been added/updated (or no tests are needed)
• Documentation has been updated (or no documentation changes are needed)
• All commits pass make validatepr (format/lint checks)
• Release note entered in the section below (or None if no user-facing changes)

Does this PR introduce a user-facing change?

[openshift-ci]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arsenalzp
Once this PR has been reviewed and has the lgtm label, please assign mheon for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[arsenalzp]

Hello,
As the issue #27538 is not 100% clear I would like to introduce the draft PR.
If solution I propose is OK then I will crated necessary tests.

[packit-as-a-service]

Cockpit tests failed for commit eca38f9. @martinpitt, @jelly, @mvollmer please check.

[Honny1]

I checked the code. You cannot make changes directly in the vendor directory. You need to make that change in Buildah first, and then vendor the new changes into Podman. Also, I am not sure about changing the default value of that flag.

[arsenalzp]

I checked the code. You cannot make changes directly in the vendor directory. You need to make that change in Buildah first, and then vendor the new changes into Podman. Also, I am not sure about changing the default value of that flag.

Hello,
Yes, I agree with all your statements. That's why I prepared this PR as draft one.

[Honny1]

PTAL @mheon @Luap99 @nalind

[nalind]

The godoc should describe the flag's effect, not the case it's built for. As written, it's not clear what behavior the caller should expect.

[nalind]

I don't think you need to dig into the copier package to fix this. In addition to fixing the default for the archive flag, as you're doing, the patch needs to fix the documentation for the flag, which currently suggests that enabling the flag causes ownership of content being written to the container to be set to that of the container's user, when the flag being set is supposed to cause ownership information to be preserved as it was when it was read.

It then needs to account for the fact that podman cp runs inside of a user namespace when it's started by an unprivileged user, which results in ownership IDs being mapped when it reads them from the host filesystem.

One approach: in cmd/podman/containers/cp.go's copyToContainer() function, for cases where both isStdin and chown (that's a confusing variable name) aren't set, use go.podman.io/storage/pkg/unshare.GetHostIDMappings("") to read the mappings for the user namespace that podman is currently in, use github.com/containers/podman/v6/pkg/util.RuntimeSpecToIDtools() to convert the returned mapping slices to the right types for setting in the UIDMap and GIDMap fields in getOptions, and in one or the other, swap the ContainerID and HostID values for each element in the mapping slices.

copier.Get() attempts to map from the "host" ID ranges to "container" ID ranges when it processes content, and swapping the values in the slices will cause it to map ownership information as it appears in the user namespace that podman is in to the correct values before passing the content on to wherever it's going.

[arsenalzp]

I don't think you need to dig into the copier package to fix this. In addition to fixing the default for the archive flag, as you're doing, the patch needs to fix the documentation for the flag, which currently suggests that enabling the flag causes ownership of content being written to the container to be set to that of the container's user, when the flag being set is supposed to cause ownership information to be preserved as it was when it was read.

It then needs to account for the fact that podman cp runs inside of a user namespace when it's started by an unprivileged user, which results in ownership IDs being mapped when it reads them from the host filesystem.

One approach: in cmd/podman/containers/cp.go's copyToContainer() function, for cases where both isStdin and chown (that's a confusing variable name) aren't set, use go.podman.io/storage/pkg/unshare.GetHostIDMappings("") to read the mappings for the user namespace that podman is currently in, use github.com/containers/podman/v6/pkg/util.RuntimeSpecToIDtools() to convert the returned mapping slices to the right types for setting in the UIDMap and GIDMap fields in getOptions, and in one or the other, swap the ContainerID and HostID values for each element in the mapping slices.

copier.Get() attempts to map from the "host" ID ranges to "container" ID ranges when it processes content, and swapping the values in the slices will cause it to map ownership information as it appears in the user namespace that podman is in to the correct values before passing the content on to wherever it's going.

Hello,
The default behavior of archive is the one issue. It cat be fixed easy.
However, the main issue is that podman doesn't set correct UID/GID values for user-less container with rootful podman: it always sets 0/0 (root/root) values with or without archive option. In turn, in this case docker set UID/GID of file owner.
If don't want to follow docker behavior then I can leave it as is and keep the fix of archive option only.

@Luap99 mentioned here that the docker API compatibility is important, so I was trying to follow that note.