From 88a5e7b7038ac3a5e31b248fec5e680c45a2932f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 15 Nov 2024 21:37:54 +0100 Subject: [PATCH] When applying a chunked layer with a tar-split, compute its uncompressed digest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will allow c/image to validate the uncompressed digest against the config's RootFS.DiffID value (ensuring that the layer's contents are the same when pulled via TOC and traditionally); and the uncompressed digest will be used as a layer ID, ensuring users see the traditional layer and image IDs they are used to. This doesn't work for layers without a tar-split (all estargz, and old zstd:chunked layers); for those, we fall back to traditional pulls. Alternatively, for EXTREMELY restricted use cases, add an "insecure_allow_unpredictable_image_contents" option to storage.conf. This option allows partial pulls of estargz and old zstd:chunked layers, and skips the costly uncompressed digest computation. It is then up to the user to worry about images where the tar representaiton and the TOC representation don't match, and about unpredictable image IDs. Signed-off-by: Miloslav Trmač --- docs/containers-storage.conf.5.md | 21 +++++++ pkg/chunked/cache_linux.go | 2 +- pkg/chunked/storage_linux.go | 98 ++++++++++++++++++++++++++++--- storage.conf | 19 ++++++ 4 files changed, 132 insertions(+), 8 deletions(-) diff --git a/docs/containers-storage.conf.5.md b/docs/containers-storage.conf.5.md index 2f8e29b01f..e19c39ad38 100644 --- a/docs/containers-storage.conf.5.md +++ b/docs/containers-storage.conf.5.md @@ -124,6 +124,27 @@ The `storage.options.pull_options` table supports the following keys: It is an expensive operation so it is not enabled by default. This is a "string bool": "false"|"true" (cannot be native TOML boolean) +**insecure_allow_unpredictable_image_contents="false"|"true"** + This should _almost never_ be set. + It allows partial pulls ofof images without guaranteeing that "partial + pulls" and non-partial pulls both result in consistent image contents. + This allows pulling estargz images and early versions of zstd:chunked images; + otherwise, these layers always use the traditional non-partial pull path. + + This option should be enabled _extremely_ rarely, only if _all_ images that could + EVER be concievably pulled on this system are _guaranteed_ (e.g. using a signature policy) + to come from a build system trusted to never attack image integrity. + + If this consistency enforcement were disabled, malicious images could be built + in a way designed to evade other audit mechanisms, so presence of most other audit + mechanisms is not a replacement for the above-mentioned need for all images to come + from a trusted build system. + + As a side effect, enabling this option will also make image IDs unpredictable + (usually not equal to the traditional value matching the config digest). + + This is a "string bool": "false"|"true" (cannot be native TOML boolean) + ### STORAGE OPTIONS FOR AUFS TABLE The `storage.options.aufs` table supports the following options: diff --git a/pkg/chunked/cache_linux.go b/pkg/chunked/cache_linux.go index 47742b05d8..ac0c2dacd4 100644 --- a/pkg/chunked/cache_linux.go +++ b/pkg/chunked/cache_linux.go @@ -710,7 +710,7 @@ func prepareCacheFile(manifest []byte, format graphdriver.DifferOutputFormat) ([ switch format { case graphdriver.DifferOutputFormatDir: case graphdriver.DifferOutputFormatFlat: - entries, err = makeEntriesFlat(entries) + entries, err = makeEntriesFlat(entries, nil) if err != nil { return nil, err } diff --git a/pkg/chunked/storage_linux.go b/pkg/chunked/storage_linux.go index da43fde90f..85903b8f01 100644 --- a/pkg/chunked/storage_linux.go +++ b/pkg/chunked/storage_linux.go @@ -2,6 +2,7 @@ package chunked import ( archivetar "archive/tar" + "bytes" "context" "encoding/base64" "errors" @@ -28,12 +29,15 @@ import ( "github.com/containers/storage/pkg/fsverity" "github.com/containers/storage/pkg/idtools" "github.com/containers/storage/pkg/system" + securejoin "github.com/cyphar/filepath-securejoin" jsoniter "github.com/json-iterator/go" "github.com/klauspost/compress/zstd" "github.com/klauspost/pgzip" digest "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" "github.com/vbatts/tar-split/archive/tar" + "github.com/vbatts/tar-split/tar/asm" + tsStorage "github.com/vbatts/tar-split/tar/storage" "golang.org/x/sys/unix" ) @@ -113,10 +117,11 @@ type chunkedLayerData struct { // TO DO: ideally this should be parsed along with the rest of the config file into StoreOptions directly // (and then storage.Store.PullOptions would need to be somehow simulated). type pullOptions struct { - enablePartialImages bool // enable_partial_images - convertImages bool // convert_images - useHardLinks bool // use_hard_links - ostreeRepos []string // ostree_repos + enablePartialImages bool // enable_partial_images + convertImages bool // convert_images + useHardLinks bool // use_hard_links + insecureAllowUnpredictableImageContents bool // insecure_allow_unpredictable_image_contents + ostreeRepos []string // ostree_repos } func parsePullOptions(store storage.Store) pullOptions { @@ -131,6 +136,7 @@ func parsePullOptions(store storage.Store) pullOptions { {&res.enablePartialImages, "enable_partial_images", false}, {&res.convertImages, "convert_images", false}, {&res.useHardLinks, "use_hard_links", false}, + {&res.insecureAllowUnpredictableImageContents, "insecure_allow_unpredictable_image_contents", false}, } { if value, ok := options[e.name]; ok { *e.dest = strings.ToLower(value) == "true" @@ -294,12 +300,15 @@ func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest var badRequestErr ErrBadRequest return nil, errors.As(err, &badRequestErr), fmt.Errorf("read zstd:chunked manifest: %w", err) } + var uncompressedTarSize int64 = -1 if tarSplit != nil { uncompressedTarSize, err = tarSizeFromTarSplit(tarSplit) if err != nil { return nil, false, fmt.Errorf("computing size from tar-split: %w", err) } + } else if !pullOptions.insecureAllowUnpredictableImageContents { // With no tar-split, we can't compute the traditional UncompressedDigest. + return nil, true, fmt.Errorf("zstd:chunked layers without tar-split data don't support partial pulls with guaranteed consistency with non-partial pulls") } layersCache, err := getLayersCache(store) @@ -329,6 +338,10 @@ func makeZstdChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest // On error, the second return value is true if a fallback to an alternative (either the makeConverToRaw differ, or a non-partial pull) // is permissible. func makeEstargzChunkedDiffer(store storage.Store, blobSize int64, tocDigest digest.Digest, iss ImageSourceSeekable, pullOptions pullOptions) (*chunkedDiffer, bool, error) { + if !pullOptions.insecureAllowUnpredictableImageContents { // With no tar-split, we can't compute the traditional UncompressedDigest. + return nil, true, fmt.Errorf("estargz layers don't support partial pulls with guaranteed consistency with non-partial pulls") + } + manifest, tocOffset, err := readEstargzChunkedManifest(iss, blobSize, tocDigest) if err != nil { // If the error is a bad request to the server, then signal to the caller that it can try a different method. @@ -1150,7 +1163,10 @@ func (c *chunkedDiffer) findAndCopyFile(dirfd int, r *fileMetadata, copyOptions return false, nil } -func makeEntriesFlat(mergedEntries []fileMetadata) ([]fileMetadata, error) { +// makeEntriesFlat collects regular-file entries from mergedEntries, and produces a new list +// where each file content is only represented once, and uses composefs.RegularFilePathForValidatedDigest for its name. +// if flatPathNameMap is not nil, this function writes to it a mapping from filepath.Clean(originalName) to the composefs name. +func makeEntriesFlat(mergedEntries []fileMetadata, flatPathNameMap map[string]string) ([]fileMetadata, error) { var new []fileMetadata knownFlatPaths := make(map[string]struct{}) @@ -1169,6 +1185,9 @@ func makeEntriesFlat(mergedEntries []fileMetadata) ([]fileMetadata, error) { if err != nil { return nil, fmt.Errorf("determining physical file path for %q: %w", mergedEntries[i].Name, err) } + if flatPathNameMap != nil { + flatPathNameMap[filepath.Clean(mergedEntries[i].Name)] = path + } if _, known := knownFlatPaths[path]; known { continue @@ -1468,10 +1487,13 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff if err != nil { return output, &fs.PathError{Op: "open", Path: dest, Err: err} } - defer unix.Close(dirfd) + dirFile := os.NewFile(uintptr(dirfd), dest) + defer dirFile.Close() + var flatPathNameMap map[string]string // = nil if differOpts != nil && differOpts.Format == graphdriver.DifferOutputFormatFlat { - mergedEntries, err = makeEntriesFlat(mergedEntries) + flatPathNameMap = map[string]string{} + mergedEntries, err = makeEntriesFlat(mergedEntries, flatPathNameMap) if err != nil { return output, err } @@ -1763,6 +1785,38 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff } } + // To ensure that consumers of the layer who decompress and read the full tar stream, + // and consumers who consume the data via the TOC, both see exactly the same data and metadata, + // compute the UncompressedDigest. + // c/image will then ensure that this value matches the value in the image config’s RootFS.DiffID, i.e. the image must commit + // to one UncompressedDigest value for each layer, and that will avoid the ambiguity (in consumers who validate layers against DiffID). + // + // c/image also uses the UncompressedDigest as a layer ID, allowing it to use the traditional layer and image IDs. + // + // This is, sadly, quite costly: Up to now we might have only have had to write, and digest, only the new/modified files. + // Here we need to read, and digest, the whole layer, even if almost all of it was already present locally previously. + // + // Also, layers without a tar-split (estargz layers and old zstd:chunked layers) can't produce an UncompressedDigest that + // matches the expected RootFS.DiffID; they will need to use a special TOC-based layer ID, and we need to somehow (??) + // ensure unambiguity - either by always falling back to full pulls, or by never falling back. + if output.UncompressedDigest == "" { + switch { + case c.pullOptions.insecureAllowUnpredictableImageContents: + // Oh well. Skip the costly digest computation. + case output.TarSplit != nil: + metadata := tsStorage.NewJSONUnpacker(bytes.NewReader(output.TarSplit)) + fg := newStagedFileGetter(dirFile, flatPathNameMap) + digester := digest.Canonical.Digester() + if err := asm.WriteOutputTarStream(fg, metadata, digester.Hash()); err != nil { + return output, fmt.Errorf("digesting staged uncompressed stream: %w", err) + } + output.UncompressedDigest = digester.Digest() + default: + // We are checking for this earlier in GetDiffer, so this should not be reachable. + return output, fmt.Errorf(`internal error: layer's UncompressedDigest is unknown and "insecure_allow_unpredictable_image_contents" is not set`) + } + } + if totalChunksSize > 0 { logrus.Debugf("Missing %d bytes out of %d (%.2f %%)", missingPartsSize, totalChunksSize, float32(missingPartsSize*100.0)/float32(totalChunksSize)) } @@ -1888,3 +1942,33 @@ func validateChunkChecksum(chunk *internal.FileMetadata, root, path string, offs return digester.Digest() == digest } + +// newStagedFileGetter returns an object usable as storage.FileGetter for rootDir. +// if flatPathNameMap is not nil, it must be used to map logical file names into the backing file paths. +func newStagedFileGetter(rootDir *os.File, flatPathNameMap map[string]string) *stagedFileGetter { + return &stagedFileGetter{ + rootDir: rootDir, + flatPathNameMap: flatPathNameMap, + } +} + +type stagedFileGetter struct { + rootDir *os.File + flatPathNameMap map[string]string // nil, or a map from filepath.Clean()ed tar file names to expected on-filesystem names +} + +func (fg *stagedFileGetter) Get(filename string) (io.ReadCloser, error) { + if fg.flatPathNameMap != nil { + path, ok := fg.flatPathNameMap[filepath.Clean(filename)] + if !ok { + return nil, fmt.Errorf("no path mapping exists for tar entry %q", filename) + } + filename = path + } + pathFD, err := securejoin.OpenatInRoot(fg.rootDir, filename) + if err != nil { + return nil, err + } + defer pathFD.Close() + return securejoin.Reopen(pathFD, unix.O_RDONLY) +} diff --git a/storage.conf b/storage.conf index 962325233e..909e47b39b 100644 --- a/storage.conf +++ b/storage.conf @@ -80,6 +80,25 @@ additionalimagestores = [ # This is a "string bool": "false" | "true" (cannot be native TOML boolean) # convert_images = "false" +# This should ALMOST NEVER be set. +# It allows partial pulls ofof images without guaranteeing that "partial +# pulls" and non-partial pulls both result in consistent image contents. +# This allows pulling estargz images and early versions of zstd:chunked images; +# otherwise, these layers always use the traditional non-partial pull path. +# +# This option should be enabled EXTREMELY rarely, only if ALL images that could +# EVER be concievably pulled on this system are GUARANTEED (e.g. using a signature policy) +# to come from a build system trusted to never attack image integrity. +# +# If this consistency enforcement were disabled, malicious images could be built +# in a way designed to evade other audit mechanisms, so presence of most other audit +# mechanisms is not a replacement for the above-mentioned need for all images to come +# from a trusted build system. +# +# As a side effect, enabling this option will also make image IDs unpredictable +# (usually not equal to the traditional value matching the config digest). +# insecure_allow_unpredictable_image_contents = "false" + # Root-auto-userns-user is a user name which can be used to look up one or more UID/GID # ranges in the /etc/subuid and /etc/subgid file. These ranges will be partitioned # to containers configured to create automatically a user namespace. Containers