Fix --from with absolute paths to local image transports - #6975
Conversation
nalind
left a comment
nalind
left a comment
There was a problem hiding this comment.
This needs to be much more careful about attempting to read data from the filesystem that isn't somewhere within the context directory.
| imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, archiveSource), tarOptions, contextDir) | ||
| if filepath.IsAbs(archiveSource) { | ||
| if _, statErr := os.Stat(archiveSource); statErr == nil { | ||
| imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) |
There was a problem hiding this comment.
This bypasses the attempt to ensure that we're only reading content from the context directory for all cases, not only those where "--from" was used to provide the value that we're processing.
| imageArchive, err = newSingleItemArchive(contextDir, archiveSource) | ||
| if filepath.IsAbs(archiveSource) { | ||
| if _, statErr := os.Stat(archiveSource); statErr == nil { | ||
| imageArchive, err = newSingleItemArchive(filepath.Dir(archiveSource), filepath.Base(archiveSource)) |
There was a problem hiding this comment.
This bypasses the attempt to ensure that we're only reading content from the context directory for all cases, not only those where "--from" was used to provide the value that we're processing.
| imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, archiveSource), tarOptions, contextDir) | ||
| if filepath.IsAbs(archiveSource) { | ||
| if _, statErr := os.Stat(archiveSource); statErr == nil { | ||
| imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) |
There was a problem hiding this comment.
This bypasses the attempt to ensure that we're only reading content from the context directory for all cases, not only those where "--from" was used to provide the value that we're processing.
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Fixes: podman-container-tools#6932 Signed-off-by: Jan Rodák <hony.com@seznam.cz>
|
|
||
| // Decide before source policy may rewrite from: absolute paths are only | ||
| // allowed for the value supplied via --from, not Containerfile FROM lines. | ||
| allowAbsolutePaths := s.executor.originalFromOverride != "" && from == s.executor.originalFromOverride |
There was a problem hiding this comment.
This evaluates as true for every stage of a multi-stage build that uses the same base as the first stage. It isn't clear from the commit log if that's intentional.
| // archive only the archive file for copying to the new archive file | ||
| imageArchive, err = newSingleItemArchive(contextDir, archiveSource) | ||
| if allowAbsolutePaths && filepath.IsAbs(archiveSource) { | ||
| imageArchive, err = newSingleItemArchive(filepath.Dir(archiveSource), filepath.Base(archiveSource)) |
There was a problem hiding this comment.
Writing out a new copy seems unnecessary. Passing back a reader for the original file and setting isEmbeddedArchive false for this case would provide the same data.
| tarOptions := &archive.TarOptions{} | ||
| imageArchive, err = chrootarchive.Tar(filepath.Join(contextDir, archiveSource), tarOptions, contextDir) | ||
| if allowAbsolutePaths && filepath.IsAbs(archiveSource) { | ||
| imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) |
There was a problem hiding this comment.
Is the intent here to disrupt links to items outside of the context directory when absolute paths are being allowed? If it is not, then we don't need to be calling this function.
| imageArchive, err = chrootarchive.Tar(archiveSource, tarOptions, filepath.Dir(archiveSource)) | ||
| } else { | ||
| // Leading "/" is context-absolute, not a host path; avoid filepath.Join | ||
| // dropping contextDir when archiveSource is absolute. |
There was a problem hiding this comment.
I'm not sure that filepath.Join() does that?
| case directoryTransport.Transport.Name(): // this is also a directory tree | ||
| // this takes the form of just a path | ||
| transportRef := restOfImageName | ||
| archiveSource = transportRef |
There was a problem hiding this comment.
Moved for stylistic reasons?
| if allowAbsolutePaths && filepath.IsAbs(archiveSource) { | ||
| archiveRoot = filepath.Dir(archiveSource) | ||
| } | ||
| return "", fmt.Errorf("error archiving source at %q under %q", archiveSource, archiveRoot) |
There was a problem hiding this comment.
Should this error message wrap err?
| cat > "${contextdir}"/Containerfile << 'EOF' | ||
| FROM overridden | ||
| RUN touch /absolute-path-test | ||
| EOF |
There was a problem hiding this comment.
We expect --from to only replace the value referenced in the first FROM instruction, but this doesn't check that for the types of values that we're going to start handling specially here.
2 participants
Fixes: #6932
What type of PR is this?
What this PR does / why we need it:
How to verify it
Which issue(s) this PR fixes:
Special notes for your reviewer:
Does this PR introduce a user-facing change?