From 6a8a79201954875ad97a6835d4c9db95935ac98f Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 23 Oct 2023 15:59:33 -0400 Subject: [PATCH 1/4] pkg/archive: test tar headers are interoperable The existing pkg/archive unit tests are primarily round-trip tests which assert that pkg/archive produces tarballs which pkg/archive can unpack. While these tests are effective at catching regressions in archiving or unarchiving, they have a blind spot for regressions in compatibility with the rest of the ecosystem. For example, a typo in the capabilities extended attribute constant would result in subtly broken image layer tarballs, but the existing tests would not catch the bug if both the archiving and unarchiving implementations have the same typo. Extend the test for archiving an overlay filesystem layer to assert that the overlayfs style whiteouts (extended attributes and device files) are transformed into AUFS-style whiteouts (magic file names). Extend the test for archiving files with extended attributes to assert that the extended attribute is encoded into the file's tar header in the standard, interoperable format compatible with the rest of the ecosystem. Signed-off-by: Cory Snider --- pkg/archive/archive_linux_test.go | 39 ++++++++++++++++++++++++++++--- pkg/archive/archive_unix_test.go | 21 +++++++++++++++++ 2 files changed, 57 insertions(+), 3 deletions(-) diff --git a/pkg/archive/archive_linux_test.go b/pkg/archive/archive_linux_test.go index 5159e81f16..4402f66dde 100644 --- a/pkg/archive/archive_linux_test.go +++ b/pkg/archive/archive_linux_test.go @@ -1,6 +1,9 @@ package archive // import "github.com/docker/docker/pkg/archive" import ( + "archive/tar" + "bytes" + "io" "os" "path/filepath" "syscall" @@ -8,8 +11,10 @@ import ( "github.com/containerd/containerd/pkg/userns" "github.com/docker/docker/pkg/system" + "github.com/google/go-cmp/cmp/cmpopts" "golang.org/x/sys/unix" "gotest.tools/v3/assert" + is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/skip" ) @@ -103,11 +108,39 @@ func TestOverlayTarUntar(t *testing.T) { Compression: Uncompressed, WhiteoutFormat: OverlayWhiteoutFormat, } - archive, err := TarWithOptions(src, options) + reader, err := TarWithOptions(src, options) + assert.NilError(t, err) + archive, err := io.ReadAll(reader) + reader.Close() assert.NilError(t, err) - defer archive.Close() - err = Untar(archive, dst, options) + // The archive should encode opaque directories and file whiteouts + // in AUFS format. + entries := make(map[string]struct{}) + rdr := tar.NewReader(bytes.NewReader(archive)) + for { + h, err := rdr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + assert.Check(t, is.Equal(h.Devmajor, int64(0)), "unexpected device file in archive") + assert.Check(t, is.DeepEqual(h.PAXRecords, map[string]string(nil), cmpopts.EquateEmpty())) + entries[h.Name] = struct{}{} + } + + assert.DeepEqual(t, entries, map[string]struct{}{ + "d1/": {}, + "d1/" + WhiteoutOpaqueDir: {}, + "d1/f1": {}, + "d2/": {}, + "d2/" + WhiteoutOpaqueDir: {}, + "d2/f1": {}, + "d3/": {}, + "d3/" + WhiteoutPrefix + "f1": {}, + }) + + err = Untar(bytes.NewReader(archive), dst, options) assert.NilError(t, err) checkFileMode(t, filepath.Join(dst, "d1"), 0o700|os.ModeDir) diff --git a/pkg/archive/archive_unix_test.go b/pkg/archive/archive_unix_test.go index e80051695d..6f9816c7ee 100644 --- a/pkg/archive/archive_unix_test.go +++ b/pkg/archive/archive_unix_test.go @@ -254,6 +254,27 @@ func TestTarUntarWithXattr(t *testing.T) { out, err := exec.Command("setcap", "cap_block_suspend+ep", filepath.Join(origin, "2")).CombinedOutput() assert.NilError(t, err, string(out)) + tarball, err := Tar(origin, Uncompressed) + assert.NilError(t, err) + defer tarball.Close() + rdr := tar.NewReader(tarball) + for { + h, err := rdr.Next() + if err == io.EOF { + break + } + assert.NilError(t, err) + capability, hasxattr := h.PAXRecords["SCHILY.xattr.security.capability"] + switch h.Name { + case "2": + if assert.Check(t, hasxattr, "tar entry %q should have the 'security.capability' xattr", h.Name) { + assert.Check(t, len(capability) > 0, "tar entry %q has a blank 'security.capability' xattr value") + } + default: + assert.Check(t, !hasxattr, "tar entry %q should not have the 'security.capability' xattr", h.Name) + } + } + for _, c := range []Compression{ Uncompressed, Gzip, From 3cf409aa9e6e1f8d28e82e2e0cb2a272e7094d78 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 23 Oct 2023 14:40:42 -0400 Subject: [PATCH 2/4] pkg/archive: migrate to (tar.Header).PAXRecords Signed-off-by: Cory Snider --- pkg/archive/archive.go | 19 +++++++++++++------ pkg/archive/archive_linux.go | 4 +--- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/archive/archive.go b/pkg/archive/archive.go index fb42864b6b..43133a0950 100644 --- a/pkg/archive/archive.go +++ b/pkg/archive/archive.go @@ -481,6 +481,8 @@ func FileInfoHeader(name string, fi os.FileInfo, link string) (*tar.Header, erro return hdr, nil } +const paxSchilyXattr = "SCHILY.xattr." + // ReadSecurityXattrToTarHeader reads security.capability xattr from filesystem // to a tar header func ReadSecurityXattrToTarHeader(path string, hdr *tar.Header) error { @@ -493,15 +495,16 @@ func ReadSecurityXattrToTarHeader(path string, hdr *tar.Header) error { ) capability, _ := system.Lgetxattr(path, "security.capability") if capability != nil { - length := len(capability) if capability[versionOffset] == vfsCapRevision3 { // Convert VFS_CAP_REVISION_3 to VFS_CAP_REVISION_2 as root UID makes no // sense outside the user namespace the archive is built in. capability[versionOffset] = vfsCapRevision2 - length = xattrCapsSz2 + capability = capability[:xattrCapsSz2] } - hdr.Xattrs = make(map[string]string) - hdr.Xattrs["security.capability"] = string(capability[:length]) + if hdr.PAXRecords == nil { + hdr.PAXRecords = make(map[string]string) + } + hdr.PAXRecords[paxSchilyXattr+"security.capability"] = string(capability) } return nil } @@ -776,8 +779,12 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, o } var xattrErrs []string - for key, value := range hdr.Xattrs { - if err := system.Lsetxattr(path, key, []byte(value), 0); err != nil { + for key, value := range hdr.PAXRecords { + xattr, ok := strings.CutPrefix(key, paxSchilyXattr) + if !ok { + continue + } + if err := system.Lsetxattr(path, xattr, []byte(value), 0); err != nil { if bestEffortXattrs && errors.Is(err, syscall.ENOTSUP) || errors.Is(err, syscall.EPERM) { // EPERM occurs if modifying xattrs is not allowed. This can // happen when running in userns with restrictions (ChromeOS). diff --git a/pkg/archive/archive_linux.go b/pkg/archive/archive_linux.go index 6f18e0c598..2c3786cd50 100644 --- a/pkg/archive/archive_linux.go +++ b/pkg/archive/archive_linux.go @@ -41,9 +41,7 @@ func (overlayWhiteoutConverter) ConvertWrite(hdr *tar.Header, path string, fi os return nil, err } if len(opaque) == 1 && opaque[0] == 'y' { - if hdr.Xattrs != nil { - delete(hdr.Xattrs, "trusted.overlay.opaque") - } + delete(hdr.PAXRecords, paxSchilyXattr+"trusted.overlay.opaque") // create a header for the whiteout file // it should inherit some properties from the parent, but be a regular file From c44c9dfa79a44b3e185d954a18ab3bd36a2d84dd Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 23 Oct 2023 14:29:27 -0400 Subject: [PATCH 3/4] pkg/tarsum: migrate to (tar.Header).PAXRecords Fix a silly bug in the implementation which had the effect of len(h.Xattrs) blank entries being inserted in the middle of orderedHeaders. Luckily this is not a load-bearing bug: empty headers are ignored as the tarsum digest is computed by concatenating header keys and values without any intervening delimiter. Signed-off-by: Cory Snider --- pkg/tarsum/versioning.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/tarsum/versioning.go b/pkg/tarsum/versioning.go index edc3ec8cfd..c475dd9fe0 100644 --- a/pkg/tarsum/versioning.go +++ b/pkg/tarsum/versioning.go @@ -115,15 +115,18 @@ func v0TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) { func v1TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) { // Get extended attributes. - xAttrKeys := make([]string, len(h.Xattrs)) - for k := range h.Xattrs { - xAttrKeys = append(xAttrKeys, k) + const paxSchilyXattr = "SCHILY.xattr." + var xattrs [][2]string + for k, v := range h.PAXRecords { + if xattr, ok := strings.CutPrefix(k, paxSchilyXattr); ok { + xattrs = append(xattrs, [2]string{xattr, v}) + } } - sort.Strings(xAttrKeys) + sort.Slice(xattrs, func(i, j int) bool { return xattrs[i][0] < xattrs[j][0] }) // Make the slice with enough capacity to hold the 11 basic headers // we want from the v0 selector plus however many xattrs we have. - orderedHeaders = make([][2]string, 0, 11+len(xAttrKeys)) + orderedHeaders = make([][2]string, 0, 11+len(xattrs)) // Copy all headers from v0 excluding the 'mtime' header (the 5th element). v0headers := v0TarHeaderSelect(h) @@ -131,9 +134,7 @@ func v1TarHeaderSelect(h *tar.Header) (orderedHeaders [][2]string) { orderedHeaders = append(orderedHeaders, v0headers[6:]...) // Finally, append the sorted xattrs. - for _, k := range xAttrKeys { - orderedHeaders = append(orderedHeaders, [2]string{k, h.Xattrs[k]}) - } + orderedHeaders = append(orderedHeaders, xattrs...) return } From 52da88201c5fec0d31699cba4e614eb90e5a1468 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Mon, 23 Oct 2023 14:41:22 -0400 Subject: [PATCH 4/4] hack/validate: stop suppressing Xattrs deprecation Signed-off-by: Cory Snider --- hack/validate/golangci-lint.yml | 4 ---- 1 file changed, 4 deletions(-) diff --git a/hack/validate/golangci-lint.yml b/hack/validate/golangci-lint.yml index 89221ec2da..af744abf17 100644 --- a/hack/validate/golangci-lint.yml +++ b/hack/validate/golangci-lint.yml @@ -113,10 +113,6 @@ issues: path: "api/types/(volume|container)/" linters: - revive - # FIXME temporarily suppress these. See #39924 - - text: "SA1019: .*\\.Xattrs has been deprecated since Go 1.10: Use PAXRecords instead" - linters: - - staticcheck # FIXME temporarily suppress these. See #39926 - text: "SA1019: httputil.NewClientConn" linters: