Load the manifest check downloads from memory, not a temp file (closes #223) #227

Merged
clawbot merged 1 commits from issue-223-check-url-in-memory into next 2026-10-15 21:59:13 +02:00
7 changed files with 134 additions and 140 deletions
+33 -51
View File
@@ -2,6 +2,7 @@
package cli package cli
import ( import (
"bytes"
"context" "context"
"encoding/hex" "encoding/hex"
"errors" "errors"
@@ -94,47 +95,36 @@ func findManifest(fs afero.Fs, dir string) (string, error) {
return path, nil return path, nil
} }
// fetchManifestToTemp downloads a manifest URL to a temporary file and // downloadManifest downloads the manifest at url into memory. It refuses a
// returns the temp file path. The caller is responsible for removing it. // manifest larger than mfa.maxManifestSize.
func (mfa *CLIApp) fetchManifestToTemp( func (mfa *CLIApp) downloadManifest(ctx context.Context, url string) ([]byte, error) {
ctx context.Context, url string, rc, err := mfa.openManifestReader(ctx, url)
) (string, error) { if err != nil {
rc, fetchErr := mfa.openManifestReader(ctx, url) return nil, err
if fetchErr != nil {
return "", fetchErr
} }
tmpFile, tmpErr := afero.TempFile(mfa.Fs, "", "mfer-manifest-*.mf") defer func() { _ = rc.Close() }()
if tmpErr != nil {
_ = rc.Close()
return "", tmpErr // Reading stops one byte past mfa.maxManifestSize, which is enough to
}
tmpPath := tmpFile.Name()
// Copying stops one byte past mfa.maxManifestSize, which is enough to
// tell that the manifest is too large. // tell that the manifest is too large.
written, cpErr := io.Copy(tmpFile, io.LimitReader(rc, mfa.maxManifestSize+1)) manifestData, err := io.ReadAll(io.LimitReader(rc, mfa.maxManifestSize+1))
_ = rc.Close() if err != nil {
_ = tmpFile.Close() return nil, fmt.Errorf("download manifest: %w", err)
if cpErr == nil && written > mfa.maxManifestSize {
cpErr = fmt.Errorf("%w of %d bytes", errManifestTooLarge, mfa.maxManifestSize)
} }
if cpErr != nil { if int64(len(manifestData)) > mfa.maxManifestSize {
_ = mfa.Fs.Remove(tmpPath) return nil, fmt.Errorf("download manifest: %w of %d bytes",
errManifestTooLarge, mfa.maxManifestSize)
return "", fmt.Errorf("download manifest: %w", cpErr)
} }
return tmpPath, nil return manifestData, nil
} }
// verifyRequiredSigner enforces the --require-signature fingerprint // verifyRequiredSigner enforces the --require-signature fingerprint
// against the key that made the manifest's signature. // against signer, the fingerprint of the key that made the manifest's
func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { // 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 // Validate fingerprint format: must be exactly 40 hex characters
if len(requiredSigner) != fingerprintHexLen { if len(requiredSigner) != fingerprintHexLen {
return fmt.Errorf("%w, got %d", errInvalidFingerprint, len(requiredSigner)) 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) return fmt.Errorf("invalid fingerprint: %w", err)
} }
if !chk.IsSigned() { if signer == "" {
return fmt.Errorf("%w, but signature from %s is required", return fmt.Errorf("%w, but signature from %s is required",
errManifestNotSigned, requiredSigner) 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) // Compare fingerprints - must be exact match (case-insensitive)
if !strings.EqualFold(signer, requiredSigner) { if !strings.EqualFold(signer, requiredSigner) {
return fmt.Errorf("embedded signing key fingerprint %s %w %s", return fmt.Errorf("embedded signing key fingerprint %s %w %s",
@@ -299,32 +285,28 @@ func (mfa *CLIApp) checkManifestOperation(
return err 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) 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) { if isHTTPURL(manifestPath) {
tmpPath, tmpErr := mfa.fetchManifestToTemp(ctx, manifestPath) manifestData, downloadErr := mfa.downloadManifest(ctx, manifestPath)
if tmpErr != nil { if downloadErr != nil {
return tmpErr return downloadErr
} }
defer func() { _ = mfa.Fs.Remove(tmpPath) }() opts.ManifestReader = bytes.NewReader(manifestData)
} else {
manifestPath = tmpPath opts.ManifestPath = manifestPath
} }
showProgress := cmd.Bool("progress") showProgress := cmd.Bool("progress")
log.Infof("checking manifest %s with base %s", manifestPath, basePath) log.Infof("checking manifest %s with base %s", manifestPath, basePath)
// Create checker chk, err := mfer.NewChecker(opts)
chk, err := mfer.NewChecker(&mfer.CheckerOptions{
ManifestPath: manifestPath,
BasePath: basePath,
Fs: mfa.Fs,
})
if err != nil { if err != nil {
return fmt.Errorf("load manifest: %w", err) return fmt.Errorf("load manifest: %w", err)
} }
@@ -332,7 +314,7 @@ func (mfa *CLIApp) checkManifestOperation(
// Check signature requirement. An empty fingerprint is checked too, // Check signature requirement. An empty fingerprint is checked too,
// and refused as one of the wrong length. // and refused as one of the wrong length.
if cmd.IsSet(flagRequireSignature) { if cmd.IsSet(flagRequireSignature) {
err = verifyRequiredSigner(chk, cmd.String(flagRequireSignature)) err = verifyRequiredSigner(string(chk.Signer()), cmd.String(flagRequireSignature))
if err != nil { if err != nil {
return err return err
} }
+23
View File
@@ -1167,6 +1167,29 @@ func TestCheckURLManifestResolvesEntriesAgainstCurrentDirectory(t *testing.T) {
assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) 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. // unlistableDirFs is a filesystem on which one directory cannot be listed.
type unlistableDirFs struct { type unlistableDirFs struct {
afero.Fs afero.Fs
+6 -38
View File
@@ -50,38 +50,6 @@ func runLocked(fn func() error) error {
return fn() 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) { func TestNoManifestFoundMessage(t *testing.T) {
t.Parallel() t.Parallel()
@@ -97,7 +65,7 @@ func TestVerifyRequiredSignerMessages(t *testing.T) {
t.Run("invalid fingerprint length", func(t *testing.T) { t.Run("invalid fingerprint length", func(t *testing.T) {
t.Parallel() t.Parallel()
err := verifyRequiredSigner(unsignedChecker(t), "12345678") err := verifyRequiredSigner("", "12345678")
require.ErrorIs(t, err, errInvalidFingerprint) require.ErrorIs(t, err, errInvalidFingerprint)
assert.EqualError(t, err, assert.EqualError(t, err,
"invalid fingerprint: must be exactly 40 hex characters, got 8") "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.Run("manifest not signed", func(t *testing.T) {
t.Parallel() t.Parallel()
err := verifyRequiredSigner(unsignedChecker(t), msgFpA) err := verifyRequiredSigner("", msgFpA)
require.ErrorIs(t, err, errManifestNotSigned) require.ErrorIs(t, err, errManifestNotSigned)
assert.EqualError(t, err, assert.EqualError(t, err,
"manifest is not signed, but signature from "+msgFpA+" is required") "manifest is not signed, but signature from "+msgFpA+" is required")
@@ -121,13 +89,13 @@ func TestVerifyRequiredSignerMessages(t *testing.T) {
func TestSignerMismatchMessage(t *testing.T) { func TestSignerMismatchMessage(t *testing.T) {
t.Parallel() t.Parallel()
chk := signedChecker(t, signer := string(signedChecker(t,
signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})) signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})).Signer())
err := verifyRequiredSigner(chk, msgFpB) err := verifyRequiredSigner(signer, msgFpB)
require.ErrorIs(t, err, errSignerMismatch) require.ErrorIs(t, err, errSignerMismatch)
assert.EqualError(t, err, assert.EqualError(t, err,
"embedded signing key fingerprint "+string(chk.Signer())+ "embedded signing key fingerprint "+signer+
" does not match required "+msgFpB) " does not match required "+msgFpB)
} }
+1 -27
View File
@@ -19,7 +19,6 @@ import (
"github.com/dustin/go-humanize" "github.com/dustin/go-humanize"
"github.com/multiformats/go-multihash" "github.com/multiformats/go-multihash"
"github.com/spf13/afero"
"github.com/urfave/cli/v3" "github.com/urfave/cli/v3"
"sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/internal/log"
"sneak.berlin/go/mfer/mfer" "sneak.berlin/go/mfer/mfer"
@@ -475,7 +474,7 @@ func (mfa *CLIApp) fetchManifest(
} }
if requiredSigner != "" { if requiredSigner != "" {
err = verifyFetchedSigner(manifestData, requiredSigner) err = verifyRequiredSigner(string(manifest.Signer()), requiredSigner)
if err != nil { if err != nil {
return nil, nil, err return nil, nil, err
} }
@@ -553,31 +552,6 @@ func checkNoNameClash(files []*mfer.MFFilePath) error {
return nil 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 // saveManifest writes the fetched manifest into dest under the default
// manifest name, the way fetch writes every file: to a new temp file that // manifest name, the way fetch writes every file: to a new temp file that
// is then renamed into place. // is then renamed into place.
+4 -9
View File
@@ -489,18 +489,17 @@ func TestFetchStopsReadingFilePastListedSize(t *testing.T) {
// TestManifestDownloadStopsAtLimit serves a manifest that never ends to // TestManifestDownloadStopsAtLimit serves a manifest that never ends to
// fetch and to check, with the most they download of a manifest lowered // 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 // to 64 KiB. Each must stop reading at that limit and fail with an error
// naming it and leave no temp file. // naming it.
func TestManifestDownloadStopsAtLimit(t *testing.T) { func TestManifestDownloadStopsAtLimit(t *testing.T) {
t.Parallel()
server := httptest.NewServer( server := httptest.NewServer(
http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
_, _ = io.Copy(w, zeros{}) _, _ = io.Copy(w, zeros{})
})) }))
defer server.Close() defer server.Close()
tmpDir := t.TempDir()
t.Setenv("TMPDIR", tmpDir)
mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10} mfa := &CLIApp{Fs: afero.NewOsFs(), maxManifestSize: 64 << 10}
fetch := mfa.fetchCommand() fetch := mfa.fetchCommand()
@@ -520,10 +519,6 @@ func TestManifestDownloadStopsAtLimit(t *testing.T) {
"download manifest: file exceeds maximum allowed size of 65536 bytes", "download manifest: file exceeds maximum allowed size of 65536 bytes",
cmd.Name) cmd.Name)
} }
leftover, err := os.ReadDir(tmpDir)
require.NoError(t, err)
assert.Empty(t, leftover)
} }
//nolint:paralleltest // changes the process-global working directory //nolint:paralleltest // changes the process-global working directory
+35 -15
View File
@@ -80,7 +80,8 @@ type Checker struct {
// manifestPaths is a set of paths in the manifest for quick lookup // manifestPaths is a set of paths in the manifest for quick lookup
manifestPaths map[RelFilePath]struct{} manifestPaths map[RelFilePath]struct{}
// manifestInfo is the manifest file, which FindExtraFiles leaves out, // 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 manifestInfo os.FileInfo
// signature info from the manifest // signature info from the manifest
signature []byte signature []byte
@@ -90,8 +91,13 @@ type Checker struct {
// CheckerOptions configures a Checker. // CheckerOptions configures a Checker.
type CheckerOptions struct { 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 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 // BasePath is the directory relative to which manifest paths are
// resolved (required). // resolved (required).
BasePath string BasePath string
@@ -100,9 +106,10 @@ type CheckerOptions struct {
} }
// NewChecker creates a new Checker with the given options. It returns an // 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) { func NewChecker(opts *CheckerOptions) (*Checker, error) {
if opts == nil || opts.ManifestPath == "" { if opts == nil || (opts.ManifestPath == "" && opts.ManifestReader == nil) {
return nil, errManifestPathEmpty return nil, errManifestPathEmpty
} }
@@ -115,12 +122,30 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) {
fs = afero.NewOsFs() fs = afero.NewOsFs()
} }
m, err := NewManifestFromFile(&ManifestFromFileOptions{ var (
Path: opts.ManifestPath, m *manifest
Fs: fs, manifestInfo os.FileInfo
}) err error
if err != nil { )
return nil, err
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) abs, err := filepath.Abs(opts.BasePath)
@@ -135,11 +160,6 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) {
manifestPaths[RelFilePath(f.GetPath())] = struct{}{} manifestPaths[RelFilePath(f.GetPath())] = struct{}{}
} }
manifestInfo, err := fs.Stat(opts.ManifestPath)
if err != nil {
return nil, err
}
return &Checker{ return &Checker{
basePath: AbsFilePath(abs), basePath: AbsFilePath(abs),
files: files, files: files,
+32
View File
@@ -189,6 +189,38 @@ func TestNewCheckerNilFsUsesOsFs(t *testing.T) {
assert.Equal(t, StatusOK, (<-results).Status) 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) { func TestCheckerFileCountAndTotalBytes(t *testing.T) {
t.Parallel() t.Parallel()