Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop glob'ing on podman cp #3942

Merged
merged 1 commit into from
Sep 13, 2019

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Sep 4, 2019

  • Do not support wildcards
  • Restore pause=true to avoid race conditions. User can still do --pause=false to avoid container pause.

Signed-off-by: Jhon Honce [email protected]

@jwhonce jwhonce requested a review from mheon September 4, 2019 22:07
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jwhonce

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:

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

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L labels Sep 4, 2019
cmd/podman/cp.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Removing the globs is fine, but I can’t see how the rest of the PR is secure; it removes a large part of the existing protections without any replacement that I can see.

If it is doing something non-obvious, please write an explicit argument (that someone unfamiliar with the code can follow, like a future maintainer) for why it is secure (what are the security objectives, how are those goals upheld; maybe what are the attacks and how they are prevented) — ideally as comments directly in the code to decrease the risk that future maintainers will undo/break that without noticing.

cmd/podman/cp.go Outdated Show resolved Hide resolved
cmd/podman/cp.go Outdated Show resolved Hide resolved
cmd/podman/cp.go Outdated Show resolved Hide resolved
@rhatdan
Copy link
Member

rhatdan commented Sep 9, 2019

BTW, I think we should go back to default to pausing the container. Then if someone wants to have better performance is willing to take on the risk, they can run it with the --pause=false flag.

@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2019

Lets get the default changed to default to Pause and get the temporary patch fixed. And then open a card to do a full test.

@jwhonce jwhonce self-assigned this Sep 12, 2019
@jwhonce jwhonce changed the title Correct source path processing on podman cp Stop glob'ing on podman cp Sep 12, 2019
@jwhonce jwhonce force-pushed the issue/3829 branch 3 times, most recently from 7444102 to db04149 Compare September 12, 2019 22:12
* symlink processing and wildcarding led to unexpected files
  being copied

Signed-off-by: Jhon Honce <[email protected]>
@TomSweeneyRedHat
Copy link
Member

LGTM assuming happy tests

@rhatdan
Copy link
Member

rhatdan commented Sep 13, 2019

One issue with the pause is this will fail on rootless containers not running with cgroups V2, but I still think it is a good default.

@rhatdan
Copy link
Member

rhatdan commented Sep 13, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 13, 2019
@openshift-merge-robot openshift-merge-robot merged commit 5c09c4d into containers:master Sep 13, 2019
@mheon
Copy link
Member

mheon commented Sep 13, 2019

We can't break cp for rootless containers by default - need to verify it still works.

@haircommander
Copy link
Collaborator

haircommander commented Oct 2, 2019

/cherry-pick v1.4.2-stable

@haircommander
Copy link
Collaborator

/cherrypick v1.4.2-stable

@jwhonce jwhonce deleted the issue/3829 branch June 30, 2021 16:11
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants