diff --git a/syft/source/snapsource/manifest.go b/syft/source/snapsource/manifest.go index 8d6c61611..a2d0b04d2 100644 --- a/syft/source/snapsource/manifest.go +++ b/syft/source/snapsource/manifest.go @@ -1,6 +1,7 @@ package snapsource import ( + "errors" "fmt" "github.com/goccy/go-yaml" @@ -21,6 +22,11 @@ type snapManifest struct { const manifestLocation = "/meta/snap.yaml" +// errNoManifest means the payload is a readable squashfs that simply does not carry a snap manifest. +// This is distinct from a manifest that is present but unusable, and callers are expected to treat +// the two differently. +var errNoManifest = errors.New("no snap manifest file found") + func parseManifest(resolver file.Resolver) (*snapManifest, error) { locations, err := resolver.FilesByPath(manifestLocation) if err != nil { @@ -28,7 +34,7 @@ func parseManifest(resolver file.Resolver) (*snapManifest, error) { } if len(locations) == 0 { - return nil, fmt.Errorf("no snap manifest file found") + return nil, errNoManifest } if len(locations) > 1 { @@ -48,9 +54,8 @@ func parseManifest(resolver file.Resolver) (*snapManifest, error) { return nil, fmt.Errorf("unable to decode snap manifest file: %w", err) } - if manifest.Name == "" || manifest.Version == "" { - return nil, fmt.Errorf("invalid snap manifest file: missing name or version") - } - + // note: name and version are deliberately not validated here. a manifest that decoded but is + // missing them still carries usable fields (summary, base, grade, ...) and throwing it away would + // describe less than we know. the caller surfaces the missing identity. return &manifest, nil } diff --git a/syft/source/snapsource/snap.go b/syft/source/snapsource/snap.go index c32ed86b6..0cf413a0d 100644 --- a/syft/source/snapsource/snap.go +++ b/syft/source/snapsource/snap.go @@ -77,22 +77,33 @@ func getRemoteSnapFile(ctx context.Context, fs afero.Fs, getter intFile.Getter, return newSnapFileFromRemote(ctx, fs, cfg, getter, info) } -func newSnapFileFromRemote(ctx context.Context, fs afero.Fs, cfg Config, getter intFile.Getter, info *remoteSnap) (*snapFile, error) { +func newSnapFileFromRemote(ctx context.Context, fs afero.Fs, cfg Config, getter intFile.Getter, info *remoteSnap) (_ *snapFile, err error) { t, err := afero.TempDir(fs, "", "syft-snap-") if err != nil { return nil, fmt.Errorf("failed to create temp directory: %w", err) } + closer := func() error { + return fs.RemoveAll(t) + } + + // any failure past this point must not leave the temp directory (or a partially downloaded snap) + // behind. On success the caller owns the directory by way of snapFile.Cleanup. + defer func() { + if err == nil { + return + } + if closeErr := closer(); closeErr != nil { + log.WithFields("error", closeErr, "directory", t).Warn("unable to remove temp directory for snap") + } + }() + snapFilePath := path.Join(t, path.Base(info.URL)) err = downloadSnap(getter, info, snapFilePath) if err != nil { return nil, fmt.Errorf("failed to download snap file: %w", err) } - closer := func() error { - return fs.RemoveAll(t) - } - mimeType, digests, err := getSnapFileInfo(ctx, fs, snapFilePath, cfg.DigestAlgorithms) if err != nil { return nil, err diff --git a/syft/source/snapsource/snap_source.go b/syft/source/snapsource/snap_source.go index 6710615d1..962e57e3c 100644 --- a/syft/source/snapsource/snap_source.go +++ b/syft/source/snapsource/snap_source.go @@ -3,6 +3,7 @@ package snapsource import ( "context" "crypto" + "errors" "fmt" "io" "os" @@ -119,7 +120,21 @@ func newFromPath(cfg Config, f *snapFile) (source.Source, error) { closer: f.Cleanup, } - return s, s.extractManifest() + if err := s.extractManifest(); err != nil { + // It is tried on any path ending in .snap or .squashfs, so declining one is routine and + // a later provider usually succeeds. When the user asked for --from snap there is no + // fallback and this error reaches them intact. + log.WithFields("error", err, "path", f.Path).Debug("unable to read snap") + + // never hand back a source alongside an error: callers that discard the source on error would + // leave behind the open squashfs file and the temp directory holding a downloaded snap + if closeErr := s.Close(); closeErr != nil { + log.WithFields("error", closeErr, "path", f.Path).Warn("unable to clean up snap source") + } + return nil, err + } + + return s, nil } func (s *snapSource) extractManifest() error { @@ -128,13 +143,26 @@ func (s *snapSource) extractManifest() error { return fmt.Errorf("unable to create snap file resolver: %w", err) } + // a manifest problem must never fail the source. this source is the only provider that can descend + // into squashfs (filesource cannot), so returning an error here would fall through to a provider + // that reports zero packages for a payload that has plenty. describe what we can instead. manifest, err := parseManifest(r) - if err != nil { - return fmt.Errorf("unable to parse snap manifest file: %w", err) + switch { + case errors.Is(err, errNoManifest): + // not every squashfs payload is a snap, and one without a manifest is still worth cataloging + log.WithFields("path", s.squashfsPath).Debug("no snap manifest file found") + return nil + case err != nil: + // the manifest is there but unusable, which is worth surfacing: the SBOM will have no source + // name or version to show for it + log.WithFields("error", err, "path", s.squashfsPath).Warn("unable to parse snap manifest file") + return nil } - if manifest != nil { - s.manifest = *manifest + s.manifest = *manifest + + if s.manifest.Name == "" || s.manifest.Version == "" { + log.WithFields("path", s.squashfsPath).Warn("snap manifest is missing name or version") } return nil } @@ -176,25 +204,34 @@ func (s snapSource) Describe() source.Description { } } +// Close releases everything the source holds. Every step runs even if an earlier one fails, since +// bailing early would skip the temp directory removal and leave a downloaded snap on disk. Safe to +// call more than once. func (s *snapSource) Close() error { + s.mutex.Lock() + defer s.mutex.Unlock() + + var errs error if s.squashFileCloser != nil { if err := s.squashFileCloser(); err != nil { - return fmt.Errorf("unable to close snap resolver: %w", err) + errs = errors.Join(errs, fmt.Errorf("unable to close snap resolver: %w", err)) } s.squashFileCloser = nil } s.resolver = nil if s.fs != nil { if err := s.fs.Close(); err != nil { - return fmt.Errorf("unable to close snap squashfs: %w", err) + errs = errors.Join(errs, fmt.Errorf("unable to close snap squashfs: %w", err)) } + s.fs = nil } if s.closer != nil { if err := s.closer(); err != nil { - return fmt.Errorf("unable to close snap source: %w", err) + errs = errors.Join(errs, fmt.Errorf("unable to close snap source: %w", err)) } + s.closer = nil } - return nil + return errs } func (s *snapSource) FileResolver(_ source.Scope) (file.Resolver, error) { diff --git a/syft/source/snapsource/snap_source_test.go b/syft/source/snapsource/snap_source_test.go index 99e8da7dc..4515f9c47 100644 --- a/syft/source/snapsource/snap_source_test.go +++ b/syft/source/snapsource/snap_source_test.go @@ -2,15 +2,23 @@ package snapsource import ( "crypto" + "errors" "fmt" + "os" + "path/filepath" + "strings" + "sync" "testing" + diskFile "github.com/diskfs/go-diskfs/backend/file" + "github.com/diskfs/go-diskfs/filesystem/squashfs" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/anchore/go-homedir" "github.com/anchore/stereoscope/pkg/image" + "github.com/anchore/syft/syft/source" ) func TestNewFromLocal(t *testing.T) { @@ -84,3 +92,205 @@ func TestNewFromLocal(t *testing.T) { }) } } + +func TestNewFromPathOnResolverError(t *testing.T) { + tests := []struct { + name string + // remote snaps carry a cleanup closure for the temp directory holding the download; local snaps + // have none, since the file belongs to the user + remote bool + wantCleanupRuns int + wantFileRemoved bool + }{ + { + name: "downloaded snap is cleaned up", + remote: true, + wantCleanupRuns: 1, + wantFileRemoved: true, + }, + { + name: "local snap file is left in place", + remote: false, + wantCleanupRuns: 0, + wantFileRemoved: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // the resolver reads the squashfs from the real filesystem, so the payload must live on disk + tempDir := t.TempDir() + snapPath := filepath.Join(tempDir, "not-really-a-snap.snap") + require.NoError(t, os.WriteFile(snapPath, []byte("not squashfs"), 0600)) + + var cleanupRuns int + f := &snapFile{ + Path: snapPath, + MimeType: "text/plain", + } + if tt.remote { + f.Cleanup = func() error { + cleanupRuns++ + return os.RemoveAll(tempDir) + } + } + + got, err := newFromPath(Config{Request: snapPath}, f) + + require.Error(t, err) + assert.Contains(t, err.Error(), "unable to create snap file resolver") + + // a source returned alongside an error is discarded by callers without being closed, which + // would leave the downloaded snap behind + assert.Nil(t, got, "expected nil source on error") + assert.Equal(t, tt.wantCleanupRuns, cleanupRuns) + + _, statErr := os.Stat(snapPath) + if tt.wantFileRemoved { + assert.True(t, os.IsNotExist(statErr), "snap file was not cleaned up") + } else { + assert.NoError(t, statErr, "the user's snap file must never be removed") + } + }) + } +} + +// a manifest problem must never fail the source: snapsource is the only provider that can descend +// into squashfs, so an error here falls through to filesource and every package inside the payload +// silently disappears from the SBOM. +func TestNewFromPathToleratesManifestProblems(t *testing.T) { + tests := []struct { + name string + manifest string // written to /meta/snap.yaml; empty means no manifest at all + wantName string + wantVersion string + wantSummary string + }{ + { + name: "no manifest at all", + manifest: "", + }, + { + name: "manifest is not valid yaml", + manifest: "name: etcd\n\tversion: 3.4\n", + }, + { + name: "manifest is missing name and version", + manifest: "summary: a thing\nbase: core22\n", + wantSummary: "a thing", + }, + { + name: "manifest is complete", + manifest: "name: etcd\nversion: 3.4.0\nsummary: a thing\n", + wantName: "etcd", + wantVersion: "3.4.0", + wantSummary: "a thing", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + snapPath := writeSquashfs(t, tt.manifest) + + src, err := newFromPath(Config{Request: snapPath}, &snapFile{Path: snapPath}) + require.NoError(t, err, "a manifest problem must not fail the source") + require.NotNil(t, src) + t.Cleanup(func() { _ = src.Close() }) + + desc := src.Describe() + assert.Equal(t, tt.wantName, desc.Name) + assert.Equal(t, tt.wantVersion, desc.Version) + + meta, ok := desc.Metadata.(source.SnapMetadata) + require.True(t, ok, "expected snap metadata") + assert.Equal(t, tt.wantSummary, meta.Summary, "fields that did decode must survive") + + // the whole point: the payload is still walkable, so cataloging sees what is inside + r, err := src.FileResolver(source.SquashedScope) + require.NoError(t, err) + locations, err := r.FilesByPath("/payload.txt") + require.NoError(t, err) + assert.Len(t, locations, 1, "contents must remain catalogable regardless of the manifest") + }) + } +} + +func TestSnapSourceClose(t *testing.T) { + newSource := func(closeErr, cleanupErr error) (*snapSource, *int) { + var cleanupRuns int + return &snapSource{ + mutex: &sync.Mutex{}, + squashFileCloser: func() error { + return closeErr + }, + closer: func() error { + cleanupRuns++ + return cleanupErr + }, + }, &cleanupRuns + } + + t.Run("temp directory is removed even when closing the file handle fails", func(t *testing.T) { + s, cleanupRuns := newSource(errors.New("handle is busy"), nil) + + err := s.Close() + + // the whole point: a failure here used to return early and skip the cleanup below, leaving a + // downloaded snap on disk + require.Error(t, err) + assert.Contains(t, err.Error(), "handle is busy") + assert.Equal(t, 1, *cleanupRuns, "temp directory removal was skipped") + }) + + t.Run("every failure is reported, not just the first", func(t *testing.T) { + s, _ := newSource(errors.New("handle is busy"), errors.New("directory is busy")) + + err := s.Close() + + require.Error(t, err) + assert.Contains(t, err.Error(), "handle is busy") + assert.Contains(t, err.Error(), "directory is busy") + }) + + t.Run("is safe to call twice", func(t *testing.T) { + s, cleanupRuns := newSource(nil, nil) + + require.NoError(t, s.Close()) + require.NoError(t, s.Close()) + assert.Equal(t, 1, *cleanupRuns, "cleanup must not run again on a second close") + }) +} + +// writeSquashfs builds a real squashfs image containing /payload.txt and, when manifest is non-empty, +// /meta/snap.yaml. Returns the path to the image. +func writeSquashfs(t *testing.T, manifest string) string { + t.Helper() + + snapPath := filepath.Join(t.TempDir(), "test.snap") + f, err := os.Create(snapPath) + require.NoError(t, err) + defer func() { require.NoError(t, f.Close()) }() + + fs, err := squashfs.Create(diskFile.New(f, false), 0, 0, 4096) + require.NoError(t, err) + + write := func(path, contents string) { + t.Helper() + w, err := fs.OpenFile(path, os.O_CREATE|os.O_RDWR) + require.NoError(t, err) + _, err = w.Write([]byte(contents)) + require.NoError(t, err) + } + + // note: go-diskfs wants paths relative to the image root, without a leading slash + require.NoError(t, fs.Mkdir(".")) + write("payload.txt", "hello") + + if manifest != "" { + require.NoError(t, fs.Mkdir("meta")) + write(strings.TrimPrefix(manifestLocation, "/"), manifest) + } + + require.NoError(t, fs.Finalize(squashfs.FinalizeOptions{})) + return snapPath +} diff --git a/syft/source/snapsource/snap_test.go b/syft/source/snapsource/snap_test.go index 2a0dbbe48..e6b3ac450 100644 --- a/syft/source/snapsource/snap_test.go +++ b/syft/source/snapsource/snap_test.go @@ -264,6 +264,10 @@ func TestNewSnapFileFromRemote(t *testing.T) { _, err = fs.Stat(result.Path) assert.True(t, os.IsNotExist(err)) + + // cleanup owns the whole temp directory, not just the snap file within it + _, err = fs.Stat(filepath.Dir(result.Path)) + assert.True(t, os.IsNotExist(err), "temp directory outlived the cleanup closure") }, }, { @@ -315,6 +319,75 @@ func TestNewSnapFileFromRemote(t *testing.T) { expectError: true, errorMsg: "failed to download snap file", }, + { + name: "download writes partial data then fails", + cfg: Config{ + DigestAlgorithms: []crypto.Hash{crypto.SHA256}, + }, + info: &remoteSnap{ + snapIdentity: snapIdentity{ + Name: "partial-snap", + Channel: "stable", + Architecture: "amd64", + }, + URL: "https://api.snapcraft.io/download/partial_snap.snap", + }, + setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) { + mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/partial_snap.snap", mock.Anything).Run(func(args mock.Arguments) { + // simulate a transfer that writes some bytes before the connection drops + require.NoError(t, createMockSquashfsFile(fs, args.String(0))) + }).Return(fmt.Errorf("connection reset by peer")) + }, + expectError: true, + errorMsg: "failed to download snap file", + }, + { + // isSquashFSFile trusts the .snap extension, so non-squashfs content under that name is accepted + name: "downloaded file has a snap extension but non-snap content", + cfg: Config{ + DigestAlgorithms: []crypto.Hash{crypto.SHA256}, + }, + info: &remoteSnap{ + snapIdentity: snapIdentity{ + Name: "not-a-snap", + Channel: "stable", + Architecture: "amd64", + }, + URL: "https://api.snapcraft.io/download/not_a_snap.snap", + }, + setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) { + mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/not_a_snap.snap", mock.Anything).Run(func(args mock.Arguments) { + require.NoError(t, afero.WriteFile(fs, args.String(0), []byte("not squashfs"), 0644)) + }).Return(nil) + }, + expectError: false, + validate: func(t *testing.T, result *snapFile, fs afero.Fs) { + assert.NotNil(t, result) + assert.Contains(t, result.Path, "not_a_snap.snap") + assert.NotNil(t, result.Cleanup) + }, + }, + { + name: "downloaded file is not a snap", + cfg: Config{ + DigestAlgorithms: []crypto.Hash{crypto.SHA256}, + }, + info: &remoteSnap{ + snapIdentity: snapIdentity{ + Name: "not-a-snap", + Channel: "stable", + Architecture: "amd64", + }, + URL: "https://api.snapcraft.io/download/not_a_snap.tar.gz", + }, + setupMock: func(mockGetter *mockFileGetter, fs afero.Fs) { + mockGetter.On("GetFile", mock.AnythingOfType("string"), "https://api.snapcraft.io/download/not_a_snap.tar.gz", mock.Anything).Run(func(args mock.Arguments) { + require.NoError(t, afero.WriteFile(fs, args.String(0), []byte("not squashfs"), 0644)) + }).Return(nil) + }, + expectError: true, + errorMsg: "not a valid squashfs/snap file", + }, } for _, tt := range tests { @@ -334,8 +407,18 @@ func TestNewSnapFileFromRemote(t *testing.T) { assert.Contains(t, err.Error(), tt.errorMsg) } assert.Nil(t, result) + + // the temp directory and any partially downloaded payload must not be left behind. The + // download destination handed to the getter lives directly under the temp directory. + require.Len(t, mockGetter.Calls, 1) + tempDir := filepath.Dir(mockGetter.Calls[0].Arguments.String(0)) + _, statErr := fs.Stat(tempDir) + assert.True(t, os.IsNotExist(statErr), "temp directory %q was not cleaned up after failure", tempDir) } else { require.NoError(t, err) + if result.Cleanup != nil { + t.Cleanup(func() { _ = result.Cleanup() }) + } if tt.validate != nil { tt.validate(t, result, fs) }