diff --git a/auth_providers/auth_basic.go b/auth_providers/auth_basic.go index e36d427..a923a61 100644 --- a/auth_providers/auth_basic.go +++ b/auth_providers/auth_basic.go @@ -241,22 +241,18 @@ func (a *CommandAuthConfigBasic) parseUsernameDomain() error { // GetServerConfig returns the server configuration func (a *CommandAuthConfigBasic) GetServerConfig() *Server { - server := Server{ - Host: a.CommandHostName, - Port: a.CommandPort, - Username: a.Username, - Password: a.Password, - Domain: a.Domain, - ClientID: "", - ClientSecret: "", - OAuthTokenUrl: "", - APIPath: a.CommandAPIPath, - //AuthProvider: AuthProvider{}, - SkipTLSVerify: a.SkipVerify, - CACertPath: a.CommandCACert, - AuthType: "basic", - } - return &server + // Delegate to the embedded CommandAuthConfig for the fields it already + // knows how to populate correctly -- notably ClientTimeout, which must be + // omitted (not the ValidateAuthConfig-synthesized default) unless the + // caller explicitly configured it. See clientTimeoutDefaulted's doc + // comment on CommandAuthConfig for why persisting a synthesized default + // is harmful. Layer basic-auth-specific fields on top. + server := a.CommandAuthConfig.GetServerConfig() + server.Username = a.Username + server.Password = a.Password + server.Domain = a.Domain + server.AuthType = "basic" + return server } // Example usage of CommandAuthConfigBasic diff --git a/auth_providers/auth_basic_test.go b/auth_providers/auth_basic_test.go index 78f322f..61bd1d0 100644 --- a/auth_providers/auth_basic_test.go +++ b/auth_providers/auth_basic_test.go @@ -15,6 +15,7 @@ package auth_providers_test import ( + "encoding/json" "fmt" "net/http" "os" @@ -272,6 +273,129 @@ func unsetBasicEnvVariables() { os.Unsetenv(auth_providers.EnvKeyfactorDomain) } +// TestCommandAuthConfigBasic_GetServerConfig_DoesNotPersistSynthesizedDefault +// is the CommandAuthConfigBasic analogue of +// TestCommandAuthConfig_GetServerConfig_DoesNotPersistSynthesizedDefault in +// auth_core_test.go. CommandAuthConfigBasic defines its own GetServerConfig() +// that shadows the embedded CommandAuthConfig's method via Go's method +// resolution, so a fix landed only on the base type does not protect this -- +// or any other real caller-facing -- concrete type. CommandAuthConfigBasic is +// what every real basic-auth caller (kfutil, keyfactor-go-client, etc.) +// actually constructs. +// +// A value that was never explicitly configured (no struct field, no +// WithClientTimeout(), no env var, no file value) must not be serialized. +func TestCommandAuthConfigBasic_GetServerConfig_DoesNotPersistSynthesizedDefault(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorUsername, "test-user") + t.Setenv(auth_providers.EnvKeyfactorPassword, "test-pass") + t.Setenv(auth_providers.EnvKeyfactorDomain, "test-domain") + + config := &auth_providers.CommandAuthConfigBasic{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 0 { + t.Fatalf("expected Server.ClientTimeout to be omitted (0) for a synthesized default, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfigBasic_GetServerConfig_PersistsExplicitTimeout proves +// the companion positive case: an explicitly configured timeout must still be +// serialized by CommandAuthConfigBasic.GetServerConfig(). +func TestCommandAuthConfigBasic_GetServerConfig_PersistsExplicitTimeout(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorUsername, "test-user") + t.Setenv(auth_providers.EnvKeyfactorPassword, "test-pass") + t.Setenv(auth_providers.EnvKeyfactorDomain, "test-domain") + + config := &auth_providers.CommandAuthConfigBasic{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + config.WithClientTimeout(300) + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 300 { + t.Fatalf("expected Server.ClientTimeout to be 300, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfigBasic_PersistedDefaultConfigFile_DoesNotShadowEnvVar is +// the CommandAuthConfigBasic analogue of +// TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar: a +// synthesized default persisted to a config file by a first run must not +// shadow KEYFACTOR_CLIENT_TIMEOUT on a second run that loads that file. +func TestCommandAuthConfigBasic_PersistedDefaultConfigFile_DoesNotShadowEnvVar(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorUsername, "test-user") + t.Setenv(auth_providers.EnvKeyfactorPassword, "test-pass") + t.Setenv(auth_providers.EnvKeyfactorDomain, "test-domain") + + // Run 1: nothing explicitly configured for client timeout. + run1 := &auth_providers.CommandAuthConfigBasic{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + if err := run1.ValidateAuthConfig(); err != nil { + t.Fatalf("run1: expected no error, got %v", err) + } + + persisted := run1.GetServerConfig() + + // Persist exactly what kfutil's login flow persists: the resolved Server + // config, verbatim, to the "default" profile of a config file. + dir := t.TempDir() + path := dir + "/command_config.json" + fileContents, mErr := json.Marshal( + map[string]interface{}{ + "servers": map[string]interface{}{ + "default": persisted, + }, + }, + ) + if mErr != nil { + t.Fatalf("failed to marshal persisted config: %v", mErr) + } + if err := os.WriteFile(path, fileContents, 0o600); err != nil { + t.Fatalf("failed to write persisted config file: %v", err) + } + + // Run 2: a fresh process loads that persisted file and has + // KEYFACTOR_CLIENT_TIMEOUT set in its environment. + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "1800") + + run2 := &auth_providers.CommandAuthConfigBasic{} + run2.WithConfigFile(path).WithConfigProfile("default") + + if err := run2.ValidateAuthConfig(); err != nil { + t.Fatalf("run2: expected no error from ValidateAuthConfig, got %v", err) + } + + if run2.HttpClientTimeout != 1800 { + t.Fatalf( + "expected KEYFACTOR_CLIENT_TIMEOUT=1800 to be honored, but a persisted synthesized default shadowed it: got HttpClientTimeout=%d", + run2.HttpClientTimeout, + ) + } +} + func authBasicTest( t *testing.T, testName string, allowFail bool, config *auth_providers.CommandAuthConfigBasic, errorContains ...string, diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index c2ca07d..380cc20 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -23,7 +23,9 @@ import ( "fmt" "io" "log" + "net" "net/http" + "net/url" "os" "path/filepath" "strconv" @@ -87,6 +89,40 @@ const ( EnvKeyfactorClientTimeout = "KEYFACTOR_CLIENT_TIMEOUT" ) +// These transport-level timeouts govern connection pool/handshake behavior, +// not the overall request deadline (that's HttpClientTimeout, which drives +// ResponseHeaderTimeout). They are fixed, sane defaults -- matching +// net/http.DefaultTransport -- and must never scale with HttpClientTimeout; +// see newHTTPTransport's doc comment for the resource-leak history behind +// this. +const ( + // DefaultIdleConnTimeout is how long an idle pooled connection is + // retained before being closed. Matches net/http.DefaultTransport. + DefaultIdleConnTimeout = 90 * time.Second + + // DefaultExpectContinueTimeout is how long to wait for a "100 Continue" + // response before sending the request body. Matches + // net/http.DefaultTransport. + DefaultExpectContinueTimeout = 1 * time.Second + + // DefaultTLSHandshakeTimeout is how long to wait for the TLS handshake + // to complete. Matches net/http.DefaultTransport. + DefaultTLSHandshakeTimeout = 10 * time.Second + + // DefaultDialTimeout bounds the TCP connect (dial) phase of a request. + // Matches net/http.DefaultTransport's own dialer timeout. Neither + // ResponseHeaderTimeout nor TLSHandshakeTimeout starts counting until + // *after* a TCP connection exists, so without an explicit dial timeout a + // black-holed destination (connection attempt met with silence, not even + // a RST/ICMP rejection) hangs with no ceiling at all -- independent of, + // and unbounded by, HttpClientTimeout. Pinned to a fixed default rather + // than scaled with HttpClientTimeout for the same reason as the other + // constants in this block: a large HttpClientTimeout configured for slow + // request bodies (e.g. 1800s for PFX enrollment) must not also permit a + // 1800s hang just to establish the TCP connection. + DefaultDialTimeout = 30 * time.Second +) + // Authenticator is an interface for authentication to Keyfactor Command API. type Authenticator interface { GetHttpClient() (*http.Client, error) @@ -150,6 +186,19 @@ type CommandAuthConfig struct { // HttpClient is the http Client to be used for authentication to Keyfactor Command API HttpClient *http.Client //DefaultHttpClient *http.Client + + // clientTimeoutDefaulted records whether HttpClientTimeout's current + // value was synthesized by ValidateAuthConfig's package-default fallback + // (DefaultClientTimeout) rather than explicitly configured by the caller + // (struct field, WithClientTimeout(), the KEYFACTOR_CLIENT_TIMEOUT env + // var, or an existing FileConfig value). GetServerConfig() consults this + // to avoid persisting a value the user never chose -- see + // TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar + // for why persisting the synthesized default is actively harmful: it + // gets written to disk, and on the next run is indistinguishable from a + // real file-configured value, which by design takes precedence over the + // env var and so permanently shadows it. + clientTimeoutDefaulted bool } // GetCommandVersion returns the Keyfactor Command product version detected during authentication. @@ -247,6 +296,10 @@ func (c *CommandAuthConfig) WithConfigProfile(profile string) *CommandAuthConfig // WithClientTimeout sets the timeout for the http Client. func (c *CommandAuthConfig) WithClientTimeout(timeout int) *CommandAuthConfig { c.HttpClientTimeout = timeout + // An explicit caller choice always overrides any earlier + // ValidateAuthConfig-synthesized default -- see clientTimeoutDefaulted's + // doc comment. + c.clientTimeoutDefaulted = false return c } @@ -284,11 +337,35 @@ func (c *CommandAuthConfig) ValidateAuthConfig() error { if c.HttpClientTimeout <= 0 { if timeout, ok := os.LookupEnv(EnvKeyfactorClientTimeout); ok { configTimeout, tErr := strconv.Atoi(timeout) - if tErr == nil { + if tErr != nil { + log.Printf( + "[ERROR] invalid value %q for environment variable %s: %v; falling back to config file/default timeout", + timeout, EnvKeyfactorClientTimeout, tErr, + ) + } else if configTimeout <= 0 { + log.Printf( + "[WARN] environment variable %s must be a positive integer, got %d; falling back to config file/default timeout", + EnvKeyfactorClientTimeout, configTimeout, + ) + } else { c.HttpClientTimeout = configTimeout } - } else { - c.HttpClientTimeout = DefaultClientTimeout + } + // Fall back to the value loaded from the config file (if any), then the + // package default. This mirrors the CommandHostName fallback above and + // ensures an unset/unparseable env var can never leave HttpClientTimeout + // at its zero value, which would otherwise disable http.Client/Transport + // timeouts entirely (see issue tracking the unbounded-wait hazard). + if c.HttpClientTimeout <= 0 { + if c.FileConfig != nil && c.FileConfig.ClientTimeout > 0 { + c.HttpClientTimeout = c.FileConfig.ClientTimeout + } else { + c.HttpClientTimeout = DefaultClientTimeout + // This value was synthesized, not chosen -- see + // clientTimeoutDefaulted's doc comment. GetServerConfig() + // must not persist it. + c.clientTimeoutDefaulted = true + } } } @@ -306,22 +383,78 @@ func (c *CommandAuthConfig) ValidateAuthConfig() error { return nil } -// BuildTransport creates a custom http Transport for authentication to Keyfactor Command API. -func (c *CommandAuthConfig) BuildTransport() (*http.Transport, error) { - defaultTimeout := time.Duration(c.HttpClientTimeout) * time.Second - output := http.Transport{ +// newHTTPTransport builds the *http.Transport shared by BuildTransport and +// SetClient's zero-value client construction. +// +// Only ResponseHeaderTimeout is derived from CommandAuthConfig.HttpClientTimeout, +// since it is the one true per-request deadline here -- it's what surfaces to +// callers as "net/http: timeout awaiting response headers" and is the field a +// large HttpClientTimeout (e.g. 1800s for slow PFX enrollments) is meant to +// fix. +// +// IdleConnTimeout, ExpectContinueTimeout, and TLSHandshakeTimeout are pinned +// to fixed, sane defaults instead of scaling with HttpClientTimeout: +// +// - IdleConnTimeout governs how long an *idle* pooled connection is kept +// around, not a request deadline. Tying it to HttpClientTimeout meant a +// large configured timeout (needed for slow requests) also kept every +// idle socket -- and its goroutine -- alive for that same duration. A +// `terraform apply` issuing many sequential requests at a 1800s timeout +// therefore leaked hundreds of open sockets/goroutines for half an hour; +// at a 1s timeout everything was released almost immediately. We use +// net/http.DefaultTransport's default of 90s. +// - ExpectContinueTimeout is how long to wait for a "100 Continue" response +// before sending the request body; it's unrelated to the response +// deadline. We use net/http.DefaultTransport's default of 1s. +// - TLSHandshakeTimeout is a handshake deadline, not an idle-resource +// timeout, so it doesn't contribute to the leak above. It's pinned here +// anyway (rather than left scaling with HttpClientTimeout) on the same +// principle: a hung TLS handshake should fail fast and free the +// connection attempt independent of how long the caller is willing to +// wait for a slow response body. We use net/http.DefaultTransport's +// default of 10s. +// - DialContext bounds the TCP connect phase itself, before +// TLSHandshakeTimeout or ResponseHeaderTimeout ever start counting. Left +// unset, the underlying http.Transport falls back to a zero-value +// net.Dialer with no timeout at all, so a black-holed destination (no +// RST/ICMP, just silence) hangs indefinitely -- unbounded by +// HttpClientTimeout, TLSHandshakeTimeout, or anything else in this +// chain. Pinned to DefaultDialTimeout (fixed, matching +// net/http.DefaultTransport) rather than scaled with HttpClientTimeout, +// for the same reason as the other fixed defaults above. +func (c *CommandAuthConfig) newHTTPTransport() *http.Transport { + return &http.Transport{ Proxy: http.ProxyFromEnvironment, TLSClientConfig: &tls.Config{ Renegotiation: tls.RenegotiateOnceAsClient, }, - TLSHandshakeTimeout: defaultTimeout, - ResponseHeaderTimeout: defaultTimeout, - IdleConnTimeout: defaultTimeout, - ExpectContinueTimeout: defaultTimeout, + DialContext: (&net.Dialer{Timeout: DefaultDialTimeout}).DialContext, + TLSHandshakeTimeout: DefaultTLSHandshakeTimeout, + ResponseHeaderTimeout: time.Duration(c.HttpClientTimeout) * time.Second, + IdleConnTimeout: DefaultIdleConnTimeout, + ExpectContinueTimeout: DefaultExpectContinueTimeout, MaxIdleConns: 10, MaxIdleConnsPerHost: 10, - MaxConnsPerHost: 10, + // MaxConnsPerHost is intentionally left at 0 (unbounded, matching + // net/http.DefaultTransport). This transport is now cached and reused + // as a single long-lived *http.Client/*http.Transport by callers (to + // fix a socket-leak bug where a fresh transport was built per + // request), so a nonzero MaxConnsPerHost here would become a hard, + // unqueued-timeout ceiling on concurrent in-flight requests per host + // for the lifetime of the 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 this client's + // Timeout nor its requests' contexts impose one. MaxIdleConns/ + // MaxIdleConnsPerHost above still bound long-term idle-socket + // retention, which is the resource concern MaxConnsPerHost was + // presumably added for. + MaxConnsPerHost: 0, } +} + +// BuildTransport creates a custom http Transport for authentication to Keyfactor Command API. +func (c *CommandAuthConfig) BuildTransport() (*http.Transport, error) { + output := c.newHTTPTransport() if c.SkipVerify { output.TLSClientConfig.InsecureSkipVerify = true @@ -331,7 +464,7 @@ func (c *CommandAuthConfig) BuildTransport() (*http.Transport, error) { if _, err := os.Stat(c.CommandCACert); err == nil { cert, ioErr := os.ReadFile(c.CommandCACert) if ioErr != nil { - return &output, ioErr + return output, ioErr } // check if output.TLSClientConfig.RootCAs is nil if output.TLSClientConfig.RootCAs == nil { @@ -339,7 +472,7 @@ func (c *CommandAuthConfig) BuildTransport() (*http.Transport, error) { } // Append your custom cert to the pool if ok := output.TLSClientConfig.RootCAs.AppendCertsFromPEM(cert); !ok { - return &output, fmt.Errorf("failed to append custom CA cert to pool") + return output, fmt.Errorf("failed to append custom CA cert to pool") } } else { if output.TLSClientConfig.RootCAs == nil { @@ -347,12 +480,12 @@ func (c *CommandAuthConfig) BuildTransport() (*http.Transport, error) { } // Append your custom cert to the pool if ok := output.TLSClientConfig.RootCAs.AppendCertsFromPEM([]byte(c.CommandCACert)); !ok { - return &output, fmt.Errorf("failed to append custom CA cert to pool") + return output, fmt.Errorf("failed to append custom CA cert to pool") } } } - return &output, nil + return output, nil } // SetClient sets the http Client for authentication to Keyfactor Command API. @@ -365,27 +498,12 @@ func (c *CommandAuthConfig) SetClient(client *http.Client) *http.Client { //defaultTransport := http.DefaultTransport.(*http.Transport).Clone() ////defaultTransport.TLSClientConfig = tlsConfig //c.HttpClient = &http.Client{Transport: defaultTransport} - defaultTimeout := time.Duration(c.HttpClientTimeout) * time.Second + // Shares its transport construction (and, critically, the fixed + // IdleConnTimeout/ExpectContinueTimeout/TLSHandshakeTimeout defaults) + // with BuildTransport() via newHTTPTransport() -- see its doc comment + // for why those must not scale with HttpClientTimeout. c.HttpClient = &http.Client{ - Transport: &http.Transport{ - Proxy: http.ProxyFromEnvironment, - TLSClientConfig: &tls.Config{ - Renegotiation: tls.RenegotiateOnceAsClient, - }, - TLSHandshakeTimeout: defaultTimeout, - DisableKeepAlives: false, - DisableCompression: false, - MaxIdleConns: 10, - MaxIdleConnsPerHost: 10, - MaxConnsPerHost: 10, - IdleConnTimeout: defaultTimeout, - ResponseHeaderTimeout: defaultTimeout, - ExpectContinueTimeout: defaultTimeout, - MaxResponseHeaderBytes: 0, - WriteBufferSize: 0, - ReadBufferSize: 0, - ForceAttemptHTTP2: false, - }, + Transport: c.newHTTPTransport(), } } @@ -708,6 +826,9 @@ func (c *CommandAuthConfig) LoadConfig(profile string, configFilePath string, si if !c.SkipVerify { c.SkipVerify = server.SkipTLSVerify } + if c.HttpClientTimeout <= 0 { + c.HttpClientTimeout = server.ClientTimeout + } //if !silentLoad { // c.CommandHostName = server.Host @@ -764,6 +885,16 @@ func (c *CommandAuthConfig) GetServerConfig() *Server { CACertPath: c.CommandCACert, AuthType: "", } + // Never persist a timeout the user never chose. If ValidateAuthConfig + // synthesized HttpClientTimeout from DefaultClientTimeout because nothing + // else was configured, leave Server.ClientTimeout at its zero value (and + // therefore omitted by its `omitempty` JSON/YAML tag) rather than writing + // out a value that would masquerade as an explicit file-configured + // setting -- and therefore permanently shadow KEYFACTOR_CLIENT_TIMEOUT -- + // on the next load. See clientTimeoutDefaulted's doc comment. + if !c.clientTimeoutDefaulted { + server.ClientTimeout = c.HttpClientTimeout + } return &server } @@ -793,6 +924,286 @@ type contextKey string // } // } +// redactedPlaceholder replaces the value of any sensitive field before a +// request body is rendered into a shareable curl command or written to a +// log. It is intentionally distinctive so it can never be mistaken for real +// data. +const redactedPlaceholder = "***REDACTED***" + +// sensitiveBodyKeys is the set of JSON/form field names -- matched +// case-insensitively -- whose values must never be written to a log or a +// generated curl command. This covers the Keyfactor Command API's +// credential-bearing request fields (certificate enrollment/PFX passwords, +// PAM secret values, etc.) as well as common OAuth2 token exchange fields. +// +// "value" is deliberately blanket-redacted rather than only when nested under +// a credential-bearing parent key (e.g. PAM's ProviderTypeParamValues): it is +// how PAM provider creation carries its secret +// (ProviderCreateRequestTypeParamValue.Value), and this redactor walks +// structure generically without tracking which object it's currently inside, +// so a parent-key allowlist would need its own maintenance burden and would +// still miss any future generic-"Value" secret field. "value" as a bare key +// name is not common enough elsewhere in the Command API surface to justify +// that risk, and the surrounding key names (e.g. the parameter name and +// ProviderTypeParamValues itself) remain visible, so little diagnostic value +// is actually lost. +// +// "properties" is deliberately NOT in this set: certificate stores serialize +// their entire (mostly non-secret) Properties map into a single JSON-encoded +// string field, and blanket-redacting it would hide store configuration +// (container names, client machine paths, etc.) that's routinely needed for +// diagnostics. Instead, redactJSONValue re-parses JSON-encoded string values +// (see below) and redacts sensitive keys *within* Properties, preserving the +// rest of its structure. +var sensitiveBodyKeys = map[string]struct{}{ + "password": {}, + "pfxpassword": {}, + "keypassword": {}, + "entrypassword": {}, + "explicitpassword": {}, + "authcertificatepassword": {}, + "newpassword": {}, + "serverpassword": {}, + "storepassword": {}, + "relaypassword": {}, + "passphrase": {}, + "privatekey": {}, + "pkcs12blob": {}, + "secret": {}, + "secretvalue": {}, + "value": {}, + "clientsecret": {}, + "client_secret": {}, + "accesstoken": {}, + "access_token": {}, + "refreshtoken": {}, + "refresh_token": {}, + "apikey": {}, + "api_key": {}, +} + +// isSensitiveBodyKey reports whether key names a field whose value should be +// redacted before logging, matching case-insensitively. +func isSensitiveBodyKey(key string) bool { + _, ok := sensitiveBodyKeys[strings.ToLower(key)] + return ok +} + +const ( + // maxNestedJSONStringDepth bounds how many levels of JSON-encoded-string + // nesting redactJSONValue will unwrap (e.g. a JSON body whose string + // field is itself a JSON document whose string field is itself JSON, + // and so on -- exactly how keyfactor-go-client encodes a certificate + // store's Properties map). This is unrelated to, and does not limit, + // ordinary object/array nesting depth; it only bounds re-parsing a + // string value as a fresh JSON document, which is what makes + // pathological/adversarial nesting expensive. It defends against a body + // crafted to smuggle a secret past redaction via deep string-in-string + // nesting. + maxNestedJSONStringDepth = 6 + + // maxNestedJSONStringLen bounds the size of a string value redactJSONValue + // will attempt to re-parse as nested JSON, so a single request log line + // can't be forced to do unbounded parsing work on an attacker-controlled + // multi-megabyte string. + maxNestedJSONStringLen = 1 << 20 // 1 MiB +) + +// redactJSONValue walks a value decoded from JSON (map[string]interface{}, +// []interface{}, or a scalar) and returns a copy with the values of any +// sensitive keys replaced by redactedPlaceholder. Structure (object/array +// nesting) is preserved so the rest of the body remains useful for +// diagnostics. +// +// String values that look like a JSON document (e.g. a certificate store's +// Properties field, which keyfactor-go-client marshals into a JSON-encoded +// string rather than a nested object) are recursively re-parsed and redacted +// the same way, up to maxNestedJSONStringDepth levels deep -- otherwise a +// sensitive field nested inside such a string would never be inspected at +// all, since its key name is invisible until the string is parsed. +func redactJSONValue(v interface{}) interface{} { + return redactJSONValueAtDepth(v, 0) +} + +func redactJSONValueAtDepth(v interface{}, nestedStringDepth int) interface{} { + switch val := v.(type) { + case map[string]interface{}: + out := make(map[string]interface{}, len(val)) + for k, vv := range val { + if isSensitiveBodyKey(k) { + out[k] = redactedPlaceholder + continue + } + out[k] = redactJSONValueAtDepth(vv, nestedStringDepth) + } + return out + case []interface{}: + out := make([]interface{}, len(val)) + for i, vv := range val { + out[i] = redactJSONValueAtDepth(vv, nestedStringDepth) + } + return out + case string: + return redactNestedJSONString(val, nestedStringDepth) + default: + return val + } +} + +// utf8BOM is the UTF-8 encoding of U+FEFF, the Unicode byte-order mark. +// Files/values authored on Windows (e.g. a PAM/orchestrator service-account +// JSON key embedded in a Properties map value) commonly carry a leading BOM. +const utf8BOM = "\uFEFF" + +// stripLeadingBOM removes a leading UTF-8 byte-order-mark from s, if present. +// strings.TrimSpace does not do this: unicode.IsSpace deliberately does not +// treat U+FEFF as whitespace (it's a formatting character, not a space), so a +// BOM-prefixed JSON document survives TrimSpace untouched. Both +// looksLikeJSONDocument's outermost-byte check and redactNestedJSONString's +// actual json.Unmarshal call need the BOM stripped first: encoding/json does +// not tolerate a leading BOM either (json.Valid/json.Unmarshal reject it, not +// silently skip it -- verified empirically), so stripping it explicitly is +// required here, not merely one option among equally-robust choices. +func stripLeadingBOM(s string) string { + return strings.TrimPrefix(s, utf8BOM) +} + +// looksLikeJSONDocument reports whether s is plausibly a JSON object or +// array, based solely on its outermost delimiters. It is intentionally cheap +// and permissive (an unbalanced-but-bracketed string will still attempt to +// parse and fail cleanly in redactNestedJSONString) so that every candidate +// gets a real parse attempt rather than being skipped on a heuristic and +// potentially leaking a secret verbatim. A leading byte-order-mark is +// stripped first -- see stripLeadingBOM -- so a BOM-prefixed JSON document is +// still recognized as JSON rather than silently treated as an opaque string +// and never inspected for nested secrets at all. +func looksLikeJSONDocument(s string) bool { + t := strings.TrimSpace(stripLeadingBOM(s)) + if len(t) < 2 { + return false + } + return (t[0] == '{' && t[len(t)-1] == '}') || (t[0] == '[' && t[len(t)-1] == ']') +} + +// redactNestedJSONString handles a single string value encountered while +// walking a decoded JSON body. Strings that don't look like a JSON document +// are left untouched. Strings that do are re-parsed and redacted like any +// other JSON value and re-serialized -- unless doing so isn't safe (parse +// failure, or the depth/size guards below are hit), in which case the whole +// value is replaced with redactedPlaceholder rather than ever emitting a +// string that looked like it might contain structured secret data. +func redactNestedJSONString(s string, nestedStringDepth int) interface{} { + if !looksLikeJSONDocument(s) { + return s + } + + if nestedStringDepth >= maxNestedJSONStringDepth { + log.Printf( + "[WARN] request body redaction: JSON-in-string nesting exceeded max depth %d; redacting the value entirely rather than risk an unredacted secret", + maxNestedJSONStringDepth, + ) + return redactedPlaceholder + } + if len(s) > maxNestedJSONStringLen { + log.Printf( + "[WARN] request body redaction: JSON-in-string value exceeded %d bytes; redacting the value entirely rather than risk an unredacted secret", + maxNestedJSONStringLen, + ) + return redactedPlaceholder + } + + var parsed interface{} + // encoding/json rejects a leading BOM outright (json.Unmarshal returns an + // error rather than skipping it), so it must be stripped here too, not + // just in looksLikeJSONDocument's sniff above -- otherwise every + // BOM-prefixed value that reaches this point would always fail to parse + // and fall through to the whole-value redactedPlaceholder branch below, + // losing the surrounding key names' diagnostic value for no security + // benefit (the BOM carries no information worth preserving). + if err := json.Unmarshal([]byte(stripLeadingBOM(s)), &parsed); err != nil { + // Looks like JSON (balanced outer brackets) but doesn't actually + // parse -- could be a truncated or malformed secret-bearing + // fragment. Never emit it raw. + return redactedPlaceholder + } + + redacted := redactJSONValueAtDepth(parsed, nestedStringDepth+1) + out, err := json.Marshal(redacted) + if err != nil { + return redactedPlaceholder + } + return string(out) +} + +// opaqueBodyMarker renders the safe placeholder used whenever a request +// body cannot be confidently classified (and therefore redacted) as JSON or +// form-encoded. It deliberately omits the body content entirely rather than +// guessing, since printing raw bytes here could leak a secret. +func opaqueBodyMarker(contentType string, size int) string { + ct := contentType + if ct == "" { + ct = "unknown" + } + return fmt.Sprintf("", size, ct) +} + +// redactRequestBody renders a safe, loggable representation of an HTTP +// request body for inclusion in a generated curl command. JSON bodies are +// parsed and re-serialized with sensitive values replaced; form-encoded +// bodies (e.g. OAuth2 client_credentials token requests carrying +// client_secret) have sensitive form values replaced. Any body that can't be +// safely classified -- including a body declared as JSON that fails to parse +// -- is omitted entirely behind opaqueBodyMarker rather than risking a raw +// secret leak. +// +// A field name being absent from sensitiveBodyKeys is not by itself proof a +// value is safe to print: the Keyfactor Command API also carries secrets +// inside ordinary JSON string values that are themselves JSON documents +// (e.g. a certificate store's Properties field). redactJSONValue re-parses +// and redacts those recursively (bounded by maxNestedJSONStringDepth/ +// maxNestedJSONStringLen) rather than treating a string as an opaque scalar, +// so a sensitive key hidden inside such a string is still found and +// redacted. +func redactRequestBody(contentType string, body []byte) string { + if len(body) == 0 { + return "" + } + + ct := strings.ToLower(contentType) + + switch { + case strings.Contains(ct, "json"), ct == "" && json.Valid(body): + var parsed interface{} + if err := json.Unmarshal(body, &parsed); err != nil { + log.Printf("[ERROR] failed to parse request body declared as JSON for redaction: %v", err) + return opaqueBodyMarker(contentType, len(body)) + } + redacted := redactJSONValue(parsed) + out, err := json.Marshal(redacted) + if err != nil { + log.Printf("[ERROR] failed to marshal redacted request body: %v", err) + return opaqueBodyMarker(contentType, len(body)) + } + return string(out) + case strings.Contains(ct, "www-form-urlencoded"): + values, err := url.ParseQuery(string(body)) + if err != nil { + log.Printf("[ERROR] failed to parse form-encoded request body for redaction: %v", err) + return opaqueBodyMarker(contentType, len(body)) + } + for k := range values { + if isSensitiveBodyKey(k) { + values[k] = []string{redactedPlaceholder} + } + } + return values.Encode() + default: + // Unknown/opaque content type: never print raw bytes, since we can't + // confirm there's no secret buried in them. + return opaqueBodyMarker(contentType, len(body)) + } +} + func RequestToCurl(req *http.Request) (string, error) { var curlCommand strings.Builder @@ -853,7 +1264,8 @@ 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))) + redactedBody := redactRequestBody(req.Header.Get("Content-Type"), body) + curlCommand.WriteString(fmt.Sprintf("--data %q ", redactedBody)) } } diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 43eb765..a62ab6d 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -15,9 +15,13 @@ package auth_providers_test import ( + "encoding/json" + "fmt" "net/http" + "os" "strings" "testing" + "time" "github.com/Keyfactor/keyfactor-auth-client-go/auth_providers" ) @@ -63,6 +67,502 @@ func TestCommandAuthConfig_SetClient(t *testing.T) { } } +// TestCommandAuthConfig_ClientTimeout_ServerRoundTrip is a regression test for +// https://github.com/Keyfactor/keyfactor-auth-client-go/issues/51: a +// WithClientTimeout value set on CommandAuthConfig must survive the round trip +// through GetServerConfig()'s *Server representation instead of being silently +// dropped. Before the fix, Server had no ClientTimeout field at all, so this +// assertion failed to compile/would read the Go zero value (0). +func TestCommandAuthConfig_ClientTimeout_ServerRoundTrip(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + config.WithClientTimeout(300) + + server := config.GetServerConfig() + if server.ClientTimeout != 300 { + t.Fatalf("expected Server.ClientTimeout to be 300, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfig_ClientTimeout_BuildTransport is a regression test proving +// that a non-default client timeout actually reaches BuildTransport()'s derived +// ResponseHeaderTimeout (the field responsible for the customer-observed +// "net/http: timeout awaiting response headers" error at the default 60s). +func TestCommandAuthConfig_ClientTimeout_BuildTransport(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + config.WithClientTimeout(300) + + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + expected := 300 * time.Second + if transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } +} + +// TestCommandAuthConfig_IdleAndExpectContinueTimeouts_NotDerivedFromClientTimeout +// is a regression test for a resource leak: BuildTransport() and SetClient() +// both derived IdleConnTimeout and ExpectContinueTimeout from the same +// HttpClientTimeout value used for the request deadline +// (ResponseHeaderTimeout). IdleConnTimeout governs how long an *idle* pooled +// connection is retained -- it is not a request deadline -- so a large +// configured HttpClientTimeout (e.g. 1800s, exactly what's needed for slow +// PFX enrollments) caused idle sockets and their goroutines to be retained +// for the full 1800s after every request, instead of net/http's normal 90s. +// A large `terraform apply` issuing many sequential requests therefore held +// open hundreds of sockets/goroutines for half an hour. ExpectContinueTimeout +// has the same bug for the same reason. +// +// ResponseHeaderTimeout must continue to track HttpClientTimeout -- that is +// the customer-facing fix the timeout work exists for -- while +// IdleConnTimeout and ExpectContinueTimeout must stay pinned to fixed, +// sane defaults (matching net/http.DefaultTransport) regardless of how large +// HttpClientTimeout is configured. +func TestCommandAuthConfig_IdleAndExpectContinueTimeouts_NotDerivedFromClientTimeout(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + config.WithClientTimeout(1800) + + t.Run("BuildTransport", func(t *testing.T) { + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if expected := 1800 * time.Second; transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } + if transport.IdleConnTimeout != auth_providers.DefaultIdleConnTimeout { + t.Fatalf( + "expected IdleConnTimeout to stay pinned at the fixed default %v regardless of a 1800s HttpClientTimeout, got %v", + auth_providers.DefaultIdleConnTimeout, transport.IdleConnTimeout, + ) + } + if transport.ExpectContinueTimeout != auth_providers.DefaultExpectContinueTimeout { + t.Fatalf( + "expected ExpectContinueTimeout to stay pinned at the fixed default %v regardless of a 1800s HttpClientTimeout, got %v", + auth_providers.DefaultExpectContinueTimeout, transport.ExpectContinueTimeout, + ) + } + }) + + t.Run("SetClient", func(t *testing.T) { + client := config.SetClient(nil) + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected client.Transport to be *http.Transport, got %T", client.Transport) + } + + if expected := 1800 * time.Second; transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } + if transport.IdleConnTimeout != auth_providers.DefaultIdleConnTimeout { + t.Fatalf( + "expected IdleConnTimeout to stay pinned at the fixed default %v regardless of a 1800s HttpClientTimeout, got %v", + auth_providers.DefaultIdleConnTimeout, transport.IdleConnTimeout, + ) + } + if transport.ExpectContinueTimeout != auth_providers.DefaultExpectContinueTimeout { + t.Fatalf( + "expected ExpectContinueTimeout to stay pinned at the fixed default %v regardless of a 1800s HttpClientTimeout, got %v", + auth_providers.DefaultExpectContinueTimeout, transport.ExpectContinueTimeout, + ) + } + }) +} + +// writeTimeoutConfigFile writes a minimal config file with a single "default" +// profile carrying the given client_timeout (in seconds) and returns its path. +func writeTimeoutConfigFile(t *testing.T, clientTimeout int) string { + t.Helper() + dir := t.TempDir() + path := dir + "/command_config.json" + contents := fmt.Sprintf( + `{"servers":{"default":{"host":"file-host.example.com","client_timeout":%d}}}`, + clientTimeout, + ) + if err := os.WriteFile(path, []byte(contents), 0o600); err != nil { + t.Fatalf("failed to write test config file: %v", err) + } + return path +} + +// TestCommandAuthConfig_ClientTimeout_FileConfigHonored is a regression test +// proving that a client_timeout set only in a config file profile is actually +// honored end-to-end: LoadConfig -> ValidateAuthConfig -> BuildTransport. +// Before the fix, LoadConfig never merged Server.ClientTimeout into +// CommandAuthConfig.HttpClientTimeout (unlike Host/Port/APIPath/CACertPath/ +// SkipVerify, which it already merged), and ValidateAuthConfig never +// consulted c.FileConfig as a fallback (unlike the CommandHostName branch), +// so the file value was silently dropped and HttpClientTimeout landed on the +// 60s default instead. +func TestCommandAuthConfig_ClientTimeout_FileConfigHonored(t *testing.T) { + path := writeTimeoutConfigFile(t, 300) + + config := &auth_providers.CommandAuthConfig{} + config.WithConfigFile(path).WithConfigProfile("default") + + if _, err := config.LoadConfig(config.ConfigProfile, config.ConfigFilePath, true); err != nil { + t.Fatalf("expected no error from LoadConfig, got %v", err) + } + + if config.FileConfig == nil || config.FileConfig.ClientTimeout != 300 { + t.Fatalf("expected FileConfig.ClientTimeout to be 300, got %+v", config.FileConfig) + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error from ValidateAuthConfig, got %v", err) + } + + if config.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300, got %d", config.HttpClientTimeout) + } + + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error from BuildTransport, got %v", err) + } + + expected := 300 * time.Second + if transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } +} + +// TestCommandAuthConfig_ClientTimeout_FileConfigFallbackOnly is a narrower +// regression test isolating the ValidateAuthConfig fallback path: it sets +// FileConfig directly (as would happen if a caller populates it without +// LoadConfig's eager merge) and confirms ValidateAuthConfig still falls back +// to it, mirroring the existing CommandHostName/c.FileConfig fallback +// convention in this function. +func TestCommandAuthConfig_ClientTimeout_FileConfigFallbackOnly(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + FileConfig: &auth_providers.Server{ClientTimeout: 120}, + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if config.HttpClientTimeout != 120 { + t.Fatalf("expected HttpClientTimeout to be 120, got %d", config.HttpClientTimeout) + } +} + +// TestCommandAuthConfig_ClientTimeout_Precedence proves the full fallback +// chain resolves in the intended order: explicit struct value/WithClientTimeout() +// wins outright; absent that, LoadConfig's eager file merge (mirroring how +// Host/Port/APIPath/CACertPath/SkipVerify are merged) takes effect before +// ValidateAuthConfig's env var check ever runs, so a config file value takes +// precedence over the environment variable -- consistent with how +// CommandHostName already behaves in this codebase (LoadConfig always runs +// before ValidateAuthConfig in every concrete auth type's ValidateAuthConfig +// wrapper, so a file-resolved field is never re-overridden by env). Finally, +// with neither struct, file, nor env set, the package default applies. +func TestCommandAuthConfig_ClientTimeout_Precedence(t *testing.T) { + t.Run("struct value wins over file and env", func(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "45") + path := writeTimeoutConfigFile(t, 300) + + config := &auth_providers.CommandAuthConfig{} + config.WithConfigFile(path).WithConfigProfile("default") + config.WithClientTimeout(15) + + if _, err := config.LoadConfig(config.ConfigProfile, config.ConfigFilePath, true); err != nil { + t.Fatalf("expected no error from LoadConfig, got %v", err) + } + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if config.HttpClientTimeout != 15 { + t.Fatalf("expected HttpClientTimeout to be 15, got %d", config.HttpClientTimeout) + } + }) + + t.Run("file value wins over env when no struct value is set", func(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "45") + path := writeTimeoutConfigFile(t, 300) + + config := &auth_providers.CommandAuthConfig{} + config.WithConfigFile(path).WithConfigProfile("default") + + if _, err := config.LoadConfig(config.ConfigProfile, config.ConfigFilePath, true); err != nil { + t.Fatalf("expected no error from LoadConfig, got %v", err) + } + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if config.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300 (file value), got %d", config.HttpClientTimeout) + } + }) + + t.Run("env value used when no struct or file value is set", func(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "45") + + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if config.HttpClientTimeout != 45 { + t.Fatalf("expected HttpClientTimeout to be 45 (env value), got %d", config.HttpClientTimeout) + } + }) + + t.Run("default used when nothing else is set", func(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if config.HttpClientTimeout != auth_providers.DefaultClientTimeout { + t.Fatalf( + "expected HttpClientTimeout to be default %d, got %d", + auth_providers.DefaultClientTimeout, config.HttpClientTimeout, + ) + } + }) +} + +// TestCommandAuthConfig_ClientTimeout_BadEnvVarNeverDisablesTimeout is a +// regression test for the unbounded-wait hazard: os.LookupEnv reports ok=true +// for a set-but-empty env var, and an unparseable/non-positive value must +// never leave HttpClientTimeout at its zero value, since a zero timeout +// means "no timeout" throughout net/http (ResponseHeaderTimeout, +// TLSHandshakeTimeout, IdleConnTimeout, http.Client.Timeout), and +// Authenticate() builds requests with no context to otherwise bound them. +func TestCommandAuthConfig_ClientTimeout_BadEnvVarNeverDisablesTimeout(t *testing.T) { + badValues := []string{"", "abc", "60s", "0", "-5"} + + for _, v := range badValues { + v := v + t.Run(fmt.Sprintf("env=%q", v), func(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, v) + + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if config.HttpClientTimeout != auth_providers.DefaultClientTimeout { + t.Fatalf( + "expected HttpClientTimeout to fall back to default %d for env value %q, got %d", + auth_providers.DefaultClientTimeout, v, config.HttpClientTimeout, + ) + } + + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error from BuildTransport, got %v", err) + } + + expected := time.Duration(auth_providers.DefaultClientTimeout) * time.Second + if transport.ResponseHeaderTimeout != expected || transport.ResponseHeaderTimeout == 0 { + t.Fatalf( + "expected ResponseHeaderTimeout to be %v for env value %q, got %v (0 means unlimited)", + expected, v, transport.ResponseHeaderTimeout, + ) + } + }) + } +} + +// TestCommandAuthConfig_GetServerConfig_DoesNotPersistSynthesizedDefault is a +// regression test for a precedence bug introduced alongside the +// HttpClientTimeout/Server.ClientTimeout round trip: GetServerConfig() +// serialized the *resolved* HttpClientTimeout, including the 60s value +// ValidateAuthConfig synthesizes when nothing was configured. A caller that +// persists GetServerConfig()'s output to a config file (as kfutil's login +// flow does) would therefore always write client_timeout: 60 to disk, even +// though the user never chose it -- see +// TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar for +// why that phantom value is actively harmful on the next run. +// +// A value that was never explicitly configured (no struct field, no +// WithClientTimeout(), no env var, no file value) must not be serialized. +func TestCommandAuthConfig_GetServerConfig_DoesNotPersistSynthesizedDefault(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + if config.HttpClientTimeout != auth_providers.DefaultClientTimeout { + t.Fatalf("expected HttpClientTimeout to be defaulted to %d, got %d", auth_providers.DefaultClientTimeout, config.HttpClientTimeout) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 0 { + t.Fatalf("expected Server.ClientTimeout to be omitted (0) for a synthesized default, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfig_GetServerConfig_PersistsExplicitTimeout proves the +// companion positive case: an explicitly configured timeout (whether set +// directly, via WithClientTimeout(), via the environment, or via a file +// value already present on CommandAuthConfig.FileConfig before +// ValidateAuthConfig runs) must still be serialized by GetServerConfig(), so +// TestCommandAuthConfig_ClientTimeout_ServerRoundTrip's guarantee is +// preserved. +func TestCommandAuthConfig_GetServerConfig_PersistsExplicitTimeout(t *testing.T) { + t.Run("WithClientTimeout", func(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + config.WithClientTimeout(300) + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 300 { + t.Fatalf("expected Server.ClientTimeout to be 300, got %d", server.ClientTimeout) + } + }) + + t.Run("environment variable", func(t *testing.T) { + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "1800") + + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 1800 { + t.Fatalf("expected Server.ClientTimeout to be 1800, got %d", server.ClientTimeout) + } + }) + + t.Run("file config fallback", func(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + FileConfig: &auth_providers.Server{ClientTimeout: 120}, + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 120 { + t.Fatalf("expected Server.ClientTimeout to be 120, got %d", server.ClientTimeout) + } + }) +} + +// TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar is an +// end-to-end regression test for the actual customer-facing bug: kfutil's +// login flow calls ValidateAuthConfig() then GetServerConfig(), and persists +// the result verbatim to ~/.keyfactor/command_config.json. Before the fix, +// that meant a run with nothing configured wrote client_timeout: 60 to disk. +// On the *next* run, LoadConfig merges that file value into +// CommandAuthConfig.HttpClientTimeout before ValidateAuthConfig ever runs +// (mirroring how Host/Port/etc. are merged), so ValidateAuthConfig's +// `if c.HttpClientTimeout <= 0` guard was already false and the +// KEYFACTOR_CLIENT_TIMEOUT env var branch was skipped entirely -- +// permanently and silently shadowing the env var, with no diagnostic. This +// is a real regression: the env var always worked before Server gained a +// ClientTimeout field to persist. +// +// This test reproduces the full two-run cycle: run 1 resolves nothing +// explicit and persists its Server config to a file; run 2 loads that file +// with KEYFACTOR_CLIENT_TIMEOUT set and must honor the env var. +func TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar(t *testing.T) { + // Run 1: nothing explicitly configured. + run1 := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + if err := run1.ValidateAuthConfig(); err != nil { + t.Fatalf("run1: expected no error, got %v", err) + } + + persisted := run1.GetServerConfig() + + // Persist exactly what kfutil's login flow persists: the resolved Server + // config, verbatim, to the "default" profile of a config file. + dir := t.TempDir() + path := dir + "/command_config.json" + fileContents, mErr := json.Marshal(map[string]interface{}{ + "servers": map[string]interface{}{ + "default": persisted, + }, + }) + if mErr != nil { + t.Fatalf("failed to marshal persisted config: %v", mErr) + } + if err := os.WriteFile(path, fileContents, 0o600); err != nil { + t.Fatalf("failed to write persisted config file: %v", err) + } + + // Run 2: a fresh process loads that persisted file and has + // KEYFACTOR_CLIENT_TIMEOUT set in its environment. + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "1800") + + run2 := &auth_providers.CommandAuthConfig{} + run2.WithConfigFile(path).WithConfigProfile("default") + + if _, err := run2.LoadConfig(run2.ConfigProfile, run2.ConfigFilePath, true); err != nil { + t.Fatalf("run2: expected no error from LoadConfig, got %v", err) + } + + if err := run2.ValidateAuthConfig(); err != nil { + t.Fatalf("run2: expected no error from ValidateAuthConfig, got %v", err) + } + + if run2.HttpClientTimeout != 1800 { + t.Fatalf( + "expected KEYFACTOR_CLIENT_TIMEOUT=1800 to be honored, but a persisted synthesized default shadowed it: got HttpClientTimeout=%d", + run2.HttpClientTimeout, + ) + } +} + func TestCommandAuthConfig_Authenticate(t *testing.T) { config := &auth_providers.CommandAuthConfig{ CommandHostName: "test-host", @@ -175,3 +675,452 @@ func TestRequestToCurl(t *testing.T) { t.Logf("%s: curl command: %s", tt.name, curlStr) } } + +// TestRequestToCurl_BodyRedaction is a regression test for secrets being +// logged verbatim in the curl command RequestToCurl produces. Before the +// fix, RequestToCurl appended the raw request body via `--data %q` with no +// redaction at all, so any secret-bearing payload (e.g. a PFX enrollment +// request carrying a private-key password) was written in plaintext to the +// log whenever TRACE logging is enabled -- which is exactly what support +// asks a customer to enable when reporting the slow-request/timeout issues +// this library exists to fix, so plaintext secrets would routinely end up in +// support bundles. +// +// The fix must redact known-sensitive field values from JSON and +// form-encoded bodies while preserving the rest of the body for +// diagnostics, and must never fall back to printing a body it can't safely +// classify. +func TestRequestToCurl_BodyRedaction(t *testing.T) { + tests := []struct { + name string + contentType string + body string + wantInCurl []string + notWantInCurl []string + }{ + { + name: "JSON top-level sensitive key", + contentType: "application/json", + body: `{"Password":"SuperSecret1","CommonName":"test.example.com"}`, + wantInCurl: []string{ + `\"CommonName\":\"test.example.com\"`, + `\"Password\":\"***REDACTED***\"`, + }, + notWantInCurl: []string{ + "SuperSecret1", + }, + }, + { + name: "JSON nested sensitive key", + contentType: "application/json", + body: `{"Subject":"CN=test","PFXPassword":{"Value":"NestedSecret!","SecretSource":"Inline"}}`, + wantInCurl: []string{ + `\"Subject\":\"CN=test\"`, + `\"PFXPassword\":\"***REDACTED***\"`, + }, + notWantInCurl: []string{ + "NestedSecret!", + "SecretSource", + }, + }, + { + name: "JSON sensitive key inside array element", + contentType: "application/json", + body: `{"Stores":[{"StoreId":"abc","KeyPassword":"ArraySecret"}]}`, + wantInCurl: []string{ + `\"StoreId\":\"abc\"`, + `\"KeyPassword\":\"***REDACTED***\"`, + }, + notWantInCurl: []string{ + "ArraySecret", + }, + }, + { + name: "JSON case-insensitive key match", + contentType: "application/json", + body: `{"clientSecret":"CaseSecret","Name":"svc"}`, + wantInCurl: []string{ + `\"Name\":\"svc\"`, + `\"clientSecret\":\"***REDACTED***\"`, + }, + notWantInCurl: []string{ + "CaseSecret", + }, + }, + { + name: "Form-encoded body with client_secret", + contentType: "application/x-www-form-urlencoded", + body: "grant_type=client_credentials&client_id=my-client&client_secret=FormSecret", + wantInCurl: []string{ + "grant_type=client_credentials", + "client_id=my-client", + "client_secret=%2A%2A%2AREDACTED%2A%2A%2A", + }, + notWantInCurl: []string{ + "FormSecret", + }, + }, + { + name: "Opaque/unknown content type is omitted entirely", + contentType: "application/octet-stream", + body: "raw-binary-looking-payload-with-a-Password=OpaqueSecret-inside", + wantInCurl: []string{ + "redacted", + "application/octet-stream", + }, + notWantInCurl: []string{ + "OpaqueSecret", + "raw-binary-looking-payload", + }, + }, + { + name: "Empty body", + contentType: "application/json", + body: "", + wantInCurl: []string{ + "curl", "-X", "POST", + }, + }, + { + name: "JSON body with no sensitive keys stays fully visible", + contentType: "application/json", + body: `{"CommonName":"test.example.com","Template":"WebServer"}`, + wantInCurl: []string{ + `\"CommonName\":\"test.example.com\"`, + `\"Template\":\"WebServer\"`, + }, + }, + { + // Confirmed leak path 1: keyfactor-go-client v3 marshals a + // certificate store's Properties map into a JSON-encoded STRING + // field (store_models.go PropertiesString, json:"Properties"; + // see store.go). terraform-provider-keyfactor puts + // ServerPassword in that map, and for K8S store types this field + // can carry an entire kubeconfig or service-account token. + // Because redaction only inspected each value's own key against + // sensitiveBodyKeys, and never re-parsed a string value that was + // itself JSON, ServerPassword's nested SecretValue was emitted + // verbatim. + name: "certificate store Properties JSON-encoded-string leak", + contentType: "application/json", + body: `{"ClientMachine":"k8s","Properties":"{\"ServerPassword\":{\"value\":{\"SecretValue\":\"SuperSecret123\"}}}","Password":{"SecretValue":"StorePw!"}}`, + wantInCurl: []string{ + "ClientMachine", + "k8s", + "Properties", + "ServerPassword", + "REDACTED", + }, + notWantInCurl: []string{ + "SuperSecret123", + "StorePw!", + }, + }, + { + // Confirmed leak path 2: PAM provider creation carries the + // secret under the generic key "Value" + // (ProviderCreateRequestTypeParamValue.Value, pam_types_models.go), + // nested under the ordinary key ProviderTypeParamValues. "value" + // was not in sensitiveBodyKeys, so a Vault token/Delinea + // password was logged verbatim. + name: "PAM ProviderTypeParamValues generic Value key leak", + contentType: "application/json", + body: `{"Name":"my-pam-provider","ProviderTypeParamValues":{"Vault-Token":{"Value":"hvs.CONFIDENTIALVAULTTOKEN"}}}`, + wantInCurl: []string{ + "my-pam-provider", + "ProviderTypeParamValues", + "Vault-Token", + "REDACTED", + }, + notWantInCurl: []string{ + "hvs.CONFIDENTIALVAULTTOKEN", + }, + }, + { + // A string value that merely *looks* like a JSON document (starts + // with '{'/'[' and ends with '}'/']') but does not actually parse + // must never be emitted verbatim -- it could be a + // truncated/malformed secret-bearing fragment. The whole value + // must be redacted instead of falling through to raw output. + name: "malformed JSON-looking string value is redacted whole, not emitted raw", + contentType: "application/json", + body: `{"ClientMachine":"k8s","Properties":"{ServerPassword: SuperSecret123}"}`, + wantInCurl: []string{ + "ClientMachine", + "k8s", + "REDACTED", + }, + notWantInCurl: []string{ + "SuperSecret123", + }, + }, + } + + for _, tt := range tests { + req, err := http.NewRequest("POST", "https://example.com/api", strings.NewReader(tt.body)) + if err != nil { + t.Fatalf("%s: failed to create request: %v", tt.name, err) + } + if tt.contentType != "" { + req.Header.Set("Content-Type", tt.contentType) + } + + curlStr, err := auth_providers.RequestToCurl(req) + if err != nil { + t.Errorf("%s: RequestToCurl returned error: %v", tt.name, err) + continue + } + + for _, want := range tt.wantInCurl { + if !strings.Contains(curlStr, want) { + t.Errorf("%s: curl string missing %q\nGot: %s", tt.name, want, curlStr) + } + } + for _, notWant := range tt.notWantInCurl { + if strings.Contains(curlStr, notWant) { + t.Errorf("%s: curl string contains unwanted %q\nGot: %s", tt.name, notWant, curlStr) + } + } + t.Logf("%s: curl command: %s", tt.name, curlStr) + } +} + +// nestJSONString wraps innermost as the value of a "Wrapper" key, levels +// times, each wrap itself re-marshaled to JSON so quotes/backslashes are +// escaped exactly as a real nested JSON-encoded-string field would be. The +// result is a top-level JSON document whose "Wrapper" field must be re-parsed +// `levels` times by redaction before the original innermost document is +// reached -- used to probe redactJSONValue's nested-JSON-in-string recursion, +// including its depth guard against pathological/adversarial input. +func nestJSONString(t *testing.T, innermost string, levels int) string { + t.Helper() + current := innermost + for i := 0; i < levels; i++ { + wrapped, err := json.Marshal(map[string]string{"Wrapper": current}) + if err != nil { + t.Fatalf("failed to nest JSON string at level %d: %v", i, err) + } + current = string(wrapped) + } + return current +} + +// TestRequestToCurl_BodyRedaction_NestedJSONInString is a regression test +// proving redaction recurses into JSON-encoded-string values, not just +// object/array structure. It covers the two-levels-deep case explicitly +// requested as a minimum (mirroring how a real Properties string could itself +// carry a field whose value is further JSON-encoded), plus a pathological +// depth well past any real payload to prove the depth guard never lets a +// secret fall through to raw output. +func TestRequestToCurl_BodyRedaction_NestedJSONInString(t *testing.T) { + t.Run("two levels deep", func(t *testing.T) { + innermost := `{"Password":"DeepSecret"}` + body := nestJSONString(t, innermost, 2) + + req, err := http.NewRequest("POST", "https://example.com/api", strings.NewReader(body)) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + + curlStr, err := auth_providers.RequestToCurl(req) + if err != nil { + t.Fatalf("RequestToCurl returned error: %v", err) + } + + if strings.Contains(curlStr, "DeepSecret") { + t.Fatalf("secret leaked through two levels of JSON-in-string nesting\nGot: %s", curlStr) + } + if !strings.Contains(curlStr, "REDACTED") { + t.Fatalf("expected the nested Password field to be redacted, found no redaction marker\nGot: %s", curlStr) + } + if !strings.Contains(curlStr, "Password") { + t.Fatalf("expected the Password key name to remain visible for diagnostics\nGot: %s", curlStr) + } + t.Logf("curl command: %s", curlStr) + }) + + t.Run("pathological depth never leaks the secret", func(t *testing.T) { + innermost := `{"Password":"DeepSecret"}` + // Comfortably past any sane nesting depth a real API payload would + // use. Each wrap re-escapes the prior level's quotes/backslashes, so + // the encoded size grows roughly exponentially with levels -- keep + // this small (10 levels is already ~1000x deeper than any real + // payload) to avoid an enormous test body. + body := nestJSONString(t, innermost, 10) + + req, err := http.NewRequest("POST", "https://example.com/api", strings.NewReader(body)) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + + curlStr, err := auth_providers.RequestToCurl(req) + if err != nil { + t.Fatalf("RequestToCurl returned error: %v", err) + } + + if strings.Contains(curlStr, "DeepSecret") { + t.Fatalf("secret leaked through pathologically deep JSON-in-string nesting\nGot: %s", curlStr) + } + t.Logf("curl command: %s", curlStr) + }) +} + +// TestRequestToCurl_BodyRedaction_NestedJSONInString_BOMPrefixed is a +// regression test for a BOM byte defeating looksLikeJSONDocument's +// nested-JSON-in-string heuristic. That heuristic inspected only the +// first/last byte of a string value (after strings.TrimSpace) to decide +// whether it looked like a JSON document worth re-parsing and redacting. +// strings.TrimSpace uses unicode.IsSpace, which does not strip a U+FEFF +// byte-order-mark, so a nested JSON-encoded string value prefixed with a BOM +// -- plausible for a value that originated from a Windows-authored file, +// e.g. a PAM/orchestrator service-account JSON key embedded in a Properties +// map value -- was judged "not JSON" and redactNestedJSONString returned it +// completely unredacted, defeating the whole point of the nested-JSON fix +// for exactly the kind of payload it was built to catch. +// +// A nested JSON-in-string value prefixed with a BOM, containing a sensitive +// key, must be redacted just like the non-BOM-prefixed case. +func TestRequestToCurl_BodyRedaction_NestedJSONInString_BOMPrefixed(t *testing.T) { + innermost := "\uFEFF" + `{"Password":"DeepSecret"}` + wrapped, err := json.Marshal(map[string]string{"Properties": innermost}) + if err != nil { + t.Fatalf("failed to build nested body: %v", err) + } + + req, err := http.NewRequest("POST", "https://example.com/api", strings.NewReader(string(wrapped))) + if err != nil { + t.Fatalf("failed to create request: %v", err) + } + req.Header.Set("Content-Type", "application/json") + + curlStr, err := auth_providers.RequestToCurl(req) + if err != nil { + t.Fatalf("RequestToCurl returned error: %v", err) + } + + if strings.Contains(curlStr, "DeepSecret") { + t.Fatalf("secret leaked through a BOM-prefixed nested JSON-in-string value\nGot: %s", curlStr) + } + if !strings.Contains(curlStr, "REDACTED") { + t.Fatalf("expected the nested Password field to be redacted, found no redaction marker\nGot: %s", curlStr) + } + if !strings.Contains(curlStr, "Password") { + t.Fatalf("expected the Password key name to remain visible for diagnostics\nGot: %s", curlStr) + } + t.Logf("curl command: %s", curlStr) +} + +// TestCommandAuthConfig_MaxConnsPerHost_Unbounded is a regression test for a +// global-concurrency-cap bug: newHTTPTransport() hardcoded +// MaxConnsPerHost: 10. That was harmless as long as every request built its +// own throwaway transport, but consumers (e.g. keyfactor-go-client) now +// correctly cache and reuse a single *http.Client/*http.Transport across all +// requests to fix a socket leak -- which turns MaxConnsPerHost: 10 into a +// hard ceiling of 10 concurrent in-flight requests per host, no matter how +// much client-side parallelism (e.g. `terraform apply -parallelism=25`) +// callers configure. Since the returned client has Timeout: 0 and requests +// carry no context deadline, requests beyond the 10th queue with no bound. +// +// MaxConnsPerHost must match net/http.DefaultTransport's unbounded default +// (0), while the idle-connection pool limits (which bound long-term resource +// retention, not concurrency) stay as configured. +func TestCommandAuthConfig_MaxConnsPerHost_Unbounded(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + + t.Run("BuildTransport", func(t *testing.T) { + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if transport.MaxConnsPerHost != 0 { + t.Fatalf("expected MaxConnsPerHost to be unbounded (0), got %d", transport.MaxConnsPerHost) + } + if transport.MaxIdleConns != 10 { + t.Fatalf("expected MaxIdleConns to stay at 10, got %d", transport.MaxIdleConns) + } + if transport.MaxIdleConnsPerHost != 10 { + t.Fatalf("expected MaxIdleConnsPerHost to stay at 10, got %d", transport.MaxIdleConnsPerHost) + } + }) + + t.Run("SetClient", func(t *testing.T) { + client := config.SetClient(nil) + transport, ok := client.Transport.(*http.Transport) + if !ok { + t.Fatalf("expected client.Transport to be *http.Transport, got %T", client.Transport) + } + if transport.MaxConnsPerHost != 0 { + t.Fatalf("expected MaxConnsPerHost to be unbounded (0), got %d", transport.MaxConnsPerHost) + } + if transport.MaxIdleConns != 10 { + t.Fatalf("expected MaxIdleConns to stay at 10, got %d", transport.MaxIdleConns) + } + if transport.MaxIdleConnsPerHost != 10 { + t.Fatalf("expected MaxIdleConnsPerHost to stay at 10, got %d", transport.MaxIdleConnsPerHost) + } + }) +} + +// TestCommandAuthConfig_DialTimeout_BuildTransport is a regression test for the +// unbounded-TCP-dial hazard: newHTTPTransport() previously left DialContext +// unset, so the underlying http.Transport fell back to a zero-value +// net.Dialer with no timeout at all. Neither TLSHandshakeTimeout nor +// ResponseHeaderTimeout starts counting until *after* a TCP connection +// exists, so a black-holed destination (connection attempt met with silence, +// not even a RST/ICMP rejection) hung with no ceiling whatsoever -- +// independent of, and unbounded by, HttpClientTimeout. +// +// DialContext must now be set to a fixed, sane default (DefaultDialTimeout, +// matching net/http.DefaultTransport's own convention), and -- like +// IdleConnTimeout/ExpectContinueTimeout/TLSHandshakeTimeout above -- it must +// stay pinned regardless of how large HttpClientTimeout is configured (e.g. +// 1800s for slow PFX enrollments); otherwise a large HttpClientTimeout would +// also permit a 1800s hang just to establish the TCP connection. +func TestCommandAuthConfig_DialTimeout_BuildTransport(t *testing.T) { + if auth_providers.DefaultDialTimeout != 30*time.Second { + t.Fatalf("expected DefaultDialTimeout to be 30s (matching net/http.DefaultTransport), got %v", auth_providers.DefaultDialTimeout) + } + + t.Run("default HttpClientTimeout", func(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + } + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if transport.DialContext == nil { + t.Fatal("expected DialContext to be set so the TCP dial phase is bounded, got nil (unbounded dial)") + } + }) + + t.Run("large HttpClientTimeout does not scale the dial timeout", func(t *testing.T) { + config := &auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + HttpClientTimeout: 1800, + } + transport, err := config.BuildTransport() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if transport.DialContext == nil { + t.Fatal("expected DialContext to be set even with a large HttpClientTimeout, got nil (unbounded dial)") + } + // ResponseHeaderTimeout is meant to scale with HttpClientTimeout -- + // confirm this test's config actually exercises the "large timeout" + // case, so a future refactor can't silently make this a no-op. + if expected := 1800 * time.Second; transport.ResponseHeaderTimeout != expected { + t.Fatalf("expected ResponseHeaderTimeout to be %v, got %v", expected, transport.ResponseHeaderTimeout) + } + }) +} diff --git a/auth_providers/auth_kerberos.go b/auth_providers/auth_kerberos.go index 9a83f54..add16f8 100644 --- a/auth_providers/auth_kerberos.go +++ b/auth_providers/auth_kerberos.go @@ -434,22 +434,22 @@ func (k *CommandAuthConfigKerberos) parseUsernameRealm() { // GetServerConfig returns the server configuration func (k *CommandAuthConfigKerberos) GetServerConfig() *Server { - server := Server{ - Host: k.CommandHostName, - Port: k.CommandPort, - Username: k.Username, - Password: k.Password, - APIPath: k.CommandAPIPath, - SkipTLSVerify: k.SkipVerify, - CACertPath: k.CommandCACert, - AuthType: "kerberos", - KerberosRealm: k.Realm, - KerberosKeytab: k.KeytabPath, - KerberosConfig: k.ConfigPath, - KerberosCCache: k.CCachePath, - KerberosSPN: k.SPN, - } - return &server + // Delegate to the embedded CommandAuthConfig for the fields it already + // knows how to populate correctly -- notably ClientTimeout, which must be + // omitted (not the ValidateAuthConfig-synthesized default) unless the + // caller explicitly configured it. See clientTimeoutDefaulted's doc + // comment on CommandAuthConfig for why persisting a synthesized default + // is harmful. Layer Kerberos-specific fields on top. + server := k.CommandAuthConfig.GetServerConfig() + server.Username = k.Username + server.Password = k.Password + server.AuthType = "kerberos" + server.KerberosRealm = k.Realm + server.KerberosKeytab = k.KeytabPath + server.KerberosConfig = k.ConfigPath + server.KerberosCCache = k.CCachePath + server.KerberosSPN = k.SPN + return server } // fileExists checks if a file exists at the given path diff --git a/auth_providers/auth_kerberos_test.go b/auth_providers/auth_kerberos_test.go index 660a98d..ae60a05 100644 --- a/auth_providers/auth_kerberos_test.go +++ b/auth_providers/auth_kerberos_test.go @@ -15,6 +15,7 @@ package auth_providers_test import ( + "encoding/json" "fmt" "net/http" "os" @@ -121,6 +122,122 @@ func TestCommandAuthConfigKerberos_RealmNormalization(t *testing.T) { } } +// TestCommandAuthConfigKerberos_GetServerConfig_DoesNotPersistSynthesizedDefault +// is the CommandAuthConfigKerberos analogue of +// TestCommandAuthConfig_GetServerConfig_DoesNotPersistSynthesizedDefault in +// auth_core_test.go. CommandAuthConfigKerberos defines its own +// GetServerConfig() that shadows the embedded CommandAuthConfig's method via +// Go's method resolution, so a fix landed only on the base type does not +// protect this -- or any other real caller-facing -- concrete type. +// +// This exercises only the embedded CommandAuthConfig.ValidateAuthConfig(), +// not CommandAuthConfigKerberos.ValidateAuthConfig(), which requires a real +// krb5.conf file/ticket cache on disk and is irrelevant to this bug: the +// clientTimeoutDefaulted flag being tested is set by the embedded method +// regardless of which concrete type wraps it. +func TestCommandAuthConfigKerberos_GetServerConfig_DoesNotPersistSynthesizedDefault(t *testing.T) { + config := &auth_providers.CommandAuthConfigKerberos{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + + if err := config.CommandAuthConfig.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 0 { + t.Fatalf("expected Server.ClientTimeout to be omitted (0) for a synthesized default, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfigKerberos_GetServerConfig_PersistsExplicitTimeout proves +// the companion positive case: an explicitly configured timeout must still be +// serialized by CommandAuthConfigKerberos.GetServerConfig(). +func TestCommandAuthConfigKerberos_GetServerConfig_PersistsExplicitTimeout(t *testing.T) { + config := &auth_providers.CommandAuthConfigKerberos{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + config.WithClientTimeout(300) + + if err := config.CommandAuthConfig.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 300 { + t.Fatalf("expected Server.ClientTimeout to be 300, got %d", server.ClientTimeout) + } +} + +// TestCommandAuthConfigKerberos_PersistedDefaultConfigFile_DoesNotShadowEnvVar +// is the CommandAuthConfigKerberos analogue of +// TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar: a +// synthesized default persisted to a config file by a first run must not +// shadow KEYFACTOR_CLIENT_TIMEOUT on a second run that loads that file. +func TestCommandAuthConfigKerberos_PersistedDefaultConfigFile_DoesNotShadowEnvVar(t *testing.T) { + // Run 1: nothing explicitly configured for client timeout. + run1 := &auth_providers.CommandAuthConfigKerberos{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + } + if err := run1.CommandAuthConfig.ValidateAuthConfig(); err != nil { + t.Fatalf("run1: expected no error, got %v", err) + } + + persisted := run1.GetServerConfig() + + // Persist exactly what kfutil's login flow persists: the resolved Server + // config, verbatim, to the "default" profile of a config file. + dir := t.TempDir() + path := dir + "/command_config.json" + fileContents, mErr := json.Marshal( + map[string]interface{}{ + "servers": map[string]interface{}{ + "default": persisted, + }, + }, + ) + if mErr != nil { + t.Fatalf("failed to marshal persisted config: %v", mErr) + } + if err := os.WriteFile(path, fileContents, 0o600); err != nil { + t.Fatalf("failed to write persisted config file: %v", err) + } + + // Run 2: a fresh process loads that persisted file and has + // KEYFACTOR_CLIENT_TIMEOUT set in its environment. + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "1800") + + run2 := &auth_providers.CommandAuthConfigKerberos{} + run2.WithConfigFile(path).WithConfigProfile("default") + + if _, err := run2.CommandAuthConfig.LoadConfig(run2.ConfigProfile, run2.ConfigFilePath, true); err != nil { + t.Fatalf("run2: expected no error from LoadConfig, got %v", err) + } + + if err := run2.CommandAuthConfig.ValidateAuthConfig(); err != nil { + t.Fatalf("run2: expected no error from ValidateAuthConfig, got %v", err) + } + + if run2.HttpClientTimeout != 1800 { + t.Fatalf( + "expected KEYFACTOR_CLIENT_TIMEOUT=1800 to be honored, but a persisted synthesized default shadowed it: got HttpClientTimeout=%d", + run2.HttpClientTimeout, + ) + } +} + func TestCommandAuthConfigKerberos_GetHttpClient(t *testing.T) { // Skip test if TEST_KEYFACTOR_KRB_AUTH is not set if os.Getenv("TEST_KEYFACTOR_KRB_AUTH") != "1" && os.Getenv("TEST_KEYFACTOR_KRB_AUTH") != "true" { diff --git a/auth_providers/auth_oauth.go b/auth_providers/auth_oauth.go index 225c3fc..ec999e0 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -77,6 +77,104 @@ type oauth2Transport struct { src oauth2.TokenSource } +// oauthTokenFetchContext returns a context carrying an oauth2.HTTPClient +// value pointing at an *http.Client that wraps baseTransport with a bounded +// Timeout derived from httpClientTimeout, AND an overall deadline on the +// returned context itself (via context.WithTimeout) bounding the same +// duration. Every call site that hands a context to the golang.org/x/oauth2 +// machinery for a token fetch MUST use this helper instead of +// context.Background(): golang.org/x/oauth2/internal.ContextClient falls +// back to http.DefaultClient (Timeout: 0, unbounded) whenever the context +// carries no oauth2.HTTPClient value, and net/http.DefaultTransport sets no +// ResponseHeaderTimeout -- so a TCP connection that succeeds and then a +// hung/overloaded token endpoint simply never responds hangs the caller +// forever, regardless of HttpClientTimeout. +// +// The context-level deadline (not just the http.Client.Timeout field) is +// required because golang.org/x/oauth2/internal.RetrieveToken silently +// performs up to TWO sequential HTTP round trips for a single logical token +// fetch: on the first-ever call to a given tokenURL/clientID pair it doesn't +// yet know whether the server wants client credentials sent as +// AuthStyleInHeader or AuthStyleInParams, so it tries the first style and, +// if that attempt fails for ANY reason (including a timeout), immediately +// retries with the other style using the exact same ctx. http.Client.Do() +// re-derives its deadline as time.Now().Add(c.Timeout) fresh on every call, +// so relying on the *http.Client.Timeout field alone gives each of those two +// sequential attempts its own full httpClientTimeout budget -- silently +// doubling the observed worst-case wall-clock cost of a hard failure (a +// black-holed/unroutable token endpoint) to ~2x httpClientTimeout, per +// attempt further capped at DefaultDialTimeout during the dial phase +// specifically. (An httpClientTimeout of 15s measured as *exactly* 30s in +// the wild against such an endpoint -- 2x15 -- which happens to equal +// DefaultDialTimeout and is easy to misdiagnose as a dial-timeout bug; it +// is not, the 30s was coincidental.) A context-level deadline fixes this +// because it is an absolute point in time set once, shared by both +// sequential attempts: the first attempt consumes some (or all) of the +// budget, and http.Client.Do()'s own per-call deadline computation always +// defers to an earlier deadline already present on the request's context +// (see net/http's setRequestCancel/timeBeforeContextDeadline), so the +// second attempt is bounded by whatever budget is actually left -- zero, if +// the first attempt already exhausted it -- rather than getting a fresh +// full window. +// +// This context must NOT be cached and reused across multiple logical token +// fetches spread out over time (e.g. an oauth2 token source that refreshes +// hours after it was constructed): its deadline is relative to the moment +// this function is called, so a stale cached instance would eventually +// make every future refresh fail instantly with "context deadline +// exceeded" regardless of network conditions. Every call site must invoke +// this function fresh for each logical fetch and must call the returned +// CancelFunc once that fetch completes to release the timer promptly (see +// boundedClientCredentialsTokenSource for how GetHttpClient()'s +// long-lived, cached token source still gets a fresh context per actual +// refresh). +// +// Guards against httpClientTimeout <= 0 (ValidateAuthConfig should already +// guarantee a positive value by the time callers reach this point, but +// http.Client.Timeout: 0 means "no timeout," so an unguarded fallthrough here +// would silently reintroduce the exact same unbounded-wait hazard in a new +// place). +func oauthTokenFetchContext(baseTransport http.RoundTripper, httpClientTimeout int) (context.Context, context.CancelFunc) { + tokenFetchTimeoutSeconds := httpClientTimeout + if tokenFetchTimeoutSeconds <= 0 { + tokenFetchTimeoutSeconds = DefaultClientTimeout + } + timeout := time.Duration(tokenFetchTimeoutSeconds) * time.Second + + ctx, cancel := context.WithTimeout(context.Background(), timeout) + ctx = context.WithValue(ctx, oauth2.HTTPClient, &http.Client{Transport: baseTransport, Timeout: timeout}) + return ctx, cancel +} + +// boundedClientCredentialsTokenSource wraps a *clientcredentials.Config so +// that EVERY actual token fetch -- not just the first -- gets a freshly +// bounded context/http.Client pair from oauthTokenFetchContext, rather than +// the single ctx/http.Client that clientcredentials.Config.TokenSource would +// otherwise capture once (at GetHttpClient() call time) and reuse forever. +// +// This matters for two independent reasons: +// 1. oauthTokenFetchContext's returned context now carries an absolute +// deadline (see its doc comment) that must not be reused past the +// logical fetch it was created for, or every future token refresh would +// fail instantly once that original deadline has passed. +// 2. Building the context fresh on every actual refresh, rather than once +// up front, is also simply correct: it is oauth2.ReuseTokenSource (see +// GetHttpClient) that decides when a real network fetch is even +// necessary, by checking the cached token's validity first. This type +// is only ever asked for a new Token() when a real fetch is required. +type boundedClientCredentialsTokenSource struct { + config *clientcredentials.Config + baseTransport http.RoundTripper + httpClientTimeout int +} + +// Token performs a single, freshly-bounded client_credentials token fetch. +func (s *boundedClientCredentialsTokenSource) Token() (*oauth2.Token, error) { + ctx, cancel := oauthTokenFetchContext(s.baseTransport, s.httpClientTimeout) + defer cancel() + return s.config.Token(ctx) +} + // GetHttpClient returns the http client func (a *OAuthAuthenticator) GetHttpClient() (*http.Client, error) { return a.Client, nil @@ -226,13 +324,27 @@ func (b *CommandConfigOauth) GetHttpClient() (*http.Client, error) { b.Scopes = DefaultScopes } - ctx := context.WithValue(context.Background(), oauth2.HTTPClient, &http.Client{Transport: baseTransport}) - - // Lazily initialize the token source and cache it + // The client_credentials token fetch is NOT bounded by baseTransport's + // ResponseHeaderTimeout/TLSHandshakeTimeout in any useful way here, and + // must not rely on a single ctx/http.Client captured once and reused for + // every future token refresh: see boundedClientCredentialsTokenSource's + // and oauthTokenFetchContext's doc comments for why a fresh bounded + // context is built for every actual refresh instead, and why that + // context must carry its own deadline rather than relying solely on the + // wrapped http.Client's Timeout field. + // + // Lazily initialize the token source and cache it. oauth2.ReuseTokenSource + // caches the resulting token and only calls back into + // boundedClientCredentialsTokenSource.Token() when a real network fetch + // is actually required (initial fetch, or refresh after expiry). b.tsMu.Lock() if b.tokenSource == nil { log.Printf("[DEBUG] Initializing OAuth2 token source for client ID: %s", b.ClientID) - b.tokenSource = config.TokenSource(ctx) + b.tokenSource = oauth2.ReuseTokenSource(nil, &boundedClientCredentialsTokenSource{ + config: config, + baseTransport: baseTransport, + httpClientTimeout: b.HttpClientTimeout, + }) } tokenSource := b.tokenSource b.tsMu.Unlock() @@ -435,22 +547,21 @@ func (b *CommandConfigOauth) Authenticate() error { // GetServerConfig returns the server configuration for Keyfactor Command API using OAuth2. func (b *CommandConfigOauth) GetServerConfig() *Server { - server := Server{ - Host: b.CommandHostName, - Port: b.CommandPort, - ClientID: b.ClientID, - ClientSecret: b.ClientSecret, - AccessToken: b.AccessToken, - OAuthTokenUrl: b.TokenURL, - APIPath: b.CommandAPIPath, - Scopes: b.Scopes, - Audience: b.Audience, - //AuthProvider: AuthProvider{}, - SkipTLSVerify: b.SkipVerify, - CACertPath: b.CommandCACert, - AuthType: "oauth", - } - return &server + // Delegate to the embedded CommandAuthConfig for the fields it already + // knows how to populate correctly -- notably ClientTimeout, which must be + // omitted (not the ValidateAuthConfig-synthesized default) unless the + // caller explicitly configured it. See clientTimeoutDefaulted's doc + // comment on CommandAuthConfig for why persisting a synthesized default + // is harmful. Layer OAuth-specific fields on top. + server := b.CommandAuthConfig.GetServerConfig() + server.ClientID = b.ClientID + server.ClientSecret = b.ClientSecret + server.AccessToken = b.AccessToken + server.OAuthTokenUrl = b.TokenURL + server.Scopes = b.Scopes + server.Audience = b.Audience + server.AuthType = "oauth" + return server } // GetAccessToken returns the OAuth2 token source for the given configuration. @@ -490,13 +601,20 @@ func (b *CommandConfigOauth) GetAccessToken() (*oauth2.Token, error) { } } - ctx := context.Background() - log.Printf("[DEBUG] Returning call config.TokenSource() for client ID: %s", b.ClientID) - tokenSource := config.TokenSource(ctx) - if tokenSource == nil { - return nil, fmt.Errorf("failed to create token source for client ID: %s", b.ClientID) + // See oauthTokenFetchContext's doc comment: without this, config.Token + // below falls back to http.DefaultClient, which has no Timeout, so a + // TCP-connected-but-silent token endpoint would hang this call forever. + // This is a single one-shot fetch (no caching/reuse across calls like + // GetHttpClient()'s token source), so the bounded context's lifetime is + // scoped to just this call via defer cancel(). + baseTransport, tErr := b.BuildTransport() + if tErr != nil { + return nil, tErr } - token, tErr := tokenSource.Token() + ctx, cancel := oauthTokenFetchContext(baseTransport, b.HttpClientTimeout) + defer cancel() + log.Printf("[DEBUG] Fetching OAuth2 token for client ID: %s", b.ClientID) + token, tErr := config.Token(ctx) if tErr != nil { return nil, fmt.Errorf("failed to retrieve token for client ID %s: %w", b.ClientID, tErr) } diff --git a/auth_providers/auth_oauth_test.go b/auth_providers/auth_oauth_test.go index d8dc23f..4e1bde1 100644 --- a/auth_providers/auth_oauth_test.go +++ b/auth_providers/auth_oauth_test.go @@ -25,8 +25,10 @@ import ( "os" "path/filepath" "strings" + "sync" "sync/atomic" "testing" + "time" "github.com/Keyfactor/keyfactor-auth-client-go/auth_providers" ) @@ -69,6 +71,129 @@ func TestCommandConfigOauth_ValidateAuthConfig(t *testing.T) { } } +// TestCommandConfigOauth_GetServerConfig_DoesNotPersistSynthesizedDefault is +// the CommandConfigOauth analogue of +// TestCommandAuthConfig_GetServerConfig_DoesNotPersistSynthesizedDefault in +// auth_core_test.go. CommandConfigOauth defines its own GetServerConfig() +// that shadows the embedded CommandAuthConfig's method via Go's method +// resolution, so a fix landed only on the base type does not protect this -- +// or any other real caller-facing -- concrete type. CommandConfigOauth is +// what every real OAuth caller (kfutil, keyfactor-go-client, etc.) actually +// constructs. +// +// A value that was never explicitly configured (no struct field, no +// WithClientTimeout(), no env var, no file value) must not be serialized. +func TestCommandConfigOauth_GetServerConfig_DoesNotPersistSynthesizedDefault(t *testing.T) { + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + // A static access token lets ValidateAuthConfig succeed without a + // live client ID/secret/token URL, which is irrelevant to this bug. + AccessToken: "static-test-token", + } + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 0 { + t.Fatalf("expected Server.ClientTimeout to be omitted (0) for a synthesized default, got %d", server.ClientTimeout) + } +} + +// TestCommandConfigOauth_GetServerConfig_PersistsExplicitTimeout proves the +// companion positive case: an explicitly configured timeout must still be +// serialized by CommandConfigOauth.GetServerConfig(). +func TestCommandConfigOauth_GetServerConfig_PersistsExplicitTimeout(t *testing.T) { + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + AccessToken: "static-test-token", + } + config.WithClientTimeout(300) + + if err := config.ValidateAuthConfig(); err != nil { + t.Fatalf("expected no error, got %v", err) + } + + server := config.GetServerConfig() + if server.ClientTimeout != 300 { + t.Fatalf("expected Server.ClientTimeout to be 300, got %d", server.ClientTimeout) + } +} + +// TestCommandConfigOauth_PersistedDefaultConfigFile_DoesNotShadowEnvVar is the +// CommandConfigOauth analogue of +// TestCommandAuthConfig_PersistedDefaultConfigFile_DoesNotShadowEnvVar: a +// synthesized default persisted to a config file by a first run must not +// shadow KEYFACTOR_CLIENT_TIMEOUT on a second run that loads that file. +func TestCommandConfigOauth_PersistedDefaultConfigFile_DoesNotShadowEnvVar(t *testing.T) { + // Run 1: nothing explicitly configured for client timeout. ClientID/ + // ClientSecret/TokenURL (rather than a static AccessToken) are used here + // specifically because they round-trip through the persisted file's + // serverConfig fallback, letting run2's ValidateAuthConfig succeed + // without any env vars -- AccessToken deliberately has no such fallback + // in ValidateAuthConfig. + run1 := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + CommandPort: 443, + CommandAPIPath: "KeyfactorAPI", + }, + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + TokenURL: "https://test-host/oauth/token", + } + if err := run1.ValidateAuthConfig(); err != nil { + t.Fatalf("run1: expected no error, got %v", err) + } + + persisted := run1.GetServerConfig() + + // Persist exactly what kfutil's login flow persists: the resolved Server + // config, verbatim, to the "default" profile of a config file. + dir := t.TempDir() + path := dir + "/command_config.json" + fileContents, mErr := json.Marshal( + map[string]interface{}{ + "servers": map[string]interface{}{ + "default": persisted, + }, + }, + ) + if mErr != nil { + t.Fatalf("failed to marshal persisted config: %v", mErr) + } + if err := os.WriteFile(path, fileContents, 0o600); err != nil { + t.Fatalf("failed to write persisted config file: %v", err) + } + + // Run 2: a fresh process loads that persisted file and has + // KEYFACTOR_CLIENT_TIMEOUT set in its environment. + t.Setenv(auth_providers.EnvKeyfactorClientTimeout, "1800") + + run2 := &auth_providers.CommandConfigOauth{} + run2.WithConfigFile(path).WithConfigProfile("default") + + if err := run2.ValidateAuthConfig(); err != nil { + t.Fatalf("run2: expected no error from ValidateAuthConfig, got %v", err) + } + + if run2.HttpClientTimeout != 1800 { + t.Fatalf( + "expected KEYFACTOR_CLIENT_TIMEOUT=1800 to be honored, but a persisted synthesized default shadowed it: got HttpClientTimeout=%d", + run2.HttpClientTimeout, + ) + } +} + func TestCommandConfigOauth_GetHttpClient(t *testing.T) { // Skip test if TEST_KEYFACTOR_AD_AUTH is set to 1 or true if os.Getenv("TEST_KEYFACTOR_AD_AUTH") == "1" || os.Getenv("TEST_KEYFACTOR_AD_AUTH") == "true" { @@ -617,3 +742,333 @@ func TestCommandConfigOauth_TokenSourceIsReused(t *testing.T) { t.Errorf("expected token endpoint to be called once, got %d — token source is not being reused across GetHttpClient() calls", tokenRequestCount.Load()) } } + +// TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout is +// a regression test for the unbounded initial OAuth client_credentials +// token-fetch: GetHttpClient() built the ctx/http.Client pair injected into +// the oauth2 token source with Timeout left at the zero value (meaning "no +// timeout"). That client/ctx is captured once, lazily, inside the token +// source and is never subject to anything set on the outer client +// afterward (e.g. CommandAuthConfig.Authenticate's c.HttpClient.Timeout +// assignment only bounds the *outer* request). A hung token endpoint +// therefore blocked with no ceiling at all, regardless of HttpClientTimeout. +// +// This test isolates the token-fetch client's overall Timeout specifically +// (as opposed to baseTransport's pre-existing ResponseHeaderTimeout, which +// only bounds waiting for response *headers*, not a hang while streaming the +// body): the fake token endpoint sends response headers immediately -- so +// ResponseHeaderTimeout is satisfied -- and then hangs indefinitely without +// finishing the body. Only an http.Client.Timeout catches that. A short +// safety-net timer guarantees the test can't hang the suite even if this +// regresses further. +// +// Note: the oauth2 library probes both AuthStyleInHeader and +// AuthStyleInParams on the first-ever call to an unrecognized token +// endpoint (see golang.org/x/oauth2/internal.RetrieveToken). oauthTokenFetchContext +// now shares a single aggregate deadline across both attempts (see its doc +// comment and TestCommandConfigOauth_GetHttpClient_TokenFetchNotDoubledByAuthStyleProbe), +// so a hung endpoint is bounded by ~1x HttpClientTimeout in practice, not +// 2x -- the bound below is intentionally left loose (up to ~2x) since this +// test's purpose is confirming *some* real bound exists at all; the +// tighter ~1x guarantee has its own dedicated regression test. +func TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout(t *testing.T) { + releaseBody := make(chan struct{}) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(releaseBody) }) } + // Absolute safety net: even if the fix regresses, the handler -- and + // therefore this test -- cannot hang past this bound. + safetyNet := time.AfterFunc(6*time.Second, release) + defer safetyNet.Stop() + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + <-releaseBody + })) + // release() must run *before* tokenServer.Close(), which otherwise waits + // for the still-blocked handler goroutine -- deferring them separately + // (in either order) would make this test's own cleanup take as long as + // the safety net instead of finishing right after the assertions below. + defer func() { + release() + tokenServer.Close() + }() + + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + HttpClientTimeout: 1, // seconds -- deliberately short so the test runs fast + }, + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + TokenURL: tokenServer.URL, + } + + client, err := config.GetHttpClient() + if err != nil { + t.Fatalf("GetHttpClient() returned error: %v", err) + } + + start := time.Now() + resp, doErr := client.Get(tokenServer.URL) // triggers the token fetch before ever reaching the outer request + elapsed := time.Since(start) + if resp != nil { + resp.Body.Close() + } + + if doErr == nil { + t.Fatalf("expected the token fetch to fail once the body-read hang exceeds HttpClientTimeout (1s), got success after %v", elapsed) + } + // Comfortably above the worst-case ~2x HttpClientTimeout (2s, from the + // auth-style probe retry described above), comfortably below the 6s + // safety net -- so this only passes if HttpClientTimeout is actually + // what bounded the call. + if elapsed > 4*time.Second { + t.Fatalf("expected the token fetch to be bounded by ~2x HttpClientTimeout (~2s), took %v (err: %v)", elapsed, doErr) + } + t.Logf("token fetch failed after %v as expected: %v", elapsed, doErr) +} + +// TestCommandConfigOauth_GetAccessToken_TokenFetchBoundedByHttpClientTimeout +// is the GetAccessToken() analogue of +// TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout +// above. GetAccessToken() is a separate, independently-reachable entry point +// that built its own context.Background() for config.TokenSource() / +// tokenSource.Token() with no oauth2.HTTPClient value attached -- so it fell +// back to http.DefaultClient (Timeout: 0), the exact same unbounded-hang +// hazard GetHttpClient() had, just reachable through this sibling method +// instead. A hung token endpoint (one that accepts the connection, sends +// headers, and then never finishes the body) must not block this call +// forever. +// +// Same technique as the GetHttpClient() test: the fake token endpoint sends +// response headers immediately (so any ResponseHeaderTimeout on the +// transport alone is satisfied) and then hangs indefinitely on the body. +// Only an http.Client.Timeout derived from HttpClientTimeout catches that. A +// short safety-net timer guarantees the test can't hang the suite even if +// this regresses further. +func TestCommandConfigOauth_GetAccessToken_TokenFetchBoundedByHttpClientTimeout(t *testing.T) { + releaseBody := make(chan struct{}) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(releaseBody) }) } + // Absolute safety net: even if the fix regresses, the handler -- and + // therefore this test -- cannot hang past this bound. + safetyNet := time.AfterFunc(6*time.Second, release) + defer safetyNet.Stop() + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + <-releaseBody + })) + // release() must run *before* tokenServer.Close(), which otherwise waits + // for the still-blocked handler goroutine -- deferring them separately + // (in either order) would make this test's own cleanup take as long as + // the safety net instead of finishing right after the assertions below. + defer func() { + release() + tokenServer.Close() + }() + + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + HttpClientTimeout: 1, // seconds -- deliberately short so the test runs fast + }, + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + TokenURL: tokenServer.URL, + } + + start := time.Now() + token, err := config.GetAccessToken() + elapsed := time.Since(start) + + if err == nil { + t.Fatalf("expected GetAccessToken() to fail once the body-read hang exceeds HttpClientTimeout (1s), got token %+v after %v", token, elapsed) + } + // Comfortably above the worst-case bound (now ~1x HttpClientTimeout in + // practice -- see TestCommandConfigOauth_GetAccessToken_TokenFetchNotDoubledByAuthStyleProbe + // for the tight guarantee -- but left loose here at up to ~2x since this + // test's purpose is confirming *some* real bound exists at all), + // comfortably below the 6s safety net. + if elapsed > 4*time.Second { + t.Fatalf("expected GetAccessToken() to be bounded by ~2x HttpClientTimeout (~2s), took %v (err: %v)", elapsed, err) + } + t.Logf("GetAccessToken() failed after %v as expected: %v", elapsed, err) +} + +// TestCommandConfigOauth_GetHttpClient_TokenFetchNotDoubledByAuthStyleProbe +// is a regression test for a subtler variant of the unbounded-token-fetch +// hazard than the TokenFetchBoundedByHttpClientTimeout tests above catch. +// +// golang.org/x/oauth2/internal.RetrieveToken silently performs up to TWO +// sequential HTTP round trips for a single logical client_credentials token +// fetch whenever the AuthStyle for a given tokenURL/clientID pair hasn't +// been learned yet (see clientcredentials.Config.AuthStyle / +// AuthStyleUnknown): it tries AuthStyleInHeader first and, on ANY failure +// (including a timeout), immediately retries with AuthStyleInParams using +// the same context. Every call in this package builds a brand new +// clientcredentials.Config per logical fetch, so this always applies. +// +// oauthTokenFetchContext previously bounded the fetch only via an +// http.Client.Timeout field, which http.Client.Do() re-derives fresh +// (time.Now().Add(Timeout)) on every call -- so each of the two sequential +// attempts silently got its own full HttpClientTimeout budget, doubling the +// real-world worst-case cost of a hard failure (e.g. an unroutable token +// endpoint) to ~2x HttpClientTimeout. This went undetected because the two +// TokenFetchBoundedByHttpClientTimeout tests above intentionally tolerate up +// to ~2x as "not a regression" (see their comments) -- they were written to +// confirm SOME bound exists, not that the bound is tight, so they cannot +// distinguish "capped at 1x" from "capped at 2x." +// +// This test asserts the tight bound directly, using the request counter as +// the primary, deterministic signal: with the fix, the context passed to +// both AuthStyle attempts shares a single absolute deadline, so by the time +// the first attempt's hang exhausts that budget and RetrieveToken tries the +// second AuthStyle, the shared context is already past its deadline and the +// second attempt fails before ever reaching the network -- the token +// endpoint sees exactly one request, not two. Before the fix this test +// reliably measures exactly two requests and ~2x HttpClientTimeout elapsed +// (verified against the pre-fix commit). +func TestCommandConfigOauth_GetHttpClient_TokenFetchNotDoubledByAuthStyleProbe(t *testing.T) { + var attemptCount atomic.Int32 + releaseBody := make(chan struct{}) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(releaseBody) }) } + // Absolute safety net: even if the fix regresses further than the old + // ~2x behavior, the handler -- and therefore this test -- cannot hang + // indefinitely. + safetyNet := time.AfterFunc(10*time.Second, release) + defer safetyNet.Stop() + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attemptCount.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + <-releaseBody + })) + defer func() { + release() + tokenServer.Close() + }() + + const httpClientTimeoutSeconds = 2 + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + HttpClientTimeout: httpClientTimeoutSeconds, + }, + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + TokenURL: tokenServer.URL, + } + + client, err := config.GetHttpClient() + if err != nil { + t.Fatalf("GetHttpClient() returned error: %v", err) + } + + start := time.Now() + resp, doErr := client.Get(tokenServer.URL) + elapsed := time.Since(start) + if resp != nil { + resp.Body.Close() + } + + if doErr == nil { + t.Fatalf("expected the token fetch to fail once the body-read hang exceeds HttpClientTimeout (%ds), got success after %v", httpClientTimeoutSeconds, elapsed) + } + + if got := attemptCount.Load(); got != 1 { + t.Fatalf( + "expected exactly 1 request to the token endpoint (the second AuthStyle-probe attempt should fail against the already-exhausted shared deadline before ever reaching the network), got %d requests after %v -- this indicates the 2x-doubling bug has regressed", + got, elapsed, + ) + } + + // Secondary, looser confirmation: bounded by 1.5x rather than exactly 1x + // to tolerate real scheduling/IO overhead, while still failing hard if + // the aggregate reverts to ~2x HttpClientTimeout. + if maxAllowed := time.Duration(float64(httpClientTimeoutSeconds)*1.5) * time.Second; elapsed > maxAllowed { + t.Fatalf( + "expected the token fetch to be bounded by ~1x HttpClientTimeout (%ds), got %v", + httpClientTimeoutSeconds, elapsed, + ) + } + t.Logf("token fetch failed after %v with exactly 1 request to the token endpoint, as expected", elapsed) +} + +// TestCommandConfigOauth_GetAccessToken_TokenFetchNotDoubledByAuthStyleProbe +// is the GetAccessToken() analogue of +// TestCommandConfigOauth_GetHttpClient_TokenFetchNotDoubledByAuthStyleProbe +// above -- see its doc comment for the full mechanism. GetAccessToken() +// takes a separate code path (a fresh, uncached context/config built on +// every call, rather than GetHttpClient()'s cached token source) but was +// subject to the exact same ~2x-HttpClientTimeout doubling hazard before the +// fix, since it shares oauthTokenFetchContext. +func TestCommandConfigOauth_GetAccessToken_TokenFetchNotDoubledByAuthStyleProbe(t *testing.T) { + var attemptCount atomic.Int32 + releaseBody := make(chan struct{}) + var releaseOnce sync.Once + release := func() { releaseOnce.Do(func() { close(releaseBody) }) } + safetyNet := time.AfterFunc(10*time.Second, release) + defer safetyNet.Stop() + + tokenServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attemptCount.Add(1) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + if f, ok := w.(http.Flusher); ok { + f.Flush() + } + <-releaseBody + })) + defer func() { + release() + tokenServer.Close() + }() + + const httpClientTimeoutSeconds = 2 + config := &auth_providers.CommandConfigOauth{ + CommandAuthConfig: auth_providers.CommandAuthConfig{ + CommandHostName: "test-host", + HttpClientTimeout: httpClientTimeoutSeconds, + }, + ClientID: "test-client-id", + ClientSecret: "test-client-secret", + TokenURL: tokenServer.URL, + } + + start := time.Now() + token, err := config.GetAccessToken() + elapsed := time.Since(start) + + if err == nil { + t.Fatalf("expected GetAccessToken() to fail once the body-read hang exceeds HttpClientTimeout (%ds), got token %+v after %v", httpClientTimeoutSeconds, token, elapsed) + } + + if got := attemptCount.Load(); got != 1 { + t.Fatalf( + "expected exactly 1 request to the token endpoint (the second AuthStyle-probe attempt should fail against the already-exhausted shared deadline before ever reaching the network), got %d requests after %v -- this indicates the 2x-doubling bug has regressed", + got, elapsed, + ) + } + + if maxAllowed := time.Duration(float64(httpClientTimeoutSeconds)*1.5) * time.Second; elapsed > maxAllowed { + t.Fatalf( + "expected GetAccessToken() to be bounded by ~1x HttpClientTimeout (%ds), got %v (err: %v)", + httpClientTimeoutSeconds, elapsed, err, + ) + } + t.Logf("GetAccessToken() failed after %v with exactly 1 request to the token endpoint, as expected", elapsed) +} diff --git a/auth_providers/command_config.go b/auth_providers/command_config.go index e8a14a3..f5ebc05 100644 --- a/auth_providers/command_config.go +++ b/auth_providers/command_config.go @@ -40,6 +40,7 @@ type Server struct { SkipTLSVerify bool `json:"skip_tls_verify,omitempty" yaml:"skip_tls_verify,omitempty"` // TLSVerify determines whether to verify the TLS certificate. CACertPath string `json:"ca_cert_path,omitempty" yaml:"ca_cert_path,omitempty"` // CACertPath is the path to the CA certificate to trust. AuthType string `json:"auth_type,omitempty" yaml:"auth_type,omitempty"` // AuthType is the type of authentication to use. + ClientTimeout int `json:"client_timeout,omitempty" yaml:"client_timeout,omitempty"` // ClientTimeout is the http Client timeout, in seconds, mirrored from CommandAuthConfig.HttpClientTimeout. // Kerberos authentication fields KerberosRealm string `json:"kerberos_realm,omitempty" yaml:"kerberos_realm,omitempty"` // KerberosRealm is the Kerberos realm (uppercase). @@ -226,7 +227,8 @@ func (s *Server) Compare(other *Server) bool { s.KerberosKeytab == other.KerberosKeytab && s.KerberosConfig == other.KerberosConfig && s.KerberosCCache == other.KerberosCCache && - s.KerberosSPN == other.KerberosSPN + s.KerberosSPN == other.KerberosSPN && + s.ClientTimeout == other.ClientTimeout } // MergeConfigFromFile merges the configuration from a file into the existing Config. @@ -287,7 +289,8 @@ func (s *Server) GetBasicAuthClientConfig() (*CommandAuthConfigBasic, error) { WithCommandPort(s.Port). WithCommandAPIPath(s.APIPath). WithCommandCACert(s.CACertPath). - WithSkipVerify(s.SkipTLSVerify) + WithSkipVerify(s.SkipTLSVerify). + WithClientTimeout(s.ClientTimeout) basicConfig := CommandAuthConfigBasic{ CommandAuthConfig: baseConfig, @@ -317,7 +320,8 @@ func (s *Server) GetOAuthClientConfig() (*CommandConfigOauth, error) { WithCommandPort(s.Port). WithCommandAPIPath(s.APIPath). WithCommandCACert(s.CACertPath). - WithSkipVerify(s.SkipTLSVerify) + WithSkipVerify(s.SkipTLSVerify). + WithClientTimeout(s.ClientTimeout) oauthConfig := CommandConfigOauth{ CommandAuthConfig: baseConfig, @@ -350,7 +354,8 @@ func (s *Server) GetKerberosClientConfig() (*CommandAuthConfigKerberos, error) { WithCommandPort(s.Port). WithCommandAPIPath(s.APIPath). WithCommandCACert(s.CACertPath). - WithSkipVerify(s.SkipTLSVerify) + WithSkipVerify(s.SkipTLSVerify). + WithClientTimeout(s.ClientTimeout) kerberosConfig := CommandAuthConfigKerberos{ CommandAuthConfig: baseConfig, diff --git a/auth_providers/command_config_test.go b/auth_providers/command_config_test.go index af6c969..fb7a62b 100644 --- a/auth_providers/command_config_test.go +++ b/auth_providers/command_config_test.go @@ -332,6 +332,77 @@ func TestReadBasicAuthConfigExample(t *testing.T) { } } +// TestServer_ClientTimeout_BasicAuthRoundTrip is a regression test for +// https://github.com/Keyfactor/keyfactor-auth-client-go/issues/51: a +// Server.ClientTimeout value must be honored when the Server is converted back +// into a basic-auth CommandAuthConfigBasic, not silently dropped. +func TestServer_ClientTimeout_BasicAuthRoundTrip(t *testing.T) { + server := &auth_providers.Server{ + Host: "test-host", + Username: "user", + Password: "pass", + ClientTimeout: 300, + } + + config, err := server.GetBasicAuthClientConfig() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if config.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300, got %d", config.HttpClientTimeout) + } +} + +// TestServer_ClientTimeout_OAuthRoundTrip mirrors the basic-auth regression test +// above for the OAuth client config path. +func TestServer_ClientTimeout_OAuthRoundTrip(t *testing.T) { + server := &auth_providers.Server{ + Host: "test-host", + ClientID: "client-id", + ClientSecret: "client-secret", + OAuthTokenUrl: "https://idp.example.com/oauth2/token", + ClientTimeout: 300, + } + + config, err := server.GetOAuthClientConfig() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if config.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300, got %d", config.HttpClientTimeout) + } +} + +// TestServer_ClientTimeout_KerberosRoundTrip mirrors the basic-auth regression +// test above for the Kerberos client config path. +func TestServer_ClientTimeout_KerberosRoundTrip(t *testing.T) { + krb5Conf := "test_krb5.conf" + if err := os.WriteFile(krb5Conf, []byte("[libdefaults]\n"), 0644); err != nil { + t.Fatalf("failed to write temp krb5.conf: %v", err) + } + defer os.Remove(krb5Conf) + + server := &auth_providers.Server{ + Host: "test-host", + Username: "user", + Password: "pass", + KerberosRealm: "EXAMPLE.COM", + KerberosConfig: krb5Conf, + ClientTimeout: 300, + } + + config, err := server.GetKerberosClientConfig() + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + + if config.HttpClientTimeout != 300 { + t.Fatalf("expected HttpClientTimeout to be 300, got %d", config.HttpClientTimeout) + } +} + func compareConfigs(a, b *auth_providers.Config) bool { if len(a.Servers) != len(b.Servers) { return false