Enforce real timeouts on gpg subprocess calls (closes #62)
check / check (push) Successful in 1m0s
check / check (push) Successful in 1m0s
Every gpg invocation went through runGPG, which built its command with exec.CommandContext(context.Background(), ...). That is the right call with the wrong context: context.Background() never expires, so no deadline was ever enforced on any of the five gpg call sites. runGPG now takes a context and derives a gpgTimeout deadline from it, honouring an earlier caller deadline when there is one. The gpg-touching library entry points take a ctx as their first argument so cancellation propagates from above: Builder.Build, NewManifestFromReader, NewManifestFromFile, NewChecker, Checker.ExtractEmbeddedSigningKeyFP. Scanner.ToManifest already had a ctx and now passes it down, which withdraws the //nolint:contextcheck claiming signing was "not cancellable by design" -- it is, and now it is. A deadline alone is not enough, and the added test proves it. gpg delegates to helpers (gpg-agent, pinentry) that inherit the captured stdout and stderr pipes. Go's default cancellation kills only the direct child, so the helper keeps the pipes open and Cmd.Wait blocks on the output-copying goroutines forever -- a dead process and a call that still never returns. Two additions fix that: the child runs in its own process group and cancellation kills the group, and Cmd.WaitDelay caps how long Wait will hold on for the pipes if something escapes the group anyway. Measured with the stand-in gpg from the new test: neither mechanism, hangs until `go test` gives up; WaitDelay only, returns in 2.2s; both, returns in 0.20s. Timeout errors now name the operation and how long gpg ran instead of surfacing a bare "signal: killed" or "context deadline exceeded", and a cancellation from above is reported as a cancellation rather than a timeout, so an abort is distinguishable from a stall. The test helper's own keygen invocations had the same unbounded context.Background() and the same pipe-inheriting agent problem, which makes them the actual mechanism behind the intermittent suite timeout noted in the issue: keygen starts gpg-agent, and a stalled agent hung the suite rather than failing it. They now run under a deadline with the same hardening, so a broken gpg environment skips instead of hanging. Verified with a cold `docker buildx build --no-cache`: prettier, gofmt, `make lint` (0 issues) and `make test` all executed and passed. The golang:1.23 image ships gpg, so the real signing, export, fingerprint, import and verify tests run against real gpg there, not skipped. The process-group kill is unix-only and lives in a build-tagged file; on other platforms the deadline is still enforced via cancellation plus WaitDelay, only the group kill of helpers is unavailable.
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user