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

feature: add pouch cp cmd #2879

Merged
merged 1 commit into from
Jun 3, 2019

Conversation

ZYecho
Copy link
Contributor

@ZYecho ZYecho commented May 30, 2019

Signed-off-by: zhangyue zy675793960@yeah.net

Ⅰ. Describe what this PR did

add pouch cp cmd.
take part of #2182

Ⅱ. Does this pull request fix one issue?

fix #2609

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗?)

add later

Ⅳ. Describe how to verify it

add later

Ⅴ. Special notes for reviews

didn't support follow-link and copy uid/gid maps now.

@codecov
Copy link

codecov bot commented May 30, 2019

Codecov Report

Merging #2879 into master will decrease coverage by 0.31%.
The diff coverage is 52.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2879      +/-   ##
==========================================
- Coverage   69.08%   68.77%   -0.32%     
==========================================
  Files         287      291       +4     
  Lines       17947    18230     +283     
==========================================
+ Hits        12399    12538     +139     
- Misses       4128     4232     +104     
- Partials     1420     1460      +40
Flag Coverage Δ
#criv1alpha2_test 38.4% <6.06%> (-0.56%) ⬇️
#integration_test_0 36.15% <6.06%> (-0.42%) ⬇️
#integration_test_1 35.69% <51.51%> (+0.29%) ⬆️
#integration_test_2 36.01% <6.06%> (-0.52%) ⬇️
#integration_test_3 35.64% <39.39%> (+0.06%) ⬆️
#node_e2e_test 34.07% <6.06%> (-0.6%) ⬇️
#unittest 28.06% <0.34%> (-0.51%) ⬇️
Impacted Files Coverage Δ
daemon/mgr/container.go 59.93% <ø> (+0.2%) ⬆️
client/container_copy.go 0% <0%> (ø)
apis/server/router.go 91.51% <100%> (+0.15%) ⬆️
pkg/ioutils/reader.go 100% <100%> (ø)
client/request.go 52.12% <20%> (-1.81%) ⬇️
daemon/mgr/container_copy.go 60% <60%> (ø)
daemon/mgr/container_storage.go 60.46% <62.16%> (+0.22%) ⬆️
apis/server/copy_bridge.go 63.26% <63.26%> (ø)
ctrd/container.go 51.62% <0%> (-1.92%) ⬇️
cri/v1alpha2/cri.go 68.3% <0%> (-0.76%) ⬇️
... and 7 more

if err != nil {
return nil, err
}
defer mgr.Unmount(ctx, c)
Copy link
Contributor

@CodeJuan CodeJuan May 30, 2019

Choose a reason for hiding this comment

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

Hi Yue,
Would you please take a look at moby/moby#39252 and (kubernetes/kubernetes#75037) at your convenience?

cp command are vulnerable to a symlink-exchange attack with Directory Traversal, giving attackers arbitrary read-write access to the host filesystem with root privileges.

The kubectl cp command allows copying files between containers and the user machine. To copy files from a container, Kubernetes creates a tar inside the container, copies it over the network, and kubectl unpacks it on the user’s machine.
If the tar binary in the container is malicious, it could run any code and output unexpected, malicious results. An attacker could use this to write files to any path on the user’s machine when kubectl cp is called, limited only by the system permissions of the local user.

Copy link
Contributor Author

@ZYecho ZYecho May 30, 2019

Choose a reason for hiding this comment

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

I think the latter one we don't have risk, since we don't support follow link now. But the former one issue in moby we have same risk. invite @allencloud @fuweid to see.

Copy link
Contributor

@CodeJuan CodeJuan May 31, 2019

Choose a reason for hiding this comment

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

I'm not sure it's necessary to do filesystem operations(mount/umount) in pouch cp, I was wondering if it's possible to implement cp by using tar/untar like kubectl cp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodeJuan What kind of file operations you mean? seems there's a patch fix on chrootarchive package #39292

Copy link

@markzhang0928 markzhang0928 Jun 1, 2019

Choose a reason for hiding this comment

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

kubectl cp doesn't work well when the container is in distroless mode, it also may frustrate by some malicious image with bad tar binary @CodeJuan

Actually, the solution of this issue #39292 might solve the problem of TOCTTOU attack, and I am really excited about the newCommand docker-tar which is chroot tar. Whether it couldn't be merged asap due to it changes the core piece of docker ?@ZYecho

Is there another method to fix the problem with cp? how about using another ephemeral container to enter the namespace of target container ,and exchange data with it?

@ZYecho ZYecho changed the title [WIP]feature: add pouch cp daemon side feature: add pouch cp daemon side May 31, 2019
@ZYecho ZYecho changed the title feature: add pouch cp daemon side feature: add pouch cp cmd May 31, 2019
apis/server/copy_bridge.go Outdated Show resolved Hide resolved
cli/cp.go Outdated Show resolved Hide resolved
client/container_copy.go Show resolved Hide resolved
client/request.go Outdated Show resolved Hide resolved
daemon/mgr/container_copy.go Outdated Show resolved Hide resolved
daemon/mgr/container_copy.go Outdated Show resolved Hide resolved
daemon/mgr/container_storage.go Outdated Show resolved Hide resolved
daemon/mgr/container_storage.go Outdated Show resolved Hide resolved
apis/server/copy_bridge.go Outdated Show resolved Hide resolved
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

  1. stopped container: cp doesn't work
  2. stopped container -> cp action -> restart: containerd will raise error like
root@ubuntu-xenial ~/g/s/g/a/pouch# pouch start 986486                                                                                                                                                                              5c253f9!?
Error: failed to start containers: {"message":"failed to create container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3) on containerd: failed to create task for container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3): mkdir /var/lib/pouch/containerd/state/io.containerd.runtime.v1.linux/default/986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3: file exists: unknown"}

It seems that cleanup action is not right.

@ZYecho
Copy link
Contributor Author

ZYecho commented Jun 2, 2019

  1. stopped container: cp doesn't work
  2. stopped container -> cp action -> restart: containerd will raise error like
root@ubuntu-xenial ~/g/s/g/a/pouch# pouch start 986486                                                                                                                                                                              5c253f9!?
Error: failed to start containers: {"message":"failed to create container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3) on containerd: failed to create task for container(986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3): mkdir /var/lib/pouch/containerd/state/io.containerd.runtime.v1.linux/default/986486d7c6b6a64a987d6083192dc6e081fba9e504cf740ff28f9fd2e8e7beb3: file exists: unknown"}

It seems that cleanup action is not right.

  1. fix it by adding self define closer func for read operation, in closer, we unmount and detach if need.
  2. fix it by changing logic when do clean up.

@fuweid PTAL.

apis/server/copy_bridge.go Outdated Show resolved Hide resolved
daemon/mgr/container_copy.go Outdated Show resolved Hide resolved
daemon/mgr/container_copy.go Outdated Show resolved Hide resolved
@ZYecho ZYecho force-pushed the add-copy-daemon branch 2 times, most recently from 985ac07 to 73456e5 Compare June 2, 2019 13:54
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: zhangyue <zy675793960@yeah.net>
@allencloud
Copy link
Collaborator

Merging...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] pouch cp is needed
7 participants