diff --git a/README.md b/README.md index 11434bc..27d5884 100644 --- a/README.md +++ b/README.md @@ -167,6 +167,7 @@ gh api \ | `--server-url` | No | `SERVER_URL`, `BIFROST_SERVER_URL` | URL to the bifrost server. | | `--retry-attempts` | No | | Number of retry attempts for transient upload failures. | | `--retry-delay` | No | | Delay between upload retry attempts. | +| `--http-timeout` | No | | Maximum duration to wait for an HTTP upload request. Defaults to 30 seconds. | | `--git-branch` | No | | Git branch name to attach to the upload. | | `--git-commit-sha` | No | | Git commit SHA to attach to the upload. | | `--git-origin` | No | | Git origin URL to attach to the upload. | diff --git a/internal/bifrost/api.go b/internal/bifrost/api.go index 0510b19..30dec34 100644 --- a/internal/bifrost/api.go +++ b/internal/bifrost/api.go @@ -18,6 +18,7 @@ const ( DefaultServerURL = "https://portal.bifrostsec.com" DefaultRetryAttempts = 3 DefaultRetryDelay = 2 * time.Second + DefaultHTTPTimeout = 30 * time.Second userAgentProduct = "bifrost-cli" ) @@ -30,6 +31,7 @@ type APIConfig struct { Token string RetryAttempts int RetryDelay time.Duration + HTTPTimeout time.Duration RetryOutput io.Writer GitBranch string GitCommitSHA string @@ -50,11 +52,14 @@ func NewAPI(cfg APIConfig) API { if cfg.RetryDelay < 0 { cfg.RetryDelay = 0 } + if cfg.HTTPTimeout <= 0 { + cfg.HTTPTimeout = DefaultHTTPTimeout + } if cfg.RetryOutput == nil { cfg.RetryOutput = os.Stderr } return &api{ - client: http.Client{}, + client: http.Client{Timeout: cfg.HTTPTimeout}, cfg: cfg, } } @@ -91,6 +96,9 @@ func (a *api) uploadSBOM(ctx context.Context, service string, serviceVersion str if err == nil { return nil } + if err := ctx.Err(); err != nil { + return err + } if attempt == a.cfg.RetryAttempts || !shouldRetry(err) { return err } @@ -214,7 +222,7 @@ func (e *requestError) Unwrap() error { } func shouldRetry(err error) bool { - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + if errors.Is(err, context.Canceled) { return false } var uploadErr *uploadError diff --git a/internal/bifrost/api_test.go b/internal/bifrost/api_test.go index 9fc3e56..10d6831 100644 --- a/internal/bifrost/api_test.go +++ b/internal/bifrost/api_test.go @@ -301,6 +301,70 @@ func TestAPI_UploadSBOM_DoesNotRetryClientFailure(t *testing.T) { assert.EqualValues(t, 1, attempts.Load()) } +func TestAPI_UploadSBOM_RetriesHTTPTimeout(t *testing.T) { + var attempts atomic.Int32 + releaseFirstRequest := make(chan struct{}) + httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if attempts.Add(1) == 1 { + select { + case <-r.Context().Done(): + case <-releaseFirstRequest: + } + return + } + w.WriteHeader(http.StatusOK) + })) + defer httpServer.Close() + defer close(releaseFirstRequest) + + path := "test-sbom.json" + assert.NoError(t, os.WriteFile(path, []byte(`{"name":"test","version":"1.0"}`), 0644)) + defer func() { + _ = os.Remove(path) + }() + + cfg := newTestAPIConfig(httpServer.URL) + cfg.HTTPTimeout = 100 * time.Millisecond + cfg.RetryAttempts = 1 + cfg.RetryDelay = time.Millisecond + api := NewAPI(cfg) + + err := api.UploadSBOMFile(context.Background(), "test-service", "test-version", path) + assert.NoError(t, err) + assert.EqualValues(t, 2, attempts.Load()) +} + +func TestAPI_UploadSBOM_DoesNotRetryExpiredContext(t *testing.T) { + var attempts atomic.Int32 + releaseRequest := make(chan struct{}) + httpServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + select { + case <-r.Context().Done(): + case <-releaseRequest: + } + })) + defer httpServer.Close() + defer close(releaseRequest) + + path := "test-sbom.json" + assert.NoError(t, os.WriteFile(path, []byte(`{"name":"test","version":"1.0"}`), 0644)) + defer func() { + _ = os.Remove(path) + }() + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + cfg := newTestAPIConfig(httpServer.URL) + cfg.HTTPTimeout = time.Second + cfg.RetryAttempts = 1 + api := NewAPI(cfg) + + err := api.UploadSBOMFile(ctx, "test-service", "test-version", path) + assert.ErrorIs(t, err, context.DeadlineExceeded) + assert.EqualValues(t, 1, attempts.Load()) +} + func TestAPI_NewAPI_NormalizesNegativeRetryConfiguration(t *testing.T) { client := NewAPI(APIConfig{ ServerURL: "https://example.com", @@ -312,16 +376,29 @@ func TestAPI_NewAPI_NormalizesNegativeRetryConfiguration(t *testing.T) { assert.True(t, ok) assert.Equal(t, 0, internalAPI.cfg.RetryAttempts) assert.Equal(t, time.Duration(0), internalAPI.cfg.RetryDelay) + assert.Equal(t, DefaultHTTPTimeout, internalAPI.cfg.HTTPTimeout) + assert.Equal(t, DefaultHTTPTimeout, internalAPI.client.Timeout) +} + +func TestAPI_NewAPI_UsesConfiguredHTTPTimeout(t *testing.T) { + client := NewAPI(APIConfig{ + ServerURL: "https://example.com", + Token: "test-token", + HTTPTimeout: 5 * time.Second, + }) + internalAPI, ok := client.(*api) + assert.True(t, ok) + assert.Equal(t, 5*time.Second, internalAPI.client.Timeout) } func TestShouldRetry_ContextCancellationIsNotRetryable(t *testing.T) { assert.False(t, shouldRetry(&requestError{cause: context.Canceled})) - assert.False(t, shouldRetry(&requestError{cause: context.DeadlineExceeded})) + assert.True(t, shouldRetry(&requestError{cause: context.DeadlineExceeded})) } func TestShouldRetry_WrappedContextCancellationIsNotRetryable(t *testing.T) { assert.False(t, shouldRetry(&requestError{cause: fmt.Errorf("request failed: %w", context.Canceled)})) - assert.False(t, shouldRetry(&requestError{cause: fmt.Errorf("request failed: %w", context.DeadlineExceeded)})) + assert.True(t, shouldRetry(&requestError{cause: fmt.Errorf("request failed: %w", context.DeadlineExceeded)})) } func TestShouldRetry_NonContextRequestErrorIsRetryable(t *testing.T) { diff --git a/internal/bifrost/cli.go b/internal/bifrost/cli.go index 26a2ffe..f175538 100644 --- a/internal/bifrost/cli.go +++ b/internal/bifrost/cli.go @@ -9,7 +9,9 @@ import ( "fmt" "io" "os" + "os/signal" "runtime" + "syscall" ) type Task interface { @@ -17,6 +19,13 @@ type Task interface { } func CLI(version, gitCommit string, args []string) int { + ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer stop() + + return runCLI(ctx, version, gitCommit, args) +} + +func runCLI(ctx context.Context, version, gitCommit string, args []string) int { fl := NewAliasedFlagSet("", flag.ContinueOnError) showHelp := false fl.BoolVar(&showHelp, "help", false, "show this help and exit", "h") @@ -65,7 +74,7 @@ func CLI(version, gitCommit string, args []string) int { return 2 } - err = task.Run(context.Background()) + err = task.Run(ctx) if err != nil { _, _ = fmt.Fprintf(os.Stderr, "Error: %s\n", err) return 2 diff --git a/internal/bifrost/cli_test.go b/internal/bifrost/cli_test.go index 7a2b0d7..79c9221 100644 --- a/internal/bifrost/cli_test.go +++ b/internal/bifrost/cli_test.go @@ -5,6 +5,7 @@ package bifrost import ( "errors" + "flag" "fmt" "io" "net/http" @@ -924,3 +925,30 @@ func TestCLI_InvalidRetryDelay(t *testing.T) { exitCode := CLI("1.0", "commit", args) assert.Equal(t, 2, exitCode) } + +func TestCLI_InvalidHTTPTimeout(t *testing.T) { + for _, timeout := range []string{"0s", "-1s"} { + t.Run(timeout, func(t *testing.T) { + args := []string{ + "--server-url=https://portal.bifrostsec.com", + "--service=test-service", + "--service-version=1.0", + "--api-key=test-token", + "--http-timeout=" + timeout, + "sbom", "upload", "test-sbom.json", + } + + exitCode := CLI("1.0", "commit", args) + assert.Equal(t, 2, exitCode) + }) + } +} + +func TestRegisterOptions_ParsesHTTPTimeout(t *testing.T) { + fl := flag.NewFlagSet("", flag.ContinueOnError) + options := Options{} + RegisterOptions(fl, &options) + + assert.NoError(t, fl.Parse([]string{"--http-timeout=5s"})) + assert.Equal(t, 5*time.Second, options.httpTimeout) +} diff --git a/internal/bifrost/options.go b/internal/bifrost/options.go index bd85a88..46166a3 100644 --- a/internal/bifrost/options.go +++ b/internal/bifrost/options.go @@ -31,6 +31,7 @@ type Options struct { image string retryAttempts int retryDelay time.Duration + httpTimeout time.Duration gitBranch string gitCommitSHA string gitOrigin string @@ -46,6 +47,7 @@ func RegisterOptions(fl *flag.FlagSet, opts *Options) { fl.StringVar(&opts.image, "image", "", "Container image reference for the uploaded SBOM (or IMAGE environment variable); required unless a service version is provided") fl.IntVar(&opts.retryAttempts, "retry-attempts", DefaultRetryAttempts, "Number of retry attempts for transient upload failures") fl.DurationVar(&opts.retryDelay, "retry-delay", DefaultRetryDelay, "Delay between upload retry attempts") + fl.DurationVar(&opts.httpTimeout, "http-timeout", DefaultHTTPTimeout, "Maximum duration to wait for an HTTP upload request") fl.StringVar(&opts.gitBranch, "git-branch", "", "Optional Git branch name for the uploaded SBOM") fl.StringVar(&opts.gitCommitSHA, "git-commit-sha", "", "Optional Git commit SHA for the uploaded SBOM") fl.StringVar(&opts.gitOrigin, "git-origin", "", "Optional Git origin URL for the uploaded SBOM") @@ -77,6 +79,10 @@ func ValidateBaseOptions(fl *flag.FlagSet, opts *Options) error { if opts.retryDelay < 0 { return fmt.Errorf("retry delay must be zero or greater") } + if opts.httpTimeout <= 0 { + return fmt.Errorf("HTTP timeout must be greater than zero") + } + // An explicitly passed flag takes precedence over the environment variable. if opts.gitRepoPath == "" { opts.gitRepoPath = os.Getenv(gitRepoPathEnvironmentVariable) } diff --git a/internal/bifrost/sbom_upload.go b/internal/bifrost/sbom_upload.go index d8d328a..dcb9cc3 100644 --- a/internal/bifrost/sbom_upload.go +++ b/internal/bifrost/sbom_upload.go @@ -61,6 +61,7 @@ func (t sbomUploadTask) Run(ctx context.Context) error { Token: t.apiKey, RetryAttempts: t.retryAttempts, RetryDelay: t.retryDelay, + HTTPTimeout: t.httpTimeout, GitBranch: t.gitBranch, GitCommitSHA: t.gitCommitSHA, GitOrigin: t.gitOrigin, @@ -134,7 +135,7 @@ func (t sbomUploadTask) uploadStdinSBOM(ctx context.Context, api API) error { _ = os.Remove(tmpPath) }() - if _, err := io.Copy(tmpFile, os.Stdin); err != nil { + if err := copyStdinWithContext(ctx, tmpFile, os.Stdin); err != nil { _ = tmpFile.Close() return fmt.Errorf("failed to read SBOM from stdin: %w", err) } @@ -144,3 +145,17 @@ func (t sbomUploadTask) uploadStdinSBOM(ctx context.Context, api API) error { return api.UploadSBOMFile(ctx, t.service, t.serviceVersion, tmpPath) } + +func copyStdinWithContext(ctx context.Context, destination io.Writer, stdin *os.File) error { + done := make(chan error, 1) + go func() { + _, err := io.Copy(destination, stdin) + done <- err + }() + select { + case err := <-done: + return err + case <-ctx.Done(): + return ctx.Err() + } +} diff --git a/internal/bifrost/sbom_upload_test.go b/internal/bifrost/sbom_upload_test.go new file mode 100644 index 0000000..fa7c6e8 --- /dev/null +++ b/internal/bifrost/sbom_upload_test.go @@ -0,0 +1,143 @@ +// Copyright 2026 bifrost security +// SPDX-License-Identifier: Apache-2.0 + +//go:build unix + +package bifrost + +import ( + "context" + "errors" + "os" + "path/filepath" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type unexpectedUploadAPI struct{} + +func (unexpectedUploadAPI) UploadSBOMFile(context.Context, string, string, string) error { + return errors.New("upload should not be reached") +} + +func newBlockingStdinFIFO(t *testing.T) (stdin *os.File, writer *os.File) { + t.Helper() + path := filepath.Join(t.TempDir(), "stdin.fifo") + require.NoError(t, syscall.Mkfifo(path, 0o600)) + + type openResult struct { + file *os.File + err error + } + writerResult := make(chan openResult, 1) + go func() { + file, err := os.OpenFile(path, os.O_WRONLY, 0) + writerResult <- openResult{file: file, err: err} + }() + + stdin, err := os.Open(path) + require.NoError(t, err) + result := <-writerResult + require.NoError(t, result.err) + return stdin, result.file +} + +func TestSBOMUploadTask_StdinCancellationCleansUpTemporaryFile(t *testing.T) { + // This test replaces os.Stdin and must remain serial. + tempDir := t.TempDir() + t.Setenv("TMPDIR", tempDir) + + stdin, writer := newBlockingStdinFIFO(t) + defer func() { _ = writer.Close() }() + + originalStdin := os.Stdin + os.Stdin = stdin + defer func() { + os.Stdin = originalStdin + _ = stdin.Close() + }() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + task := sbomUploadTask{Options: Options{service: "test-service", serviceVersion: "test-version"}} + errCh := make(chan error, 1) + go func() { + errCh <- task.uploadStdinSBOM(ctx, unexpectedUploadAPI{}) + }() + + waitForTemporaryStdinSBOM(t, tempDir) + _, err := writer.Write([]byte("{")) + require.NoError(t, err) + waitForTemporaryStdinSBOMContent(t, tempDir) + select { + case err := <-errCh: + t.Fatalf("stdin upload finished before cancellation: %v", err) + default: + } + cancel() + + select { + case err := <-errCh: + assert.ErrorIs(t, err, context.Canceled) + case <-time.After(time.Second): + t.Fatal("stdin upload did not stop after cancellation") + } + + matches, err := filepath.Glob(filepath.Join(tempDir, "bifrost-stdin-sbom-*.json")) + assert.NoError(t, err) + assert.Empty(t, matches, "temporary stdin SBOM was not removed") +} + +func waitForTemporaryStdinSBOM(t *testing.T, dir string) { + t.Helper() + deadline := time.After(time.Second) + ticker := time.NewTicker(time.Millisecond) + defer ticker.Stop() + + for { + matches, err := filepath.Glob(filepath.Join(dir, "bifrost-stdin-sbom-*.json")) + if err != nil { + t.Fatalf("failed to find temporary stdin SBOM: %v", err) + } + if len(matches) > 0 { + return + } + select { + case <-deadline: + t.Fatal("temporary stdin SBOM was not created") + case <-ticker.C: + } + } +} + +func waitForTemporaryStdinSBOMContent(t *testing.T, dir string) { + t.Helper() + deadline := time.After(time.Second) + ticker := time.NewTicker(time.Millisecond) + defer ticker.Stop() + + for { + matches, err := filepath.Glob(filepath.Join(dir, "bifrost-stdin-sbom-*.json")) + if err != nil { + t.Fatalf("failed to find temporary stdin SBOM: %v", err) + } + for _, path := range matches { + info, err := os.Stat(path) + if err != nil { + t.Fatalf("failed to stat temporary stdin SBOM: %v", err) + } + if info.Size() > 0 { + return + } + } + select { + case <-deadline: + t.Fatal("stdin data was not copied to the temporary file") + case <-ticker.C: + } + } +}