fix(core): preserve HttpClientTimeout in Server via ClientTimeout field - #52
Open
spbsoluble wants to merge 13 commits into
Open
fix(core): preserve HttpClientTimeout in Server via ClientTimeout field#52spbsoluble wants to merge 13 commits into
spbsoluble wants to merge 13 commits into
Conversation
…ld (fixes #51) GetServerConfig() on CommandAuthConfig (and its basic/oauth/kerberos embedders) dropped the caller's HttpClientTimeout entirely: Server had no timeout field, so any WithClientTimeout() value set via CommandAuthConfig.WithClientTimeout was lost once the config was flattened to a Server for downstream consumers (e.g. keyfactor-go-client's NewKeyfactorClient, which rebuilds its own CommandAuthConfig from a Server). Consumers silently fell back to DefaultClientTimeout (60s), producing "net/http: timeout awaiting response headers" on long-running calls such as PFX enrollment even when a much larger timeout was explicitly configured upstream. Add Server.ClientTimeout (client_timeout json/yaml tag) and populate it from HttpClientTimeout in all four GetServerConfig() implementations (core, basic, oauth, kerberos). Also honor it in the reverse direction -- GetBasicAuthClientConfig/GetOAuthClientConfig/GetKerberosClientConfig now call WithClientTimeout(s.ClientTimeout) so a Server round-trips losslessly back into a CommandAuthConfig-derived config.
This was referenced Aug 14, 2026
…o default LoadConfig merged Host/Port/APIPath/CACertPath/SkipVerify from a loaded Server into CommandAuthConfig but never ClientTimeout, and ValidateAuthConfig never consulted FileConfig as a fallback for HttpClientTimeout the way it already does for CommandHostName. A config-file-only client_timeout was silently dropped, landing on the 60s default instead. Separately, ValidateAuthConfig's env var fallback treated any LookupEnv ok=true (including an empty string, common when .env files pre-declare all KEYFACTOR_* vars) as authoritative, swallowing Atoi errors and skipping the default. An empty/unparseable/non-positive KEYFACTOR_CLIENT_TIMEOUT left HttpClientTimeout at its zero value, which disables ResponseHeaderTimeout/TLSHandshakeTimeout/ IdleConnTimeout/http.Client.Timeout entirely -- an unbounded-wait hazard since Authenticate() has no request context to otherwise bound the call. Now: explicit struct value/WithClientTimeout() wins outright; absent that, a config file value (merged eagerly in LoadConfig, consistent with the other Server fields, and consulted again in ValidateAuthConfig as a defensive fallback like CommandHostName) takes effect before the env var is ever checked; an unparseable or <=0 env var is logged and ignored rather than silently zeroing the timeout; and the package default applies only when nothing else resolved a positive value. Basic, Kerberos, and OAuth auth types all delegate to CommandAuthConfig.LoadConfig/ValidateAuthConfig, so no separate per-type fix was needed.
… idle/handshake timeouts with HttpClientTimeout RequestToCurl appended the full, unredacted request body to the curl command it generates for TRACE logging (auth_oauth.go's oauth2Transport RoundTrip logs this on every OAuth-authenticated request, and the auth probe path does the same). Any secret-bearing request -- e.g. a PFX enrollment carrying a private-key password -- was therefore written to the log in plaintext whenever TRACE logging is enabled, which is exactly what support asks a customer to turn on when reporting the slow-request issue this timeout work exists to fix. RequestToCurl now parses JSON and form-encoded bodies and replaces known-sensitive field values (password, secret, token, and private-key variants, matched case-insensitively, nested objects/arrays included) with a placeholder while preserving the rest of the body for diagnostics. A body that can't be confidently classified as JSON or form-encoded is omitted entirely behind a "<redacted: N bytes, content-type X>" marker rather than ever risking a raw secret leak. Separately, BuildTransport() and SetClient() derived IdleConnTimeout and ExpectContinueTimeout from the same HttpClientTimeout value used for the request deadline (ResponseHeaderTimeout). IdleConnTimeout governs how long an idle pooled connection is retained, not a request deadline, so a large configured timeout (e.g. 1800s, needed for slow PFX enrollments) kept every idle socket -- and its goroutine -- alive for that same duration; a large `terraform apply` issuing many sequential requests could hold open hundreds of sockets/goroutines for half an hour. IdleConnTimeout, ExpectContinueTimeout, and TLSHandshakeTimeout are now pinned to fixed defaults matching net/http.DefaultTransport (90s/1s/10s) via a shared newHTTPTransport() constructor used by both BuildTransport and SetClient, while ResponseHeaderTimeout continues to track HttpClientTimeout as intended.
newHTTPTransport() hardcoded MaxConnsPerHost: 10, which was harmless while every request built its own throwaway transport. Now that consumers cache and reuse a single *http.Client/*http.Transport (to fix a socket-leak bug), that cap becomes a hard, unqueued-timeout ceiling of 10 concurrent in-flight requests per host for the life of the process -- e.g. terraform apply -parallelism=25 silently serializes into batches of 10 with no bound on queue wait, since the client has Timeout: 0 and requests carry no context deadline. Set MaxConnsPerHost to 0 (unbounded, matching net/http.DefaultTransport) while leaving the idle-connection pool limits (MaxIdleConns/MaxIdleConnsPerHost) unchanged.
…hadows the env var GetServerConfig() serialized the resolved HttpClientTimeout verbatim, including the 60s value ValidateAuthConfig synthesizes when nothing was configured. Callers that persist GetServerConfig()'s output to a config file (e.g. kfutil's login flow, which writes to ~/.keyfactor/command_config.json) therefore always wrote client_timeout: 60 to disk even when the user chose nothing. On the next run, LoadConfig merges that file value into HttpClientTimeout before ValidateAuthConfig runs (mirroring how Host/Port/etc. are merged), so ValidateAuthConfig's `if c.HttpClientTimeout <= 0` guard was already false and the KEYFACTOR_CLIENT_TIMEOUT env var branch was skipped -- permanently and silently shadowing the env var. This is a regression: the env var always worked before Server gained a ClientTimeout field to persist. Track whether HttpClientTimeout's value was synthesized by the package-default fallback (new unexported clientTimeoutDefaulted field, cleared by WithClientTimeout) versus explicitly configured, and have GetServerConfig() omit ClientTimeout (via its existing omitempty tag) whenever it was only defaulted. Explicit values (struct/ WithClientTimeout(), env var, or an existing file value) are still persisted, and the round-1 precedence order is unchanged.
The request-body redactor only inspected each JSON value's own key against sensitiveBodyKeys and never re-parsed string values that were themselves JSON documents, leaving two confirmed leak paths: - keyfactor-go-client v3 marshals a certificate store's Properties map into a JSON-encoded STRING field. terraform-provider-keyfactor puts ServerPassword in that map (and for K8S store types this field can carry an entire kubeconfig/service-account token), so it was emitted verbatim in generated curl commands. - PAM provider creation carries its secret under the generic key "Value", nested under ProviderTypeParamValues. "value" wasn't in sensitiveBodyKeys, so a Vault token/Delinea password was logged verbatim. redactJSONValue now recognizes string values that look like a JSON document (balanced outer brackets), re-parses and redacts them recursively, and re-serializes the result -- bounded by a depth limit (maxNestedJSONStringDepth) and size limit (maxNestedJSONStringLen) to bound the cost of adversarial nesting. A string that looks like JSON but fails to parse, or that hits either guard, is redacted in its entirety rather than ever emitted raw. sensitiveBodyKeys gains serverpassword, storepassword, newpassword, relaypassword, pkcs12blob, and value. "value" is blanket-redacted (rather than only within a credential-bearing parent) since this redactor walks structure without tracking its ancestry, and a parent-key allowlist would still miss future generic-"Value" secret fields; "properties" is deliberately NOT added, since blanket-hiding it would erase non-secret store configuration -- the nested-JSON-string handling above already redacts secrets within it while preserving the rest of its structure.
CommandAuthConfigBasic.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout. Since CommandAuthConfigBasic is what real basic-auth callers actually construct, the round-2 fix never took effect for them. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer basic-auth-specific fields on top.
…ation CommandAuthConfigKerberos.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout, so the fix never took effect for real Kerberos callers. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer Kerberos-specific fields on top.
CommandConfigOauth.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout, so the fix never took effect for real OAuth callers. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer OAuth-specific fields on top.
…heck looksLikeJSONDocument only inspected the first/last byte after strings.TrimSpace to decide whether a nested string value looked like JSON worth re-parsing and redacting. TrimSpace does not strip a U+FEFF byte-order-mark, so a nested JSON-encoded string value prefixed with a BOM (e.g. a PAM/orchestrator service-account JSON key embedded in a Properties map value, plausible from a Windows-authored file) was judged "not JSON" and returned completely unredacted. encoding/json also rejects a leading BOM outright rather than tolerating it, so the fix strips the BOM explicitly before both the heuristic check and the actual json.Unmarshal call.
The initial client_credentials token fetch performed during Configure() was unbounded: CommandConfigOauth.GetHttpClient() injected an http.Client with Timeout left at its zero value into the oauth2 token source's context, and that ctx/client pair is captured once and reused forever, permanently divorced from HttpClientTimeout enforcement applied to the outer client elsewhere. Compounding this, newHTTPTransport() never set DialContext, so the TCP dial phase itself had no ceiling at all -- a black-holed destination (no RST/ICMP, just silence) hung indefinitely, regardless of any configured request_timeout. Bound both gaps: - Give the token-fetch client a real Timeout derived from HttpClientTimeout (falling back to DefaultClientTimeout if somehow unset), so the whole token-fetch call is bounded. - Add a fixed DefaultDialTimeout (30s, matching net/http.DefaultTransport) to newHTTPTransport()'s DialContext, so the dial phase specifically fails fast even when a large HttpClientTimeout is configured for slow request bodies elsewhere.
…ame omission GetAccessToken() built its own context.Background() for the oauth2 client_credentials token fetch, so a TCP-connected-but-unresponsive token endpoint hung the call forever -- the same unbounded-hang hazard this round's GetHttpClient() fix just closed, reachable through this sibling entry point instead. Extract oauthTokenFetchContext() as a shared helper for building a properly-bounded oauth2 context (transport + Timeout derived from HttpClientTimeout) and use it from both GetHttpClient() and GetAccessToken(), rather than duplicating the construction. Also fix TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout, which omitted CommandHostName and therefore only exercised the intended hung-body assertion by accident of an ambient KEYFACTOR_HOSTNAME left set in the dev/review shell -- in a clean environment it failed at the hostname-validation gate before ever reaching the fake token server.
oauthTokenFetchContext previously bounded a token fetch only via an http.Client.Timeout field, which http.Client.Do() re-derives fresh on every call. golang.org/x/oauth2/internal.RetrieveToken silently makes up to two sequential HTTP round trips per logical client_credentials fetch (it probes AuthStyleInHeader, then retries with AuthStyleInParams on any failure) using the same context, so each attempt got its own full HttpClientTimeout budget -- doubling the real-world worst-case cost of a hard failure to ~2x HttpClientTimeout. An HttpClientTimeout of 15s against an unroutable endpoint measured as exactly 30s, which coincidentally equals DefaultDialTimeout and looked like a dial-timeout bug but wasn't. oauthTokenFetchContext now derives its context via context.WithTimeout so both sequential attempts share one absolute deadline: once the first attempt exhausts the budget, the second fails immediately rather than getting a fresh window. Since that context must not be reused across future token refreshes (its deadline is relative to creation time), GetHttpClient()'s cached, long-lived token source is refactored to build a fresh context on every actual refresh via a new boundedClientCredentialsTokenSource wrapped in oauth2.ReuseTokenSource, rather than capturing one context/http.Client pair once and reusing it forever. Verified against a black-holed destination that the aggregate now matches the configured value across 5s/25s/45s (bracketing DefaultDialTimeout=30s), instead of 2x or a fixed 30s.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #51.
Problem
Serverhad no client-timeout field, so everyGetServerConfig()implementation silently droppedCommandAuthConfig.HttpClientTimeout. Downstream consumers that authenticate once and rebuild a client from the returned*Server(keyfactor-go-client v3NewKeyfactorClient, keyfactor-go-client-sdkNewAPIClient) lost the configured timeout and fell back to the 60sDefaultClientTimeout. Real-world symptom: terraform-provider-keyfactor'srequest_timeout = 300still producednet/http: timeout awaiting response headersat ~60s on slow PFX enrollments.Fix
This PR went through 7 rounds of adversarial review (correctness/compliance/security lenses + adjudicated dismissals) plus live-lab re-verification against a real Keyfactor Command instance. Each round after the first found and closed a real gap the previous round left open. Final state:
Server.ClientTimeoutadded, populated fromHttpClientTimeoutin all fourGetServerConfig()implementations (core, basic, oauth, kerberos) — each concrete type delegates to the base implementation so persistence-gating logic can't drift out of sync across auth types (an earlier fix patched only the base type, missing all three concrete types real callers construct).Get*ClientConfig) and inLoadConfig, so aclient_timeoutin a config-file profile is no longer silently ignored.GetServerConfig()only persists an explicitly configured timeout — never one that was merely defaulted. Without this, a tool that authenticates once and writes the resultingServerback to disk (e.g.kfutil) would permanently bake in a synthesized 60s default that then silently shadowsKEYFACTOR_CLIENT_TIMEOUTon every future run.KEYFACTOR_CLIENT_TIMEOUTvalue that's empty, non-numeric, or non-positive now falls back to the 60s default instead of producing an unlimited timeout (Go treats0as "no timeout").RequestToCurl's TRACE-level request logging redacts secret-bearing bodies, including secrets nested inside JSON-encoded string values (e.g. Command's certificate-storePropertiesfield, which can carry K8S kubeconfigs) and BOM-prefixed nested JSON. Previously only theAuthorizationheader was redacted; POST/PUT bodies — including PFX enrollment passwords and PAM secrets — were logged in plaintext wheneverTF_LOG=TRACEwas set.BuildTransport()/SetClient()no longer scaleIdleConnTimeout/ExpectContinueTimeout/TLSHandshakeTimeoutwith the configured request timeout (a 30-minuterequest_timeoutused to hold idle sockets open for 30 minutes); fixed defaults matchinghttp.DefaultTransportare used instead, and onlyResponseHeaderTimeoutstill tracks the configured value.MaxConnsPerHostis no longer hardcoded to 10 — that cap was harmless per-request but became a hard global concurrency ceiling once downstream consumers correctly started caching a single*http.Client.client_credentialstoken fetch duringConfigure()is now genuinely bounded by the configured timeout. This required two separate fixes discovered only via live end-to-end testing (unit tests with mocked transports could not surface either): (1) the token-fetch call had no ceiling at all on the TCP dial phase — a black-holed network path hung indefinitely regardless of any configured timeout; fixed with a bounded token-fetch client plus a fixed 30s dial-connect timeout on the shared transport. (2) Once that was fixed, live re-verification found the hang was now bounded, but at a fixed 30 seconds regardless of the configured value — root cause:golang.org/x/oauth2silently performs up to two sequential HTTP round trips per token fetch (an AuthStyle probe), and each attempt was getting its own fresh timeout budget rather than sharing one deadline, doubling worst-case latency (15s configured → 30s actual, which coincidentally matched the unrelated dial-timeout constant and looked like a different bug entirely). Fixed by giving both attempts one shared absolute deadline viacontext.WithTimeout. Verified live against a black-holed network target at four distinct configured values (10s/15s/20s/40s) — each now fails at approximately its own configured value, not a shared fixed number.Tests
Extensive unit coverage: round-trip precedence tests, config-file load/persist symmetry, malformed-env-value handling, body-redaction coverage (including nested JSON-in-string and BOM-prefixed variants) across all auth types, transport-timeout-independence assertions, and token-fetch-boundedness tests (network-free, asserting exactly one HTTP request reaches the token endpoint and elapsed time stays within the configured budget).
Validation
Final tag
v1.6.0-rc.5was propagated through the full downstream chain and verified live: keyfactor-go-clientv3.6.0-rc.4and keyfactor-go-client-sdkv24.1.2-rc.4build and test green against it, and terraform-provider-keyfactor's full unit suite (345 pass/0 fail/3 skip) passes against the published RCs with no local replaces — plus a live re-verification against a real Keyfactor Command instance confirming both the originalrequest_timeoutfix and theConfigure()-time OAuth timeout fix work end-to-end, not just at the unit-test level.Release note:
v1.6.0-rc.0–rc.2are taken by an unrelated, unmerged config-loader feature branch; this branch does not contain that work. Finalv1.6.0should be cut frommainafter both lines merge.