fix(client): plumb Server.ClientTimeout into rebuilt auth configs (v24 v1+v2) - #37
Open
spbsoluble wants to merge 11 commits into
Open
fix(client): plumb Server.ClientTimeout into rebuilt auth configs (v24 v1+v2)#37spbsoluble wants to merge 11 commits into
spbsoluble wants to merge 11 commits into
Conversation
Keyfactor Command serializes WeeklyModel.Days as day-name strings (e.g. "Monday") in some API responses, but SystemDayOfWeek.UnmarshalJSON only accepted JSON integers, causing Weekly-scheduled resources to fail to deserialize. Try the integer form first, preserving existing enum validation, and fall back to the existing Parse() day-name mapping when the payload is a JSON string. Malformed strings and out-of-range ints still error clearly. Applies to both v1 and v2 API packages in this module.
Fixes keyfactor-pub/terraform-provider-keyfactor#185: Keyfactor Command v25.5 serializes WeeklyModel.Days as day-name strings (e.g. "Monday") in GET /CertificateAuthority responses, but SystemDayOfWeek.UnmarshalJSON only accepted JSON integers, causing any Weekly-scheduled CA to fail to deserialize. Try the integer form first, preserving existing enum validation, and fall back to the existing Parse() day-name mapping when the payload is a JSON string. Malformed strings and out-of-range ints still error clearly. Applies to both v1 and v2 API packages in this module.
buildHttpClientV2 in both v24/api/keyfactor/v1/client.go and v24/api/keyfactor/v2/client.go rebuilds a fresh CommandAuthConfig from the caller's *auth_providers.Server but never carried over ClientTimeout. Every consumer ended up authenticating and issuing requests with DefaultClientTimeout (60s) regardless of what was configured upstream (e.g. the Terraform provider's request_timeout), producing "net/http: timeout awaiting response headers" on long-running calls like PFX enrollment. Set HttpClientTimeout: cfg.ClientTimeout in both baseConfig literals. This is a hand-edit to generator output (see HAND_EDITS.md); do not drop it on regen. 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`.
Establishes HAND_EDITS.md for this branch, scoped to v24 (no v24 swagger exists yet, so it isn't covered by any regen pipeline). Documents commit 2a6c5b4 so a future regeneration effort doesn't silently drop the Server.ClientTimeout plumbing fix.
Removes the local replace directive and TODO now that the ClientTimeout fix is published, and validates against the published dependency. Vet disabled for the test run due to a pre-existing non-constant format string in generated configuration.go (unrelated to this change).
…t env ValidateAuthConfig (keyfactor-auth-client-go) unconditionally overwrites SkipVerify from KEYFACTOR_SKIP_VERIFY whenever the variable is merely present, regardless of value. Anything other than exactly "true"/"1" (e.g. "TRUE", "false", "0", or an empty string sourced from a lab env file) flips SkipTLSVerify back to false, so TestBuildHttpClientV2_ClientTimeoutPropagation failed against its own self-signed httptest server with "x509: certificate signed by unknown authority" on any machine with such a value exported. Pin KEYFACTOR_SKIP_VERIFY to "true" for the duration of the test and neutralize KEYFACTOR_CA_CERT (a stale path would make BuildTransport fail) and KEYFACTOR_CLIENT_TIMEOUT (defense-in-depth) via a proper unset-and-restore helper, since t.Setenv(key, "") does not unset a variable for os.LookupEnv purposes. Verified green with KEYFACTOR_SKIP_VERIFY set to false, TRUE, 0, and empty string, in both v1 and v2 packages.
buildHttpClientV2 in v25/api/keyfactor/v1/client.go and v2/client.go rebuilds its own auth_providers.CommandAuthConfig from the caller's Server but never copied ClientTimeout into HttpClientTimeout, so every v25 caller silently fell back to the 60s default regardless of what was configured -- the same bug already fixed for v24 in 2a6c5b4. Server.ClientTimeout only exists starting at keyfactor-auth-client-go v1.6.0-rc.2 (added for issue #51), so bump v25's dependency from v1.3.0 to match. `go mod tidy` and `go build ./...` are clean with no API compatibility breaks between those versions. Port TestBuildHttpClientV2_ClientTimeoutPropagation (and its unsetEnvForTest hermeticity helper) from v24 to v25's v1 and v2 packages verbatim. Verified green, including with KEYFACTOR_SKIP_VERIFY set to false, TRUE, 0, and empty string.
Commit 229db7d added "&& serverConfig.Port != 443" to prepareRequest's port guard in both v1 and v2 client.go (skip appending port 443 to the request host to avoid duplicate/explicit-port HTTPS URLs), but shipped with no test. Reverting that guard would fail nothing in CI despite being an auth/URL-critical hand-edit. Add TestPrepareRequest_Port443Guard to v24 and v25's v1 and v2 packages, covering both the omitted-port-443 case and a non-443 port to confirm the guard doesn't over-broadly strip other ports. Constructs the APIClient directly via its exported AuthClient field rather than NewAPIClientWithAuth, so the test doesn't depend on an unrelated hand-edit (and works uniformly in v25, which never got that helper ported from v24). Verified red against the pre-229db7d guard (asserts URL.Host == "command.example.com:443" instead of without the port) and green against the current guard, in all four packages.
… scope v25
The previous version of this document claimed to catalog every
post-generation hand-edit under v24/api/keyfactor/v{1,2}/ but only
listed the ClientTimeout edit -- 11 non-test files differ from the
v24 generation baseline (2ed41db), 10 of them uncataloged. Enumerate
all of them: NewAPIClientWithAuth, the OAuth AccessToken/Audience/Scopes
restoration, the prepareRequest port-443 guard (previously untested --
see the companion test commit), the CA cleanup/enrollment fields, the
EnrollmentType bitmask values, KeyRetentionPolicy's string-form
UnmarshalJSON, the new CertificateCleanupTimeUnits enum, SystemDayOfWeek's
day-name fallback, and the template cleanup/Manageability fields. Each
entry states plainly whether a regression test protects it.
Also corrects a factual error: the .openapi-generator-ignore files are
1040 bytes of generator boilerplate comments, not empty.
Narrow the document's stated scope to v24 only, since v25's independent
hand-edit history (from its own generation baseline) has not been
audited to the same standard -- document only the two v25 edits made
as part of this change set (ClientTimeout plumbing, port-443 test) and
say so explicitly, rather than letting the file imply broader v25
coverage it doesn't have.
Note the root and v2 modules carry the identical ClientTimeout bug and
were verified fixable (dependency bump builds clean), but are left
unfixed here because they have no existing test scaffolding and appear
to have no active consumers -- documented as a deliberate scope
decision, not a silent gap.
…HttpClientV2 HAND_EDITS.md's v24 entry #2 claimed TestCommandConfigOauth_AccessTokenFieldPropagation pinned the AccessToken/Audience/Scopes restoration in buildHttpClientV2's OAuth branch. It doesn't: that test only builds its own local CommandConfigOauth literal and never calls buildHttpClientV2, so deleting the three fields from the actual hand-edit still compiled and passed. Add TestBuildHttpClientV2_OAuthAccessTokenPropagation, which drives buildHttpClientV2 end-to-end against a fake Command server with an access-token-only Server config and asserts the token reaches the wire as a Bearer credential. Reproduced red (fails with a client_id-required error) against the pre-fix literal, green with it restored. Applied identically to v24 v1/v2 and ported to v25 v1/v2, which had the same gap. Corrected the HAND_EDITS.md entry to describe what actually protects this hand-edit now.
Picks up the fully-converged fix set: gated ClientTimeout persistence across all three concrete auth types (delegated to the base type, affecting CommandConfigOauth.GetServerConfig() which buildHttpClientV2 constructs and calls), a BOM-prefix bypass fix in nested-JSON secret redaction, an unbounded MaxConnsPerHost fix, and body redaction coverage for JSON-in-string values such as Command's certificate-store Properties field and PAM's Value key. Validated against the published tag with no replace directive: go build and go test (-vet=off for the pre-existing generated-code printf vet failure, unrelated) are green in both v1 and v2 packages of both modules, including the ClientTimeout/OAuth-field propagation and port-443 guard regression tests and their env-isolation hermeticity helpers.
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); same fix as Keyfactor/keyfactor-go-client's v3 change.
Problem
buildHttpClientV2(bothv24/api/keyfactor/v1/client.goandv2/client.go) rebuilds aCommandAuthConfigfrom the incoming*auth_providers.Serverwithout a client timeout, so SDK clients always fell back to the 60s default regardless of what the caller configured before handing off theServer.Fix
keyfactor-auth-client-gotov1.6.0-rc.2(addsServer.ClientTimeout).cfg.ClientTimeoutintoHttpClientTimeoutin both hand-editedbuildHttpClientV2implementations. No code regeneration — the edit is documented inHAND_EDITS.md(new file at this base; none existed atv24.1.2-rc.0).Tests
New tests call
buildHttpClientV2directly against a fake server and assert the configured timeout reaches the transport; red→green. Note:go testneeds-vet=offin the v24 module due to a pre-existingnon-constant format stringvet failure in generatedconfiguration.go(introduced by the original "Generate V24 client" commit, unrelated to this change).Based on
v24.1.2-rc.0; RC tagv24.1.2-rc.1was cut from this branch and terraform-provider-keyfactor's full unit suite passes against it.