Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
add: preserve ownerships and permissions on ADDed archives
When extracting archives that are added using ADD, don't override
permissions and ownership information.  We regressed on this when we
switched to using the copier package to handle them.

Add a conformance test to prevent regressions on this.

Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
  • Loading branch information
nalind committed Sep 29, 2020
1 parent b3f6ed8 commit c1a1805
Show file tree
Hide file tree
Showing 4 changed files with 221 additions and 17 deletions.
26 changes: 16 additions & 10 deletions add.go
Expand Up @@ -33,7 +33,8 @@ type AddAndCopyOptions struct {
Chown string
// PreserveOwnership, if Chown is not set, tells us to avoid setting
// ownership of copied items to 0:0, instead using whatever ownership
// information is already set. Not meaningful for remote sources.
// information is already set. Not meaningful for remote sources or
// local archives that we extract.
PreserveOwnership bool
// All of the data being copied will pass through Hasher, if set.
// If the sources are URLs or files, their contents will be passed to
Expand Down Expand Up @@ -210,7 +211,6 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption

// Find out which user (and group) the destination should belong to.
var chownDirs, chownFiles *idtools.IDPair
var chmodDirs, chmodFiles *os.FileMode
var user specs.User
if options.Chown != "" {
user, _, err = b.user(mountPoint, options.Chown)
Expand Down Expand Up @@ -319,9 +319,9 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
UIDMap: destUIDMap,
GIDMap: destGIDMap,
ChownDirs: chownDirs,
ChmodDirs: chmodDirs,
ChmodDirs: nil,
ChownFiles: chownFiles,
ChmodFiles: chmodFiles,
ChmodFiles: nil,
}
putErr = copier.Put(mountPoint, extractDirectory, putOptions, io.TeeReader(pipeReader, hasher))
}
Expand Down Expand Up @@ -396,6 +396,10 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
GIDMap: srcGIDMap,
Excludes: options.Excludes,
ExpandArchives: extract,
ChownDirs: chownDirs,
ChmodDirs: nil,
ChownFiles: chownFiles,
ChmodFiles: nil,
StripSetuidBit: options.StripSetuidBit,
StripSetgidBit: options.StripSetgidBit,
StripStickyBit: options.StripStickyBit,
Expand Down Expand Up @@ -423,12 +427,14 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption
_, putErr = io.Copy(hasher, pipeReader)
} else {
putOptions := copier.PutOptions{
UIDMap: destUIDMap,
GIDMap: destGIDMap,
ChownDirs: chownDirs,
ChmodDirs: chmodDirs,
ChownFiles: chownFiles,
ChmodFiles: chmodFiles,
UIDMap: destUIDMap,
GIDMap: destGIDMap,
DefaultDirOwner: chownDirs,
DefaultDirMode: nil,
ChownDirs: nil,
ChmodDirs: nil,
ChownFiles: nil,
ChmodFiles: nil,
}
putErr = copier.Put(mountPoint, extractDirectory, putOptions, io.TeeReader(pipeReader, hasher))
}
Expand Down
43 changes: 36 additions & 7 deletions copier/copier.go
Expand Up @@ -222,6 +222,10 @@ type GetOptions struct {
UIDMap, GIDMap []idtools.IDMap // map from hostIDs to containerIDs in the output archive
Excludes []string // contents to pretend don't exist, using the OS-specific path separator
ExpandArchives bool // extract the contents of named items that are archives
ChownDirs *idtools.IDPair // set ownership on directories. no effect on archives being extracted
ChmodDirs *os.FileMode // set permissions on directories. no effect on archives being extracted
ChownFiles *idtools.IDPair // set ownership of files. no effect on archives being extracted
ChmodFiles *os.FileMode // set permissions on files. no effect on archives being extracted
StripSetuidBit bool // strip the setuid bit off of items being copied. no effect on archives being extracted
StripSetgidBit bool // strip the setgid bit off of items being copied. no effect on archives being extracted
StripStickyBit bool // strip the sticky bit off of items being copied. no effect on archives being extracted
Expand Down Expand Up @@ -265,6 +269,8 @@ func Get(root string, directory string, options GetOptions, globs []string, bulk
// PutOptions controls parts of Put()'s behavior.
type PutOptions struct {
UIDMap, GIDMap []idtools.IDMap // map from containerIDs to hostIDs when writing contents to disk
DefaultDirOwner *idtools.IDPair // set ownership of implicitly-created directories, default is ChownDirs, or 0:0 if ChownDirs not set
DefaultDirMode *os.FileMode // set permissions on implicitly-created directories, default is ChmodDirs, or 0755 if ChmodDirs not set
ChownDirs *idtools.IDPair // set ownership of newly-created directories
ChmodDirs *os.FileMode // set permissions on newly-created directories
ChownFiles *idtools.IDPair // set ownership of newly-created files
Expand Down Expand Up @@ -1193,6 +1199,22 @@ func copierHandlerGetOne(srcfi os.FileInfo, symlinkTarget, name, contentPath str
return errors.Wrapf(err, "error mapping host filesystem owners %#v to container filesystem owners", hostPair)
}
}
// force ownership and/or permissions, if requested
if hdr.Typeflag == tar.TypeDir {
if options.ChownDirs != nil {
hdr.Uid, hdr.Gid = options.ChownDirs.UID, options.ChownDirs.GID
}
if options.ChmodDirs != nil {
hdr.Mode = int64(*options.ChmodDirs)
}
} else {
if options.ChownFiles != nil {
hdr.Uid, hdr.Gid = options.ChownFiles.UID, options.ChownFiles.GID
}
if options.ChmodFiles != nil {
hdr.Mode = int64(*options.ChmodFiles)
}
}
// output the header
if err = tw.WriteHeader(hdr); err != nil {
return errors.Wrapf(err, "error writing header for %s (%s)", contentPath, hdr.Name)
Expand Down Expand Up @@ -1220,13 +1242,20 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM
errorResponse := func(fmtspec string, args ...interface{}) (*response, func() error, error) {
return &response{Error: fmt.Sprintf(fmtspec, args...), Put: putResponse{}}, nil, nil
}
dirUID, dirGID := 0, 0
dirUID, dirGID, defaultDirUID, defaultDirGID := 0, 0, 0, 0
if req.PutOptions.ChownDirs != nil {
dirUID, dirGID = req.PutOptions.ChownDirs.UID, req.PutOptions.ChownDirs.GID
defaultDirUID, defaultDirGID = dirUID, dirGID
}
dirMode := os.FileMode(0755)
defaultDirMode := os.FileMode(0755)
if req.PutOptions.ChmodDirs != nil {
dirMode = *req.PutOptions.ChmodDirs
defaultDirMode = *req.PutOptions.ChmodDirs
}
if req.PutOptions.DefaultDirOwner != nil {
defaultDirUID, defaultDirGID = req.PutOptions.DefaultDirOwner.UID, req.PutOptions.DefaultDirOwner.GID
}
if req.PutOptions.DefaultDirMode != nil {
defaultDirMode = *req.PutOptions.DefaultDirMode
}
var fileUID, fileGID *int
if req.PutOptions.ChownFiles != nil {
Expand Down Expand Up @@ -1258,11 +1287,11 @@ func copierHandlerPut(bulkReader io.Reader, req request, idMappings *idtools.IDM
subdir = filepath.Join(subdir, component)
path := filepath.Join(req.Root, subdir)
if err := os.Mkdir(path, 0700); err == nil {
if err = lchown(path, dirUID, dirGID); err != nil {
return errors.Wrapf(err, "copier: put: error setting owner of %q to %d:%d", path, dirUID, dirGID)
if err = lchown(path, defaultDirUID, defaultDirGID); err != nil {
return errors.Wrapf(err, "copier: put: error setting owner of %q to %d:%d", path, defaultDirUID, defaultDirGID)
}
if err = os.Chmod(path, dirMode); err != nil {
return errors.Wrapf(err, "copier: put: error setting permissions on %q to 0%o", path, dirMode)
if err = os.Chmod(path, defaultDirMode); err != nil {
return errors.Wrapf(err, "copier: put: error setting permissions on %q to 0%o", path, defaultDirMode)
}
} else {
if !os.IsExist(err) {
Expand Down
164 changes: 164 additions & 0 deletions tests/conformance/conformance_test.go
Expand Up @@ -1473,6 +1473,170 @@ var internalTestCases = []testCase{
},
},

{
name: "add-permissions",
dockerfileContents: strings.Join([]string{
"FROM scratch",
fmt.Sprintf("# Does ADD preserve permissions differently for archives and files?"),
"ADD archive.tar subdir1/",
"ADD archive/ subdir2/",
}, "\n"),
tweakContextDir: func(t *testing.T, contextDir, storageDriver, storageRoot string) (err error) {
content := []byte("test content")

if err := os.Mkdir(filepath.Join(contextDir, "archive"), 0755); err != nil {
return errors.Wrapf(err, "error creating subdirectory of temporary context directory")
}
filename := filepath.Join(contextDir, "archive", "should-be-owned-by-root")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by root in temporary context directory")
}
if err = os.Chown(filename, 0, 0); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by root in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by root file in temporary context directory")
}
filename = filepath.Join(contextDir, "archive", "should-be-owned-by-99")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by 99 in temporary context directory")
}
if err = os.Chown(filename, 99, 99); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by 99 in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by 99 in temporary context directory")
}

filename = filepath.Join(contextDir, "archive.tar")
f, err := os.OpenFile(filename, os.O_CREATE|os.O_WRONLY, 0644)
if err != nil {
return errors.Wrapf(err, "error creating archive file")
}
defer f.Close()
tw := tar.NewWriter(f)
defer tw.Close()
err = tw.WriteHeader(&tar.Header{
Name: "archive/should-be-owned-by-root",
Typeflag: tar.TypeReg,
Size: int64(len(content)),
ModTime: testDate,
Mode: 0640,
Uid: 0,
Gid: 0,
})
if err != nil {
return errors.Wrapf(err, "error writing archive file header")
}
n, err := tw.Write(content)
if err != nil {
return errors.Wrapf(err, "error writing archive file contents")
}
if n != len(content) {
return errors.Errorf("short write writing archive file contents")
}
err = tw.WriteHeader(&tar.Header{
Name: "archive/should-be-owned-by-99",
Typeflag: tar.TypeReg,
Size: int64(len(content)),
ModTime: testDate,
Mode: 0640,
Uid: 99,
Gid: 99,
})
if err != nil {
return errors.Wrapf(err, "error writing archive file header")
}
n, err = tw.Write(content)
if err != nil {
return errors.Wrapf(err, "error writing archive file contents")
}
if n != len(content) {
return errors.Errorf("short write writing archive file contents")
}
return nil
},
fsSkip: []string{"(dir):subdir1:mtime", "(dir):subdir2:mtime"},
},

{
name: "copy-permissions",
dockerfileContents: strings.Join([]string{
"FROM scratch",
fmt.Sprintf("# Does COPY --chown change permissions on already-present directories?"),
"COPY subdir/ subdir/",
"COPY --chown=99:99 subdir/ subdir/",
}, "\n"),
tweakContextDir: func(t *testing.T, contextDir, storageDriver, storageRoot string) (err error) {
content := []byte("test content")

if err := os.Mkdir(filepath.Join(contextDir, "subdir"), 0755); err != nil {
return errors.Wrapf(err, "error creating subdirectory of temporary context directory")
}
filename := filepath.Join(contextDir, "subdir", "would-be-owned-by-root")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by root in temporary context directory")
}
if err = os.Chown(filename, 0, 0); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by root in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by root file in temporary context directory")
}
filename = filepath.Join(contextDir, "subdir", "would-be-owned-by-99")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by 99 in temporary context directory")
}
if err = os.Chown(filename, 99, 99); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by 99 in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by 99 in temporary context directory")
}
return nil
},
fsSkip: []string{"(dir):subdir:mtime"},
},

