diff --git a/internal/cli/check.go b/internal/cli/check.go index b629a93..534f57b 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -108,7 +108,7 @@ func (mfa *CLIApp) fetchManifestToTemp( if tmpErr != nil { _ = rc.Close() - return "", fmt.Errorf("failed to create temp file: %w", tmpErr) + return "", tmpErr } tmpPath := tmpFile.Name() @@ -126,7 +126,7 @@ func (mfa *CLIApp) fetchManifestToTemp( if cpErr != nil { _ = mfa.Fs.Remove(tmpPath) - return "", fmt.Errorf("failed to download manifest: %w", cpErr) + return "", fmt.Errorf("download manifest: %w", cpErr) } return tmpPath, nil @@ -142,7 +142,7 @@ func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { _, err := hex.DecodeString(requiredSigner) if err != nil { - return fmt.Errorf("invalid fingerprint: must be valid hex: %w", err) + return fmt.Errorf("invalid fingerprint: %w", err) } if !chk.IsSigned() { @@ -232,7 +232,7 @@ func findExtraFiles( err := chk.FindExtraFiles(ctx, extraResults) if err != nil { - return fmt.Errorf("failed to check for extra files: %w", err) + return fmt.Errorf("find extra files: %w", err) } <-extraDone @@ -275,7 +275,7 @@ func runCheck( progressWg.Wait() if err != nil { - return 0, fmt.Errorf("check failed: %w", err) + return 0, fmt.Errorf("check files: %w", err) } // Wait for results processing to complete @@ -296,14 +296,14 @@ func (mfa *CLIApp) checkManifestOperation( manifestPath, err := mfa.resolveManifestArg(cmd) if err != nil { - return fmt.Errorf("check: %w", err) + return err } // URL manifests need to be downloaded to a temp file for the checker if isHTTPURL(manifestPath) { tmpPath, tmpErr := mfa.fetchManifestToTemp(ctx, manifestPath) if tmpErr != nil { - return fmt.Errorf("check: %w", tmpErr) + return tmpErr } defer func() { _ = mfa.Fs.Remove(tmpPath) }() @@ -324,7 +324,7 @@ func (mfa *CLIApp) checkManifestOperation( Fs: mfa.Fs, }) if err != nil { - return fmt.Errorf("failed to load manifest: %w", err) + return fmt.Errorf("load manifest: %w", err) } // Check signature requirement diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index dabfb17..d12a1c6 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -382,7 +382,7 @@ func TestGenerateRefusesTwoFilesAtOnePath(t *testing.T) { }, fs) assert.Equal(t, 1, runCLI(opts)) assert.Contains(t, testStderr(t, opts), - `generate: failed to enumerate paths: duplicate path "a.txt": `+ + `enumerate files: duplicate path "a.txt": `+ tc.first+"/a.txt and "+tc.second+"/a.txt") exists, err := afero.Exists(fs, testOutput) @@ -674,7 +674,7 @@ func TestCheckRequireSignatureRefusesOtherSigningKey(t *testing.T) { }, fs) assert.Equal(t, 1, runCLI(opts)) assert.Contains(t, testStderr(t, opts), - "failed to load manifest: signature verification failed: "+ + "load manifest: "+ "embedded public key block must hold exactly one key, found 2") } diff --git a/internal/cli/errmsg_test.go b/internal/cli/errmsg_test.go index 9b65ecf..ac5fb5f 100644 --- a/internal/cli/errmsg_test.go +++ b/internal/cli/errmsg_test.go @@ -303,7 +303,7 @@ func TestManifestLoaderHTTPStatusMessage(t *testing.T) { _, err := mfa.openManifestReader(context.Background(), server.URL+"/foo.mf") require.ErrorIs(t, err, errHTTPStatus) assert.EqualError(t, err, - "failed to fetch "+server.URL+"/foo.mf: HTTP 404") + "download manifest "+server.URL+"/foo.mf: unexpected HTTP status 404") } func TestFetchManifestHTTPStatusMessage(t *testing.T) { @@ -325,7 +325,7 @@ func TestFetchManifestHTTPStatusMessage(t *testing.T) { return cmd.Run(context.Background(), []string{cmdFetch, server.URL}) }) require.ErrorIs(t, err, errHTTPStatus) - assert.EqualError(t, err, "failed to fetch manifest: HTTP 404") + assert.EqualError(t, err, "download manifest: unexpected HTTP status 404") } func TestFetchFileHTTPStatusMessage(t *testing.T) { @@ -343,7 +343,108 @@ func TestFetchFileHTTPStatusMessage(t *testing.T) { &mfer.MFFilePath{}, nil) }) require.ErrorIs(t, err, errHTTPStatus) - assert.EqualError(t, err, "HTTP 500") + assert.EqualError(t, err, "unexpected HTTP status 500") +} + +// TestCheckCorruptManifestMessage runs check on a file that is not a +// manifest. +func TestCheckCorruptManifestMessage(t *testing.T) { + t.Parallel() + + fs := afero.NewMemMapFs() + require.NoError(t, afero.WriteFile(fs, "/bad.mf", []byte("not a manifest"), 0o644)) + + mfa := &CLIApp{Fs: fs} + cmd := mfa.checkCommand() + cmd.Action = mfa.checkManifestOperation + + // checkManifestOperation logs to the process-global logger. + err := runLocked(func() error { + return cmd.Run(context.Background(), []string{cmdCheck, "/bad.mf"}) + }) + assert.EqualError(t, err, "load manifest: invalid file format") +} + +// TestListMissingManifestMessage runs list on a manifest file that does not +// exist. +func TestListMissingManifestMessage(t *testing.T) { + t.Parallel() + + mfa := &CLIApp{Fs: afero.NewMemMapFs()} + cmd := mfa.listCommand() + + // listManifestOperation sets the process-global log level. + err := runLocked(func() error { + return cmd.Run(context.Background(), []string{cmdList, "/nope.mf"}) + }) + require.ErrorIs(t, err, os.ErrNotExist) + assert.EqualError(t, err, "open /nope.mf: file does not exist") +} + +// TestFetchHashMismatchMessage runs fetch against a server that sends a +// listed file with other content of the same size. +func TestFetchHashMismatchMessage(t *testing.T) { + t.Parallel() + + manifest := builtManifest(t, map[string][]byte{testFileTxt: []byte("listed")}) + + server := httptest.NewServer(fetchTestHandler(manifest, + map[string][]byte{testFileTxt: []byte("served")})) + defer server.Close() + + mfa := &CLIApp{Fs: afero.NewMemMapFs(), maxManifestSize: mfer.MaxManifestSize} + cmd := mfa.fetchCommand() + cmd.Action = mfa.fetchManifestOperation + + // fetchManifestOperation logs to the process-global logger. + err := runLocked(func() error { + return cmd.Run(context.Background(), + []string{cmdFetch, "--" + flagDest, t.TempDir(), server.URL}) + }) + require.ErrorIs(t, err, errHashMismatch) + assert.EqualError(t, err, "download "+testFileTxt+": hash mismatch") +} + +// TestFreshenBackslashPathMessage runs freshen on a tree that has gained a +// file whose name holds a backslash, which a manifest path may not contain. +func TestFreshenBackslashPathMessage(t *testing.T) { + t.Parallel() + + fs := afero.NewOsFs() + root, manifestPath := setupFreshenDir(t, fs, + map[string]string{testFileTxt: "content"}) + writeTestFile(t, fs, filepath.Join(root, `a\b.txt`), "new") + + mfa := &CLIApp{Fs: fs} + cmd := mfa.freshenCommand() + cmd.Action = mfa.freshenManifestOperation + + // freshenManifestOperation logs to the process-global logger. + err := runLocked(func() error { + return cmd.Run(context.Background(), + []string{cmdFreshen, testFlagBase, root, manifestPath}) + }) + assert.EqualError(t, err, + `path "a\\b.txt" contains backslash; use forward slashes only`) +} + +// TestFreshenReadErrorMessage has freshen hash a directory as though it +// were a file, so reading it fails. +func TestFreshenReadErrorMessage(t *testing.T) { + t.Parallel() + + root := t.TempDir() + require.NoError(t, os.Mkdir(filepath.Join(root, "sub"), 0o750)) + + hasher := &freshenHasher{ + fs: afero.NewOsFs(), + absBase: root, + builder: mfer.NewBuilder(), + } + + err := hasher.processEntry(&freshenEntry{path: "sub", needsHash: true}) + assert.EqualError(t, err, + "read "+filepath.Join(root, "sub")+": is a directory") } func TestURLRequiredMessage(t *testing.T) { diff --git a/internal/cli/export.go b/internal/cli/export.go index 8f9d2c0..d537039 100644 --- a/internal/cli/export.go +++ b/internal/cli/export.go @@ -26,12 +26,12 @@ func (mfa *CLIApp) exportManifestOperation( ) error { pathOrURL, err := mfa.resolveManifestArg(cmd) if err != nil { - return fmt.Errorf("export: %w", err) + return err } rc, err := mfa.openManifestReader(ctx, pathOrURL) if err != nil { - return fmt.Errorf("export: %w", err) + return err } defer func() { _ = rc.Close() }() @@ -39,7 +39,7 @@ func (mfa *CLIApp) exportManifestOperation( //nolint:contextcheck // mfer loads a manifest without a context manifest, err := mfer.NewManifestFromReader(rc) if err != nil { - return fmt.Errorf("export: failed to parse manifest: %w", err) + return fmt.Errorf("parse manifest: %w", err) } files := manifest.Files() @@ -76,7 +76,7 @@ func (mfa *CLIApp) exportManifestOperation( err = enc.Encode(entries) if err != nil { - return fmt.Errorf("export: failed to encode JSON: %w", err) + return fmt.Errorf("encode JSON: %w", err) } return nil diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 0244d67..8b0f569 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -239,7 +239,7 @@ func reportDownloadProgress(progress <-chan DownloadProgress, done chan<- struct func manifestBaseURL(manifestURL string) (*url.URL, error) { parsed, err := url.Parse(manifestURL) if err != nil { - return nil, fmt.Errorf("fetch: invalid manifest URL: %w", err) + return nil, fmt.Errorf("invalid manifest URL: %w", err) } // JoinPath cleans the path it builds, so ".." drops the manifest's @@ -268,7 +268,7 @@ func downloadManifestFiles( // Sanitize the path to prevent path traversal attacks localPath, err := sanitizePath(f.GetPath()) if err != nil { - return 0, 0, fmt.Errorf("invalid path in manifest: %w", err) + return 0, 0, fmt.Errorf("invalid file entry: %w", err) } if alreadyPresent(dest, localPath, f) { @@ -284,7 +284,7 @@ func downloadManifestFiles( err = downloadFile(ctx, client, fileURL, dest, localPath, f, progress) if err != nil { - return 0, 0, fmt.Errorf("failed to download %s: %w", f.GetPath(), err) + return 0, 0, fmt.Errorf("download %s: %w", f.GetPath(), err) } downloaded++ @@ -376,7 +376,7 @@ func (mfa *CLIApp) fetchManifestOperation( err = os.MkdirAll(dest, dirPerms) if err != nil { - return fmt.Errorf("failed to create destination directory %s: %w", dest, err) + return err } // Create progress channel and start progress reporter goroutine @@ -403,7 +403,7 @@ func (mfa *CLIApp) fetchManifestOperation( // "mfer check" can verify the tree later. err = saveManifest(dest, manifestData) if err != nil { - return fmt.Errorf("failed to save manifest: %w", err) + return fmt.Errorf("save manifest: %w", err) } // Print summary @@ -446,11 +446,11 @@ func (mfa *CLIApp) fetchManifest( return readErr }) if err != nil { - return nil, nil, fmt.Errorf("failed to fetch manifest: %w", err) + return nil, nil, fmt.Errorf("download manifest: %w", err) } if int64(len(manifestData)) > mfa.maxManifestSize { - return nil, nil, fmt.Errorf("failed to fetch manifest: %w of %d bytes", + return nil, nil, fmt.Errorf("download manifest: %w of %d bytes", errManifestTooLarge, mfa.maxManifestSize) } @@ -458,7 +458,7 @@ func (mfa *CLIApp) fetchManifest( //nolint:contextcheck // mfer loads a manifest without a context manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) if err != nil { - return nil, nil, fmt.Errorf("failed to parse manifest: %w", err) + return nil, nil, fmt.Errorf("parse manifest: %w", err) } requiredSigner := cmd.String(flagRequireSignature) @@ -561,7 +561,7 @@ func verifyFetchedSigner(manifestData []byte, requiredSigner string) error { Fs: memFs, }) if err != nil { - return fmt.Errorf("failed to load manifest: %w", err) + return fmt.Errorf("load manifest: %w", err) } return verifyRequiredSigner(chk, requiredSigner) @@ -653,7 +653,7 @@ func checkNoSymlinks(dest, p string) error { } if err != nil { - return fmt.Errorf("failed to check %s for a symlink: %w", current, err) + return err } if info.Mode()&os.ModeSymlink != 0 { @@ -770,7 +770,7 @@ func tempPathFor(localPath string) string { func verifyDownloadedHash(digest []byte, entry *mfer.MFFilePath) error { computed, err := multihash.Encode(digest, multihash.SHA2_256) if err != nil { - return fmt.Errorf("failed to encode hash: %w", err) + return fmt.Errorf("encode hash: %w", err) } for _, hash := range entry.GetHashes() { @@ -797,7 +797,7 @@ func downloadFile( // so every entry point to downloadFile gets the same treatment. localPath, err := sanitizePath(localPath) if err != nil { - return fmt.Errorf("invalid path: %w", err) + return fmt.Errorf("invalid file entry: %w", err) } // Create parent directories if needed @@ -812,7 +812,7 @@ func downloadFile( err = os.MkdirAll(dir, dirPerms) if err != nil { - return fmt.Errorf("failed to create directory %s: %w", dir, err) + return err } } @@ -846,7 +846,7 @@ func createTempFile(dest, tmpPath string) (*os.File, error) { 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 nil, err } return out, nil @@ -859,12 +859,7 @@ func moveIntoPlace(dest, tmpPath, localPath string) error { 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 + return os.Rename(filepath.Join(dest, tmpPath), filepath.Join(dest, localPath)) } // saveResponse writes resp's body to tmpPath, verifies it against entry, @@ -899,7 +894,7 @@ func saveResponse( _ = out.Close() _ = os.Remove(filepath.Join(dest, tmpPath)) - return fmt.Errorf("failed to set mode: %w", err) + return err } } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index bb79298..8d61b20 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -502,8 +502,9 @@ func TestManifestDownloadStopsAtLimit(t *testing.T) { []string{cmd.Name, server.URL + "/index.mf"}) }) require.ErrorIs(t, err, errManifestTooLarge, cmd.Name) - require.ErrorContains(t, err, - "maximum allowed size of 65536 bytes", cmd.Name) + require.EqualError(t, err, + "download manifest: file exceeds maximum allowed size of 65536 bytes", + cmd.Name) } leftover, err := os.ReadDir(tmpDir) @@ -605,27 +606,27 @@ func TestFetchRefusesSymlinks(t *testing.T) { }{ { "parent directory", "sub/deeper/file.txt", "sub", ".", - "failed to download sub/deeper/file.txt", + "download sub/deeper/file.txt", }, { "directory inside a plain directory", "docs/data/passwd", "docs/data", ".", - "failed to download docs/data/passwd", + "download docs/data/passwd", }, { "temp file", testFileTxt, ".file.txt.tmp", newFile, - "failed to download " + testFileTxt, + "download " + testFileTxt, }, { "file", testFileTxt, testFileTxt, newFile, - "failed to download " + testFileTxt, + "download " + testFileTxt, }, { "manifest temp file", testFileTxt, tempPathFor(defaultManifestName), newFile, - "failed to save manifest", + "save manifest", }, { "manifest", testFileTxt, defaultManifestName, newFile, - "failed to save manifest", + "save manifest", }, } @@ -692,7 +693,7 @@ func TestFetchDoesNotSkipThroughSymlink(t *testing.T) { }, 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) + "download sub/"+testFileTxt+": symlink in path not allowed: "+link) assert.Equal(t, map[string][]byte{testFileTxt: content}, filesUnder(t, outside)) } @@ -762,7 +763,8 @@ func TestGetRetriesTransientStatusesOnly(t *testing.T) { err := getNothing(testClient(), server.URL, 10*time.Second) require.ErrorIs(t, err, errHTTPStatus) - require.EqualError(t, err, fmt.Sprintf("HTTP %d", tt.status)) + require.EqualError(t, err, + fmt.Sprintf("unexpected HTTP status %d", tt.status)) assert.Equal(t, tt.requests, requests.Load()) }) } @@ -1228,7 +1230,7 @@ func TestFetchRequireSignature(t *testing.T) { manifest, required := manifestSignedByAnotherKey(t, files) assertFetchRefused(t, manifest, files, - "failed to parse manifest: signature verification failed: "+ + "parse manifest: "+ "embedded public key block must hold exactly one key, found 2", "--"+flagRequireSignature, required) }) diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index e39fbb5..8a1e0a7 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -165,8 +165,7 @@ func (s *freshenScanner) walk(path string, info fs.FileInfo, walkErr error) erro // Get relative path relPath, err := filepath.Rel(s.absBase, path) if err != nil { - return fmt.Errorf( - "freshen: failed to compute relative path for %s: %w", path, err) + return err } // Handle dotfiles @@ -280,13 +279,8 @@ func (h *freshenHasher) reportProgress(n int64) { // processEntry hashes the entry if needed and adds it to the builder. func (h *freshenHasher) processEntry(e *freshenEntry) error { if !e.needsHash { - // Use existing entry - err := addExistingToBuilder(h.builder, e.existing) - if err != nil { - return fmt.Errorf("failed to add %s: %w", e.path, err) - } - - return nil + // Use existing entry; the error names the entry + return addExistingToBuilder(h.builder, e.existing) } // Need to read and hash the file @@ -294,26 +288,21 @@ func (h *freshenHasher) processEntry(e *freshenEntry) error { f, err := h.fs.Open(absPath) if err != nil { - return fmt.Errorf("failed to open %s: %w", e.path, err) + return err } hash, bytesRead, err := hashFile(f, h.reportProgress) _ = f.Close() if err != nil { - return fmt.Errorf("failed to hash %s: %w", e.path, err) + return err } h.hashedBytes += bytesRead h.hashedFiles++ - // Add to builder with computed hash - err = addFileToBuilder(h.builder, e.path, e.size, e.mtime, e.mode, hash) - if err != nil { - return fmt.Errorf("failed to add %s: %w", e.path, err) - } - - return nil + // Add to builder with computed hash; a refused path is named in the error + return addFileToBuilder(h.builder, e.path, e.size, e.mtime, e.mode, hash) } // writeFreshenedManifest writes the manifest atomically (write to a @@ -325,7 +314,7 @@ func writeFreshenedManifest( outFile, err := afs.Create(tmpPath) if err != nil { - return fmt.Errorf("failed to create temp file: %w", err) + return err } err = builder.Build(ctx, outFile) @@ -334,7 +323,7 @@ func writeFreshenedManifest( if err != nil { _ = afs.Remove(tmpPath) - return fmt.Errorf("failed to write manifest: %w", err) + return fmt.Errorf("build manifest: %w", err) } // Rename temp to final @@ -342,7 +331,7 @@ func writeFreshenedManifest( if err != nil { _ = afs.Remove(tmpPath) - return fmt.Errorf("failed to rename manifest: %w", err) + return err } return nil @@ -408,7 +397,7 @@ func (mfa *CLIApp) freshenScan( } if err != nil { - return nil, 0, fmt.Errorf("failed to scan filesystem: %w", err) + return nil, 0, fmt.Errorf("scan filesystem: %w", err) } // Remaining entries in existingByPath are removed files @@ -477,7 +466,7 @@ func (mfa *CLIApp) loadExistingEntries( Fs: mfa.Fs, }) if err != nil { - return nil, fmt.Errorf("failed to load manifest: %w", err) + return nil, fmt.Errorf("load manifest: %w", err) } existingFiles := manifest.Files() @@ -503,7 +492,7 @@ func (mfa *CLIApp) freshenManifestOperation( // Find manifest file manifestPath, err := mfa.resolveFreshenManifestPath(cmd) if err != nil { - return fmt.Errorf("freshen: %w", err) + return err } //nolint:contextcheck // mfer loads a manifest without a context @@ -514,7 +503,7 @@ func (mfa *CLIApp) freshenManifestOperation( absBase, err := filepath.Abs(basePath) if err != nil { - return fmt.Errorf("freshen: invalid base path: %w", err) + return fmt.Errorf("invalid base path: %w", err) } // Phase 1: Scan filesystem @@ -606,9 +595,7 @@ func hashFile(r io.Reader, progress func(int64)) ([]byte, int64, error) { break } - // Returned unwrapped: the caller renders this as - // "failed to hash : " and adding a second layer here - // would change that message. + // Returned unwrapped: a read error already names the file. if err != nil { return nil, total, err } diff --git a/internal/cli/gen.go b/internal/cli/gen.go index 16ac69c..b32170f 100644 --- a/internal/cli/gen.go +++ b/internal/cli/gen.go @@ -74,7 +74,7 @@ func (mfa *CLIApp) collectInputPaths(args cli.Args) ([]string, error) { ap, err := filepath.Abs(inputPath) if err != nil { - return nil, fmt.Errorf("generate: invalid path %q: %w", inputPath, err) + return nil, fmt.Errorf("invalid path %q: %w", inputPath, err) } // Validate path exists before adding to list if exists, _ := afero.Exists(mfa.Fs, ap); !exists { @@ -129,8 +129,7 @@ func (mfa *CLIApp) enumerateInputs( // Default to current directory err := s.EnumeratePath(".", enumProgress) if err != nil { - return fmt.Errorf( - "generate: failed to enumerate current directory: %w", err) + return fmt.Errorf("enumerate current directory: %w", err) } return nil @@ -144,7 +143,7 @@ func (mfa *CLIApp) enumerateInputs( err = s.EnumeratePaths(enumProgress, paths...) if err != nil { - return fmt.Errorf("generate: failed to enumerate paths: %w", err) + return fmt.Errorf("enumerate files: %w", err) } return nil @@ -229,7 +228,7 @@ func (mfa *CLIApp) generateManifestOperation( outFile, err := mfa.Fs.Create(tmpPath) if err != nil { - return fmt.Errorf("failed to create temp file: %w", err) + return err } // Set up signal handler to clean up temp file on Ctrl-C @@ -268,19 +267,19 @@ func (mfa *CLIApp) generateManifestOperation( scanWg.Wait() if err != nil { - return fmt.Errorf("failed to generate manifest: %w", err) + return fmt.Errorf("generate manifest: %w", err) } // Close file before rename to ensure all data is flushed err = outFile.Close() if err != nil { - return fmt.Errorf("failed to close temp file: %w", err) + return err } // Atomic rename err = mfa.Fs.Rename(tmpPath, outputPath) if err != nil { - return fmt.Errorf("failed to rename temp file: %w", err) + return err } success = true diff --git a/internal/cli/list.go b/internal/cli/list.go index adc9e37..ed0fff9 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -19,12 +19,12 @@ func (mfa *CLIApp) listManifestOperation(ctx context.Context, cmd *cli.Command) pathOrURL, err := mfa.resolveManifestArg(cmd) if err != nil { - return fmt.Errorf("list: %w", err) + return err } rc, err := mfa.openManifestReader(ctx, pathOrURL) if err != nil { - return fmt.Errorf("list: %w", err) + return err } defer func() { _ = rc.Close() }() @@ -32,7 +32,7 @@ func (mfa *CLIApp) listManifestOperation(ctx context.Context, cmd *cli.Command) //nolint:contextcheck // mfer loads a manifest without a context manifest, err := mfer.NewManifestFromReader(rc) if err != nil { - return fmt.Errorf("list: failed to parse manifest: %w", err) + return fmt.Errorf("parse manifest: %w", err) } files := manifest.Files() diff --git a/internal/cli/manifest_loader.go b/internal/cli/manifest_loader.go index ca3e64b..d661f7b 100644 --- a/internal/cli/manifest_loader.go +++ b/internal/cli/manifest_loader.go @@ -15,17 +15,13 @@ import ( // manifestFetchTimeout bounds HTTP requests made to fetch a manifest. const manifestFetchTimeout = 30 * time.Second -// errHTTPStatus indicates an HTTP response with a non-OK status code. -// -// Its text is the literal "HTTP" prefix of the rendered "HTTP " -// message that mfer has always printed, so that wrapping it does not -// change any user-visible output. Match it with errors.Is; do not read -// its message. -var errHTTPStatus = errors.New("HTTP") +// errHTTPStatus indicates an HTTP response with a non-OK status code. It is +// followed by the code, as in "unexpected HTTP status 404". +var errHTTPStatus = errors.New("unexpected HTTP status") // errManifestTooLarge indicates a manifest download that passed // CLIApp.maxManifestSize. -var errManifestTooLarge = errors.New("manifest exceeds maximum allowed size") +var errManifestTooLarge = errors.New("file exceeds maximum allowed size") // isHTTPURL returns true if the string starts with http:// or https://. func isHTTPURL(s string) bool { @@ -40,20 +36,22 @@ func (mfa *CLIApp) openManifestReader( if isHTTPURL(pathOrURL) { client := &http.Client{Timeout: manifestFetchTimeout} + // The *url.Error that NewRequestWithContext and Do return names + // the URL. req, err := http.NewRequestWithContext(ctx, http.MethodGet, pathOrURL, nil) if err != nil { - return nil, fmt.Errorf("failed to fetch %s: %w", pathOrURL, err) + return nil, fmt.Errorf("download manifest: %w", err) } resp, err := client.Do(req) if err != nil { - return nil, fmt.Errorf("failed to fetch %s: %w", pathOrURL, err) + return nil, fmt.Errorf("download manifest: %w", err) } if resp.StatusCode != http.StatusOK { _ = resp.Body.Close() - return nil, fmt.Errorf("failed to fetch %s: %w %d", + return nil, fmt.Errorf("download manifest %s: %w %d", pathOrURL, errHTTPStatus, resp.StatusCode) } diff --git a/mfer/builder.go b/mfer/builder.go index 470e2ee..ddc98c6 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -246,7 +246,7 @@ func (b *Builder) AddFileWithHash( ) error { err := ValidatePath(string(path)) if err != nil { - return fmt.Errorf("add file: %w", err) + return err } if size < 0 { @@ -329,22 +329,19 @@ func (b *Builder) Build(ctx context.Context, w io.Writer) error { // Generate outer wrapper err := m.generateOuter(ctx) if err != nil { - return fmt.Errorf("build: generate outer: %w", err) + return err } // Generate final output err = m.generate(ctx) if err != nil { - return fmt.Errorf("build: generate: %w", err) + return err } // Write to output _, err = w.Write(m.output.Bytes()) - if err != nil { - return fmt.Errorf("build: write output: %w", err) - } - return nil + return err } // addEntry adds entry to the manifest unless an entry with its path is diff --git a/mfer/deserialize.go b/mfer/deserialize.go index bfd019c..ddbaffd 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -24,13 +24,12 @@ var ( errCompressedHashWrong = errors.New("compressed data hash mismatch") errSignatureNoPubKey = errors.New("signature present but no public key") errDecompressedTooLarge = errors.New("decompressed data exceeds maximum allowed size") - errManifestTooLarge = errors.New("manifest exceeds maximum allowed size") + errManifestTooLarge = errors.New("file exceeds maximum allowed size") errUUIDMismatch = errors.New("outer and inner UUID mismatch") errInvalidFileFormat = errors.New("invalid file format") - errInvalidManifestPath = errors.New("manifest contains invalid path") - errDecodedTooLarge = errors.New( - "manifest would take too much memory to decode") - errSignerNotSigningKey = errors.New( + errInvalidManifestPath = errors.New("invalid file entry") + errDecodedTooLarge = errors.New("too much memory needed") + errSignerNotSigningKey = errors.New( "signer is not the fingerprint of the key that made the signature") ) @@ -58,7 +57,7 @@ func (m *manifest) validateOuterHeader() error { // Validate outer UUID before any decompression err := validateUUID(m.pbOuter.GetUuid()) if err != nil { - return fmt.Errorf("outer UUID invalid: %w", err) + return fmt.Errorf("outer message: %w", err) } return nil @@ -73,7 +72,7 @@ func (m *manifest) verifyOuterIntegrity() error { _, err := h.Write(m.pbOuter.GetInnerMessage()) if err != nil { - return fmt.Errorf("deserialize: hash write: %w", err) + return fmt.Errorf("hash inner message: %w", err) } sha256Hash := h.Sum(nil) @@ -91,9 +90,7 @@ func (m *manifest) verifyOuterIntegrity() error { sigString, err := m.signatureString() if err != nil { - return fmt.Errorf( - "failed to generate signature string for verification: %w", err, - ) + return fmt.Errorf("build signature string: %w", err) } // Loading a manifest takes no context; gpgTimeout still bounds gpg. @@ -104,7 +101,7 @@ func (m *manifest) verifyOuterIntegrity() error { m.pbOuter.GetSigningPubKey(), ) if err != nil { - return fmt.Errorf("signature verification failed: %w", err) + return err } if !strings.EqualFold(string(m.pbOuter.GetSigner()), signingKey) { @@ -135,7 +132,7 @@ func (m *manifest) decompressInner() ([]byte, error) { zstd.WithDecodeBuffersBelow(0), zstd.WithDecoderMaxWindow(zstdWindowSize)) if err != nil { - return nil, fmt.Errorf("deserialize: zstd reader: %w", err) + return nil, fmt.Errorf("create decompressor: %w", err) } defer zr.Close() @@ -150,7 +147,7 @@ func (m *manifest) decompressInner() ([]byte, error) { dat, err := io.ReadAll(limitedReader) if err != nil { - return nil, fmt.Errorf("deserialize: decompress: %w", err) + return nil, fmt.Errorf("decompress inner message: %w", err) } if int64(len(dat)) >= MaxDecompressedSize { @@ -266,7 +263,7 @@ func (m *manifest) deserializeInner() error { err = checkDecodedSize(dat) if err != nil { - return fmt.Errorf("deserialize: unmarshal inner: %w", err) + return fmt.Errorf("unmarshal inner message: %w", err) } // Deserialize inner message @@ -275,7 +272,7 @@ func (m *manifest) deserializeInner() error { // Unknown fields would cost memory; mfer never writes a loaded manifest out. err = proto.UnmarshalOptions{DiscardUnknown: true}.Unmarshal(dat, m.pbInner) if err != nil { - return fmt.Errorf("deserialize: unmarshal inner: %w", err) + return fmt.Errorf("unmarshal inner message: %w", err) } if m.pbInner.GetVersion() != MFFile_VERSION_ONE { @@ -285,7 +282,7 @@ func (m *manifest) deserializeInner() error { // Validate inner UUID err = validateUUID(m.pbInner.GetUuid()) if err != nil { - return fmt.Errorf("inner UUID invalid: %w", err) + return fmt.Errorf("inner message: %w", err) } // Verify UUIDs match diff --git a/mfer/deserialize_test.go b/mfer/deserialize_test.go index 878a6a8..a8b53d6 100644 --- a/mfer/deserialize_test.go +++ b/mfer/deserialize_test.go @@ -49,6 +49,6 @@ func TestReadAtMost(t *testing.T) { _, err = readAtMost(input, maxSize) require.ErrorIs(t, err, errManifestTooLarge) require.EqualError(t, err, - "manifest exceeds maximum allowed size of 65536 bytes") + "file exceeds maximum allowed size of 65536 bytes") assert.Equal(t, maxSize-1, input.Len(), "bytes left unread") } diff --git a/mfer/errmsg_test.go b/mfer/errmsg_test.go index d2dc32c..1f2d3c9 100644 --- a/mfer/errmsg_test.go +++ b/mfer/errmsg_test.go @@ -74,14 +74,18 @@ func TestValidatePathMessagesVerbatim(t *testing.T) { } } -// TestSerializeInternalErrorMessagesVerbatim pins the two distinct -// "internal error" messages, which differ between generate and -// generateOuter and have always done so. -func TestSerializeInternalErrorMessagesVerbatim(t *testing.T) { +// TestSerializeInnerNotSetMessagesVerbatim pins the messages generate and +// generateOuter return when the inner message is missing. +func TestSerializeInnerNotSetMessagesVerbatim(t *testing.T) { t.Parallel() m := &manifest{} - require.EqualError(t, m.generate(context.Background()), - "internal error: pbInner not set") - require.EqualError(t, m.generateOuter(context.Background()), "internal error") + + err := m.generate(context.Background()) + require.ErrorIs(t, err, errInnerNotSet) + require.EqualError(t, err, "inner message not set") + + err = m.generateOuter(context.Background()) + require.ErrorIs(t, err, errInternal) + require.EqualError(t, err, "inner message not set") } diff --git a/mfer/gpg.go b/mfer/gpg.go index f2c095e..21b89c3 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -53,7 +53,7 @@ const ( ) var ( - errGPGKeyNotFound = errors.New("gpg key not found") + errGPGKeyNotFound = errors.New("GPG key not found") errFingerprintNotFound = errors.New("fingerprint not found for key") errSigningKeyCount = errors.New( "embedded public key block must hold exactly one key") @@ -88,8 +88,9 @@ func gpgArgs(opts []string, positional ...string) []string { } // runGPG runs the gpg binary in batch mode with the given arguments and -// optional stdin, returning captured stdout and stderr. gpg is killed when -// ctx ends or gpgTimeout passes, whichever comes first. +// optional stdin, returning captured stdout and stderr. If gpg fails, the +// error ends with what gpg wrote to stderr. gpg is killed when ctx ends or +// gpgTimeout passes, whichever comes first. func runGPG( ctx context.Context, stdin io.Reader, args ...string, ) (*bytes.Buffer, *bytes.Buffer, error) { @@ -125,13 +126,28 @@ func runGPG( // "signal: killed"; return the reason instead. err = ctx.Err() if errors.Is(err, context.DeadlineExceeded) { - err = fmt.Errorf("gpg timed out: %w", err) + err = fmt.Errorf("timed out: %w", err) } } + if err != nil { + err = withStderr(err, &stderr) + } + return &stdout, &stderr, err } +// withStderr returns err followed by what gpg wrote to stderr, or err alone +// when gpg wrote nothing. +func withStderr(err error, stderr *bytes.Buffer) error { + messages := strings.TrimSpace(stderr.String()) + if messages == "" { + return err + } + + return fmt.Errorf("%w: %s", err, messages) +} + // parseFingerprint extracts the first fingerprint from gpg --with-colons // output, or returns ok=false if none is present. func parseFingerprint(colonOutput string) (string, bool) { @@ -174,7 +190,7 @@ func gpgSign( ) ([]byte, string, error) { tmpDir, err := os.MkdirTemp("", "mfer-gpg-sign-*") if err != nil { - return nil, "", fmt.Errorf("failed to create temp dir: %w", err) + return nil, "", err } defer func() { _ = os.RemoveAll(tmpDir) }() @@ -191,19 +207,19 @@ func gpgSign( "--local-user", string(keyID), ) if err != nil { - return nil, "", fmt.Errorf("gpg sign failed: %w: %s", err, stderr.String()) + return nil, "", fmt.Errorf("gpg sign: %w", err) } // The last argument of SIG_CREATED is the fingerprint of the key that // made the signature. created, ok := parseStatusLine(stdout.String(), "SIG_CREATED") if !ok { - return nil, "", fmt.Errorf("%w: %s", errSigningKeyNotReported, stderr.String()) + return nil, "", withStderr(errSigningKeyNotReported, stderr) } sig, err := os.ReadFile(sigFile) //nolint:gosec // G304: inside tmpDir, made above if err != nil { - return nil, "", fmt.Errorf("failed to read signature: %w", err) + return nil, "", err } return sig, created[len(created)-1], nil @@ -212,11 +228,11 @@ func gpgSign( // gpgExportPublicKey exports the public key for the specified key ID. // Returns the armored public key. func gpgExportPublicKey(ctx context.Context, keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(ctx, nil, + stdout, _, err := runGPG(ctx, nil, gpgArgs([]string{"--export", gpgOptArmor}, string(keyID))..., ) if err != nil { - return nil, fmt.Errorf("gpg export failed: %w: %s", err, stderr.String()) + return nil, fmt.Errorf("gpg export: %w", err) } if stdout.Len() == 0 { @@ -228,13 +244,11 @@ func gpgExportPublicKey(ctx context.Context, keyID GPGKeyID) ([]byte, error) { // gpgGetKeyFingerprint gets the full fingerprint for a key ID. func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(ctx, nil, + stdout, _, err := runGPG(ctx, nil, gpgArgs([]string{"--with-colons", "--fingerprint"}, string(keyID))..., ) if err != nil { - return nil, fmt.Errorf( - "gpg fingerprint lookup failed: %w: %s", err, stderr.String(), - ) + return nil, fmt.Errorf("gpg fingerprint lookup: %w", err) } fpr, ok := parseFingerprint(stdout.String()) @@ -250,14 +264,12 @@ func gpgGetKeyFingerprint(ctx context.Context, keyID GPGKeyID) ([]byte, error) { func gpgImportOneKey(ctx context.Context, gpgHome, pubKeyFile string) error { // --status-fd 1 sends gpg's status lines to stdout, which importing // otherwise leaves empty; its messages go to stderr. - importStdout, importStderr, err := runGPG(ctx, nil, + importStdout, _, err := runGPG(ctx, nil, gpgArgs([]string{gpgOptHomedir, gpgHome, gpgOptStatusFD, "1", "--import"}, pubKeyFile)..., ) if err != nil { - return fmt.Errorf( - "failed to import public key: %w: %s", err, importStderr.String(), - ) + return fmt.Errorf("gpg import: %w", err) } // The first argument of IMPORT_RES counts the primary keys gpg read @@ -284,7 +296,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err // Create temporary directory for GPG operations tmpDir, err := os.MkdirTemp("", "mfer-gpg-verify-*") if err != nil { - return "", fmt.Errorf("failed to create temp dir: %w", err) + return "", err } defer func() { _ = os.RemoveAll(tmpDir) }() @@ -292,7 +304,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err // Set restrictive permissions err = os.Chmod(tmpDir, privateDirPerms) if err != nil { - return "", fmt.Errorf("failed to set temp dir permissions: %w", err) + return "", err } // Write public key to temp file @@ -300,7 +312,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err err = os.WriteFile(pubKeyFile, pubKey, privateFilePerms) if err != nil { - return "", fmt.Errorf("failed to write public key: %w", err) + return "", err } // Write signature to temp file @@ -308,7 +320,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err err = os.WriteFile(sigFile, signature, privateFilePerms) if err != nil { - return "", fmt.Errorf("failed to write signature: %w", err) + return "", err } // Write data to temp file @@ -316,7 +328,7 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err err = os.WriteFile(dataFile, data, privateFilePerms) if err != nil { - return "", fmt.Errorf("failed to write data: %w", err) + return "", err } err = gpgImportOneKey(ctx, tmpDir, pubKeyFile) @@ -326,14 +338,12 @@ func gpgVerify(ctx context.Context, data, signature, pubKey []byte) (string, err // --status-fd 1 sends gpg's status lines to stdout, which verifying a // detached signature otherwise leaves empty; its messages go to stderr. - verifyStdout, verifyStderr, err := runGPG(ctx, nil, + verifyStdout, _, err := runGPG(ctx, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptStatusFD, "1", gpgOptVerify}, sigFile, dataFile)..., ) if err != nil { - return "", fmt.Errorf( - "signature verification failed: %w: %s", err, verifyStderr.String(), - ) + return "", fmt.Errorf("gpg verify: %w", err) } // gpg writes a VALIDSIG line for each good signature. Its first diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 495b4fb..91eb13d 100644 --- a/mfer/gpg_test.go +++ b/mfer/gpg_test.go @@ -603,7 +603,8 @@ func fakeGPGPath(t *testing.T, script string) string { // TestGPGTimeoutKillsGPG puts a fake gpg that never finishes first on // PATH and checks that a run past its deadline is killed and reported as -// a timeout of the named operation, instead of hanging. +// a timeout of the named operation, instead of hanging. The fake gpg writes +// nothing to stderr, so the message ends with the timeout. func TestGPGTimeoutKillsGPG(t *testing.T) { t.Setenv("PATH", fakeGPGPath(t, "#!/bin/sh\nexec sleep 10\n")) @@ -612,7 +613,21 @@ func TestGPGTimeoutKillsGPG(t *testing.T) { _, _, err := gpgSign(ctx, []byte("data"), GPGKeyID("any")) require.ErrorIs(t, err, context.DeadlineExceeded) - assert.Contains(t, err.Error(), "gpg sign failed: gpg timed out") + assert.EqualError(t, err, "gpg sign: timed out: context deadline exceeded") +} + +// TestGPGSignKeyNotReportedKeepsStderr puts a fake gpg first on PATH that +// exits cleanly without reporting the key that signed, and checks that what +// it wrote to stderr is in the message. +func TestGPGSignKeyNotReportedKeepsStderr(t *testing.T) { + t.Setenv("PATH", fakeGPGPath(t, + "#!/bin/sh\necho 'gpg: note from the fake gpg' >&2\n")) + + _, _, err := gpgSign(context.Background(), []byte("data"), GPGKeyID("any")) + require.ErrorIs(t, err, errSigningKeyNotReported) + assert.EqualError(t, err, + "gpg did not report the key that made the signature: "+ + "gpg: note from the fake gpg") } // TestGPGCancelWhenChildHoldsOutput uses a fake gpg that runs sleep as a diff --git a/mfer/manifest.go b/mfer/manifest.go index 7e847d1..b172a12 100644 --- a/mfer/manifest.go +++ b/mfer/manifest.go @@ -10,7 +10,7 @@ import ( ) var ( - errOuterNotSet = errors.New("pbOuter not set") + errOuterNotSet = errors.New("outer message not set") errUUIDNotSet = errors.New("UUID not set") errSHA256NotSet = errors.New("SHA256 hash not set") ) @@ -65,7 +65,7 @@ func (m *manifest) signatureString() (string, error) { mh, err := multihash.Encode(m.pbOuter.GetSha256(), multihash.SHA2_256) if err != nil { - return "", fmt.Errorf("failed to encode multihash: %w", err) + return "", fmt.Errorf("encode multihash: %w", err) } uuidStr := hex.EncodeToString(m.pbOuter.GetUuid()) diff --git a/mfer/serialize.go b/mfer/serialize.go index f0945a7..6688ed9 100644 --- a/mfer/serialize.go +++ b/mfer/serialize.go @@ -20,11 +20,9 @@ const MAGIC string = "ZNAVSRFG" var ( // errInnerNotSet is returned by generate when the inner manifest is // missing. - errInnerNotSet = errors.New("internal error: pbInner not set") + errInnerNotSet = errors.New("inner message not set") // errInternal is returned by generateOuter for the same condition. - // The two messages differ, and both are load-bearing for callers that - // match on text, so they are kept distinct. - errInternal = errors.New("internal error") + errInternal = errors.New("inner message not set") ) // nanosecondsInt32 converts t's nanosecond component to int32. @@ -65,14 +63,14 @@ func (m *manifest) generate(ctx context.Context) error { dat, err := proto.MarshalOptions{Deterministic: true}.Marshal(m.pbOuter) if err != nil { - return fmt.Errorf("serialize: marshal outer: %w", err) + return fmt.Errorf("marshal outer message: %w", err) } m.output = bytes.NewBufferString(MAGIC) _, err = m.output.Write(dat) if err != nil { - return fmt.Errorf("serialize: write output: %w", err) + return fmt.Errorf("write outer message: %w", err) } return nil @@ -95,7 +93,7 @@ func (m *manifest) generateOuter(ctx context.Context) error { innerData, err := proto.MarshalOptions{Deterministic: true}.Marshal(m.pbInner) if err != nil { - return fmt.Errorf("serialize: marshal inner: %w", err) + return fmt.Errorf("marshal inner message: %w", err) } // Compress the inner data @@ -103,12 +101,12 @@ func (m *manifest) generateOuter(ctx context.Context) error { zw, err := zstd.NewWriter(idc, zstd.WithEncoderLevel(zstd.SpeedBestCompression)) if err != nil { - return fmt.Errorf("serialize: create compressor: %w", err) + return fmt.Errorf("create compressor: %w", err) } _, err = zw.Write(innerData) if err != nil { - return fmt.Errorf("serialize: compress: %w", err) + return fmt.Errorf("compress inner message: %w", err) } _ = zw.Close() @@ -120,7 +118,7 @@ func (m *manifest) generateOuter(ctx context.Context) error { _, err = h.Write(compressedData) if err != nil { - return fmt.Errorf("serialize: hash write: %w", err) + return fmt.Errorf("hash inner message: %w", err) } sha256Hash := h.Sum(nil) @@ -149,12 +147,12 @@ func (m *manifest) generateOuter(ctx context.Context) error { func (m *manifest) signOuter(ctx context.Context) error { sigString, err := m.signatureString() if err != nil { - return fmt.Errorf("failed to generate signature string: %w", err) + return fmt.Errorf("build signature string: %w", err) } sig, signingKey, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID) if err != nil { - return fmt.Errorf("failed to sign manifest: %w", err) + return err } m.pbOuter.Signature = sig @@ -163,14 +161,14 @@ func (m *manifest) signOuter(ctx context.Context) error { // fingerprint first. fingerprint, err := gpgGetKeyFingerprint(ctx, GPGKeyID(signingKey)) if err != nil { - return fmt.Errorf("failed to get key fingerprint: %w", err) + return err } m.pbOuter.Signer = fingerprint pubKey, err := gpgExportPublicKey(ctx, GPGKeyID(fingerprint)) if err != nil { - return fmt.Errorf("failed to export public key: %w", err) + return err } m.pbOuter.SigningPubKey = pubKey