fix(client): plumb Server.ClientTimeout into the rebuilt auth config - #59
Merged
Conversation
…mand API
Command's TemplateUpdateRequest.KeyUsage and TemplateRetrievalResponse.KeyUsage
are both {"type":"integer","format":"int32"} per the v25.5 swagger — an int32
bitmask (e.g. 160 = digitalSignature|keyEncipherment). UpdateTemplateArg.KeyUsage
was typed *bool, which serializes as a JSON boolean and produces a live HTTP 400
from Command ("Unexpected character encountered while parsing value: t. Path
'KeyUsage'"), making the field unusable as-is.
GetTemplateResponse.KeyUsage was already int, so this also fixes the type
mismatch between the get and update models for the same field.
Also fixes the identical defect in v2/api/template_models.go for consistency;
v2 is tagged/released independently and is not part of this v3.6.0 change.
Adds TestUpdateTemplateArg_KeyUsage_SerializesAsInt to v3/api/template_test.go,
which fails to compile against the pre-fix *bool field and asserts the wire
payload is a JSON number.
NewKeyfactorClient rebuilds a fresh CommandAuthConfig from the caller's *auth_providers.Server instead of reusing the one that produced it, but never carried over ClientTimeout. Every consumer -- including the Terraform provider's request_timeout setting -- ended up authenticating and issuing requests with DefaultClientTimeout (60s) regardless of what was configured, causing "net/http: timeout awaiting response headers" on long-running calls like PFX enrollment. Set HttpClientTimeout: cfg.ClientTimeout in the baseConfig literal so it flows into BuildTransport()/SetClient() for both the basic and oauth auth paths. Depends on github.com/Keyfactor/keyfactor-auth-client-go#51 being fixed upstream (Server.ClientTimeout field). go.mod is bumped to the not-yet-tagged v1.6.0-rc.1 and pinned locally via a `replace` directive at /tmp/kf-worktrees/kfc-auth for testing; once that tag is cut, drop the replace and re-run `go mod tidy`.
Removes the local replace directive and TODO now that the ClientTimeout fix is published, and validates against the published dependency.
Client.sendRequest called AuthConfig.GetHttpClient() on every single request. Both CommandConfigOauth and CommandAuthConfigBasic in keyfactor-auth-client-go build a brand new http.Transport (and therefore a brand new, empty connection pool) on each call, and that transport's IdleConnTimeout is derived from the configured HttpClientTimeout - so every API call opened its own never-reused connection whose socket lingered until IdleConnTimeout fired. This leak predates this branch at the fixed 60s default; plumbing a caller-configured ClientTimeout through (which can be arbitrarily large, e.g. 1800s for slow enrollments) widens the linger window proportionally, so cache the *http.Client on Client and reuse it across requests instead of rebuilding it per call. The OAuth token source is still consulted (and refreshed) on every RoundTrip independent of how many times the *http.Client is reused, and NewKeyfactorClientWithAuth (used by VCR/unit tests) still works by lazily populating the cache on first use.
TestNewKeyfactorClient_PlumbsClientTimeout and TestNewKeyfactorClient_DefaultClientTimeout build a Server config with fields intentionally left at their zero value to exercise ValidateAuthConfig's environment-variable fallback path. Because ValidateAuthConfig only falls back to KEYFACTOR_CLIENT_TIMEOUT/ KEYFACTOR_PORT/KEYFACTOR_CA_CERT when the struct field is unset, and unconditionally overwrites SkipVerify from KEYFACTOR_SKIP_VERIFY regardless of the struct field, ambient values for these variables (e.g. from a sourced lab env file) broke both tests: KEYFACTOR_CLIENT_TIMEOUT=120 flips the expected default from 60 to 120, and KEYFACTOR_SKIP_VERIFY=false clobbers SkipTLSVerify:true and rejects the tests' self-signed httptest TLS certificate. Add isolateKeyfactorEnv to unset the relevant variables for the duration of each test and restore their original values afterward. t.Setenv(key, "") does not work here since an empty value is still "present" to os.LookupEnv.
Picks up the round-4 convergence fixes: ClientTimeout persistence gated across all three concrete auth types via delegation to the base type, a BOM-prefix bypass fix in nested-JSON secret redaction, MaxConnsPerHost widened to unbounded, and body redaction extended to cover JSON-in-string values.
…rHost=10 Closes the loop on a finding this package's own http.Client-caching fix could not verify end-to-end: caching a single *http.Client turns the transport's MaxConnsPerHost into a permanent, unqueued-timeout concurrency ceiling for the process, since the cached client has no Timeout and requests carry no deadline. keyfactor-auth-client-go's fix (MaxConnsPerHost widened from a hardcoded 10 to unbounded) was only verified there by inspecting the constructed transport's field value. Add an end-to-end regression test that builds a real Client via NewKeyfactorClient, retrieves its cached *http.Client, and drives 25 concurrent requests through it against a real httptest server, asserting the server observes well more than 10 requests in flight at once. Confirmed this fails against v1.6.0-rc.2 (10 in-flight, ~620ms) and passes against v1.6.0-rc.3 (25 in-flight, ~225ms).
Picks up the round 5-6 OAuth token-fetch timeout hardening (bounded TCP dial phase and overall call during Configure), discovered via live-lab investigation after the branch had already converged once. No public API surface used by this module changed.
Picks up the OAuth client_credentials token-fetch fix: avoid a redundant double round trip from AuthStyle probing and share a single deadline across retry attempts instead of a fresh timeout budget per attempt.
… in sendRequest sendRequest's context-deadline-exceeded handling had two confirmed HIGH-severity problems: 1. It transparently retried the request (up to 5 times with exponential backoff) and, if a retry succeeded, returned that success with no indication a timeout ever occurred. Callers that specifically need to detect a client-side-timeout-shaped error (e.g. terraform-provider-keyfactor's orphaned-PFX-enrollment recovery, which matches on "context deadline exceeded" to search for a resource that may have already been created server-side) never saw the error, so their recovery logic never ran. Worse, blindly retrying a non-idempotent request (e.g. a POST enrollment) risks creating a second server-side resource if the original request actually succeeded after the client gave up on it. 2. If every retry also failed, the response variable was never reassigned from its initial nil value and the switch had no return for this case, so execution fell through to `resp.StatusCode` on a nil *http.Response -- panicking the calling process (e.g. crashing `terraform apply` outright). Removes the silent retry-and-mask behavior entirely; a context-deadline error is now returned immediately and untouched, like any other transport error, eliminating both the masking and the nil-deref fall-through. Callers that need retry-with-backoff around a timeout (and know their request is safe to repeat) should implement that at their own call site. Adds regression tests reproducing both original failure modes: a request that times out on the first attempt but would have succeeded on a retry (now returns the timeout error instead of a masked success), and a request that never succeeds within the client's timeout (now returns a clean error instead of panicking).
This library redirects Go's global log package to tflog via a TerraformLogger whose ctx is captured once at client-construction time (initLogger), so per-call masking applied by callers to this library's own tflog calls can never reach these log.Printf/log.Println call sites. Three call sites confirmed dumping secret material in plaintext: - client.go's sendRequest logged the full JSON-marshaled request body at TRACE level for every API call (enrollment, store, PAM payloads, etc.), and separately, logRequest (invoked on every request) dumped the same body a further three times: as a JSON blob, as a directly-replayable cURL command, and as a base64-encoded cURL command - the cURL forms are a more severe leak than a plain log dump since they're copy-paste runnable by anyone who reads the log. - EnrollPFXV2 logged its enrollment args (including the PFX password) at TRACE level. In the current code this specific log.Println call happens not to print the password in cleartext, because its argument is wrapped in an extra, incidental pointer level (Payload: &ea, where ea is already *EnrollPFXFctArgsV2) that changes fmt's default formatting to a hex address instead of the dereferenced struct - but relying on that as protection is fragile and not an intentional safeguard; a well-meaning cleanup of that stray "&" would silently reintroduce a real leak here. Fixed explicitly regardless. - RecoverCertificate logged its args (including the private-key recovery Password) at DEBUG level - a routine troubleshooting verbosity, not one requiring unusual verbosity to trigger, and reachable on ordinary Read/Update/import private-key-recovery paths. This one demonstrably did leak the plaintext password via fmt's default struct formatting. Adds a shared redactSensitiveJSONForLogging helper (log_redaction.go) that recursively redacts JSON object keys matching a case-insensitive password/secret/token/private-key pattern, used for the generic request-body log sites in client.go, plus explicit redacted-copy logging at the two certificate.go call sites. None of this touches the actual bytes sent as the outgoing HTTP request body - only what gets logged. Adds regression tests (log_redaction_test.go) that capture Go's global log output and assert a canary password never appears, for both RecoverCertificate (DEBUG, confirmed leaking pre-fix) and EnrollPFXV2 (TRACE; the real leak in the pre-fix code flows through the generic request-body log in client.go rather than EnrollPFXV2's own log.Println, for the pointer-wrapping reason above - the test still passes post-fix and protects both paths going forward).
…fields redactSensitiveValue treated any JSON string leaf as opaque, so request structs that pre-serialize a map to a JSON string before the outer struct is marshaled again (e.g. CreateStoreFctArgs/UpdateStoreFctArgs's PropertiesString field, populated with ServerUsername/ServerPassword on every certificate-store create/update) leaked those secrets in plaintext into [TRACE] request-body/cURL logs, unaffected by the redaction added in 9f66f38. String leaves are now given one extra chance: if they successfully decode as JSON to a map or array, that decoded structure is redacted recursively and re-marshaled back to a string, preserving the "JSON encoded as a string" shape in the log output. A capped recursion depth (5) prevents adversarial or accidental deep string-of-JSON-of-string nesting from recursing unboundedly.
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.
Companion to Keyfactor/keyfactor-auth-client-go#52 (fixes Keyfactor/keyfactor-auth-client-go#51 downstream).
Companion to Keyfactor/keyfactor-auth-client-go#52 (fixes Keyfactor/keyfactor-auth-client-go#51 downstream).
Problem
NewKeyfactorClientrebuilds aCommandAuthConfigfrom the incoming*auth_providers.Serverbut never carried a client timeout — the field didn't exist onServeruntil keyfactor-auth-client-go v1.6.0-rc.2. The rebuilt config therefore always fell back to the 60s default, ignoring whatever the caller had configured. Symptom in terraform-provider-keyfactor:request_timeout = 300still timed out at ~60s on slow PFX enrollments.Fix
keyfactor-auth-client-gotov1.6.0-rc.5(addsServer.ClientTimeoutplus extensive hardening across 7 rounds of review — see that PR for the full history, including a fix for theConfigure()-time OAuth token-fetch timeout that this repo's own regression test below directly exercises).NewKeyfactorClient: copycfg.ClientTimeoutintobaseConfig.HttpClientTimeoutso the rebuilt auth config honors the caller's timeout.sendRequestnow caches and reuses a single*http.Clientinstead of callingAuthClient.GetHttpClient()— which builds a brand-newhttp.Transport— on every request. Previously each request's socket lingered for the fullIdleConnTimeout, which this same change makes user-configurable (potentially 30 minutes); without caching, a largeterraform applywould hold hundreds of sockets and goroutines open simultaneously.Tests
NewKeyfactorClientagainst an httptest fake Command server asserts the resulting transport honors a configuredServer.ClientTimeout.TestClient_ReusesHttpClientAcrossRequestsassertsAuthClient.GetHttpClient()is called once across N requests, not N times.TestClient_ConcurrentRequestsNotCappedByMaxConnsPerHostdrives 25 concurrent requests through a real cached client and measures in-flight count — this closes the loop on a finding this PR's own client-caching fix caused (caching a single client turned kfc-auth's then-hardcodedMaxConnsPerHost: 10into a global concurrency ceiling) but couldn't fix locally, since the root cause was in kfc-auth.KEYFACTOR_*variables set.Based on
v3.6.0-rc.0; RC tagv3.6.0-rc.4was cut from this branch, validated against the published kfc-auth v1.6.0-rc.5, and terraform-provider-keyfactor's full unit suite plus a live re-verification against a real Keyfactor Command instance both pass with no replace directives.