projectatomic / docker

Docker - the open-source application container engine
http://www.docker.com
Apache License 2.0
81 stars 58 forks source link

Fix docker cp issues BZ1723491 & BZ1717087 #348

Closed TomSweeneyRedHat closed 5 years ago

TomSweeneyRedHat commented 5 years ago

Signed-off-by: TomSweeneyRedHat tsweeney@redhat.com

- What I did There are two BZs https://bugzilla.redhat.com/show_bug.cgi?id=1723491 and https://bugzilla.redhat.com/show_bug.cgi?id=1717087 reporting an issue using docker cp to a container. Upon testing, further issues were discovered if the target in the container was a symlink. This code finds the target directory on the host's local container storage (/var/lib/docker/overlay2), resolves any symlinks and ensures the file gets copied to/from the container without any potential cve leakage.

- How I did it VI and lots of blood, sweat and tears for this one.

- How to verify it

# docker run -t -i --name testctr --rm fedora bash

//Inside the container:
# ln -s /tmp /test

//Outside the container:
# touch testfile; docker cp testfile testctr:/test

//In the container,
# ls /tmp

// Outside the container,
# ls /tmp

//The file "testfile" should be shown in the ls within the container, but NOT with the ls outside the container.

# docker cp testctr:/test/testfile.txt  /tmp/newtestfile.txt
// /tmp on the host should have the file newtestfile.txt

- Description for the changelog Addresses docker cp issue noted in BZ1717087 and BZ1723491

- A picture of a cute animal (not mandatory but encouraged)

TomSweeneyRedHat commented 5 years ago

This is a WIP until I can have @edsantiago run tests on it. Please review prior, I believe it's gtg.

TomSweeneyRedHat commented 5 years ago

Follow on PR for #341 #342 and completes #347

rhatdan commented 5 years ago

@TomSweeneyRedHat I am confused by this, is this ready Now?

TomSweeneyRedHat commented 5 years ago

@rhatdan yes, ready to go given LGTM's which I don't have any atm.

giuseppe commented 5 years ago

LGTM

giuseppe commented 5 years ago

this was fixed by another PR. Let's close it

rhatdan commented 5 years ago

@giuseppe So should we close this PR?

TomSweeneyRedHat commented 5 years ago

I think #353 by @giuseppe takes care of this. @giuseppe can you confirm?

TomSweeneyRedHat commented 5 years ago

I've looked through #353 from @giuseppe and have tested with it. It does appear to replace this one and solves a wart that this PR had in a specific pull sequence. I'm going to close this and we can resurrect it if we find out it's not necessary.