Make error message wording consistent (closes #165) #176

Merged
clawbot merged 1 commits from issue-165-error-wording into next 2026-10-07 17:25:42 +02:00
18 changed files with 289 additions and 175 deletions
+8 -8
View File
@@ -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
+2 -2
View File
@@ -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")
}
+104 -3
View File
@@ -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) {
+4 -4
View File
@@ -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
+16 -21
View File
@@ -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
}
}
+13 -11
View File
@@ -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)
})
+15 -28
View File
@@ -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 <path>: <err>" 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
}
+7 -8
View File
@@ -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
+3 -3
View File
@@ -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()
+9 -11
View File
@@ -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 <code>"
// 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)
}
+4 -7
View File
@@ -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
+13 -16
View File
@@ -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
+1 -1
View File
@@ -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")
}
+11 -7
View File
@@ -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")
}
+37 -27
View File
@@ -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
+28 -2
View File
@@ -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,32 @@ 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")
}
// TestGPGFailureKeepsStderr puts a fake gpg first on PATH that writes to
// stderr and exits non-zero, and checks that what it wrote ends the message.
func TestGPGFailureKeepsStderr(t *testing.T) {
t.Setenv("PATH", fakeGPGPath(t,
"#!/bin/sh\necho 'gpg: signing failed: No secret key' >&2\nexit 2\n"))
_, _, err := gpgSign(context.Background(), []byte("data"), GPGKeyID("any"))
assert.EqualError(t, err,
"gpg sign: exit status 2: gpg: signing failed: No secret key")
}
// TestGPGCancelWhenChildHoldsOutput uses a fake gpg that runs sleep as a
+2 -2
View File
@@ -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())
+12 -14
View File
@@ -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