From 1337133745a81b93c1328c70a5ab465810b65a2e Mon Sep 17 00:00:00 2001 From: Billy Lau Date: Wed, 23 Sep 2026 10:13:11 -0500 Subject: [PATCH] verifier_tools/verify: Factor shared HTTP client and add checkpoint timeouts/context Previously, `getSignedCheckpoint` in `internal/checkpoint/checkpoint.go` called `http.Get` directly, using `http.DefaultClient` (`Timeout: 0`) without caller `context.Context` propagation. If a remote server stalled or dropped packets while fetching `checkpoint` or `checkpoint.txt`, the verifier could block indefinitely. 1. Factor the tuned `http.Transport` configuration (`DialContext` with `10s` timeout, `ForceAttemptHTTP2: true`, `TLSHandshakeTimeout: 10s`, and `ResponseHeaderTimeout: 30s`) into `internal/httpclient.New(timeout)` and share it across `internal/tiles` (`5m` total timeout) and `internal/checkpoint` (`30s` total timeout). 2. Add `getSignedCheckpointContext` and `FromURLWithPathContext` in `internal/checkpoint` using `http.NewRequestWithContext` and `%w` error wrapping to preserve timeout and cancellation error chains. 3. Wire `signal.NotifyContext` (`SIGINT`/`SIGTERM`) in `cmd/verifier/verifier.go` across both `--fetch_entries` and `--payload_path` verification paths so signal cancellation propagates to all checkpoint fetches. Test: - `cd verifier_tools/verify && gofmt -l . && go test -race -v ./...` - Added `TestNew` in `internal/httpclient/httpclient_test.go` and `TestHTTPClientTimeoutAndContextCancellation` in `internal/checkpoint/checkpoint_test.go`. Change-Id: Ibcadd345cf22a38083b9f7fb86c5379c8681fb59 --- .../verify/cmd/verifier/verifier.go | 10 ++--- .../verify/internal/checkpoint/checkpoint.go | 38 +++++++++++++--- .../internal/checkpoint/checkpoint_test.go | 32 +++++++++++++ .../verify/internal/httpclient/httpclient.go | 42 +++++++++++++++++ .../internal/httpclient/httpclient_test.go | 45 +++++++++++++++++++ .../verify/internal/tiles/reader.go | 33 +++----------- .../verify/internal/tiles/reader_test.go | 17 ------- 7 files changed, 162 insertions(+), 55 deletions(-) create mode 100644 verifier_tools/verify/internal/httpclient/httpclient.go create mode 100644 verifier_tools/verify/internal/httpclient/httpclient_test.go diff --git a/verifier_tools/verify/cmd/verifier/verifier.go b/verifier_tools/verify/cmd/verifier/verifier.go index 62e7256..cd188b6 100644 --- a/verifier_tools/verify/cmd/verifier/verifier.go +++ b/verifier_tools/verify/cmd/verifier/verifier.go @@ -240,7 +240,7 @@ func resolveTargets(logType string) ([]logTarget, error) { func runFetchEntries(ctx context.Context, targets []logTarget, concurrency int) error { for _, target := range targets { slog.Info("Syncing entries for log", "log", target.name, "url", target.baseURL) - root, err := checkpoint.FromURLWithPath(target.baseURL, target.checkpointPath, target.verifier) + root, err := checkpoint.FromURLWithPathContext(ctx, target.baseURL, target.checkpointPath, target.verifier) if err != nil { return fmt.Errorf("failed to read checkpoint for %s: %w", target.name, err) } @@ -278,10 +278,10 @@ func main() { os.Exit(1) } - if *fetchEntries { - ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) - defer cancel() + ctx, cancel := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + defer cancel() + if *fetchEntries { if err := runFetchEntries(ctx, targets, *concurrency); err != nil { slog.Error("FAILURE: error fetching entries", "error", err) os.Exit(1) @@ -311,7 +311,7 @@ func main() { var verified bool for _, target := range targets { slog.Info("Checking log", "log", target.name, "url", target.baseURL) - root, err := checkpoint.FromURLWithPath(target.baseURL, target.checkpointPath, target.verifier) + root, err := checkpoint.FromURLWithPathContext(ctx, target.baseURL, target.checkpointPath, target.verifier) if err != nil { slog.Warn("Failed to read checkpoint", "log", target.name, "error", err) continue diff --git a/verifier_tools/verify/internal/checkpoint/checkpoint.go b/verifier_tools/verify/internal/checkpoint/checkpoint.go index 48cc021..4031203 100644 --- a/verifier_tools/verify/internal/checkpoint/checkpoint.go +++ b/verifier_tools/verify/internal/checkpoint/checkpoint.go @@ -19,6 +19,7 @@ package checkpoint import ( + "context" "crypto/ecdsa" "crypto/sha256" "crypto/x509" @@ -32,7 +33,9 @@ import ( "path" "strconv" "strings" + "time" + "github.com/android/android-binary-transparency/verifier_tools/verify/internal/httpclient" "golang.org/x/mod/sumdb/note" ) @@ -166,7 +169,17 @@ func parseCheckpoint(ckpt string) (Root, error) { return Root{Size: size, Hash: rh}, nil } +const ( + defaultHTTPTimeout = 30 * time.Second +) + +var httpClient = httpclient.New(defaultHTTPTimeout) + func getSignedCheckpoint(logURL, checkpointPath string) ([]byte, error) { + return getSignedCheckpointContext(context.Background(), logURL, checkpointPath) +} + +func getSignedCheckpointContext(ctx context.Context, logURL, checkpointPath string) ([]byte, error) { // Sanity check the input url. u, err := url.Parse(logURL) if err != nil { @@ -175,9 +188,14 @@ func getSignedCheckpoint(logURL, checkpointPath string) ([]byte, error) { u.Path = path.Join(u.Path, checkpointPath) - resp, err := http.Get(u.String()) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) + if err != nil { + return []byte{}, fmt.Errorf("http.NewRequestWithContext(%s): %w", u, err) + } + + resp, err := httpClient.Do(req) if err != nil { - return []byte{}, fmt.Errorf("http.Get(%s): %v", u, err) + return []byte{}, fmt.Errorf("http.Get(%s): %w", u, err) } defer resp.Body.Close() if code := resp.StatusCode; code != 200 { @@ -187,11 +205,14 @@ func getSignedCheckpoint(logURL, checkpointPath string) ([]byte, error) { return io.ReadAll(resp.Body) } -// FromURLWithPath verifies the signature and unpacks and returns a Root from the given checkpoint path. -func FromURLWithPath(logURL, checkpointPath string, v note.Verifier) (Root, error) { - b, err := getSignedCheckpoint(logURL, checkpointPath) +// FromURLWithPathContext verifies the signature and unpacks and returns a Root from the given checkpoint path using ctx. +func FromURLWithPathContext(ctx context.Context, logURL, checkpointPath string, v note.Verifier) (Root, error) { + if ctx == nil { + ctx = context.Background() + } + b, err := getSignedCheckpointContext(ctx, logURL, checkpointPath) if err != nil { - return Root{}, fmt.Errorf("failed to get signed checkpoint: %v", err) + return Root{}, fmt.Errorf("failed to get signed checkpoint: %w", err) } n, err := note.Open(b, note.VerifierList(v)) @@ -201,6 +222,11 @@ func FromURLWithPath(logURL, checkpointPath string, v note.Verifier) (Root, erro return parseCheckpoint(n.Text) } +// FromURLWithPath verifies the signature and unpacks and returns a Root from the given checkpoint path. +func FromURLWithPath(logURL, checkpointPath string, v note.Verifier) (Root, error) { + return FromURLWithPathContext(context.Background(), logURL, checkpointPath, v) +} + // FromURL verifies the signature and unpacks and returns a Root. // // Validates signature before reading data, using a provided verifier. diff --git a/verifier_tools/verify/internal/checkpoint/checkpoint_test.go b/verifier_tools/verify/internal/checkpoint/checkpoint_test.go index b5d5ba9..be42e1b 100644 --- a/verifier_tools/verify/internal/checkpoint/checkpoint_test.go +++ b/verifier_tools/verify/internal/checkpoint/checkpoint_test.go @@ -1,11 +1,14 @@ package checkpoint import ( + "context" + "errors" "net/http" "net/http/httptest" "net/url" "path" "testing" + "time" "github.com/google/go-cmp/cmp" ) @@ -172,3 +175,32 @@ func TestValidCheckpointFormat(t *testing.T) { }) } } + +func TestHTTPClientTimeoutAndContextCancellation(t *testing.T) { + if httpClient.Timeout != defaultHTTPTimeout { + t.Errorf("httpClient.Timeout = %v, want %v", httpClient.Timeout, defaultHTTPTimeout) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Block until client request context is cancelled or times out. + <-r.Context().Done() + })) + defer server.Close() + + // 1. Caller context timeout aborts promptly and preserves context.DeadlineExceeded. + ctx, cancel := context.WithTimeout(context.Background(), 40*time.Millisecond) + defer cancel() + if _, err := getSignedCheckpointContext(ctx, server.URL, "checkpoint.txt"); !errors.Is(err, context.DeadlineExceeded) { + t.Errorf("expected context.DeadlineExceeded when checkpoint fetch times out, got %v", err) + } + + // 2. httpClient.Timeout aborts hanging server even with context.Background(). + origTimeout := httpClient.Timeout + httpClient.Timeout = 40 * time.Millisecond + defer func() { + httpClient.Timeout = origTimeout + }() + if _, err := getSignedCheckpoint(server.URL, "checkpoint.txt"); err == nil { + t.Errorf("expected error when server exceeds httpClient.Timeout, got nil") + } +} diff --git a/verifier_tools/verify/internal/httpclient/httpclient.go b/verifier_tools/verify/internal/httpclient/httpclient.go new file mode 100644 index 000000000..e66b22b --- /dev/null +++ b/verifier_tools/verify/internal/httpclient/httpclient.go @@ -0,0 +1,42 @@ +// Package httpclient provides a shared HTTP client constructor with tuned +// connection, TLS, and response header timeouts for transparency log requests. +package httpclient + +import ( + "net" + "net/http" + "time" +) + +const ( + // DefaultResponseHeaderTimeout bounds the time spent waiting for a server's + // response headers after a request is written. + DefaultResponseHeaderTimeout = 30 * time.Second + // DefaultDialTimeout bounds TCP connection establishment. + DefaultDialTimeout = 10 * time.Second + // DefaultTLSHandshakeTimeout bounds the TLS handshake. + DefaultTLSHandshakeTimeout = 10 * time.Second +) + +// New returns an *http.Client configured with the given overall request timeout +// and a shared transport policy (10s dial/TLS timeouts, 30s response header +// timeout, and HTTP/2 enabled). +func New(timeout time.Duration) *http.Client { + return &http.Client{ + Timeout: timeout, + Transport: &http.Transport{ + Proxy: http.ProxyFromEnvironment, + DialContext: (&net.Dialer{ + Timeout: DefaultDialTimeout, + KeepAlive: 30 * time.Second, + }).DialContext, + ForceAttemptHTTP2: true, + MaxIdleConns: 100, + MaxIdleConnsPerHost: 32, + IdleConnTimeout: 90 * time.Second, + TLSHandshakeTimeout: DefaultTLSHandshakeTimeout, + ResponseHeaderTimeout: DefaultResponseHeaderTimeout, + ExpectContinueTimeout: 1 * time.Second, + }, + } +} diff --git a/verifier_tools/verify/internal/httpclient/httpclient_test.go b/verifier_tools/verify/internal/httpclient/httpclient_test.go new file mode 100644 index 000000000..79b6291 --- /dev/null +++ b/verifier_tools/verify/internal/httpclient/httpclient_test.go @@ -0,0 +1,45 @@ +package httpclient + +import ( + "net/http" + "testing" + "time" +) + +func TestNew(t *testing.T) { + const customTimeout = 45 * time.Second + client := New(customTimeout) + + if client.Timeout != customTimeout { + t.Errorf("client.Timeout = %v, want %v", client.Timeout, customTimeout) + } + + transport, ok := client.Transport.(*http.Transport) + if !ok || transport == nil { + t.Fatalf("expected client.Transport to be *http.Transport, got %T", client.Transport) + } + if !transport.ForceAttemptHTTP2 { + t.Errorf("transport.ForceAttemptHTTP2 = false, want true (required when DialContext is non-nil)") + } + if transport.ResponseHeaderTimeout != DefaultResponseHeaderTimeout { + t.Errorf("transport.ResponseHeaderTimeout = %v, want %v", transport.ResponseHeaderTimeout, DefaultResponseHeaderTimeout) + } + if transport.TLSHandshakeTimeout != DefaultTLSHandshakeTimeout { + t.Errorf("transport.TLSHandshakeTimeout = %v, want %v", transport.TLSHandshakeTimeout, DefaultTLSHandshakeTimeout) + } + if transport.DialContext == nil { + t.Errorf("expected transport.DialContext to be non-nil") + } + if transport.MaxIdleConns != 100 { + t.Errorf("transport.MaxIdleConns = %d, want 100", transport.MaxIdleConns) + } + if transport.MaxIdleConnsPerHost != 32 { + t.Errorf("transport.MaxIdleConnsPerHost = %d, want 32", transport.MaxIdleConnsPerHost) + } + if transport.IdleConnTimeout != 90*time.Second { + t.Errorf("transport.IdleConnTimeout = %v, want 90s", transport.IdleConnTimeout) + } + if transport.ExpectContinueTimeout != 1*time.Second { + t.Errorf("transport.ExpectContinueTimeout = %v, want 1s", transport.ExpectContinueTimeout) + } +} diff --git a/verifier_tools/verify/internal/tiles/reader.go b/verifier_tools/verify/internal/tiles/reader.go index c43a8f5..adeb597 100644 --- a/verifier_tools/verify/internal/tiles/reader.go +++ b/verifier_tools/verify/internal/tiles/reader.go @@ -9,7 +9,6 @@ import ( "fmt" "io" "log/slog" - "net" "net/http" "net/url" "os" @@ -21,6 +20,7 @@ import ( "sync/atomic" "time" + "github.com/android/android-binary-transparency/verifier_tools/verify/internal/httpclient" "golang.org/x/mod/sumdb/tlog" ) @@ -120,33 +120,12 @@ func BinaryInfosIndex(logBaseURL string, binaryInfoFilename string, treeSize int return parseBinaryInfosIndex(binaryInfos, binaryInfoFilename) } -const ( - defaultHTTPTimeout = 5 * time.Minute - defaultResponseHeaderTimeout = 30 * time.Second - defaultDialTimeout = 10 * time.Second - defaultTLSHandshakeTimeout = 10 * time.Second -) +const defaultHTTPTimeout = 5 * time.Minute -var httpClient = &http.Client{ - // Timeout covers the entire request including reading large legacy binary info - // response bodies (e.g. ~210 MB package_info.txt), while Transport timeouts bound - // connection establishment and waiting for response headers. - Timeout: defaultHTTPTimeout, - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - DialContext: (&net.Dialer{ - Timeout: defaultDialTimeout, - KeepAlive: 30 * time.Second, - }).DialContext, - ForceAttemptHTTP2: true, - MaxIdleConns: 100, - MaxIdleConnsPerHost: 32, - IdleConnTimeout: 90 * time.Second, - TLSHandshakeTimeout: defaultTLSHandshakeTimeout, - ResponseHeaderTimeout: defaultResponseHeaderTimeout, - ExpectContinueTimeout: 1 * time.Second, - }, -} +// Timeout covers the entire request including reading large legacy binary info +// response bodies (e.g. ~210 MB package_info.txt), while Transport timeouts bound +// connection establishment and waiting for response headers. +var httpClient = httpclient.New(defaultHTTPTimeout) var ( customCacheDirMu sync.RWMutex diff --git a/verifier_tools/verify/internal/tiles/reader_test.go b/verifier_tools/verify/internal/tiles/reader_test.go index 24ef6a8..386aa40 100644 --- a/verifier_tools/verify/internal/tiles/reader_test.go +++ b/verifier_tools/verify/internal/tiles/reader_test.go @@ -865,23 +865,6 @@ func TestHTTPClientTimeoutConfiguration(t *testing.T) { if httpClient.Timeout < 5*time.Minute { t.Errorf("httpClient.Timeout (%v) is too short for large ~210 MB package_info.txt downloads; want >= 5m", httpClient.Timeout) } - - transport, ok := httpClient.Transport.(*http.Transport) - if !ok || transport == nil { - t.Fatalf("expected httpClient.Transport to be *http.Transport, got %T", httpClient.Transport) - } - if !transport.ForceAttemptHTTP2 { - t.Errorf("transport.ForceAttemptHTTP2 = false, want true (required when DialContext is non-nil)") - } - if transport.ResponseHeaderTimeout != defaultResponseHeaderTimeout { - t.Errorf("transport.ResponseHeaderTimeout = %v, want %v", transport.ResponseHeaderTimeout, defaultResponseHeaderTimeout) - } - if transport.TLSHandshakeTimeout != defaultTLSHandshakeTimeout { - t.Errorf("transport.TLSHandshakeTimeout = %v, want %v", transport.TLSHandshakeTimeout, defaultTLSHandshakeTimeout) - } - if transport.DialContext == nil { - t.Errorf("expected transport.DialContext to be non-nil") - } } func TestReadFromURLContextStreamingAndCancellation(t *testing.T) {