diff --git a/internal/cli/check.go b/internal/cli/check.go index 662a6d6..c848307 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -2,6 +2,7 @@ package cli import ( + "bytes" "context" "encoding/hex" "errors" @@ -94,47 +95,36 @@ func findManifest(fs afero.Fs, dir string) (string, error) { return path, nil } -// fetchManifestToTemp downloads a manifest URL to a temporary file and -// returns the temp file path. The caller is responsible for removing it. -func (mfa *CLIApp) fetchManifestToTemp( - ctx context.Context, url string, -) (string, error) { - rc, fetchErr := mfa.openManifestReader(ctx, url) - if fetchErr != nil { - return "", fetchErr +// downloadManifest downloads the manifest at url into memory. It refuses a +// manifest larger than mfa.maxManifestSize. +func (mfa *CLIApp) downloadManifest(ctx context.Context, url string) ([]byte, error) { + rc, err := mfa.openManifestReader(ctx, url) + if err != nil { + return nil, err } - tmpFile, tmpErr := afero.TempFile(mfa.Fs, "", "mfer-manifest-*.mf") - if tmpErr != nil { - _ = rc.Close() + defer func() { _ = rc.Close() }() - return "", tmpErr - } - - tmpPath := tmpFile.Name() - - // Copying stops one byte past mfa.maxManifestSize, which is enough to + // Reading stops one byte past mfa.maxManifestSize, which is enough to // tell that the manifest is too large. - written, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfa.maxManifestSize+1)) - _ = rc.Close() - _ = tmpFile.Close() - - if cpErr == nil && written > mfa.maxManifestSize { - cpErr = fmt.Errorf("%w of %d bytes", errManifestTooLarge, mfa.maxManifestSize) + manifestData, err := io.ReadAll(io.LimitReader(rc, mfa.maxManifestSize+1)) + if err != nil { + return nil, fmt.Errorf("download manifest: %w", err) } - if cpErr != nil { - _ = mfa.Fs.Remove(tmpPath) - - return "", fmt.Errorf("download manifest: %w", cpErr) + if int64(len(manifestData)) > mfa.maxManifestSize { + return nil, fmt.Errorf("download manifest: %w of %d bytes", + errManifestTooLarge, mfa.maxManifestSize) } - return tmpPath, nil + return manifestData, nil } // verifyRequiredSigner enforces the --require-signature fingerprint -// against the key that made the manifest's signature. -func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { +// against signer, the fingerprint of the key that made the manifest's +// signature, which is empty when the manifest is not signed. Loading the +// manifest checked that signer is that key's fingerprint. +func verifyRequiredSigner(signer, requiredSigner string) error { // Validate fingerprint format: must be exactly 40 hex characters if len(requiredSigner) != fingerprintHexLen { return fmt.Errorf("%w, got %d", errInvalidFingerprint, len(requiredSigner)) @@ -145,15 +135,11 @@ func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { return fmt.Errorf("invalid fingerprint: %w", err) } - if !chk.IsSigned() { + if signer == "" { return fmt.Errorf("%w, but signature from %s is required", errManifestNotSigned, requiredSigner) } - // Loading the manifest checked that the signer is the fingerprint of - // the key that made the signature. - signer := string(chk.Signer()) - // Compare fingerprints - must be exact match (case-insensitive) if !strings.EqualFold(signer, requiredSigner) { return fmt.Errorf("embedded signing key fingerprint %s %w %s", @@ -299,32 +285,28 @@ func (mfa *CLIApp) checkManifestOperation( return err } - // Done before a URL is swapped for the temp file it is downloaded to, - // whose directory is not the base. basePath := resolveBasePath(cmd, manifestPath) - // URL manifests need to be downloaded to a temp file for the checker + opts := &mfer.CheckerOptions{BasePath: basePath, Fs: mfa.Fs} + + // A manifest given by URL is downloaded into memory and read from + // there. if isHTTPURL(manifestPath) { - tmpPath, tmpErr := mfa.fetchManifestToTemp(ctx, manifestPath) - if tmpErr != nil { - return tmpErr + manifestData, downloadErr := mfa.downloadManifest(ctx, manifestPath) + if downloadErr != nil { + return downloadErr } - defer func() { _ = mfa.Fs.Remove(tmpPath) }() - - manifestPath = tmpPath + opts.ManifestReader = bytes.NewReader(manifestData) + } else { + opts.ManifestPath = manifestPath } showProgress := cmd.Bool("progress") log.Infof("checking manifest %s with base %s", manifestPath, basePath) - // Create checker - chk, err := mfer.NewChecker(&mfer.CheckerOptions{ - ManifestPath: manifestPath, - BasePath: basePath, - Fs: mfa.Fs, - }) + chk, err := mfer.NewChecker(opts) if err != nil { return fmt.Errorf("load manifest: %w", err) } @@ -332,7 +314,7 @@ func (mfa *CLIApp) checkManifestOperation( // Check signature requirement. An empty fingerprint is checked too, // and refused as one of the wrong length. if cmd.IsSet(flagRequireSignature) { - err = verifyRequiredSigner(chk, cmd.String(flagRequireSignature)) + err = verifyRequiredSigner(string(chk.Signer()), cmd.String(flagRequireSignature)) if err != nil { return err } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 2490d75..768087e 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -1167,6 +1167,29 @@ func TestCheckURLManifestResolvesEntriesAgainstCurrentDirectory(t *testing.T) { assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) } +// TestCheckURLManifestWithoutTempDirectory checks the tree under --base +// against a manifest given by URL while TMPDIR names a directory that does +// not exist. check keeps the manifest it downloads in memory, so it passes. +// Setting TMPDIR keeps it from running in parallel with other tests. +func TestCheckURLManifestWithoutTempDirectory(t *testing.T) { + files := map[string][]byte{testFileTxt: []byte("hello")} + + server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files)) + defer server.Close() + + base := t.TempDir() + require.NoError(t, + os.WriteFile(filepath.Join(base, testFileTxt), files[testFileTxt], 0o600)) + + t.Setenv("TMPDIR", filepath.Join(t.TempDir(), "missing")) + + opts := testOpts([]string{ + testApp, cmdCheck, testFlagNoExtra, testFlagBase, base, + server.URL + "/" + defaultManifestName, + }, afero.NewOsFs()) + assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) +} + // unlistableDirFs is a filesystem on which one directory cannot be listed. type unlistableDirFs struct { afero.Fs diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 5445ebd..11ecb4b 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -50,38 +50,6 @@ func runLocked(fn func() error) error { return fn() } -// unsignedChecker builds a Checker over a freshly scanned, unsigned manifest. -func unsignedChecker(t *testing.T) *mfer.Checker { - t.Helper() - - // The scan and the manifest load log to the process-global logger, so - // serialize with the CLI runs. - runMu.Lock() - defer runMu.Unlock() - - fs := afero.NewMemMapFs() - require.NoError(t, fs.MkdirAll("/d", 0o755)) - require.NoError(t, afero.WriteFile(fs, "/d/f.txt", []byte("hi"), 0o644)) - - s := mfer.NewScannerWithOptions(&mfer.ScannerOptions{Fs: fs}) - require.NoError(t, s.EnumeratePath("/d", nil)) - - var buf bytes.Buffer - - require.NoError(t, s.ToManifest(context.Background(), &buf, nil)) - require.NoError(t, afero.WriteFile(fs, "/d/index.mf", buf.Bytes(), 0o644)) - - chk, err := mfer.NewChecker(&mfer.CheckerOptions{ - ManifestPath: "/d/index.mf", - BasePath: "/d", - Fs: fs, - }) - require.NoError(t, err) - require.False(t, chk.IsSigned()) - - return chk -} - func TestNoManifestFoundMessage(t *testing.T) { t.Parallel() @@ -97,7 +65,7 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("invalid fingerprint length", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(unsignedChecker(t), "12345678") + err := verifyRequiredSigner("", "12345678") require.ErrorIs(t, err, errInvalidFingerprint) assert.EqualError(t, err, "invalid fingerprint: must be exactly 40 hex characters, got 8") @@ -106,7 +74,7 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { t.Run("manifest not signed", func(t *testing.T) { t.Parallel() - err := verifyRequiredSigner(unsignedChecker(t), msgFpA) + err := verifyRequiredSigner("", msgFpA) require.ErrorIs(t, err, errManifestNotSigned) assert.EqualError(t, err, "manifest is not signed, but signature from "+msgFpA+" is required") @@ -121,13 +89,13 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { func TestSignerMismatchMessage(t *testing.T) { t.Parallel() - chk := signedChecker(t, - signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})) + signer := string(signedChecker(t, + signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})).Signer()) - err := verifyRequiredSigner(chk, msgFpB) + err := verifyRequiredSigner(signer, msgFpB) require.ErrorIs(t, err, errSignerMismatch) assert.EqualError(t, err, - "embedded signing key fingerprint "+string(chk.Signer())+ + "embedded signing key fingerprint "+signer+ " does not match required "+msgFpB) } diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index d2497f2..9cd3268 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -19,7 +19,6 @@ import ( "github.com/dustin/go-humanize" "github.com/multiformats/go-multihash" - "github.com/spf13/afero" "github.com/urfave/cli/v3" "sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/mfer" @@ -475,7 +474,7 @@ func (mfa *CLIApp) fetchManifest( } if requiredSigner != "" { - err = verifyFetchedSigner(manifestData, requiredSigner) + err = verifyRequiredSigner(string(manifest.Signer()), requiredSigner) if err != nil { return nil, nil, err } @@ -553,31 +552,6 @@ func checkNoNameClash(files []*mfer.MFFilePath) error { return nil } -// verifyFetchedSigner enforces --require-signature on the fetched manifest -// exactly as check does. verifyRequiredSigner takes a Checker, which loads -// its manifest from a file, so the manifest is handed to it as a file in -// memory. -func verifyFetchedSigner(manifestData []byte, requiredSigner string) error { - memFs := afero.NewMemMapFs() - manifestPath := "/" + defaultManifestName - - err := afero.WriteFile(memFs, manifestPath, manifestData, filePerms) - if err != nil { - return err - } - - chk, err := mfer.NewChecker(&mfer.CheckerOptions{ - ManifestPath: manifestPath, - BasePath: "/", - Fs: memFs, - }) - if err != nil { - return fmt.Errorf("load manifest: %w", err) - } - - return verifyRequiredSigner(chk, requiredSigner) -} - // saveManifest writes the fetched manifest into dest under the default // manifest name, the way fetch writes every file: to a new temp file that // is then renamed into place. diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index bb4b053..66b9d5f 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -489,18 +489,17 @@ func TestFetchStopsReadingFilePastListedSize(t *testing.T) { // TestManifestDownloadStopsAtLimit serves a manifest that never ends to // fetch and to check, with the most they download of a manifest lowered -// to 64 KiB. Each must stop reading at that limit, fail with an error -// naming it and leave no temp file. +// to 64 KiB. Each must stop reading at that limit and fail with an error +// naming it. func TestManifestDownloadStopsAtLimit(t *testing.T) { + t.Parallel() + server := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { _, _ = io.Copy(w, zeros{}) })) defer server.Close() - tmpDir := t.TempDir() - t.Setenv("TMPDIR", tmpDir) - mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10} fetch := mfa.fetchCommand() @@ -520,10 +519,6 @@ func TestManifestDownloadStopsAtLimit(t *testing.T) { "download manifest: file exceeds maximum allowed size of 65536 bytes", cmd.Name) } - - leftover, err := os.ReadDir(tmpDir) - require.NoError(t, err) - assert.Empty(t, leftover) } //nolint:paralleltest // changes the process-global working directory diff --git a/mfer/checker.go b/mfer/checker.go index a34b689..5184058 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -80,7 +80,8 @@ type Checker struct { // manifestPaths is a set of paths in the manifest for quick lookup manifestPaths map[RelFilePath]struct{} // manifestInfo is the manifest file, which FindExtraFiles leaves out, - // matched with os.SameFile. + // matched with os.SameFile. It is nil for a manifest read from + // CheckerOptions.ManifestReader. manifestInfo os.FileInfo // signature info from the manifest signature []byte @@ -90,8 +91,13 @@ type Checker struct { // CheckerOptions configures a Checker. type CheckerOptions struct { - // ManifestPath is the manifest file to check against (required). + // ManifestPath is the manifest file to check against. It is required + // unless ManifestReader is given. ManifestPath string + // ManifestReader, if not nil, is read for the manifest to check against + // in place of ManifestPath, such as a manifest downloaded into memory. + // FindExtraFiles then has no manifest file to leave out. + ManifestReader io.Reader // BasePath is the directory relative to which manifest paths are // resolved (required). BasePath string @@ -100,9 +106,10 @@ type CheckerOptions struct { } // NewChecker creates a new Checker with the given options. It returns an -// error if opts is nil or either path is empty. +// error if opts is nil, the base path is empty, or neither a manifest path +// nor a manifest reader is given. func NewChecker(opts *CheckerOptions) (*Checker, error) { - if opts == nil || opts.ManifestPath == "" { + if opts == nil || (opts.ManifestPath == "" && opts.ManifestReader == nil) { return nil, errManifestPathEmpty } @@ -115,12 +122,30 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) { fs = afero.NewOsFs() } - m, err := NewManifestFromFile(&ManifestFromFileOptions{ - Path: opts.ManifestPath, - Fs: fs, - }) - if err != nil { - return nil, err + var ( + m *manifest + manifestInfo os.FileInfo + err error + ) + + if opts.ManifestReader != nil { + m, err = NewManifestFromReader(opts.ManifestReader) + if err != nil { + return nil, err + } + } else { + m, err = NewManifestFromFile(&ManifestFromFileOptions{ + Path: opts.ManifestPath, + Fs: fs, + }) + if err != nil { + return nil, err + } + + manifestInfo, err = fs.Stat(opts.ManifestPath) + if err != nil { + return nil, err + } } abs, err := filepath.Abs(opts.BasePath) @@ -135,11 +160,6 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) { manifestPaths[RelFilePath(f.GetPath())] = struct{}{} } - manifestInfo, err := fs.Stat(opts.ManifestPath) - if err != nil { - return nil, err - } - return &Checker{ basePath: AbsFilePath(abs), files: files, diff --git a/mfer/checker_test.go b/mfer/checker_test.go index d7180b4..b000046 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -189,6 +189,38 @@ func TestNewCheckerNilFsUsesOsFs(t *testing.T) { assert.Equal(t, StatusOK, (<-results).Status) } +// TestNewCheckerReadsManifestFromReader gives NewChecker the manifest as a +// reader and no manifest path, on a filesystem that holds only the files it +// lists: it checks those files and finds nothing unlisted. +func TestNewCheckerReadsManifestFromReader(t *testing.T) { + t.Parallel() + + files := map[string][]byte{testFile1: []byte("hello")} + + manifestFs := afero.NewMemMapFs() + createTestManifest(t, manifestFs, testManifestPath, files) + manifestData, err := afero.ReadFile(manifestFs, testManifestPath) + require.NoError(t, err) + + fs := afero.NewMemMapFs() + createFilesOnDisk(t, fs, files) + + chk, err := NewChecker(&CheckerOptions{ + ManifestReader: bytes.NewReader(manifestData), + BasePath: testDataDir, + Fs: fs, + }) + require.NoError(t, err) + + results := make(chan Result, 1) + require.NoError(t, chk.Check(context.Background(), results, nil)) + assert.Equal(t, Result{Path: testFile1, Status: StatusOK}, <-results) + + extras := make(chan Result, len(files)) + require.NoError(t, chk.FindExtraFiles(context.Background(), extras)) + assert.Empty(t, extras) +} + func TestCheckerFileCountAndTotalBytes(t *testing.T) { t.Parallel()