diff --git a/scanner/internal/e2e/vulnreport_test.go b/scanner/internal/e2e/vulnreport_test.go index 8da7d13..d72e599 100644 --- a/scanner/internal/e2e/vulnreport_test.go +++ b/scanner/internal/e2e/vulnreport_test.go @@ -29,6 +29,7 @@ package e2e // has been end-of-life for years. import ( + "crypto/sha256" "encoding/json" "fmt" "os" @@ -155,8 +156,9 @@ type spdxDoc struct { } // finding is one match reduced to the identity a user would recognise. It is -// the unit the determinism scenario compares, because the full report JSON -// carries a scan-local temp path (see TestVulnDBScanIsDeterministic). +// what the determinism scenario compares alongside the report digest, because +// a digest mismatch says only that something moved; the findings say whether +// the matching itself did. type finding struct { ID string Package string @@ -176,8 +178,15 @@ type vulnScan struct { Report appviewReport Raw []byte SBOM spdxDoc + SBOMRaw []byte Digest string + // VulnDigest and SBOMDigest are what the hold stores on the + // io.atcr.hold.scan record, computed here the same way the scanner + // computes them: sha256 over the exact bytes it published. + VulnDigest string + SBOMDigest string + Elapsed time.Duration PeakRSS uint64 // bytes, sampled from /proc/self/statm during the scan HeapEnd uint64 @@ -283,11 +292,14 @@ func sendVulnScan(t *testing.T, h *Harness, job *scanner.ScanJob, fixture string } out := vulnScan{ - Fixture: fixture, - Summary: *msg.Summary, - Raw: []byte(msg.VulnReport), - Elapsed: elapsed, - PeakRSS: peakRSS, + Fixture: fixture, + Summary: *msg.Summary, + Raw: []byte(msg.VulnReport), + SBOMRaw: []byte(msg.SBOM), + VulnDigest: sha256Digest([]byte(msg.VulnReport)), + SBOMDigest: sha256Digest([]byte(msg.SBOM)), + Elapsed: elapsed, + PeakRSS: peakRSS, } if err := json.Unmarshal(out.Raw, &out.Report); err != nil { t.Fatalf("%s: the appview's own structs cannot parse the vulnerability report: %v", fixture, err) @@ -784,13 +796,17 @@ func TestVulnDBReportIsTheShapeTheAppviewReads(t *testing.T) { // TestVulnDBScanIsDeterministic scans the same image twice through one // harness, so both scans run against the same loaded provider. The findings // must be identical: same summary, same set of (vulnerability, package, -// version) triples. +// version) triples — and, now that the report no longer carries the per-job +// scratch path, the same vulnDigest. +// +// That last one is the assertion with teeth. vulnDigest is the content address +// the hold stores on the io.atcr.hold.scan record, so a digest that moves when +// the image did not means every pass of the stale-scan loop uploads a fresh +// blob and orphans the one before it. The report's source used to be the +// scanner's os.MkdirTemp("scan-*") directory, which guaranteed exactly that. // // Feed-independent by construction — a feed refresh between the two scans is // impossible, since the provider is loaded once and held in memory. -// -// It deliberately does not compare the report digests. See the log line at the -// end for why. func TestVulnDBScanIsDeterministic(t *testing.T) { primeVulnDB(t) requireVulnFixture(t, vulnFixtureOld) @@ -816,44 +832,86 @@ func TestVulnDBScanIsDeterministic(t *testing.T) { } } - // Content-identical scans, byte-identical reports? Not necessarily. The - // report's digest is what the hold stores as vulnDigest, so a report that - // changes when nothing about the image did means every rescan uploads a - // fresh blob under a fresh digest. Rather than assert either way — this is - // current behaviour, not a requirement — name the top-level keys that - // moved, so the cause is in the log instead of in a comment's guess. - if string(first.Raw) == string(second.Raw) { - t.Logf("the two reports are byte-identical (%d bytes), so a rescan of unchanged content "+ - "reuses the same vulnDigest", len(first.Raw)) + // The report, byte for byte. Naming the keys that moved is what makes a + // regression here diagnosable rather than just red. + if first.VulnDigest != second.VulnDigest { + t.Errorf("rescanning identical content produced a different vulnDigest, so the hold "+ + "stores a fresh blob and orphans the previous one:\n first: %s (%d bytes)\nsecond: %s (%d bytes)", + first.VulnDigest, len(first.Raw), second.VulnDigest, len(second.Raw)) + for _, key := range jsonKeysThatMoved(t, first.Raw, second.Raw) { + t.Errorf(" report key %q differs:\n %s\n %s", key, + firstLineOf(jsonField(t, first.Raw, key)), firstLineOf(jsonField(t, second.Raw, key))) + } + } + + // The SBOM is the other half of what the scan stores, and it is not yet + // stable: Syft's SPDX encoder stamps creationInfo.created from time.Now() + // and appends a random UUID to documentNamespace, neither of which it + // exposes a knob for. Nothing scan-local reaches it any more — that is + // what the assertion below pins, and internal/scan/syft_test.go pins the + // same thing without needing a database — but sbomDigest still moves on + // every rescan, so the orphaning is halved rather than closed. Removing + // the residue means changing what the encoder writes, which is a bigger + // change than the source reference and belongs to its own decision. + for _, key := range jsonKeysThatMoved(t, first.SBOMRaw, second.SBOMRaw) { + if key == "creationInfo" || key == "documentNamespace" { + continue + } + t.Errorf("SBOM key %q differs between two scans of identical content:\n %s\n %s", key, + firstLineOf(jsonField(t, first.SBOMRaw, key)), firstLineOf(jsonField(t, second.SBOMRaw, key))) + } + if first.SBOMDigest != second.SBOMDigest { + t.Logf("sbomDigest still moves (%s vs %s): creationInfo.created and documentNamespace "+ + "are stamped by Syft's SPDX encoder. Tighten this to an equality assertion if that is ever fixed.", + first.SBOMDigest, second.SBOMDigest) } else { - var ka, kb map[string]json.RawMessage - if err := json.Unmarshal(first.Raw, &ka); err != nil { - t.Fatalf("parse first report: %v", err) - } - if err := json.Unmarshal(second.Raw, &kb); err != nil { - t.Fatalf("parse second report: %v", err) - } - var moved []string - for k := range ka { - if string(ka[k]) != string(kb[k]) { - moved = append(moved, k) - } - } - sort.Strings(moved) - t.Logf("the two reports differ byte-for-byte despite identical findings (%d vs %d bytes); "+ - "the top-level keys that changed are %v, so the vulnDigest the hold records changes "+ - "on every rescan of unchanged content", - len(first.Raw), len(second.Raw), moved) - if len(moved) == 1 && moved[0] == "source" { - t.Logf(" \"source\" is s.Source from the SBOM, whose Name/Reference is the per-job "+ - "OCI layout path: %s", firstLineOf(string(ka["source"]))) - } + t.Errorf("sbomDigest is now stable (%s); turn the loop above into a plain digest "+ + "equality assertion", first.SBOMDigest) } t.Logf("first scan %s, rescan %s (the difference is the database load, paid once)", first.Elapsed.Round(time.Millisecond), second.Elapsed.Round(time.Millisecond)) } +// jsonKeysThatMoved names the top-level keys whose encoded values differ +// between two JSON documents. +func jsonKeysThatMoved(t *testing.T, a, b []byte) []string { + t.Helper() + var ka, kb map[string]json.RawMessage + if err := json.Unmarshal(a, &ka); err != nil { + t.Fatalf("parse first document: %v", err) + } + if err := json.Unmarshal(b, &kb); err != nil { + t.Fatalf("parse second document: %v", err) + } + var moved []string + for k := range ka { + if string(ka[k]) != string(kb[k]) { + moved = append(moved, k) + } + } + for k := range kb { + if _, ok := ka[k]; !ok { + moved = append(moved, k) + } + } + sort.Strings(moved) + return moved +} + +func jsonField(t *testing.T, doc []byte, key string) string { + t.Helper() + var m map[string]json.RawMessage + if err := json.Unmarshal(doc, &m); err != nil { + t.Fatalf("parse document: %v", err) + } + return string(m[key]) +} + +func sha256Digest(b []byte) string { + return fmt.Sprintf("sha256:%x", sha256.Sum256(b)) +} + // TestVulnDBMatchingCost measures what Grype adds to a scan, which every // performance number in SCANNER_BUGS.md section 3 explicitly excludes. It // asserts nothing about the numbers — a threshold here would be a flake on diff --git a/scanner/internal/scan/syft.go b/scanner/internal/scan/syft.go index be2d357..8ea7f0a 100644 --- a/scanner/internal/scan/syft.go +++ b/scanner/internal/scan/syft.go @@ -12,13 +12,21 @@ import ( "github.com/anchore/syft/syft/format" "github.com/anchore/syft/syft/format/spdxjson" "github.com/anchore/syft/syft/sbom" + "github.com/anchore/syft/syft/source" "github.com/anchore/syft/syft/source/stereoscopesource" ) // generateSBOM generates an SBOM using Syft from an OCI image layout directory. // Returns the SBOM object, SBOM JSON bytes, and its digest. -func generateSBOM(ctx context.Context, ociLayoutDir string) (*sbom.SBOM, []byte, string, error) { - slog.Info("Generating SBOM with Syft", "ociLayout", ociLayoutDir) +// +// sourceRef names the thing being scanned. It is what Syft records as the +// source, and it must be a function of the content and nothing else: it lands +// in the SBOM's documentName, documentNamespace, root package and describing +// relationship, and grype.go copies the whole source description into the +// vulnerability report. Callers pass the manifest digest — see the comment on +// the stereoscopesource config below. +func generateSBOM(ctx context.Context, ociLayoutDir, sourceRef string) (*sbom.SBOM, []byte, string, error) { + slog.Info("Generating SBOM with Syft", "ociLayout", ociLayoutDir, "source", sourceRef) // Create stereoscope OCI directory provider tmpGen := file.NewTempDirGenerator("syft-scan") @@ -37,8 +45,33 @@ func generateSBOM(ctx context.Context, ociLayoutDir string) (*sbom.SBOM, []byte, // Wrap in Syft source — src.Close() calls img.Cleanup() internally, // so we don't defer img.Cleanup() separately. + // + // The reference used to be ociLayoutDir, which is the per-job + // os.MkdirTemp("scan-*") path buildOCILayout hands us. That path is + // different on every scan, and it reaches five places in the encoded SPDX + // document (documentName, documentNamespace, the DocumentRoot package's + // name and SPDXID, and the DESCRIBES relationship) plus the "source" + // object grype.go embeds in the vulnerability report. Rescanning unchanged + // content therefore produced byte-different artifacts under fresh digests, + // so the hold uploaded new blobs and orphaned the old ones on every pass of + // its stale-scan loop. + // + // The manifest digest is the identity the hold already uses: the + // io.atcr.hold.scan record's rkey is atproto.ScanRecordKey(manifestDigest), + // one record per digest regardless of which repository the job arrived + // under. "repository@digest" reads better, but the same digest is + // dispatched under whichever user's manifest discovery walked first, so it + // would reintroduce the same drift at a slower rate. + // + // Alias pins Name and Version rather than letting Describe() infer them: + // it runs the reference through distribution/reference, where a bare + // "sha256:" parses as the repository "sha256" tagged with the hex. src := stereoscopesource.New(img, stereoscopesource.ImageConfig{ - Reference: ociLayoutDir, + Reference: sourceRef, + Alias: source.Alias{ + Name: sourceRef, + Version: sourceRef, + }, }) defer src.Close() diff --git a/scanner/internal/scan/syft_test.go b/scanner/internal/scan/syft_test.go new file mode 100644 index 0000000..b4cf159 --- /dev/null +++ b/scanner/internal/scan/syft_test.go @@ -0,0 +1,340 @@ +package scan + +import ( + "archive/tar" + "bytes" + "compress/gzip" + "context" + "crypto/sha256" + "encoding/json" + "fmt" + "os" + "path/filepath" + "sort" + "strconv" + "testing" + "time" +) + +// The SBOM the scanner publishes must be a function of the image it describes +// and nothing else. It is content-addressed on the hold: the digest of these +// bytes becomes sbomDigest on the io.atcr.hold.scan record, and a digest that +// moves means every rescan of unchanged content uploads a fresh blob and +// orphans the one before it. The hold's stale-scan loop rescans on a schedule, +// so anything scan-local that leaks into these bytes accumulates storage +// forever. +// +// The scenario below is exactly what the stale loop does: the same image, +// scanned twice, from two different scratch directories. buildOCILayout makes +// a fresh os.MkdirTemp "scan-*" per job, so the two paths are never the same +// in production either. + +// testLayoutFiles builds a minimal but real OCI image layout as an in-memory +// path -> bytes map, so the same bytes can be written into two different +// directories. No fixture download, no network: this runs in the default +// suite. +func testLayoutFiles(t *testing.T) map[string][]byte { + t.Helper() + + // One gzipped tar layer. Everything is pinned (fixed modtimes, USTAR + // headers, gzip with no embedded timestamp) so the bytes are the same on + // every run, not merely the same between the two copies. + var raw bytes.Buffer + tw := tar.NewWriter(&raw) + for _, f := range []struct{ name, body string }{ + {"etc/os-release", "NAME=\"Alpine Linux\"\nID=alpine\nVERSION_ID=3.19.0\nPRETTY_NAME=\"Alpine Linux v3.19\"\n"}, + {"bin/hello", "#!/bin/sh\necho hello\n"}, + } { + hdr := &tar.Header{ + Typeflag: tar.TypeReg, + Name: f.name, + Mode: 0o644, + Size: int64(len(f.body)), + ModTime: time.Unix(1000000000, 0).UTC(), + Format: tar.FormatUSTAR, + } + if err := tw.WriteHeader(hdr); err != nil { + t.Fatalf("tar header %s: %v", f.name, err) + } + if _, err := tw.Write([]byte(f.body)); err != nil { + t.Fatalf("tar body %s: %v", f.name, err) + } + } + if err := tw.Close(); err != nil { + t.Fatalf("close tar: %v", err) + } + uncompressed := raw.Bytes() + diffID := fmt.Sprintf("sha256:%x", sha256.Sum256(uncompressed)) + + var gz bytes.Buffer + zw := gzip.NewWriter(&gz) + if _, err := zw.Write(uncompressed); err != nil { + t.Fatalf("gzip write: %v", err) + } + if err := zw.Close(); err != nil { + t.Fatalf("gzip close: %v", err) + } + layer := gz.Bytes() + layerDigest := fmt.Sprintf("sha256:%x", sha256.Sum256(layer)) + + config, err := json.Marshal(map[string]any{ + "architecture": "amd64", + "os": "linux", + "config": map[string]any{"Cmd": []string{"/bin/hello"}}, + "rootfs": map[string]any{"type": "layers", "diff_ids": []string{diffID}}, + }) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + configDigest := fmt.Sprintf("sha256:%x", sha256.Sum256(config)) + + manifest, err := json.Marshal(map[string]any{ + "schemaVersion": 2, + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "config": map[string]any{ + "mediaType": "application/vnd.oci.image.config.v1+json", + "digest": configDigest, + "size": len(config), + }, + "layers": []any{map[string]any{ + "mediaType": "application/vnd.oci.image.layer.v1.tar+gzip", + "digest": layerDigest, + "size": len(layer), + }}, + }) + if err != nil { + t.Fatalf("marshal manifest: %v", err) + } + manifestDigest := fmt.Sprintf("sha256:%x", sha256.Sum256(manifest)) + + index, err := json.Marshal(map[string]any{ + "schemaVersion": 2, + "manifests": []any{map[string]any{ + "mediaType": "application/vnd.oci.image.manifest.v1+json", + "digest": manifestDigest, + "size": len(manifest), + }}, + }) + if err != nil { + t.Fatalf("marshal index: %v", err) + } + + hex := func(d string) string { return d[len("sha256:"):] } + return map[string][]byte{ + "oci-layout": []byte(`{"imageLayoutVersion":"1.0.0"}`), + "index.json": index, + "blobs/sha256/" + hex(manifestDigest): manifest, + "blobs/sha256/" + hex(configDigest): config, + "blobs/sha256/" + hex(layerDigest): layer, + } +} + +// writeTestLayout materialises the layout under a fresh scratch directory, +// standing in for one buildOCILayout call. +func writeTestLayout(t *testing.T, files map[string][]byte) string { + t.Helper() + dir := t.TempDir() + for name, body := range files { + path := filepath.Join(dir, filepath.FromSlash(name)) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", path, err) + } + if err := os.WriteFile(path, body, 0o644); err != nil { + t.Fatalf("write %s: %v", path, err) + } + } + return dir +} + +// scanLocalResidue is the set of top-level SPDX keys that still move between +// two encodings of the same image, and why. Both come from Syft's SPDX +// encoder and neither is configurable: +// +// - creationInfo carries "created", set from time.Now() in +// syft/format/common/spdxhelpers.ToFormatModel; +// - documentNamespace appends a fresh uuid.NewRandom() in +// syft/format/internal/spdxutil/helpers.DocumentNamespace. +// +// So sbomDigest is still not stable across a rescan, even though nothing +// scan-local reaches the document any more. That is a separate defect with a +// separate cause, and this set is what keeps the two apart: if a third key +// starts moving, the assertions below fail rather than shrugging, and if these +// two stop moving the test says so and asks to be tightened to a plain digest +// comparison. +var scanLocalResidue = map[string]string{ + "creationInfo": "creationInfo.created is time.Now() inside Syft's SPDX encoder", + "documentNamespace": "documentNamespace embeds a random UUID inside Syft's SPDX encoder", +} + +// TestSBOMIsStableAcrossScanDirectories is the requirement, as far as this +// package can carry it: the same image scanned twice from two different +// scratch directories must produce the same SBOM, apart from the two encoder +// fields named in scanLocalResidue. +// +// The scenario is what the hold's stale-scan loop does. buildOCILayout makes a +// fresh os.MkdirTemp("scan-*") per job, so the two paths are never equal in +// production either. +func TestSBOMIsStableAcrossScanDirectories(t *testing.T) { + files := testLayoutFiles(t) + dirA := writeTestLayout(t, files) + dirB := writeTestLayout(t, files) + + ctx := context.Background() + + _, jsonA, _, err := generateSBOM(ctx, dirA, testManifestDigest) + if err != nil { + t.Fatalf("first scan: %v", err) + } + _, jsonB, _, err := generateSBOM(ctx, dirB, testManifestDigest) + if err != nil { + t.Fatalf("rescan: %v", err) + } + + // 1. Nothing outside the known residue may move. This is the assertion + // that would have caught the original defect: documentName, + // documentNamespace, packages and relationships all carried the scratch + // path, and four of those five are now stable. + for _, key := range topLevelDiff(t, jsonA, jsonB) { + if _, known := scanLocalResidue[key]; !known { + t.Errorf("SBOM key %q differs between two scans of identical content:\n %s\n %s", + key, firstBytes(fieldOf(t, jsonA, key)), firstBytes(fieldOf(t, jsonB, key))) + } + } + + // 2. With the residue removed the two documents must be byte-identical, so + // this is a real digest comparison and not a field-by-field spot check. + if a, b := digestWithout(t, jsonA, scanLocalResidue), digestWithout(t, jsonB, scanLocalResidue); a != b { + t.Errorf("SBOMs differ beyond %v:\n first: %s\nsecond: %s", keysOf(scanLocalResidue), a, b) + } + + // 3. The scratch path must appear nowhere at all. The diff above only sees + // keys that moved; a path baked identically into both documents would + // pass it and still be wrong. + for _, dir := range []string{dirA, dirB} { + for name, doc := range map[string][]byte{"first": jsonA, "second": jsonB} { + if bytes.Contains(doc, []byte(dir)) { + t.Errorf("%s SBOM still contains the scan directory %s", name, dir) + } + } + } + + // 4. And it must be the manifest digest that took its place, which is what + // makes a stored SBOM say which image it describes. + if got := fieldOf(t, jsonA, "name"); got != strconv.Quote(testManifestDigest) { + t.Errorf("SBOM documentName is %s, want the manifest digest %q", got, testManifestDigest) + } + + // If the residue is ever fixed upstream or worked around here, this test + // should become a plain digest equality check. + for key, why := range scanLocalResidue { + if fieldOf(t, jsonA, key) == fieldOf(t, jsonB, key) { + t.Errorf("%q no longer varies between scans (%s); sbomDigest may now be stable, "+ + "so drop it from scanLocalResidue and assert digest equality directly", key, why) + } + } +} + +// testManifestDigest stands in for job.ManifestDigest: the identity the hold +// keys the io.atcr.hold.scan record by, and now the SBOM's source reference. +const testManifestDigest = "sha256:0f1e2d3c4b5a69788796a5b4c3d2e1f00f1e2d3c4b5a69788796a5b4c3d2e1f0" + +// TestSBOMSourceReferenceIsTheManifestDigest pins the one field grype.go +// copies wholesale into the vulnerability report ("source": s.Source). The +// report has no timestamp and no UUID, so unlike the SBOM its digest is fully +// determined by this value plus the matches — which is what lets +// TestVulnDBScanIsDeterministic, in internal/e2e, assert vulnDigest equality +// outright. +func TestSBOMSourceReferenceIsTheManifestDigest(t *testing.T) { + dir := writeTestLayout(t, testLayoutFiles(t)) + + result, _, _, err := generateSBOM(context.Background(), dir, testManifestDigest) + if err != nil { + t.Fatalf("scan: %v", err) + } + + desc := result.Source + if desc.Name != testManifestDigest { + t.Errorf("source name is %q, want the manifest digest %q", desc.Name, testManifestDigest) + } + if desc.Version != testManifestDigest { + t.Errorf("source version is %q, want the manifest digest %q", desc.Version, testManifestDigest) + } + + // The report embeds the whole description, metadata included, so the + // scratch path must not survive anywhere inside it. + encoded, err := json.Marshal(desc) + if err != nil { + t.Fatalf("marshal source description: %v", err) + } + if bytes.Contains(encoded, []byte(dir)) { + t.Errorf("the source description still carries the scan directory %s: %s", dir, encoded) + } +} + +// digestWithout hashes the document with the named top-level keys removed. +func digestWithout(t *testing.T, doc []byte, drop map[string]string) string { + t.Helper() + var m map[string]json.RawMessage + if err := json.Unmarshal(doc, &m); err != nil { + t.Fatalf("parse SBOM: %v", err) + } + for k := range drop { + delete(m, k) + } + // json.Marshal of a map sorts keys, so this is stable. + stripped, err := json.Marshal(m) + if err != nil { + t.Fatalf("re-encode SBOM: %v", err) + } + return fmt.Sprintf("sha256:%x", sha256.Sum256(stripped)) +} + +func keysOf(m map[string]string) []string { + out := make([]string, 0, len(m)) + for k := range m { + out = append(out, k) + } + sort.Strings(out) + return out +} + +// topLevelDiff names the top-level JSON keys whose encoded values differ. +func topLevelDiff(t *testing.T, a, b []byte) []string { + t.Helper() + var ka, kb map[string]json.RawMessage + if err := json.Unmarshal(a, &ka); err != nil { + t.Fatalf("parse first SBOM: %v", err) + } + if err := json.Unmarshal(b, &kb); err != nil { + t.Fatalf("parse second SBOM: %v", err) + } + var moved []string + for k := range ka { + if string(ka[k]) != string(kb[k]) { + moved = append(moved, k) + } + } + for k := range kb { + if _, ok := ka[k]; !ok { + moved = append(moved, k) + } + } + sort.Strings(moved) + return moved +} + +func fieldOf(t *testing.T, doc []byte, key string) string { + t.Helper() + var m map[string]json.RawMessage + if err := json.Unmarshal(doc, &m); err != nil { + t.Fatalf("parse SBOM: %v", err) + } + return string(m[key]) +} + +func firstBytes(s string) string { + if len(s) > 400 { + return s[:400] + "..." + } + return s +} diff --git a/scanner/internal/scan/worker.go b/scanner/internal/scan/worker.go index 1754040..820aeba 100644 --- a/scanner/internal/scan/worker.go +++ b/scanner/internal/scan/worker.go @@ -266,7 +266,7 @@ func (wp *WorkerPool) processJob(ctx context.Context, job *scanner.ScanJob) (*sc // Step 2: Generate SBOM with Syft slog.Info("Generating SBOM", "repository", job.Repository) - sbomResult, sbomJSON, sbomDigest, err := generateSBOM(ctx, ociLayoutDir) + sbomResult, sbomJSON, sbomDigest, err := generateSBOM(ctx, ociLayoutDir, job.ManifestDigest) if err != nil { return nil, fmt.Errorf("failed to generate SBOM: %w", err) }