fix: container filesystem resolves symlinks correctly#6559
fix: container filesystem resolves symlinks correctly#6559tonistiigi merged 1 commit intomoby:masterfrom
Conversation
jsternberg
force-pushed
the
container-fs-readlink
branch
from
c1875b7 to
e8d1939
Compare
jsternberg
force-pushed
the
container-fs-readlink
branch
2 times, most recently
from
c1875b7 to
3a21bae
Compare
| } | ||
|
|
||
| path, err := filepath.Rel("/", req.Path) | ||
| fpath, err := filepath.Rel("/", req.Path) |
There was a problem hiding this comment.
Check if this should be path, or maybe path.Join("/", req.Path)[1:] is better.
There was a problem hiding this comment.
I'm going to update this to another implementation that just trims the prefix. I think this is probably more complicated than it was intended. The intention was that the API was only supposed to accept absolute values. Using path.Join causes it to accept both absolute and relative paths which isn't the intention. At the same time, the current implementation accepts filepaths relative to the OS rather than only Unix filepaths which is the standard everywhere else.
It's probably better to just clean the path, check if the first character is a slash, then trim it. I'll also update the other functions to use the same utility.
jsternberg
force-pushed
the
container-fs-readlink
branch
2 times, most recently
from
e0b36ea to
ebec28d
Compare
jsternberg
force-pushed
the
container-fs-readlink
branch
from
ebec28d to
494737b
Compare
|
|
||
| // Resolving this symlink causes the path to escape. | ||
| // Attempt to use lstat and allow the client to perform the resolution. | ||
| fi, err = fs.Lstat(fsys, fpath) |
There was a problem hiding this comment.
This seems like a fine fallback, but maybe we should consider LStatFile in follow-up, as it doesn't really seem to be possible to reliably differentiate between them.
There was a problem hiding this comment.
I was thinking about adding a NoFollowLinks to the stat request but then I'd have to implement it on the other implementation of StatFile and it didn't really seem like something that anybody would actually use right now.
The symlink reader with the new container exec filesystem capability was reading symlinks in an invalid way. The `io/fs` interface requires all paths to be relative paths, but it was passing an absolute path to the function which returned an error. This was primarily affecting `ReadDir` which would error out if it tried to read a directory with a symlink in it. Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
jsternberg
force-pushed
the
container-fs-readlink
branch
from
494737b to
fd45966
Compare
2 participants
The symlink reader with the new container exec filesystem capability was
reading symlinks in an invalid way. The
io/fsinterface requires allpaths to be relative paths, but it was passing an absolute path to the
function which returned an error.
This was primarily affecting
ReadDirwhich would error out if it triedto read a directory with a symlink in it.