fix(core): preserve HttpClientTimeout in Server via ClientTimeout field - #52
Open
spbsoluble wants to merge 10 commits into
Open
fix(core): preserve HttpClientTimeout in Server via ClientTimeout field#52spbsoluble wants to merge 10 commits into
spbsoluble wants to merge 10 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.
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
Serverhas no client-timeout field, so everyGetServerConfig()implementation silently dropsCommandAuthConfig.HttpClientTimeout. Downstream consumers that authenticate once and rebuild a client from the returned*Server(keyfactor-go-client v3NewKeyfactorClient, keyfactor-go-client-sdkNewAPIClient) lose the configured timeout and fall back to the 60sDefaultClientTimeout— all transport timeouts (ResponseHeaderTimeoutet al.) derive from it inBuildTransport()/SetClient(). Real-world symptom: terraform-provider-keyfactor'srequest_timeout = 300still producednet/http: timeout awaiting response headersat ~60s on slow PFX enrollments.Fix
Server.ClientTimeout(client_timeoutjson/yaml).HttpClientTimeoutin all fourGetServerConfig()implementations (core, basic, oauth, kerberos).GetBasicAuthClientConfig/GetOAuthClientConfig/GetKerberosClientConfig) so round-trips are lossless.Tests
5 new tests:
WithClientTimeout(300)→GetServerConfig().ClientTimeout == 300round-trips for each auth type, andBuildTransport()uses the configured value forResponseHeaderTimeout. Red before the fix (field absent), green after.Validation
RC tag
v1.6.0-rc.2was cut from this branch and validated through the full downstream chain: keyfactor-go-clientv3.6.0-rc.1and keyfactor-go-client-sdkv24.1.2-rc.1build and test green against it, and terraform-provider-keyfactor's full unit suite (323 pass / 0 fail) passes against the published RCs with no local replaces.Release note:
v1.6.0-rc.0/-rc.1were cut from the (unmerged) config-loader feature branch; this branch does not contain that work. Finalv1.6.0should be cut frommainafter both lines merge.