{
name: "copy-permissions-implicit",
dockerfileContents: strings.Join([]string{
"FROM scratch",
fmt.Sprintf("# Does COPY --chown change permissions on already-present directories?"),
"COPY --chown=99:99 subdir/ subdir/",
"COPY subdir/ subdir/",
}, "\n"),
tweakContextDir: func(t *testing.T, contextDir, storageDriver, storageRoot string) (err error) {
content := []byte("test content")

if err := os.Mkdir(filepath.Join(contextDir, "subdir"), 0755); err != nil {
return errors.Wrapf(err, "error creating subdirectory of temporary context directory")
}
filename := filepath.Join(contextDir, "subdir", "would-be-owned-by-root")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by root in temporary context directory")
}
if err = os.Chown(filename, 0, 0); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by root in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by root file in temporary context directory")
}
filename = filepath.Join(contextDir, "subdir", "would-be-owned-by-99")
if err = ioutil.WriteFile(filename, content, 0640); err != nil {
return errors.Wrapf(err, "error creating file owned by 99 in temporary context directory")
}
if err = os.Chown(filename, 99, 99); err != nil {
return errors.Wrapf(err, "error setting ownership on file owned by 99 in temporary context directory")
}
if err = os.Chtimes(filename, testDate, testDate); err != nil {
return errors.Wrapf(err, "error setting date on file owned by 99 in temporary context directory")
}
return nil
},
fsSkip: []string{"(dir):subdir:mtime"},
},

{
// the digest just ensures that we can handle a digest
// reference to a manifest list; the digest of any manifest
Expand Down
5 changes: 5 additions & 0 deletions tests/copy.bats
Expand Up @@ -179,12 +179,17 @@ load helpers
expect_output "nobody:root" "stat UG /subdir/$i"
done

# subdir will have been implicitly created, and the --chown should have had an effect
run_buildah run $cid stat -c "%U:%G" /subdir
expect_output "nobody:root" "stat UG /subdir"

run_buildah copy --chown root:root $cid ${TESTDIR}/other-subdir /subdir
for i in randomfile other-randomfile ; do
run_buildah run $cid stat -c "%U:%G" /subdir/$i
expect_output "root:root" "stat UG /subdir/$i (after chown)"
done

# subdir itself will have not been copied (the destination directory was created implicitly), so its permissions should not have changed
run_buildah run $cid stat -c "%U:%G" /subdir
expect_output "nobody:root" "stat UG /subdir"
}
Expand Down

0 comments on commit c1a1805

Please sign in to comment.