From 418f69e58b90af16d32d79fe6240aa588ee73e78 Mon Sep 17 00:00:00 2001 From: Alano Terblanche <18033717+Benehiko@users.noreply.github.com> Date: Thu, 13 Aug 2026 17:39:53 +0200 Subject: [PATCH] fix(client): apply no response-header timeout by default The client defaulted ResponseHeaderTimeout to 1 second, so any request where the daemon took more than a second to start responding failed at the transport layer. A request can legitimately block far longer than that on user interaction daemon-side: a store unlock, an authorization prompt, or a plugin waiting on input. The request timeout already defaults to 0 (no limit) for exactly this reason, and the docker pass CLI already overrides both timeouts to 0. Make that the default for every consumer: DefaultClientResponseHeaderTimeout is now 0 as well. Callers that need a hard bound can still set one with WithResponseTimeout or a per-call context deadline. Co-Authored-By: Claude Fable 5 --- client/client.go | 10 +++------- plugins/pass/commands/run.go | 7 ++----- plugins/pass/commands/run_test.go | 19 ++++++------------- x/api/accesscontrol/accesscontrol_darwin.go | 9 ++++----- x/api/accesscontrol/v1/api.pb.go | 8 +++----- x/api/accesscontrol/v1/api.proto | 8 +++----- x/api/defaults.go | 9 +++++---- 7 files changed, 26 insertions(+), 44 deletions(-) diff --git a/client/client.go b/client/client.go index 5a2f0b9d..3d6dc6e3 100644 --- a/client/client.go +++ b/client/client.go @@ -96,12 +96,8 @@ func WithTimeout(timeout time.Duration) Option { } // WithResponseTimeout overrides the response header timeout of the client. -// -// It is useful to set if there are long-lived user interactions required -// when the Secrets Engine requests secrets from a plugin. -// -// A responseTimeout of 0 means no response header timeout will be applied. -// Negative durations are not allowed and will result in an error. +// By default no timeout is applied, because a request may block on user +// interaction. 0 means no timeout; negative durations return an error. func WithResponseTimeout(responseTimeout time.Duration) Option { return func(s *config) error { if responseTimeout < 0 { @@ -211,7 +207,7 @@ func New(options ...Option) (Client, error) { MaxIdleConnsPerHost: api.DefaultClientMaxIdleConnsPerHost, // keep the connection alive (good for long-lived clients) IdleConnTimeout: api.DefaultClientIdleConnTimeout, - // By default it is 1 second, but can be overridden with [WithResponseTimeout] + // no timeout by default; override with [WithResponseTimeout] ResponseHeaderTimeout: cfg.responseTimeout, TLSHandshakeTimeout: api.DefaultClientTLSHandshakeTimeout, diff --git a/plugins/pass/commands/run.go b/plugins/pass/commands/run.go index 3b321a88..c36f5676 100644 --- a/plugins/pass/commands/run.go +++ b/plugins/pass/commands/run.go @@ -204,11 +204,8 @@ func newRunClient(opts runOpts) (client.Client, error) { return client.New(copts...) } -// preflightPing fails fast when the engine is unreachable, instead of letting -// an unbounded client block resolution indefinitely. -// -// The Docker CLI bounds its daemon connection ping the same way -// (docker/cli#3722, fixing the unreachable-daemon hang in docker/cli#3652). +// preflightPing fails fast when the engine is unreachable, so an unbounded +// client cannot hang resolution indefinitely. func preflightPing(ctx context.Context, c client.Client, timeout time.Duration) error { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() diff --git a/plugins/pass/commands/run_test.go b/plugins/pass/commands/run_test.go index 10baac9c..619baa26 100644 --- a/plugins/pass/commands/run_test.go +++ b/plugins/pass/commands/run_test.go @@ -48,9 +48,8 @@ const ( helperActiveEnv = "GO_PASS_RUN_HELPER_ACTIVE" helperExitEnv = "GO_PASS_RUN_HELPER_EXIT" helperSleepEnv = "GO_PASS_RUN_HELPER_SLEEP" - // helperSocketEnv switches the wrapper to preflight mode: RunCommand is - // built with WithSocketPath(value) and no request timeout, so the - // preflight ping must run and fail against the dead socket. + // helperSocketEnv makes the wrapper target this socket with no request + // timeout, forcing the preflight ping to run. helperSocketEnv = "GO_PASS_RUN_HELPER_SOCKET" ) @@ -89,8 +88,7 @@ func runAsWrapper() { if err != nil { os.Exit(2) } - // A bounded timeout skips the preflight ping, so these subprocess tests - // exercise child-process mechanics without needing a running engine. + // bounded timeout skips the preflight ping; no engine needed ropts := []RunOption{WithTimeout(time.Second)} if socket := os.Getenv(helperSocketEnv); socket != "" { ropts = []RunOption{WithSocketPath(socket)} @@ -349,9 +347,7 @@ func waitForReady(t *testing.T, r io.Reader) { go func() { _, _ = io.Copy(io.Discard, r) }() } -// pingClient adapts a Version func to client.Client. The embedded -// MockResolver supplies GetSecrets, so no hand-rolled resolver mock can drift -// from the shared one. +// pingClient adapts a Version func to client.Client. type pingClient struct { testhelper.MockResolver ping func(context.Context) (client.DaemonVersion, error) @@ -391,11 +387,8 @@ func TestPreflightPing(t *testing.T) { <-ctx.Done() return client.DaemonVersion{}, ctx.Err() }} - // Watchdog parent: if preflightPing loses its own deadline, ping - // unblocks here and the elapsed assertion fails fast, instead of the - // package hanging until the go test panic. A parent deadline alone is - // not enough — the regressed path would still surface - // DeadlineExceeded, just later, and pass spuriously. + // Watchdog: if preflightPing loses its own deadline, the elapsed + // assertion fails instead of the package hanging. watchdogCtx, cancel := context.WithTimeout(t.Context(), 5*time.Second) defer cancel() start := time.Now() diff --git a/x/api/accesscontrol/accesscontrol_darwin.go b/x/api/accesscontrol/accesscontrol_darwin.go index 15147262..edd71a73 100644 --- a/x/api/accesscontrol/accesscontrol_darwin.go +++ b/x/api/accesscontrol/accesscontrol_darwin.go @@ -62,11 +62,10 @@ type SigningIdentity struct { // so suitable for display/logging rather than as a sole trust key. Organization string - // BundleName is the human-readable application name from the Info.plist - // bound into the code signature (kSecCodeInfoPList, CFBundleDisplayName - // falling back to CFBundleName), e.g. "Docker Desktop". Present for .app - // bundles and bare binaries with an embedded __info_plist section; empty - // otherwise. Display-only: chosen freely by the signer, not unique. + // BundleName is the app name from the signed Info.plist + // (CFBundleDisplayName, falling back to CFBundleName), e.g. + // "Docker Desktop". Empty when the binary embeds no Info.plist. + // Display-only: not unique, chosen by the signer. BundleName string // CommonName is the leaf certificate subject.CN, e.g. diff --git a/x/api/accesscontrol/v1/api.pb.go b/x/api/accesscontrol/v1/api.pb.go index b12e7f08..cf0ce91e 100644 --- a/x/api/accesscontrol/v1/api.pb.go +++ b/x/api/accesscontrol/v1/api.pb.go @@ -1596,11 +1596,9 @@ type DarwinSigningInfo_SigningIdentity_builder struct { Anchor *DarwinSigningInfo_Anchor // True if this process's signature chains to Docker's signing identity. SignedByDocker *bool - // Human-readable application name from the Info.plist bound into the code - // signature (kSecCodeInfoPList, CFBundleDisplayName falling back to - // CFBundleName), e.g. "Docker Desktop". Present for .app bundles and bare - // binaries with an embedded __info_plist section; empty otherwise. - // Display-only: chosen freely by the signer, not unique. + // App name from the signed Info.plist (CFBundleDisplayName, falling back + // to CFBundleName), e.g. "Docker Desktop". Empty when the binary embeds + // no Info.plist. Display-only: not unique, chosen by the signer. BundleName *string } diff --git a/x/api/accesscontrol/v1/api.proto b/x/api/accesscontrol/v1/api.proto index 79610e9b..6f27d7dd 100644 --- a/x/api/accesscontrol/v1/api.proto +++ b/x/api/accesscontrol/v1/api.proto @@ -117,11 +117,9 @@ message DarwinSigningInfo { Anchor anchor = 7; // True if this process's signature chains to Docker's signing identity. bool signed_by_docker = 8; - // Human-readable application name from the Info.plist bound into the code - // signature (kSecCodeInfoPList, CFBundleDisplayName falling back to - // CFBundleName), e.g. "Docker Desktop". Present for .app bundles and bare - // binaries with an embedded __info_plist section; empty otherwise. - // Display-only: chosen freely by the signer, not unique. + // App name from the signed Info.plist (CFBundleDisplayName, falling back + // to CFBundleName), e.g. "Docker Desktop". Empty when the binary embeds + // no Info.plist. Display-only: not unique, chosen by the signer. string bundle_name = 9; } diff --git a/x/api/defaults.go b/x/api/defaults.go index 1b836eef..24cc6d39 100644 --- a/x/api/defaults.go +++ b/x/api/defaults.go @@ -27,10 +27,11 @@ const ( DefaultPluginRegistrationTimeout = 5 * time.Second // DefaultClientRequestTimeout is the default timeout for clients to handle a request. DefaultClientRequestTimeout = time.Duration(0) // 0 means no limit - // DefaultClientResponseHeaderTimeout is the default timeout for clients to handle - // header responses, this does not include the response body and usually should - // be short. - DefaultClientResponseHeaderTimeout = time.Second + // DefaultClientResponseHeaderTimeout is the default timeout for response + // headers, excluding the body. No limit by default: a request may block + // on user interaction daemon-side. Use [client.WithResponseTimeout] or a + // context deadline for a hard bound. + DefaultClientResponseHeaderTimeout = time.Duration(0) // 0 means no limit // DefaultClientTLSHandshakeTimeout is the default timeout for clients to handle // tls handshakes. It should usually be short. DefaultClientTLSHandshakeTimeout = time.Second