diff --git a/v2/api/template_models.go b/v2/api/template_models.go index 526b8c4..70eefd7 100644 --- a/v2/api/template_models.go +++ b/v2/api/template_models.go @@ -81,7 +81,13 @@ type UpdateTemplateArg struct { AllowedRequesters *[]string `json:"AllowedRequesters,omitempty"` RFCEnforcement *bool `json:"RFCEnforcement,omitempty"` RequiresApproval *bool `json:"RequiresApproval,omitempty"` - KeyUsage *bool `json:"KeyUsage,omitempty"` + // KeyUsage is an int32 bitmask on Command's wire format (e.g. 160 = + // digitalSignature|keyEncipherment), matching GetTemplateResponse.KeyUsage and + // Command's TemplateUpdateRequest/TemplateRetrievalResponse swagger schema + // (both typed "integer"/"int32"). A *bool here previously produced a live + // HTTP 400 ("Unexpected character encountered while parsing value: t. Path + // 'KeyUsage'") since Command rejects a JSON boolean for an integer field. + KeyUsage *int `json:"KeyUsage,omitempty"` } type UpdateTemplateResponse struct{ GetTemplateResponse } diff --git a/v3/api/certificate.go b/v3/api/certificate.go index 700083e..c4edf1f 100644 --- a/v3/api/certificate.go +++ b/v3/api/certificate.go @@ -142,7 +142,21 @@ func (c *Client) EnrollPFXV2(ea *EnrollPFXFctArgsV2) (*EnrollResponseV2, error) Payload: &ea, } - log.Println("[TRACE] Request: ", keyfactorAPIStruct) + // Log a redacted copy of the enrollment args rather than ea/keyfactorAPIStruct + // directly: ea.Password carries the PFX private-key protection password, and + // %v-formatting the struct (as this TRACE log historically did) would dump it + // in plaintext. redactedEA is a value copy (ea is *EnrollPFXFctArgsV2) so + // mutating its Password field below never touches the real request's ea. + redactedEA := *ea + if redactedEA.Password != "" { + redactedEA.Password = redactedLogValue + } + log.Println("[TRACE] Request: ", &request{ + Method: keyfactorAPIStruct.Method, + Endpoint: keyfactorAPIStruct.Endpoint, + Headers: keyfactorAPIStruct.Headers, + Payload: &redactedEA, + }) resp, err := c.sendRequest(keyfactorAPIStruct) if err != nil { @@ -725,7 +739,18 @@ func (c *Client) RecoverCertificate( IncludeChain: true, } - log.Println("[DEBUG] RecoverCertificate: Recovering certificate with args:", rca) + // Log a redacted copy: rca.Password is the private-key recovery password + // supplied by the caller, and this DEBUG-level log (a common + // troubleshooting verbosity, reachable on ordinary Read/Update/import + // private-key-recovery paths) used to dump it in plaintext via %v-style + // struct formatting. redactedRCA is a value copy (rca is + // *recoverCertArgs) so mutating its Password field below never touches + // the real rca used to build the outgoing request below. + redactedRCA := *rca + if redactedRCA.Password != "" { + redactedRCA.Password = redactedLogValue + } + log.Println("[DEBUG] RecoverCertificate: Recovering certificate with args:", &redactedRCA) // Set Keyfactor-specific headers headers := &apiHeaders{ Headers: []StringTuple{ diff --git a/v3/api/client.go b/v3/api/client.go index add6a21..4320056 100644 --- a/v3/api/client.go +++ b/v3/api/client.go @@ -28,6 +28,7 @@ import ( "net/url" "path" "strings" + "sync" "time" "github.com/Keyfactor/keyfactor-auth-client-go/auth_providers" @@ -69,6 +70,47 @@ var ( type Client struct { AuthClient AuthConfig LoggerType string + + // httpClient caches the *http.Client returned by AuthClient.GetHttpClient() + // so that sendRequest reuses a single underlying transport/connection pool + // across requests instead of asking AuthClient to build a brand new one on + // every call. Both CommandConfigOauth.GetHttpClient() and + // CommandAuthConfigBasic.GetHttpClient() (in keyfactor-auth-client-go) + // construct a fresh http.Transport per invocation, and that transport's + // IdleConnTimeout is derived from the configured HttpClientTimeout - so + // without this cache, every request opens its own connection pool whose + // sockets linger for up to HttpClientTimeout before being reclaimed. This + // was already true at the old fixed 60s default; plumbing a caller-supplied + // ClientTimeout (see NewKeyfactorClient) just widens the window, so caching + // here keeps that fix from amplifying a pre-existing resource leak. + httpClient *http.Client + httpClientMu sync.Mutex +} + +// getHttpClient returns the cached *http.Client if one has already been +// resolved for this Client, populating the cache on first use otherwise. +// This guarantees AuthClient.GetHttpClient() is invoked at most once per +// Client instance, so the transport (and its connection pool) is reused +// across requests. It is safe for concurrent use. +// +// Note this does not affect OAuth token refresh: the cached *http.Client's +// transport wraps an oauth2 TokenSource that is consulted (and refreshed as +// needed) on every RoundTrip, independent of how many times the *http.Client +// itself is reused. +func (c *Client) getHttpClient() (*http.Client, error) { + c.httpClientMu.Lock() + defer c.httpClientMu.Unlock() + + if c.httpClient != nil { + return c.httpClient, nil + } + + httpClient, err := c.AuthClient.GetHttpClient() + if err != nil { + return nil, err + } + c.httpClient = httpClient + return httpClient, nil } // TerraformLogger wraps the tflog logging to handle Go's log messages with log level mapping. @@ -142,11 +184,12 @@ func NewKeyfactorClient(cfg *auth_providers.Server, ctx *context.Context) (*Clie clientAuthType := cfg.GetAuthType() baseConfig := auth_providers.CommandAuthConfig{ - CommandHostName: cfg.Host, - CommandPort: cfg.Port, - CommandAPIPath: cfg.APIPath, - CommandCACert: cfg.CACertPath, - SkipVerify: cfg.SkipTLSVerify, + CommandHostName: cfg.Host, + CommandPort: cfg.Port, + CommandAPIPath: cfg.APIPath, + CommandCACert: cfg.CACertPath, + SkipVerify: cfg.SkipTLSVerify, + HttpClientTimeout: cfg.ClientTimeout, } if clientAuthType == "basic" { @@ -160,11 +203,12 @@ func NewKeyfactorClient(cfg *auth_providers.Server, ctx *context.Context) (*Clie if aErr != nil { return nil, aErr } - _, cErr := basicCfg.GetHttpClient() + httpClient, cErr := basicCfg.GetHttpClient() if cErr != nil { return nil, cErr } client.AuthClient = &basicCfg + client.httpClient = httpClient return &client, nil } else if clientAuthType == "oauth" { oauthCfg := auth_providers.CommandConfigOauth{ @@ -180,11 +224,12 @@ func NewKeyfactorClient(cfg *auth_providers.Server, ctx *context.Context) (*Clie if aErr != nil { return nil, aErr } - _, cErr := oauthCfg.GetHttpClient() + httpClient, cErr := oauthCfg.GetHttpClient() if cErr != nil { return nil, cErr } client.AuthClient = &oauthCfg + client.httpClient = httpClient return &client, nil } else { return nil, fmt.Errorf("unsupported auth type or authentication cfg: '%s'", clientAuthType) @@ -204,7 +249,12 @@ func logRequest(req *http.Request) error { // Restore the request body so it can be read later req.Body = io.NopCloser(bytes.NewBuffer(body)) - // Create a struct to hold request data + // Create a struct to hold request data. The body is redacted before + // logging (see redactSensitiveJSONForLogging) since it's always the same + // JSON-marshaled request.Payload passed into sendRequest, which may carry + // a certificate/PFX recovery password or other secret - this must not be + // dumped verbatim into TRACE-level logs. + redactedBody := redactSensitiveJSONForLogging(body) requestData := struct { Method string `json:"method"` URL string `json:"url"` @@ -214,7 +264,7 @@ func logRequest(req *http.Request) error { Method: req.Method, URL: req.URL.String(), Headers: req.Header, - Body: string(body), + Body: string(redactedBody), } // Convert struct to JSON @@ -251,7 +301,10 @@ func requestToCurl(req *http.Request) (string, error) { } } - // Add the body if it exists + // Add the body if it exists. The body is redacted before being embedded + // in the logged cURL command (see redactSensitiveJSONForLogging) since a + // TRACE-level cURL command containing a raw password is directly + // replayable by anyone who reads the log, not just informational. if req.Method == http.MethodPost || req.Method == http.MethodPut { body, err := io.ReadAll(req.Body) if err != nil { @@ -259,7 +312,7 @@ func requestToCurl(req *http.Request) (string, error) { } req.Body = io.NopCloser(bytes.NewBuffer(body)) // Restore the request body - curlCommand.WriteString(fmt.Sprintf("--data %q ", string(body))) + curlCommand.WriteString(fmt.Sprintf("--data %q ", string(redactSensitiveJSONForLogging(body)))) } return curlCommand.String(), nil @@ -317,7 +370,7 @@ func (c *Client) sendRequest(request *request) (*http.Response, error) { if mErr != nil { return nil, mErr } - log.Printf("[TRACE] Request body: %s", jsonByes) + log.Printf("[TRACE] Request body: %s", redactSensitiveJSONForLogging(jsonByes)) req, reqErr := http.NewRequest(request.Method, keyfactorPath, bytes.NewBuffer(jsonByes)) if reqErr != nil { @@ -342,49 +395,30 @@ func (c *Client) sendRequest(request *request) (*http.Response, error) { // Log the request logRequest(req) - httpClient, cErr := c.AuthClient.GetHttpClient() + httpClient, cErr := c.getHttpClient() if cErr != nil { return nil, cErr } resp, respErr := httpClient.Do(req) - // check if context deadline exceeded + // NOTE: this used to silently retry on "context deadline exceeded" (up to + // MAX_CONTEXT_DEADLINE_RETRIES times) without ever surfacing that a retry + // happened. That's unsafe for two reasons: + // 1. Retrying a non-idempotent request (e.g. a POST enrollment) after a + // client-side timeout risks creating a second server-side resource + // if the original request actually succeeded after the client gave + // up on it -- exactly the scenario callers need to detect via the + // returned error, not have hidden from them by a "successful" retry. + // 2. If every retry also failed, `resp` was never reassigned from its + // original nil value and there was no `return` for this case, so + // control fell through to `resp.StatusCode` below on a nil + // *http.Response, panicking the caller (e.g. crashing `terraform + // apply` outright). + // Callers that need retry-with-backoff semantics around a timeout (and + // that know their request is safe to repeat) should implement that at + // their own call site, where they have the context to decide; this layer + // now always returns the transport error untouched. switch { - case respErr != nil && (strings.Contains(respErr.Error(), "context deadline exceeded")): - sleepDuration := time.Duration(1) * time.Second - for i := 0; i < MAX_CONTEXT_DEADLINE_RETRIES; i++ { - // sleep for exponential backoff - if i > 0 { - sleepDuration *= 2 - if sleepDuration > time.Duration(MAX_WAIT_SECONDS)*time.Second { - sleepDuration = time.Duration(MAX_WAIT_SECONDS) * time.Second - } - log.Printf( - "[DEBUG] %s request to %s failed with error %s, retrying in %s seconds...", - request.Method, - keyfactorPath, - respErr.Error(), - sleepDuration, - ) - time.Sleep(sleepDuration) - } - - log.Printf( - "[DEBUG] %s request to %s failed with error %s, retrying...", - request.Method, - keyfactorPath, - respErr.Error(), - ) - req, reqErr = http.NewRequest(request.Method, keyfactorPath, bytes.NewBuffer(jsonByes)) - if reqErr != nil { - return nil, reqErr - } - resp2, respErr2 := httpClient.Do(req) - if respErr2 == nil && resp2 != nil { - resp = resp2 - break - } - } case respErr != nil: log.Printf("[ERROR] Error sending '%s' request to '%s': %s", request.Method, request.Endpoint, respErr) return nil, respErr diff --git a/v3/api/client_test.go b/v3/api/client_test.go new file mode 100644 index 0000000..cf8d4a0 --- /dev/null +++ b/v3/api/client_test.go @@ -0,0 +1,590 @@ +// Copyright 2024 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package api + +import ( + "context" + "crypto/tls" + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "os" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/Keyfactor/keyfactor-auth-client-go/auth_providers" +) + +// newFakeCommandServer stands in for a Keyfactor Command instance for +// CommandAuthConfigBasic.Authenticate(), which performs a real GET against +// {host}/{apiPath}/Status/Endpoints as part of authentication. It always +// returns 200 with a valid JSON string array, regardless of credentials. +func newFakeCommandServer(t *testing.T) *httptest.Server { + t.Helper() + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`["endpoint1"]`)) + })) + t.Cleanup(server.Close) + return server +} + +// isolateKeyfactorEnv unsets ambient KEYFACTOR_* environment variables that +// CommandAuthConfig.ValidateAuthConfig() falls back to whenever the +// corresponding struct field is left at its zero value, restoring their +// original values (present-and-unset, or present-with-value) once the test +// completes. This makes tests that build a Server/CommandAuthConfig with an +// intentionally-zero field (e.g. ClientTimeout: 0 to exercise the "use the +// default" path, or SkipTLSVerify relying on a literal true) hermetic: +// without this, a developer or CI job with KEYFACTOR_CLIENT_TIMEOUT or +// KEYFACTOR_SKIP_VERIFY exported in their shell would get spurious failures +// or, worse, a silently-clobbered SkipVerify that rejects the test's +// self-signed httptest TLS cert. +// +// Note: t.Setenv(key, "") is NOT equivalent to unsetting - os.LookupEnv still +// reports the variable as present with an empty value, which is enough to +// take the "environment variable is set" branch in ValidateAuthConfig (e.g. +// strconv.Atoi("") fails silently and leaves HttpClientTimeout at 0 rather +// than falling through to DefaultClientTimeout). The variable must be +// actually removed from the environment. +func isolateKeyfactorEnv(t *testing.T, keys ...string) { + t.Helper() + for _, key := range keys { + key := key + originalValue, wasSet := os.LookupEnv(key) + if err := os.Unsetenv(key); err != nil { + t.Fatalf("failed to unset %s: %v", key, err) + } + t.Cleanup(func() { + if wasSet { + _ = os.Setenv(key, originalValue) + } else { + _ = os.Unsetenv(key) + } + }) + } +} + +// TestNewKeyfactorClient_PlumbsClientTimeout is a regression test proving that +// a Server.ClientTimeout value survives NewKeyfactorClient's rebuild of the +// CommandAuthConfig. Before this fix, baseConfig never set HttpClientTimeout, +// so the rebuilt auth config (and everything derived from it, including +// BuildTransport's ResponseHeaderTimeout) silently fell back to +// DefaultClientTimeout (60s) regardless of what the caller configured, +// producing "net/http: timeout awaiting response headers" on long-running +// calls such as PFX enrollment. +func TestNewKeyfactorClient_PlumbsClientTimeout(t *testing.T) { + isolateKeyfactorEnv( + t, + auth_providers.EnvKeyfactorClientTimeout, + auth_providers.EnvKeyfactorSkipVerify, + auth_providers.EnvKeyfactorPort, + auth_providers.EnvKeyfactorCACert, + ) + server := newFakeCommandServer(t) + u, uErr := url.Parse(server.URL) + if uErr != nil { + t.Fatalf("failed to parse test server URL: %v", uErr) + } + + cfg := &auth_providers.Server{ + Host: u.Host, + Username: "user", + Password: "pass", + APIPath: "api", + SkipTLSVerify: true, + ClientTimeout: 300, + } + + ctx := context.Background() + client, err := NewKeyfactorClient(cfg, &ctx) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + basicCfg, ok := client.AuthClient.(*auth_providers.CommandAuthConfigBasic) + if !ok { + t.Fatalf("expected AuthClient to be *auth_providers.CommandAuthConfigBasic, got %T", client.AuthClient) + } + + if basicCfg.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300, got %d", basicCfg.HttpClientTimeout) + } + + transport, tErr := basicCfg.CommandAuthConfig.BuildTransport() + if tErr != nil { + t.Fatalf("expected no error building transport, got %v", tErr) + } + + expected := 300 * time.Second + if transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } +} + +// TestNewKeyfactorClient_DefaultClientTimeout confirms the zero-value +// (unset) case still falls back to the library default rather than 0s, +// preserving pre-fix behavior for callers who don't set ClientTimeout. +func TestNewKeyfactorClient_DefaultClientTimeout(t *testing.T) { + isolateKeyfactorEnv( + t, + auth_providers.EnvKeyfactorClientTimeout, + auth_providers.EnvKeyfactorSkipVerify, + auth_providers.EnvKeyfactorPort, + auth_providers.EnvKeyfactorCACert, + ) + server := newFakeCommandServer(t) + u, uErr := url.Parse(server.URL) + if uErr != nil { + t.Fatalf("failed to parse test server URL: %v", uErr) + } + + cfg := &auth_providers.Server{ + Host: u.Host, + Username: "user", + Password: "pass", + APIPath: "api", + SkipTLSVerify: true, + } + + ctx := context.Background() + client, err := NewKeyfactorClient(cfg, &ctx) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + basicCfg, ok := client.AuthClient.(*auth_providers.CommandAuthConfigBasic) + if !ok { + t.Fatalf("expected AuthClient to be *auth_providers.CommandAuthConfigBasic, got %T", client.AuthClient) + } + + if basicCfg.HttpClientTimeout != auth_providers.DefaultClientTimeout { + t.Fatalf("expected HttpClientTimeout to fall back to default %d, got %d", auth_providers.DefaultClientTimeout, basicCfg.HttpClientTimeout) + } +} + +// perCallTransportAuthConfig is a minimal AuthConfig test double that mimics +// the real behavior of keyfactor-auth-client-go's CommandConfigOauth and +// CommandAuthConfigBasic GetHttpClient() implementations: every call builds a +// brand new *http.Transport (and therefore a brand new, empty connection +// pool) rather than reusing one. It exists to prove that Client caches the +// *http.Client it gets back rather than calling GetHttpClient() (and paying +// for a fresh transport/connection pool) on every request. +type perCallTransportAuthConfig struct { + server *httptest.Server + getClientCalls int32 +} + +func (a *perCallTransportAuthConfig) GetServerConfig() *auth_providers.Server { + return &auth_providers.Server{ + Host: a.server.URL, + APIPath: "KeyfactorAPI", + SkipTLSVerify: true, + } +} + +func (a *perCallTransportAuthConfig) GetHttpClient() (*http.Client, error) { + atomic.AddInt32(&a.getClientCalls, 1) + return &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{InsecureSkipVerify: true}, + }, + }, nil +} + +func (a *perCallTransportAuthConfig) Authenticate() error { return nil } + +func (a *perCallTransportAuthConfig) GetCommandVersion() string { return "25.1.0.0" } + +// TestClient_ReusesHttpClientAcrossRequests is a regression test for a +// resource leak: sendRequest used to call c.AuthClient.GetHttpClient() on +// every single request. Since the real AuthConfig implementations build a +// brand new http.Transport (and connection pool) per call, and that +// transport's IdleConnTimeout is derived from the configured +// HttpClientTimeout, every API call opened its own never-reused connection +// whose socket lingered until IdleConnTimeout fired - amplified by the fix +// that plumbs a caller-configured ClientTimeout (which can be arbitrarily +// large, e.g. 1800s) all the way through instead of the fixed 60s default. +// +// This test drives Client.sendRequest directly across multiple requests and +// asserts both that AuthConfig.GetHttpClient() is invoked at most once +// (proving the *http.Client is cached) and that the underlying TCP +// connection is reused rather than growing linearly with the request count. +func TestClient_ReusesHttpClientAcrossRequests(t *testing.T) { + var newConns int32 + srv := httptest.NewUnstartedServer( + http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`[]`)) + }, + ), + ) + srv.Config.ConnState = func(_ net.Conn, state http.ConnState) { + if state == http.StateNew { + atomic.AddInt32(&newConns, 1) + } + } + srv.StartTLS() + t.Cleanup(srv.Close) + + auth := &perCallTransportAuthConfig{server: srv} + client := NewKeyfactorClientWithAuth(auth, nil) + + const requestCount = 10 + for i := 0; i < requestCount; i++ { + resp, err := client.sendRequest( + &request{ + Method: http.MethodGet, + Endpoint: "CertificateStoreContainers", + Headers: &apiHeaders{}, + }, + ) + if err != nil { + t.Fatalf("request %d failed: %v", i, err) + } + // Fully drain and close the body so the underlying transport is free + // to return the connection to its idle pool for reuse. + _, _ = io.Copy(io.Discard, resp.Body) + _ = resp.Body.Close() + } + + if calls := atomic.LoadInt32(&auth.getClientCalls); calls != 1 { + t.Fatalf( + "expected AuthClient.GetHttpClient to be called exactly once across %d requests (client should be cached), got %d calls", + requestCount, + calls, + ) + } + + if conns := atomic.LoadInt32(&newConns); conns > 2 { + t.Fatalf( + "expected the TCP connection to be reused across %d sequential requests (at most ~1-2 new connections), observed %d new connections", + requestCount, + conns, + ) + } +} + +// TestClient_ConcurrentRequestsNotCappedByMaxConnsPerHost is an end-to-end +// regression test closing the loop on a finding this package's own +// http.Client-caching fix (see TestClient_ReusesHttpClientAcrossRequests) +// caused but could not fix locally: caching a single *http.Client means the +// transport keyfactor-auth-client-go builds is now reused for the lifetime +// of the Client instance, so any nonzero MaxConnsPerHost on that transport +// stops being a harmless per-request default and becomes a permanent, +// unqueued-timeout ceiling on concurrent in-flight requests for the whole +// process - e.g. `terraform apply -parallelism=25` would silently serialize +// into batches of N with no bound on how long excess requests wait, since +// neither the cached client's Timeout (0, unset) nor its requests' contexts +// impose one. +// +// keyfactor-auth-client-go previously hardcoded MaxConnsPerHost: 10 on this +// transport. Its own fix (auth_core.go's newHTTPTransport, now pinned at 0 / +// unbounded to match net/http.DefaultTransport) was verified from that +// repo's side by inspecting the constructed *http.Transport's field value. +// This test verifies the fix end-to-end from this repo's perspective instead +// of trusting that inspection alone: it builds a real Client via +// NewKeyfactorClient (exactly as production code does), retrieves its cached +// *http.Client via getHttpClient(), and drives 25 concurrent requests through +// it against a real httptest server, asserting the server actually observes +// well more than 10 requests in flight at once rather than serializing into +// batches of 10. +func TestClient_ConcurrentRequestsNotCappedByMaxConnsPerHost(t *testing.T) { + const ( + concurrentRequests = 25 + holdDuration = 200 * time.Millisecond + ) + + var ( + mu sync.Mutex + inFlight int + maxInFlight int + ) + + srv := httptest.NewTLSServer( + http.HandlerFunc( + func(w http.ResponseWriter, r *http.Request) { + // The initial CommandAuthConfigBasic.Authenticate() call made + // by NewKeyfactorClient below hits this same handler; letting + // it fall through the same slow path is harmless since it + // happens once, sequentially, before the concurrent phase + // starts timing anything. + mu.Lock() + inFlight++ + if inFlight > maxInFlight { + maxInFlight = inFlight + } + mu.Unlock() + + time.Sleep(holdDuration) + + mu.Lock() + inFlight-- + mu.Unlock() + + w.Header().Set("Content-Type", "application/json") + w.Header().Set("x-keyfactor-product-version", "99.9.9") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`["endpoint1"]`)) + }, + ), + ) + t.Cleanup(srv.Close) + + u, uErr := url.Parse(srv.URL) + if uErr != nil { + t.Fatalf("failed to parse test server URL: %v", uErr) + } + + cfg := &auth_providers.Server{ + Host: u.Host, + Username: "user", + Password: "pass", + APIPath: "api", + SkipTLSVerify: true, + } + + ctx := context.Background() + client, err := NewKeyfactorClient(cfg, &ctx) + if err != nil { + t.Fatalf("NewKeyfactorClient failed: %v", err) + } + + httpClient, hErr := client.getHttpClient() + if hErr != nil { + t.Fatalf("getHttpClient failed: %v", hErr) + } + + // Reset the counters: the single sequential Authenticate() call above + // already touched inFlight/maxInFlight and this resets the baseline so + // the assertion below reflects only the concurrent phase. + mu.Lock() + inFlight = 0 + maxInFlight = 0 + mu.Unlock() + + start := time.Now() + var wg sync.WaitGroup + for i := 0; i < concurrentRequests; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req, rErr := http.NewRequest(http.MethodGet, srv.URL+"/KeyfactorAPI/concurrent-probe", nil) + if rErr != nil { + t.Errorf("failed to build request: %v", rErr) + return + } + resp, dErr := httpClient.Do(req) + if dErr != nil { + t.Errorf("request failed: %v", dErr) + return + } + _ = resp.Body.Close() + }() + } + wg.Wait() + elapsed := time.Since(start) + + mu.Lock() + observedMax := maxInFlight + mu.Unlock() + + t.Logf( + "observed max in-flight requests: %d/%d, wall time: %s (old MaxConnsPerHost=10 cap measured ~10 in-flight/909ms for a comparable batch; unbounded measured ~315ms)", + observedMax, + concurrentRequests, + elapsed, + ) + + // The old hardcoded MaxConnsPerHost: 10 would cap this at exactly 10 + // no matter how many requests are fired concurrently. Assert well above + // that ceiling (comfortably below concurrentRequests to tolerate + // scheduler jitter) to prove the requests are not being serialized into + // batches of 10. + const minAcceptableMaxInFlight = 15 + if observedMax <= 10 { + t.Fatalf( + "expected max concurrent in-flight requests to exceed the old MaxConnsPerHost=10 ceiling, got %d (elapsed %s) - concurrency ceiling regression", + observedMax, + elapsed, + ) + } + if observedMax < minAcceptableMaxInFlight { + t.Fatalf( + "expected max concurrent in-flight requests to be close to %d (unbounded), got only %d (elapsed %s)", + concurrentRequests, + observedMax, + elapsed, + ) + } + + // Wall time is a secondary signal: fully serialized into batches of 10 + // would take ceil(25/10)*holdDuration ~= 3*200ms = 600ms; unbounded + // concurrency should complete in roughly one holdDuration plus overhead. + maxAcceptableElapsed := holdDuration * 2 + if elapsed > maxAcceptableElapsed { + t.Fatalf( + "expected wall time close to a single %s hold duration for unbounded concurrency, got %s (elapsed too long, suggests serialization)", + holdDuration, + elapsed, + ) + } +} + +// TestSendRequest_ContextDeadlineExceeded_NoSilentRetry reproduces the first half +// of a confirmed HIGH-severity bug in sendRequest: on a client-side timeout +// ("context deadline exceeded"), the function used to transparently retry the +// request (up to MAX_CONTEXT_DEADLINE_RETRIES times) and, if a retry +// succeeded, return that success with no indication a timeout ever happened. +// +// That silently swallowed the fact that the *original* request may have +// already succeeded server-side (e.g. a certificate enrollment), and +// defeated callers -- like terraform-provider-keyfactor's orphaned-PFX +// recovery logic -- that specifically match on "context deadline exceeded" +// to trigger their own safe recovery search instead of blindly re-issuing a +// non-idempotent request. +// +// This test sets up a server that only delays its FIRST response beyond the +// client timeout and answers instantly after that -- i.e. exactly the shape +// that used to be masked into a silent "success after retry". It asserts +// sendRequest now returns the timeout error immediately, without contacting +// the server a second time. +func TestSendRequest_ContextDeadlineExceeded_NoSilentRetry(t *testing.T) { + var requestCount int32 + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + n := atomic.AddInt32(&requestCount, 1) + if n == 1 { + // First request: sleep well past the client timeout below, so the + // client gives up on it -- but note the server continues to + // process it and will "complete" it right after. A retry that + // followed would succeed immediately, which is exactly the + // scenario that used to be silently swallowed. + time.Sleep(300 * time.Millisecond) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c := &Client{ + AuthClient: &mockAuthConfig{ + serverConfig: newTestServerConfig(srv), + httpClient: testHTTPClientWithTimeout(srv, 50*time.Millisecond), + }, + } + + resp, err := c.sendRequest(&request{Method: http.MethodGet, Endpoint: "test", Headers: &apiHeaders{}}) + + if err == nil { + t.Fatalf("sendRequest() returned nil error, want a context-deadline-exceeded error (retry must not silently succeed)") + } + if !strings.Contains(err.Error(), "context deadline exceeded") { + t.Fatalf("sendRequest() error = %q, want it to contain %q", err.Error(), "context deadline exceeded") + } + if resp != nil { + t.Fatalf("sendRequest() returned a non-nil response alongside an error: %+v", resp) + } + if got := atomic.LoadInt32(&requestCount); got != 1 { + t.Fatalf( + "server was hit %d time(s), want exactly 1 -- sendRequest must not silently retry a timed-out request", + got, + ) + } +} + +// TestSendRequest_ContextDeadlineExceeded_NoPanic reproduces the second half +// of the bug: when every attempt fails with a "context deadline exceeded" +// shaped error, sendRequest's response variable was never reassigned from +// its original nil value, and the surrounding switch had no `return` for +// this case -- so control fell through to `resp.StatusCode` on a nil +// *http.Response, panicking the caller (e.g. crashing `terraform apply` +// outright) instead of returning a normal, handleable error. +// +// This test uses a server that always delays past the client timeout, so +// every attempt sendRequest makes hits the same failure shape. It asserts a +// clean error is returned -- never a panic. +func TestSendRequest_ContextDeadlineExceeded_NoPanic(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(300 * time.Millisecond) + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + c := &Client{ + AuthClient: &mockAuthConfig{ + serverConfig: newTestServerConfig(srv), + httpClient: testHTTPClientWithTimeout(srv, 50*time.Millisecond), + }, + } + + var ( + resp *http.Response + err error + ) + + func() { + defer func() { + if r := recover(); r != nil { + t.Fatalf("sendRequest() panicked: %v", r) + } + }() + resp, err = c.sendRequest(&request{Method: http.MethodGet, Endpoint: "test", Headers: &apiHeaders{}}) + }() + + if err == nil { + t.Fatalf("sendRequest() returned nil error, want a context-deadline-exceeded error") + } + if !strings.Contains(err.Error(), "context deadline exceeded") { + t.Fatalf("sendRequest() error = %q, want it to contain %q", err.Error(), "context deadline exceeded") + } + if resp != nil { + t.Fatalf("sendRequest() returned a non-nil response alongside an error: %+v", resp) + } +} + +// newTestServerConfig builds a minimal *auth_providers.Server pointing at the +// given httptest server, matching the pattern used by newTestClient in +// pam_types_test.go. +func newTestServerConfig(server *httptest.Server) *auth_providers.Server { + return &auth_providers.Server{ + Host: server.URL, + APIPath: "/KeyfactorAPI", + SkipTLSVerify: true, + } +} + +// testHTTPClientWithTimeout returns an *http.Client that trusts the given +// httptest TLS server's certificate (sendRequest always forces the https +// scheme, so a plain httptest.NewServer can't be used directly) but with a +// short overall Timeout so requests to a deliberately slow handler produce a +// real "context deadline exceeded" error, identical in shape to what a +// production client sees against a genuinely slow/unreachable Command +// server. +func testHTTPClientWithTimeout(server *httptest.Server, timeout time.Duration) *http.Client { + base := server.Client() + return &http.Client{ + Transport: base.Transport, + Timeout: timeout, + } +} diff --git a/v3/api/log_redaction.go b/v3/api/log_redaction.go new file mode 100644 index 0000000..315f615 --- /dev/null +++ b/v3/api/log_redaction.go @@ -0,0 +1,148 @@ +// Copyright 2026 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package api + +import ( + "encoding/json" + "regexp" +) + +// sensitiveLogFieldPattern matches JSON object keys that are known or likely +// to carry secret material: certificate/PFX passwords, PAM secret values, +// private key material, API tokens, etc. It is intentionally broad (a +// case-insensitive substring match, not an exact-name allowlist) because this +// package's request payloads are logged generically at TRACE/DEBUG level +// without per-call-site awareness of which fields are sensitive - erring +// toward redacting a borderline non-sensitive field (e.g. "PasswordOptions", +// a policy-description object, not a secret) in a debug log is a far cheaper +// mistake than missing a real secret. +var sensitiveLogFieldPattern = regexp.MustCompile(`(?i)(password|passphrase|secret|privatekey|private_key|pfx|apikey|api_key|accesstoken|access_token|clientsecret|client_secret|token)`) + +const redactedLogValue = "[REDACTED]" + +// maxNestedJSONStringDepth bounds how many levels of "JSON encoded as a +// string value" redactSensitiveValue will attempt to unmarshal and recurse +// into. Several request structs (e.g. CreateStoreFctArgs/UpdateStoreFctArgs's +// PropertiesString field) are populated by pre-serializing a +// map[string]interface{} to a JSON string before the outer struct itself is +// marshaled by sendRequest, producing a JSON string leaf whose *contents* are +// themselves JSON containing secrets (e.g. ServerUsername/ServerPassword). +// Without unwrapping these string-encoded-JSON leaves, redaction never sees +// the nested keys and secrets sail through unredacted into TRACE logs. The +// depth guard exists purely so adversarial or accidentally-deep +// string-of-JSON-of-string-of-JSON... nesting can't recurse unboundedly; a +// legitimate payload should never come close to this limit. +const maxNestedJSONStringDepth = 5 + +// redactSensitiveJSONForLogging returns a copy of jsonBytes (expected to be +// the JSON encoding of an API request/response payload) with the values of +// any object key matching sensitiveLogFieldPattern replaced by +// redactedLogValue, so that logging the payload for troubleshooting doesn't +// also leak certificate/PFX recovery passwords or other secrets into the +// application log (which historically was set to Go's global *log* package, +// bypassing any per-call caller-side masking applied to this library's own +// tflog calls). +// +// This function is used purely to sanitize what gets written to a log line; +// it never mutates or replaces the original bytes used to build the actual +// outgoing HTTP request body. +// +// If jsonBytes doesn't decode as JSON (e.g. it's the literal "null", or +// malformed), it is returned unchanged: there is no structured content to +// redact, and logging a bare scalar isn't a plausible secret-leak vector on +// its own. +func redactSensitiveJSONForLogging(jsonBytes []byte) []byte { + var decoded interface{} + if err := json.Unmarshal(jsonBytes, &decoded); err != nil { + return jsonBytes + } + + redacted, err := json.Marshal(redactSensitiveValue(decoded, maxNestedJSONStringDepth)) + if err != nil { + return jsonBytes + } + return redacted +} + +// redactSensitiveValue recursively walks a decoded JSON value (as produced by +// encoding/json's default interface{} unmarshaling: map[string]interface{}, +// []interface{}, a string, or another scalar), replacing the value of any +// map key that matches sensitiveLogFieldPattern with redactedLogValue. +// +// String leaves get one extra check: some request structs pre-serialize a +// map to a JSON string before the outer struct is marshaled again (e.g. +// CreateStoreFctArgs/UpdateStoreFctArgs's PropertiesString field), so a +// string leaf's own contents may themselves be JSON carrying secrets +// (ServerUsername/ServerPassword, etc.) that would otherwise sail through +// unredacted. If a string leaf successfully unmarshals as a +// map[string]interface{} or []interface{}, it is redacted the same way and +// re-marshaled back to a string, preserving its "valid JSON encoded as a +// string" shape in the log output. If it doesn't decode to one of those two +// container types (including "it isn't valid JSON at all"), it is left +// alone: that's the common case of an ordinary string value, not a nested +// payload to unwrap. +// +// remainingDepth bounds how many further levels of string-encoded-JSON will +// be unwrapped, so adversarial or accidentally deep nesting +// (JSON-of-string-of-JSON-of-string-of-...) can't recurse unboundedly. Once +// it reaches zero, string leaves are left as-is without attempting to parse +// them further; map/slice recursion is unaffected by this guard since it can +// only nest as deeply as the decoded value's own structure allows. +func redactSensitiveValue(v interface{}, remainingDepth int) interface{} { + switch t := v.(type) { + case map[string]interface{}: + out := make(map[string]interface{}, len(t)) + for k, val := range t { + if sensitiveLogFieldPattern.MatchString(k) { + out[k] = redactedLogValue + } else { + out[k] = redactSensitiveValue(val, remainingDepth) + } + } + return out + case []interface{}: + out := make([]interface{}, len(t)) + for i, val := range t { + out[i] = redactSensitiveValue(val, remainingDepth) + } + return out + case string: + if remainingDepth <= 0 { + return t + } + var nested interface{} + if err := json.Unmarshal([]byte(t), &nested); err != nil { + return t + } + switch nested.(type) { + case map[string]interface{}, []interface{}: + redactedNested := redactSensitiveValue(nested, remainingDepth-1) + reMarshaled, err := json.Marshal(redactedNested) + if err != nil { + return t + } + return string(reMarshaled) + default: + // Decoded to a bare scalar (a JSON string/number/bool/null that + // happens to be valid JSON on its own, e.g. the string "42" or + // `"true"`) rather than a container - nothing to redact inside + // it, and re-marshaling would just be a no-op wrapped in + // pointless work. Leave the original string leaf unchanged. + return t + } + default: + return v + } +} diff --git a/v3/api/log_redaction_test.go b/v3/api/log_redaction_test.go new file mode 100644 index 0000000..de89e69 --- /dev/null +++ b/v3/api/log_redaction_test.go @@ -0,0 +1,262 @@ +// Copyright 2026 Keyfactor +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package api + +import ( + "bytes" + "encoding/json" + "log" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// canaryRecoveryPassword is a value distinctive enough that its presence in +// captured log output can only be explained by the password itself leaking, +// not by an unrelated log line coincidentally matching. +const canaryRecoveryPassword = "Sup3rS3cr3t-Canary-Password-DoNotLeak" + +// captureGlobalLogOutput redirects Go's global *log* package output (the +// same package this library's client.go/certificate.go call sites use, and +// the same package initLogger redirects to a TerraformLogger/tflog in +// production) to an in-memory buffer for the duration of the test, restoring +// whatever writer was previously configured afterward. This lets tests +// assert on the exact text that would have been logged without needing a +// live tflog/terraform-plugin-log sink. +func captureGlobalLogOutput(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + original := log.Writer() + log.SetOutput(&buf) + t.Cleanup(func() { + log.SetOutput(original) + }) + return &buf +} + +// TestRecoverCertificate_DoesNotLogPlaintextPassword is a regression test for +// a confirmed leak: RecoverCertificate's args (including the private-key +// recovery Password) used to be dumped verbatim via +// `log.Println("[DEBUG] RecoverCertificate: Recovering certificate with +// args:", rca)` -- reachable on ordinary Read/Update/import private-key +// recovery paths at DEBUG level, a routine troubleshooting verbosity, not +// something requiring an unusually verbose log level to trigger. +// +// This drives the real RecoverCertificate function (not just the log +// statement in isolation) against a minimal mock server, so it reproduces +// exactly what a caller doing certificate recovery observes in their logs. +// The server intentionally returns a non-2xx status so RecoverCertificate +// exits with an error shortly after the log statement under test, without +// requiring a valid PFX/PKCS12 response body to be fabricated. +func TestRecoverCertificate_DoesNotLogPlaintextPassword(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Message":"synthetic failure for test"}`)) + })) + defer srv.Close() + + c := newTestClient(srv) + + buf := captureGlobalLogOutput(t) + + // Return values are intentionally ignored: the server above always + // errors, so RecoverCertificate is expected to return a non-nil error. + // What matters for this test is only what got written to the log before + // that error was returned. + _, _, _, _, _ = c.RecoverCertificate(123, "", "", "", canaryRecoveryPassword, 0, "PFX") + + logged := buf.String() + if strings.Contains(logged, canaryRecoveryPassword) { + t.Fatalf( + "RecoverCertificate logged the plaintext recovery password via Go's global log package; captured log output:\n%s", + logged, + ) + } +} + +// TestCreateStore_DoesNotLogPlaintextServerPasswordInNestedProperties is a +// regression test for a confirmed leak that is distinct from the +// top-level-field leaks covered above: CreateStoreFctArgs (and +// UpdateStoreFctArgs) carry certificate store connection properties as +// PropertiesString, a JSON string PRE-SERIALIZED from the Properties map by +// CreateStore/UpdateStore themselves (see store.go) before the outer struct +// is marshaled again by sendRequest. That produces a JSON *string* value at +// the top level of the outer request object whose own contents are +// themselves JSON containing secrets such as ServerUsername/ServerPassword - +// exactly the shape real callers hit today (confirmed usage: +// terraform-provider-keyfactor's certificate store resource populates +// ServerPassword/ServerUsername in this Properties map on every create and +// update call). redactSensitiveValue's original string case treated any +// string leaf as opaque, so this nested JSON-as-a-string secret sailed +// through unredacted into the [TRACE] request-body log even though the +// top-level "Password" field redacted fine. +// +// This drives the real CreateStore function (not just the redaction helper +// in isolation) against a minimal mock server, so it reproduces exactly what +// a caller creating a certificate store observes in their logs. +func TestCreateStore_DoesNotLogPlaintextServerPasswordInNestedProperties(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Message":"synthetic failure for test"}`)) + })) + defer srv.Close() + + c := newTestClient(srv) + + buf := captureGlobalLogOutput(t) + + // Return value is intentionally ignored: the server above always errors, + // so CreateStore is expected to return a non-nil error. What matters for + // this test is only what got written to the log before that error was + // returned. + _, _ = c.CreateStore(&CreateStoreFctArgs{ + ClientMachine: "test-client-machine", + StorePath: "/opt/test/store", + AgentId: "11111111-1111-1111-1111-111111111111", + Properties: map[string]interface{}{ + "ServerUsername": "admin", + "ServerPassword": canaryRecoveryPassword, + }, + }) + + logged := buf.String() + if strings.Contains(logged, canaryRecoveryPassword) { + t.Fatalf( + "CreateStore logged the plaintext ServerPassword nested inside the pre-serialized Properties JSON string; captured log output:\n%s", + logged, + ) + } +} + +// TestRedactSensitiveValue_UnwrapsNestedJSONEncodedAsString is a narrower +// unit test against redactSensitiveValue directly (rather than a full +// end-to-end CreateStore drive), pinning down the exact contract: a string +// leaf whose own contents decode to a JSON object or array gets redacted the +// same way a "real" nested object would, and the result is re-marshaled back +// to a string so the log output keeps the "valid JSON encoded as a string" +// shape instead of silently becoming a nested object. +func TestRedactSensitiveValue_UnwrapsNestedJSONEncodedAsString(t *testing.T) { + outer := map[string]interface{}{ + "ClientMachine": "test-client-machine", + "Properties": `{"ServerUsername":"admin","ServerPassword":"` + canaryRecoveryPassword + `"}`, + } + + redacted := redactSensitiveValue(outer, maxNestedJSONStringDepth) + + redactedBytes, err := json.Marshal(redacted) + if err != nil { + t.Fatalf("failed to marshal redacted value: %v", err) + } + if strings.Contains(string(redactedBytes), canaryRecoveryPassword) { + t.Fatalf("redactSensitiveValue did not redact a secret nested inside a JSON-encoded-as-string leaf; got: %s", redactedBytes) + } + + // The Properties value should still be a JSON string (not have been + // promoted to a nested object), and it should still be valid JSON once + // unwrapped, with ServerUsername left intact and only ServerPassword + // redacted. + redactedMap, ok := redacted.(map[string]interface{}) + if !ok { + t.Fatalf("expected top-level redacted value to remain a map, got %T", redacted) + } + propertiesVal, ok := redactedMap["Properties"].(string) + if !ok { + t.Fatalf("expected Properties to remain a JSON-encoded string, got %T: %v", redactedMap["Properties"], redactedMap["Properties"]) + } + var reparsed map[string]interface{} + if err := json.Unmarshal([]byte(propertiesVal), &reparsed); err != nil { + t.Fatalf("Properties string is no longer valid JSON after redaction: %v", err) + } + if reparsed["ServerUsername"] != "admin" { + t.Fatalf("expected non-sensitive ServerUsername to survive redaction unchanged, got: %v", reparsed["ServerUsername"]) + } + if reparsed["ServerPassword"] != redactedLogValue { + t.Fatalf("expected ServerPassword to be redacted to %q, got: %v", redactedLogValue, reparsed["ServerPassword"]) + } +} + +// TestRedactSensitiveValue_DepthGuardStopsOnDeeplyNestedJSONStrings ensures +// the recursion depth guard actually bounds how many levels of +// string-encoded-JSON get unwrapped, so adversarial or accidental +// JSON-of-string-of-JSON-of-string-of-... nesting can't cause unbounded +// recursion. It doesn't assert a specific leak/no-leak outcome beyond the +// guard depth (that's an explicit, documented tradeoff) - only that +// redaction terminates and produces valid JSON. +func TestRedactSensitiveValue_DepthGuardStopsOnDeeplyNestedJSONStrings(t *testing.T) { + // Build a value nested well beyond maxNestedJSONStringDepth: each layer + // is a JSON object whose single field's value is itself a JSON-encoded + // string of the next layer down, innermost carrying the canary secret. + value := canaryRecoveryPassword + for i := 0; i < maxNestedJSONStringDepth+5; i++ { + layer, err := json.Marshal(map[string]interface{}{"Secret": value}) + if err != nil { + t.Fatalf("failed to build nested fixture at layer %d: %v", i, err) + } + value = string(layer) + } + + var decoded interface{} + if err := json.Unmarshal([]byte(value), &decoded); err != nil { + t.Fatalf("failed to decode fixture: %v", err) + } + + redacted := redactSensitiveValue(decoded, maxNestedJSONStringDepth) + + // This must not hang or panic (the real assertion here is that the test + // completes at all); as a secondary check, confirm the result still + // marshals to valid JSON. + if _, err := json.Marshal(redacted); err != nil { + t.Fatalf("redacted deeply-nested value failed to re-marshal: %v", err) + } +} + +// TestEnrollPFXV2_DoesNotLogPlaintextPassword is a regression test for a +// confirmed leak: EnrollPFXV2's request struct (whose Payload carries +// ea.Password, the PFX private-key protection password) used to be dumped +// verbatim via `log.Println("[TRACE] Request: ", keyfactorAPIStruct)`. +// +// This drives the real EnrollPFXV2 function against a minimal mock server +// that always errors, so the function returns shortly after the log +// statement under test without requiring a valid enrollment response body. +func TestEnrollPFXV2_DoesNotLogPlaintextPassword(t *testing.T) { + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"Message":"synthetic failure for test"}`)) + })) + defer srv.Close() + + c := newTestClient(srv) + + buf := captureGlobalLogOutput(t) + + _, _ = c.EnrollPFXV2(&EnrollPFXFctArgsV2{ + Template: "test-template", + CertFormat: "PFX", + SubjectString: "CN=test.example.com", + Password: canaryRecoveryPassword, + }) + + logged := buf.String() + if strings.Contains(logged, canaryRecoveryPassword) { + t.Fatalf( + "EnrollPFXV2 logged the plaintext PFX password via Go's global log package; captured log output:\n%s", + logged, + ) + } +} diff --git a/v3/api/template_models.go b/v3/api/template_models.go index 127683b..39d905b 100644 --- a/v3/api/template_models.go +++ b/v3/api/template_models.go @@ -119,7 +119,13 @@ type UpdateTemplateArg struct { AllowedRequesters *[]string `json:"AllowedRequesters,omitempty"` RFCEnforcement *bool `json:"RFCEnforcement,omitempty"` RequiresApproval *bool `json:"RequiresApproval,omitempty"` - KeyUsage *bool `json:"KeyUsage,omitempty"` + // KeyUsage is an int32 bitmask on Command's wire format (e.g. 160 = + // digitalSignature|keyEncipherment), matching GetTemplateResponse.KeyUsage and + // Command's TemplateUpdateRequest/TemplateRetrievalResponse swagger schema + // (both typed "integer"/"int32"). A *bool here previously produced a live + // HTTP 400 ("Unexpected character encountered while parsing value: t. Path + // 'KeyUsage'") since Command rejects a JSON boolean for an integer field. + KeyUsage *int `json:"KeyUsage,omitempty"` // TemplatePolicy must be round-tripped from the corresponding GetTemplateResponse // on every update; see the field comment on GetTemplateResponse.TemplatePolicy. TemplatePolicy *TemplatePolicy `json:"TemplatePolicy,omitempty"` diff --git a/v3/api/template_test.go b/v3/api/template_test.go index c18c629..ee27197 100644 --- a/v3/api/template_test.go +++ b/v3/api/template_test.go @@ -244,3 +244,62 @@ func TestUpdateTemplateArg_TemplatePolicy_Roundtrip(t *testing.T) { t.Fatalf("TemplatePolicy.PrimaryKeyAlgorithms on the wire = %v, want 2 entries", policy["PrimaryKeyAlgorithms"]) } } + +// TestUpdateTemplateArg_KeyUsage_SerializesAsInt verifies that UpdateTemplateArg.KeyUsage +// serializes onto the wire as a JSON number, matching Command's TemplateUpdateRequest +// swagger schema ({"type":"integer","format":"int32"}) confirmed against a live v25.5 +// instance. Before the fix, KeyUsage was typed *bool, which serialized as a JSON boolean +// and produced a live HTTP 400 from Command ("Unexpected character encountered while +// parsing value: t. Path 'KeyUsage'"). This also verifies the value returned by +// GetTemplateResponse.KeyUsage (an int) can be assigned directly to +// UpdateTemplateArg.KeyUsage without a type conversion, since both now agree on int. +func TestUpdateTemplateArg_KeyUsage_SerializesAsInt(t *testing.T) { + var receivedBody []byte + + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var err error + receivedBody, err = io.ReadAll(r.Body) + if err != nil { + t.Fatalf("failed to read request body: %v", err) + } + w.Header().Set("Content-Type", "application/json") + w.Write(receivedBody) + })) + defer srv.Close() + + c := newTestClient(srv) + + // Simulate a real read-modify-write: KeyUsage comes straight off a + // GetTemplateResponse (int) with no bool<->int conversion required. + fetched := GetTemplateResponse{Id: 4, KeyUsage: 160} // digitalSignature|keyEncipherment + keyUsage := fetched.KeyUsage + + arg := &UpdateTemplateArg{ + Id: 4, + KeyUsage: &keyUsage, + } + + if _, err := c.UpdateTemplate(arg); err != nil { + t.Fatalf("UpdateTemplate() error: %v", err) + } + + var onWire map[string]interface{} + if err := json.Unmarshal(receivedBody, &onWire); err != nil { + t.Fatalf("failed to decode request body sent to server: %v", err) + } + + rawKeyUsage, ok := onWire["KeyUsage"] + if !ok { + t.Fatalf("request body sent to server has no KeyUsage field; got keys: %v", onWire) + } + switch v := rawKeyUsage.(type) { + case float64: + if v != 160 { + t.Errorf("KeyUsage on the wire = %v, want 160", v) + } + case bool: + t.Fatalf("KeyUsage on the wire is a JSON boolean (%v); Command's API expects an int32 bitmask and returns HTTP 400 for a boolean payload", v) + default: + t.Fatalf("KeyUsage on the wire has unexpected type %T (value %v), want a JSON number", v, v) + } +} diff --git a/v3/go.mod b/v3/go.mod index f4eb047..705858a 100644 --- a/v3/go.mod +++ b/v3/go.mod @@ -19,7 +19,7 @@ go 1.24.0 toolchain go1.24.5 require ( - github.com/Keyfactor/keyfactor-auth-client-go v1.5.0 + github.com/Keyfactor/keyfactor-auth-client-go v1.6.0-rc.5 github.com/hashicorp/terraform-plugin-log v0.10.0 github.com/spbsoluble/go-pkcs12 v0.4.0 github.com/youmark/pkcs8 v0.0.0-20240726163527-a2c0da244d78 diff --git a/v3/go.sum b/v3/go.sum index 451aad8..df302cb 100644 --- a/v3/go.sum +++ b/v3/go.sum @@ -14,8 +14,8 @@ github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1 h1:WJ github.com/AzureAD/microsoft-authentication-extensions-for-go/cache v0.1.1/go.mod h1:tCcJZ0uHAmvjsVYzEFivsRTN00oz5BEsRgQHu5JZ9WE= github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0 h1:XRzhVemXdgvJqCH0sFfrBUTnUJSBrBf7++ypk+twtRs= github.com/AzureAD/microsoft-authentication-library-for-go v1.6.0/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk= -github.com/Keyfactor/keyfactor-auth-client-go v1.5.0 h1:sq7SGkJeTtDspFSuX2oJxTmFiiFfaQ68B4JP7jryl94= -github.com/Keyfactor/keyfactor-auth-client-go v1.5.0/go.mod h1:rFBZPMSHWwWuUwE1kXhLsDaOxjGiHMbXTTEni8Dmufo= +github.com/Keyfactor/keyfactor-auth-client-go v1.6.0-rc.5 h1:nsp5hrG7EtGFOAaAIyRHt7FSLSWJSIDz66GrLaJU4yA= +github.com/Keyfactor/keyfactor-auth-client-go v1.6.0-rc.5/go.mod h1:rFBZPMSHWwWuUwE1kXhLsDaOxjGiHMbXTTEni8Dmufo= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= @@ -75,8 +75,6 @@ github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZb github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= github.com/rogpeppe/go-internal v1.12.0 h1:exVL4IDcn6na9z1rAb56Vxr+CgyK3nn3O+epU5NdKM8= github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99yedzYV+kq4uf4= -github.com/spbsoluble/go-pkcs12 v0.3.3 h1:3nh7IKn16RDpmrSMtOu1JvbB0XHYq1j+IsICdU1c7J4= -github.com/spbsoluble/go-pkcs12 v0.3.3/go.mod h1:MAxKIUEIl/QVcua/I1L4Otyxl9UvLCCIktce2Tjz6Nw= github.com/spbsoluble/go-pkcs12 v0.4.0 h1:3HOVPZ8pvYqhAyz/NJzT9YODQJ3HbZvB9/CMVmvGaUM= github.com/spbsoluble/go-pkcs12 v0.4.0/go.mod h1:MAxKIUEIl/QVcua/I1L4Otyxl9UvLCCIktce2Tjz6Nw= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=