diff --git a/README.md b/README.md index 9d86d7f..a826bf2 100644 --- a/README.md +++ b/README.md @@ -43,9 +43,11 @@ bin/mfer gen . # it lists is missing or corrupted; warns about files it does not list. bin/mfer check index.mf -# Download and cryptographically verify a tree published over HTTP: mfer -# fetches /index.mf, then downloads every file it lists. -bin/mfer fetch https://example.com/tree/ +# Download and cryptographically verify a tree published over HTTP into +# ./mirror: mfer fetches /index.mf, downloads every file it lists, +# skipping any already there with the right hash, then saves the manifest as +# mirror/index.mf. +bin/mfer fetch --dest mirror https://example.com/tree/ ``` Run `bin/mfer help` for the full command list, or `bin/mfer --help` @@ -258,9 +260,18 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues). list, hidden files included; with `--no-extra-files` each one is a failure instead - `mfer fetch https://example.com/stuff/` - - fetches `/stuff/index.mf` and downloads all files listed in manifest, - optionally resuming any that already exist locally, and assures - cryptographic integrity of downloaded files. + - fetches `/stuff/index.mf` and downloads all files listed in manifest into + the current directory, or the one given with `--dest`, and assures + cryptographic integrity of downloaded files. A file already there with the + size and hash the manifest lists is skipped. Once every file is in place, + the manifest is saved there as `index.mf`, so `mfer check` can verify the + tree later. A manifest that lists `index.mf` (in any letter case) or + `.index.mf.tmp` at the top of the tree is refused before any file is + downloaded, since saving the manifest would replace it. +- `mfer fetch --require-signature https://example.com/stuff/` + - as above, but first refuses a manifest not signed by the key with that + fingerprint, as `mfer check --require-signature` does, before downloading + any file. # Implementation Plan diff --git a/internal/cli/check.go b/internal/cli/check.go index acb99de..c146dfd 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -319,7 +319,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { } // Check signature requirement - requiredSigner := ctx.String("require-signature") + requiredSigner := ctx.String(flagRequireSignature) if requiredSigner != "" { err = verifyRequiredSigner(ctx.Context, chk, requiredSigner) if err != nil { diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 1f37556..6c642af 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -111,9 +111,10 @@ func TestVerifyRequiredSignerMessages(t *testing.T) { // string; the required signer is a fixed value that cannot match it. Requires // gpg and is skipped where it is absent, as the other signing tests are. // -//nolint:paralleltest // signedChecker calls t.Setenv, which bars t.Parallel +//nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel func TestSignerMismatchMessage(t *testing.T) { - chk := signedChecker(t) + chk := signedChecker(t, + signedManifest(t, map[string][]byte{"f.txt": []byte("signed file")})) embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(context.Background()) require.NoError(t, err) @@ -125,9 +126,10 @@ func TestSignerMismatchMessage(t *testing.T) { " does not match required "+msgFpB) } -// signedChecker builds a Checker over a manifest signed by a throwaway GPG -// key generated in a temporary GNUPGHOME. -func signedChecker(t *testing.T) *mfer.Checker { +// signedManifest returns a manifest of files signed by a throwaway GPG key +// generated in a temporary GNUPGHOME, which it leaves set for the rest of +// the test. +func signedManifest(t *testing.T, files map[string][]byte) []byte { t.Helper() _, err := exec.LookPath("gpg") @@ -159,17 +161,25 @@ func signedChecker(t *testing.T) *mfer.Checker { b := mfer.NewBuilder() b.SetSigningOptions(&mfer.SigningOptions{KeyID: mfer.GPGKeyID("test@mfer.test")}) - content := []byte("signed file") - _, err = b.AddFile("f.txt", mfer.FileSize(len(content)), mfer.ModTime{}, - bytes.NewReader(content), nil) - require.NoError(t, err) + for path, content := range files { + _, err = b.AddFile(mfer.RelFilePath(path), mfer.FileSize(len(content)), + mfer.ModTime{}, bytes.NewReader(content), nil) + require.NoError(t, err) + } var buf bytes.Buffer require.NoError(t, b.Build(context.Background(), &buf)) + return buf.Bytes() +} + +// signedChecker builds a Checker over manifest, a signed manifest. +func signedChecker(t *testing.T, manifest []byte) *mfer.Checker { + t.Helper() + fs := afero.NewMemMapFs() - require.NoError(t, afero.WriteFile(fs, "/index.mf", buf.Bytes(), 0o644)) + require.NoError(t, afero.WriteFile(fs, "/index.mf", manifest, 0o644)) chk, err := mfer.NewChecker(&mfer.CheckerOptions{ ManifestPath: "/index.mf", @@ -300,7 +310,7 @@ func TestFetchFileHTTPStatusMessage(t *testing.T) { // downloadFile logs each retry of the 500 to the process-global logger. err := runLocked(func() error { - return downloadFile(context.Background(), testClient(), server.URL+"/x", "x", + return downloadFile(context.Background(), testClient(), server.URL+"/x", ".", "x", &mfer.MFFilePath{}, nil) }) require.ErrorIs(t, err, errHTTPStatus) @@ -355,7 +365,7 @@ func TestSizeMismatchMessage(t *testing.T) { // finishDownload returns the size-mismatch error before it touches the // paths, digest, or entry, so those can be zero here. - err := finishDownload("", "", 9, 10, nil, nil, nil, nil) + err := finishDownload("", "", "", 9, 10, nil, nil, nil, nil) require.ErrorIs(t, err, errSizeMismatch) assert.EqualError(t, err, "size mismatch: expected 10 bytes, got 9") } diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 7ebcae9..2b61c7f 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -19,6 +19,7 @@ import ( "github.com/dustin/go-humanize" "github.com/multiformats/go-multihash" + "github.com/spf13/afero" "github.com/urfave/cli/v2" "sneak.berlin/go/mfer/internal/log" "sneak.berlin/go/mfer/mfer" @@ -90,6 +91,10 @@ var ( // errHashMismatch indicates a downloaded file whose hash matches no // manifest hash. errHashMismatch = errors.New("hash mismatch") + // errManifestNameListed indicates a manifest that lists a file where + // fetch saves the manifest. + errManifestNameListed = errors.New( + "manifest lists a file where fetch saves the manifest") ) // DownloadProgress reports the progress of a single file download. @@ -239,20 +244,34 @@ func manifestBaseURL(manifestURL string) (*url.URL, error) { return parsed.JoinPath(".."), nil } -// downloadManifestFiles downloads every file in the manifest, reporting -// progress on the progress channel. +// downloadManifestFiles downloads every file in the manifest into dest, +// reporting progress on the progress channel. A file already present in +// dest is skipped. It returns how many files it downloaded and their +// total size. func downloadManifestFiles( ctx context.Context, client retryingClient, baseURL *url.URL, + dest string, files []*mfer.MFFilePath, progress chan<- DownloadProgress, -) error { +) (int, int64, error) { + var ( + downloaded int + downloadedBytes int64 + ) + for _, f := range files { // Sanitize the path to prevent path traversal attacks localPath, err := sanitizePath(f.GetPath()) if err != nil { - return fmt.Errorf("invalid path in manifest: %w", err) + return 0, 0, fmt.Errorf("invalid path in manifest: %w", err) + } + + if alreadyPresent(dest, localPath, f) { + log.Infof("skipping %s: already present", f.GetPath()) + + continue } // JoinPath takes escaped path text, so a name such as "100%.txt" @@ -260,13 +279,53 @@ func downloadManifestFiles( fileURL := baseURL.JoinPath(encodeFilePath(f.GetPath())).String() log.Infof("fetching %s", f.GetPath()) - err = downloadFile(ctx, client, fileURL, localPath, f, progress) + err = downloadFile(ctx, client, fileURL, dest, localPath, f, progress) if err != nil { - return fmt.Errorf("failed to download %s: %w", f.GetPath(), err) + return 0, 0, fmt.Errorf("failed to download %s: %w", f.GetPath(), err) } + + downloaded++ + downloadedBytes += f.GetSize() } - return nil + return downloaded, downloadedBytes, nil +} + +// alreadyPresent reports whether localPath under dest is a regular file +// with the size and one of the hashes the manifest lists for entry. It +// hashes the whole file, since a matching size alone would accept a +// corrupted or partly written one. A file it cannot read, or reaches only +// through a symlink, is not present: fetch downloads it, and the download +// reports the problem. +func alreadyPresent(dest, localPath string, entry *mfer.MFFilePath) bool { + if checkNoSymlinks(dest, localPath) != nil { + return false + } + + path := filepath.Join(dest, localPath) + + info, err := os.Lstat(path) + if err != nil || !info.Mode().IsRegular() || info.Size() != entry.GetSize() { + return false + } + + // G304: localPath is a relative path that sanitizePath keeps inside + // dest as text, and checkNoSymlinks just found no symlink in it. + f, err := os.Open(path) //nolint:gosec // G304: see comment above + if err != nil { + return false + } + + defer func() { _ = f.Close() }() + + h := sha256.New() + + _, err = io.Copy(h, f) + if err != nil { + return false + } + + return verifyDownloadedHash(h.Sum(nil), entry) == nil } func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { @@ -291,43 +350,22 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { firstDelay: firstRetryDelay, } - log.Infof("fetching manifest from %s", manifestURL) - - // Read the whole manifest before parsing it, so that a connection - // lost partway through is retried rather than reported as a bad - // manifest. - var manifestData []byte - - err = client.get(ctx.Context, manifestURL, func(resp *http.Response) error { - var readErr error - - manifestData, readErr = io.ReadAll(resp.Body) - - return readErr - }) + manifestData, files, err := fetchManifest(ctx, client, manifestURL) if err != nil { - return fmt.Errorf("failed to fetch manifest: %w", err) + return err } - // Parse manifest - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) - if err != nil { - return fmt.Errorf("failed to parse manifest: %w", err) - } - - files := manifest.Files() - log.Infof("manifest contains %d files", len(files)) - // Compute base URL (directory containing manifest) baseURL, err := manifestBaseURL(manifestURL) if err != nil { return err } - // Calculate total bytes to download - var totalBytes int64 - for _, f := range files { - totalBytes += f.GetSize() + dest := ctx.String(flagDest) + + err = os.MkdirAll(dest, dirPerms) + if err != nil { + return fmt.Errorf("failed to create destination directory %s: %w", dest, err) } // Create progress channel and start progress reporter goroutine @@ -340,7 +378,8 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { startTime := time.Now() // Download each file - dlErr := downloadManifestFiles(ctx.Context, client, baseURL, files, progress) + downloaded, downloadedBytes, dlErr := downloadManifestFiles( + ctx.Context, client, baseURL, dest, files, progress) close(progress) <-done @@ -349,15 +388,150 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { return dlErr } + // Saved only now that every file is in place and verified, so that + // "mfer check" can verify the tree later. + err = saveManifest(dest, manifestData) + if err != nil { + return fmt.Errorf("failed to save manifest: %w", err) + } + // Print summary elapsed := time.Since(startTime) - avgBytesPerSec := float64(totalBytes) / elapsed.Seconds() + avgBytesPerSec := float64(downloadedBytes) / elapsed.Seconds() avgRate := formatBitrate(avgBytesPerSec * bitsPerByte) - log.Infof("downloaded %d files (%s) in %.1fs (%s avg)", - len(files), - humanize.IBytes(safeUint64(totalBytes)), + log.Infof("downloaded %d files (%s) in %.1fs (%s avg), skipped %d already present", + downloaded, + humanize.IBytes(safeUint64(downloadedBytes)), elapsed.Seconds(), - avgRate) + avgRate, + len(files)-downloaded) + log.Infof("saved manifest to %s", filepath.Join(dest, defaultManifestName)) + + return nil +} + +// fetchManifest downloads the manifest at manifestURL and parses it, +// enforcing --require-signature if it is given and refusing a manifest +// that lists a file where it will be saved. It returns the manifest as +// downloaded, to be saved once the files are in place, and the files it +// lists. +func fetchManifest( + ctx *cli.Context, client retryingClient, manifestURL string, +) ([]byte, []*mfer.MFFilePath, error) { + log.Infof("fetching manifest from %s", manifestURL) + + // Read the whole manifest before parsing it, so that a connection + // lost partway through is retried rather than reported as a bad + // manifest. + var manifestData []byte + + err := client.get(ctx.Context, manifestURL, func(resp *http.Response) error { + var readErr error + + manifestData, readErr = io.ReadAll(resp.Body) + + return readErr + }) + if err != nil { + return nil, nil, fmt.Errorf("failed to fetch manifest: %w", err) + } + + // Parse manifest + manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + if err != nil { + return nil, nil, fmt.Errorf("failed to parse manifest: %w", err) + } + + requiredSigner := ctx.String(flagRequireSignature) + if requiredSigner != "" { + err = verifyFetchedSigner(ctx, manifestData, requiredSigner) + if err != nil { + return nil, nil, err + } + } + + files := manifest.Files() + + err = checkManifestNameUnlisted(files) + if err != nil { + return nil, nil, err + } + + log.Infof("manifest contains %d files", len(files)) + + return manifestData, files, nil +} + +// checkManifestNameUnlisted returns an error if files lists a file or +// directory at the top of the tree under the name fetch saves the +// manifest as, or under that name's temp file. Saving the manifest would +// replace or remove it, or fail once every file was downloaded, leaving a +// tree check rejects. Names are compared ignoring case, since on a +// case-insensitive filesystem INDEX.MF and index.mf are one file. +func checkManifestNameUnlisted(files []*mfer.MFFilePath) error { + for _, f := range files { + top, _, _ := strings.Cut(filepath.Clean(f.GetPath()), string(filepath.Separator)) + + if strings.EqualFold(top, defaultManifestName) || + strings.EqualFold(top, tempPathFor(defaultManifestName)) { + return fmt.Errorf("%w: %s", errManifestNameListed, f.GetPath()) + } + } + + 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( + ctx *cli.Context, 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("failed to load manifest: %w", err) + } + + return verifyRequiredSigner(ctx.Context, 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. +func saveManifest(dest string, manifestData []byte) error { + tmpPath := tempPathFor(defaultManifestName) + + out, err := createTempFile(dest, tmpPath) + if err != nil { + return err + } + + _, writeErr := out.Write(manifestData) + closeErr := out.Close() + + err = errors.Join(writeErr, closeErr) + if err == nil { + err = moveIntoPlace(dest, tmpPath, defaultManifestName) + } + + if err != nil { + _ = os.Remove(filepath.Join(dest, tmpPath)) + + return err + } return nil } @@ -401,14 +575,15 @@ func sanitizePath(p string) (string, error) { return cleaned, nil } -// checkNoSymlinks returns an error if any part of the relative path p -// already exists as a symlink. sanitizePath checks p only as text, so -// without this a symlink inside the target directory could send a write -// to p outside of it. Parts that do not exist yet are fine: fetch creates -// them as plain directories and files. Call it immediately before each -// write: a symlink created after it returns is not caught. -func checkNoSymlinks(p string) error { - current := "" +// checkNoSymlinks returns an error if any part of p, a path relative to +// dest, already exists under dest as a symlink. dest itself is the user's +// choice and may be one. sanitizePath checks p only as text, so without +// this a symlink inside dest could send a write to p outside of it. Parts +// that do not exist yet are fine: fetch creates them as plain directories +// and files. Call it immediately before each write: a symlink created +// after it returns is not caught. +func checkNoSymlinks(dest, p string) error { + current := dest for _, part := range strings.Split(p, string(filepath.Separator)) { current = filepath.Join(current, part) @@ -548,13 +723,14 @@ func verifyDownloadedHash(digest []byte, entry *mfer.MFFilePath) error { return errHashMismatch } -// downloadFile downloads a URL to a local file path with hash verification. -// It downloads to a temporary file, verifies the hash, then renames to the final path. -// Progress is reported via the progress channel. +// downloadFile downloads a URL to localPath, a path relative to dest, with +// hash verification. It downloads to a temporary file, verifies the hash, +// then renames to the final path. Progress is reported via the progress +// channel. func downloadFile( ctx context.Context, client retryingClient, - fileURL, localPath string, + fileURL, dest, localPath string, entry *mfer.MFFilePath, progress chan<- DownloadProgress, ) error { @@ -568,11 +744,13 @@ func downloadFile( // Create parent directories if needed dir := filepath.Dir(localPath) if dir != "" && dir != "." { - err = checkNoSymlinks(dir) + err = checkNoSymlinks(dest, dir) if err != nil { return err } + dir = filepath.Join(dest, dir) + err = os.MkdirAll(dir, dirPerms) if err != nil { return fmt.Errorf("failed to create directory %s: %w", dir, err) @@ -582,17 +760,61 @@ func downloadFile( tmpPath := tempPathFor(localPath) return client.get(ctx, fileURL, func(resp *http.Response) error { - return saveResponse(resp, tmpPath, localPath, entry, progress) + return saveResponse(resp, dest, tmpPath, localPath, entry, progress) }) } +// createTempFile creates tmpPath, a path relative to dest, as a new empty +// file. +func createTempFile(dest, tmpPath string) (*os.File, error) { + err := checkNoSymlinks(dest, tmpPath) + if err != nil { + return nil, err + } + + path := filepath.Join(dest, tmpPath) + + // Remove whatever is at tmpPath, such as a leftover from an + // interrupted run, rather than write into it: it may be a hard link + // to a file outside dest, and removing a hard link removes only this + // name. If the removal fails, O_EXCL below makes the create fail. + _ = os.Remove(path) + + // Create the temp file only if nothing is at tmpPath (O_EXCL). + // + // G304: tmpPath is a relative path that sanitizePath keeps inside dest + // as text, and checkNoSymlinks just found no symlink in it. + out, err := os.OpenFile( //nolint:gosec // G304: see comment above + path, os.O_RDWR|os.O_CREATE|os.O_EXCL, filePerms) + if err != nil { + return nil, fmt.Errorf("failed to create temp file: %w", err) + } + + return out, nil +} + +// moveIntoPlace renames tmpPath to localPath, both relative to dest. +func moveIntoPlace(dest, tmpPath, localPath string) error { + err := checkNoSymlinks(dest, localPath) + if err != nil { + return err + } + + err = os.Rename(filepath.Join(dest, tmpPath), filepath.Join(dest, localPath)) + if err != nil { + return fmt.Errorf("failed to rename temp file: %w", err) + } + + return nil +} + // saveResponse writes resp's body to tmpPath, verifies it against entry, -// and renames it to localPath. It starts a new temp file each time and -// removes it on failure, so a retry after a failed try never appends to -// or keeps a partial file. +// and renames it to localPath, both paths relative to dest. It starts a +// new temp file each time and removes it on failure, so a retry after a +// failed try never appends to or keeps a partial file. func saveResponse( resp *http.Response, - tmpPath, localPath string, + dest, tmpPath, localPath string, entry *mfer.MFFilePath, progress chan<- DownloadProgress, ) error { @@ -604,29 +826,11 @@ func saveResponse( totalBytes = expectedSize } - err := checkNoSymlinks(tmpPath) + out, err := createTempFile(dest, tmpPath) if err != nil { return err } - // Remove whatever is at tmpPath, such as a leftover from an - // interrupted run, rather than write into it: it may be a hard link - // to a file outside the target directory, and removing a hard link - // removes only this name. If the removal fails, O_EXCL below makes - // the create fail. - _ = os.Remove(tmpPath) - - // Create the temp file only if nothing is at tmpPath (O_EXCL). - // - // G304: tmpPath is a relative path that sanitizePath keeps inside the - // target directory as text, and checkNoSymlinks just found no symlink - // in it. - out, err := os.OpenFile( //nolint:gosec // G304: see comment above - tmpPath, os.O_RDWR|os.O_CREATE|os.O_EXCL, filePerms) - if err != nil { - return fmt.Errorf("failed to create temp file: %w", err) - } - // Set up hash computation h := sha256.New() @@ -646,10 +850,10 @@ func saveResponse( closeErr := out.Close() err = finishDownload( - tmpPath, localPath, written, expectedSize, h.Sum(nil), entry, + dest, tmpPath, localPath, written, expectedSize, h.Sum(nil), entry, copyErr, closeErr) if err != nil { - _ = os.Remove(tmpPath) + _ = os.Remove(filepath.Join(dest, tmpPath)) return err } @@ -660,7 +864,7 @@ func saveResponse( // finishDownload validates the copy result, verifies size and hash, and // moves the temp file into place. On error the caller removes tmpPath. func finishDownload( - tmpPath, localPath string, + dest, tmpPath, localPath string, written, expectedSize int64, digest []byte, entry *mfer.MFFilePath, @@ -686,16 +890,5 @@ func finishDownload( return err } - err = checkNoSymlinks(localPath) - if err != nil { - return err - } - - // Rename temp file to final path - err = os.Rename(tmpPath, localPath) - if err != nil { - return fmt.Errorf("failed to rename temp file: %w", err) - } - - return nil + return moveIntoPlace(dest, tmpPath, localPath) } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 0a0bc5f..d9e0a20 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -13,6 +13,7 @@ import ( "net/http/httptest" "os" "path/filepath" + "slices" "strconv" "sync" "sync/atomic" @@ -338,7 +339,7 @@ func TestFetchFromHTTP(t *testing.T) { fileURL := baseURL + f.GetPath() err = downloadFile(context.Background(), testClient(), - fileURL, localPath, f, progress) + fileURL, ".", localPath, f, progress) require.NoError(t, err, "failed to download %s", f.GetPath()) } @@ -386,7 +387,7 @@ func TestFetchHashMismatch(t *testing.T) { // Try to download - should fail with hash mismatch err = downloadFile(context.Background(), testClient(), - server.URL+"/file.txt", testFileTxt, files[0], nil) + server.URL+"/file.txt", ".", testFileTxt, files[0], nil) require.Error(t, err) assert.Contains(t, err.Error(), "mismatch") @@ -432,7 +433,7 @@ func TestFetchSizeMismatch(t *testing.T) { // Try to download - should fail with size mismatch err = downloadFile(context.Background(), testClient(), - server.URL+"/file.txt", testFileTxt, files[0], nil) + server.URL+"/file.txt", ".", testFileTxt, files[0], nil) require.Error(t, err) assert.Contains(t, err.Error(), "size mismatch") @@ -490,7 +491,7 @@ func TestFetchProgress(t *testing.T) { // Download err = downloadFile(context.Background(), testClient(), - server.URL+"/large.txt", "large.txt", files[0], progress) + server.URL+"/large.txt", ".", "large.txt", files[0], progress) close(progress) <-done @@ -513,24 +514,51 @@ func TestFetchProgress(t *testing.T) { assert.Equal(t, content, downloaded) } -// TestFetchRefusesSymlinks runs fetch into a destination directory that -// holds a symlink pointing outside it, in each of the three places fetch -// writes: a parent directory, the temp file, and the file itself, which -// the temp file is renamed onto; and once as a directory inside a plain -// directory. The fetch must fail and nothing outside may change. +// TestFetchRefusesSymlinks runs fetch with --dest naming a directory other +// than the current one, which holds a symlink pointing outside it, in each +// place fetch writes: a parent directory, the temp file, the file itself, +// which the temp file is renamed onto, and the saved manifest's temp file +// and final name; and once as a directory inside a plain directory. The +// fetch must fail, and neither the outside directory nor the current one +// may change. // //nolint:paralleltest // changes the process-global working directory func TestFetchRefusesSymlinks(t *testing.T) { + // What a link standing for a file points to: a file outside that does + // not exist yet. + const newFile = "new.txt" + tests := []struct { - name string - entry string // the manifest's only file - link string // symlink placed in the destination directory - target string // what link points to, relative to the outside directory + name string + entry string // the manifest's only file + link string // symlink placed in the destination directory + target string // what link points to, relative to the outside directory + failure string // what fetch reports it was doing when it found link }{ - {"parent directory", "sub/deeper/file.txt", "sub", "."}, - {"directory inside a plain directory", "docs/data/passwd", "docs/data", "."}, - {"temp file", testFileTxt, ".file.txt.tmp", "new.txt"}, - {"file", testFileTxt, testFileTxt, "new.txt"}, + { + "parent directory", "sub/deeper/file.txt", "sub", ".", + "failed to download sub/deeper/file.txt", + }, + { + "directory inside a plain directory", "docs/data/passwd", "docs/data", ".", + "failed to download docs/data/passwd", + }, + { + "temp file", testFileTxt, ".file.txt.tmp", newFile, + "failed to download " + testFileTxt, + }, + { + "file", testFileTxt, testFileTxt, newFile, + "failed to download " + testFileTxt, + }, + { + "manifest temp file", testFileTxt, tempPathFor(defaultManifestName), newFile, + "failed to save manifest", + }, + { + "manifest", testFileTxt, defaultManifestName, newFile, + "failed to save manifest", + }, } for _, tt := range tests { @@ -545,23 +573,61 @@ func TestFetchRefusesSymlinks(t *testing.T) { defer server.Close() outside := t.TempDir() + cwd := chdirTemp(t) + dest := t.TempDir() + link := filepath.Join(dest, tt.link) - chdirTemp(t) - require.NoError(t, os.MkdirAll(filepath.Dir(tt.link), 0o750)) - require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), tt.link)) + require.NoError(t, os.MkdirAll(filepath.Dir(link), 0o750)) + require.NoError(t, os.Symlink(filepath.Join(outside, tt.target), link)) - opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs()) + opts := testOpts([]string{ + testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL, + }, afero.NewOsFs()) assert.Equal(t, 1, runCLI(opts)) - assert.Contains(t, testStderr(t, opts), "failed to download "+tt.entry+ - ": symlink in path not allowed: "+tt.link) + assert.Contains(t, testStderr(t, opts), + tt.failure+": symlink in path not allowed: "+link) written, err := os.ReadDir(outside) require.NoError(t, err) assert.Empty(t, written, "fetch wrote outside the destination") + + written, err = os.ReadDir(cwd) + require.NoError(t, err) + assert.Empty(t, written, "fetch wrote to the current directory") }) } } +// TestFetchDoesNotSkipThroughSymlink runs fetch with --dest holding a +// symlink to a directory outside it, where the file the manifest lists +// through that symlink already sits with the listed content. fetch must +// not take that file as already present: it must fail on the symlink, as +// the download would, and leave the outside file alone. +func TestFetchDoesNotSkipThroughSymlink(t *testing.T) { + t.Parallel() + + content := []byte("fetched") + files := map[string][]byte{"sub/" + testFileTxt: content} + + server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files)) + defer server.Close() + + outside := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(outside, testFileTxt), content, 0o600)) + + dest := t.TempDir() + link := filepath.Join(dest, "sub") + require.NoError(t, os.Symlink(outside, link)) + + opts := testOpts([]string{ + testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL, + }, afero.NewOsFs()) + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), + "failed to download sub/"+testFileTxt+": symlink in path not allowed: "+link) + assert.Equal(t, map[string][]byte{testFileTxt: content}, filesUnder(t, outside)) +} + // TestFetchReplacesHardLinkAtTempName runs fetch into a destination // directory that holds, at the temp file's name, a hard link to a file // outside it. To fetch that is an ordinary leftover from an interrupted @@ -787,7 +853,7 @@ func TestDownloadFileRetriesToSuccess(t *testing.T) { chdirTemp(t) err = downloadFile(context.Background(), testClient(), - server.URL+"/"+testFileTxt, testFileTxt, manifest.Files()[0], nil) + server.URL+"/"+testFileTxt, ".", testFileTxt, manifest.Files()[0], nil) require.NoError(t, err) assert.Equal(t, int32(3), requests.Load()) @@ -878,7 +944,7 @@ func TestFetchEscapedPaths(t *testing.T) { // TestFetchTree runs fetch on a tree with nested directories. Every file // the manifest lists must land under its own path with its own content, -// and nothing else may be left in the destination. +// beside the manifest, and nothing else may be left in the destination. // //nolint:paralleltest // changes the process-global working directory func TestFetchTree(t *testing.T) { @@ -889,7 +955,9 @@ func TestFetchTree(t *testing.T) { "other/deep/est.txt": []byte("in a second directory"), } - server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files)) + manifest := manifestOf(t, files) + + server := httptest.NewServer(fetchTestHandler(manifest, files)) defer server.Close() dest := chdirTemp(t) @@ -897,7 +965,9 @@ func TestFetchTree(t *testing.T) { opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs()) require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) - assert.Equal(t, files, filesUnder(t, dest)) + want := maps.Clone(files) + want[defaultManifestName] = manifest + assert.Equal(t, want, filesUnder(t, dest)) } // TestFetchFailsOnHashMismatch runs fetch against a server that serves a @@ -922,9 +992,10 @@ func TestFetchFailsOnHashMismatch(t *testing.T) { // TestFetchIntoPartlyFilledDestination runs fetch where an interrupted // fetch of an older version of the tree left one file current, one file // out of date and one half written to its temp file, beside a file the -// manifest does not list. fetch downloads every file the manifest lists, -// those already present included, and replaces what is there; the file -// the manifest does not list is left alone. +// manifest does not list. fetch skips the current file and downloads the +// other two. The out-of-date file has the same size as the new version, +// so only its hash shows that it must be replaced. The file the manifest +// does not list is left alone, and the manifest is saved beside the files. // //nolint:paralleltest // changes the process-global working directory func TestFetchIntoPartlyFilledDestination(t *testing.T) { @@ -935,7 +1006,8 @@ func TestFetchIntoPartlyFilledDestination(t *testing.T) { } unlisted := []byte("not in the manifest") - tree := fetchTestHandler(manifestOf(t, files), files) + manifest := manifestOf(t, files) + tree := fetchTestHandler(manifest, files) var ( mu sync.Mutex @@ -963,21 +1035,208 @@ func TestFetchIntoPartlyFilledDestination(t *testing.T) { os.WriteFile(tempPathFor("sub/partial.txt"), []byte("cut off"), 0o600)) require.NoError(t, os.WriteFile("unlisted.txt", unlisted, 0o600)) - opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs()) + opts := testOpts([]string{testApp, cmdFetch, server.URL}, afero.NewOsFs()) require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + assert.Contains(t, testStderr(t, opts), "skipping current.txt: already present") want := maps.Clone(files) want["unlisted.txt"] = unlisted + want[defaultManifestName] = manifest assert.Equal(t, want, filesUnder(t, dest)) mu.Lock() defer mu.Unlock() assert.ElementsMatch(t, []string{ - "/" + defaultManifestName, "/current.txt", "/sub/changed.txt", "/sub/partial.txt", + "/" + defaultManifestName, "/sub/changed.txt", "/sub/partial.txt", }, requested) } +// TestFetchIntoDest fetches a tree with --dest into a directory that does +// not exist yet. The files and the manifest must land there and nowhere +// else, and check must pass on the result with no extra files. A second +// fetch into the same directory must download nothing but the manifest. +// +//nolint:paralleltest // changes the process-global working directory +func TestFetchIntoDest(t *testing.T) { + files := map[string][]byte{ + "top.txt": []byte("at the top"), + "sub/one.txt": []byte("one level down"), + } + + manifest := manifestOf(t, files) + tree := fetchTestHandler(manifest, files) + + var ( + mu sync.Mutex + requested []string + ) + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + + requested = append(requested, r.URL.Path) + + mu.Unlock() + + tree.ServeHTTP(w, r) + })) + defer server.Close() + + cwd := chdirTemp(t) + dest := filepath.Join(t.TempDir(), "mirror") + fetch := []string{testApp, cmdFetch, "-q", "--" + flagDest, dest, server.URL} + + opts := testOpts(fetch, afero.NewOsFs()) + require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + + want := maps.Clone(files) + want[defaultManifestName] = manifest + assert.Equal(t, want, filesUnder(t, dest)) + assert.Empty(t, filesUnder(t, cwd), "fetch wrote outside --dest") + + check := testOpts([]string{ + testApp, cmdCheck, "-q", testFlagBase, dest, testFlagNoExtra, + filepath.Join(dest, defaultManifestName), + }, afero.NewOsFs()) + require.Equal(t, 0, runCLI(check), testStderr(t, check)) + + mu.Lock() + requested = nil + mu.Unlock() + + opts = testOpts(fetch, afero.NewOsFs()) + require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + assert.Equal(t, want, filesUnder(t, dest)) + + mu.Lock() + defer mu.Unlock() + + assert.Equal(t, []string{"/" + defaultManifestName}, requested) +} + +// TestFetchRequireSignature runs fetch with --require-signature. A +// manifest that is unsigned, or signed by another key, must stop fetch +// with check's message before it downloads or writes anything; the +// required key lets it through. The signed cases need gpg and are skipped +// without it, as the other signing tests are. +// +//nolint:paralleltest // signedManifest calls t.Setenv, which bars t.Parallel +func TestFetchRequireSignature(t *testing.T) { + files := map[string][]byte{testFileTxt: []byte("signed file")} + + t.Run("unsigned", func(t *testing.T) { + assertFetchRefused(t, manifestOf(t, files), files, + "manifest is not signed, but signature from "+msgFpA+" is required", + "--"+flagRequireSignature, msgFpA) + }) + + t.Run("signed", func(t *testing.T) { + manifest := signedManifest(t, files) + + signer, err := signedChecker(t, manifest). + ExtractEmbeddedSigningKeyFP(context.Background()) + require.NoError(t, err) + + assertFetchRefused(t, manifest, files, + "embedded signing key fingerprint "+signer+" does not match required "+msgFpB, + "--"+flagRequireSignature, msgFpB) + + server := httptest.NewServer(fetchTestHandler(manifest, files)) + defer server.Close() + + dest := t.TempDir() + + opts := testOpts([]string{ + testApp, cmdFetch, "-q", "--" + flagDest, dest, + "--" + flagRequireSignature, signer, server.URL, + }, afero.NewOsFs()) + require.Equal(t, 0, runCLI(opts), testStderr(t, opts)) + assert.Equal(t, files[testFileTxt], filesUnder(t, dest)[testFileTxt]) + }) +} + +// TestFetchRefusesListedManifestName fetches manifests that list, at the +// top of the tree, the name fetch saves the manifest under or that name's +// temp file: as a file, as a directory, in capitals, and with a leading +// "./". Saving the manifest would replace or remove what is listed there, +// so fetch must refuse the manifest before it creates the destination or +// requests any file. +func TestFetchRefusesListedManifestName(t *testing.T) { + t.Parallel() + + for _, listed := range []string{ + defaultManifestName, + tempPathFor(defaultManifestName), + defaultManifestName + "/" + testFileTxt, + "INDEX.MF", + "./" + defaultManifestName, + } { + t.Run(listed, func(t *testing.T) { + t.Parallel() + + // Built directly rather than scanned, since a scan lists no + // hidden files and never a path starting with "./". + content := []byte("listed") + builder := mfer.NewBuilder() + _, err := builder.AddFile(mfer.RelFilePath(listed), mfer.FileSize(len(content)), + mfer.ModTime(time.Now()), bytes.NewReader(content), nil) + require.NoError(t, err) + + var manifest bytes.Buffer + require.NoError(t, builder.Build(context.Background(), &manifest)) + + assertFetchRefused(t, manifest.Bytes(), map[string][]byte{listed: content}, + "manifest lists a file where fetch saves the manifest: "+listed) + }) + } +} + +// assertFetchRefused serves manifest, a manifest of files, and fetches it +// with flags into a directory that does not exist yet. fetch must fail +// with message after requesting only the manifest, and must not create +// the directory. +func assertFetchRefused( + t *testing.T, manifest []byte, files map[string][]byte, + message string, flags ...string, +) { + t.Helper() + + tree := fetchTestHandler(manifest, files) + + var ( + mu sync.Mutex + requested []string + ) + + server := httptest.NewServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + + requested = append(requested, r.URL.Path) + + mu.Unlock() + + tree.ServeHTTP(w, r) + })) + defer server.Close() + + dest := filepath.Join(t.TempDir(), "mirror") + + opts := testOpts(slices.Concat( + []string{testApp, cmdFetch, "-q", "--" + flagDest, dest}, flags, []string{server.URL}, + ), afero.NewOsFs()) + assert.Equal(t, 1, runCLI(opts)) + assert.Contains(t, testStderr(t, opts), message) + assert.NoDirExists(t, dest, "fetch created the destination before refusing") + + mu.Lock() + defer mu.Unlock() + + assert.Equal(t, []string{"/" + defaultManifestName}, requested) +} + // TestFetchTimeoutFlag runs fetch with --timeout against a server that // never answers. Without the flag's limit the request would wait forever; // once fetch gives up on it, the server cancels fetch's context so that diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index fe848b4..86516d3 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -22,8 +22,10 @@ const ( cmdFetch = "fetch" cmdVersion = "version" - flagProgress = "progress" - flagTimeout = "timeout" + flagProgress = "progress" + flagTimeout = "timeout" + flagDest = "dest" + flagRequireSignature = "require-signature" manifestArgsUsage = "[manifest file]" @@ -142,6 +144,17 @@ func commonFlags() []cli.Flag { } } +// requireSignatureFlag returns the --require-signature flag taken by the +// check and fetch subcommands. +func requireSignatureFlag() *cli.StringFlag { + return &cli.StringFlag{ + Name: flagRequireSignature, + Aliases: []string{"S"}, + Usage: "Require manifest to be signed by the specified GPG key ID", + EnvVars: []string{"MFER_REQUIRE_SIGNATURE"}, + } +} + func (mfa *CLIApp) generateCommand() *cli.Command { return &cli.Command{ Name: cmdGenerate, @@ -229,12 +242,7 @@ func (mfa *CLIApp) checkCommand() *cli.Command { Name: "no-extra-files", Usage: "Fail, instead of warning, if files in base directory are not in manifest", }, - &cli.StringFlag{ - Name: "require-signature", - Aliases: []string{"S"}, - Usage: "Require manifest to be signed by the specified GPG key ID", - EnvVars: []string{"MFER_REQUIRE_SIGNATURE"}, - }, + requireSignatureFlag(), ), } } @@ -354,6 +362,13 @@ func (mfa *CLIApp) fetchCommand() *cli.Command { Usage: "Time limit for each HTTP request, including the download " + "of its body", }, + &cli.StringFlag{ + Name: flagDest, + Aliases: []string{"d"}, + Value: ".", + Usage: "Directory to download the files and the manifest into", + }, + requireSignatureFlag(), ), } }