diff --git a/README.md b/README.md index ec05d47..6cd74c3 100644 --- a/README.md +++ b/README.md @@ -404,7 +404,14 @@ proto `go_package` option. Which is canonical? - [ ] Replace GPG subprocess calls with pure-Go crypto (pending design question answer) — current implementation shells out to `gpg` which may not be installed -- [ ] Add timeout to any remaining subprocess calls +- [x] Add timeout to any remaining subprocess calls — every `gpg` invocation now + runs under a real deadline (`gpgTimeout`, or an earlier caller deadline), + with a process-group kill and a `Cmd.WaitDelay` backstop so a helper + process holding the output pipes cannot outlive it. The library entry + points that can reach `gpg` (`Builder.Build`, `NewManifestFromReader`, + `NewManifestFromFile`, `NewChecker`, + `Checker.ExtractEmbeddedSigningKeyFP`) take a `context.Context` as their + first argument so cancellation propagates from above. ### CLI diff --git a/TODO.md b/TODO.md index 06f9dba..b63b5ac 100644 --- a/TODO.md +++ b/TODO.md @@ -24,6 +24,11 @@ only thing left of the `chore/align-repo-policies` branch is the list below. # Completed Steps +- 2026-09-03: enforced real deadlines on every gpg subprocess call + (`gpgTimeout`, process-group kill, `Cmd.WaitDelay`) and threaded + `context.Context` through the library entry points that can reach gpg, + withdrawing the "signing exec is not cancellable" `//nolint` in + `mfer/scanner.go` (#62) - 2026-08-09: added `.prettierrc`/`.prettierignore`, gave `script/fmt` and `script/fmt-check` one shared prettier file set via `script/prettier`, dropped the `|| true` that hid prettier failures, and added a node-based Dockerfile @@ -92,8 +97,7 @@ only thing left of the `chore/align-repo-policies` branch is the list below. - Add decompression size limit via io.LimitReader in deserializeInner() - Fix errors.Is dead code in checker; make AddFile verify totalRead == size - Export manifest type or define a public interface (pending) - - Replace GPG subprocess with pure-Go crypto (pending); add timeouts to - remaining subprocess calls + - Replace GPG subprocess with pure-Go crypto (pending) - CLI: - Kebab-case primary flag names; fix fetch URL construction with url.JoinPath; add http.Client timeout and retry with backoff to fetch; diff --git a/internal/cli/check.go b/internal/cli/check.go index 459465e..ab3b900 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -2,6 +2,7 @@ package cli import ( + "context" "encoding/hex" "errors" "fmt" @@ -126,7 +127,9 @@ func (mfa *CLIApp) fetchManifestToTemp(url string) (string, error) { // verifyRequiredSigner enforces the --require-signature fingerprint // against the manifest's embedded signing key. -func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { +func verifyRequiredSigner( + ctx context.Context, chk *mfer.Checker, requiredSigner string, +) error { // Validate fingerprint format: must be exactly 40 hex characters if len(requiredSigner) != fingerprintHexLen { return fmt.Errorf("%w, got %d", errInvalidFingerprint, len(requiredSigner)) @@ -145,7 +148,7 @@ func verifyRequiredSigner(chk *mfer.Checker, requiredSigner string) error { // Extract fingerprint from the embedded public key (not from the // signer field). This validates the key is importable and gets its // actual fingerprint. - embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP() + embeddedFP, err := chk.ExtractEmbeddedSigningKeyFP(ctx) if err != nil { return fmt.Errorf( "failed to extract fingerprint from embedded signing key: %w", err) @@ -295,7 +298,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { log.Infof("checking manifest %s with base %s", manifestPath, basePath) // Create checker - chk, err := mfer.NewChecker(manifestPath, basePath, mfa.Fs) + chk, err := mfer.NewChecker(ctx.Context, manifestPath, basePath, mfa.Fs) if err != nil { return fmt.Errorf("failed to load manifest: %w", err) } @@ -303,7 +306,7 @@ func (mfa *CLIApp) checkManifestOperation(ctx *cli.Context) error { // Check signature requirement requiredSigner := ctx.String("require-signature") if requiredSigner != "" { - err = verifyRequiredSigner(chk, requiredSigner) + err = verifyRequiredSigner(ctx.Context, chk, requiredSigner) if err != nil { return err } diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index 61868ac..8b3e389 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -3,6 +3,7 @@ package cli import ( "bytes" + "context" "errors" "fmt" "math/rand" @@ -283,7 +284,7 @@ func TestGenerateExcludesDotfilesByDefault(t *testing.T) { assert.True(t, exists) // Verify manifest only has 1 file (the non-dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(context.Background(), fs, testMF) require.NoError(t, err) assert.Len(t, manifest.Files(), 1) assert.Equal(t, "file1.txt", manifest.Files()[0].GetPath()) @@ -307,7 +308,7 @@ func TestGenerateWithIncludeDotfiles(t *testing.T) { require.Equal(t, 0, exitCode) // Verify manifest has 2 files (including dotfile) - manifest, err := mfer.NewManifestFromFile(fs, testMF) + manifest, err := mfer.NewManifestFromFile(context.Background(), fs, testMF) require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } diff --git a/internal/cli/export.go b/internal/cli/export.go index a557137..1889e63 100644 --- a/internal/cli/export.go +++ b/internal/cli/export.go @@ -32,7 +32,7 @@ func (mfa *CLIApp) exportManifestOperation(ctx *cli.Context) error { defer func() { _ = rc.Close() }() - manifest, err := mfer.NewManifestFromReader(rc) + manifest, err := mfer.NewManifestFromReader(ctx.Context, rc) if err != nil { return fmt.Errorf("export: failed to parse manifest: %w", err) } diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 8254a4c..6811118 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -183,7 +183,7 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error { } // Parse manifest - manifest, err := mfer.NewManifestFromReader(resp.Body) + manifest, err := mfer.NewManifestFromReader(ctx.Context, resp.Body) if err != nil { return fmt.Errorf("failed to parse manifest: %w", err) } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index dd7a3e8..f18169d 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -244,7 +244,8 @@ func TestFetchFromHTTP(t *testing.T) { destDir := chdirTemp(t) // Parse the manifest to get file entries - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -293,7 +294,8 @@ func TestFetchHashMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -339,7 +341,8 @@ func TestFetchSizeMismatch(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() @@ -381,7 +384,8 @@ func TestFetchProgress(t *testing.T) { // Generate and parse manifest manifestData := scanToManifest(t, sourceFs) - manifest, err := mfer.NewManifestFromReader(bytes.NewReader(manifestData)) + manifest, err := mfer.NewManifestFromReader( + context.Background(), bytes.NewReader(manifestData)) require.NoError(t, err) files := manifest.Files() diff --git a/internal/cli/freshen.go b/internal/cli/freshen.go index bc0ee20..179110a 100644 --- a/internal/cli/freshen.go +++ b/internal/cli/freshen.go @@ -1,6 +1,7 @@ package cli import ( + "context" "crypto/sha256" "errors" "fmt" @@ -304,7 +305,8 @@ func (h *freshenHasher) processEntry(e *freshenEntry) error { // writeFreshenedManifest writes the manifest atomically (write to a // temp file, then rename over the target). func writeFreshenedManifest( - afs afero.Fs, builder *mfer.Builder, manifestPath string, + ctx context.Context, afs afero.Fs, builder *mfer.Builder, + manifestPath string, ) error { tmpPath := manifestPath + ".tmp" @@ -313,7 +315,7 @@ func writeFreshenedManifest( return fmt.Errorf("failed to create temp file: %w", err) } - err = builder.Build(outFile) + err = builder.Build(ctx, outFile) _ = outFile.Close() if err != nil { @@ -439,12 +441,12 @@ func runFreshenHash( // loadExistingEntries loads the manifest and indexes its file entries // by path. func (mfa *CLIApp) loadExistingEntries( - manifestPath string, + ctx context.Context, manifestPath string, ) (map[string]*mfer.MFFilePath, error) { log.Infof("loading manifest from %s", manifestPath) // Load existing manifest - manifest, err := mfer.NewManifestFromFile(mfa.Fs, manifestPath) + manifest, err := mfer.NewManifestFromFile(ctx, mfa.Fs, manifestPath) if err != nil { return nil, fmt.Errorf("failed to load manifest: %w", err) } @@ -473,7 +475,7 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { return fmt.Errorf("freshen: %w", err) } - existingByPath, err := mfa.loadExistingEntries(manifestPath) + existingByPath, err := mfa.loadExistingEntries(ctx.Context, manifestPath) if err != nil { return err } @@ -530,7 +532,8 @@ func (mfa *CLIApp) freshenManifestOperation(ctx *cli.Context) error { } // Write updated manifest atomically (write to temp, then rename) - err = writeFreshenedManifest(mfa.Fs, hasher.builder, manifestPath) + err = writeFreshenedManifest( + ctx.Context, mfa.Fs, hasher.builder, manifestPath) if err != nil { return err } diff --git a/internal/cli/freshen_test.go b/internal/cli/freshen_test.go index e5d844d..9ee72fc 100644 --- a/internal/cli/freshen_test.go +++ b/internal/cli/freshen_test.go @@ -58,7 +58,8 @@ func TestFreshenUnchanged(t *testing.T) { setupFreshenDir(t, fs) // Parse manifest to verify - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile( + context.Background(), fs, "/testdir/.index.mf") require.NoError(t, err) assert.Len(t, manifest.Files(), 2) } @@ -70,7 +71,8 @@ func TestFreshenWithChanges(t *testing.T) { setupFreshenDir(t, fs) // Verify initial manifest has 2 files - manifest, err := mfer.NewManifestFromFile(fs, "/testdir/.index.mf") + manifest, err := mfer.NewManifestFromFile( + context.Background(), fs, "/testdir/.index.mf") require.NoError(t, err) assert.Len(t, manifest.Files(), 2) diff --git a/internal/cli/list.go b/internal/cli/list.go index dbf2096..f883283 100644 --- a/internal/cli/list.go +++ b/internal/cli/list.go @@ -28,7 +28,7 @@ func (mfa *CLIApp) listManifestOperation(ctx *cli.Context) error { defer func() { _ = rc.Close() }() - manifest, err := mfer.NewManifestFromReader(rc) + manifest, err := mfer.NewManifestFromReader(ctx.Context, rc) if err != nil { return fmt.Errorf("list: failed to parse manifest: %w", err) } diff --git a/mfer/builder.go b/mfer/builder.go index b93f79e..5e1524e 100644 --- a/mfer/builder.go +++ b/mfer/builder.go @@ -3,6 +3,7 @@ package mfer import ( + "context" "crypto/sha256" "errors" "fmt" @@ -282,7 +283,10 @@ func (b *Builder) SetSigningOptions(opts *SigningOptions) { } // Build finalizes the manifest and writes it to the writer. -func (b *Builder) Build(w io.Writer) error { +// +// When signing options are set, Build shells out to gpg; ctx bounds +// those invocations and cancels them if it is cancelled. +func (b *Builder) Build(ctx context.Context, w io.Writer) error { b.mu.Lock() defer b.mu.Unlock() @@ -308,13 +312,13 @@ func (b *Builder) Build(w io.Writer) error { } // Generate outer wrapper - err := m.generateOuter() + err := m.generateOuter(ctx) if err != nil { return fmt.Errorf("build: generate outer: %w", err) } // Generate final output - err = m.generate() + err = m.generate(ctx) if err != nil { return fmt.Errorf("build: generate: %w", err) } diff --git a/mfer/builder_test.go b/mfer/builder_test.go index 7c20e9c..d7d9aca 100644 --- a/mfer/builder_test.go +++ b/mfer/builder_test.go @@ -3,6 +3,7 @@ package mfer import ( "bytes" + "context" "strings" "testing" "time" @@ -113,7 +114,7 @@ func TestBuilderBuild(t *testing.T) { var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Should have magic bytes @@ -177,7 +178,7 @@ func TestBuilderDeterministicOutput(t *testing.T) { var buf bytes.Buffer - err := b.Build(&buf) + err := b.Build(context.Background(), &buf) require.NoError(t, err) return buf.Bytes() @@ -325,9 +326,9 @@ func TestBuilderBuildRoundTrip(t *testing.T) { } var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) mfiles := m.Files() @@ -351,7 +352,8 @@ func TestBuilderBuildRoundTrip(t *testing.T) { func TestNewManifestFromReaderInvalidMagic(t *testing.T) { t.Parallel() - _, err := NewManifestFromReader(bytes.NewReader([]byte("NOT_VALID"))) + _, err := NewManifestFromReader( + context.Background(), bytes.NewReader([]byte("NOT_VALID"))) require.Error(t, err) assert.Contains(t, err.Error(), "invalid file format") } @@ -359,7 +361,7 @@ func TestNewManifestFromReaderInvalidMagic(t *testing.T) { func TestNewManifestFromReaderEmpty(t *testing.T) { t.Parallel() - _, err := NewManifestFromReader(bytes.NewReader([]byte{})) + _, err := NewManifestFromReader(context.Background(), bytes.NewReader([]byte{})) assert.Error(t, err) } @@ -367,7 +369,7 @@ func TestNewManifestFromReaderTruncated(t *testing.T) { t.Parallel() // Just the magic with nothing after - _, err := NewManifestFromReader(bytes.NewReader([]byte(MAGIC))) + _, err := NewManifestFromReader(context.Background(), bytes.NewReader([]byte(MAGIC))) assert.Error(t, err) } @@ -383,9 +385,9 @@ func TestManifestString(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.Contains(t, m.String(), "count=1") } @@ -397,7 +399,7 @@ func TestBuilderBuildEmpty(t *testing.T) { var buf bytes.Buffer - err := b.Build(&buf) + err := b.Build(context.Background(), &buf) require.NoError(t, err) // Should still produce valid manifest with 0 files @@ -416,9 +418,9 @@ func TestBuilderOmitsCreatedAtByDefault(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.Nil(t, m.pbInner.GetCreatedAt(), "createdAt should be nil by default for deterministic output") @@ -438,9 +440,9 @@ func TestBuilderIncludesCreatedAtWhenRequested(t *testing.T) { require.NoError(t, err) var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) + require.NoError(t, b.Build(context.Background(), &buf)) - m, err := NewManifestFromReader(&buf) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.NotNil(t, m.pbInner.GetCreatedAt(), "createdAt should be set when IncludeTimestamps is true") @@ -464,8 +466,8 @@ func TestBuilderDeterministicFileOrder(t *testing.T) { } var buf bytes.Buffer - require.NoError(t, b.Build(&buf)) - m, err := NewManifestFromReader(&buf) + require.NoError(t, b.Build(context.Background(), &buf)) + m, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) return m.Files() diff --git a/mfer/checker.go b/mfer/checker.go index fe5969e..db928e4 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -83,14 +83,19 @@ type Checker struct { } // NewChecker creates a new Checker for the given manifest, base path, and filesystem. +// +// Loading a signed manifest verifies its signature, which shells out to +// gpg; ctx bounds that invocation. // The basePath is the directory relative to which manifest paths are resolved. // If fs is nil, the real filesystem (OsFs) is used. -func NewChecker(manifestPath string, basePath string, fs afero.Fs) (*Checker, error) { +func NewChecker( + ctx context.Context, manifestPath string, basePath string, fs afero.Fs, +) (*Checker, error) { if fs == nil { fs = afero.NewOsFs() } - m, err := NewManifestFromFile(fs, manifestPath) + m, err := NewManifestFromFile(ctx, fs, manifestPath) if err != nil { return nil, err } @@ -164,12 +169,16 @@ func (c *Checker) SigningPubKey() []byte { // ExtractEmbeddedSigningKeyFP imports the manifest's embedded public key into a // temporary keyring and extracts its fingerprint. This validates the key and // returns its actual fingerprint from the key material itself. -func (c *Checker) ExtractEmbeddedSigningKeyFP() (string, error) { +// +// The import shells out to gpg; ctx bounds that invocation. +func (c *Checker) ExtractEmbeddedSigningKeyFP( + ctx context.Context, +) (string, error) { if len(c.signingPubKey) == 0 { return "", errNoSigningPubKey } - return gpgExtractPubKeyFingerprint(c.signingPubKey) + return gpgExtractPubKeyFingerprint(ctx, c.signingPubKey) } // Check verifies all files against the manifest. diff --git a/mfer/checker_test.go b/mfer/checker_test.go index 1e79218..35b79f3 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -61,7 +61,7 @@ func createTestManifest( } var buf bytes.Buffer - require.NoError(t, builder.Build(&buf)) + require.NoError(t, builder.Build(context.Background(), &buf)) require.NoError(t, afero.WriteFile(fs, manifestPath, buf.Bytes(), 0o644)) } @@ -92,7 +92,7 @@ func TestNewChecker(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/", fs) require.NoError(t, err) assert.NotNil(t, chk) assert.Equal(t, FileCount(2), chk.FileCount()) @@ -102,7 +102,7 @@ func TestNewChecker(t *testing.T) { t.Parallel() fs := afero.NewMemMapFs() - _, err := NewChecker("/nonexistent.mf", "/", fs) + _, err := NewChecker(context.Background(), "/nonexistent.mf", "/", fs) assert.Error(t, err) }) @@ -111,7 +111,7 @@ func TestNewChecker(t *testing.T) { fs := afero.NewMemMapFs() require.NoError(t, afero.WriteFile(fs, "/bad.mf", []byte("not a manifest"), 0o644)) - _, err := NewChecker("/bad.mf", "/", fs) + _, err := NewChecker(context.Background(), "/bad.mf", "/", fs) assert.Error(t, err) }) } @@ -127,7 +127,7 @@ func TestCheckerFileCountAndTotalBytes(t *testing.T) { } createTestManifest(t, fs, "/manifest.mf", files) - chk, err := NewChecker("/manifest.mf", "/", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/", fs) require.NoError(t, err) assert.Equal(t, FileCount(3), chk.FileCount()) @@ -145,7 +145,7 @@ func TestCheckAllFilesOK(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -178,7 +178,7 @@ func TestCheckMissingFile(t *testing.T) { testExistsFile: []byte("I exist"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -217,7 +217,7 @@ func TestCheckSizeMismatch(t *testing.T) { testFileName: []byte("short"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -245,7 +245,7 @@ func TestCheckHashMismatch(t *testing.T) { testFileName: differentContent, }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -268,7 +268,7 @@ func TestCheckWithProgress(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -308,7 +308,7 @@ func TestCheckContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -335,7 +335,7 @@ func TestFindExtraFiles(t *testing.T) { testFile2: []byte("extra file"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -371,7 +371,7 @@ func TestFindExtraFilesSkipsManifestAndDotfiles(t *testing.T) { require.NoError(t, fs.MkdirAll("/data", 0o755)) require.NoError(t, afero.WriteFile(fs, "/data/extra.txt", []byte("extra"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/.index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -403,7 +403,7 @@ func TestFindExtraFilesContextCancellation(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) ctx, cancel := context.WithCancel(context.Background()) @@ -422,7 +422,7 @@ func TestCheckNilChannels(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) // Should not panic with nil channels @@ -438,7 +438,7 @@ func TestFindExtraFilesNilChannel(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) // Should not panic with nil channel @@ -465,7 +465,7 @@ func TestCheckSubdirectories(t *testing.T) { require.NoError(t, afero.WriteFile(fs, fullPath, content, 0o644)) } - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -499,7 +499,7 @@ func TestCheckMissingFileDetectedWithoutFallback(t *testing.T) { testExistsFile: []byte("here"), }) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -537,7 +537,7 @@ func TestFindExtraFilesSkipsDotfiles(t *testing.T) { require.NoError(t, afero.WriteFile(fs, "/data/.git/config", []byte("git config"), 0o644)) - chk, err := NewChecker("/data/.index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/.index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -566,7 +566,7 @@ func TestFindExtraFilesSkipsManifestFile(t *testing.T) { createTestManifest(t, fs, "/data/index.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/data/index.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/data/index.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 10) @@ -589,7 +589,7 @@ func TestCheckEmptyManifest(t *testing.T) { // Create manifest with no files createTestManifest(t, fs, "/manifest.mf", map[string][]byte{}) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) assert.Equal(t, FileCount(0), chk.FileCount()) @@ -624,7 +624,7 @@ func TestCheckProgressRateLimited(t *testing.T) { createTestManifest(t, fs, "/manifest.mf", files) createFilesOnDisk(t, fs, files) - chk, err := NewChecker("/manifest.mf", "/data", fs) + chk, err := NewChecker(context.Background(), "/manifest.mf", "/data", fs) require.NoError(t, err) results := make(chan Result, 200) diff --git a/mfer/deserialize.go b/mfer/deserialize.go index a099d4a..383d5c0 100644 --- a/mfer/deserialize.go +++ b/mfer/deserialize.go @@ -2,6 +2,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "errors" "fmt" @@ -63,7 +64,7 @@ func (m *manifest) validateOuterHeader() error { // verifyOuterIntegrity checks the hash of the compressed payload and, // if a signature is present, verifies it against the embedded public key. -func (m *manifest) verifyOuterIntegrity() error { +func (m *manifest) verifyOuterIntegrity(ctx context.Context) error { h := sha256.New() _, err := h.Write(m.pbOuter.GetInnerMessage()) @@ -92,6 +93,7 @@ func (m *manifest) verifyOuterIntegrity() error { } err = gpgVerify( + ctx, []byte(sigString), m.pbOuter.GetSignature(), m.pbOuter.GetSigningPubKey(), @@ -139,13 +141,13 @@ func (m *manifest) decompressInner() ([]byte, error) { return dat, nil } -func (m *manifest) deserializeInner() error { +func (m *manifest) deserializeInner(ctx context.Context) error { err := m.validateOuterHeader() if err != nil { return err } - err = m.verifyOuterIntegrity() + err = m.verifyOuterIntegrity(ctx) if err != nil { return err } @@ -200,8 +202,13 @@ func validateMagic(dat []byte) bool { // NewManifestFromReader reads a manifest from an io.Reader. // +// A signed manifest is verified as it is loaded, which shells out to +// gpg; ctx bounds that invocation and cancels it if it is cancelled. +// //nolint:revive // unexported-return: exporting manifest is owner question 13 -func NewManifestFromReader(input io.Reader) (*manifest, error) { +func NewManifestFromReader( + ctx context.Context, input io.Reader, +) (*manifest, error) { m := &manifest{} dat, err := io.ReadAll(input) @@ -227,7 +234,7 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { } // deserialize inner: - err = m.deserializeInner() + err = m.deserializeInner(ctx) if err != nil { return nil, err } @@ -239,7 +246,9 @@ func NewManifestFromReader(input io.Reader) (*manifest, error) { // If fs is nil, the real filesystem (OsFs) is used. // //nolint:revive // unexported-return: exporting manifest is owner question 13 -func NewManifestFromFile(fs afero.Fs, path string) (*manifest, error) { +func NewManifestFromFile( + ctx context.Context, fs afero.Fs, path string, +) (*manifest, error) { if fs == nil { fs = afero.NewOsFs() } @@ -251,5 +260,5 @@ func NewManifestFromFile(fs afero.Fs, path string) (*manifest, error) { defer func() { _ = f.Close() }() - return NewManifestFromReader(f) + return NewManifestFromReader(ctx, f) } diff --git a/mfer/errmsg_test.go b/mfer/errmsg_test.go index f07e06c..d2dc32c 100644 --- a/mfer/errmsg_test.go +++ b/mfer/errmsg_test.go @@ -2,6 +2,7 @@ package mfer import ( + "context" "testing" "github.com/stretchr/testify/assert" @@ -80,6 +81,7 @@ func TestSerializeInternalErrorMessagesVerbatim(t *testing.T) { t.Parallel() m := &manifest{} - require.EqualError(t, m.generate(), "internal error: pbInner not set") - require.EqualError(t, m.generateOuter(), "internal error") + require.EqualError(t, m.generate(context.Background()), + "internal error: pbInner not set") + require.EqualError(t, m.generateOuter(context.Background()), "internal error") } diff --git a/mfer/gpg.go b/mfer/gpg.go index 568d672..70b3ebe 100644 --- a/mfer/gpg.go +++ b/mfer/gpg.go @@ -10,6 +10,7 @@ import ( "os/exec" "path/filepath" "strings" + "time" ) const ( @@ -33,12 +34,51 @@ const ( gpgOptArmor = "--armor" gpgOptHomedir = "--homedir" gpgOptVerify = "--verify" + + // gpgTimeout bounds a single gpg invocation. + // + // Every invocation runs with --batch --no-tty, so gpg never waits on + // human input: a missing passphrase or an unusable pinentry makes it + // fail immediately instead of blocking. The legitimately slow paths + // that remain are a cold gpg-agent start, hardware-token crypto, and + // entropy starvation on a freshly booted VM -- all low single-digit + // seconds. 30s leaves an order of magnitude of headroom over that + // while still bounding a wedged agent to something that fits inside a + // normal CI step instead of hanging forever. + // + // Callers needing a tighter bound pass a ctx with an earlier + // deadline; runGPG honours whichever of the two comes first. + gpgTimeout = 30 * time.Second + + // gpgWaitDelay bounds how long Cmd.Wait keeps waiting after the + // invocation has been cancelled. + // + // Cancellation kills gpg, but any descendant that inherited the write + // end of our stdout or stderr pipe keeps it open, and Wait blocks on + // the output-copying goroutines until every writer is gone. Without a + // WaitDelay that turns the deadline into no deadline at all: gpg is + // dead and Run still never returns. Once this delay elapses Cmd + // force-closes the pipes and Wait returns. gpg itself dies to SIGKILL + // at once, so this only ever elapses when something downstream is + // still holding a pipe. + gpgWaitDelay = 2 * time.Second + + // gpg operation labels. These name the failing invocation in timeout + // and cancellation errors, which the os/exec layer would otherwise + // report as a bare "signal: killed". + gpgOpSign = "sign" + gpgOpExport = "export" + gpgOpFingerprint = "fingerprint" + gpgOpImport = "import" + gpgOpListKeys = "list-keys" + gpgOpVerify = "verify" ) var ( errGPGKeyNotFound = errors.New("gpg key not found") errFingerprintNotFound = errors.New("fingerprint not found for key") errImportedFPRNotFound = errors.New("fingerprint not found in imported key") + errGPGTimeout = errors.New("gpg invocation timed out") ) // GPGKeyID represents a GPG key identifier (fingerprint or key ID). @@ -67,25 +107,53 @@ 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. -func runGPG(stdin io.Reader, args ...string) (*bytes.Buffer, *bytes.Buffer, error) { +// +// The invocation is bounded by gpgTimeout, or by ctx's own deadline when +// that is sooner. op names the operation for diagnostics; see the gpgOp* +// constants. +func runGPG( + ctx context.Context, op string, stdin io.Reader, args ...string, +) (*bytes.Buffer, *bytes.Buffer, error) { fullArgs := append([]string{"--batch", "--no-tty"}, args...) + runCtx, cancel := context.WithTimeout(ctx, gpgTimeout) + defer cancel() + // G204: the executable name is a compile-time constant. The arguments // are not, so the guarantee that matters is placement: every // caller-supplied value is passed either as the value of a named // option or after the "--" end-of-options marker inserted by gpgArgs, // and therefore cannot be reinterpreted by gpg as an option. cmd := exec.CommandContext( //nolint:gosec // G204: see comment above - context.Background(), "gpg", fullArgs...) + runCtx, "gpg", fullArgs...) cmd.Stdin = stdin + cmd.WaitDelay = gpgWaitDelay + + setGPGProcessGroup(cmd) var stdout, stderr bytes.Buffer cmd.Stdout = &stdout cmd.Stderr = &stderr + started := time.Now() err := cmd.Run() + // A killed subprocess reports only "signal: killed", which names + // neither the operation nor the reason, and the bare context error is + // no better. Say which invocation died and why. Whether the deadline + // that fired was gpgTimeout or an earlier one from the caller does not + // change the diagnosis, so report how long gpg actually ran rather + // than guessing which limit applied. + switch { + case err == nil: + case errors.Is(runCtx.Err(), context.DeadlineExceeded): + err = fmt.Errorf("%w: operation %q killed after %s", + errGPGTimeout, op, time.Since(started).Round(time.Millisecond)) + case errors.Is(runCtx.Err(), context.Canceled): + err = fmt.Errorf("gpg %s cancelled: %w", op, runCtx.Err()) + } + return &stdout, &stderr, err } @@ -105,8 +173,10 @@ func parseFingerprint(colonOutput string) (string, bool) { // gpgSign creates a detached signature of the data using the specified key. // Returns the armored detached signature. -func gpgSign(data []byte, keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(bytes.NewReader(data), +func gpgSign( + ctx context.Context, data []byte, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpSign, bytes.NewReader(data), "--detach-sign", gpgOptArmor, "--local-user", string(keyID), @@ -120,8 +190,10 @@ func gpgSign(data []byte, keyID GPGKeyID) ([]byte, error) { // gpgExportPublicKey exports the public key for the specified key ID. // Returns the armored public key. -func gpgExportPublicKey(keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(nil, +func gpgExportPublicKey( + ctx context.Context, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpExport, nil, gpgArgs([]string{"--export", gpgOptArmor}, string(keyID))..., ) if err != nil { @@ -136,8 +208,10 @@ func gpgExportPublicKey(keyID GPGKeyID) ([]byte, error) { } // gpgGetKeyFingerprint gets the full fingerprint for a key ID. -func gpgGetKeyFingerprint(keyID GPGKeyID) ([]byte, error) { - stdout, stderr, err := runGPG(nil, +func gpgGetKeyFingerprint( + ctx context.Context, keyID GPGKeyID, +) ([]byte, error) { + stdout, stderr, err := runGPG(ctx, gpgOpFingerprint, nil, gpgArgs([]string{"--with-colons", "--fingerprint"}, string(keyID))..., ) if err != nil { @@ -157,7 +231,9 @@ func gpgGetKeyFingerprint(keyID GPGKeyID) ([]byte, error) { // gpgExtractPubKeyFingerprint imports a public key into a temporary keyring // and extracts its fingerprint. This verifies the key is valid and returns // the actual fingerprint from the key material. -func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { +func gpgExtractPubKeyFingerprint( + ctx context.Context, pubKey []byte, +) (string, error) { // Create temporary directory for GPG operations tmpDir, err := os.MkdirTemp("", "mfer-gpg-fingerprint-*") if err != nil { @@ -181,7 +257,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, gpgOpImport, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -191,7 +267,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { } // List keys to get fingerprint - listStdout, listStderr, err := runGPG(nil, + listStdout, listStderr, err := runGPG(ctx, gpgOpListKeys, nil, "--homedir", tmpDir, "--with-colons", "--fingerprint", @@ -212,7 +288,7 @@ func gpgExtractPubKeyFingerprint(pubKey []byte) (string, error) { // gpgVerify verifies a detached signature against data using the provided public key. // It creates a temporary keyring to import the public key for verification. -func gpgVerify(data, signature, pubKey []byte) error { +func gpgVerify(ctx context.Context, data, signature, pubKey []byte) error { // Create temporary directory for GPG operations tmpDir, err := os.MkdirTemp("", "mfer-gpg-verify-*") if err != nil { @@ -252,7 +328,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Import the public key into the temporary keyring - _, importStderr, err := runGPG(nil, + _, importStderr, err := runGPG(ctx, gpgOpImport, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, "--import"}, pubKeyFile)..., ) if err != nil { @@ -262,7 +338,7 @@ func gpgVerify(data, signature, pubKey []byte) error { } // Verify the signature - _, verifyStderr, err := runGPG(nil, + _, verifyStderr, err := runGPG(ctx, gpgOpVerify, nil, gpgArgs([]string{gpgOptHomedir, tmpDir, gpgOptVerify}, sigFile, dataFile)..., ) diff --git a/mfer/gpg_procgroup_other.go b/mfer/gpg_procgroup_other.go new file mode 100644 index 0000000..bb3842e --- /dev/null +++ b/mfer/gpg_procgroup_other.go @@ -0,0 +1,11 @@ +//go:build !unix + +package mfer + +import "os/exec" + +// setGPGProcessGroup is a no-op on platforms without POSIX process +// groups. The deadline is still enforced there: cancellation kills the +// gpg process itself and Cmd.WaitDelay still bounds Cmd.Wait. Only the +// group kill of helper processes gpg may have started is unavailable. +func setGPGProcessGroup(_ *exec.Cmd) {} diff --git a/mfer/gpg_procgroup_unix.go b/mfer/gpg_procgroup_unix.go new file mode 100644 index 0000000..8f10bfd --- /dev/null +++ b/mfer/gpg_procgroup_unix.go @@ -0,0 +1,36 @@ +//go:build unix + +package mfer + +import ( + "os/exec" + "syscall" +) + +// setGPGProcessGroup puts gpg in its own process group and makes +// cancellation kill that whole group rather than gpg alone. +// +// Go's default cancel function signals only the direct child. gpg +// routinely delegates to helpers (gpg-agent, pinentry, scdaemon) and +// starts them itself on first use; killing gpg alone leaves those +// running, still holding the write end of our stdout and stderr pipes, +// which is precisely what would keep Cmd.Wait blocked past the deadline. +// Killing the group kills everything gpg started that has not +// deliberately left it. +// +// A gpg-agent that was already running is unaffected: it calls setsid at +// startup and so is never a member of our group. Nor can we guarantee +// every future helper stays in the group. That is why gpgWaitDelay +// remains necessary as a backstop rather than being made redundant by +// this. +func setGPGProcessGroup(cmd *exec.Cmd) { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + + cmd.Cancel = func() error { + // A negative pid targets the process group. SIGKILL rather than + // SIGINT: gpg blocked on an agent socket does not reliably act + // on SIGINT, and by the time Cancel runs the deadline has + // already expired, so there is nothing left to be polite about. + return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) + } +} diff --git a/mfer/gpg_test.go b/mfer/gpg_test.go index 43aa9d6..8a069f5 100644 --- a/mfer/gpg_test.go +++ b/mfer/gpg_test.go @@ -9,12 +9,55 @@ import ( "path/filepath" "strings" "testing" + "time" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +// gpgTestKeygenTimeout bounds the gpg invocations that build the test +// keyring. Generating an RSA-2048 key also starts gpg-agent, which is +// far slower than any operation under test (measured at well under a +// second with a working agent). If it has not finished by now the +// environment is not going to produce a key at all, and the test should +// skip rather than consume the whole `go test` budget. +const gpgTestKeygenTimeout = 5 * time.Second + +// runTestGPG runs gpg for test setup under a real deadline, returning +// captured stdout and stderr. +// +// This mirrors runGPG's hardening rather than calling it: setup needs its +// own GNUPGHOME, and it runs commands (--gen-key) that no production call +// site issues. The deadline matters as much here as in production -- +// keygen starts gpg-agent, which inherits the output pipes, so without +// WaitDelay and the process-group kill a stalled agent hangs the suite +// instead of failing it. +func runTestGPG( + t *testing.T, gpgHome string, args ...string, +) (*bytes.Buffer, *bytes.Buffer, error) { + t.Helper() + + ctx, cancel := context.WithTimeout( + context.Background(), gpgTestKeygenTimeout) + defer cancel() + + //nolint:gosec // G204: every argument here is a test-controlled literal + cmd := exec.CommandContext(ctx, "gpg", args...) + + cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) + cmd.WaitDelay = gpgWaitDelay + + setGPGProcessGroup(cmd) + + var stdout, stderr bytes.Buffer + + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + return &stdout, &stderr, cmd.Run() +} + // testGPGEnv sets up a temporary GPG home directory with a test key. // Returns the key ID and the GPG home directory; callers must point // GNUPGHOME at the returned directory (via t.Setenv) before using the @@ -43,24 +86,16 @@ Expire-Date: 0 paramsFile := filepath.Join(gpgHome, "key-params") require.NoError(t, os.WriteFile(paramsFile, []byte(keyParams), 0o600)) - //nolint:gosec // paramsFile is a test-controlled path inside t.TempDir() - cmd := exec.CommandContext(context.Background(), "gpg", + _, genStderr, err := runTestGPG(t, gpgHome, "--batch", "--gen-key", paramsFile) - - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - - output, err := cmd.CombinedOutput() if err != nil { - t.Skipf("failed to generate test GPG key: %v: %s", err, output) + t.Skipf("failed to generate test GPG key: %v: %s", + err, genStderr.String()) } // Get the key fingerprint - cmd = exec.CommandContext(context.Background(), "gpg", + listStdout, _, err := runTestGPG(t, gpgHome, "--list-keys", "--with-colons", "test@mfer.test") - - cmd.Env = append(os.Environ(), "GNUPGHOME="+gpgHome) - - output, err = cmd.Output() if err != nil { t.Fatalf("failed to list test key: %v", err) } @@ -68,7 +103,7 @@ Expire-Date: 0 // Parse fingerprint from output var keyID string - for _, line := range strings.Split(string(output), "\n") { + for _, line := range strings.Split(listStdout.String(), "\n") { fields := strings.Split(line, ":") if len(fields) >= gpgFingerprintMinFields && fields[0] == gpgFingerprintField { @@ -90,7 +125,7 @@ func TestGPGSign(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) assert.NotEmpty(t, sig) assert.Contains(t, string(sig), "-----BEGIN PGP SIGNATURE-----") @@ -101,7 +136,7 @@ func TestGPGExportPublicKey(t *testing.T) { keyID, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) assert.NotEmpty(t, pubKey) assert.Contains(t, string(pubKey), "-----BEGIN PGP PUBLIC KEY BLOCK-----") @@ -112,7 +147,7 @@ func TestGPGGetKeyFingerprint(t *testing.T) { keyID, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - fingerprint, err := gpgGetKeyFingerprint(keyID) + fingerprint, err := gpgGetKeyFingerprint(context.Background(), keyID) require.NoError(t, err) assert.NotEmpty(t, fingerprint) // The fingerprint should be 40 hex chars @@ -146,12 +181,12 @@ func TestGPGOptionLikeKeyIDIsNotAnOption(t *testing.T) { _, gpgHome := testGPGEnv(t) t.Setenv("GNUPGHOME", gpgHome) - pubKey, err := gpgExportPublicKey(GPGKeyID("--version")) + pubKey, err := gpgExportPublicKey(context.Background(), GPGKeyID("--version")) require.Error(t, err) require.ErrorIs(t, err, errGPGKeyNotFound) assert.NotContains(t, string(pubKey), "gpg (GnuPG)") - fpr, err := gpgGetKeyFingerprint(GPGKeyID("--version")) + fpr, err := gpgGetKeyFingerprint(context.Background(), GPGKeyID("--version")) require.Error(t, err) assert.NotContains(t, string(fpr), "gpg (GnuPG)") } @@ -162,7 +197,7 @@ func TestGPGSignInvalidKey(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data") - _, err := gpgSign(data, GPGKeyID("NONEXISTENT_KEY_ID_12345")) + _, err := gpgSign(context.Background(), data, GPGKeyID("NONEXISTENT_KEY_ID_12345")) assert.Error(t, err) } @@ -185,11 +220,11 @@ func TestBuilderWithSigning(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest and verify signature fields are populated - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest.pbOuter) @@ -238,7 +273,7 @@ func TestScannerWithSigning(t *testing.T) { require.NoError(t, s.ToManifest(context.Background(), &buf, nil)) // Parse and verify - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) assert.NotEmpty(t, manifest.pbOuter.GetSignature()) @@ -251,14 +286,14 @@ func TestGPGVerify(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign and verify") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) // Verify the signature - err = gpgVerify(data, sig, pubKey) + err = gpgVerify(context.Background(), data, sig, pubKey) require.NoError(t, err) } @@ -267,15 +302,15 @@ func TestGPGVerifyInvalidSignature(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data to sign") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) - pubKey, err := gpgExportPublicKey(keyID) + pubKey, err := gpgExportPublicKey(context.Background(), keyID) require.NoError(t, err) // Try to verify with different data - should fail wrongData := []byte("different data") - err = gpgVerify(wrongData, sig, pubKey) + err = gpgVerify(context.Background(), wrongData, sig, pubKey) assert.Error(t, err) } @@ -284,12 +319,12 @@ func TestGPGVerifyBadPublicKey(t *testing.T) { t.Setenv("GNUPGHOME", gpgHome) data := []byte("test data") - sig, err := gpgSign(data, keyID) + sig, err := gpgSign(context.Background(), data, keyID) require.NoError(t, err) // Try to verify with invalid public key - should fail badPubKey := []byte("not a valid public key") - err = gpgVerify(data, sig, badPubKey) + err = gpgVerify(context.Background(), data, sig, badPubKey) assert.Error(t, err) } @@ -312,11 +347,11 @@ func TestManifestSignatureVerification(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest - signature should be verified during load - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest) @@ -341,7 +376,7 @@ func TestManifestTamperedSignatureFails(t *testing.T) { var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Tamper with the signature by replacing some bytes @@ -356,7 +391,7 @@ func TestManifestTamperedSignatureFails(t *testing.T) { } // Try to load the tampered manifest - should fail - _, err = NewManifestFromReader(bytes.NewReader(data)) + _, err = NewManifestFromReader(context.Background(), bytes.NewReader(data)) assert.Error(t, err) } @@ -375,11 +410,11 @@ func TestBuilderWithoutSigning(t *testing.T) { // Build the manifest var buf bytes.Buffer - err = b.Build(&buf) + err = b.Build(context.Background(), &buf) require.NoError(t, err) // Parse the manifest and verify signature fields are empty - manifest, err := NewManifestFromReader(&buf) + manifest, err := NewManifestFromReader(context.Background(), &buf) require.NoError(t, err) require.NotNil(t, manifest.pbOuter) diff --git a/mfer/gpg_timeout_unix_test.go b/mfer/gpg_timeout_unix_test.go new file mode 100644 index 0000000..d79e6ed --- /dev/null +++ b/mfer/gpg_timeout_unix_test.go @@ -0,0 +1,109 @@ +//go:build unix + +// The deadline tests need an executable stand-in for gpg first on PATH, +// which is written as a POSIX shell script, and they exercise the +// process-group kill that only exists on unix. There is no equivalent +// coverage on other platforms. + +//nolint:testpackage // white-box tests exercise unexported internals +package mfer + +import ( + "context" + "os" + "path/filepath" + "strconv" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + // gpgTestDeadline is the deadline the deadline tests give gpg. It is + // short so the suite stays fast; gpgTimeout itself is far too long to + // wait out in a test, and passing an earlier deadline is exactly the + // caller-propagation path under test. + gpgTestDeadline = 200 * time.Millisecond + + // fakeGPGSleepSeconds is how long the stand-in gpg blocks. It must be + // long enough that only cancellation can end it -- if the deadline is + // not enforced, the test hangs until `go test` times out, which is the + // loud failure we want rather than a silently passing race. + fakeGPGSleepSeconds = 30 + + // fakeGPGPerms makes the stand-in script executable. + fakeGPGPerms os.FileMode = 0o700 +) + +// fakeSlowGPGOnPath puts a stand-in for gpg first on PATH that blocks +// until it is killed, so a deadline is the only thing that can end the +// invocation. +// +// The stand-in is a shell script that spawns a child, which means the +// child inherits the write end of the captured stdout and stderr pipes. +// That mirrors a real gpg delegating to a helper process, and it is the +// case that hangs Cmd.Wait if only the direct child is killed. +func fakeSlowGPGOnPath(t *testing.T) { + t.Helper() + + dir := t.TempDir() + script := "#!/bin/sh\nsleep " + strconv.Itoa(fakeGPGSleepSeconds) + "\n" + + // fakeGPGPerms is 0700 rather than 0600 because the stand-in has to + // be executable to be run at all; it lives in a test-controlled + // t.TempDir(). + require.NoError(t, + os.WriteFile(filepath.Join(dir, "gpg"), []byte(script), fakeGPGPerms)) + + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + +// TestGPGDeadlineIsEnforced pins that a gpg invocation which outlives its +// deadline returns a timeout error naming the operation, rather than +// hanging or surfacing a bare context error. +// +//nolint:paralleltest // fakeSlowGPGOnPath mutates the process-global PATH +func TestGPGDeadlineIsEnforced(t *testing.T) { + fakeSlowGPGOnPath(t) + + ctx, cancel := context.WithTimeout(context.Background(), gpgTestDeadline) + defer cancel() + + started := time.Now() + sig, err := gpgSign(ctx, []byte("data to sign"), GPGKeyID("KEYID")) + elapsed := time.Since(started) + + require.Error(t, err) + require.ErrorIs(t, err, errGPGTimeout) + assert.Empty(t, sig) + + // The error must say which invocation stalled. "context deadline + // exceeded" alone tells an operator nothing. + assert.Contains(t, err.Error(), gpgOpSign) + + // Returning at all is the headline, but it has to return promptly: + // if the stand-in's child kept the output pipes open, Cmd.Wait would + // sit there until gpgWaitDelay elapsed. + assert.Less(t, elapsed, gpgWaitDelay, + "Run should return on cancellation, not wait out gpgWaitDelay") +} + +// TestGPGCancellationPropagates pins that a caller's cancellation reaches +// gpg and is reported as a cancellation rather than as a timeout. +// +//nolint:paralleltest // fakeSlowGPGOnPath mutates the process-global PATH +func TestGPGCancellationPropagates(t *testing.T) { + fakeSlowGPGOnPath(t) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := gpgSign(ctx, []byte("data to sign"), GPGKeyID("KEYID")) + + require.Error(t, err) + require.ErrorIs(t, err, context.Canceled) + require.NotErrorIs(t, err, errGPGTimeout) + assert.Contains(t, err.Error(), gpgOpSign) +} diff --git a/mfer/scanner.go b/mfer/scanner.go index 8d83051..ba131c3 100644 --- a/mfer/scanner.go +++ b/mfer/scanner.go @@ -282,9 +282,9 @@ func (s *Scanner) ToManifest( }) } - // Build and write manifest - //nolint:contextcheck // Build's GPG signing exec is not cancellable by design - return builder.Build(w) + // Build and write manifest. Build takes ctx because signing shells + // out to gpg, and those invocations are cancellable. + return builder.Build(ctx, w) } // configureBuilder constructs a manifest builder configured from the diff --git a/mfer/serialize.go b/mfer/serialize.go index 487d49b..69fbe95 100644 --- a/mfer/serialize.go +++ b/mfer/serialize.go @@ -2,6 +2,7 @@ package mfer import ( "bytes" + "context" "crypto/sha256" "errors" "fmt" @@ -50,13 +51,13 @@ func newTimestampFromTime(t time.Time) *Timestamp { } } -func (m *manifest) generate() error { +func (m *manifest) generate(ctx context.Context) error { if m.pbInner == nil { return errInnerNotSet } if m.pbOuter == nil { - e := m.generateOuter() + e := m.generateOuter(ctx) if e != nil { return e } @@ -77,7 +78,7 @@ func (m *manifest) generate() error { return nil } -func (m *manifest) generateOuter() error { +func (m *manifest) generateOuter(ctx context.Context) error { if m.pbInner == nil { return errInternal } @@ -135,7 +136,7 @@ func (m *manifest) generateOuter() error { // Sign the manifest if signing options are provided if m.signingOptions != nil && m.signingOptions.KeyID != "" { - return m.signOuter() + return m.signOuter(ctx) } return nil @@ -143,27 +144,27 @@ func (m *manifest) generateOuter() error { // signOuter signs the outer message with the configured GPG key and // embeds the signature, signer fingerprint, and public key. -func (m *manifest) signOuter() 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) } - sig, err := gpgSign([]byte(sigString), m.signingOptions.KeyID) + sig, err := gpgSign(ctx, []byte(sigString), m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to sign manifest: %w", err) } m.pbOuter.Signature = sig - fingerprint, err := gpgGetKeyFingerprint(m.signingOptions.KeyID) + fingerprint, err := gpgGetKeyFingerprint(ctx, m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to get key fingerprint: %w", err) } m.pbOuter.Signer = fingerprint - pubKey, err := gpgExportPublicKey(m.signingOptions.KeyID) + pubKey, err := gpgExportPublicKey(ctx, m.signingOptions.KeyID) if err != nil { return fmt.Errorf("failed to export public key: %w", err) }