From 301031b3d3f7ee6a0d61900f55e90993be7463f6 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:04:49 +0100 Subject: [PATCH 01/30] docs(adr): fingerprint S3 buckets from a virtual tree Record the decision to stop laying bucket objects out by key on the operator's filesystem. Objects download to anonymous temp files, are hashed and deleted, and digest.VirtualDirSha256 computes the fingerprint from (key, sha256) pairs. Captures the compatibility contract, the shared key rule for content and metadata mode, the rejected CRC64 alternative, and the delivery slices. --- ...260911-s3-fingerprint-from-virtual-tree.md | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 docs/adr/20260911-s3-fingerprint-from-virtual-tree.md diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md new file mode 100644 index 000000000..ee159cb27 --- /dev/null +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -0,0 +1,60 @@ +--- +title: "20260911 - Fingerprint S3 buckets from a virtual tree; object keys never become local paths" +description: "Download each object to an anonymous temp file, hash it, delete it, and compute the directory fingerprint from (key, sha256) pairs so that no S3 key is ever used as a filename" +status: "Proposed" +date: "2026-09-11" +--- + +# 20260911 - Fingerprint S3 buckets from a virtual tree; object keys never become local paths + +## Overview + +`kosli snapshot s3` stops recreating the bucket as a directory on the operator's machine. Each object is downloaded to an anonymous temp file, hashed, and deleted. The fingerprint is computed by `digest.VirtualDirSha256` from `(key, sha256)` pairs, which reproduces `digest.DirSha256` byte for byte. Content mode and the metadata mode of #1069 become one pipeline that differs only in where each object's sha256 comes from. + +## Context + +Today `getS3DataFromClient` writes every object to `filepath.Join(tempDir, key)` and fingerprints the resulting tree with `DirSha256`. The object key, which anyone with write access to the bucket controls, therefore names a file on the operator's filesystem. + +PR #1155 fenced that key after a privately reported traversal: it rejects `..` segments, opens each destination with `O_EXCL` so two keys cannot share a file, and turns `ENOTDIR` into a readable error. It closes the reported hole, but the key still shapes a path, so a class of problems remains: + +- Two directories differing only in case merge on macOS and Windows. Unicode normalisation on macOS does the same. Both are fingerprint collisions between distinct bucket contents. +- Legitimate keys fail: `...` and `.. ` directories everywhere, and on Windows also `CON`, any colon, and a leading backslash, purely because Windows would misread the name on disk. +- A key component over 255 bytes fails with a bare `ENAMETOOLONG`. +- The fingerprint depends on the platform the snapshot runs on. A bucket with `A/x` and `a/y`, or with `d\e.txt`, fingerprints differently on Linux than on macOS or Windows. +- Around sixty lines of production code and most of the #1155 test table reason about Windows and macOS filesystem semantics that CI never executes. + +Separately, #1069 (`--fingerprint-source metadata`) built `VirtualDirSha256` to reproduce `DirSha256` without a filesystem, and its key rule (`validateVirtualPath`, strict `path.Clean` equality) disagrees with #1155's on-disk rule in both directions: `a//b`, `./c.txt` and `/lead.txt` are accepted on disk and rejected virtually; `...` is rejected on disk and accepted virtually. A bucket that snapshots today would break when the default flips. + +The fingerprint format itself is fixed. An S3 snapshot must match the fingerprint of the deployed directory attested earlier with `kosli attest artifact --artifact-type dir`, and every existing environment snapshot on the server was computed as `DirSha256` of the tree the key layout produced. So the change must preserve, for every bucket that snapshots successfully today, the identical fingerprint and artifact name. + +## Decision + +1. **The key never touches the filesystem.** Each object is downloaded to `os.CreateTemp(tempDir, "object-*")`, hashed with `FileSha256`, closed and removed. Peak disk drops from the bucket size to the in-flight objects. The transfer manager keeps working unchanged because `*os.File` is an `io.WriterAt`, and so does `FakeS3Client`. + +2. **The key becomes a virtual path by one rule, shared by both modes:** `path.Clean(strings.TrimLeft(key, "/"))`, rejecting a result of `.`, `..` or a leading `../`. This is exactly what `filepath.Join` produced on Linux, so `a//b`, `./c.txt` and `/lead.txt` land where they do today. Everything else, including `...`, `.. `, `CON`, colons and backslashes, is an ordinary name because nothing is created under it. The rule exists for fingerprint stability, not safety. + +3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped like `combineUnusableObjectErrors`, with the existing advice to use `--exclude-regex` or narrow the include filter. + +4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed with the same comment and whitespace handling as `excludePathsFromFile` and matched against virtual paths with a `**`-capable segment matcher. A matched directory drops its subtree. The ignore file can never exclude itself, as in `DirSha256`. Excluded and ignored objects are not downloaded at all. Equivalence with `DirSha256` on a materialised tree is asserted for every ignore-file case the digest tests already hold. + +5. **Downloads run in parallel** behind a bounded semaphore and a bytes-in-flight budget derived from listing sizes, with results written by index for determinism, a cancellable context so the first transport error stops in-flight multipart downloads, and per-key errors collected rather than aborting. + +6. **The switch is pinned, not argued.** Fingerprints of representative fake buckets are recorded against `main` before the implementation changes and asserted afterwards, alongside `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys` from #1155, which must stay green through the change. + +A throwaway equivalence test run on 2026-09-11 confirmed that `VirtualDirSha256` fed by rule 2 reproduces today's on-disk fingerprint for the #1155 unusual-keys bucket, the `a.txt` beside `a/z` ordering case, nested prefixes with folder markers, and the single-object case including its basename artifact name. + +## Alternatives considered + +- **Keep #1155's fenced layout and add rules for the remaining cases.** Each remaining case (case folding, Unicode normalisation, component length) needs another platform-specific rule and none can be exercised in Linux CI. The layout is the cause, so fencing it further does not converge. +- **Temp files alone, still hashing the on-disk tree.** Not viable: `DirSha256` hashes every entry's basename, so renaming files changes the fingerprint. +- **A CRC64- or ETag-derived fingerprint to avoid downloading.** S3 stores a full-object CRC64NVME for every object uploaded since December 2024, which makes it tempting. Rejected: CRC64 is linear and trivially forgeable, so an attacker who can write the bucket can replace approved content while keeping the fingerprint, which defeats the purpose of a compliance fingerprint. Hashing the CRC with SHA256 adds no resistance. ETag is MD5 of parts for multipart uploads and depends on client chunk size. +- **Incremental snapshots keyed on stored checksums.** Only `VersionId` is a trustworthy skip signal and only with versioning enabled; the design needs persisted state the stateless CLI does not have. Left for a separate decision. + +## Consequences + +- The fingerprint is the same on every operating system, with Linux semantics. A snapshot run on Windows or macOS against a bucket with case-colliding or backslash keys moves to the Linux value. This is a correction and is release-noted. +- `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. The codebase ends with less code than before #1155. +- `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. +- Excluded and ignored objects are not downloaded, and disk use is bounded by the in-flight budget rather than the bucket size. +- #1069 rebases onto the shared layer: metadata mode becomes a sha256 source plugged into the same list, normalise, exclude, tree pipeline, its key rule disappears in favour of rule 2, and its rejection of buckets with a root `.kosli_ignore` becomes a download of that one object. +- Delivery is sliced in `TODO.md` (local, gitignored): lift `VirtualDirSha256` with its tests; add the key rule and collision errors; add virtual ignore rules; switch content mode to temp files sequentially with pinned fingerprints; parallelise. From fa159976426df52f2d02f57d2dda03c1ebda05eb Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:04:55 +0100 Subject: [PATCH 02/30] docs(adr): check for ".." segments before cleaning the S3 key path.Clean folds a/../b onto b silently, so the rejection has to run on the raw key. Also spell out that the rule exists for fingerprint compatibility, not safety: DirSha256 can never produce a tree holding ".", ".." or an empty name. --- docs/adr/20260911-s3-fingerprint-from-virtual-tree.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index ee159cb27..9336a9c50 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -31,7 +31,7 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin 1. **The key never touches the filesystem.** Each object is downloaded to `os.CreateTemp(tempDir, "object-*")`, hashed with `FileSha256`, closed and removed. Peak disk drops from the bucket size to the in-flight objects. The transfer manager keeps working unchanged because `*os.File` is an `io.WriterAt`, and so does `FakeS3Client`. -2. **The key becomes a virtual path by one rule, shared by both modes:** `path.Clean(strings.TrimLeft(key, "/"))`, rejecting a result of `.`, `..` or a leading `../`. This is exactly what `filepath.Join` produced on Linux, so `a//b`, `./c.txt` and `/lead.txt` land where they do today. Everything else, including `...`, `.. `, `CON`, colons and backslashes, is an ordinary name because nothing is created under it. The rule exists for fingerprint stability, not safety. +2. **The key becomes a virtual path by one rule, shared by both modes.** A key holding a `..` segment is rejected first, on the raw key, so `a/../b` cannot fold silently onto `b`. The rest is `path.Clean(strings.TrimLeft(key, "/"))`, rejecting a result of `.`. The fold is exactly what `filepath.Join` produced on Linux, so `a//b`, `./c.txt` and `/lead.txt` land where they do today. Everything else, including `...`, `.. `, `CON`, colons and backslashes, is an ordinary name because nothing is created under it. None of this is for safety. `DirSha256` walks real directories, which can never hold an entry named `.`, `..` or the empty string, so the rule keeps virtual fingerprints inside the space an attested directory can match, and preserves the fingerprints existing buckets already have. 3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped like `combineUnusableObjectErrors`, with the existing advice to use `--exclude-regex` or narrow the include filter. From ca5f7cc422c69238908a9f5dccf6f2b732494e58 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:04:56 +0100 Subject: [PATCH 03/30] docs(adr): state the compatibility and no-silent-loss guarantees Name the three properties the virtual-tree change must hold, snapshot equals attestation, snapshot equals its own history, and no object is dropped silently, together with the test that holds each one. --- docs/adr/20260911-s3-fingerprint-from-virtual-tree.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index 9336a9c50..d3f1dcbd3 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -43,6 +43,14 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin A throwaway equivalence test run on 2026-09-11 confirmed that `VirtualDirSha256` fed by rule 2 reproduces today's on-disk fingerprint for the #1155 unusual-keys bucket, the `a.txt` beside `a/z` ordering case, nested prefixes with folder markers, and the single-object case including its basename artifact name. +## Guarantees + +The attest side is unchanged: `kosli attest artifact --artifact-type dir` still runs `DirSha256` on a real directory, and that defines the format. The snapshot side re-derives the same value from the bucket. Three guarantees follow, each with the test that holds it: + +- **Snapshot equals attestation.** `VirtualDirSha256` of the bucket's `(path, sha256)` pairs equals `DirSha256` of the directory those objects were uploaded from, and a single object equals `FileSha256` plus its basename. Held by the materialise-then-compare tests in `internal/digest` and an end-to-end test in `internal/aws` that hashes a directory, uploads its files to the fake bucket and snapshots it. +- **Snapshot equals its own history.** Every bucket that snapshots successfully on `main` keeps its fingerprint and artifact name, because the fold rule is what `filepath.Join` did. Held by fingerprints recorded before the switch and by `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys`. +- **No object is lost silently.** Every key S3 accepts is representable, whatever the operating system, because the key never becomes a path. The only rejections are keys that cannot form a directory tree at all: a `..` segment, two keys folding onto one path, and an object that is also a prefix. S3 permits those, no filesystem does, so no attested directory could match them. Each rejection fails the snapshot and names every key involved; the manifest is never shortened to make a fingerprint. Held by a generated-key representability test and a manifest-count invariant test. + ## Alternatives considered - **Keep #1155's fenced layout and add rules for the remaining cases.** Each remaining case (case folding, Unicode normalisation, component length) needs another platform-specific rule and none can be exercised in Linux CI. The layout is the cause, so fencing it further does not converge. From 04aff5270b51e27785a120d3f8be919807871326 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:04:58 +0100 Subject: [PATCH 04/30] feat(digest): add VirtualDirSha256 for fingerprinting a tree without a filesystem VirtualDirSha256 reproduces DirSha256 from (path, sha256) pairs alone: it builds the tree, walks it in filepath.WalkDir order and hashes each entry's base name plus each file's content digest. SingleVirtualFile mirrors containsSingleFile so a one-file tree can take the FileSha256 branch. Every equivalence test materialises the tree on disk and requires DirSha256 to agree, so the two cannot drift silently. No callers yet. This is the first slice of moving kosli snapshot s3 off key-named local files (see docs/adr/20260911-s3-fingerprint-from-virtual-tree.md). --- internal/digest/virtualdir.go | 176 ++++++++++++++++++ internal/digest/virtualdir_test.go | 283 +++++++++++++++++++++++++++++ 2 files changed, 459 insertions(+) create mode 100644 internal/digest/virtualdir.go create mode 100644 internal/digest/virtualdir_test.go diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go new file mode 100644 index 000000000..c30f39121 --- /dev/null +++ b/internal/digest/virtualdir.go @@ -0,0 +1,176 @@ +package digest + +import ( + "crypto/sha256" + "encoding/hex" + "fmt" + "hash" + "path" + "sort" + "strings" + + "github.com/kosli-dev/cli/internal/logger" +) + +// VirtualFile is one file in a virtual directory tree: a slash-separated path +// relative to the tree root, plus the hex sha256 of the file's content. +type VirtualFile struct { + // Path is relative to the tree root, slash-separated, with no leading or + // trailing slash and no "." or ".." segments (e.g. "dummy/template.yml"). + Path string + // Sha256 is the hex-encoded sha256 of the file content. + Sha256 string +} + +// Name returns the last segment of the file's path. +func (f VirtualFile) Name() string { + return path.Base(f.Path) +} + +// SingleVirtualFile reports whether the tree holds exactly one file, and +// returns it. +// +// This mirrors what containsSingleFile decides for a tree on disk: a tree built +// only from file paths has no empty directories, so a single leaf means every +// level has exactly one child, and two distinct leaves must diverge at some +// node and give it two children. Counting the files is therefore equivalent to +// walking the tree, and callers can pick the FileSha256 branch on len == 1. +func SingleVirtualFile(files []VirtualFile) (VirtualFile, bool) { + if len(files) != 1 { + return VirtualFile{}, false + } + return files[0], true +} + +// VirtualDirSha256 returns the fingerprint DirSha256 would return for a +// directory containing exactly these files, without touching the disk. +// +// It reproduces calculateDirContentSha256 exactly: walk the tree in +// filepath.WalkDir order -- which is lexical by name within each directory, +// depth-first, with directories and files interleaved -- and append, for every +// entry, the hex sha256 of its base name, plus for files the hex sha256 of +// their content. The fingerprint is the sha256 of that concatenation. +// +// Note that the tree has to be built before sorting: object stores list keys in +// byte order of the whole key, and '.' (0x2E) sorts before '/' (0x2F), so keys +// "a.txt" and "a/z" list as [a.txt, a/z] while WalkDir yields [a, a/z, a.txt]. +// Sorting the flat path list instead of the tree produces a different digest +// whenever a directory shares a name prefix with a sibling file. +// +// .kosli_ignore is deliberately not handled here: reading it needs the file's +// content, which the caller may not have, so exclusions stay the caller's +// concern. +func VirtualDirSha256(files []VirtualFile, logger *logger.Logger) (string, error) { + if len(files) == 0 { + return "", fmt.Errorf("cannot calculate a fingerprint: no files were provided") + } + + root, err := buildVirtualTree(files) + if err != nil { + return "", err + } + + logger.Debug("calculating fingerprint for a virtual tree of %d files", len(files)) + hasher := sha256.New() + root.writeDigests(hasher, logger) + return hex.EncodeToString(hasher.Sum(nil)), nil +} + +// virtualNode is a directory or a file in the virtual tree. Files are leaves +// and carry a content digest; directories carry children keyed by base name. +type virtualNode struct { + name string + sha256 string + isDir bool + children map[string]*virtualNode +} + +// buildVirtualTree turns a flat list of files into a tree, rejecting anything +// that cannot be represented as one: unclean paths, duplicates, and names used +// as both a file and a directory. +func buildVirtualTree(files []VirtualFile) (*virtualNode, error) { + root := &virtualNode{isDir: true, children: map[string]*virtualNode{}} + + for _, file := range files { + if err := validateVirtualPath(file.Path); err != nil { + return nil, err + } + if err := ValidateDigest(file.Sha256); err != nil { + return nil, fmt.Errorf("invalid fingerprint for %q: %w", file.Path, err) + } + + segments := strings.Split(file.Path, "/") + parent := root + for i, segment := range segments[:len(segments)-1] { + child, ok := parent.children[segment] + if !ok { + child = &virtualNode{name: segment, isDir: true, children: map[string]*virtualNode{}} + parent.children[segment] = child + } + if !child.isDir { + return nil, fmt.Errorf("path %q is both a file and a directory", + strings.Join(segments[:i+1], "/")) + } + parent = child + } + + name := segments[len(segments)-1] + if existing, ok := parent.children[name]; ok { + if existing.isDir { + return nil, fmt.Errorf("path %q is both a file and a directory", file.Path) + } + return nil, fmt.Errorf("duplicate path %q", file.Path) + } + parent.children[name] = &virtualNode{name: name, sha256: file.Sha256} + } + + return root, nil +} + +// writeDigests appends this node's children to the hash in WalkDir order. +func (n *virtualNode) writeDigests(hasher hash.Hash, logger *logger.Logger) { + for _, name := range n.sortedChildNames() { + child := n.children[name] + nameSha256 := sha256OfString(child.name) + hasher.Write([]byte(nameSha256)) //nolint:errcheck // hash.Hash never returns an error + + if child.isDir { + logger.Debug("dir: %s -- dirname digest: %s", child.name, nameSha256) + child.writeDigests(hasher, logger) + continue + } + logger.Debug("file: %s -- filename digest: %s -- content digest: %s", + child.name, nameSha256, child.sha256) + hasher.Write([]byte(child.sha256)) //nolint:errcheck // hash.Hash never returns an error + } +} + +// sortedChildNames returns child names in the byte order os.ReadDir uses, so +// directories and files interleave exactly as filepath.WalkDir visits them. +func (n *virtualNode) sortedChildNames() []string { + names := make([]string, 0, len(n.children)) + for name := range n.children { + names = append(names, name) + } + sort.Strings(names) + return names +} + +// validateVirtualPath rejects paths that cannot be mapped onto a directory tree +// unambiguously. path.Clean collapses "a//b" to "a/b" and resolves "." and +// "..", so a path that differs from its cleaned form would silently collide +// with, or escape, another entry. +func validateVirtualPath(p string) error { + if p == "" || p != path.Clean(p) || path.IsAbs(p) || strings.HasPrefix(p, "../") || p == ".." { + return fmt.Errorf("path %q is not a clean relative path: it must not be empty, absolute, "+ + "or contain empty, \".\" or \"..\" segments", p) + } + return nil +} + +// sha256OfString returns the hex sha256 of s. DirSha256 hashes an entry's name +// by writing it to a file and hashing that file, which is the same bytes. +func sha256OfString(s string) string { + sum := sha256.Sum256([]byte(s)) + return hex.EncodeToString(sum[:]) +} diff --git a/internal/digest/virtualdir_test.go b/internal/digest/virtualdir_test.go new file mode 100644 index 000000000..93ffd14c8 --- /dev/null +++ b/internal/digest/virtualdir_test.go @@ -0,0 +1,283 @@ +package digest + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/utils" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type VirtualDirTestSuite struct { + suite.Suite + tmpDir string +} + +func (suite *VirtualDirTestSuite) SetupTest() { + suite.tmpDir = suite.T().TempDir() +} + +// TestVirtualDirSha256MatchesDirSha256 is the test that matters: for each tree, +// materialise it on disk, fingerprint it with DirSha256, then fingerprint the +// same (path, content sha256) pairs with VirtualDirSha256 and require the two +// to be identical. Anything VirtualDirSha256 gets wrong about walk order, name +// hashing or nesting shows up here as a mismatch. +func (suite *VirtualDirTestSuite) TestVirtualDirSha256MatchesDirSha256() { + for _, t := range []struct { + name string + files map[string]string // path relative to the tree root -> content + }{ + { + name: "a single file at the root", + files: map[string]string{"README.md": "# readme\n"}, + }, + { + name: "two files at the root", + files: map[string]string{"README.md": "# readme\n", "notes.txt": "notes\n"}, + }, + { + name: "nested directories", + files: map[string]string{ + "README.md": "# readme\n", + "dummy/dummy_2/template.yml": "key: value\n", + "dummy/other.txt": "other\n", + }, + }, + { + // '.' (0x2E) sorts before '/' (0x2F), so a flat sort of the keys + // gives a.txt, a/z -- while WalkDir gives a, a/z, a.txt. Sorting + // the key list instead of the tree fails exactly here. + name: "a dir name sorting between two file names", + files: map[string]string{ + "a.txt": "a\n", + "a/z": "z\n", + "b.txt": "b\n", + }, + }, + { + name: "a deep single-child chain", + files: map[string]string{"a/b/c/d/e/f.txt": "deep\n"}, + }, + { + name: "dot-prefixed and unicode names", + files: map[string]string{ + ".hidden": "hidden\n", + "ünïcode.txt": "unicode\n", + "dir/.keep": "", + "dir/naïve.md": "naive\n", + }, + }, + { + name: "an empty file", + files: map[string]string{"empty.txt": "", "other.txt": "x\n"}, + }, + { + name: "many files across several levels", + files: map[string]string{ + "a.txt": "a\n", "b.txt": "b\n", "c/d.txt": "d\n", "c/e.txt": "e\n", + "c/f/g.txt": "g\n", "c/f/h.txt": "h\n", "i/j.txt": "j\n", + }, + }, + } { + suite.Run(t.name, func() { + root := suite.T().TempDir() + virtualFiles := make([]VirtualFile, 0, len(t.files)) + for path, content := range t.files { + suite.createFile(filepath.Join(root, filepath.FromSlash(path)), content) + virtualFiles = append(virtualFiles, VirtualFile{ + Path: path, + Sha256: sha256OfString(content), + }) + } + + want, err := DirSha256(root, []string{}, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + got, err := VirtualDirSha256(virtualFiles, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + require.Equal(suite.T(), want, got, + "VirtualDirSha256 should equal DirSha256 of the same tree") + }) + } +} + +// TestVirtualDirSha256IgnoresInputOrder pins that the result depends on the tree, +// not on the order S3 happened to list the objects in. +func (suite *VirtualDirTestSuite) TestVirtualDirSha256IgnoresInputOrder() { + files := []VirtualFile{ + {Path: "c/f/g.txt", Sha256: sha256OfString("g")}, + {Path: "a.txt", Sha256: sha256OfString("a")}, + {Path: "c/d.txt", Sha256: sha256OfString("d")}, + {Path: "b.txt", Sha256: sha256OfString("b")}, + } + reversed := make([]VirtualFile, len(files)) + for i, f := range files { + reversed[len(files)-1-i] = f + } + + first, err := VirtualDirSha256(files, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + second, err := VirtualDirSha256(reversed, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + require.Equal(suite.T(), first, second) +} + +func (suite *VirtualDirTestSuite) TestVirtualDirSha256Errors() { + validSha := sha256OfString("x") + for _, t := range []struct { + name string + files []VirtualFile + wantErrMsg string + }{ + { + name: "no files", + files: []VirtualFile{}, + wantErrMsg: "no files", + }, + { + name: "a duplicate path", + files: []VirtualFile{ + {Path: "a.txt", Sha256: validSha}, + {Path: "a.txt", Sha256: validSha}, + }, + wantErrMsg: "duplicate path", + }, + { + name: "a path used as both file and directory", + files: []VirtualFile{ + {Path: "a", Sha256: validSha}, + {Path: "a/b", Sha256: validSha}, + }, + wantErrMsg: "both a file and a directory", + }, + { + name: "a path used as both directory and file", + files: []VirtualFile{ + {Path: "a/b", Sha256: validSha}, + {Path: "a", Sha256: validSha}, + }, + wantErrMsg: "both a file and a directory", + }, + { + name: "an empty path segment", + files: []VirtualFile{{Path: "a//b", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "a parent directory segment", + files: []VirtualFile{{Path: "../evil", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "a current directory segment", + files: []VirtualFile{{Path: "a/./b", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "a leading slash", + files: []VirtualFile{{Path: "/a.txt", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "a trailing slash", + files: []VirtualFile{{Path: "a/", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "an empty path", + files: []VirtualFile{{Path: "", Sha256: validSha}}, + wantErrMsg: "not a clean relative path", + }, + { + name: "an invalid sha256", + files: []VirtualFile{{Path: "a.txt", Sha256: "not-a-digest"}}, + wantErrMsg: "not a valid SHA256 fingerprint", + }, + { + name: "an uppercase sha256", + files: []VirtualFile{{Path: "a.txt", Sha256: strings.ToUpper(validSha)}}, + wantErrMsg: "not a valid SHA256 fingerprint", + }, + } { + suite.Run(t.name, func() { + _, err := VirtualDirSha256(t.files, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), t.wantErrMsg) + }) + } +} + +func (suite *VirtualDirTestSuite) TestSingleVirtualFile() { + validSha := sha256OfString("x") + for _, t := range []struct { + name string + files []VirtualFile + wantOK bool + wantBase string + }{ + { + name: "one file at the root", + files: []VirtualFile{{Path: "README.md", Sha256: validSha}}, + wantOK: true, + wantBase: "README.md", + }, + { + name: "one file nested under prefixes keeps its base name", + files: []VirtualFile{{Path: "dummy/dummy_2/template.yml", Sha256: validSha}}, + wantOK: true, + wantBase: "template.yml", + }, + { + name: "two files is not a single file", + files: []VirtualFile{ + {Path: "a.txt", Sha256: validSha}, + {Path: "b.txt", Sha256: validSha}, + }, + wantOK: false, + }, + { + name: "no files is not a single file", + files: []VirtualFile{}, + wantOK: false, + }, + } { + suite.Run(t.name, func() { + file, ok := SingleVirtualFile(t.files) + require.Equal(suite.T(), t.wantOK, ok) + if t.wantOK { + require.Equal(suite.T(), t.wantBase, file.Name()) + } + }) + } +} + +// TestSingleFileMatchesFileSha256 pins the equivalence the aws package relies on: +// a one-object snapshot is fingerprinted as that file's content digest, exactly +// as content mode does via containsSingleFile + FileSha256. +func (suite *VirtualDirTestSuite) TestSingleFileMatchesFileSha256() { + content := "the only object\n" + path := filepath.Join(suite.tmpDir, "only.txt") + suite.createFile(path, content) + + want, err := FileSha256(path, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + file, ok := SingleVirtualFile([]VirtualFile{{Path: "nested/only.txt", Sha256: sha256OfString(content)}}) + require.True(suite.T(), ok) + require.Equal(suite.T(), want, file.Sha256) +} + +// createFile writes content to path, creating parent directories as needed. +func (suite *VirtualDirTestSuite) createFile(path, content string) { + suite.T().Helper() + require.NoError(suite.T(), utils.CreateFileWithContent(path, content)) +} + +func TestVirtualDirTestSuite(t *testing.T) { + suite.Run(t, new(VirtualDirTestSuite)) +} From e17a5104175905444a4c08233c16db06ec53d1f4 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:05:00 +0100 Subject: [PATCH 05/30] feat(snapshot s3): map object keys to virtual paths and report every colliding key virtualPathForS3Key turns an object key into the path it occupies in the virtual tree that will be fingerprinted. A ".." segment is rejected on the raw key, before path.Clean can fold it onto a sibling; a leading slash, "." segments and doubled slashes fold exactly as filepath.Join did, so existing fingerprints are unchanged. Nothing is created under the path, so reserved names, colons, backslashes and overlong components are ordinary names and there is no per-OS branch. virtualPathsForS3Keys validates a whole key set at once and reports every problem together: rejected keys, keys folding onto one path, and an object whose path is also a directory holding other objects, capped at ten. No callers yet; content mode switches to it in a later slice. --- internal/aws/s3_keys.go | 138 +++++++++++++++++++++ internal/aws/s3_keys_test.go | 228 +++++++++++++++++++++++++++++++++++ 2 files changed, 366 insertions(+) create mode 100644 internal/aws/s3_keys.go create mode 100644 internal/aws/s3_keys_test.go diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go new file mode 100644 index 000000000..9b7e1a3d4 --- /dev/null +++ b/internal/aws/s3_keys.go @@ -0,0 +1,138 @@ +package aws + +import ( + "fmt" + "path" + "sort" + "strings" +) + +// maxReportedS3KeyProblems caps how many keys one error lists before +// summarising the rest, so a bucket-wide problem stays readable. +const maxReportedS3KeyProblems = 10 + +// virtualPathForS3Key returns the path an object key occupies in the virtual +// tree that is fingerprinted, or rejects a key no directory tree can hold. +// +// Nothing is ever created under the returned path, so the operating system's +// naming rules do not apply: reserved names, colons, backslashes and overlong +// components are all ordinary names here. The fold of "." segments, doubled +// slashes and a leading slash is what filepath.Join did when objects were +// written under their keys, and keeps existing fingerprints unchanged. +// +// A ".." segment is checked on the raw key rather than left to path.Clean, +// which would fold "a/../b" onto "b" silently. DirSha256 walks real +// directories, which never hold an entry named ".", ".." or "", so rejecting +// these keeps every virtual fingerprint inside the space an attested directory +// can match. +func virtualPathForS3Key(key string) (string, error) { + trimmed := strings.TrimLeft(key, "/") + if trimmed == "" { + return "", s3KeyProblem(key, "names no file") + } + if strings.HasSuffix(trimmed, "/") { + return "", s3KeyProblem(key, "is a folder marker, not an object") + } + for _, segment := range strings.Split(trimmed, "/") { + if segment == ".." { + return "", s3KeyProblem(key, `contains a ".." segment`) + } + } + cleaned := path.Clean(trimmed) + if cleaned == "." { + return "", s3KeyProblem(key, "names no file") + } + return cleaned, nil +} + +// virtualPathsForS3Keys maps every object key to its virtual path, or reports +// every key that cannot take part in one directory tree: keys the rule above +// rejects, keys that fold onto the same path, and an object whose path is also +// a directory holding other objects. All problems are reported together so one +// run tells the operator about every key they need to act on. +// +// Folder markers (keys ending in "/") are the caller's to filter out first. +func virtualPathsForS3Keys(keys []string) (map[string]string, error) { + paths := make(map[string]string, len(keys)) + keysByPath := map[string][]string{} + problems := []string{} + + for _, key := range keys { + virtualPath, err := virtualPathForS3Key(key) + if err != nil { + problems = append(problems, err.Error()) + continue + } + paths[key] = virtualPath + keysByPath[virtualPath] = append(keysByPath[virtualPath], key) + } + + for virtualPath, colliding := range keysByPath { + if len(colliding) > 1 { + sort.Strings(colliding) + problems = append(problems, fmt.Sprintf("object keys %s fingerprint as the same path [%s]", + bracketed(colliding), virtualPath)) + } + } + + // The lexically smallest key under each directory stands as the example in + // the message, so the report does not depend on listing order. + exampleObjectUnder := map[string]string{} + for virtualPath, keys := range keysByPath { + sort.Strings(keys) + for dir := path.Dir(virtualPath); dir != "."; dir = path.Dir(dir) { + if existing, ok := exampleObjectUnder[dir]; !ok || keys[0] < existing { + exampleObjectUnder[dir] = keys[0] + } + } + } + for virtualPath, keys := range keysByPath { + child, isAlsoDir := exampleObjectUnder[virtualPath] + if !isAlsoDir { + continue + } + for _, key := range keys { + problems = append(problems, fmt.Sprintf("object key [%s] fingerprints as [%s], which is also a directory holding object key [%s]", + key, virtualPath, child)) + } + } + + if len(problems) > 0 { + return nil, s3KeyProblemsError(problems) + } + return paths, nil +} + +// s3KeyProblem describes a failure the key itself causes. The advice belongs +// only on such failures: suggesting exclusion for a machine fault would drop a +// legitimate object from the snapshot. +func s3KeyProblem(key, reason string) error { + return fmt.Errorf("object key [%s] cannot be fingerprinted: %s", key, reason) +} + +// s3KeyProblemsError joins every key problem into one error with the remedy. +func s3KeyProblemsError(problems []string) error { + // Map iteration supplied these in any order; sorting keeps the message stable. + sort.Strings(problems) + if len(problems) == 1 { + return fmt.Errorf("%s; exclude it with --exclude-regex, or narrow the include filter if one is set", problems[0]) + } + + shown := problems + suffix := "" + if len(shown) > maxReportedS3KeyProblems { + shown = shown[:maxReportedS3KeyProblems] + suffix = fmt.Sprintf("\n(and %d more)", len(problems)-maxReportedS3KeyProblems) + } + return fmt.Errorf("%d object keys cannot be fingerprinted:\n%s%s\nexclude them with --exclude-regex, or narrow the include filter if one is set", + len(problems), strings.Join(shown, "\n"), suffix) +} + +// bracketed formats keys as "[a], [b]" for error messages. +func bracketed(keys []string) string { + parts := make([]string, len(keys)) + for i, key := range keys { + parts[i] = "[" + key + "]" + } + return strings.Join(parts, ", ") +} diff --git a/internal/aws/s3_keys_test.go b/internal/aws/s3_keys_test.go new file mode 100644 index 000000000..4630c2a54 --- /dev/null +++ b/internal/aws/s3_keys_test.go @@ -0,0 +1,228 @@ +package aws + +import ( + "fmt" + "math/rand" + "path" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type S3KeysTestSuite struct { + suite.Suite +} + +// Accepted keys land on exactly the path filepath.Join(dir, key) produced on +// Linux before objects stopped being written under their own names, so every +// bucket that fingerprints today keeps its fingerprint. +func (suite *S3KeysTestSuite) TestVirtualPathForS3Key() { + longComponent := strings.Repeat("n", 300) + for _, t := range []struct { + name string + key string + wantPath string + wantErrMsg string + }{ + {name: "an ordinary nested key", key: "protected/release.bin", wantPath: "protected/release.bin"}, + {name: "a plain filename", key: "a.txt", wantPath: "a.txt"}, + {name: "a short nested key", key: "a/z", wantPath: "a/z"}, + {name: "a dotfile", key: ".kosli_ignore", wantPath: ".kosli_ignore"}, + {name: "a key with spaces", key: "file with spaces.txt", wantPath: "file with spaces.txt"}, + {name: "a key with punctuation", key: "weird!*'().txt", wantPath: "weird!*'().txt"}, + {name: "a unicode key", key: "ünïcödé/файл.txt", wantPath: "ünïcödé/файл.txt"}, + {name: "a dot followed by a space is a literal name", key: ". ", wantPath: ". "}, + {name: "a name that merely starts with two dots", key: "..hidden", wantPath: "..hidden"}, + {name: "a leading slash is trimmed", key: "/etc/passwd", wantPath: "etc/passwd"}, + {name: "doubled leading slashes are trimmed", key: "//x", wantPath: "x"}, + {name: "a leading dot segment is folded", key: "./a.txt", wantPath: "a.txt"}, + {name: "a doubled interior slash is folded", key: "a//b", wantPath: "a/b"}, + {name: "a dot segment is folded", key: "a/./b", wantPath: "a/b"}, + // Nothing is created under these names, so the operating system's rules + // about them no longer apply. + {name: "a backslash is a literal character", key: `dir\file.txt`, wantPath: `dir\file.txt`}, + {name: "a leading backslash is a literal character", key: `\evil.txt`, wantPath: `\evil.txt`}, + {name: "a backslash-separated \"..\" is a literal name", key: `uploads/user-a/..\..\x`, wantPath: `uploads/user-a/..\..\x`}, + {name: "a reserved Windows name", key: "CON", wantPath: "CON"}, + {name: "a drive-looking segment", key: "C:evil", wantPath: "C:evil"}, + {name: "a colon segment", key: "a:b", wantPath: "a:b"}, + {name: "three dots", key: "a/.../b", wantPath: "a/.../b"}, + {name: "two dots and a space", key: "a/.. /x", wantPath: "a/.. /x"}, + {name: "a component longer than any filesystem allows", key: "dir/" + longComponent, wantPath: "dir/" + longComponent}, + {name: "a key of the maximum S3 length", key: strings.Repeat("s/", 511) + "ab", wantPath: strings.Repeat("s/", 511) + "ab"}, + // Rejected: these cannot be entries of any directory tree. + {name: "a traversing key", key: "uploads/user-a/../../protected/release.bin", wantErrMsg: `contains a ".." segment`}, + {name: "a bare \"..\"", key: "..", wantErrMsg: `contains a ".." segment`}, + {name: "a leading \"..\"", key: "../x", wantErrMsg: `contains a ".." segment`}, + {name: "a trailing \"..\"", key: "a/..", wantErrMsg: `contains a ".." segment`}, + {name: "a \"..\" that would fold onto a sibling", key: "a/../b", wantErrMsg: `contains a ".." segment`}, + {name: "an empty key", key: "", wantErrMsg: "names no file"}, + {name: "a bare slash", key: "/", wantErrMsg: "names no file"}, + {name: "doubled slashes with nothing else", key: "//", wantErrMsg: "names no file"}, + {name: "a bare dot", key: ".", wantErrMsg: "names no file"}, + {name: "a dot slash dot", key: "./.", wantErrMsg: "names no file"}, + {name: "a folder marker", key: "dir/", wantErrMsg: "folder marker"}, + {name: "a folder marker with a dot segment", key: "a/./", wantErrMsg: "folder marker"}, + } { + suite.Run(t.name, func() { + got, err := virtualPathForS3Key(t.key) + if t.wantErrMsg != "" { + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", t.key)) + require.Contains(suite.T(), err.Error(), t.wantErrMsg) + return + } + require.NoError(suite.T(), err) + require.Equal(suite.T(), t.wantPath, got) + }) + } +} + +// Any key S3 accepts is representable unless it holds a ".." segment, whatever +// operating system runs the snapshot. Components are drawn from an alphabet with +// no '/' and are never exactly "..", so every generated key must be accepted. +func (suite *S3KeysTestSuite) TestEveryS3KeyIsRepresentable() { + alphabet := []rune("abcXYZ019 !-_.*'()&$@=;:+,?\\\t\x01ünï文файл") + random := rand.New(rand.NewSource(20260911)) + component := func() string { + for { + length := 1 + random.Intn(40) + runes := make([]rune, length) + for i := range runes { + runes[i] = alphabet[random.Intn(len(alphabet))] + } + if s := string(runes); s != ".." { + return s + } + } + } + + for i := 0; i < 2000; i++ { + depth := 1 + random.Intn(6) + components := make([]string, depth) + for j := range components { + components[j] = component() + } + if components[0] == "." { + components[0] = "x" + } + key := strings.Join(components, "/") + if len(key) > 1024 { + continue + } + + got, err := virtualPathForS3Key(key) + require.NoError(suite.T(), err, "key %q must be representable", key) + require.Equal(suite.T(), path.Clean(key), got) + require.NotContains(suite.T(), strings.Split(got, "/"), "..") + } +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysMapsEveryKey() { + got, err := virtualPathsForS3Keys([]string{"README.md", "/lead.txt", "a//b", "./c.txt", `d\e.txt`}) + require.NoError(suite.T(), err) + require.Equal(suite.T(), map[string]string{ + "README.md": "README.md", + "/lead.txt": "lead.txt", + "a//b": "a/b", + "./c.txt": "c.txt", + `d\e.txt`: `d\e.txt`, + }, got) +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysNamesEveryCollidingKey() { + _, err := virtualPathsForS3Keys([]string{"x", "a/b", "a//b", "./a/b"}) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "[./a/b]") + require.Contains(suite.T(), err.Error(), "[a//b]") + require.Contains(suite.T(), err.Error(), "[a/b]") + require.Contains(suite.T(), err.Error(), "--exclude-regex") + require.NotContains(suite.T(), err.Error(), "[x]", "an unaffected key must not be named") +} + +// An object "a" beside objects under "a/" is legal in S3 and impossible in a +// directory tree, whichever order S3 lists them in. +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysObjectAndPrefix() { + for _, keys := range [][]string{ + {"a", "a/b"}, + {"a/b", "a"}, + {"a/b/c", "a/b", "z"}, + {"lib", "lib/x", "lib/y"}, + } { + suite.Run(strings.Join(keys, ","), func() { + _, err := virtualPathsForS3Keys(keys) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "also a directory") + require.Contains(suite.T(), err.Error(), "--exclude-regex") + }) + } + + _, err := virtualPathsForS3Keys([]string{"a", "a/b"}) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "object key [a] fingerprints as [a], which is also a directory holding object key [a/b]") +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysReportsEveryBadKey() { + _, err := virtualPathsForS3Keys([]string{"ok.txt", "../one", "two/..", "", "dup", "./dup"}) + require.Error(suite.T(), err) + msg := err.Error() + require.Contains(suite.T(), msg, "4 object keys cannot be fingerprinted") + require.Contains(suite.T(), msg, "[../one]") + require.Contains(suite.T(), msg, "[two/..]") + require.Contains(suite.T(), msg, "object key []") + require.Contains(suite.T(), msg, "[./dup]") + require.Contains(suite.T(), msg, "--exclude-regex") + require.NotContains(suite.T(), msg, "[ok.txt]") +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysCapsTheReport() { + keys := make([]string, 0, 13) + for i := 0; i < 13; i++ { + keys = append(keys, fmt.Sprintf("%02d/../x", i)) + } + _, err := virtualPathsForS3Keys(keys) + require.Error(suite.T(), err) + msg := err.Error() + require.Contains(suite.T(), msg, "13 object keys cannot be fingerprinted") + require.Contains(suite.T(), msg, "(and 3 more)") + require.Equal(suite.T(), maxReportedS3KeyProblems, strings.Count(msg, "object key [")) +} + +// The reported attack shape from #1155: the traversing key is rejected for its +// ".." segment, and is never allowed to fold onto the object it names. +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysTraversalKeyIsRejected() { + _, err := virtualPathsForS3Keys([]string{ + "protected/release.bin", + "uploads/user-a/../../protected/release.bin", + }) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "object key [uploads/user-a/../../protected/release.bin]") + require.Contains(suite.T(), err.Error(), `contains a ".." segment`) + require.NotContains(suite.T(), err.Error(), "fingerprint as the same path", + "the traversing key must be rejected outright, not reported as a collision") +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysSingleProblemReadsAsOneLine() { + _, err := virtualPathsForS3Keys([]string{"good", "bad/.."}) + require.Error(suite.T(), err) + require.Equal(suite.T(), + `object key [bad/..] cannot be fingerprinted: contains a ".." segment; exclude it with --exclude-regex, or narrow the include filter if one is set`, + err.Error()) +} + +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysIsDeterministic() { + keys := []string{"c/..", "b/..", "a/..", "d", "d/e"} + first, err := virtualPathsForS3Keys(keys) + require.Error(suite.T(), err) + require.Nil(suite.T(), first) + for i := 0; i < 20; i++ { + _, again := virtualPathsForS3Keys(keys) + require.Equal(suite.T(), err.Error(), again.Error()) + } +} + +func TestS3KeysTestSuite(t *testing.T) { + suite.Run(t, new(S3KeysTestSuite)) +} From fbddbc54d5455be401af1801342bd343dee358e3 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:05:01 +0100 Subject: [PATCH 06/30] feat(digest): apply .kosli_ignore rules to a virtual tree VirtualDirSha256 takes the rules of the tree's root .kosli_ignore and excludes what DirSha256 would exclude on disk. Rather than reimplement what the globs appear to mean, virtualFS reproduces filepathx.Glob, filepath.Glob and filepath.Walk step for step over the virtual tree, so their quirks come out identical: a literal "**/x" never matches at the root because the pieces concatenate to a double slash, "**/*.log" does, and excluding "logs/*" leaves an empty directory whose name is still hashed. Exclusion therefore happens inside the tree walk, not by filtering the file list. The root ignore file is never excluded by its own rules, as on disk. ParseIgnoreRules is extracted from excludePathsFromFile so callers that hold the file's bytes get the same reading DirSha256 gives the file. Every rule set in the equivalence test is materialised on disk and fingerprinted with DirSha256, and the virtual digest must match; rows that should change the digest also assert that they do. --- internal/digest/digest.go | 38 ++-- internal/digest/virtualdir.go | 39 +++- internal/digest/virtualdir_test.go | 8 +- internal/digest/virtualglob.go | 197 ++++++++++++++++++++ internal/digest/virtualignore_test.go | 247 ++++++++++++++++++++++++++ 5 files changed, 503 insertions(+), 26 deletions(-) create mode 100644 internal/digest/virtualglob.go create mode 100644 internal/digest/virtualignore_test.go diff --git a/internal/digest/digest.go b/internal/digest/digest.go index 750330920..0774eac1a 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -534,19 +534,8 @@ func excludePathsFromFile(path string) ([]string, error) { fmt.Printf("warning: failed to close file %s: %v\n", path, err) } }() - var excludes = []string{} - scanner := bufio.NewScanner(file) - for scanner.Scan() { - line := scanner.Text() - line = removeComments(line) - line = strings.TrimSpace(line) - if len(line) > 0 { - excludes = append(excludes, line) - } - } - // A stopped scan yields the entries read so far, so an unchecked error means - // fingerprinting against a rule set the file does not hold. - if err := scanner.Err(); err != nil { + excludes, err := ParseIgnoreRules(file) + if err != nil { return nil, fmt.Errorf("failed to read %s: %w", path, err) } return excludes, nil @@ -556,6 +545,29 @@ func excludePathsFromFile(path string) ([]string, error) { return nil, err } +// ParseIgnoreRules reads the content of a .kosli_ignore file: one path or glob +// per line, with "#" starting a comment and blank lines skipped. It is the same +// reading DirSha256 gives the file, for callers that hold the bytes rather than +// a path. +func ParseIgnoreRules(r io.Reader) ([]string, error) { + var rules = []string{} + scanner := bufio.NewScanner(r) + for scanner.Scan() { + line := scanner.Text() + line = removeComments(line) + line = strings.TrimSpace(line) + if len(line) > 0 { + rules = append(rules, line) + } + } + // A stopped scan yields the entries read so far, so an unchecked error means + // fingerprinting against a rule set the file does not hold. + if err := scanner.Err(); err != nil { + return nil, err + } + return rules, nil +} + func removeComments(line string) string { parts := strings.SplitN(line, "#", 2) return strings.TrimRight(parts[0], " ") diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index c30f39121..e5cee7508 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -57,10 +57,13 @@ func SingleVirtualFile(files []VirtualFile) (VirtualFile, bool) { // Sorting the flat path list instead of the tree produces a different digest // whenever a directory shares a name prefix with a sibling file. // -// .kosli_ignore is deliberately not handled here: reading it needs the file's -// content, which the caller may not have, so exclusions stay the caller's -// concern. -func VirtualDirSha256(files []VirtualFile, logger *logger.Logger) (string, error) { +// ignoreRules are the entries of the tree's root .kosli_ignore, as +// ParseIgnoreRules returns them. They are resolved against the tree exactly as +// DirSha256 resolves them against a directory (see virtualFS), and the root +// .kosli_ignore itself is never excluded by them, so a tree cannot change its +// exclusion list without changing its fingerprint. Callers that hold the file's +// content pass its rules here; the file is an ordinary entry of files. +func VirtualDirSha256(files []VirtualFile, ignoreRules []string, logger *logger.Logger) (string, error) { if len(files) == 0 { return "", fmt.Errorf("cannot calculate a fingerprint: no files were provided") } @@ -70,9 +73,14 @@ func VirtualDirSha256(files []VirtualFile, logger *logger.Logger) (string, error return "", err } - logger.Debug("calculating fingerprint for a virtual tree of %d files", len(files)) + excluded, err := virtualFS{root: root}.excludedPaths(ignoreRules) + if err != nil { + return "", err + } + + logger.Debug("calculating fingerprint for a virtual tree of %d files -- excluding %d paths", len(files), len(excluded)) hasher := sha256.New() - root.writeDigests(hasher, logger) + root.writeDigests(hasher, virtualRoot, excluded, path.Join(virtualRoot, ignoreFileName), logger) return hex.EncodeToString(hasher.Sum(nil)), nil } @@ -127,16 +135,29 @@ func buildVirtualTree(files []VirtualFile) (*virtualNode, error) { return root, nil } -// writeDigests appends this node's children to the hash in WalkDir order. -func (n *virtualNode) writeDigests(hasher hash.Hash, logger *logger.Logger) { +// writeDigests appends this node's children to the hash in WalkDir order, +// skipping excluded entries as calculateDirContentSha256 does: an excluded +// directory takes its subtree with it, and the protected path is kept whatever +// the rules say. +func (n *virtualNode) writeDigests(hasher hash.Hash, dir string, excluded map[string]bool, protected string, logger *logger.Logger) { for _, name := range n.sortedChildNames() { child := n.children[name] + childPath := path.Join(dir, name) + if excluded[childPath] { + if childPath == protected { + logger.Debug("keeping %s although an exclusion matches it: an exclusion list cannot exclude itself", childPath) + } else { + logger.Debug("skipping %s as it matches excluded paths", childPath) + continue + } + } + nameSha256 := sha256OfString(child.name) hasher.Write([]byte(nameSha256)) //nolint:errcheck // hash.Hash never returns an error if child.isDir { logger.Debug("dir: %s -- dirname digest: %s", child.name, nameSha256) - child.writeDigests(hasher, logger) + child.writeDigests(hasher, childPath, excluded, protected, logger) continue } logger.Debug("file: %s -- filename digest: %s -- content digest: %s", diff --git a/internal/digest/virtualdir_test.go b/internal/digest/virtualdir_test.go index 93ffd14c8..def791a5f 100644 --- a/internal/digest/virtualdir_test.go +++ b/internal/digest/virtualdir_test.go @@ -96,7 +96,7 @@ func (suite *VirtualDirTestSuite) TestVirtualDirSha256MatchesDirSha256() { want, err := DirSha256(root, []string{}, logger.NewStandardLogger()) require.NoError(suite.T(), err) - got, err := VirtualDirSha256(virtualFiles, logger.NewStandardLogger()) + got, err := VirtualDirSha256(virtualFiles, nil, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), want, got, @@ -119,9 +119,9 @@ func (suite *VirtualDirTestSuite) TestVirtualDirSha256IgnoresInputOrder() { reversed[len(files)-1-i] = f } - first, err := VirtualDirSha256(files, logger.NewStandardLogger()) + first, err := VirtualDirSha256(files, nil, logger.NewStandardLogger()) require.NoError(suite.T(), err) - second, err := VirtualDirSha256(reversed, logger.NewStandardLogger()) + second, err := VirtualDirSha256(reversed, nil, logger.NewStandardLogger()) require.NoError(suite.T(), err) require.Equal(suite.T(), first, second) @@ -205,7 +205,7 @@ func (suite *VirtualDirTestSuite) TestVirtualDirSha256Errors() { }, } { suite.Run(t.name, func() { - _, err := VirtualDirSha256(t.files, logger.NewStandardLogger()) + _, err := VirtualDirSha256(t.files, nil, logger.NewStandardLogger()) require.Error(suite.T(), err) require.Contains(suite.T(), err.Error(), t.wantErrMsg) }) diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go new file mode 100644 index 000000000..8f03ad1a7 --- /dev/null +++ b/internal/digest/virtualglob.go @@ -0,0 +1,197 @@ +package digest + +import ( + "fmt" + "path" + "strings" +) + +// virtualRoot stands in for the directory path DirSha256 is given. Every path in +// the virtual tree is spelled "tree/", so a rule joined onto the +// root goes through exactly the string handling filepath.Join, filepath.Glob and +// filepath.Walk apply on disk. +const virtualRoot = "tree" + +// virtualFS answers the questions filepath.Glob and filepath.Walk ask of a +// filesystem, over a virtual tree instead. +// +// DirSha256 resolves .kosli_ignore rules with filepathx.Glob, which has its own +// reading of "**" and inherits filepath.Glob's: a pattern without wildcards is +// returned as written, uncleaned, while one with wildcards is rebuilt from the +// directory listing, cleaned. So "**/x" never matches x at the root (the pieces +// concatenate to "tree//x") but "**/*.log" does. Reproducing the algorithm step +// for step, rather than its apparent meaning, is what keeps the virtual digest +// equal to the on-disk one for every rule anyone has already written. +type virtualFS struct { + root *virtualNode +} + +// excludedPaths resolves ignore rules to the set of tree paths DirSha256 would +// skip, spelled exactly as the resolving glob returned them. +func (fs virtualFS) excludedPaths(rules []string) (map[string]bool, error) { + excluded := map[string]bool{} + for _, rule := range rules { + pattern := path.Join(virtualRoot, rule) + // On disk the root is a temp directory with an unguessable name, so a rule + // that resolves to the root or outside it cannot match anything there. + if pattern == virtualRoot || !strings.HasPrefix(pattern, virtualRoot+"/") { + continue + } + matches, err := fs.globDoubleStar(pattern) + if err != nil { + return nil, fmt.Errorf("ignore rule %q: %w", rule, err) + } + for _, match := range matches { + excluded[match] = true + } + } + return excluded, nil +} + +// globDoubleStar mirrors filepathx.Glob: split the pattern on "**", glob each +// piece appended to every match so far, and walk every hit so the next piece is +// tried under all of its descendants. +func (fs virtualFS) globDoubleStar(pattern string) ([]string, error) { + if !strings.Contains(pattern, "**") { + return fs.glob(pattern) + } + matches := []string{""} + for _, piece := range strings.Split(pattern, "**") { + var hits []string + seen := map[string]bool{} + for _, match := range matches { + paths, err := fs.glob(match + piece) + if err != nil { + return nil, err + } + for _, p := range paths { + err := fs.walk(p, func(visited string) { + if !seen[visited] { + hits = append(hits, visited) + seen[visited] = true + } + }) + if err != nil { + return nil, err + } + } + } + matches = hits + } + return matches, nil +} + +// glob mirrors filepath.Glob on a Unix filesystem. +func (fs virtualFS) glob(pattern string) ([]string, error) { + if !hasGlobMeta(pattern) { + // A literal pattern is returned as written, not cleaned. + if _, ok := fs.lookup(pattern); !ok { + return nil, nil + } + return []string{pattern}, nil + } + + dir, file := path.Split(pattern) + dir = cleanGlobPath(dir) + if !hasGlobMeta(dir) { + return fs.globDir(dir, file, nil) + } + if dir == pattern { + return nil, path.ErrBadPattern + } + dirs, err := fs.glob(dir) + if err != nil { + return nil, err + } + var matches []string + for _, d := range dirs { + matches, err = fs.globDir(d, file, matches) + if err != nil { + return nil, err + } + } + return matches, nil +} + +// globDir mirrors filepath.glob: match the pattern against each name in one +// directory and return the joined, cleaned paths. +func (fs virtualFS) globDir(dir, pattern string, matches []string) ([]string, error) { + node, ok := fs.lookup(dir) + if !ok || !node.isDir { + return matches, nil + } + for _, name := range node.sortedChildNames() { + matched, err := path.Match(pattern, name) + if err != nil { + return matches, err + } + if matched { + matches = append(matches, path.Join(dir, name)) + } + } + return matches, nil +} + +// walk mirrors filepath.Walk as filepathx drives it: the root is reported as +// given, descendants as joined, cleaned paths, in lexical order. +func (fs virtualFS) walk(root string, visit func(string)) error { + node, ok := fs.lookup(root) + if !ok { + return fmt.Errorf("lstat %s: no such file or directory", root) + } + fs.walkNode(root, node, visit) + return nil +} + +func (fs virtualFS) walkNode(p string, node *virtualNode, visit func(string)) { + visit(p) + if !node.isDir { + return + } + for _, name := range node.sortedChildNames() { + fs.walkNode(path.Join(p, name), node.children[name], visit) + } +} + +// lookup finds the node a possibly uncleaned path spells, or reports that no +// such entry exists. +func (fs virtualFS) lookup(p string) (*virtualNode, bool) { + cleaned := path.Clean(p) + if cleaned == virtualRoot { + return fs.root, true + } + if !strings.HasPrefix(cleaned, virtualRoot+"/") { + return nil, false + } + node := fs.root + for _, segment := range strings.Split(cleaned[len(virtualRoot)+1:], "/") { + if !node.isDir { + return nil, false + } + child, ok := node.children[segment] + if !ok { + return nil, false + } + node = child + } + return node, true +} + +// hasGlobMeta reports whether filepath.Glob would treat the pattern as a glob +// on Unix, where a backslash is an escape character. +func hasGlobMeta(pattern string) bool { + return strings.ContainsAny(pattern, `*?[\`) +} + +// cleanGlobPath is filepath's helper of the same name: drop the trailing +// separator path.Split leaves on a directory. +func cleanGlobPath(dir string) string { + switch dir { + case "": + return "." + case "/": + return dir + default: + return dir[:len(dir)-1] + } +} diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go new file mode 100644 index 000000000..06d276b9a --- /dev/null +++ b/internal/digest/virtualignore_test.go @@ -0,0 +1,247 @@ +package digest + +import ( + "path/filepath" + "sort" + "strings" + "testing" + + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/utils" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type VirtualIgnoreTestSuite struct { + suite.Suite +} + +// ignoreTestTree is shaped to reach every glob behaviour DirSha256 has on disk: +// a directory and a file sharing a stem, the same name at several depths, a +// nested .kosli_ignore, and a directory whose content can be excluded while the +// directory itself stays. +var ignoreTestTree = map[string]string{ + "app.js": "app", + "app.log": "log at the root", + "notes.txt": "notes", + "logs/file1": "c1", + "logs/deep/file2": "c2", + "nested-dir/file1": "n1", + "nested-dir/logs/log.txt": "nl", + "vendor/lib/v.js": "v", + "vendor/lib/.kosli_ignore": "nested-rules", + "a/x": "ax", + "a/b/x": "abx", + "a/b/c/x": "abcx", +} + +// TestMatchesDirSha256 is the test that matters. For each rule set, the tree +// plus a root .kosli_ignore holding the rules is materialised on disk and +// fingerprinted with DirSha256; the same files and rules go through +// VirtualDirSha256 and must give the identical digest. Rows marked hasEffect +// also require the rules to have changed the digest, so a row cannot pass +// because both sides ignored the rules. +func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { + for _, t := range []struct { + name string + ignore string + hasEffect bool + }{ + {name: "no rules", ignore: ""}, + {name: "only comments and blanks", ignore: "# nothing\n\n \n"}, + {name: "a directory by name", ignore: "logs", hasEffect: true}, + {name: "a file by name", ignore: "app.js", hasEffect: true}, + {name: "a directory at depth one", ignore: "*/logs", hasEffect: true}, + {name: "the content of a directory but not the directory", ignore: "logs/*", hasEffect: true}, + {name: "a suffix at the root", ignore: "*.log", hasEffect: true}, + {name: "a suffix at any depth", ignore: "**/*.log", hasEffect: true}, + {name: "a literal name at any depth", ignore: "**/x", hasEffect: true}, + {name: "a literal name at any depth under a prefix", ignore: "a/**/x", hasEffect: true}, + {name: "a directory at any depth", ignore: "**/logs", hasEffect: true}, + {name: "a bare double star", ignore: "**", hasEffect: true}, + // filepathx appends ".log" to every existing path, so this matches only an + // "x.log" whose sibling "x" also exists. Nothing here, on either side. + {name: "a double star glued to a suffix", ignore: "**.log"}, + {name: "a nested ignore file under a prefix", ignore: "vendor/**/.kosli_ignore", hasEffect: true}, + {name: "a trailing slash", ignore: "logs/", hasEffect: true}, + {name: "a leading slash", ignore: "/logs", hasEffect: true}, + {name: "a leading dot segment", ignore: "./logs", hasEffect: true}, + {name: "a doubled slash", ignore: "nested-dir//logs", hasEffect: true}, + {name: "a parent segment that leaves the tree", ignore: "../logs"}, + {name: "a rule that matches nothing", ignore: "does-not-exist"}, + {name: "a star alone", ignore: "*", hasEffect: true}, + {name: "a character class", ignore: "app.[jl]?", hasEffect: true}, + {name: "a question mark", ignore: "a/?", hasEffect: true}, + {name: "an escaped star is a literal", ignore: `app\*`}, + {name: "several rules with a comment", ignore: "logs\n# keep vendor\n*/logs\napp.log # trailing comment\n", hasEffect: true}, + {name: "the ignore file itself", ignore: ".kosli_ignore"}, + {name: "a glob matching the ignore file", ignore: "*ignore*"}, + {name: "a dotted glob matching the ignore file", ignore: ".kosli*"}, + {name: "a double star matching the ignore file", ignore: "**/.kosli_ignore", hasEffect: true}, + } { + suite.Run(t.name, func() { + root := suite.T().TempDir() + files := suite.materialise(root, ignoreTestTree, t.ignore) + + want, err := DirSha256(root, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + rules, err := ParseIgnoreRules(strings.NewReader(t.ignore)) + require.NoError(suite.T(), err) + got, err := VirtualDirSha256(files, rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), want, got, "VirtualDirSha256 must equal DirSha256 with rules %q", t.ignore) + + noRules, err := VirtualDirSha256(files, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + if t.hasEffect { + require.NotEqual(suite.T(), noRules, got, "rules %q were expected to change the digest", t.ignore) + } else { + require.Equal(suite.T(), noRules, got, "rules %q were expected to leave the digest unchanged", t.ignore) + } + }) + } +} + +// A malformed pattern fails DirSha256, so it must fail the virtual digest too +// rather than silently excluding nothing. +func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { + root := suite.T().TempDir() + files := suite.materialise(root, ignoreTestTree, "[") + + _, err := DirSha256(root, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + + _, err = VirtualDirSha256(files, []string{"["}, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "[") +} + +// Mirrors TestDirSha256IgnoreFileCannotHideItself: an ignore file that lists +// itself cannot hide an added file, because the file's own content stays in the +// digest. +func (suite *VirtualIgnoreTestSuite) TestIgnoreFileCannotHideItself() { + baseline := map[string]string{ + "app/index.js": "console.log(1)", + "app/lib/util.js": "exports.x = 1", + } + approvedFiles := virtualFilesFor(baseline, "") + approved, err := VirtualDirSha256(approvedFiles, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + for _, ignore := range []string{ + ".kosli_ignore\napp/backdoor.js", + "*ignore*\napp/backdoor.js", + ".kosli*\napp/backdoor.js", + "**/.kosli_ignore\napp/backdoor.js", + "**\napp/backdoor.js", + } { + suite.Run(ignore, func() { + deployed := map[string]string{} + for k, v := range baseline { + deployed[k] = v + } + deployed["app/backdoor.js"] = "BACKDOOR" + rules, err := ParseIgnoreRules(strings.NewReader(ignore)) + require.NoError(suite.T(), err) + + got, err := VirtualDirSha256(virtualFilesFor(deployed, ignore), rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.NotEqual(suite.T(), approved, got, "the added file is invisible to the fingerprint") + }) + } +} + +// Only the root ignore file is protected; a nested one is an ordinary file the +// root list may exclude. +func (suite *VirtualIgnoreTestSuite) TestNestedIgnoreFileIsExcludable() { + rules := "vendor/**/.kosli_ignore" + withNested := virtualFilesFor(map[string]string{ + "app.js": "app", + "vendor/lib/v.js": "v", + "vendor/lib/.kosli_ignore": "nested-rules", + }, rules) + withoutNested := virtualFilesFor(map[string]string{ + "app.js": "app", + "vendor/lib/v.js": "v", + }, rules) + + parsed, err := ParseIgnoreRules(strings.NewReader(rules)) + require.NoError(suite.T(), err) + a, err := VirtualDirSha256(withNested, parsed, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + b, err := VirtualDirSha256(withoutNested, parsed, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), b, a) +} + +func (suite *VirtualIgnoreTestSuite) TestParseIgnoreRules() { + for _, t := range []struct { + name string + input string + want []string + }{ + {name: "empty input", input: "", want: []string{}}, + {name: "one rule", input: "logs", want: []string{"logs"}}, + {name: "blank lines and whitespace are dropped", input: "\n logs \n\n\t*.log\t\n", want: []string{"logs", "*.log"}}, + {name: "comment lines are dropped", input: "# all logs\nlogs\n# done", want: []string{"logs"}}, + {name: "a trailing comment is stripped", input: "logs # noisy", want: []string{"logs"}}, + {name: "a comment with no rule before it is dropped", input: " # only a comment", want: []string{}}, + {name: "windows line endings", input: "logs\r\n*.log\r\n", want: []string{"logs", "*.log"}}, + } { + suite.Run(t.name, func() { + got, err := ParseIgnoreRules(strings.NewReader(t.input)) + require.NoError(suite.T(), err) + require.Equal(suite.T(), t.want, got) + }) + } +} + +// ParseIgnoreRules and excludePathsFromFile must agree, since DirSha256 reads +// the file while the virtual digest is handed the same bytes. +func (suite *VirtualIgnoreTestSuite) TestParseIgnoreRulesAgreesWithTheFileReader() { + content := "logs\n# comment\n*/logs # trailing\n\n app.log\n" + path := filepath.Join(suite.T().TempDir(), ".kosli_ignore") + require.NoError(suite.T(), utils.CreateFileWithContent(path, content)) + + fromFile, err := excludePathsFromFile(path) + require.NoError(suite.T(), err) + fromReader, err := ParseIgnoreRules(strings.NewReader(content)) + require.NoError(suite.T(), err) + require.Equal(suite.T(), fromFile, fromReader) +} + +// materialise writes the tree and, when ignore is non-empty, a root +// .kosli_ignore holding it; it returns the matching virtual files. +func (suite *VirtualIgnoreTestSuite) materialise(root string, tree map[string]string, ignore string) []VirtualFile { + suite.T().Helper() + for p, content := range tree { + require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, filepath.FromSlash(p)), content)) + } + if ignore != "" { + require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, ignoreFileName), ignore)) + } + return virtualFilesFor(tree, ignore) +} + +// virtualFilesFor returns the (path, sha256) pairs for tree plus, when ignore +// is non-empty, a root .kosli_ignore with that content, in a fixed order. +func virtualFilesFor(tree map[string]string, ignore string) []VirtualFile { + paths := make([]string, 0, len(tree)+1) + for p := range tree { + paths = append(paths, p) + } + sort.Strings(paths) + files := make([]VirtualFile, 0, len(paths)+1) + for _, p := range paths { + files = append(files, VirtualFile{Path: p, Sha256: sha256OfString(tree[p])}) + } + if ignore != "" { + files = append(files, VirtualFile{Path: ignoreFileName, Sha256: sha256OfString(ignore)}) + } + return files +} + +func TestVirtualIgnoreTestSuite(t *testing.T) { + suite.Run(t, new(VirtualIgnoreTestSuite)) +} From c6d1a58575ecc5869b9de2a69df680b8c67908a4 Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:05:03 +0100 Subject: [PATCH 07/30] feat(snapshot s3): fingerprint the bucket without using object keys as local paths Each object now downloads to an anonymous temp file that is hashed and removed; the fingerprint comes from digest.VirtualDirSha256 over the (key, sha256) pairs, which reproduces what DirSha256 gave the same tree on disk. No key ever names a file, so traversal, overwrites, reserved names, case folding and component length limits stop being properties of this code, and the fingerprint is the same on every operating system. A root .kosli_ignore is downloaded first and its rules applied; objects the rules exclude are not downloaded at all. digest.FilesNeedingContent decides which files need a digest, and VirtualDirSha256 refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Keys that cannot form a directory tree (a ".." segment, two keys folding onto one path, an object that is also a prefix) fail the snapshot and name every key involved. Fingerprints recorded against main are pinned before the switch and unchanged after it; a bucket built from a directory fingerprints as DirSha256 fingerprints the directory. localPathForS3Key, the O_EXCL and ENOTDIR handling and containsSingleFile are gone with their tests. The cmd/kosli TestSnapshotS3 suite needs the local Kosli server and is left to CI. --- cmd/kosli/snapshotS3.go | 3 +- internal/aws/aws.go | 252 ++++++++++++++------------ internal/aws/aws_test.go | 144 +-------------- internal/aws/s3_fingerprint_test.go | 242 +++++++++++++++++++++++++ internal/digest/digest.go | 6 +- internal/digest/virtualdir.go | 107 ++++++++--- internal/digest/virtualdir_test.go | 7 + internal/digest/virtualignore_test.go | 80 ++++++++ 8 files changed, 559 insertions(+), 282 deletions(-) create mode 100644 internal/aws/s3_fingerprint_test.go diff --git a/cmd/kosli/snapshotS3.go b/cmd/kosli/snapshotS3.go index 41ae7f896..975c4a2f8 100644 --- a/cmd/kosli/snapshotS3.go +++ b/cmd/kosli/snapshotS3.go @@ -15,7 +15,8 @@ const snapshotS3ShortDesc = `Report a snapshot of the content of an AWS S3 bucke const snapshotS3LongDesc = snapshotS3ShortDesc + awsAuthDesc + ` You can report the entire bucket content, or filter some of the content using ^--include^ / ^--exclude^ (literal prefix match) or ^--include-regex^ / ^--exclude-regex^ (Go regular expressions matched against the full object key). In all cases, the content is reported as one artifact. If you wish to report separate files/dirs within the same bucket as separate artifacts, you need to run the command twice. -Object keys that cannot be stored as a local file, such as keys containing a ^..^ path segment, are rejected and fail the snapshot, naming the key. Two keys that resolve to the same local file are also an error. A legitimate key of that shape can be left out with ^--exclude-regex^ (anchor and escape it, since the pattern is a regular expression matched against the whole key); when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead. +Object keys are never used as local file names: each object is downloaded to a temporary file, hashed and removed, and the fingerprint is computed from the keys and the content digests, so any key S3 accepts can be fingerprinted on any operating system. +Keys that cannot form a directory tree are rejected and fail the snapshot, naming every key involved: a key containing a ^..^ segment, two keys that resolve to the same path (such as ^a//b^ and ^a/b^), or an object whose key is also a prefix of other objects (such as ^a^ beside ^a/b^). A legitimate key of that shape can be left out with ^--exclude-regex^ (anchor and escape it, since the pattern is a regular expression matched against the whole key); when ^--include^ or ^--include-regex^ is set, exclude filters are ignored, so narrow the include filter instead. ` + kosliIgnoreDescNoExclude diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 9b750e4f6..314a80f6c 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -4,15 +4,13 @@ import ( "context" "encoding/base64" "encoding/hex" - "errors" "fmt" - "io/fs" + "io" "os" - "path/filepath" + "path" "regexp" "strings" "sync" - "syscall" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -420,30 +418,6 @@ func compilePathRegex(patterns []string) ([]*regexp.Regexp, error) { return compiled, nil } -// containsSingleFile checks if a path contains only a single file -func containsSingleFile(directoryPath string) (bool, string, error) { - files, err := os.ReadDir(directoryPath) - if err != nil { - return false, "", err - } - - if len(files) == 1 { - fileInfo := files[0] - - if fileInfo.IsDir() { - // If it's a directory, recursively check inside - subDir := filepath.Join(directoryPath, fileInfo.Name()) - return containsSingleFile(subDir) - } - - // If it's a file, return information about it - path := filepath.Join(directoryPath, fileInfo.Name()) - return true, path, nil - } - - return false, "", nil -} - // objectMatchesFilter reports whether key matches any of the filter entries. // A key matches when it is prefixed by one of paths (literal prefix match) // or when one of patterns matches the full key. @@ -487,134 +461,169 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex return s3Data, err } - tempDirName, err := os.MkdirTemp("", "bucketContent") + objects, err := listMatchingS3Objects(client, bucket, includePaths, includeRegexCompiled, excludePaths, excludeRegexCompiled) if err != nil { return s3Data, err } - defer func() { - if err := os.RemoveAll(tempDirName); err != nil { - logger.Warn("failed to remove temp dir %s: %v", tempDirName, err) + if len(objects) == 0 { + return s3Data, fmt.Errorf("no matching file or dirs in bucket: [%s]", bucket) + } + + newest := objects[0].lastModified + for _, object := range objects { + if object.lastModified.After(newest) { + newest = object.lastModified } - }() + } - params := &s3.ListObjectsV2Input{ - Bucket: aws.String(bucket), + artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, logger) + if err != nil { + return s3Data, err } - var lastModifiedTime *time.Time - paginator := s3.NewListObjectsV2Paginator(client, params) + s3Data = append(s3Data, &S3Data{Digests: map[string]string{artifactName: sha256}, LastModifiedTimestamp: newest.Unix()}) + + return s3Data, nil +} + +// s3Object is one listed object that survived the include and exclude filters. +type s3Object struct { + key string + lastModified time.Time +} + +// listMatchingS3Objects lists the bucket, dropping folder markers and keys the +// filters exclude, in the order S3 returns them. +func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []string, includeRegex []*regexp.Regexp, + excludePaths []string, excludeRegex []*regexp.Regexp) ([]s3Object, error) { + objects := []s3Object{} + paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{ + Bucket: aws.String(bucket), + }) for paginator.HasMorePages() { - objects, err := paginator.NextPage(context.TODO()) + page, err := paginator.NextPage(context.TODO()) if err != nil { - return s3Data, err + return nil, err } - - for _, object := range objects.Contents { + for _, object := range page.Contents { if strings.HasSuffix(*object.Key, "/") { // skip folders continue } - if shouldExcludePath(*object.Key, includePaths, includeRegexCompiled, excludePaths, excludeRegexCompiled) { + if shouldExcludePath(*object.Key, includePaths, includeRegex, excludePaths, excludeRegex) { continue } - err := downloadFileFromBucket(client, tempDirName, *object.Key, bucket, logger) - if err != nil { - return s3Data, err - } - - if lastModifiedTime == nil || object.LastModified.After(*lastModifiedTime) { - lastModifiedTime = object.LastModified - } + objects = append(objects, s3Object{key: *object.Key, lastModified: *object.LastModified}) } } + return objects, nil +} - if lastModifiedTime == nil { - return s3Data, fmt.Errorf("no matching file or dirs in bucket: [%s]", bucket) +// fingerprintS3Objects fingerprints the objects as the directory their keys +// describe, without ever using a key as a local file name. Each object is +// downloaded to an anonymous temp file, hashed and removed; the fingerprint is +// then computed from the (key, sha256) pairs by digest.VirtualDirSha256, which +// reproduces what digest.DirSha256 gives the same tree on disk. A single object +// is fingerprinted as that file and named after it, as before. +// +// A root .kosli_ignore is downloaded first so its rules can be applied, and +// objects the rules exclude are not downloaded at all. +func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, logger *logger.Logger) (artifactName, sha256 string, err error) { + keys := make([]string, len(objects)) + for i, object := range objects { + keys[i] = object.key + } + paths, err := virtualPathsForS3Keys(keys) + if err != nil { + return "", "", err } - fileSnapshot, artifactPath, err := containsSingleFile(tempDirName) + tempDir, err := os.MkdirTemp("", "bucketContent") if err != nil { - return s3Data, err + return "", "", err } - var sha256 string - artifactName := bucket - if fileSnapshot { - sha256, err = digest.FileSha256(artifactPath, logger) - if err != nil { - return s3Data, err + defer func() { + if err := os.RemoveAll(tempDir); err != nil { + logger.Warn("failed to remove temp dir %s: %v", tempDir, err) } - artifactName = filepath.Base(artifactPath) - } else { - sha256, err = digest.DirSha256(tempDirName, []string{}, logger) + }() + + if len(objects) == 1 { + sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, keys[0], nil, logger) if err != nil { - return s3Data, err + return "", "", err } + return path.Base(paths[keys[0]]), sha256, nil } - s3Data = append(s3Data, &S3Data{Digests: map[string]string{artifactName: sha256}, LastModifiedTimestamp: lastModifiedTime.Unix()}) - - return s3Data, nil -} - -// localPathForS3Key turns an S3 object key into a path under the download -// directory, or rejects it. A key holding a ".." segment resolves onto a path -// it does not name, taking another key's place or leaving the directory. -func localPathForS3Key(key string) (string, error) { - // Windows separates on '\\' and drops trailing dots and spaces from a - // name, so ".. " and "..." resolve as ".." there. - segments := strings.FieldsFunc(key, func(r rune) bool { return r == '/' || r == '\\' }) - for _, segment := range segments { - if strings.HasPrefix(segment, "..") && strings.TrimRight(segment, ". ") == "" { - return "", unusableS3KeyError(key, `contains a segment that resolves to ".."`) + var rules []string + contentSha256 := map[string]string{} + for _, key := range keys { + if paths[key] != digest.IgnoreFileName { + continue } + sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, key, func(file *os.File) error { + if _, err := file.Seek(0, io.SeekStart); err != nil { + return err + } + rules, err = digest.ParseIgnoreRules(file) + return err + }, logger) + if err != nil { + return "", "", err + } + contentSha256[key] = sha256 + logger.Debug("object key [%s] is the bucket's %s -- excluding paths: %s", key, digest.IgnoreFileName, rules) } - // A leading '\\' is left for filepath.IsLocal: rooted on Windows, an - // ordinary filename elsewhere. - rel := strings.TrimLeft(key, "/") - if filepath.Clean(rel) == "." { - return "", unusableS3KeyError(key, "names no file") - } - if !filepath.IsLocal(rel) { - return "", unusableS3KeyError(key, "is not a local path") + allPaths := make([]string, len(keys)) + for i, key := range keys { + allPaths[i] = paths[key] } - - return rel, nil -} - -// unusableS3KeyError is only for failures the key itself causes. Advising -// exclusion on a machine fault such as a full disk would drop a legitimate -// object from the snapshot. -func unusableS3KeyError(key, reason string) error { - return fmt.Errorf("object key [%s] cannot be stored as a local file: %s; exclude it with --exclude-regex, or narrow the include filter if one is set", key, reason) -} - -func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket string, logger *logger.Logger) error { - rel, err := localPathForS3Key(key) + needed, err := digest.FilesNeedingContent(allPaths, rules) if err != nil { - return err + return "", "", fmt.Errorf("invalid rule in the bucket's %s: %w", digest.IgnoreFileName, err) } - dest := filepath.Join(dirName, rel) - err = os.MkdirAll(filepath.Dir(dest), 0770) - if errors.Is(err, syscall.ENOTDIR) { - // Legal in S3, impossible on disk: an object "a" and a key under "a/". - return unusableS3KeyError(key, "one of its parent prefixes has already been downloaded as an object") + + files := make([]digest.VirtualFile, 0, len(keys)) + for _, key := range keys { + virtualPath := paths[key] + sha256, downloaded := contentSha256[key] + switch { + case downloaded: + case needed[virtualPath]: + sha256, err = downloadAndHashS3Object(downloader, tempDir, bucket, key, nil, logger) + if err != nil { + return "", "", err + } + default: + logger.Debug("object key [%s] is excluded by %s and is not downloaded", key, digest.IgnoreFileName) + } + files = append(files, digest.VirtualFile{Path: virtualPath, Sha256: sha256}) } + + sha256, err = digest.VirtualDirSha256(files, rules, logger) if err != nil { - return fmt.Errorf("object key [%s]: %w", key, err) - } - // O_EXCL fails the snapshot when two keys map to one file rather than - // letting the second overwrite the first. Directories are not covered: - // "A/x" and "a/y" share one on a case-insensitive filesystem. - file, err := os.OpenFile(dest, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0666) - if errors.Is(err, fs.ErrExist) { - return unusableS3KeyError(key, "another object already downloaded to the same local path") + return "", "", err } + return bucket, sha256, nil +} + +// downloadAndHashS3Object fetches one object into a fresh temp file, lets +// inspect read it when given, returns the sha256 of its content and removes the +// file. The file's name comes from the OS, so nothing about the key reaches the +// filesystem. +func downloadAndHashS3Object(downloader S3DownloadAPI, tempDir, bucket, key string, inspect func(*os.File) error, logger *logger.Logger) (string, error) { + file, err := os.CreateTemp(tempDir, "object-*") if err != nil { - return fmt.Errorf("object key [%s]: %w", key, err) + return "", fmt.Errorf("object key [%s]: %w", key, err) } defer func() { + // Close before remove: Windows will not delete an open file. if err := file.Close(); err != nil { - logger.Warn("failed to close file %s: %v", file.Name(), err) + logger.Warn("failed to close temp file for object key [%s]: %v", key, err) + } + if err := os.Remove(file.Name()); err != nil { + logger.Warn("failed to remove temp file for object key [%s]: %v", key, err) } }() @@ -624,13 +633,18 @@ func downloadFileFromBucket(downloader S3DownloadAPI, dirName, key, bucket strin WriterAt: file, }) if err != nil { - return fmt.Errorf("failed to download object key [%s]: %w", key, err) + return "", fmt.Errorf("failed to download object key [%s]: %w", key, err) } if result.ContentLength != nil { - logger.Debug("downloaded", file.Name(), *result.ContentLength, "bytes") + logger.Debug("downloaded object key [%s]: %d bytes", key, *result.ContentLength) } - return nil + if inspect != nil { + if err := inspect(file); err != nil { + return "", fmt.Errorf("object key [%s]: %w", key, err) + } + } + return digest.FileSha256(file.Name(), logger) } // getFilteredECSClusters fetches a filtered set of ECS clusters recursively (50 at a time) and returns a list of ecs Clusters diff --git a/internal/aws/aws_test.go b/internal/aws/aws_test.go index 1b15e5932..ad6cbbfb8 100644 --- a/internal/aws/aws_test.go +++ b/internal/aws/aws_test.go @@ -3,11 +3,7 @@ package aws import ( "context" "fmt" - "io/fs" - "os" - "path/filepath" "regexp" - "runtime" "testing" "time" @@ -1248,102 +1244,7 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientRejectsKeysWithDotDotSegments( require.Contains(suite.T(), err.Error(), "uploads/user-a/../../protected/release.bin") } -// Accept rows compare joined paths because the helper returns the key -// uncleaned; filepath.Join is what collapses "a//b" and "./a.txt". -func (suite *AWSTestSuite) TestLocalPathForS3Key() { - for _, t := range []struct { - name string - key string - wantPath string // accept: the path filepath.Join(dir, key) produced before this change - wantErr bool - wantErrMsg string - }{ - {name: "an ordinary nested key", key: "protected/release.bin", wantPath: "protected/release.bin"}, - {name: "a plain filename", key: "a.txt", wantPath: "a.txt"}, - {name: "a short nested key", key: "a/z", wantPath: "a/z"}, - {name: "a dotfile", key: ".kosli_ignore", wantPath: ".kosli_ignore"}, - {name: "a key with spaces", key: "file with spaces.txt", wantPath: "file with spaces.txt"}, - {name: "a key with punctuation", key: "weird!*'().txt", wantPath: "weird!*'().txt"}, - {name: "a unicode key", key: "ünïcödé/файл.txt", wantPath: "ünïcödé/файл.txt"}, - {name: "a dot followed by a space is a literal name", key: ". ", wantPath: ". "}, - {name: "a name that merely starts with two dots", key: "..hidden", wantPath: "..hidden"}, - {name: "a backslash key is a literal filename on this OS", key: `dir\file.txt`, wantPath: `dir\file.txt`}, - {name: "a leading slash is trimmed", key: "/etc/passwd", wantPath: "etc/passwd"}, - {name: "doubled leading slashes are trimmed", key: "//x", wantPath: "x"}, - {name: "a leading dot segment is dropped by Join", key: "./a.txt", wantPath: "a.txt"}, - {name: "a doubled interior slash is collapsed by Join", key: "a//b", wantPath: "a/b"}, - {name: "a dot segment is dropped by Join", key: "a/./b", wantPath: "a/b"}, - // filepath.IsLocal rejects reserved device names and colons on Windows only. - {name: "a reserved Windows name", key: "CON", wantPath: "CON", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, - {name: "a drive-looking segment", key: "C:evil", wantPath: "C:evil", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, - {name: "a colon segment", key: "a:b", wantPath: "a:b", wantErr: runtime.GOOS == "windows", wantErrMsg: "is not a local path"}, - { - name: "a traversing key is rejected", - key: "uploads/user-a/../../protected/release.bin", - wantErr: true, - wantErrMsg: `resolves to ".."`, - }, - { - name: "a backslash-separated traversal is rejected", - key: `uploads/user-a/..\..\..\..\Users\Public\kosli-poc.txt`, - wantErr: true, - wantErrMsg: `resolves to ".."`, - }, - { - name: "a short backslash-separated traversal is rejected", - key: `uploads/user-a/..\x`, - wantErr: true, - wantErrMsg: `resolves to ".."`, - }, - {name: "a bare \"..\" is rejected", key: "..", wantErr: true, wantErrMsg: `resolves to ".."`}, - // Windows drops trailing spaces and dots from a name, so these resolve as "..". - {name: "a \"..\" with a trailing space is rejected", key: "a/.. /x", wantErr: true, wantErrMsg: `resolves to ".."`}, - {name: "three dots are rejected", key: "a/.../b", wantErr: true, wantErrMsg: `resolves to ".."`}, - {name: "a bare \"./.\" is rejected", key: "./.", wantErr: true, wantErrMsg: "names no file"}, - {name: "a trailing \"..\" segment is rejected", key: "a/..", wantErr: true, wantErrMsg: `resolves to ".."`}, - {name: "an empty key is rejected", key: "", wantErr: true, wantErrMsg: "names no file"}, - {name: "a bare slash is rejected", key: "/", wantErr: true, wantErrMsg: "names no file"}, - {name: "doubled slashes with nothing else are rejected", key: "//", wantErr: true, wantErrMsg: "names no file"}, - {name: "a bare dot is rejected", key: ".", wantErr: true, wantErrMsg: "names no file"}, - } { - suite.Run(t.name, func() { - got, err := localPathForS3Key(t.key) - if t.wantErr { - require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), t.key) - require.Contains(suite.T(), err.Error(), t.wantErrMsg) - return - } - require.NoError(suite.T(), err) - require.Equal(suite.T(), filepath.Join("base", t.wantPath), filepath.Join("base", got)) - }) - } -} - -func (suite *AWSTestSuite) TestDownloadFileFromBucketRefusesToOverwrite() { - tempDir := suite.T().TempDir() - preexisting := filepath.Join(tempDir, "README.md") - require.NoError(suite.T(), os.WriteFile(preexisting, []byte("pre-existing content\n"), 0666)) - - client := &FakeS3Client{ - Bucket: fakeS3TestBucketName, - Objects: map[string][]byte{ - "README.md": []byte(fakeReadmeBody), - }, - } - - err := downloadFileFromBucket(client, tempDir, "README.md", fakeS3TestBucketName, logger.NewStandardLogger()) - require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), "object key [README.md]") - require.Contains(suite.T(), err.Error(), "--exclude-regex") - - content, readErr := os.ReadFile(preexisting) - require.NoError(suite.T(), readErr) - require.Equal(suite.T(), "pre-existing content\n", string(content), - "the pre-existing file must be left untouched, not truncated") -} - -// Neither key holds a ".." segment, so O_EXCL is what catches this pair. +// Neither key holds a ".." segment; both fold onto one virtual path. func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() { client := &FakeS3Client{ Bucket: fakeS3TestBucketName, @@ -1355,13 +1256,13 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientCollidingKeysAreAnError() { _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) require.Error(suite.T(), err) - // '/' sorts before 'b', so "a//b" downloads first and "a/b" collides. - require.Contains(suite.T(), err.Error(), "object key [a/b]", "the error must name the key that collided") + require.Contains(suite.T(), err.Error(), "[a//b]", "the error must name both colliding keys") + require.Contains(suite.T(), err.Error(), "[a/b]", "the error must name both colliding keys") require.Contains(suite.T(), err.Error(), "--exclude-regex") } -// An object "a" alongside the prefix "a/" is legal in S3 and impossible on a -// filesystem, so MkdirAll fails with ENOTDIR once "a" lands first. +// An object "a" alongside the prefix "a/" is legal in S3 and impossible in a +// directory tree. func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnError() { client := &FakeS3Client{ Bucket: fakeS3TestBucketName, @@ -1373,42 +1274,11 @@ func (suite *AWSTestSuite) TestGetS3DataFromClientObjectAndPrefixCollideAreAnErr _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), "object key [a/b]") + require.Contains(suite.T(), err.Error(), "object key [a]", "the error must name the object") + require.Contains(suite.T(), err.Error(), "object key [a/b]", "the error must name an object under the prefix") require.Contains(suite.T(), err.Error(), "--exclude-regex") } -func (suite *AWSTestSuite) TestDownloadFileFromBucketNamesTheKeyWithoutAdviceOnFilesystemErrors() { - if runtime.GOOS == "windows" { - suite.T().Skip("chmod on a directory does not block file creation on Windows") - } - if os.Getuid() == 0 { - suite.T().Skip("root ignores directory permissions") - } - tempDir := suite.T().TempDir() - require.NoError(suite.T(), os.Chmod(tempDir, 0500)) - suite.T().Cleanup(func() { _ = os.Chmod(tempDir, 0700) }) - - client := &FakeS3Client{ - Bucket: fakeS3TestBucketName, - Objects: map[string][]byte{ - "README.md": []byte(fakeReadmeBody), - "sub/README.md": []byte(fakeReadmeBody), - }, - } - - // A bare key fails in OpenFile; a nested one has a directory left to - // create, so it fails in MkdirAll. - for _, key := range []string{"README.md", "sub/README.md"} { - suite.Run(key, func() { - err := downloadFileFromBucket(client, tempDir, key, fakeS3TestBucketName, logger.NewStandardLogger()) - require.Error(suite.T(), err) - require.ErrorIs(suite.T(), err, fs.ErrPermission) - require.Contains(suite.T(), err.Error(), fmt.Sprintf("object key [%s]", key)) - require.NotContains(suite.T(), err.Error(), "--exclude-regex") - }) - } -} - // Equal fingerprints mean the odd-shaped keys landed on the same paths as the // plain ones. func (suite *AWSTestSuite) TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys() { diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go new file mode 100644 index 000000000..a41b1c0c7 --- /dev/null +++ b/internal/aws/s3_fingerprint_test.go @@ -0,0 +1,242 @@ +package aws + +import ( + "context" + "os" + "path/filepath" + "sort" + "sync" + "testing" + + "github.com/aws/aws-sdk-go-v2/feature/s3/transfermanager" + "github.com/kosli-dev/cli/internal/digest" + "github.com/kosli-dev/cli/internal/logger" + "github.com/kosli-dev/cli/internal/utils" + "github.com/stretchr/testify/require" + "github.com/stretchr/testify/suite" +) + +type S3FingerprintTestSuite struct { + suite.Suite +} + +// recordingDownloader wraps an S3API and records every DownloadObject call: the +// key, and the local file the bytes were written to. +type recordingDownloader struct { + S3API + mu sync.Mutex + keys []string + files []string + // onDownload runs inside each DownloadObject call, before delegating. + onDownload func(key string, file *os.File) +} + +func (r *recordingDownloader) DownloadObject(ctx context.Context, params *transfermanager.DownloadObjectInput, optFns ...func(*transfermanager.Options)) (*transfermanager.DownloadObjectOutput, error) { + file, _ := params.WriterAt.(*os.File) + r.mu.Lock() + r.keys = append(r.keys, *params.Key) + if file != nil { + r.files = append(r.files, file.Name()) + } + r.mu.Unlock() + if r.onDownload != nil { + r.onDownload(*params.Key, file) + } + return r.S3API.DownloadObject(ctx, params, optFns...) +} + +func (r *recordingDownloader) downloadedKeys() []string { + r.mu.Lock() + defer r.mu.Unlock() + keys := append([]string{}, r.keys...) + sort.Strings(keys) + return keys +} + +func snapshotFake(t *testing.T, client S3API) (artifactName, fingerprint string) { + t.Helper() + data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(t, err) + require.Len(t, data, 1) + require.Len(t, data[0].Digests, 1) + for name, sha := range data[0].Digests { + return name, sha + } + return "", "" +} + +// TestPinnedFingerprints holds the fingerprints recorded against main before +// content mode stopped writing objects under their keys. They must never move: +// every existing environment snapshot on the server was computed this way. +func (suite *S3FingerprintTestSuite) TestPinnedFingerprints() { + for _, t := range []struct { + name string + objects map[string][]byte + wantArtifactName string + wantFingerprint string + }{ + { + name: "unusual key shapes fold as filepath.Join folded them", + objects: map[string][]byte{ + "/lead.txt": []byte("u\n"), "a//b": []byte("o\n"), "./c.txt": []byte("t\n"), `d\e.txt`: []byte("n\n"), + }, + wantArtifactName: fakeS3TestBucketName, + wantFingerprint: "27e2d8aa07677b7818b8cf101b45c928aa8fbf7d17a8c8efc84469e24a106ec3", + }, + { + name: "a dot sorts before a slash", + objects: map[string][]byte{ + "a.txt": []byte("1"), "a/z": []byte("2"), "a/b/c": []byte("3"), "b": []byte("4"), + }, + wantArtifactName: fakeS3TestBucketName, + wantFingerprint: "aaddb8f3e299e12316d42fa9ac36d4ed7d38c34e7ec002239269c86a033bb9fd", + }, + { + name: "nested prefixes with folder markers", + objects: map[string][]byte{ + "dir/": nil, "dir/sub/": nil, "dir/sub/x.yml": []byte("x"), "dir/y.txt": []byte("y"), "README.md": []byte("r"), + }, + wantArtifactName: fakeS3TestBucketName, + wantFingerprint: "c26910cdb177dde3c6493d18ba1e916b04715cdfc535e4552b5f9831590e933a", + }, + { + name: "a single object is the file, named by its base name", + objects: map[string][]byte{"only/one/file.bin": []byte("solo")}, + wantArtifactName: "file.bin", + wantFingerprint: "5364f2f2fc4f54e9d47ad29cfb08ef430c8153394bf2a0dff5cbe77a0ffef861", + }, + } { + suite.Run(t.name, func() { + name, sha := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: t.objects}) + require.Equal(suite.T(), t.wantArtifactName, name) + require.Equal(suite.T(), t.wantFingerprint, sha) + }) + } +} + +// TestMatchesAttestedDirectory is the property the whole command exists for: +// a bucket holding the files of a directory fingerprints as that directory does +// at attestation time, and a single object as that file does. +func (suite *S3FingerprintTestSuite) TestMatchesAttestedDirectory() { + tree := map[string]string{ + "README.md": "# readme\n", + "dummy/dummy_2/template.yml": "key: value\n", + "dummy/other.txt": "other\n", + "a.txt": "a\n", + "a/z": "z\n", + ".kosli_ignore": "logs\n*.tmp\n", + "logs/noise.log": "noise\n", + "scratch.tmp": "tmp\n", + } + root := suite.T().TempDir() + objects := map[string][]byte{} + for p, content := range tree { + require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, filepath.FromSlash(p)), content)) + objects[p] = []byte(content) + } + attested, err := digest.DirSha256(root, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + name, sha := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects}) + require.Equal(suite.T(), fakeS3TestBucketName, name) + require.Equal(suite.T(), attested, sha) + + suite.Run("a single object", func() { + path := filepath.Join(suite.T().TempDir(), "release.bin") + require.NoError(suite.T(), utils.CreateFileWithContent(path, "the release")) + attestedFile, err := digest.FileSha256(path, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + name, sha := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, + Objects: map[string][]byte{"builds/v1/release.bin": []byte("the release")}}) + require.Equal(suite.T(), "release.bin", name) + require.Equal(suite.T(), attestedFile, sha) + }) +} + +// A root .kosli_ignore in the bucket applies its rules, as DirSha256 applies +// them on disk: a bucket with an excluded directory fingerprints as one that +// never held it. +func (suite *S3FingerprintTestSuite) TestHonoursRootKosliIgnore() { + ignore := []byte("logs\n") + _, withLogs := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + ".kosli_ignore": ignore, "app.js": []byte("app"), "logs/a.log": []byte("a"), "logs/deep/b.log": []byte("b"), + }}) + _, withoutLogs := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + ".kosli_ignore": ignore, "app.js": []byte("app"), + }}) + require.Equal(suite.T(), withoutLogs, withLogs) + + _, noIgnore := snapshotFake(suite.T(), &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + "app.js": []byte("app"), "logs/a.log": []byte("a"), "logs/deep/b.log": []byte("b"), + }}) + require.NotEqual(suite.T(), noIgnore, withLogs, "the ignore file must have had an effect") +} + +// Exactly the objects that contribute content are downloaded: not folder +// markers, not filter-excluded keys, not keys the root .kosli_ignore excludes. +// Nothing else is skipped, so no object is dropped silently. +func (suite *S3FingerprintTestSuite) TestDownloadsExactlyTheContributingObjects() { + client := &recordingDownloader{S3API: &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + ".kosli_ignore": []byte("logs\n*.tmp\n"), + "app.js": []byte("app"), + "lib/util.js": []byte("util"), + "lib/": nil, + "logs/a.log": []byte("a"), + "logs/deep/b.log": []byte("b"), + "scratch.tmp": []byte("tmp"), + "filtered/out.txt": []byte("out"), + }}} + data, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, []string{"filtered/"}, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), data, 1) + require.Equal(suite.T(), []string{".kosli_ignore", "app.js", "lib/util.js"}, client.downloadedKeys()) +} + +// Objects are downloaded to files whose names owe nothing to the key, each is +// removed once hashed, and the download directory is gone at the end. +func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLinger() { + keys := []string{"alpha.bin", "beta/gamma.bin", "delta/epsilon/zeta.bin"} + objects := map[string][]byte{} + for _, key := range keys { + objects[key] = []byte(key) + } + client := &recordingDownloader{S3API: &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects}} + client.onDownload = func(key string, file *os.File) { + require.NotNil(suite.T(), file, "the transfer manager must be handed a real file") + require.NotContains(suite.T(), filepath.Base(file.Name()), filepath.Base(key), + "the local file name must owe nothing to the key") + client.mu.Lock() + earlier := append([]string{}, client.files[:len(client.files)-1]...) + client.mu.Unlock() + for _, previous := range earlier { + _, err := os.Stat(previous) + require.ErrorIs(suite.T(), err, os.ErrNotExist, "an earlier object's file must be gone before the next download") + } + } + + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Len(suite.T(), client.files, len(keys)) + for _, file := range client.files { + _, err := os.Stat(file) + require.ErrorIs(suite.T(), err, os.ErrNotExist) + _, err = os.Stat(filepath.Dir(file)) + require.ErrorIs(suite.T(), err, os.ErrNotExist, "the download directory must be removed") + } +} + +func (suite *S3FingerprintTestSuite) TestADownloadErrorNamesTheKey() { + client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + "README.md": []byte(fakeReadmeBody), "notes.txt": []byte(fakeNotesBody), + }, DownloadObjectErr: os.ErrDeadlineExceeded} + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.ErrorIs(suite.T(), err, os.ErrDeadlineExceeded) + require.Contains(suite.T(), err.Error(), "object key [README.md]") + require.NotContains(suite.T(), err.Error(), "--exclude-regex", "a transport failure must not advise dropping the object") +} + +func TestS3FingerprintTestSuite(t *testing.T) { + suite.Run(t, new(S3FingerprintTestSuite)) +} diff --git a/internal/digest/digest.go b/internal/digest/digest.go index 0774eac1a..68eaefcd5 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -32,8 +32,10 @@ var ( "has it been pushed to or pulled from a registry?") ) -// ignoreFileName is the exclusion list a directory artifact may carry at its root. -const ignoreFileName = ".kosli_ignore" +// IgnoreFileName is the exclusion list a directory artifact may carry at its root. +const IgnoreFileName = ".kosli_ignore" + +const ignoreFileName = IgnoreFileName // DirSha256 returns sha256 digest of a directory func DirSha256(dirPath string, excludePaths []string, logger *logger.Logger) (string, error) { diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index e5cee7508..dbee89fb4 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -4,7 +4,6 @@ import ( "crypto/sha256" "encoding/hex" "fmt" - "hash" "path" "sort" "strings" @@ -80,10 +79,67 @@ func VirtualDirSha256(files []VirtualFile, ignoreRules []string, logger *logger. logger.Debug("calculating fingerprint for a virtual tree of %d files -- excluding %d paths", len(files), len(excluded)) hasher := sha256.New() - root.writeDigests(hasher, virtualRoot, excluded, path.Join(virtualRoot, ignoreFileName), logger) + err = root.walkIncluded(virtualRoot, excluded, protectedVirtualPath(), logger, func(childPath string, child *virtualNode) error { + nameSha256 := sha256OfString(child.name) + hasher.Write([]byte(nameSha256)) //nolint:errcheck // hash.Hash never returns an error + if child.isDir { + logger.Debug("dir: %s -- dirname digest: %s", child.name, nameSha256) + return nil + } + if child.sha256 == "" { + return fmt.Errorf("no content digest for %q, whose content the fingerprint needs", relativeVirtualPath(childPath)) + } + logger.Debug("file: %s -- filename digest: %s -- content digest: %s", child.name, nameSha256, child.sha256) + hasher.Write([]byte(child.sha256)) //nolint:errcheck // hash.Hash never returns an error + return nil + }) + if err != nil { + return "", err + } return hex.EncodeToString(hasher.Sum(nil)), nil } +// FilesNeedingContent reports which of paths VirtualDirSha256 reads the content +// digest of under these ignore rules. A file it leaves out is skipped by the +// rules, so its content need not be fetched and it may be passed with an empty +// Sha256 without changing the fingerprint. The two share one walk, so they +// cannot disagree. +func FilesNeedingContent(paths []string, ignoreRules []string) (map[string]bool, error) { + files := make([]VirtualFile, len(paths)) + for i, p := range paths { + files[i] = VirtualFile{Path: p} + } + root, err := buildVirtualTree(files) + if err != nil { + return nil, err + } + excluded, err := virtualFS{root: root}.excludedPaths(ignoreRules) + if err != nil { + return nil, err + } + needed := map[string]bool{} + err = root.walkIncluded(virtualRoot, excluded, protectedVirtualPath(), nil, func(childPath string, child *virtualNode) error { + if !child.isDir { + needed[relativeVirtualPath(childPath)] = true + } + return nil + }) + if err != nil { + return nil, err + } + return needed, nil +} + +// protectedVirtualPath is the root ignore file, which its own rules never exclude. +func protectedVirtualPath() string { + return path.Join(virtualRoot, ignoreFileName) +} + +// relativeVirtualPath strips the synthetic root from a tree path. +func relativeVirtualPath(p string) string { + return strings.TrimPrefix(p, virtualRoot+"/") +} + // virtualNode is a directory or a file in the virtual tree. Files are leaves // and carry a content digest; directories carry children keyed by base name. type virtualNode struct { @@ -103,8 +159,12 @@ func buildVirtualTree(files []VirtualFile) (*virtualNode, error) { if err := validateVirtualPath(file.Path); err != nil { return nil, err } - if err := ValidateDigest(file.Sha256); err != nil { - return nil, fmt.Errorf("invalid fingerprint for %q: %w", file.Path, err) + // An empty digest means the content was not read. That is only acceptable + // for a file the rules exclude, which writeDigests enforces when it gets there. + if file.Sha256 != "" { + if err := ValidateDigest(file.Sha256); err != nil { + return nil, fmt.Errorf("invalid fingerprint for %q: %w", file.Path, err) + } } segments := strings.Split(file.Path, "/") @@ -135,35 +195,36 @@ func buildVirtualTree(files []VirtualFile) (*virtualNode, error) { return root, nil } -// writeDigests appends this node's children to the hash in WalkDir order, -// skipping excluded entries as calculateDirContentSha256 does: an excluded -// directory takes its subtree with it, and the protected path is kept whatever -// the rules say. -func (n *virtualNode) writeDigests(hasher hash.Hash, dir string, excluded map[string]bool, protected string, logger *logger.Logger) { +// walkIncluded visits this node's children in WalkDir order, skipping excluded +// entries as calculateDirContentSha256 does: an excluded directory takes its +// subtree with it, and the protected path is kept whatever the rules say. +// A nil logger is allowed for callers that only want the visits. +func (n *virtualNode) walkIncluded(dir string, excluded map[string]bool, protected string, logger *logger.Logger, + visit func(childPath string, child *virtualNode) error) error { for _, name := range n.sortedChildNames() { child := n.children[name] childPath := path.Join(dir, name) if excluded[childPath] { - if childPath == protected { - logger.Debug("keeping %s although an exclusion matches it: an exclusion list cannot exclude itself", childPath) - } else { - logger.Debug("skipping %s as it matches excluded paths", childPath) + if childPath != protected { + if logger != nil { + logger.Debug("skipping %s as it matches excluded paths", childPath) + } continue } + if logger != nil { + logger.Debug("keeping %s although an exclusion matches it: an exclusion list cannot exclude itself", childPath) + } + } + if err := visit(childPath, child); err != nil { + return err } - - nameSha256 := sha256OfString(child.name) - hasher.Write([]byte(nameSha256)) //nolint:errcheck // hash.Hash never returns an error - if child.isDir { - logger.Debug("dir: %s -- dirname digest: %s", child.name, nameSha256) - child.writeDigests(hasher, childPath, excluded, protected, logger) - continue + if err := child.walkIncluded(childPath, excluded, protected, logger, visit); err != nil { + return err + } } - logger.Debug("file: %s -- filename digest: %s -- content digest: %s", - child.name, nameSha256, child.sha256) - hasher.Write([]byte(child.sha256)) //nolint:errcheck // hash.Hash never returns an error } + return nil } // sortedChildNames returns child names in the byte order os.ReadDir uses, so diff --git a/internal/digest/virtualdir_test.go b/internal/digest/virtualdir_test.go index def791a5f..daf80f393 100644 --- a/internal/digest/virtualdir_test.go +++ b/internal/digest/virtualdir_test.go @@ -198,6 +198,13 @@ func (suite *VirtualDirTestSuite) TestVirtualDirSha256Errors() { files: []VirtualFile{{Path: "a.txt", Sha256: "not-a-digest"}}, wantErrMsg: "not a valid SHA256 fingerprint", }, + { + // An empty digest means the content was not read; that is only + // acceptable for a file the rules exclude. + name: "a missing sha256 on a file that is hashed", + files: []VirtualFile{{Path: "a.txt", Sha256: ""}}, + wantErrMsg: "no content digest", + }, { name: "an uppercase sha256", files: []VirtualFile{{Path: "a.txt", Sha256: strings.ToUpper(validSha)}}, diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index 06d276b9a..37a9a4a34 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -175,6 +175,86 @@ func (suite *VirtualIgnoreTestSuite) TestNestedIgnoreFileIsExcludable() { require.Equal(suite.T(), b, a) } +// FilesNeedingContent is what lets a caller skip fetching excluded content: it +// must name exactly the files whose digest VirtualDirSha256 reads, so a file it +// leaves out may carry no digest at all without changing the fingerprint. +func (suite *VirtualIgnoreTestSuite) TestFilesNeedingContentAgreesWithTheDigest() { + for _, t := range []struct { + name string + rules []string + want []string + }{ + {name: "no rules hash everything", rules: nil, want: allPaths(ignoreTestTree, true)}, + {name: "a directory takes its files with it", rules: []string{"logs"}, want: without(allPaths(ignoreTestTree, true), "logs/file1", "logs/deep/file2")}, + // "*" matches the subdirectory "deep" as well, and an excluded directory + // takes its subtree with it. + {name: "the content of a directory", rules: []string{"logs/*"}, want: without(allPaths(ignoreTestTree, true), "logs/file1", "logs/deep/file2")}, + {name: "a suffix at any depth", rules: []string{"**/*.log"}, want: without(allPaths(ignoreTestTree, true), "app.log")}, + {name: "the ignore file cannot exclude itself", rules: []string{".kosli_ignore", "**"}, want: []string{".kosli_ignore"}}, + } { + suite.Run(t.name, func() { + paths := allPaths(ignoreTestTree, true) + needed, err := FilesNeedingContent(paths, t.rules) + require.NoError(suite.T(), err) + got := make([]string, 0, len(needed)) + for p := range needed { + got = append(got, p) + } + sort.Strings(got) + sort.Strings(t.want) + require.Equal(suite.T(), t.want, got) + + // Files not needed may carry no digest and the fingerprint is unchanged. + full := virtualFilesFor(ignoreTestTree, "rules") + sparse := make([]VirtualFile, 0, len(full)) + for _, f := range full { + if !needed[f.Path] { + f.Sha256 = "" + } + sparse = append(sparse, f) + } + wantSha, err := VirtualDirSha256(full, t.rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + gotSha, err := VirtualDirSha256(sparse, t.rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), wantSha, gotSha) + }) + } +} + +func (suite *VirtualIgnoreTestSuite) TestFilesNeedingContentRejectsAMalformedRule() { + _, err := FilesNeedingContent([]string{"a.txt"}, []string{"["}) + require.Error(suite.T(), err) +} + +func allPaths(tree map[string]string, withIgnoreFile bool) []string { + paths := make([]string, 0, len(tree)+1) + for p := range tree { + paths = append(paths, p) + } + if withIgnoreFile { + paths = append(paths, ignoreFileName) + } + sort.Strings(paths) + return paths +} + +func without(paths []string, drop ...string) []string { + kept := make([]string, 0, len(paths)) + for _, p := range paths { + skip := false + for _, d := range drop { + if p == d { + skip = true + } + } + if !skip { + kept = append(kept, p) + } + } + return kept +} + func (suite *VirtualIgnoreTestSuite) TestParseIgnoreRules() { for _, t := range []struct { name string From 3b9629db80f0ffea073ca897b02618c5958059eb Mon Sep 17 00:00:00 2001 From: Marko Bevc Date: Mon, 14 Sep 2026 18:05:06 +0100 Subject: [PATCH 08/30] docs(adr): make the S3 virtual-tree record stand on its own Drop references that only made sense during development: the throwaway experiment and the gitignored TODO.md. Name the tests that hold each guarantee, describe the .kosli_ignore resolution as the filepathx simulation that was actually built, mark parallel downloads as delivered separately in #1167, and correct the claim that the codebase gets smaller: the platform-dependent code is gone, but the faithful glob simulation costs more lines than the fenced layout it replaces. --- ...260911-s3-fingerprint-from-virtual-tree.md | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index d3f1dcbd3..3ae66965b 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -35,21 +35,19 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin 3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped like `combineUnusableObjectErrors`, with the existing advice to use `--exclude-regex` or narrow the include filter. -4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed with the same comment and whitespace handling as `excludePathsFromFile` and matched against virtual paths with a `**`-capable segment matcher. A matched directory drops its subtree. The ignore file can never exclude itself, as in `DirSha256`. Excluded and ignored objects are not downloaded at all. Equivalence with `DirSha256` on a materialised tree is asserted for every ignore-file case the digest tests already hold. +4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed by `digest.ParseIgnoreRules`, the same reading `DirSha256` gives the file, and resolved by `digest/virtualglob.go`, which reproduces `filepathx.Glob`, `filepath.Glob` and `filepath.Walk` step for step over the virtual tree rather than reimplementing what the globs appear to mean. That is what keeps their quirks identical: a literal `**/x` never matches at the root because the pieces concatenate to a double slash, `**/*.log` does, and excluding `logs/*` leaves an empty directory whose name is still hashed. Exclusion therefore runs inside the tree walk, not by filtering the file list. The ignore file can never exclude itself, as in `DirSha256`. `digest.FilesNeedingContent` shares that walk so excluded objects are not downloaded at all, and `VirtualDirSha256` refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Equivalence with `DirSha256` on a materialised tree is asserted for every rule shape in `TestVirtualIgnoreTestSuite`. -5. **Downloads run in parallel** behind a bounded semaphore and a bytes-in-flight budget derived from listing sizes, with results written by index for determinism, a cancellable context so the first transport error stops in-flight multipart downloads, and per-key errors collected rather than aborting. +5. **Downloads may run in parallel**, behind a fixed worker pool and a bytes-in-flight budget derived from listing sizes, with results written by index for determinism and a cancellable context so the first transport error stops in-flight multipart downloads. This is a performance property, not a safety one, and is delivered separately from the change this record describes; see #1167. Until it lands, downloads are sequential, exactly as before, and peak temp disk is one object. -6. **The switch is pinned, not argued.** Fingerprints of representative fake buckets are recorded against `main` before the implementation changes and asserted afterwards, alongside `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys` from #1155, which must stay green through the change. - -A throwaway equivalence test run on 2026-09-11 confirmed that `VirtualDirSha256` fed by rule 2 reproduces today's on-disk fingerprint for the #1155 unusual-keys bucket, the `a.txt` beside `a/z` ordering case, nested prefixes with folder markers, and the single-object case including its basename artifact name. +6. **The switch is pinned, not argued.** `TestPinnedFingerprints` in `internal/aws` holds fingerprints of representative fake buckets recorded against the key-layout implementation before it was replaced: unusual key shapes, a `.` sorting before a `/`, nested prefixes with folder markers, and a single object with its basename as artifact name. It was green before the switch and must stay green after it, alongside `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys` from #1155. ## Guarantees The attest side is unchanged: `kosli attest artifact --artifact-type dir` still runs `DirSha256` on a real directory, and that defines the format. The snapshot side re-derives the same value from the bucket. Three guarantees follow, each with the test that holds it: -- **Snapshot equals attestation.** `VirtualDirSha256` of the bucket's `(path, sha256)` pairs equals `DirSha256` of the directory those objects were uploaded from, and a single object equals `FileSha256` plus its basename. Held by the materialise-then-compare tests in `internal/digest` and an end-to-end test in `internal/aws` that hashes a directory, uploads its files to the fake bucket and snapshots it. -- **Snapshot equals its own history.** Every bucket that snapshots successfully on `main` keeps its fingerprint and artifact name, because the fold rule is what `filepath.Join` did. Held by fingerprints recorded before the switch and by `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys`. -- **No object is lost silently.** Every key S3 accepts is representable, whatever the operating system, because the key never becomes a path. The only rejections are keys that cannot form a directory tree at all: a `..` segment, two keys folding onto one path, and an object that is also a prefix. S3 permits those, no filesystem does, so no attested directory could match them. Each rejection fails the snapshot and names every key involved; the manifest is never shortened to make a fingerprint. Held by a generated-key representability test and a manifest-count invariant test. +- **Snapshot equals attestation.** `VirtualDirSha256` of the bucket's `(path, sha256)` pairs equals `DirSha256` of the directory those objects were uploaded from, and a single object equals `FileSha256` plus its basename. Held by the materialise-then-compare tests in `internal/digest` (`TestVirtualDirTestSuite`, `TestVirtualIgnoreTestSuite`) and by `TestMatchesAttestedDirectory` in `internal/aws`, which hashes a directory, uploads its files to the fake bucket and snapshots it. +- **Snapshot equals its own history.** Every bucket that snapshotted successfully under the key-layout implementation keeps its fingerprint and artifact name, because the fold rule is what `filepath.Join` did. Held by `TestPinnedFingerprints` and `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys`. +- **No object is lost silently.** Every key S3 accepts is representable, whatever the operating system, because the key never becomes a path. The only rejections are keys that cannot form a directory tree at all: a `..` segment, two keys folding onto one path, and an object that is also a prefix. S3 permits those, no filesystem does, so no attested directory could match them. Each rejection fails the snapshot and names every key involved; the manifest is never shortened to make a fingerprint. Held by `TestEveryS3KeyIsRepresentable`, which generates keys over S3's full character set, and `TestDownloadsExactlyTheContributingObjects`, which asserts the downloaded set equals the listed objects minus markers and exclusions. ## Alternatives considered @@ -61,8 +59,8 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still ## Consequences - The fingerprint is the same on every operating system, with Linux semantics. A snapshot run on Windows or macOS against a bucket with case-colliding or backslash keys moves to the Linux value. This is a correction and is release-noted. -- `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. The codebase ends with less code than before #1155. +- `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. No code in the S3 path branches on the operating system any more. The codebase does not get smaller, though: the virtual tree, the key rule with its collision reporting, and above all the faithful simulation of `filepathx.Glob` add several hundred lines, most of them owed to reproducing `.kosli_ignore` semantics exactly. That is the price of the compatibility contract, paid once and shared with #1069. - `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. -- Excluded and ignored objects are not downloaded, and disk use is bounded by the in-flight budget rather than the bucket size. +- Excluded and ignored objects are not downloaded. Each object's temp file is removed once hashed, so peak temp disk falls from the whole bucket to one object, and to the configured budget once #1167 lands. - #1069 rebases onto the shared layer: metadata mode becomes a sha256 source plugged into the same list, normalise, exclude, tree pipeline, its key rule disappears in favour of rule 2, and its rejection of buckets with a root `.kosli_ignore` becomes a download of that one object. -- Delivery is sliced in `TODO.md` (local, gitignored): lift `VirtualDirSha256` with its tests; add the key rule and collision errors; add virtual ignore rules; switch content mode to temp files sequentially with pinned fingerprints; parallelise. +- Delivery is two pull requests. The first carries this decision with sequential downloads, so the security-relevant review is not mixed with performance work. The second, #1167, adds parallel downloads, the byte budget and the flags that tune them. From d5553dacac1204c73ae79420bab253a3efca6501 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 17:25:59 +0000 Subject: [PATCH 09/30] fix(snapshot s3): validate ignore rules before matching, and tidy review findings filepath.Glob rejects a malformed pattern before it looks at the filesystem, so a rule such as "nonexistent/a[" fails DirSha256 even though nothing could match it. The virtual mirror only surfaced the error from the per-name match, which is never reached when the directory is missing, so the same rule silently excluded nothing. The mirror now validates the pattern up front as Glob does; the parity test covers rules under missing directories and behind a double star. Also from review: the ignore-file closure no longer assigns to the function's named result, and the results are plain now; a bad rule is reported the same way whichever call reaches it, naming the bucket's .kosli_ignore only when the error is a bad pattern; the multi-problem message counts problems rather than keys, since a collision names several; the redundant ignoreFileName alias is gone; and the ADR names s3KeyProblemsError instead of a function that does not exist, and records that the ignore file is now matched by exact key. --- ...260911-s3-fingerprint-from-virtual-tree.md | 3 +- internal/aws/aws.go | 26 +++++++++++---- internal/aws/s3_fingerprint_test.go | 16 +++++++++ internal/aws/s3_keys.go | 2 +- internal/aws/s3_keys_test.go | 4 +-- internal/digest/digest.go | 10 +++--- internal/digest/virtualdir.go | 2 +- internal/digest/virtualglob.go | 5 +++ internal/digest/virtualignore_test.go | 33 ++++++++++++------- 9 files changed, 73 insertions(+), 28 deletions(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index 3ae66965b..f974803af 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -33,7 +33,7 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin 2. **The key becomes a virtual path by one rule, shared by both modes.** A key holding a `..` segment is rejected first, on the raw key, so `a/../b` cannot fold silently onto `b`. The rest is `path.Clean(strings.TrimLeft(key, "/"))`, rejecting a result of `.`. The fold is exactly what `filepath.Join` produced on Linux, so `a//b`, `./c.txt` and `/lead.txt` land where they do today. Everything else, including `...`, `.. `, `CON`, colons and backslashes, is an ordinary name because nothing is created under it. None of this is for safety. `DirSha256` walks real directories, which can never hold an entry named `.`, `..` or the empty string, so the rule keeps virtual fingerprints inside the space an attested directory can match, and preserves the fingerprints existing buckets already have. -3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped like `combineUnusableObjectErrors`, with the existing advice to use `--exclude-regex` or narrow the include filter. +3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped at ten by `s3KeyProblemsError`, with the existing advice to use `--exclude-regex` or narrow the include filter. 4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed by `digest.ParseIgnoreRules`, the same reading `DirSha256` gives the file, and resolved by `digest/virtualglob.go`, which reproduces `filepathx.Glob`, `filepath.Glob` and `filepath.Walk` step for step over the virtual tree rather than reimplementing what the globs appear to mean. That is what keeps their quirks identical: a literal `**/x` never matches at the root because the pieces concatenate to a double slash, `**/*.log` does, and excluding `logs/*` leaves an empty directory whose name is still hashed. Exclusion therefore runs inside the tree walk, not by filtering the file list. The ignore file can never exclude itself, as in `DirSha256`. `digest.FilesNeedingContent` shares that walk so excluded objects are not downloaded at all, and `VirtualDirSha256` refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Equivalence with `DirSha256` on a materialised tree is asserted for every rule shape in `TestVirtualIgnoreTestSuite`. @@ -60,6 +60,7 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still - The fingerprint is the same on every operating system, with Linux semantics. A snapshot run on Windows or macOS against a bucket with case-colliding or backslash keys moves to the Linux value. This is a correction and is release-noted. - `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. No code in the S3 path branches on the operating system any more. The codebase does not get smaller, though: the virtual tree, the key rule with its collision reporting, and above all the faithful simulation of `filepathx.Glob` add several hundred lines, most of them owed to reproducing `.kosli_ignore` semantics exactly. That is the price of the compatibility contract, paid once and shared with #1069. +- The bucket's ignore file is recognised by the exact key `.kosli_ignore`. On disk, `ignoreFilePathInTree` also accepted a case-folded spelling such as `.KOSLI_IGNORE` where the operator's filesystem folded case, so on macOS or Windows such a bucket had its rules applied; now it does not, its exclusions stop applying and its fingerprint moves to the Linux value. Same correction as the key case above, and release-noted with it. - `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. - Excluded and ignored objects are not downloaded. Each object's temp file is removed once hashed, so peak temp disk falls from the whole bucket to one object, and to the configured budget once #1167 lands. - #1069 rebases onto the shared layer: metadata mode becomes a sha256 source plugged into the same list, normalise, exclude, tree pipeline, its key rule disappears in favour of rule 2, and its rejection of buckets with a root `.kosli_ignore` becomes a download of that one object. diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 314a80f6c..cfbf9d641 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/hex" + "errors" "fmt" "io" "os" @@ -527,7 +528,7 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin // // A root .kosli_ignore is downloaded first so its rules can be applied, and // objects the rules exclude are not downloaded at all. -func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, logger *logger.Logger) (artifactName, sha256 string, err error) { +func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3Object, logger *logger.Logger) (string, string, error) { keys := make([]string, len(objects)) for i, object := range objects { keys[i] = object.key @@ -565,8 +566,12 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O if _, err := file.Seek(0, io.SeekStart); err != nil { return err } - rules, err = digest.ParseIgnoreRules(file) - return err + parsed, err := digest.ParseIgnoreRules(file) + if err != nil { + return err + } + rules = parsed + return nil }, logger) if err != nil { return "", "", err @@ -581,7 +586,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O } needed, err := digest.FilesNeedingContent(allPaths, rules) if err != nil { - return "", "", fmt.Errorf("invalid rule in the bucket's %s: %w", digest.IgnoreFileName, err) + return "", "", ignoreRuleError(err) } files := make([]digest.VirtualFile, 0, len(keys)) @@ -601,13 +606,22 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O files = append(files, digest.VirtualFile{Path: virtualPath, Sha256: sha256}) } - sha256, err = digest.VirtualDirSha256(files, rules, logger) + sha256, err := digest.VirtualDirSha256(files, rules, logger) if err != nil { - return "", "", err + return "", "", ignoreRuleError(err) } return bucket, sha256, nil } +// ignoreRuleError names the bucket's ignore file when one of its rules cannot +// be applied, and leaves any other failure as it is. +func ignoreRuleError(err error) error { + if errors.Is(err, path.ErrBadPattern) { + return fmt.Errorf("the bucket's %s holds a rule that cannot be applied: %w", digest.IgnoreFileName, err) + } + return err +} + // downloadAndHashS3Object fetches one object into a fresh temp file, lets // inspect read it when given, returns the sha256 of its content and removes the // file. The file's name comes from the OS, so nothing about the key reaches the diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index a41b1c0c7..28869716c 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -226,6 +226,22 @@ func (suite *S3FingerprintTestSuite) TestObjectsNeverLandUnderTheirKeyAndDoNotLi } } +// A malformed rule in the bucket's .kosli_ignore fails the snapshot and names +// the file, even when the rule points under a prefix the bucket does not have. +func (suite *S3FingerprintTestSuite) TestAMalformedIgnoreRuleFailsTheSnapshot() { + for _, rule := range []string{"[", "nonexistent/a["} { + suite.Run(rule, func() { + client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ + ".kosli_ignore": []byte(rule + "\n"), "app.js": []byte("app"), + }} + _, err := getS3DataFromClient(client, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "the bucket's .kosli_ignore holds a rule that cannot be applied") + require.Contains(suite.T(), err.Error(), rule) + }) + } +} + func (suite *S3FingerprintTestSuite) TestADownloadErrorNamesTheKey() { client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{ "README.md": []byte(fakeReadmeBody), "notes.txt": []byte(fakeNotesBody), diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index 9b7e1a3d4..2b803bd99 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -124,7 +124,7 @@ func s3KeyProblemsError(problems []string) error { shown = shown[:maxReportedS3KeyProblems] suffix = fmt.Sprintf("\n(and %d more)", len(problems)-maxReportedS3KeyProblems) } - return fmt.Errorf("%d object keys cannot be fingerprinted:\n%s%s\nexclude them with --exclude-regex, or narrow the include filter if one is set", + return fmt.Errorf("%d problems prevent the bucket from being fingerprinted:\n%s%s\nexclude the keys with --exclude-regex, or narrow the include filter if one is set", len(problems), strings.Join(shown, "\n"), suffix) } diff --git a/internal/aws/s3_keys_test.go b/internal/aws/s3_keys_test.go index 4630c2a54..18e7a2fd6 100644 --- a/internal/aws/s3_keys_test.go +++ b/internal/aws/s3_keys_test.go @@ -168,7 +168,7 @@ func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysReportsEveryBadKey() { _, err := virtualPathsForS3Keys([]string{"ok.txt", "../one", "two/..", "", "dup", "./dup"}) require.Error(suite.T(), err) msg := err.Error() - require.Contains(suite.T(), msg, "4 object keys cannot be fingerprinted") + require.Contains(suite.T(), msg, "4 problems prevent the bucket from being fingerprinted") require.Contains(suite.T(), msg, "[../one]") require.Contains(suite.T(), msg, "[two/..]") require.Contains(suite.T(), msg, "object key []") @@ -185,7 +185,7 @@ func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysCapsTheReport() { _, err := virtualPathsForS3Keys(keys) require.Error(suite.T(), err) msg := err.Error() - require.Contains(suite.T(), msg, "13 object keys cannot be fingerprinted") + require.Contains(suite.T(), msg, "13 problems prevent the bucket from being fingerprinted") require.Contains(suite.T(), msg, "(and 3 more)") require.Equal(suite.T(), maxReportedS3KeyProblems, strings.Count(msg, "object key [")) } diff --git a/internal/digest/digest.go b/internal/digest/digest.go index 68eaefcd5..4d1e4eed9 100644 --- a/internal/digest/digest.go +++ b/internal/digest/digest.go @@ -35,8 +35,6 @@ var ( // IgnoreFileName is the exclusion list a directory artifact may carry at its root. const IgnoreFileName = ".kosli_ignore" -const ignoreFileName = IgnoreFileName - // DirSha256 returns sha256 digest of a directory func DirSha256(dirPath string, excludePaths []string, logger *logger.Logger) (string, error) { logger.Debug("calculating fingerprint for path [%s] -- excluding paths: %s", dirPath, excludePaths) @@ -237,12 +235,12 @@ func Sha256Fingerprint(parsed godigest.Digest) (string, error) { // stores one spelling and opens any of them. Snapshotting S3 or Azure unzips the // tree onto the machine running the CLI, so that filesystem is the operator's. // -// An exact match wins over a folded one so that ignoreFileName owns the rules +// An exact match wins over a folded one so that IgnoreFileName owns the rules // where a case-sensitive filesystem holds both spellings as distinct files. func ignoreFilePathInTree(dirPath string) (string, error) { // "" is also the answer for a tree with no ignore file, so a swallowed error // would silently mean "no exclusions". - if _, err := os.Lstat(filepath.Join(dirPath, ignoreFileName)); err != nil { + if _, err := os.Lstat(filepath.Join(dirPath, IgnoreFileName)); err != nil { if errors.Is(err, fs.ErrNotExist) { return "", nil } @@ -254,7 +252,7 @@ func ignoreFilePathInTree(dirPath string) (string, error) { } folded := "" for _, entry := range entries { - if !strings.EqualFold(entry.Name(), ignoreFileName) { + if !strings.EqualFold(entry.Name(), IgnoreFileName) { continue } // Only a file can carry rules. The dirent type is not enough on its own: it @@ -267,7 +265,7 @@ func ignoreFilePathInTree(dirPath string) (string, error) { if resolved, err := os.Stat(path); err == nil && resolved.IsDir() { continue } - if entry.Name() == ignoreFileName { + if entry.Name() == IgnoreFileName { return path, nil } if folded == "" { diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index dbee89fb4..3915de534 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -132,7 +132,7 @@ func FilesNeedingContent(paths []string, ignoreRules []string) (map[string]bool, // protectedVirtualPath is the root ignore file, which its own rules never exclude. func protectedVirtualPath() string { - return path.Join(virtualRoot, ignoreFileName) + return path.Join(virtualRoot, IgnoreFileName) } // relativeVirtualPath strips the synthetic root from a tree path. diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index 8f03ad1a7..495b04f09 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -83,6 +83,11 @@ func (fs virtualFS) globDoubleStar(pattern string) ([]string, error) { // glob mirrors filepath.Glob on a Unix filesystem. func (fs virtualFS) glob(pattern string) ([]string, error) { + // filepath.Glob rejects a malformed pattern before it looks at the + // filesystem, so a bad rule fails even where nothing could match it. + if _, err := path.Match(pattern, ""); err != nil { + return nil, err + } if !hasGlobMeta(pattern) { // A literal pattern is returned as written, not cleaned. if _, ok := fs.lookup(pattern); !ok { diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index 37a9a4a34..fa98337dc 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -1,6 +1,7 @@ package digest import ( + "path" "path/filepath" "sort" "strings" @@ -69,6 +70,9 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { {name: "a doubled slash", ignore: "nested-dir//logs", hasEffect: true}, {name: "a parent segment that leaves the tree", ignore: "../logs"}, {name: "a rule that matches nothing", ignore: "does-not-exist"}, + // The first piece matches nothing, so the malformed second piece is never + // evaluated, on disk or here. + {name: "a malformed pattern behind a double star that matches nothing", ignore: "nonexistent/**/a["}, {name: "a star alone", ignore: "*", hasEffect: true}, {name: "a character class", ignore: "app.[jl]?", hasEffect: true}, {name: "a question mark", ignore: "a/?", hasEffect: true}, @@ -104,17 +108,24 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { } // A malformed pattern fails DirSha256, so it must fail the virtual digest too -// rather than silently excluding nothing. +// rather than silently excluding nothing. filepath.Glob validates the pattern +// before it looks at the filesystem, so this holds even for a rule under a +// directory the tree does not have. func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { - root := suite.T().TempDir() - files := suite.materialise(root, ignoreTestTree, "[") + for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/["} { + suite.Run(rule, func() { + root := suite.T().TempDir() + files := suite.materialise(root, ignoreTestTree, rule) - _, err := DirSha256(root, nil, logger.NewStandardLogger()) - require.Error(suite.T(), err) + _, err := DirSha256(root, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err, "DirSha256 must reject the rule") - _, err = VirtualDirSha256(files, []string{"["}, logger.NewStandardLogger()) - require.Error(suite.T(), err) - require.Contains(suite.T(), err.Error(), "[") + _, err = VirtualDirSha256(files, []string{rule}, logger.NewStandardLogger()) + require.Error(suite.T(), err, "VirtualDirSha256 must reject the rule") + require.ErrorIs(suite.T(), err, path.ErrBadPattern) + require.Contains(suite.T(), err.Error(), rule) + }) + } } // Mirrors TestDirSha256IgnoreFileCannotHideItself: an ignore file that lists @@ -233,7 +244,7 @@ func allPaths(tree map[string]string, withIgnoreFile bool) []string { paths = append(paths, p) } if withIgnoreFile { - paths = append(paths, ignoreFileName) + paths = append(paths, IgnoreFileName) } sort.Strings(paths) return paths @@ -299,7 +310,7 @@ func (suite *VirtualIgnoreTestSuite) materialise(root string, tree map[string]st require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, filepath.FromSlash(p)), content)) } if ignore != "" { - require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, ignoreFileName), ignore)) + require.NoError(suite.T(), utils.CreateFileWithContent(filepath.Join(root, IgnoreFileName), ignore)) } return virtualFilesFor(tree, ignore) } @@ -317,7 +328,7 @@ func virtualFilesFor(tree map[string]string, ignore string) []VirtualFile { files = append(files, VirtualFile{Path: p, Sha256: sha256OfString(tree[p])}) } if ignore != "" { - files = append(files, VirtualFile{Path: ignoreFileName, Sha256: sha256OfString(ignore)}) + files = append(files, VirtualFile{Path: IgnoreFileName, Sha256: sha256OfString(ignore)}) } return files } From f77c37cfd4185ceb0191d7f9af56c1901f15c2ef Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 17:35:18 +0000 Subject: [PATCH 10/30] refactor(snapshot s3): decide the single-object case with SingleVirtualFile The single-object shortcut open-coded the decision SingleVirtualFile documents and tests, leaving the exported function without a caller. The manifest is now built as paths first, so the shortcut branches on SingleVirtualFile and names the artifact with VirtualFile.Name, and the digests are filled in by index afterwards. FilesNeedingContent takes the manifest itself rather than a parallel slice of paths, which removes one per-object allocation from the run. Also from review: the single-problem advice reads "exclude the affected keys", since a collision names more than one, and the lone-collision message is pinned. --- internal/aws/aws.go | 35 +++++++++++++++------------ internal/aws/s3_keys.go | 2 +- internal/aws/s3_keys_test.go | 9 ++++++- internal/digest/virtualdir.go | 17 ++++++------- internal/digest/virtualignore_test.go | 13 +++++++--- 5 files changed, 45 insertions(+), 31 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index cfbf9d641..0004923d7 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -548,12 +548,21 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O } }() - if len(objects) == 1 { - sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, keys[0], nil, logger) + // The manifest starts as paths only; digests are filled in below by index, + // so it stays in listing order. + files := make([]digest.VirtualFile, len(objects)) + for i, object := range objects { + files[i].Path = paths[object.key] + } + + // One object is fingerprinted as that file and named after it, as + // containsSingleFile decided when the objects were on disk. + if file, ok := digest.SingleVirtualFile(files); ok { + sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, objects[0].key, nil, logger) if err != nil { return "", "", err } - return path.Base(paths[keys[0]]), sha256, nil + return file.Name(), sha256, nil } var rules []string @@ -580,30 +589,24 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O logger.Debug("object key [%s] is the bucket's %s -- excluding paths: %s", key, digest.IgnoreFileName, rules) } - allPaths := make([]string, len(keys)) - for i, key := range keys { - allPaths[i] = paths[key] - } - needed, err := digest.FilesNeedingContent(allPaths, rules) + needed, err := digest.FilesNeedingContent(files, rules) if err != nil { return "", "", ignoreRuleError(err) } - files := make([]digest.VirtualFile, 0, len(keys)) - for _, key := range keys { - virtualPath := paths[key] - sha256, downloaded := contentSha256[key] + for i, object := range objects { + sha256, downloaded := contentSha256[object.key] switch { case downloaded: - case needed[virtualPath]: - sha256, err = downloadAndHashS3Object(downloader, tempDir, bucket, key, nil, logger) + case needed[files[i].Path]: + sha256, err = downloadAndHashS3Object(downloader, tempDir, bucket, object.key, nil, logger) if err != nil { return "", "", err } default: - logger.Debug("object key [%s] is excluded by %s and is not downloaded", key, digest.IgnoreFileName) + logger.Debug("object key [%s] is excluded by %s and is not downloaded", object.key, digest.IgnoreFileName) } - files = append(files, digest.VirtualFile{Path: virtualPath, Sha256: sha256}) + files[i].Sha256 = sha256 } sha256, err := digest.VirtualDirSha256(files, rules, logger) diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index 2b803bd99..607091da2 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -115,7 +115,7 @@ func s3KeyProblemsError(problems []string) error { // Map iteration supplied these in any order; sorting keeps the message stable. sort.Strings(problems) if len(problems) == 1 { - return fmt.Errorf("%s; exclude it with --exclude-regex, or narrow the include filter if one is set", problems[0]) + return fmt.Errorf("%s; exclude the affected keys with --exclude-regex, or narrow the include filter if one is set", problems[0]) } shown := problems diff --git a/internal/aws/s3_keys_test.go b/internal/aws/s3_keys_test.go index 18e7a2fd6..36ccd00d9 100644 --- a/internal/aws/s3_keys_test.go +++ b/internal/aws/s3_keys_test.go @@ -208,7 +208,14 @@ func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysSingleProblemReadsAsOneLi _, err := virtualPathsForS3Keys([]string{"good", "bad/.."}) require.Error(suite.T(), err) require.Equal(suite.T(), - `object key [bad/..] cannot be fingerprinted: contains a ".." segment; exclude it with --exclude-regex, or narrow the include filter if one is set`, + `object key [bad/..] cannot be fingerprinted: contains a ".." segment; exclude the affected keys with --exclude-regex, or narrow the include filter if one is set`, + err.Error()) + + // A single problem can still name several keys. + _, err = virtualPathsForS3Keys([]string{"a/b", "a//b"}) + require.Error(suite.T(), err) + require.Equal(suite.T(), + `object keys [a//b], [a/b] fingerprint as the same path [a/b]; exclude the affected keys with --exclude-regex, or narrow the include filter if one is set`, err.Error()) } diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index 3915de534..6377b32d6 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -99,16 +99,13 @@ func VirtualDirSha256(files []VirtualFile, ignoreRules []string, logger *logger. return hex.EncodeToString(hasher.Sum(nil)), nil } -// FilesNeedingContent reports which of paths VirtualDirSha256 reads the content -// digest of under these ignore rules. A file it leaves out is skipped by the -// rules, so its content need not be fetched and it may be passed with an empty -// Sha256 without changing the fingerprint. The two share one walk, so they -// cannot disagree. -func FilesNeedingContent(paths []string, ignoreRules []string) (map[string]bool, error) { - files := make([]VirtualFile, len(paths)) - for i, p := range paths { - files[i] = VirtualFile{Path: p} - } +// FilesNeedingContent reports, by path, which of files VirtualDirSha256 reads +// the content digest of under these ignore rules. Digests on the input are not +// needed and may be empty. A file it leaves out is skipped by the rules, so its +// content need not be fetched and it may later be passed with an empty Sha256 +// without changing the fingerprint. The two share one walk, so they cannot +// disagree. +func FilesNeedingContent(files []VirtualFile, ignoreRules []string) (map[string]bool, error) { root, err := buildVirtualTree(files) if err != nil { return nil, err diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index fa98337dc..4f367ca37 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -204,8 +204,7 @@ func (suite *VirtualIgnoreTestSuite) TestFilesNeedingContentAgreesWithTheDigest( {name: "the ignore file cannot exclude itself", rules: []string{".kosli_ignore", "**"}, want: []string{".kosli_ignore"}}, } { suite.Run(t.name, func() { - paths := allPaths(ignoreTestTree, true) - needed, err := FilesNeedingContent(paths, t.rules) + needed, err := FilesNeedingContent(virtualFilesFromPaths(allPaths(ignoreTestTree, true)), t.rules) require.NoError(suite.T(), err) got := make([]string, 0, len(needed)) for p := range needed { @@ -234,10 +233,18 @@ func (suite *VirtualIgnoreTestSuite) TestFilesNeedingContentAgreesWithTheDigest( } func (suite *VirtualIgnoreTestSuite) TestFilesNeedingContentRejectsAMalformedRule() { - _, err := FilesNeedingContent([]string{"a.txt"}, []string{"["}) + _, err := FilesNeedingContent([]VirtualFile{{Path: "a.txt"}}, []string{"["}) require.Error(suite.T(), err) } +func virtualFilesFromPaths(paths []string) []VirtualFile { + files := make([]VirtualFile, len(paths)) + for i, p := range paths { + files[i] = VirtualFile{Path: p} + } + return files +} + func allPaths(tree map[string]string, withIgnoreFile bool) []string { paths := make([]string, 0, len(tree)+1) for p := range tree { From 058952718862f185ea1c097d4000278ef3ac0b7a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 17:53:39 +0000 Subject: [PATCH 11/30] fix(digest): validate an ignore rule before skipping one that leaves the tree filepath.Glob validates the whole pattern before it looks at the filesystem, so a malformed rule such as "../a[" fails DirSha256 even though it names a path outside the directory. The virtual mirror skipped rules that resolve outside the tree before validating them, so the same rule was silently ignored. The joined pattern is now validated first; path.Clean cannot add or remove glob metacharacters, so the verdict matches the on-disk one. The parity test gains the "../a[" row. Also corrects the maxReportedS3KeyProblems comment, which still counted keys after the message moved to counting problems. --- internal/aws/s3_keys.go | 2 +- internal/digest/virtualglob.go | 6 ++++++ internal/digest/virtualignore_test.go | 2 +- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index 607091da2..a6a579ad1 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -7,7 +7,7 @@ import ( "strings" ) -// maxReportedS3KeyProblems caps how many keys one error lists before +// maxReportedS3KeyProblems caps how many problems one error lists before // summarising the rest, so a bucket-wide problem stays readable. const maxReportedS3KeyProblems = 10 diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index 495b04f09..f9bb0c0af 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -32,6 +32,12 @@ func (fs virtualFS) excludedPaths(rules []string) (map[string]bool, error) { excluded := map[string]bool{} for _, rule := range rules { pattern := path.Join(virtualRoot, rule) + // filepath.Glob validates the whole pattern before it looks at the + // filesystem, so a malformed rule fails on disk even when it names a path + // outside the tree, which the skip below never evaluates. + if _, err := path.Match(pattern, ""); err != nil { + return nil, fmt.Errorf("ignore rule %q: %w", rule, err) + } // On disk the root is a temp directory with an unguessable name, so a rule // that resolves to the root or outside it cannot match anything there. if pattern == virtualRoot || !strings.HasPrefix(pattern, virtualRoot+"/") { diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index 4f367ca37..0c2643ad6 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -112,7 +112,7 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { // before it looks at the filesystem, so this holds even for a rule under a // directory the tree does not have. func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { - for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/["} { + for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/[", "../a["} { suite.Run(rule, func() { root := suite.T().TempDir() files := suite.materialise(root, ignoreTestTree, rule) From 56c4d6e9764909668580ed8fc7d2a2f5d8595952 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 17:54:41 +0000 Subject: [PATCH 12/30] fix(digest): validate only the first "**" piece of an ignore rule up front The previous commit validated the whole joined pattern, which rejected "nonexistent/**/a[" even though on disk filepathx never evaluates the malformed second piece once the first piece matches nothing. filepathx hands filepath.Glob the first piece unconditionally and later pieces only as earlier ones match, so validating the first piece up front reproduces the on-disk verdict for escaping and non-escaping rules alike. The equivalence row for that rule is green again. --- internal/digest/virtualglob.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index f9bb0c0af..a177f9d0e 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -32,10 +32,12 @@ func (fs virtualFS) excludedPaths(rules []string) (map[string]bool, error) { excluded := map[string]bool{} for _, rule := range rules { pattern := path.Join(virtualRoot, rule) - // filepath.Glob validates the whole pattern before it looks at the - // filesystem, so a malformed rule fails on disk even when it names a path - // outside the tree, which the skip below never evaluates. - if _, err := path.Match(pattern, ""); err != nil { + // filepath.Glob validates a pattern before it looks at the filesystem, and + // filepathx hands it the first "**" piece unconditionally, so a malformed + // rule fails on disk even when it names a path outside the tree, which the + // skip below never evaluates. Later pieces are validated only once an + // earlier piece has matched, on disk and here alike. + if _, err := path.Match(strings.SplitN(pattern, "**", 2)[0], ""); err != nil { return nil, fmt.Errorf("ignore rule %q: %w", rule, err) } // On disk the root is a temp directory with an unguessable name, so a rule From ff4dfc130830963d5cd6b7d6fe88677f06b6539c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 17:56:44 +0000 Subject: [PATCH 13/30] fix(digest): bound glob recursion as filepath.Glob does filepath.Glob returns ErrBadPattern once a pattern recurses through 10,000 separators rather than exhaust the stack. The virtual mirror recursed without a bound, so a .kosli_ignore rule of very many wildcard segments, which a bucket writer controls, would fail fast on disk and grow the stack without limit here. The mirror now carries the same depth counter, and the parity test holds a rule one level past it. --- internal/digest/virtualglob.go | 15 ++++++++++++++- internal/digest/virtualignore_test.go | 4 +++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index a177f9d0e..ad39edd38 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -89,8 +89,21 @@ func (fs virtualFS) globDoubleStar(pattern string) ([]string, error) { return matches, nil } +// globSeparatorsLimit is filepath's pathSeparatorsLimit: the recursion depth +// at which Glob gives up on a pattern rather than exhaust the stack. +const globSeparatorsLimit = 10000 + // glob mirrors filepath.Glob on a Unix filesystem. func (fs virtualFS) glob(pattern string) ([]string, error) { + return fs.globWithLimit(pattern, 0) +} + +func (fs virtualFS) globWithLimit(pattern string, depth int) ([]string, error) { + // A rule is attacker-writable, so the same bound filepath.Glob applies to + // deep wildcard paths applies here. + if depth == globSeparatorsLimit { + return nil, path.ErrBadPattern + } // filepath.Glob rejects a malformed pattern before it looks at the // filesystem, so a bad rule fails even where nothing could match it. if _, err := path.Match(pattern, ""); err != nil { @@ -112,7 +125,7 @@ func (fs virtualFS) glob(pattern string) ([]string, error) { if dir == pattern { return nil, path.ErrBadPattern } - dirs, err := fs.glob(dir) + dirs, err := fs.globWithLimit(dir, depth+1) if err != nil { return nil, err } diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index 0c2643ad6..f4345af1d 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -112,7 +112,9 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { // before it looks at the filesystem, so this holds even for a rule under a // directory the tree does not have. func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { - for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/[", "../a["} { + // The last rule is a wildcard path deeper than filepath.Glob's recursion + // limit, which it rejects rather than descend. + for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/[", "../a[", strings.Repeat("*/", globSeparatorsLimit+1) + "x"} { suite.Run(rule, func() { root := suite.T().TempDir() files := suite.materialise(root, ignoreTestTree, rule) From cc870477e8b493e3953a6828f0bc6366ad621b43 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:17:42 +0000 Subject: [PATCH 14/30] docs: record the escaping-rule residue and rename a shadowing loop variable The excludedPaths comment now states which shape of malformed rule the mirror deliberately does not reproduce, so the residue reads as known rather than as a parity claim the escape skip breaks. The ADR notes that a backslash inside an ignore rule also takes Linux semantics. The per-path key slices in virtualPathsForS3Keys no longer shadow the keys parameter. --- .../adr/20260911-s3-fingerprint-from-virtual-tree.md | 2 +- internal/aws/s3_keys.go | 12 ++++++------ internal/digest/virtualglob.go | 6 +++++- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index f974803af..464be9bc7 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -58,7 +58,7 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still ## Consequences -- The fingerprint is the same on every operating system, with Linux semantics. A snapshot run on Windows or macOS against a bucket with case-colliding or backslash keys moves to the Linux value. This is a correction and is release-noted. +- The fingerprint is the same on every operating system, with Linux semantics. A snapshot run on Windows or macOS against a bucket with case-colliding or backslash keys moves to the Linux value. The same holds for a backslash inside an ignore rule: the virtual glob reads it as an escape, as `filepath.Glob` does on Linux, where on Windows `filepath.Glob` reads it as a separator, so such a rule now resolves differently for an S3 snapshot than for `attest artifact --artifact-type dir` run on the same Windows machine. This is a correction and is release-noted. - `localPathForS3Key`, `filepath.IsLocal`, the `O_EXCL` open, the `ENOTDIR` branch, `containsSingleFile` and the platform-conditional tests from #1155 are deleted. No code in the S3 path branches on the operating system any more. The codebase does not get smaller, though: the virtual tree, the key rule with its collision reporting, and above all the faithful simulation of `filepathx.Glob` add several hundred lines, most of them owed to reproducing `.kosli_ignore` semantics exactly. That is the price of the compatibility contract, paid once and shared with #1069. - The bucket's ignore file is recognised by the exact key `.kosli_ignore`. On disk, `ignoreFilePathInTree` also accepted a case-folded spelling such as `.KOSLI_IGNORE` where the operator's filesystem folded case, so on macOS or Windows such a bucket had its rules applied; now it does not, its exclusions stop applying and its fingerprint moves to the Linux value. Same correction as the key case above, and release-noted with it. - `...`, `.. `, `CON`, colon and backslash keys snapshot again. `..` segments and colliding keys remain errors and now name every key involved. diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index a6a579ad1..d32b7cac3 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -78,20 +78,20 @@ func virtualPathsForS3Keys(keys []string) (map[string]string, error) { // The lexically smallest key under each directory stands as the example in // the message, so the report does not depend on listing order. exampleObjectUnder := map[string]string{} - for virtualPath, keys := range keysByPath { - sort.Strings(keys) + for virtualPath, keysHere := range keysByPath { + sort.Strings(keysHere) for dir := path.Dir(virtualPath); dir != "."; dir = path.Dir(dir) { - if existing, ok := exampleObjectUnder[dir]; !ok || keys[0] < existing { - exampleObjectUnder[dir] = keys[0] + if existing, ok := exampleObjectUnder[dir]; !ok || keysHere[0] < existing { + exampleObjectUnder[dir] = keysHere[0] } } } - for virtualPath, keys := range keysByPath { + for virtualPath, keysHere := range keysByPath { child, isAlsoDir := exampleObjectUnder[virtualPath] if !isAlsoDir { continue } - for _, key := range keys { + for _, key := range keysHere { problems = append(problems, fmt.Sprintf("object key [%s] fingerprints as [%s], which is also a directory holding object key [%s]", key, virtualPath, child)) } diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index ad39edd38..9043e2946 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -36,7 +36,11 @@ func (fs virtualFS) excludedPaths(rules []string) (map[string]bool, error) { // filepathx hands it the first "**" piece unconditionally, so a malformed // rule fails on disk even when it names a path outside the tree, which the // skip below never evaluates. Later pieces are validated only once an - // earlier piece has matched, on disk and here alike. + // earlier piece has matched, which for a rule inside the tree happens here + // too. A rule that both leaves the tree and has a malformed later piece + // (say "../**/a[") errors on disk, where the temp directory's parent exists + // and is walked, and is skipped here; modelling that parent would be + // speculative. if _, err := path.Match(strings.SplitN(pattern, "**", 2)[0], ""); err != nil { return nil, fmt.Errorf("ignore rule %q: %w", rule, err) } From d00479ca92a4cdcf1a1dc0a41ca764ebeafc84e2 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:25:54 +0000 Subject: [PATCH 15/30] docs: drop references to the deleted containsSingleFile Three comments pointed a reader at a function this change removes. The justification each carried stands on its own, so it stays and the name goes. --- internal/aws/aws.go | 4 ++-- internal/digest/virtualdir.go | 15 ++++++++------- internal/digest/virtualdir_test.go | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 0004923d7..1b39a2ee6 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -555,8 +555,8 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O files[i].Path = paths[object.key] } - // One object is fingerprinted as that file and named after it, as - // containsSingleFile decided when the objects were on disk. + // One object is fingerprinted as that file and named after it, as it was + // when the objects were laid out on disk. if file, ok := digest.SingleVirtualFile(files); ok { sha256, err := downloadAndHashS3Object(downloader, tempDir, bucket, objects[0].key, nil, logger) if err != nil { diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index 6377b32d6..3d9de5771 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -29,11 +29,12 @@ func (f VirtualFile) Name() string { // SingleVirtualFile reports whether the tree holds exactly one file, and // returns it. // -// This mirrors what containsSingleFile decides for a tree on disk: a tree built -// only from file paths has no empty directories, so a single leaf means every -// level has exactly one child, and two distinct leaves must diverge at some -// node and give it two children. Counting the files is therefore equivalent to -// walking the tree, and callers can pick the FileSha256 branch on len == 1. +// A tree built only from file paths has no empty directories, so a single leaf +// means every level has exactly one child, and two distinct leaves must diverge +// at some node and give it two children. Counting the files is therefore +// equivalent to walking the tree, which is how the same question was answered +// when the objects were laid out on disk, and callers can pick the FileSha256 +// branch on len == 1. func SingleVirtualFile(files []VirtualFile) (VirtualFile, bool) { if len(files) != 1 { return VirtualFile{}, false @@ -103,8 +104,8 @@ func VirtualDirSha256(files []VirtualFile, ignoreRules []string, logger *logger. // the content digest of under these ignore rules. Digests on the input are not // needed and may be empty. A file it leaves out is skipped by the rules, so its // content need not be fetched and it may later be passed with an empty Sha256 -// without changing the fingerprint. The two share one walk, so they cannot -// disagree. +// without changing the fingerprint. Both run the same walk over the same tree, +// so they cannot disagree. func FilesNeedingContent(files []VirtualFile, ignoreRules []string) (map[string]bool, error) { root, err := buildVirtualTree(files) if err != nil { diff --git a/internal/digest/virtualdir_test.go b/internal/digest/virtualdir_test.go index daf80f393..25c710bd6 100644 --- a/internal/digest/virtualdir_test.go +++ b/internal/digest/virtualdir_test.go @@ -265,7 +265,7 @@ func (suite *VirtualDirTestSuite) TestSingleVirtualFile() { // TestSingleFileMatchesFileSha256 pins the equivalence the aws package relies on: // a one-object snapshot is fingerprinted as that file's content digest, exactly -// as content mode does via containsSingleFile + FileSha256. +// as content mode did with FileSha256 when the objects were laid out on disk. func (suite *VirtualDirTestSuite) TestSingleFileMatchesFileSha256() { content := "the only object\n" path := filepath.Join(suite.tmpDir, "only.txt") From 08c7a99c5fe537307e98416b3f0fc0709fe13256 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:27:00 +0000 Subject: [PATCH 16/30] docs: say the folder-marker guard backs up the caller's filter The contract sentence read as if it disagreed with the guard beneath it. --- internal/aws/s3_keys.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index d32b7cac3..f07e92bd3 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -51,7 +51,9 @@ func virtualPathForS3Key(key string) (string, error) { // a directory holding other objects. All problems are reported together so one // run tells the operator about every key they need to act on. // -// Folder markers (keys ending in "/") are the caller's to filter out first. +// Folder markers (keys ending in "/") are filtered out by the caller before +// listing reaches here; the rule above rejects any that slip through so they +// can never be mistaken for objects. func virtualPathsForS3Keys(keys []string) (map[string]string, error) { paths := make(map[string]string, len(keys)) keysByPath := map[string][]string{} From 609849c4907cef63f9c77750f12f240b0055bd85 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:36:00 +0000 Subject: [PATCH 17/30] fix(aws): cap the keys one collision names in the S3 key report The ten-problem cap bounded the list of problems but not a single collision, which named every key that folded onto the path. Folding variants are cheap to write, so a bucket could turn one line into kilobytes. A collision now names at most ten keys and counts the rest. --- internal/aws/aws.go | 2 +- internal/aws/s3_keys.go | 11 +++++++++-- internal/aws/s3_keys_test.go | 16 ++++++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 1b39a2ee6..a79092641 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -597,7 +597,7 @@ func fingerprintS3Objects(downloader S3DownloadAPI, bucket string, objects []s3O for i, object := range objects { sha256, downloaded := contentSha256[object.key] switch { - case downloaded: + case downloaded: // the ignore-file pass already hashed this object case needed[files[i].Path]: sha256, err = downloadAndHashS3Object(downloader, tempDir, bucket, object.key, nil, logger) if err != nil { diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index f07e92bd3..591a7990b 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -72,8 +72,15 @@ func virtualPathsForS3Keys(keys []string) (map[string]string, error) { for virtualPath, colliding := range keysByPath { if len(colliding) > 1 { sort.Strings(colliding) - problems = append(problems, fmt.Sprintf("object keys %s fingerprint as the same path [%s]", - bracketed(colliding), virtualPath)) + // Folding variants of one path are cheap to write, so one collision + // is bounded the same way the list of problems is. + named, more := colliding, "" + if len(named) > maxReportedS3KeyProblems { + named = named[:maxReportedS3KeyProblems] + more = fmt.Sprintf(" and %d more", len(colliding)-maxReportedS3KeyProblems) + } + problems = append(problems, fmt.Sprintf("object keys %s%s fingerprint as the same path [%s]", + bracketed(named), more, virtualPath)) } } diff --git a/internal/aws/s3_keys_test.go b/internal/aws/s3_keys_test.go index 36ccd00d9..d3e9fdfed 100644 --- a/internal/aws/s3_keys_test.go +++ b/internal/aws/s3_keys_test.go @@ -190,6 +190,22 @@ func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysCapsTheReport() { require.Equal(suite.T(), maxReportedS3KeyProblems, strings.Count(msg, "object key [")) } +// Folding variants of one path are cheap to write, so one collision must not +// name every key that folds onto it. +func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysCapsTheKeysNamedPerCollision() { + keys := []string{"a/b"} + for i := 0; i < 12; i++ { + keys = append(keys, strings.Repeat("./", i+1)+"a/b") + } + _, err := virtualPathsForS3Keys(keys) + require.Error(suite.T(), err) + msg := err.Error() + require.Contains(suite.T(), msg, "fingerprint as the same path [a/b]") + require.Contains(suite.T(), msg, " and 3 more") + require.Equal(suite.T(), maxReportedS3KeyProblems+1, strings.Count(msg, "a/b]"), "the named keys and the path itself") + require.NotContains(suite.T(), msg, "\n", "one problem still reads as one line") +} + // The reported attack shape from #1155: the traversing key is rejected for its // ".." segment, and is never allowed to fold onto the object it names. func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysTraversalKeyIsRejected() { From b3416a98b2c36db73ada30121ab1998bb18233a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:43:44 +0000 Subject: [PATCH 18/30] fix(digest): do not protect a directory named .kosli_ignore from the rules On disk only a file of that name carries rules and is shielded from them; ignoreFilePathInTree skips a directory, so a rule naming it excludes it. The virtual digest protected the name unconditionally and kept the directory and its subtree. The protected path now depends on what the tree holds at that name, and an equivalence test pins the directory case. --- internal/digest/virtualdir.go | 13 +++++++++---- internal/digest/virtualignore_test.go | 25 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index 3d9de5771..2e8d69ebe 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -80,7 +80,7 @@ func VirtualDirSha256(files []VirtualFile, ignoreRules []string, logger *logger. logger.Debug("calculating fingerprint for a virtual tree of %d files -- excluding %d paths", len(files), len(excluded)) hasher := sha256.New() - err = root.walkIncluded(virtualRoot, excluded, protectedVirtualPath(), logger, func(childPath string, child *virtualNode) error { + err = root.walkIncluded(virtualRoot, excluded, root.protectedVirtualPath(), logger, func(childPath string, child *virtualNode) error { nameSha256 := sha256OfString(child.name) hasher.Write([]byte(nameSha256)) //nolint:errcheck // hash.Hash never returns an error if child.isDir { @@ -116,7 +116,7 @@ func FilesNeedingContent(files []VirtualFile, ignoreRules []string) (map[string] return nil, err } needed := map[string]bool{} - err = root.walkIncluded(virtualRoot, excluded, protectedVirtualPath(), nil, func(childPath string, child *virtualNode) error { + err = root.walkIncluded(virtualRoot, excluded, root.protectedVirtualPath(), nil, func(childPath string, child *virtualNode) error { if !child.isDir { needed[relativeVirtualPath(childPath)] = true } @@ -128,8 +128,13 @@ func FilesNeedingContent(files []VirtualFile, ignoreRules []string) (map[string] return needed, nil } -// protectedVirtualPath is the root ignore file, which its own rules never exclude. -func protectedVirtualPath() string { +// protectedVirtualPath is the root ignore file, which its own rules never +// exclude. A directory of that name carries no rules, as ignoreFilePathInTree +// decides on disk, so it is not protected either. +func (n *virtualNode) protectedVirtualPath() string { + if child, ok := n.children[IgnoreFileName]; !ok || child.isDir { + return "" + } return path.Join(virtualRoot, IgnoreFileName) } diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index f4345af1d..21ed3e217 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -130,6 +130,31 @@ func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { } } +// On disk only a file named .kosli_ignore carries rules and is protected from +// them; a directory of that name is an ordinary entry a rule can exclude. The +// rules come from the caller here, since a tree in this shape has no ignore +// file to hold them. +func (suite *VirtualIgnoreTestSuite) TestADirectoryNamedLikeTheIgnoreFileIsNotProtected() { + tree := map[string]string{IgnoreFileName + "/x": "not rules\n", "app.js": "app\n"} + root := suite.T().TempDir() + files := suite.materialise(root, tree, "") + rules := []string{IgnoreFileName} + + want, err := DirSha256(root, rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + got, err := VirtualDirSha256(files, rules, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), want, got) + + noRules, err := VirtualDirSha256(files, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.NotEqual(suite.T(), noRules, got, "the rule must exclude the directory") + + needed, err := FilesNeedingContent(files, rules) + require.NoError(suite.T(), err) + require.Equal(suite.T(), map[string]bool{"app.js": true}, needed) +} + // Mirrors TestDirSha256IgnoreFileCannotHideItself: an ignore file that lists // itself cannot hide an added file, because the file's own content stays in the // digest. From cdaefebc4c12fb50015d2b392da2a261db9b4cda Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:45:01 +0000 Subject: [PATCH 19/30] fix(aws): name the object key when hashing a downloaded object fails The temp file's name is anonymous by design, so a bare hashing error was the one failure in the S3 path an operator could not map back to an object. --- internal/aws/aws.go | 6 +++++- internal/aws/s3_fingerprint_test.go | 15 +++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index a79092641..b73371c44 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -661,7 +661,11 @@ func downloadAndHashS3Object(downloader S3DownloadAPI, tempDir, bucket, key stri return "", fmt.Errorf("object key [%s]: %w", key, err) } } - return digest.FileSha256(file.Name(), logger) + sha256, err := digest.FileSha256(file.Name(), logger) + if err != nil { + return "", fmt.Errorf("failed to hash object key [%s]: %w", key, err) + } + return sha256, nil } // getFilteredECSClusters fetches a filtered set of ECS clusters recursively (50 at a time) and returns a list of ecs Clusters diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index 28869716c..492fc3f41 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -253,6 +253,21 @@ func (suite *S3FingerprintTestSuite) TestADownloadErrorNamesTheKey() { require.NotContains(suite.T(), err.Error(), "--exclude-regex", "a transport failure must not advise dropping the object") } +// The temp file's name says nothing about the object, so a failure while +// hashing it must name the key, as every other failure in the path does. +func (suite *S3FingerprintTestSuite) TestAHashErrorNamesTheKey() { + client := &FakeS3Client{Bucket: fakeS3TestBucketName, Objects: map[string][]byte{"README.md": []byte(fakeReadmeBody)}} + // Deleting the file between download and hash is the one way to make the + // hash fail without touching permissions. + _, err := downloadAndHashS3Object(client, suite.T().TempDir(), fakeS3TestBucketName, "README.md", func(file *os.File) error { + return os.Remove(file.Name()) + }, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.ErrorIs(suite.T(), err, os.ErrNotExist) + require.Contains(suite.T(), err.Error(), "failed to hash object key [README.md]") + require.NotContains(suite.T(), err.Error(), "--exclude-regex") +} + func TestS3FingerprintTestSuite(t *testing.T) { suite.Run(t, new(S3FingerprintTestSuite)) } From 022f33339afa16d74e0ad739fd7fc638634b91f7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:54:00 +0000 Subject: [PATCH 20/30] fix(aws): tolerate S3 listings that omit LastModified and reject ones without a key The rewrite dereferenced LastModified on every listed object where the old loop only did so from the second object on, so a listing entry without a timestamp went from a misleading error to a panic. Real S3 always sets it; S3-compatible stores may not. Such an object now stays in the fingerprint and out of the snapshot timestamp, and a listing with no timestamp at all is an error. An entry with no key is an error too, since dropping it would lose an object silently. --- internal/aws/aws.go | 16 ++++++++- internal/aws/fake_s3.go | 16 ++++++--- internal/aws/s3_fingerprint_test.go | 50 +++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 6 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index b73371c44..2e3ee203f 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -476,6 +476,9 @@ func getS3DataFromClient(client S3API, bucket string, includePaths, includeRegex newest = object.lastModified } } + if newest.IsZero() { + return s3Data, fmt.Errorf("bucket [%s] reported no modification time for any matching object", bucket) + } artifactName, sha256, err := fingerprintS3Objects(client, bucket, objects, logger) if err != nil { @@ -507,13 +510,24 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin return nil, err } for _, object := range page.Contents { + // Real S3 always sets both fields; S3-compatible stores may not. + // Dropping an entry with no key would lose an object silently. + if object.Key == nil { + return nil, fmt.Errorf("bucket [%s] listed an object with no key", bucket) + } if strings.HasSuffix(*object.Key, "/") { // skip folders continue } if shouldExcludePath(*object.Key, includePaths, includeRegex, excludePaths, excludeRegex) { continue } - objects = append(objects, s3Object{key: *object.Key, lastModified: *object.LastModified}) + // An object without a timestamp stays in the fingerprint and out of + // the snapshot timestamp, as it was before. + var lastModified time.Time + if object.LastModified != nil { + lastModified = *object.LastModified + } + objects = append(objects, s3Object{key: *object.Key, lastModified: lastModified}) } } return objects, nil diff --git a/internal/aws/fake_s3.go b/internal/aws/fake_s3.go index 15c8ee7a8..0fc388170 100644 --- a/internal/aws/fake_s3.go +++ b/internal/aws/fake_s3.go @@ -30,6 +30,9 @@ type FakeS3Client struct { // LastModified maps object key to modification time. Keys without an entry // report fakeS3LastModified. LastModified map[string]time.Time + // NoLastModified lists keys whose listing entry carries no LastModified at + // all, as some S3-compatible stores return. + NoLastModified map[string]bool // PageSize controls how many objects are returned per ListObjectsV2 call. // Defaults to 1000 (matching the AWS default) if zero. PageSize int @@ -114,11 +117,14 @@ func (f *FakeS3Client) ListObjectsV2(_ context.Context, params *s3.ListObjectsV2 contents := make([]s3Types.Object, 0, end-start) for _, key := range keys[start:end] { - contents = append(contents, s3Types.Object{ - Key: aws.String(key), - LastModified: aws.Time(f.lastModified(key)), - Size: aws.Int64(int64(len(f.Objects[key]))), - }) + object := s3Types.Object{ + Key: aws.String(key), + Size: aws.Int64(int64(len(f.Objects[key]))), + } + if !f.NoLastModified[key] { + object.LastModified = aws.Time(f.lastModified(key)) + } + contents = append(contents, object) } out := &s3.ListObjectsV2Output{ diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index 492fc3f41..233d9db2d 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -7,8 +7,12 @@ import ( "sort" "sync" "testing" + "time" + "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/feature/s3/transfermanager" + "github.com/aws/aws-sdk-go-v2/service/s3" + s3Types "github.com/aws/aws-sdk-go-v2/service/s3/types" "github.com/kosli-dev/cli/internal/digest" "github.com/kosli-dev/cli/internal/logger" "github.com/kosli-dev/cli/internal/utils" @@ -268,6 +272,52 @@ func (suite *S3FingerprintTestSuite) TestAHashErrorNamesTheKey() { require.NotContains(suite.T(), err.Error(), "--exclude-regex") } +// Some S3-compatible stores list objects without a LastModified. Such an +// object still belongs in the fingerprint; only the snapshot timestamp is +// computed without it, and a listing with no timestamps at all is an error +// rather than a panic or a zero timestamp. +func (suite *S3FingerprintTestSuite) TestAListingWithoutModificationTimesDoesNotPanic() { + objects := map[string][]byte{"README.md": []byte(fakeReadmeBody), "notes.txt": []byte(fakeNotesBody)} + later := fakeS3LastModified.Add(time.Hour) + full, err := getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, + LastModified: map[string]time.Time{"notes.txt": later}}, fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + + partial, err := getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, + LastModified: map[string]time.Time{"notes.txt": later}, NoLastModified: map[string]bool{"README.md": true}}, + fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.NoError(suite.T(), err) + require.Equal(suite.T(), full[0].Digests, partial[0].Digests, "the object without a timestamp stays in the fingerprint") + require.Equal(suite.T(), later.Unix(), partial[0].LastModifiedTimestamp) + + _, err = getS3DataFromClient(&FakeS3Client{Bucket: fakeS3TestBucketName, Objects: objects, + NoLastModified: map[string]bool{"README.md": true, "notes.txt": true}}, + fakeS3TestBucketName, nil, nil, nil, nil, logger.NewStandardLogger()) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "modification time") +} + +// A listing entry with no key cannot be fingerprinted or reported, and dropping +// it would lose an object silently, so it is an error. +func (suite *S3FingerprintTestSuite) TestAListingEntryWithoutAKeyIsAnError() { + page := &s3.ListObjectsV2Output{Contents: []s3Types.Object{ + {Key: aws.String("README.md"), LastModified: aws.Time(fakeS3LastModified)}, + {LastModified: aws.Time(fakeS3LastModified)}, + }} + _, err := listMatchingS3Objects(singlePageLister{page: page}, fakeS3TestBucketName, nil, nil, nil, nil) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "no key") +} + +// singlePageLister answers every ListObjectsV2 call with one fixed page. +type singlePageLister struct { + page *s3.ListObjectsV2Output +} + +func (l singlePageLister) ListObjectsV2(context.Context, *s3.ListObjectsV2Input, ...func(*s3.Options)) (*s3.ListObjectsV2Output, error) { + return l.page, nil +} + func TestS3FingerprintTestSuite(t *testing.T) { suite.Run(t, new(S3FingerprintTestSuite)) } From 4e7af04b6bf74c7f14e22fcde2b036ab745fb2f6 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 18:54:55 +0000 Subject: [PATCH 21/30] docs: say the key-problem cap bounds both list lengths Since the collision line was capped, one constant bounds the problems listed and the keys named per collision; the comment named only the first. --- internal/aws/s3_keys.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index 591a7990b..69d4993ab 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -7,8 +7,9 @@ import ( "strings" ) -// maxReportedS3KeyProblems caps how many problems one error lists before -// summarising the rest, so a bucket-wide problem stays readable. +// maxReportedS3KeyProblems caps how many problems one error lists and how many +// keys one collision names before summarising the rest, so a bucket-wide +// problem stays readable: at most ten lines of at most ten keys each. const maxReportedS3KeyProblems = 10 // virtualPathForS3Key returns the path an object key occupies in the virtual From aec9e1685c835c311c196ded7fb102b83763491f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:02:54 +0000 Subject: [PATCH 22/30] docs: drop a reference to the undeclared writeDigests The enforcement the comment pointed at lives in the VirtualDirSha256 walk, not in a function of that name. --- internal/digest/virtualdir.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index 2e8d69ebe..c90bf7c51 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -163,7 +163,8 @@ func buildVirtualTree(files []VirtualFile) (*virtualNode, error) { return nil, err } // An empty digest means the content was not read. That is only acceptable - // for a file the rules exclude, which writeDigests enforces when it gets there. + // for a file the rules exclude; VirtualDirSha256 fails on an empty digest + // it reaches, so a skipped download cannot reach a fingerprint. if file.Sha256 != "" { if err := ValidateDigest(file.Sha256); err != nil { return nil, fmt.Errorf("invalid fingerprint for %q: %w", file.Path, err) From 5b4c7a7a9dca6b8b7eebecaa9369a3002f1f8a41 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:12:38 +0000 Subject: [PATCH 23/30] fix(aws): reject a bucket listing that repeats an object key A key listed twice rendered as a collision of the key with itself and advised excluding the only copy. It is a listing fault, so it is now an error of its own, and the collision report can rely on distinct keys. --- internal/aws/aws.go | 7 +++++++ internal/aws/s3_fingerprint_test.go | 14 ++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 2e3ee203f..ff454f87b 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -501,6 +501,7 @@ type s3Object struct { func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []string, includeRegex []*regexp.Regexp, excludePaths []string, excludeRegex []*regexp.Regexp) ([]s3Object, error) { objects := []s3Object{} + seen := map[string]bool{} paginator := s3.NewListObjectsV2Paginator(client, &s3.ListObjectsV2Input{ Bucket: aws.String(bucket), }) @@ -521,6 +522,12 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin if shouldExcludePath(*object.Key, includePaths, includeRegex, excludePaths, excludeRegex) { continue } + // A key listed twice is a listing fault, not a collision between two + // keys, and the collision report relies on keys being distinct. + if seen[*object.Key] { + return nil, fmt.Errorf("bucket [%s] listed object key [%s] more than once", bucket, *object.Key) + } + seen[*object.Key] = true // An object without a timestamp stays in the fingerprint and out of // the snapshot timestamp, as it was before. var lastModified time.Time diff --git a/internal/aws/s3_fingerprint_test.go b/internal/aws/s3_fingerprint_test.go index 233d9db2d..4fa6e8240 100644 --- a/internal/aws/s3_fingerprint_test.go +++ b/internal/aws/s3_fingerprint_test.go @@ -309,6 +309,20 @@ func (suite *S3FingerprintTestSuite) TestAListingEntryWithoutAKeyIsAnError() { require.Contains(suite.T(), err.Error(), "no key") } +// A key listed twice is a listing fault, not two objects: it must not be +// reported as a key colliding with itself, and the advice to exclude it would +// drop the only copy. +func (suite *S3FingerprintTestSuite) TestAListingThatRepeatsAKeyIsAnError() { + page := &s3.ListObjectsV2Output{Contents: []s3Types.Object{ + {Key: aws.String("a"), LastModified: aws.Time(fakeS3LastModified)}, + {Key: aws.String("a"), LastModified: aws.Time(fakeS3LastModified)}, + }} + _, err := listMatchingS3Objects(singlePageLister{page: page}, fakeS3TestBucketName, nil, nil, nil, nil) + require.Error(suite.T(), err) + require.Contains(suite.T(), err.Error(), "object key [a] more than once") + require.NotContains(suite.T(), err.Error(), "--exclude-regex") +} + // singlePageLister answers every ListObjectsV2 call with one fixed page. type singlePageLister struct { page *s3.ListObjectsV2Output From 87cb4f375a56e729bc9eee69f1e7ccb836b62ffa Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:12:38 +0000 Subject: [PATCH 24/30] fix(digest): bound the depth of a virtual path as filepath.Glob bounds a pattern The tree walks recurse once per segment over a tree the bucket's writers shape. S3 keys stay far below the bound; it exists for callers that do not get their paths from S3. --- internal/digest/virtualdir.go | 6 ++++++ internal/digest/virtualdir_test.go | 7 +++++++ 2 files changed, 13 insertions(+) diff --git a/internal/digest/virtualdir.go b/internal/digest/virtualdir.go index c90bf7c51..17f424c6f 100644 --- a/internal/digest/virtualdir.go +++ b/internal/digest/virtualdir.go @@ -16,6 +16,7 @@ import ( type VirtualFile struct { // Path is relative to the tree root, slash-separated, with no leading or // trailing slash and no "." or ".." segments (e.g. "dummy/template.yml"). + // Its depth is bounded by globSeparatorsLimit; S3 keys stay far below it. Path string // Sha256 is the hex-encoded sha256 of the file content. Sha256 string @@ -251,6 +252,11 @@ func validateVirtualPath(p string) error { return fmt.Errorf("path %q is not a clean relative path: it must not be empty, absolute, "+ "or contain empty, \".\" or \"..\" segments", p) } + // The walks recurse once per segment over a tree whose shape the bucket's + // writers control, so depth is bounded as filepath.Glob bounds a pattern. + if strings.Count(p, "/") >= globSeparatorsLimit { + return fmt.Errorf("path %q is too deep: at most %d segments are supported", p, globSeparatorsLimit) + } return nil } diff --git a/internal/digest/virtualdir_test.go b/internal/digest/virtualdir_test.go index 25c710bd6..ee2d5a77e 100644 --- a/internal/digest/virtualdir_test.go +++ b/internal/digest/virtualdir_test.go @@ -193,6 +193,13 @@ func (suite *VirtualDirTestSuite) TestVirtualDirSha256Errors() { files: []VirtualFile{{Path: "", Sha256: validSha}}, wantErrMsg: "not a clean relative path", }, + { + // The walks recurse once per segment over a tree the bucket's writers + // shape, so depth is bounded as filepath.Glob bounds a pattern. + name: "a path deeper than the walk bound", + files: []VirtualFile{{Path: strings.Repeat("d/", globSeparatorsLimit) + "x", Sha256: validSha}}, + wantErrMsg: "too deep", + }, { name: "an invalid sha256", files: []VirtualFile{{Path: "a.txt", Sha256: "not-a-digest"}}, From 5164f778039c55cd4442d075c84d42691a824dfb Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:18:53 +0000 Subject: [PATCH 25/30] docs(adr): list the malformed-listing rejections and name the cap constant The guarantee read as exhaustive but three listing faults and the path depth bound landed after it; the collision cap is now attributed to the constant both caps read. --- docs/adr/20260911-s3-fingerprint-from-virtual-tree.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md index 464be9bc7..23823c727 100644 --- a/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md +++ b/docs/adr/20260911-s3-fingerprint-from-virtual-tree.md @@ -33,7 +33,7 @@ The fingerprint format itself is fixed. An S3 snapshot must match the fingerprin 2. **The key becomes a virtual path by one rule, shared by both modes.** A key holding a `..` segment is rejected first, on the raw key, so `a/../b` cannot fold silently onto `b`. The rest is `path.Clean(strings.TrimLeft(key, "/"))`, rejecting a result of `.`. The fold is exactly what `filepath.Join` produced on Linux, so `a//b`, `./c.txt` and `/lead.txt` land where they do today. Everything else, including `...`, `.. `, `CON`, colons and backslashes, is an ordinary name because nothing is created under it. None of this is for safety. `DirSha256` walks real directories, which can never hold an entry named `.`, `..` or the empty string, so the rule keeps virtual fingerprints inside the space an attested directory can match, and preserves the fingerprints existing buckets already have. -3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, capped at ten by `s3KeyProblemsError`, with the existing advice to use `--exclude-regex` or narrow the include filter. +3. **Collisions are data errors that name every key involved.** Two keys resolving to one path, and a key under a prefix that is also an object, cannot be represented as a tree. They are detected before the digest package sees them and reported together, bounded by `maxReportedS3KeyProblems` in both directions, at most ten problem lines each naming at most ten keys, with the existing advice to use `--exclude-regex` or narrow the include filter. 4. **A root `.kosli_ignore` is honoured virtually.** Its rules are parsed by `digest.ParseIgnoreRules`, the same reading `DirSha256` gives the file, and resolved by `digest/virtualglob.go`, which reproduces `filepathx.Glob`, `filepath.Glob` and `filepath.Walk` step for step over the virtual tree rather than reimplementing what the globs appear to mean. That is what keeps their quirks identical: a literal `**/x` never matches at the root because the pieces concatenate to a double slash, `**/*.log` does, and excluding `logs/*` leaves an empty directory whose name is still hashed. Exclusion therefore runs inside the tree walk, not by filtering the file list. The ignore file can never exclude itself, as in `DirSha256`. `digest.FilesNeedingContent` shares that walk so excluded objects are not downloaded at all, and `VirtualDirSha256` refuses a tree that needs a digest it was not given, so a skipped download can never leak into a fingerprint. Equivalence with `DirSha256` on a materialised tree is asserted for every rule shape in `TestVirtualIgnoreTestSuite`. @@ -47,7 +47,7 @@ The attest side is unchanged: `kosli attest artifact --artifact-type dir` still - **Snapshot equals attestation.** `VirtualDirSha256` of the bucket's `(path, sha256)` pairs equals `DirSha256` of the directory those objects were uploaded from, and a single object equals `FileSha256` plus its basename. Held by the materialise-then-compare tests in `internal/digest` (`TestVirtualDirTestSuite`, `TestVirtualIgnoreTestSuite`) and by `TestMatchesAttestedDirectory` in `internal/aws`, which hashes a directory, uploads its files to the fake bucket and snapshots it. - **Snapshot equals its own history.** Every bucket that snapshotted successfully under the key-layout implementation keeps its fingerprint and artifact name, because the fold rule is what `filepath.Join` did. Held by `TestPinnedFingerprints` and `TestGetS3DataFromClientKeepsTodaysLayoutForUnusualKeys`. -- **No object is lost silently.** Every key S3 accepts is representable, whatever the operating system, because the key never becomes a path. The only rejections are keys that cannot form a directory tree at all: a `..` segment, two keys folding onto one path, and an object that is also a prefix. S3 permits those, no filesystem does, so no attested directory could match them. Each rejection fails the snapshot and names every key involved; the manifest is never shortened to make a fingerprint. Held by `TestEveryS3KeyIsRepresentable`, which generates keys over S3's full character set, and `TestDownloadsExactlyTheContributingObjects`, which asserts the downloaded set equals the listed objects minus markers and exclusions. +- **No object is lost silently.** Every key S3 accepts is representable, whatever the operating system, because the key never becomes a path. Rejections are of two kinds, and each fails the snapshot rather than shorten the manifest. Keys that cannot form a directory tree at all: a `..` segment, two keys folding onto one path, and an object that is also a prefix. S3 permits those, no filesystem does, so no attested directory could match them, and the error names every key involved. And listings the store returned malformed: an entry with no key, a key listed more than once, and no modification time on any matching object. Real S3 produces none of these; an S3-compatible store might, and each is an error rather than a guess. A path deeper than `globSeparatorsLimit` segments is also rejected, which no S3 key can reach, so the exported digest functions carry the bound for other callers. Held by `TestEveryS3KeyIsRepresentable`, which generates keys over S3's full character set, and `TestDownloadsExactlyTheContributingObjects`, which asserts the downloaded set equals the listed objects minus markers and exclusions. ## Alternatives considered From 4ead6fde9e70784be1e73f689bf6e41ccb1e36ef Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:19:48 +0000 Subject: [PATCH 26/30] refactor(aws): stop a loop variable shadowing the path package aws.go now imports path, so the prefix loop in objectMatchesFilter shadowed it. --- internal/aws/aws.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index ff454f87b..75dbc0e64 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -423,9 +423,9 @@ func compilePathRegex(patterns []string) ([]*regexp.Regexp, error) { // A key matches when it is prefixed by one of paths (literal prefix match) // or when one of patterns matches the full key. func objectMatchesFilter(key string, paths []string, patterns []*regexp.Regexp) bool { - for _, path := range paths { - path = strings.TrimLeft(path, "/") - if strings.HasPrefix(key, path) { + for _, prefix := range paths { + prefix = strings.TrimLeft(prefix, "/") + if strings.HasPrefix(key, prefix) { return true } } From b4552cb400cc20c3b799eb8a3a792ffd1cbc31d1 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:26:41 +0000 Subject: [PATCH 27/30] docs(digest): say the separator limit bounds tree depth too, and fix a doubled verb --- internal/digest/virtualglob.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index 9043e2946..11c1bf3cf 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -94,7 +94,9 @@ func (fs virtualFS) globDoubleStar(pattern string) ([]string, error) { } // globSeparatorsLimit is filepath's pathSeparatorsLimit: the recursion depth -// at which Glob gives up on a pattern rather than exhaust the stack. +// at which Glob gives up on a pattern rather than exhaust the stack. It bounds +// the depth of a tree path too, since walking one recurses per segment just +// as globbing recurses per separator. const globSeparatorsLimit = 10000 // glob mirrors filepath.Glob on a Unix filesystem. @@ -103,8 +105,8 @@ func (fs virtualFS) glob(pattern string) ([]string, error) { } func (fs virtualFS) globWithLimit(pattern string, depth int) ([]string, error) { - // A rule is attacker-writable, so the same bound filepath.Glob applies to - // deep wildcard paths applies here. + // A rule is attacker-writable, so deep wildcard paths take the same bound + // filepath.Glob gives them. if depth == globSeparatorsLimit { return nil, path.ErrBadPattern } From e248d72da78e70632fa113c91d23d5e19f84ba40 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:34:12 +0000 Subject: [PATCH 28/30] fix(aws): say what a capped collision line counts "and 3 more fingerprint as" read as a verb disagreeing with its subject. --- internal/aws/s3_keys.go | 2 +- internal/aws/s3_keys_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/aws/s3_keys.go b/internal/aws/s3_keys.go index 69d4993ab..084895ce6 100644 --- a/internal/aws/s3_keys.go +++ b/internal/aws/s3_keys.go @@ -78,7 +78,7 @@ func virtualPathsForS3Keys(keys []string) (map[string]string, error) { named, more := colliding, "" if len(named) > maxReportedS3KeyProblems { named = named[:maxReportedS3KeyProblems] - more = fmt.Sprintf(" and %d more", len(colliding)-maxReportedS3KeyProblems) + more = fmt.Sprintf(" and %d more keys", len(colliding)-maxReportedS3KeyProblems) } problems = append(problems, fmt.Sprintf("object keys %s%s fingerprint as the same path [%s]", bracketed(named), more, virtualPath)) diff --git a/internal/aws/s3_keys_test.go b/internal/aws/s3_keys_test.go index d3e9fdfed..d87ece1ea 100644 --- a/internal/aws/s3_keys_test.go +++ b/internal/aws/s3_keys_test.go @@ -201,7 +201,7 @@ func (suite *S3KeysTestSuite) TestVirtualPathsForS3KeysCapsTheKeysNamedPerCollis require.Error(suite.T(), err) msg := err.Error() require.Contains(suite.T(), msg, "fingerprint as the same path [a/b]") - require.Contains(suite.T(), msg, " and 3 more") + require.Contains(suite.T(), msg, " and 3 more keys fingerprint as") require.Equal(suite.T(), maxReportedS3KeyProblems+1, strings.Count(msg, "a/b]"), "the named keys and the path itself") require.NotContains(suite.T(), msg, "\n", "one problem still reads as one line") } From ee7481946bf760318959ca3f387c3ffda167367f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:34:12 +0000 Subject: [PATCH 29/30] fix(aws): check for a repeated key before the listing filters The error claims the listing repeated a key, so it must not depend on which filters are in force. --- internal/aws/aws.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/internal/aws/aws.go b/internal/aws/aws.go index 75dbc0e64..dc2712850 100644 --- a/internal/aws/aws.go +++ b/internal/aws/aws.go @@ -516,18 +516,19 @@ func listMatchingS3Objects(client S3ListAPI, bucket string, includePaths []strin if object.Key == nil { return nil, fmt.Errorf("bucket [%s] listed an object with no key", bucket) } + // A key listed twice is a listing fault, not a collision between two + // keys, and the collision report relies on keys being distinct. It is + // checked before the filters so the error is about the listing itself. + if seen[*object.Key] { + return nil, fmt.Errorf("bucket [%s] listed object key [%s] more than once", bucket, *object.Key) + } + seen[*object.Key] = true if strings.HasSuffix(*object.Key, "/") { // skip folders continue } if shouldExcludePath(*object.Key, includePaths, includeRegex, excludePaths, excludeRegex) { continue } - // A key listed twice is a listing fault, not a collision between two - // keys, and the collision report relies on keys being distinct. - if seen[*object.Key] { - return nil, fmt.Errorf("bucket [%s] listed object key [%s] more than once", bucket, *object.Key) - } - seen[*object.Key] = true // An object without a timestamp stays in the fingerprint and out of // the snapshot timestamp, as it was before. var lastModified time.Time From 12a90d41faea22264e1c27cca38d4ff0913c6c6d Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 14 Sep 2026 19:42:38 +0000 Subject: [PATCH 30/30] fix(digest): skip an ignore rule that leaves the tree even if it names the root The escape check ran on the cleaned pattern, so "../tree/x" folded back onto "tree/x" and was applied, where on disk the temp directory's unguessable name keeps a rule that has left the tree outside it. Depth is now tracked over the raw rule's segments, which is how filepath.Join resolves "..", so a rule that dips through a wildcard and returns still matches while one that walks above the root never does. --- internal/digest/virtualglob.go | 27 +++++++++++++++++++++++++-- internal/digest/virtualignore_test.go | 7 ++++++- 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/internal/digest/virtualglob.go b/internal/digest/virtualglob.go index 11c1bf3cf..240e8beef 100644 --- a/internal/digest/virtualglob.go +++ b/internal/digest/virtualglob.go @@ -45,8 +45,10 @@ func (fs virtualFS) excludedPaths(rules []string) (map[string]bool, error) { return nil, fmt.Errorf("ignore rule %q: %w", rule, err) } // On disk the root is a temp directory with an unguessable name, so a rule - // that resolves to the root or outside it cannot match anything there. - if pattern == virtualRoot || !strings.HasPrefix(pattern, virtualRoot+"/") { + // that resolves to the root or leaves it cannot match anything there, and + // naming the root cannot bring it back, which the cleaned pattern alone + // would not show ("../tree/x" cleans to "tree/x"). + if pattern == virtualRoot || !strings.HasPrefix(pattern, virtualRoot+"/") || escapesVirtualRoot(rule) { continue } matches, err := fs.globDoubleStar(pattern) @@ -215,6 +217,27 @@ func hasGlobMeta(pattern string) bool { return strings.ContainsAny(pattern, `*?[\`) } +// escapesVirtualRoot reports whether the rule walks above the tree root before +// it is cleaned. filepath.Join resolves ".." lexically, so counting segments +// gives the same answer as the join does on disk, where anything above the +// root is outside the tree whatever the rule names next. +func escapesVirtualRoot(rule string) bool { + depth := 0 + for _, segment := range strings.Split(rule, "/") { + switch segment { + case "", ".": + case "..": + if depth == 0 { + return true + } + depth-- + default: + depth++ + } + } + return false +} + // cleanGlobPath is filepath's helper of the same name: drop the trailing // separator path.Split leaves on a directory. func cleanGlobPath(dir string) string { diff --git a/internal/digest/virtualignore_test.go b/internal/digest/virtualignore_test.go index 21ed3e217..bdb2dbfe6 100644 --- a/internal/digest/virtualignore_test.go +++ b/internal/digest/virtualignore_test.go @@ -67,6 +67,11 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { {name: "a trailing slash", ignore: "logs/", hasEffect: true}, {name: "a leading slash", ignore: "/logs", hasEffect: true}, {name: "a leading dot segment", ignore: "./logs", hasEffect: true}, + // On disk the root has an unguessable name, so a rule that leaves the + // tree cannot come back by naming it; a rule that only dips through a + // wildcard and returns still folds onto a real path. + {name: "a rule that leaves the tree and names the root", ignore: "../tree/app.js", hasEffect: false}, + {name: "a rule that dips and returns through a wildcard", ignore: "*/../app.js", hasEffect: true}, {name: "a doubled slash", ignore: "nested-dir//logs", hasEffect: true}, {name: "a parent segment that leaves the tree", ignore: "../logs"}, {name: "a rule that matches nothing", ignore: "does-not-exist"}, @@ -114,7 +119,7 @@ func (suite *VirtualIgnoreTestSuite) TestMatchesDirSha256() { func (suite *VirtualIgnoreTestSuite) TestMalformedRuleIsAnErrorOnBothSides() { // The last rule is a wildcard path deeper than filepath.Glob's recursion // limit, which it rejects rather than descend. - for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/[", "../a[", strings.Repeat("*/", globSeparatorsLimit+1) + "x"} { + for _, rule := range []string{"[", "nonexistent/a[", "logs/[", "a/**/[", "../a[", "../tree/a[", strings.Repeat("*/", globSeparatorsLimit+1) + "x"} { suite.Run(rule, func() { root := suite.T().TempDir() files := suite.materialise(root, ignoreTestTree, rule)