Fixing ‘docker cp’ to allow new target file name in a host symlinked directory #31993
Conversation
|
ping @jlhawn ptal |
| @@ -218,11 +252,11 @@ func CopyInfoDestinationPath(path string) (info CopyInfo, err error) { | |||
| // Ensure destination parent dir exists. | |||
| dstParent, _ := SplitPathDirEntry(path) | |||
|
|
|||
| parentDirStat, err := os.Lstat(dstParent) | |||
There was a problem hiding this comment.
I might be forgetting something, but wouldn't simply changing this from os.Lstat to os.Stat fix this issue? os.Stat(name) will follow symlinks and you'll get the os.Fileinfo describing the target.
Not only would this be a much shorter change but it also covers the case where dstParent is a symlink to another symlink (to possibly yet another symlink) that finally points to a directory. That is a test case that is not currently covered but looks like would still fail under this current fix.
There was a problem hiding this comment.
@jlhawn thanks for the suggestion! You are correct, os.Stat follows the link. I updated the PR with the changes and added a test case for symlink -> symlink.
dccurtis
force-pushed
the
issue-31816
branch
3 times, most recently
from
e90a301 to
61181be
Compare
|
Is there anything I should be doing to resolve the windowsRS1 failure? It looks like an intermitent issue related to a PrepareLayer timeout. |
|
@thaJeztah Is there anything left for me to do to get this change merged? This is my first contribution and I just want to make sure I didn't miss anything. Thanks! |
|
ping @tonistiigi PTAL |
|
This looks fine to me. AFAICT this is only used by the docker CLI for I wonder if it makes sense to just move this over to the cli repo since this function is only using exported functions/types anyway. @tonistiigi I see you self-assigned this. Does this LGTY? LGTM |
|
This does need a rebase, and I think the new tests will likely not work since it would require updating the CLI commit. |
|
I'm a new contributor and still getting used to the processes. Should I rebase my branch and fix tests as needed? |
|
Unfortunately rebasing will not be sufficient as first the archive package
would need to be updated here.
Then the CLI updated, then have the CLI commit updated here... but we
aren't updating the CLI commit anymore (it's frozen and should not be
updated).
The functionality is in the docker CLI and can't really be tested here.
Also I think why I think it might make sense to move this function out of
archive and into the CLI repo...
But in any case, regardless of where this function lives, it can be tested
with a unit test.
…On Mon, Jun 19, 2017 at 8:42 AM, Douglas Curtis ***@***.***> wrote:
I'm a new contributor and still getting used to the processes. Should I
rebase my branch and fix tests as needed?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#31993 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAwxZhZicV-bPv7Inv-SO3atripjiiNXks5sFpb8gaJpZM4MkrSZ>
.
--
- Brian Goff
|
|
ping @dccurtis do you have time to work on this? |
|
Discussing with @tonistiigi, and he wants to have another look at this to check if there's no possible security issue with symlinks being followed where it's not desired. |
|
@thaJeztah Yes, let me know what I can do to help. A brief situational update would help me to understand what the challenges are for getting this change merged (i.e. architecture and release issues, security, testing, etc). |
GordonTheTurtle
added
the
dco/no
|
Please sign your commits following these rules: $ git clone -b "issue-31816" git@github.com:dccurtis/docker.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842353533496
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
GordonTheTurtle
removed
the
dco/no
|
I started to re-base my branch and think I’m hitting the issue that @cpuguy83 was referring to. Commenting out my tests until I figure out how to handle this. |
|
I don’t have a lot of time to work on this but definitely want to see it through to the end. Is there a desired date to have the code change finalized by? |
| // Verify that newfile2 has the contents of file3 in the real directory (dir4) | ||
| c.Assert(fileContentEquals(c, cpPath(tmpDir, "symlinkToDir4", "newname2"), "file3\n"), checker.IsNil) | ||
| */ | ||
|
|
There was a problem hiding this comment.
Perhaps a unit test for CopyInfoDestinationPath would be enough? A quick draft;
From 5bec8cc8b4f1b89a1326d56d3040da54abf6e604 Mon Sep 17 00:00:00 2001
From: Sebastiaan van Stijn <github@gone.nl>
Date: Tue, 8 Aug 2017 17:20:53 +0200
Subject: [PATCH] Add unit tests
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
---
pkg/archive/changes_test.go | 54 +++++++++++++++++++++++--------------------
pkg/archive/copy_unix_test.go | 27 ++++++++++++++++++++++
2 files changed, 56 insertions(+), 25 deletions(-)
diff --git a/pkg/archive/changes_test.go b/pkg/archive/changes_test.go
index c5d1629e7..25294652b 100644
--- a/pkg/archive/changes_test.go
+++ b/pkg/archive/changes_test.go
@@ -45,36 +45,40 @@ type FileData struct {
path string
contents string
permissions os.FileMode
+ expected CopyInfo
}
func createSampleDir(t *testing.T, root string) {
files := []FileData{
- {Regular, "file1", "file1\n", 0600},
- {Regular, "file2", "file2\n", 0666},
- {Regular, "file3", "file3\n", 0404},
- {Regular, "file4", "file4\n", 0600},
- {Regular, "file5", "file5\n", 0600},
- {Regular, "file6", "file6\n", 0600},
- {Regular, "file7", "file7\n", 0600},
- {Dir, "dir1", "", 0740},
- {Regular, "dir1/file1-1", "file1-1\n", 01444},
- {Regular, "dir1/file1-2", "file1-2\n", 0666},
- {Dir, "dir2", "", 0700},
- {Regular, "dir2/file2-1", "file2-1\n", 0666},
- {Regular, "dir2/file2-2", "file2-2\n", 0666},
- {Dir, "dir3", "", 0700},
- {Regular, "dir3/file3-1", "file3-1\n", 0666},
- {Regular, "dir3/file3-2", "file3-2\n", 0666},
- {Dir, "dir4", "", 0700},
- {Regular, "dir4/file3-1", "file4-1\n", 0666},
- {Regular, "dir4/file3-2", "file4-2\n", 0666},
- {Symlink, "symlink1", "target1", 0666},
- {Symlink, "symlink2", "target2", 0666},
- {Symlink, "symlink3", root + "/file1", 0666},
- {Symlink, "symlink4", root + "/symlink3", 0666},
- {Symlink, "dirSymlink", root + "/dir1", 0740},
- }
+ {filetype: Regular, path: "file1", contents: "file1\n", permissions: 0600},
+ {filetype: Regular, path: "file2", contents: "file2\n", permissions: 0666},
+ {filetype: Regular, path: "file3", contents: "file3\n", permissions: 0404},
+ {filetype: Regular, path: "file4", contents: "file4\n", permissions: 0600},
+ {filetype: Regular, path: "file5", contents: "file5\n", permissions: 0600},
+ {filetype: Regular, path: "file6", contents: "file6\n", permissions: 0600},
+ {filetype: Regular, path: "file7", contents: "file7\n", permissions: 0600},
+ {filetype: Dir, path: "dir1", contents: "", permissions: 0740},
+ {filetype: Regular, path: "dir1/file1-1", contents: "file1-1\n", permissions: 01444},
+ {filetype: Regular, path: "dir1/file1-2", contents: "file1-2\n", permissions: 0666},
+ {filetype: Dir, path: "dir2", contents: "", permissions: 0700},
+ {filetype: Regular, path: "dir2/file2-1", contents: "file2-1\n", permissions: 0666},
+ {filetype: Regular, path: "dir2/file2-2", contents: "file2-2\n", permissions: 0666},
+ {filetype: Dir, path: "dir3", contents: "", permissions: 0700},
+ {filetype: Regular, path: "dir3/file3-1", contents: "file3-1\n", permissions: 0666},
+ {filetype: Regular, path: "dir3/file3-2", contents: "file3-2\n", permissions: 0666},
+ {filetype: Dir, path: "dir4", contents: "", permissions: 0700},
+ {filetype: Regular, path: "dir4/file3-1", contents: "file4-1\n", permissions: 0666},
+ {filetype: Regular, path: "dir4/file3-2", contents: "file4-2\n", permissions: 0666},
+ {filetype: Symlink, path: "symlink1", contents: "target1", permissions: 0666},
+ {filetype: Symlink, path: "symlink2", contents: "target2", permissions: 0666},
+ {filetype: Symlink, path: "symlink3", contents: root + "/file1", permissions: 0666},
+ {filetype: Symlink, path: "symlink4", contents: root + "/symlink3", permissions: 0666},
+ {filetype: Symlink, path: "dirSymlink", contents: root + "/dir1", permissions: 0740},
+ }
+ provisionSampleDir(t, root, files)
+}
+func provisionSampleDir(t *testing.T, root string, files []FileData) {
now := time.Now()
for _, info := range files {
p := path.Join(root, info.path)
diff --git a/pkg/archive/copy_unix_test.go b/pkg/archive/copy_unix_test.go
index 4d5ae79cd..fc17ae010 100644
--- a/pkg/archive/copy_unix_test.go
+++ b/pkg/archive/copy_unix_test.go
@@ -15,6 +15,8 @@ import (
"path/filepath"
"strings"
"testing"
+
+ "github.com/stretchr/testify/assert"
)
func removeAllPaths(paths ...string) {
@@ -976,3 +978,28 @@ func TestCopyCaseJFSym(t *testing.T) {
t.Fatal(err)
}
}
+
+func TestCopyInfoDestinationPathSymlink(t *testing.T) {
+ tmpDir, _ := getTestTempDirs(t)
+ defer removeAllPaths(tmpDir)
+
+ root := strings.TrimRight(tmpDir, "/") + "/"
+ files := []FileData{
+ {filetype: Regular, path: "file1", permissions: 0600, expected: CopyInfo{Path: root + "file1", Exists: true}},
+ {filetype: Dir, path: "dir1", permissions: 0700, expected: CopyInfo{Path: root + "dir1", Exists: true, IsDir: true}},
+ {filetype: Regular, path: "dir1/file1", permissions: 0600, expected: CopyInfo{Path: root + "dir1/file1", Exists: true}},
+ {filetype: Symlink, path: "symlink1", contents: "noSuchTarget", permissions: 0600, expected: CopyInfo{Path: root + "noSuchTarget", Exists: false}},
+ {filetype: Symlink, path: "symlink2", contents: "file1", permissions: 0600, expected: CopyInfo{Path: root + "file1", Exists: true}},
+ {filetype: Symlink, path: "symlink3", contents: root + "file1", permissions: 0600, expected: CopyInfo{Path: root + "file1", Exists: true}},
+ {filetype: Symlink, path: "symlink4", contents: root + "symlink3", permissions: 0600, expected: CopyInfo{Path: root + "file1", Exists: true}},
+ {filetype: Symlink, path: "dirSymlink", contents: root + "dir1", permissions: 0600, expected: CopyInfo{Path: root + "dir1", Exists: true, IsDir: true}},
+ }
+ provisionSampleDir(t, tmpDir, files)
+
+ for _, info := range files {
+ p := filepath.Join(tmpDir, info.path)
+ ci, err := CopyInfoDestinationPath(p)
+ assert.NoError(t, err)
+ assert.Equal(t, info.expected, ci)
+ }
+}
--
2.13.1
|
@thaJeztah Thanks, I think your test suggestion looks like a good start but first check my understanding of what’s happened since my original PR. I think this will have to go into the docker/cli repo since the moby/moby CopyInfoDestinationPath seems to be dead code. Let me explain:
I manually tested the same os.Lstat -> os.Stat change in docker/cli and it fixes the problem. As for me declaring the moby/moby copy.go logic dead code. I added asserts all over the moby/moby copy.go file and all of the existing ‘cp’ tests pass. This is what lead me to discover the docker/cli repo in the first place. So I don’t know this with 100% confidence but it seems pretty obvious that its not running in the integration-cli tests anymore. So if this is how things are working today; I think to solve this and cleanup the code we would have to:
Anyway, let me know what you think and I can start picking off some of these items. Also, any news on the security assessment that you mentioned earlier? |
|
Ah, I see the confusion. Let me try to explain (in a rather schizophrenic way, as I happen to be both a maintainer for the Moby project, and employed by Docker 😅) What happened with the CLI split, is that the codebase for the CLI is indeed in a separate repository. The Docker CLI is not part of the Moby project, just like many other tools that use the remote API. All existing integration tests that use the Docker CLI are kept. However, the version of the Docker CLI that is used to run them is fixated to the version that was used at the time that the Docker CLI moved to its own repository. Basically, those tests are used to verify that what worked keeps working, and that new features that are added, removed or changed in this repository, don't break those tests. It's possible changes break newer versions of the Docker CLI, or break something that isn't tested; it's Docker's responsibility to run integration tests with newer versions of the CLI, and if something broke, to report it here (or provide a fix). Given that those breaking changes would be either due to a change in the API, or due to a change in behavior of a package, testing for that can all be covered by API tests, or unit tests of the package 🎉
The code from this repository is still used 😄 Here's how: The Docker CLI uses various packages that are in this repository as a dependency (as well as many other packages from other repositories). You can find all dependencies in the For example, the vendored version of the file that's being changed in this pull request can be found in vendor/github.com/docker/docker/pkg/archive/copy.go. Files are vendored (not downloaded at compile time) so that builds are always reproducible, and don't depend on repositories we don't control (repositories may go down, someone can do a "force push" and mess things up, and repositories may move 😇 (e.g. get renamed from docker to moby)). A tool (LK4D4/vndr) is used to "vendor" those dependencies that. Vendoring involves fetching the exact git-commit (or tag) of the dependency as specified in the vendor.conf configuration file. After that, the tool analyzes all import statements, and removes all files that are not used, including test files (as described earlier, making sure a package is tested is a responsiblity of the package author / repository). If you look at the So how to get this fix into the Docker CLI?First, the PR needs to be finished 😄 - the package's unit tests need to be updated to check that it works as designed with this change. An API test may not nescessarily be needed, because "copy" in general is already tested, and this PR only changes the client-side code that resolves the target before the actual copy takes place (but I may have overlooked something 😅). Once the PR is merged, a "vendor bump" PR can be opened against the Docker CLI repository to bring the actual changes into the Docker CLI. A vendor PR basically is;
You can find an example of a "bump" pull request here: docker/cli#319 (docker/cli@a996206) Hope this helps! |
|
@thaJeztah This is exactly what I needed, thanks for clearing this up for me! |
dccurtis
force-pushed
the
issue-31816
branch
2 times, most recently
from
5ab7af8 to
2515ab1
Compare
|
@dccurtis looks like there's a gofmt issue; |
|
otherwise looks good to me |
…InfoDestinationPath Signed-off-by: Douglas Curtis <dougcurtis1@gmail.com> Commenting out tests for now Signed-off-by: Doug Curtis <dougcurtis1@gmail.com> Added unit test for CopyInfoDestionationPath. Signed-off-by: Doug Curtis <dougcurtis1@gmail.com> Removing integration-cli test case additions Signed-off-by: Doug Curtis <dougcurtis1@gmail.com> Removing extra spaces between archive_unix_test.go test cases Signed-off-by: Doug Curtis <dougcurtis1@gmail.com> Fixed gofmt issues in archive_unix_test.go Signed-off-by: Doug Curtis <dougcurtis1@gmail.com>
|
Any updates on when this change should merge? |
|
Ping @tonistiigi to see if this is OK |
|
LGTM |
|
👍 |
|
@thaJeztah Can you give me a pointer on how to resolve this error that I'm getting with vndr? Thanks! .... |
|
@dccurtis looks like a moby commit was entered on the wrong line (docker/distribution)? |
…directory Description: ‘docker cp’ fails with error ‘not a directory’ if a file is copied from a container to a host symlinked directory with a new target filename. The fix is to use os.Stat instead of os.Lstat since os.Stat follows the linked path. (Backport from moby#31993) It is not merged yet. Link: moby#31816 Signed-off-by: Wentao Zhang <zhangwentao234@huawei.com>
Fixing ‘docker cp’ to allow new target file name in a host symlinked directory Description: ‘docker cp’ fails with error ‘not a directory’ if a file is copied from a container to a host symlinked directory with a new target filename. The fix is to use os.Stat instead of os.Lstat since os.Stat follows the linked path. <br> (Backport from <a href="moby#31993" rel="nofollow noreferrer" target="_blank">https://github.com/moby/moby/pull/31993</a>) It is not merged yet. Link: <a href="moby#31816" rel="nofollow noreferrer" target="_blank">https://github.com/moby/moby/issues/31816</a> <br> Signed-off-by: Wentao Zhang <a href="mailto:zhangwentao234@huawei.com">zhangwentao234@huawei.com</a> See merge request docker/docker!632
‘docker cp’ fails with error ‘not a directory’ if a file is copied from a container to a host symlinked directory with a new target filename.
The fix is to use os.Stat instead of os.Lstat since os.Stat follows the linked path.
Fixes Issue #31816
Signed-off-by: Douglas Curtis dougcurtis1@gmail.com
- What I did
pkg/archive/copy.go:: CopyInfoDestinationPath was using os.Lstat to determine whether or not the destination parent folder is a directory or not. After some experimentation with the ‘os’ package I found that FileInfo.IsDir() returns false for a symlinked directory.
I changed os.Lstat to os.Stat in CopyInfoDestinationPath.
- How I did it
Code inspection led me to the CopyInfoDestinationPath. Once I noticed that os.Lstat was being used to determine directory status I performed some local experiments and found that IsDir() does not report symlinked directories as true.
- How to verify it
After manually reproducing the steps provided I found a test gap in TestCpFromSymlinkDestination that missed the case of a new target file name to a symlinked directory. Adding the test case reproduced the failure in the automated testing environment.
After fixing the issue the test case passed as expected.
Manual functional test:
Results: issue-31816-ut.txt
- Description for the changelog
Added isDirectory method to detect symlink directories in CopyInfoDestinationPath to enable copying to a new target file name located in a symlinked host directory.