Preserve file mode when copying files out of a pod

[evanphx]

What type of PR is this?
/kind bug

What this PR does / why we need it:
This fixes the ability to inherit the file modes from the files within the pod that are copied. Most importantly, it makes sure that the execute bits are set properly and generally makes this code match what tar would do (namely, using a readonly mode if the source was readonly)

Which issue(s) this PR fixes:
Didn't open an issue, just fixed the problem directly.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

File modes are properly maintained when copying data out of pods with `kubectl cp`

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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/test-infra repository. I understand the commands that are listed here.

[k8s-ci-robot]

Hi @evanphx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: evanphx
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: adohe

If they are not already assigned, you can assign the PR to them by writing /assign @adohe in a comment when ready.

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/kubectl/OWNERS

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

[evanphx]

signed the CLA

[neolit123]

thank you for the PR @evanphx
this change needs approval by SIG CLI.

/ok-to-test
/priority backlog

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[evanphx]

/remove-lifecycle stable

[evanphx]

@neolit123 Any thoughts on this?

[neolit123]

@neolit123 Any thoughts on this?

try mentioning this PR in the slack channels #sig-cli or #pr-reviews

[liggitt]

@soltysh @tallclair note that this will allow creating executable files via kubectl cp. What kubectl cp currently does is not correct, but we've had enough issues with cp that I'm really wary of expanding its potential as an attack vector

[evanphx]

I added this actually because I wanted to preserve the executable bits.

[evanphx]

What should kubectl cp do? I can add a flag, ala tar, to preserve executable bits.

[evanphx]

I'm happy to change this however y'all think is best, please let me know.

[soggiest]

Hello! Code Freeze is just about on us. We'll be entering into Freeze tomorrow, May 31st. Is this still planned to land in 1.15?

[apelisse]

What should kubectl cp do? I can add a flag, ala tar, to preserve executable bits.

I think I'm fine with that, what do you think @liggitt @soltysh

[soggiest]

/milestone v1.16

[liggitt]

given the repeated past vulnerabilities in kubectl cp where files from malicious tar files sent by containers could escape the target directory, I don't think making files executable is a good idea.

@kubernetes/sig-cli-maintainers should consider one or more of the following options:

• moving kubectl cp to an optional plugin (reduces attack surface for users that don't care about it, allows independent release of the command)
• rewriting kubectl cp to use tar directly instead of a custom tar file extractor. most tar implementations are hardened to avoid extracting files outside the target directory (via absolute paths, or relative backsteps, or symlinks)
• deprecating/removing kubectl cp in favor of direct use of kubectl exec and tar

[xmudrii]

@evanphx @liggitt Hello! I'm bug triage lead for the 1.16 release cycle and considering this PR has not been updated for a long time, I'd like to check what's the status of this PR. The code freeze is starting 29th August (about 2.5 weeks from now) and while there is plenty of time, we want to ensure that each PR has a chance to be merged on time.

As the PR is tagged for 1.16, is it still planned for this release?

[evanphx]

@liggitt Perhaps since you're concerned about setting executable, exposing it as a configuration option to honor the executable bit? Happy to add that to this PR.

[liggitt]

the history of kubectl cp with respect to secure extraction of files is not good (c.f. CVE-2018-1002100, CVE-2019-1002101, CVE-2019-11246, CVE-2019-11249)

even if the option is flag-gated, the capability would still have escalated the severity of those issues significantly if the placed files could be made executable.

I'd prefer we document how to use kubectl exec in combination with tar to accomplish advanced use cases like this. @tallclair looked into that, I believe, and might be able to shepherd some docs changes to that effect.

[tallclair]

The basic command to copy from a pod using exec & tar is:

kubectl exec ${POD} -- tar cf - ${FILE} | tar xf -

Of course you can use any typical arguments to tar, in this case you might be interested in --preserve --same-owner.

More examples here: https://gist.github.com/tallclair/9217e2694b5fdf27b55d6bd1fda01b53

[soltysh]

I agree with @liggitt here, we (as in sig-cli) need to reconsider kubectl cp implementation. The current causes more harm than good to both users and maintainers. I don't think that moving this to an optional plugin is a good approach, I'd prefer the rewrite option. I'll rise this problem during next sig-cli call (Wed, Aug 28th).

[liggitt]

Discussed in sig-cli this week. The immediate priority is to resolve symlink handling issues in #82143 before considering expanding the surface area to include things like wildcard support and executable-permission preservation.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[k8s-ci-robot]

@evanphx: PR needs rebase.

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/test-infra repository.

[evanphx]

I can rebase this but it sounded like before I should not bother because cp is going to be removed?

…

On Fri, Dec 27, 2019 at 7:43 PM Kubernetes Prow Robot < ***@***.***> wrote: @evanphx <https://github.com/evanphx>: PR needs rebase. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#73053?email_source=notifications&email_token=AAAAAB7RS4XNIA32UM6VM23Q224OPA5CNFSM4GQ32JH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYALPQ#issuecomment-569378238>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAABZ3P2OWFDE3AEPYVRTQ224OPANCNFSM4GQ32JHQ> .

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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/test-infra repository.

[atomd]

tx