chore(deps): Bump keyfactor-auth-client-go to v1.1.2 - #39
Open
spbsoluble wants to merge 56 commits into
Open
Conversation
go: upgraded github.com/Azure/azure-sdk-for-go/sdk/azcore v1.16.0 => v1.17.0 go: upgraded github.com/Azure/azure-sdk-for-go/sdk/azidentity v1.8.0 => v1.8.1 go: upgraded github.com/fatih/color v1.13.0 => v1.18.0 go: upgraded github.com/hashicorp/go-hclog v1.5.0 => v1.6.3 go: upgraded github.com/mattn/go-colorable v0.1.13 => v0.1.14 go: upgraded github.com/mattn/go-isatty v0.0.19 => v0.0.20 go: upgraded golang.org/x/crypto v0.30.0 => v0.32.0 go: upgraded golang.org/x/net v0.32.0 => v0.34.0 go: upgraded golang.org/x/oauth2 v0.24.0 => v0.25.0 go: upgraded golang.org/x/sys v0.28.0 => v0.29.0 ```
… commas. feat(certs): Add `collectionId` support for certificate downloads.
Expand secret type (#47)
… validation logic to require a `subject` or at least 1 `SAN`
…Type`, `AlternativeKeyLength` to `EnrollPFXFctArgsV2`
…KeySizeInBits,AltKeyType,IssuedEmail,AltSigningAlgorithm,AltKeyTypeString,HasAltPrivateKey,CARecordId,Curve,EnrollmentPatternId` to `GetCertificateResponse` model
…to `StorePasswordConfig` on `UpdateStoreFctArgs`
…h empty for enrollments
…Key,RenewalCertificateId,AdditionalEnrollmentFields,EnrollmentPatternId,OwnerRoleId,OwnerRoleName,IncludeSubjectHeader`
V25 enrollment updates
…ayloads. feat(models/stores): Add `RemoteProviderName` to `StorePasswordConfig` Signed-off-by: spbsoluble <1661003+spbsoluble@users.noreply.github.com>
…return raw JSON response in error if possible. Signed-off-by: spbsoluble <1661003+spbsoluble@users.noreply.github.com>
## Summary This PR adds several new API capabilities and bug fixes targeting Keyfactor Command v25+: - **Applications API** — Full CRUD support (`List`, `Get`, `Create`, `Update`, `Delete`) for the `/Applications` endpoint, including all schedule types and backwards compatibility for Command versions prior to v25 - **PAM Providers & Types** — Full CRUD for `/PamProviders` and `/PamProviders/Types`, with a `GetPamProviderByName` helper; model fixes for `ProviderType.Name` and store `Password` field types - **Enrollment Patterns** — Full CRUD for `/EnrollmentPattern`, with new model fields; PFX enrollments can now specify `EnrollmentPatternId` or `Template` (rather than requiring both) - **Certificate enhancements** — New fields on `GetCertificateResponse` (owner role, alt key info, curve, etc.), CSR enrollment args expanded, base64 response from `DownloadCertificate`, `findLeafCert` helper, and graceful handling of ed448 keys - **Store improvements** — Immediate inventory scheduling, `PUT` method capitalization fix, improved error messaging when deserializing store responses, password config model alignment between create/update - **Store types** — Paginate `ListStoreTypes` to avoid truncation on large deployments --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
The Applications API returns at most 50 results per page. With 50+ apps in the lab, newly created apps were not visible to ListApplications, causing TestIntKeyfactorApplicationDataSource to fail consistently. Pagination uses existing PageReturned/ReturnLimit query params (same pattern as CertificateStoreTypes). Regression tests added.
## Summary `UnpackPEM` selected the leaf certificate **positionally** — it assumed `certificates[0]` was the end-entity leaf: ```go // before certificate = certificates[0] caCertificates = certificates[1:] ``` This returns the **root CA** as the leaf whenever Keyfactor Command sends a PEM bundle that is not leaf-first. Externally-rooted chains (e.g. DigiCert PKIaaS) are commonly returned **root-first**, so `certificates[0]` is the root. Consumers that trust the returned leaf (e.g. the Terraform provider populating `common_name` / `certificate_pem`) then persist the root CA's subject, forcing certificate replacement on every run. ## Fix Select the leaf by chain topology using the package's existing `findLeafCert` (the cert no other cert in the set issued) — the same helper `DownloadCertificate` already uses for the P7B path. The remaining certs become the CA chain, preserving their original order. Falls back to index 0 when no certs parse, preserving prior behavior for degenerate inputs. This makes leaf selection order-independent and consistent across the P7B and PEM code paths. ## Also included - **go.sum:** added the missing `github.com/spbsoluble/go-pkcs12 v0.4.0` module zip checksum (`h1:`). `go.mod` pins v0.4.0 but `go.sum` only carried the `/go.mod` hash, so clean builds failed with `missing go.sum entry for module providing package github.com/spbsoluble/go-pkcs12`. ## Tests New `v3/api/unpackpem_leaf_test.go`: - `TestUnpackPEM_LeafSelection` — root-first / leaf-first / shuffled orderings, 2- and 3-cert chains; asserts the non-CA leaf is selected and the chain length is correct. - `TestUnpackPEM_WithPrivateKey_RootFirst` — root-first bundle with a private key block; asserts both key extraction and correct leaf selection. - `TestUnpackPEM_SingleCert` — single cert returned as leaf, empty chain. Verified **red→green**: the root-first / shuffled cases fail against the pre-fix code (return `Test Root CA`) and pass after the fix. Full `./api/...` suite green. Fixes #52
…tStoreContainers (#56)
…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.
…59) fix(client): plumb Server.ClientTimeout into the rebuilt auth config
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
2 participants
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.
No description provided.