-
Notifications
You must be signed in to change notification settings - Fork 18.7k
volume: evaluate symlinks before relabeling mount source #34792
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
Conversation
bdd637c
to
5e39020
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @runcom!
I have a couple of suggestions that might be useful or not.
- Requesting we update the doc of that function
- Perhaps we could collaborate on a test for this change. This is my first time diving into Docker's source code, so I could use your chaperoning. However, here is an idea for a test, based off the repro you provided
package test
import (
"io/ioutil"
"os"
"path/filepath"
"testing"
)
func TestSymlinkResolutionBeforeRelabeling(t *testing.T) {
// Initial setup:
// Our target repro on the commandline is:
// 1. $ mkdir /var/foo
// 2. $ touch /var/foo/test
// 3. $ ln -s /var/foo /var/bar
// 4. $ docker run -ti -v /var/bar:/var/bar:Z fedora sh
// 1.
scratchDir, err := ioutil.TempDir("", "foo")
if err != nil {
t.Fatalf("1. mkdir err: %v", err)
}
defer os.RemoveAll(scratchDir)
// 2.
f, err := os.Create(filepath.Join(scratchDir, "test"))
if err != nil {
t.Fatalf("2. creating test: %v", err)
}
f.Close()
// 3.
scratchParentDir := filepath.Base(scratchDir)
symlinkPath := filepath.Join(scratchParentDir, "bar")
if err := os.Symlink(scratchDir, symlinkPath); err != nil {
t.Fatalf("3. symlink err: %v", err)
}
defer os.RemoveAll(symlinkPath)
// 4.
mPt, err := moby.ParseMountRaw(symlinkPath, "fedora")
if err != nil {
t.Fatalf("4. mount: %v", err)
}
mountPath := symlinkPath + ":Z"
mountedPath, err := mPt.Setup(mountPath, "fedora", nil)
if err != nil {
t.Fatalf("4. expected a successful mount")
}
// Perhaps in here, add in the sh step
if g, w := mountedPath, scratchDir; g != w {
t.Fatalf("4. resolved paths don't match: got = %q want = %q", g, w)
}
}
Please help me fill in the blanks with the respective code steps, but otherwise I think the general structure of it reflects a 1-to-1 structure in Shell-to-Go of the repro you provided.
volume/volume.go
Outdated
@@ -156,7 +157,13 @@ func (m *MountPoint) Setup(mountLabel string, rootIDs idtools.IDPair, checkFun f | |||
return | |||
} | |||
|
|||
err = label.Relabel(m.Source, mountLabel, label.IsShared(m.Mode)) | |||
sourcePath, err := filepath.EvalSymlinks(m.Source) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we perhaps append a comment in the doc for (m *MountPoint) Setup
to describe the new behavior with something like this, maybe?
// If the MountPoint's source is a symlink, it is first
// evaluated to an absolute path before being relabeled.
Docker CI doesn't test with selinux, does it @cpuguy83? |
Not this CI, no. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems ok.
LGTM
Needs a rebase, though. |
rebased |
5e39020
to
3a46162
Compare
Janky failed; possibly due to:
(although it only prints "warning" if I see correctly) |
I guess it's confused by the named return variables (perhaps |
I'm pretty sure the linter is correct, the code is wrong (https://play.golang.org/p/bKhOuWeynv). The assignment on line 159 is masking the named return value. A unit test for |
doh, yes! I looked at that, but didn't test it, you're right @runcom ^^ |
Simple reproducer: ```sh $ mkdir /var/foo $ touch /var/foo/test $ ln -s /var/foo /var/bar $ docker run -ti -v /var/bar:/var/bar:Z fedora sh sh-4.3# ls -lZ /var/bar/ ls: cannot open directory '/var/bar/': Permission denied ``` Signed-off-by: Antonio Murdaca <runcom@redhat.com>
3a46162
to
e0b22c0
Compare
Should be fixed now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dnephin PTAL (if you have thoughts on that test; I see @odeke-em proposed a test above #34792 (review)) |
I'm not sure what you mean by thoughts on the test. The test should call |
Commit 317e94b introduced a local variable err, which masked the err used as a named return value. As a result, any relabeling error (happened in that defer) is not returned, and the caller gets "", nil, moving on to performing a bind mount with empty m.Source argument (which, apparently, results in mounting a current directory). This can be reproduced by trying to add a bind mount which has no space left (see RHBZ link below for details). This issue was spotted upstream during the review, and fixed before the merge (see [1]). In this repository, though, it was never fixed. Fixes: https://bugzilla.redhat.com/1878621 [1] moby#34792 (comment) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Simple reproducer:
$ mkdir /var/foo $ touch /var/foo/test $ ln -s /var/foo /var/bar $ docker run -ti -v /var/bar:/var/bar:Z fedora sh sh-4.3# ls -lZ /var/bar/ ls: cannot open directory '/var/bar/': Permission denied
@cpuguy83 PTAL
Signed-off-by: Antonio Murdaca runcom@redhat.com
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)