From 82090c5f48409eac6c93cce6fb1ae9460bd45de9 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:32:15 -0700 Subject: [PATCH 01/13] fix(core): preserve HttpClientTimeout in Server via ClientTimeout field (fixes #51) GetServerConfig() on CommandAuthConfig (and its basic/oauth/kerberos embedders) dropped the caller's HttpClientTimeout entirely: Server had no timeout field, so any WithClientTimeout() value set via CommandAuthConfig.WithClientTimeout was lost once the config was flattened to a Server for downstream consumers (e.g. keyfactor-go-client's NewKeyfactorClient, which rebuilds its own CommandAuthConfig from a Server). Consumers silently fell back to DefaultClientTimeout (60s), producing "net/http: timeout awaiting response headers" on long-running calls such as PFX enrollment even when a much larger timeout was explicitly configured upstream. Add Server.ClientTimeout (client_timeout json/yaml tag) and populate it from HttpClientTimeout in all four GetServerConfig() implementations (core, basic, oauth, kerberos). Also honor it in the reverse direction -- GetBasicAuthClientConfig/GetOAuthClientConfig/GetKerberosClientConfig now call WithClientTimeout(s.ClientTimeout) so a Server round-trips losslessly back into a CommandAuthConfig-derived config. --- auth_providers/auth_basic.go | 1 + auth_providers/auth_core.go | 1 + auth_providers/auth_core_test.go | 44 +++++++++++++++++ auth_providers/auth_kerberos.go | 1 + auth_providers/auth_oauth.go | 1 + auth_providers/command_config.go | 13 +++-- auth_providers/command_config_test.go | 71 +++++++++++++++++++++++++++ 7 files changed, 128 insertions(+), 4 deletions(-) diff --git a/auth_providers/auth_basic.go b/auth_providers/auth_basic.go index e36d427..7e6ee5a 100644 --- a/auth_providers/auth_basic.go +++ b/auth_providers/auth_basic.go @@ -255,6 +255,7 @@ func (a *CommandAuthConfigBasic) GetServerConfig() *Server { SkipTLSVerify: a.SkipVerify, CACertPath: a.CommandCACert, AuthType: "basic", + ClientTimeout: a.HttpClientTimeout, } return &server } diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index c2ca07d..04f8652 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -763,6 +763,7 @@ func (c *CommandAuthConfig) GetServerConfig() *Server { SkipTLSVerify: c.SkipVerify, CACertPath: c.CommandCACert, AuthType: "", + ClientTimeout: c.HttpClientTimeout, } return &server } diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 43eb765..82903d2 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -18,6 +18,7 @@ import ( "net/http" "strings" "testing" + "time" "github.com/Keyfactor/keyfactor-auth-client-go/auth_providers" ) @@ -63,6 +64,49 @@ 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) + } +} + func TestCommandAuthConfig_Authenticate(t *testing.T) { config := &auth_providers.CommandAuthConfig{ CommandHostName: "test-host", diff --git a/auth_providers/auth_kerberos.go b/auth_providers/auth_kerberos.go index 9a83f54..a034168 100644 --- a/auth_providers/auth_kerberos.go +++ b/auth_providers/auth_kerberos.go @@ -448,6 +448,7 @@ func (k *CommandAuthConfigKerberos) GetServerConfig() *Server { KerberosConfig: k.ConfigPath, KerberosCCache: k.CCachePath, KerberosSPN: k.SPN, + ClientTimeout: k.HttpClientTimeout, } return &server } diff --git a/auth_providers/auth_oauth.go b/auth_providers/auth_oauth.go index 225c3fc..5160432 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -449,6 +449,7 @@ func (b *CommandConfigOauth) GetServerConfig() *Server { SkipTLSVerify: b.SkipVerify, CACertPath: b.CommandCACert, AuthType: "oauth", + ClientTimeout: b.HttpClientTimeout, } return &server } 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 From 08a4cb4eb220e1bbd4292144c4b8333458dfd3b6 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 08:47:52 -0700 Subject: [PATCH 02/13] fix(core): honor config-file ClientTimeout and clamp bad env values to default LoadConfig merged Host/Port/APIPath/CACertPath/SkipVerify from a loaded Server into CommandAuthConfig but never ClientTimeout, and ValidateAuthConfig never consulted FileConfig as a fallback for HttpClientTimeout the way it already does for CommandHostName. A config-file-only client_timeout was silently dropped, landing on the 60s default instead. Separately, ValidateAuthConfig's env var fallback treated any LookupEnv ok=true (including an empty string, common when .env files pre-declare all KEYFACTOR_* vars) as authoritative, swallowing Atoi errors and skipping the default. An empty/unparseable/non-positive KEYFACTOR_CLIENT_TIMEOUT left HttpClientTimeout at its zero value, which disables ResponseHeaderTimeout/TLSHandshakeTimeout/ IdleConnTimeout/http.Client.Timeout entirely -- an unbounded-wait hazard since Authenticate() has no request context to otherwise bound the call. Now: explicit struct value/WithClientTimeout() wins outright; absent that, a config file value (merged eagerly in LoadConfig, consistent with the other Server fields, and consulted again in ValidateAuthConfig as a defensive fallback like CommandHostName) takes effect before the env var is ever checked; an unparseable or <=0 env var is logged and ignored rather than silently zeroing the timeout; and the package default applies only when nothing else resolved a positive value. Basic, Kerberos, and OAuth auth types all delegate to CommandAuthConfig.LoadConfig/ValidateAuthConfig, so no separate per-type fix was needed. --- auth_providers/auth_core.go | 29 ++++- auth_providers/auth_core_test.go | 215 +++++++++++++++++++++++++++++++ 2 files changed, 241 insertions(+), 3 deletions(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index 04f8652..a9d5c2d 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -284,11 +284,31 @@ 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 + } } } @@ -708,6 +728,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 diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 82903d2..e01de5a 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -15,7 +15,9 @@ package auth_providers_test import ( + "fmt" "net/http" + "os" "strings" "testing" "time" @@ -107,6 +109,219 @@ func TestCommandAuthConfig_ClientTimeout_BuildTransport(t *testing.T) { } } +// 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, + ) + } + }) + } +} + func TestCommandAuthConfig_Authenticate(t *testing.T) { config := &auth_providers.CommandAuthConfig{ CommandHostName: "test-host", From 96ef8298ed65ccb83cde0715955fc4a7b601374a Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:00:46 -0700 Subject: [PATCH 03/13] fix(auth): redact secrets from logged request bodies and stop scaling idle/handshake timeouts with HttpClientTimeout RequestToCurl appended the full, unredacted request body to the curl command it generates for TRACE logging (auth_oauth.go's oauth2Transport RoundTrip logs this on every OAuth-authenticated request, and the auth probe path does the same). Any secret-bearing request -- e.g. a PFX enrollment carrying a private-key password -- was therefore written to the log in plaintext whenever TRACE logging is enabled, which is exactly what support asks a customer to turn on when reporting the slow-request issue this timeout work exists to fix. RequestToCurl now parses JSON and form-encoded bodies and replaces known-sensitive field values (password, secret, token, and private-key variants, matched case-insensitively, nested objects/arrays included) with a placeholder while preserving the rest of the body for diagnostics. A body that can't be confidently classified as JSON or form-encoded is omitted entirely behind a "" marker rather than ever risking a raw secret leak. Separately, BuildTransport() and SetClient() 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, not a request deadline, so a large configured timeout (e.g. 1800s, needed for slow PFX enrollments) kept every idle socket -- and its goroutine -- alive for that same duration; a large `terraform apply` issuing many sequential requests could hold open hundreds of sockets/goroutines for half an hour. IdleConnTimeout, ExpectContinueTimeout, and TLSHandshakeTimeout are now pinned to fixed defaults matching net/http.DefaultTransport (90s/1s/10s) via a shared newHTTPTransport() constructor used by both BuildTransport and SetClient, while ResponseHeaderTimeout continues to track HttpClientTimeout as intended. --- auth_providers/auth_core.go | 235 ++++++++++++++++++++++++++----- auth_providers/auth_core_test.go | 219 ++++++++++++++++++++++++++++ 2 files changed, 421 insertions(+), 33 deletions(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index a9d5c2d..b390cb2 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -24,6 +24,7 @@ import ( "io" "log" "net/http" + "net/url" "os" "path/filepath" "strconv" @@ -87,6 +88,27 @@ 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 +) + // Authenticator is an interface for authentication to Keyfactor Command API. type Authenticator interface { GetHttpClient() (*http.Client, error) @@ -326,22 +348,55 @@ 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. +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, + TLSHandshakeTimeout: DefaultTLSHandshakeTimeout, + ResponseHeaderTimeout: time.Duration(c.HttpClientTimeout) * time.Second, + IdleConnTimeout: DefaultIdleConnTimeout, + ExpectContinueTimeout: DefaultExpectContinueTimeout, MaxIdleConns: 10, MaxIdleConnsPerHost: 10, MaxConnsPerHost: 10, } +} + +// 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 @@ -351,7 +406,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 { @@ -359,7 +414,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 { @@ -367,12 +422,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. @@ -385,27 +440,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(), } } @@ -817,6 +857,134 @@ 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. +var sensitiveBodyKeys = map[string]struct{}{ + "password": {}, + "pfxpassword": {}, + "keypassword": {}, + "entrypassword": {}, + "explicitpassword": {}, + "authcertificatepassword": {}, + "passphrase": {}, + "privatekey": {}, + "secret": {}, + "secretvalue": {}, + "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 +} + +// 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. +func redactJSONValue(v interface{}) 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] = redactJSONValue(vv) + } + return out + case []interface{}: + out := make([]interface{}, len(val)) + for i, vv := range val { + out[i] = redactJSONValue(vv) + } + return out + default: + return val + } +} + +// 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. It never returns +// the raw body verbatim unless every key it found was checked against +// sensitiveBodyKeys and none matched. 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. +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 @@ -877,7 +1045,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 e01de5a..35d99e5 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -109,6 +109,80 @@ func TestCommandAuthConfig_ClientTimeout_BuildTransport(t *testing.T) { } } +// 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 { @@ -434,3 +508,148 @@ 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\"`, + }, + }, + } + + 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) + } +} From bb3f0cf0aa33a51342b291fc9841a435ce8b518a Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:22:55 -0700 Subject: [PATCH 04/13] fix(transport): stop capping MaxConnsPerHost at 10 for cached clients newHTTPTransport() hardcoded MaxConnsPerHost: 10, which was harmless while every request built its own throwaway transport. Now that consumers cache and reuse a single *http.Client/*http.Transport (to fix a socket-leak bug), that cap becomes a hard, unqueued-timeout ceiling of 10 concurrent in-flight requests per host for the life of the process -- e.g. terraform apply -parallelism=25 silently serializes into batches of 10 with no bound on queue wait, since the client has Timeout: 0 and requests carry no context deadline. Set MaxConnsPerHost to 0 (unbounded, matching net/http.DefaultTransport) while leaving the idle-connection pool limits (MaxIdleConns/MaxIdleConnsPerHost) unchanged. --- auth_providers/auth_core.go | 15 ++++++++- auth_providers/auth_core_test.go | 55 ++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index b390cb2..d6b6512 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -390,7 +390,20 @@ func (c *CommandAuthConfig) newHTTPTransport() *http.Transport { 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, } } diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 35d99e5..e9ab819 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -653,3 +653,58 @@ func TestRequestToCurl_BodyRedaction(t *testing.T) { t.Logf("%s: curl command: %s", tt.name, 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) + } + }) +} From 155b8e04f0c4b5c20d3abbde50e84658229c8732 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:27:29 -0700 Subject: [PATCH 05/13] fix(core): stop persisting a synthesized default ClientTimeout that shadows the env var GetServerConfig() serialized the resolved HttpClientTimeout verbatim, including the 60s value ValidateAuthConfig synthesizes when nothing was configured. Callers that persist GetServerConfig()'s output to a config file (e.g. kfutil's login flow, which writes to ~/.keyfactor/command_config.json) therefore always wrote client_timeout: 60 to disk even when the user chose nothing. On the next run, LoadConfig merges that file value into HttpClientTimeout before ValidateAuthConfig 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 -- permanently and silently shadowing the env var. This is a regression: the env var always worked before Server gained a ClientTimeout field to persist. Track whether HttpClientTimeout's value was synthesized by the package-default fallback (new unexported clientTimeoutDefaulted field, cleared by WithClientTimeout) versus explicitly configured, and have GetServerConfig() omit ClientTimeout (via its existing omitempty tag) whenever it was only defaulted. Explicit values (struct/ WithClientTimeout(), env var, or an existing file value) are still persisted, and the round-1 precedence order is unchanged. --- auth_providers/auth_core.go | 32 +++++- auth_providers/auth_core_test.go | 167 +++++++++++++++++++++++++++++++ 2 files changed, 198 insertions(+), 1 deletion(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index d6b6512..6e634ed 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -172,6 +172,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. @@ -269,6 +282,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 } @@ -330,6 +347,10 @@ func (c *CommandAuthConfig) ValidateAuthConfig() error { 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 } } } @@ -839,7 +860,16 @@ func (c *CommandAuthConfig) GetServerConfig() *Server { SkipTLSVerify: c.SkipVerify, CACertPath: c.CommandCACert, AuthType: "", - ClientTimeout: c.HttpClientTimeout, + } + // 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 } diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index e9ab819..e8d4ea6 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -15,6 +15,7 @@ package auth_providers_test import ( + "encoding/json" "fmt" "net/http" "os" @@ -396,6 +397,172 @@ func TestCommandAuthConfig_ClientTimeout_BadEnvVarNeverDisablesTimeout(t *testin } } +// 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", From e263b6c14e8d1f54908149b71b94403caabf6692 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:39:15 -0700 Subject: [PATCH 06/13] fix(auth): redact secrets nested inside JSON-encoded string values The request-body redactor only inspected each JSON value's own key against sensitiveBodyKeys and never re-parsed string values that were themselves JSON documents, leaving two confirmed leak paths: - keyfactor-go-client v3 marshals a certificate store's Properties map into a JSON-encoded STRING field. terraform-provider-keyfactor puts ServerPassword in that map (and for K8S store types this field can carry an entire kubeconfig/service-account token), so it was emitted verbatim in generated curl commands. - PAM provider creation carries its secret under the generic key "Value", nested under ProviderTypeParamValues. "value" wasn't in sensitiveBodyKeys, so a Vault token/Delinea password was logged verbatim. redactJSONValue now recognizes string values that look like a JSON document (balanced outer brackets), re-parses and redacts them recursively, and re-serializes the result -- bounded by a depth limit (maxNestedJSONStringDepth) and size limit (maxNestedJSONStringLen) to bound the cost of adversarial nesting. A string that looks like JSON but fails to parse, or that hits either guard, is redacted in its entirety rather than ever emitted raw. sensitiveBodyKeys gains serverpassword, storepassword, newpassword, relaypassword, pkcs12blob, and value. "value" is blanket-redacted (rather than only within a credential-bearing parent) since this redactor walks structure without tracking its ancestry, and a parent-key allowlist would still miss future generic-"Value" secret fields; "properties" is deliberately NOT added, since blanket-hiding it would erase non-secret store configuration -- the nested-JSON-string handling above already redacts secrets within it while preserving the rest of its structure. --- auth_providers/auth_core.go | 144 +++++++++++++++++++++++++++--- auth_providers/auth_core_test.go | 146 +++++++++++++++++++++++++++++++ 2 files changed, 280 insertions(+), 10 deletions(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index 6e634ed..e72ac0e 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -911,6 +911,26 @@ const redactedPlaceholder = "***REDACTED***" // 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": {}, @@ -918,10 +938,16 @@ var sensitiveBodyKeys = map[string]struct{}{ "entrypassword": {}, "explicitpassword": {}, "authcertificatepassword": {}, + "newpassword": {}, + "serverpassword": {}, + "storepassword": {}, + "relaypassword": {}, "passphrase": {}, "privatekey": {}, + "pkcs12blob": {}, "secret": {}, "secretvalue": {}, + "value": {}, "clientsecret": {}, "client_secret": {}, "accesstoken": {}, @@ -939,12 +965,43 @@ func isSensitiveBodyKey(key string) bool { 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)) @@ -953,20 +1010,79 @@ func redactJSONValue(v interface{}) interface{} { out[k] = redactedPlaceholder continue } - out[k] = redactJSONValue(vv) + out[k] = redactJSONValueAtDepth(vv, nestedStringDepth) } return out case []interface{}: out := make([]interface{}, len(val)) for i, vv := range val { - out[i] = redactJSONValue(vv) + out[i] = redactJSONValueAtDepth(vv, nestedStringDepth) } return out + case string: + return redactNestedJSONString(val, nestedStringDepth) default: return val } } +// 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. +func looksLikeJSONDocument(s string) bool { + t := strings.TrimSpace(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{} + if err := json.Unmarshal([]byte(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 @@ -980,14 +1096,22 @@ func opaqueBodyMarker(contentType string, size int) string { } // redactRequestBody renders a safe, loggable representation of an HTTP -// request body for inclusion in a generated curl command. It never returns -// the raw body verbatim unless every key it found was checked against -// sensitiveBodyKeys and none matched. 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. +// 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 "" diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index e8d4ea6..7a4f0ef 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -790,6 +790,70 @@ func TestRequestToCurl_BodyRedaction(t *testing.T) { `\"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 { @@ -821,6 +885,88 @@ func TestRequestToCurl_BodyRedaction(t *testing.T) { } } +// 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) + }) +} + // 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 From 2f1d05e5f75c949922d0274423753be4d27a0c19 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:57:16 -0700 Subject: [PATCH 07/13] fix(basic-auth): gate persisted ClientTimeout on explicit configuration CommandAuthConfigBasic.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout. Since CommandAuthConfigBasic is what real basic-auth callers actually construct, the round-2 fix never took effect for them. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer basic-auth-specific fields on top. --- auth_providers/auth_basic.go | 29 +++---- auth_providers/auth_basic_test.go | 124 ++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+), 17 deletions(-) diff --git a/auth_providers/auth_basic.go b/auth_providers/auth_basic.go index 7e6ee5a..a923a61 100644 --- a/auth_providers/auth_basic.go +++ b/auth_providers/auth_basic.go @@ -241,23 +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", - ClientTimeout: a.HttpClientTimeout, - } - 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, From 27bd526ad07849947ad0726cf4a8653b83b3e656 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 09:58:50 -0700 Subject: [PATCH 08/13] fix(kerberos-auth): gate persisted ClientTimeout on explicit configuration CommandAuthConfigKerberos.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout, so the fix never took effect for real Kerberos callers. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer Kerberos-specific fields on top. --- auth_providers/auth_kerberos.go | 33 ++++---- auth_providers/auth_kerberos_test.go | 117 +++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 17 deletions(-) diff --git a/auth_providers/auth_kerberos.go b/auth_providers/auth_kerberos.go index a034168..add16f8 100644 --- a/auth_providers/auth_kerberos.go +++ b/auth_providers/auth_kerberos.go @@ -434,23 +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, - ClientTimeout: k.HttpClientTimeout, - } - 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" { From b2dc695d11bd386e7ee4c95dfb448ed49899f030 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:00:25 -0700 Subject: [PATCH 09/13] fix(oauth-auth): gate persisted ClientTimeout on explicit configuration CommandConfigOauth.GetServerConfig() shadows the embedded CommandAuthConfig method that round 2 fixed to skip persisting a ValidateAuthConfig-synthesized default ClientTimeout, so the fix never took effect for real OAuth callers. Delegate to the embedded GetServerConfig() for the correctly-gated ClientTimeout and layer OAuth-specific fields on top. --- auth_providers/auth_oauth.go | 32 ++++---- auth_providers/auth_oauth_test.go | 123 ++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+), 17 deletions(-) diff --git a/auth_providers/auth_oauth.go b/auth_providers/auth_oauth.go index 5160432..0792aaf 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -435,23 +435,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", - ClientTimeout: b.HttpClientTimeout, - } - 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. diff --git a/auth_providers/auth_oauth_test.go b/auth_providers/auth_oauth_test.go index d8dc23f..9f6a85f 100644 --- a/auth_providers/auth_oauth_test.go +++ b/auth_providers/auth_oauth_test.go @@ -69,6 +69,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" { From 343ba6612e839efd4d46f2a9d89fccb39e6cba06 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Fri, 14 Aug 2026 10:05:23 -0700 Subject: [PATCH 10/13] fix(auth): strip leading BOM before nested-JSON-in-string redaction check looksLikeJSONDocument only inspected the first/last byte after strings.TrimSpace to decide whether a nested string value looked like JSON worth re-parsing and redacting. TrimSpace does not strip a U+FEFF byte-order-mark, so a nested JSON-encoded string value prefixed with a BOM (e.g. a PAM/orchestrator service-account JSON key embedded in a Properties map value, plausible from a Windows-authored file) was judged "not JSON" and returned completely unredacted. encoding/json also rejects a leading BOM outright rather than tolerating it, so the fix strips the BOM explicitly before both the heuristic check and the actual json.Unmarshal call. --- auth_providers/auth_core.go | 34 +++++++++++++++++++++--- auth_providers/auth_core_test.go | 45 ++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index e72ac0e..36ead79 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -1026,14 +1026,35 @@ func redactJSONValueAtDepth(v interface{}, nestedStringDepth int) interface{} { } } +// 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. +// 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(s) + t := strings.TrimSpace(stripLeadingBOM(s)) if len(t) < 2 { return false } @@ -1068,7 +1089,14 @@ func redactNestedJSONString(s string, nestedStringDepth int) interface{} { } var parsed interface{} - if err := json.Unmarshal([]byte(s), &parsed); err != nil { + // 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. diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 7a4f0ef..13bf306 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -967,6 +967,51 @@ func TestRequestToCurl_BodyRedaction_NestedJSONInString(t *testing.T) { }) } +// 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 From 2ab3fc7d370a7e2d586d910821f0d3437727d348 Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:04:56 -0700 Subject: [PATCH 11/13] fix(auth): bound the OAuth token-fetch dial phase and overall timeout The initial client_credentials token fetch performed during Configure() was unbounded: CommandConfigOauth.GetHttpClient() injected an http.Client with Timeout left at its zero value into the oauth2 token source's context, and that ctx/client pair is captured once and reused forever, permanently divorced from HttpClientTimeout enforcement applied to the outer client elsewhere. Compounding this, newHTTPTransport() never set DialContext, so the TCP dial phase itself had no ceiling at all -- a black-holed destination (no RST/ICMP, just silence) hung indefinitely, regardless of any configured request_timeout. Bound both gaps: - Give the token-fetch client a real Timeout derived from HttpClientTimeout (falling back to DefaultClientTimeout if somehow unset), so the whole token-fetch call is bounded. - Add a fixed DefaultDialTimeout (30s, matching net/http.DefaultTransport) to newHTTPTransport()'s DialContext, so the dial phase specifically fails fast even when a large HttpClientTimeout is configured for slow request bodies elsewhere. --- auth_providers/auth_core.go | 24 +++++++++ auth_providers/auth_core_test.go | 58 +++++++++++++++++++++ auth_providers/auth_oauth.go | 22 +++++++- auth_providers/auth_oauth_test.go | 86 +++++++++++++++++++++++++++++++ 4 files changed, 189 insertions(+), 1 deletion(-) diff --git a/auth_providers/auth_core.go b/auth_providers/auth_core.go index 36ead79..380cc20 100644 --- a/auth_providers/auth_core.go +++ b/auth_providers/auth_core.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "log" + "net" "net/http" "net/url" "os" @@ -107,6 +108,19 @@ const ( // 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. @@ -399,12 +413,22 @@ func (c *CommandAuthConfig) ValidateAuthConfig() error { // 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, }, + DialContext: (&net.Dialer{Timeout: DefaultDialTimeout}).DialContext, TLSHandshakeTimeout: DefaultTLSHandshakeTimeout, ResponseHeaderTimeout: time.Duration(c.HttpClientTimeout) * time.Second, IdleConnTimeout: DefaultIdleConnTimeout, diff --git a/auth_providers/auth_core_test.go b/auth_providers/auth_core_test.go index 13bf306..a62ab6d 100644 --- a/auth_providers/auth_core_test.go +++ b/auth_providers/auth_core_test.go @@ -1066,3 +1066,61 @@ func TestCommandAuthConfig_MaxConnsPerHost_Unbounded(t *testing.T) { } }) } + +// 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_oauth.go b/auth_providers/auth_oauth.go index 0792aaf..296e6e0 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -226,7 +226,27 @@ func (b *CommandConfigOauth) GetHttpClient() (*http.Client, error) { b.Scopes = DefaultScopes } - ctx := context.WithValue(context.Background(), oauth2.HTTPClient, &http.Client{Transport: baseTransport}) + // The initial client_credentials token fetch performed lazily inside the + // token source below is NOT bounded by baseTransport's + // ResponseHeaderTimeout/TLSHandshakeTimeout in any useful way here: the + // oauth2 library captures this ctx/http.Client pair once and reuses it + // for every future token refresh, permanently divorced from anything set + // on the outer client later (e.g. CommandAuthConfig.Authenticate's + // c.HttpClient.Timeout assignment only bounds the *outer* request, never + // this token source's independently-cached context). Without an explicit + // Timeout here, a hung token endpoint (most notably a black-holed TCP + // dial with no RST/ICMP) blocks with no ceiling at all, regardless of + // HttpClientTimeout. Guard against HttpClientTimeout somehow still being + // <= 0 at this call site (it shouldn't be -- ValidateAuthConfig above + // already guarantees a positive value -- but Timeout: 0 means "no + // timeout" for http.Client, so an unguarded fallthrough here would + // silently reintroduce the same unbounded-wait hazard in a new place). + tokenFetchTimeoutSeconds := b.HttpClientTimeout + if tokenFetchTimeoutSeconds <= 0 { + tokenFetchTimeoutSeconds = DefaultClientTimeout + } + ctx := context.WithValue(context.Background(), oauth2.HTTPClient, + &http.Client{Transport: baseTransport, Timeout: time.Duration(tokenFetchTimeoutSeconds) * time.Second}) // Lazily initialize the token source and cache it b.tsMu.Lock() diff --git a/auth_providers/auth_oauth_test.go b/auth_providers/auth_oauth_test.go index 9f6a85f..435734e 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" ) @@ -740,3 +742,87 @@ 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), so a hung +// endpoint can cost up to ~2x HttpClientTimeout here, not 1x. The bound +// below accounts for that. +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{ + 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) +} From 14ce9fd5969e61d1c47869d596512fc275f2e0bc Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Mon, 17 Aug 2026 13:27:26 -0700 Subject: [PATCH 12/13] fix(auth): bound GetAccessToken's token-fetch call and fix test hostname omission GetAccessToken() built its own context.Background() for the oauth2 client_credentials token fetch, so a TCP-connected-but-unresponsive token endpoint hung the call forever -- the same unbounded-hang hazard this round's GetHttpClient() fix just closed, reachable through this sibling entry point instead. Extract oauthTokenFetchContext() as a shared helper for building a properly-bounded oauth2 context (transport + Timeout derived from HttpClientTimeout) and use it from both GetHttpClient() and GetAccessToken(), rather than duplicating the construction. Also fix TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout, which omitted CommandHostName and therefore only exercised the intended hung-body assertion by accident of an ambient KEYFACTOR_HOSTNAME left set in the dev/review shell -- in a clean environment it failed at the hostname-validation gate before ever reaching the fake token server. --- auth_providers/auth_oauth.go | 56 +++++++++++++++++------- auth_providers/auth_oauth_test.go | 72 +++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 15 deletions(-) diff --git a/auth_providers/auth_oauth.go b/auth_providers/auth_oauth.go index 296e6e0..05fab49 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -77,6 +77,34 @@ 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. Every call site that hands a +// context to the golang.org/x/oauth2 machinery for a token fetch (whether via +// clientcredentials.Config.TokenSource, which caches this ctx/http.Client +// pair for every future refresh, or a direct one-shot +// tokenSource.Token() call) 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. +// +// 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 { + tokenFetchTimeoutSeconds := httpClientTimeout + if tokenFetchTimeoutSeconds <= 0 { + tokenFetchTimeoutSeconds = DefaultClientTimeout + } + return context.WithValue(context.Background(), oauth2.HTTPClient, + &http.Client{Transport: baseTransport, Timeout: time.Duration(tokenFetchTimeoutSeconds) * time.Second}) +} + // GetHttpClient returns the http client func (a *OAuthAuthenticator) GetHttpClient() (*http.Client, error) { return a.Client, nil @@ -233,20 +261,10 @@ func (b *CommandConfigOauth) GetHttpClient() (*http.Client, error) { // for every future token refresh, permanently divorced from anything set // on the outer client later (e.g. CommandAuthConfig.Authenticate's // c.HttpClient.Timeout assignment only bounds the *outer* request, never - // this token source's independently-cached context). Without an explicit - // Timeout here, a hung token endpoint (most notably a black-holed TCP - // dial with no RST/ICMP) blocks with no ceiling at all, regardless of - // HttpClientTimeout. Guard against HttpClientTimeout somehow still being - // <= 0 at this call site (it shouldn't be -- ValidateAuthConfig above - // already guarantees a positive value -- but Timeout: 0 means "no - // timeout" for http.Client, so an unguarded fallthrough here would - // silently reintroduce the same unbounded-wait hazard in a new place). - tokenFetchTimeoutSeconds := b.HttpClientTimeout - if tokenFetchTimeoutSeconds <= 0 { - tokenFetchTimeoutSeconds = DefaultClientTimeout - } - ctx := context.WithValue(context.Background(), oauth2.HTTPClient, - &http.Client{Transport: baseTransport, Timeout: time.Duration(tokenFetchTimeoutSeconds) * time.Second}) + // this token source's independently-cached context). See + // oauthTokenFetchContext's doc comment for why an explicit Timeout is + // required here. + ctx := oauthTokenFetchContext(baseTransport, b.HttpClientTimeout) // Lazily initialize the token source and cache it b.tsMu.Lock() @@ -509,7 +527,15 @@ func (b *CommandConfigOauth) GetAccessToken() (*oauth2.Token, error) { } } - ctx := context.Background() + // See oauthTokenFetchContext's doc comment: without this, config.TokenSource + // below (and the eventual tokenSource.Token() call) falls back to + // http.DefaultClient, which has no Timeout, so a TCP-connected-but-silent + // token endpoint would hang this call forever. + baseTransport, tErr := b.BuildTransport() + if tErr != nil { + return nil, tErr + } + ctx := oauthTokenFetchContext(baseTransport, b.HttpClientTimeout) log.Printf("[DEBUG] Returning call config.TokenSource() for client ID: %s", b.ClientID) tokenSource := config.TokenSource(ctx) if tokenSource == nil { diff --git a/auth_providers/auth_oauth_test.go b/auth_providers/auth_oauth_test.go index 435734e..9eef441 100644 --- a/auth_providers/auth_oauth_test.go +++ b/auth_providers/auth_oauth_test.go @@ -795,6 +795,7 @@ func TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout(t 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", @@ -826,3 +827,74 @@ func TestCommandConfigOauth_GetHttpClient_TokenFetchBoundedByHttpClientTimeout(t } 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 ~2x HttpClientTimeout (2s, matching the + // auth-style probe retry noted in the GetHttpClient() test 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 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) +} From a6197350ecf6b42bfbdc8f07607aa183c0fc93cc Mon Sep 17 00:00:00 2001 From: spbsoluble <1661003+spbsoluble@users.noreply.github.com> Date: Mon, 17 Aug 2026 14:53:00 -0700 Subject: [PATCH 13/13] fix(auth): bound aggregate OAuth token-fetch time, not just per-attempt oauthTokenFetchContext previously bounded a token fetch only via an http.Client.Timeout field, which http.Client.Do() re-derives fresh on every call. golang.org/x/oauth2/internal.RetrieveToken silently makes up to two sequential HTTP round trips per logical client_credentials fetch (it probes AuthStyleInHeader, then retries with AuthStyleInParams on any failure) using the same context, so each attempt got its own full HttpClientTimeout budget -- doubling the real-world worst-case cost of a hard failure to ~2x HttpClientTimeout. An HttpClientTimeout of 15s against an unroutable endpoint measured as exactly 30s, which coincidentally equals DefaultDialTimeout and looked like a dial-timeout bug but wasn't. oauthTokenFetchContext now derives its context via context.WithTimeout so both sequential attempts share one absolute deadline: once the first attempt exhausts the budget, the second fails immediately rather than getting a fresh window. Since that context must not be reused across future token refreshes (its deadline is relative to creation time), GetHttpClient()'s cached, long-lived token source is refactored to build a fresh context on every actual refresh via a new boundedClientCredentialsTokenSource wrapped in oauth2.ReuseTokenSource, rather than capturing one context/http.Client pair once and reusing it forever. Verified against a black-holed destination that the aggregate now matches the configured value across 5s/25s/45s (bracketing DefaultDialTimeout=30s), instead of 2x or a fixed 30s. --- auth_providers/auth_oauth.go | 139 ++++++++++++++++------ auth_providers/auth_oauth_test.go | 188 ++++++++++++++++++++++++++++-- 2 files changed, 287 insertions(+), 40 deletions(-) diff --git a/auth_providers/auth_oauth.go b/auth_providers/auth_oauth.go index 05fab49..ec999e0 100644 --- a/auth_providers/auth_oauth.go +++ b/auth_providers/auth_oauth.go @@ -79,11 +79,10 @@ type oauth2Transport struct { // oauthTokenFetchContext returns a context carrying an oauth2.HTTPClient // value pointing at an *http.Client that wraps baseTransport with a bounded -// Timeout derived from httpClientTimeout. Every call site that hands a -// context to the golang.org/x/oauth2 machinery for a token fetch (whether via -// clientcredentials.Config.TokenSource, which caches this ctx/http.Client -// pair for every future refresh, or a direct one-shot -// tokenSource.Token() call) MUST use this helper instead of +// 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 @@ -91,18 +90,89 @@ type oauth2Transport struct { // 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 { +func oauthTokenFetchContext(baseTransport http.RoundTripper, httpClientTimeout int) (context.Context, context.CancelFunc) { tokenFetchTimeoutSeconds := httpClientTimeout if tokenFetchTimeoutSeconds <= 0 { tokenFetchTimeoutSeconds = DefaultClientTimeout } - return context.WithValue(context.Background(), oauth2.HTTPClient, - &http.Client{Transport: baseTransport, Timeout: time.Duration(tokenFetchTimeoutSeconds) * time.Second}) + 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 @@ -254,23 +324,27 @@ func (b *CommandConfigOauth) GetHttpClient() (*http.Client, error) { b.Scopes = DefaultScopes } - // The initial client_credentials token fetch performed lazily inside the - // token source below is NOT bounded by baseTransport's - // ResponseHeaderTimeout/TLSHandshakeTimeout in any useful way here: the - // oauth2 library captures this ctx/http.Client pair once and reuses it - // for every future token refresh, permanently divorced from anything set - // on the outer client later (e.g. CommandAuthConfig.Authenticate's - // c.HttpClient.Timeout assignment only bounds the *outer* request, never - // this token source's independently-cached context). See - // oauthTokenFetchContext's doc comment for why an explicit Timeout is - // required here. - ctx := oauthTokenFetchContext(baseTransport, b.HttpClientTimeout) - - // 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() @@ -527,21 +601,20 @@ func (b *CommandConfigOauth) GetAccessToken() (*oauth2.Token, error) { } } - // See oauthTokenFetchContext's doc comment: without this, config.TokenSource - // below (and the eventual tokenSource.Token() call) falls back to - // http.DefaultClient, which has no Timeout, so a TCP-connected-but-silent - // token endpoint would hang this call forever. + // 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 } - ctx := oauthTokenFetchContext(baseTransport, b.HttpClientTimeout) - 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) - } - 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 9eef441..4e1bde1 100644 --- a/auth_providers/auth_oauth_test.go +++ b/auth_providers/auth_oauth_test.go @@ -764,9 +764,13 @@ func TestCommandConfigOauth_TokenSourceIsReused(t *testing.T) { // // 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), so a hung -// endpoint can cost up to ~2x HttpClientTimeout here, not 1x. The bound -// below accounts for that. +// 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 @@ -889,12 +893,182 @@ func TestCommandConfigOauth_GetAccessToken_TokenFetchBoundedByHttpClientTimeout( 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 ~2x HttpClientTimeout (2s, matching the - // auth-style probe retry noted in the GetHttpClient() test above), - // comfortably below the 6s safety net -- so this only passes if - // HttpClientTimeout is actually what bounded the call. + // 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) +}