From 5d22038ad05ceb4a09d0213521bcc7b58b280b0c Mon Sep 17 00:00:00 2001 From: James Nesbitt Date: Tue, 10 Nov 2026 11:36:06 +0200 Subject: [PATCH 1/6] fix(log): inject the logger into a Config pointer, not a copy NewConnection passes the caller's logger down to the protocol Config with options.InjectLoggerTo(cfg, log.KeyProtocol, "winrm-config") where cfg is a Config value. log.InjectLogger asserts its argument to the unexported injectable interface, whose SetLogger has a pointer receiver, so a value's method set does not satisfy it. The assertion fails, the function takes its else branch, and the injection silently does nothing. The same line exists in both winrm and ssh. Evidence it is a mistake rather than a deliberate no-op: - The "winrm-config" and "ssh-config" attributes those two calls pass are unreachable today. Nothing can ever log under them. - log.InjectLogger's fallback emits "logger is not injectable" to the trace logger. That is a diagnostic for a caller error, not a supported path. - ssh/connection.go performs the same injection on bastionCfg, which is a *Config, and works. The broken and working sites differ in exactly one thing: pointer versus value. - No test asserted the current behaviour. There is no observable symptom at present, because no Config method logs anything. It is fixed because the next one would inherit the silence: a Config that is handed a logger and quietly drops it is a trap for whoever adds the first config-level log line. Injecting before the Config is copied into the Connection means the connection's embedded Config carries the logger too. Tested by asserting the wiring directly, since no Config method logs. Both tests were run against the unfixed line and fail there: logger_test.go:26: the connection's Config has no logger, want the one passed to WithLogger Written by AI: claude-opus-5 Signed-off-by: James Nesbitt --- protocol/ssh/connection.go | 2 +- protocol/ssh/logger_test.go | 31 +++++++++++++++++++++++++++++++ protocol/winrm/connection.go | 2 +- protocol/winrm/logger_test.go | 32 ++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 protocol/ssh/logger_test.go create mode 100644 protocol/winrm/logger_test.go diff --git a/protocol/ssh/connection.go b/protocol/ssh/connection.go index 0bfb1c70..69fc9c36 100644 --- a/protocol/ssh/connection.go +++ b/protocol/ssh/connection.go @@ -218,7 +218,7 @@ func (c *Connection) configureKeepalive(options *Options) { // returns has not yet contacted the host. func NewConnection(cfg Config, opts ...Option) (*Connection, error) { options := NewOptions(opts...) - options.InjectLoggerTo(cfg, log.KeyProtocol, "ssh-config") + options.InjectLoggerTo(&cfg, log.KeyProtocol, "ssh-config") if options.IgnoreSSHConfig { cfg.IgnoreSSHConfig = true } diff --git a/protocol/ssh/logger_test.go b/protocol/ssh/logger_test.go new file mode 100644 index 00000000..7b073d3b --- /dev/null +++ b/protocol/ssh/logger_test.go @@ -0,0 +1,31 @@ +package ssh + +import ( + "testing" + + "github.com/k0sproject/rig/v2/log" + "github.com/k0sproject/rig/v2/rigtest" +) + +// The injection is only observable through a Config method that logs, and none +// does; this asserts the wiring directly instead. Passing a Config value to +// InjectLoggerTo silently does nothing, because log's injectable interface +// needs the pointer-receiver SetLogger. +func TestNewConnectionInjectsLoggerIntoConfig(t *testing.T) { + logger := &rigtest.MockLogger{} + conn, err := NewConnection(Config{ + Address: "10.0.0.1", + User: "root", + IgnoreSSHConfig: true, + }, WithLogger(logger)) + if err != nil { + t.Fatalf("NewConnection() error = %v", err) + } + + if !log.HasLogger(&conn.Config) { + t.Error("the connection's Config has no logger, want the one passed to WithLogger") + } + if !log.HasLogger(conn) { + t.Error("the connection has no logger, want the one passed to WithLogger") + } +} diff --git a/protocol/winrm/connection.go b/protocol/winrm/connection.go index b8ad0c99..98b7d0c4 100644 --- a/protocol/winrm/connection.go +++ b/protocol/winrm/connection.go @@ -48,7 +48,7 @@ type dialFunc func(network, addr string) (net.Conn, error) // NewConnection creates a new WinRM connection. Error is currently always nil. func NewConnection(cfg Config, opts ...Option) (*Connection, error) { options := NewOptions(opts...) - options.InjectLoggerTo(cfg, log.KeyProtocol, "winrm-config") + options.InjectLoggerTo(&cfg, log.KeyProtocol, "winrm-config") cfg.SetDefaults() c := &Connection{Config: cfg} diff --git a/protocol/winrm/logger_test.go b/protocol/winrm/logger_test.go new file mode 100644 index 00000000..1c077d3c --- /dev/null +++ b/protocol/winrm/logger_test.go @@ -0,0 +1,32 @@ +package winrm + +import ( + "testing" + + "github.com/k0sproject/rig/v2/log" + "github.com/k0sproject/rig/v2/rigtest" +) + +// The injection is only observable through a Config method that logs, and +// none does; this asserts the wiring directly instead. Passing a Config value +// to InjectLoggerTo silently does nothing, because log's injectable interface +// needs the pointer-receiver SetLogger. +func TestNewConnectionInjectsLoggerIntoConfig(t *testing.T) { + logger := &rigtest.MockLogger{} + conn, err := NewConnection(Config{ + Address: "10.0.0.1", + User: "Administrator", + Password: "hunter2", + UseHTTPS: true, + }, WithLogger(logger)) + if err != nil { + t.Fatalf("NewConnection() error = %v", err) + } + + if !log.HasLogger(&conn.Config) { + t.Error("the connection's Config has no logger, want the one passed to WithLogger") + } + if !log.HasLogger(conn) { + t.Error("the connection has no logger, want the one passed to WithLogger") + } +} From 8ede149fa7a00076321de3934dbc57e0daacadc4 Mon Sep 17 00:00:00 2001 From: James Nesbitt Date: Tue, 10 Nov 2026 11:37:12 +0200 Subject: [PATCH 2/6] feat(winrm)!: refuse to send a password over plain HTTP Widens insecure from "accept invalid TLS certificates" to "do not refuse to expose the credential", and rejects a password on a transport that does not protect it. Both senses describe the same caller: one who has decided the transport need not protect what it carries. password + plain HTTP -> ErrValidationFailed password + plain HTTP + insecure -> allowed password + HTTPS -> allowed password + HTTPS + insecure -> allowed certPath/keyPath, no password -> allowed NTLM gets no exemption. An earlier revision of this branch excused it on the grounds that it encrypts at the message level; the WinRM client authenticates with NTLM but sends the SOAP envelopes unencrypted, so the commands, their output, and any credential inside them stay readable. A remedy that leaves the session in the clear is not a remedy. Certificate authentication is unaffected. It is only selected over HTTPS (connection.go only attaches the client certificate when UseHTTPS is set), which this never rejects, so a cert-authenticated config cannot trip the rule even when it also carries a password. This replaces the advisory warning the branch previously carried. A warning on a connection the caller is about to make anyway communicates the problem at the moment it is too late to act on it, and costs a sync.Once, a Connect-time hook and a function returning a message string to say so. The check belongs with the other auth-shape rules in validateAuth, which Validate already calls and CompositeConfig reaches for every protocol config. Breaking: an existing config with a password, no useHTTPS and no insecure now fails validation instead of connecting. The message names insecure as the one-line way to keep the old behaviour. Written by AI: claude-opus-5 Signed-off-by: James Nesbitt --- protocol/winrm/config.go | 20 +++++++++- protocol/winrm/config_test.go | 70 +++++++++++++++++++++++++++++++++++ 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 protocol/winrm/config_test.go diff --git a/protocol/winrm/config.go b/protocol/winrm/config.go index 0e9ce6fe..0fbb0d7f 100644 --- a/protocol/winrm/config.go +++ b/protocol/winrm/config.go @@ -55,7 +55,22 @@ func (c *Config) SetDefaults() { } } -// validateAuth checks that a complete authentication method is configured. +// validateAuth checks that a complete authentication method is configured, and +// that the configured transport does not hand the password to a third party. +// +// Insecure is read as "do not refuse to expose the credential", widening it +// from "accept invalid TLS certificates": both senses describe a caller who +// has decided the transport does not have to protect what it carries. +// +// Plain HTTP is refused because the default transport authenticates with HTTP +// basic, putting the password on the wire base64-encoded. NTLM does not earn +// an exemption: the WinRM client authenticates with it but sends the SOAP +// envelopes unencrypted, so the commands, their output and any credential +// inside them stay readable. +// +// TLS with Insecure is permitted by the same rule that permits plain HTTP +// with it -- the caller asked for it explicitly. Certificate authentication +// is unaffected: it is only selected over HTTPS, which this never rejects. func (c *Config) validateAuth() error { if c.User == "" && c.Password == "" && c.CertPath == "" && c.KeyPath == "" { return fmt.Errorf("%w: no authentication method set (provide user+password or certPath+keyPath)", protocol.ErrValidationFailed) @@ -66,6 +81,9 @@ func (c *Config) validateAuth() error { if (c.User == "") != (c.Password == "") && c.CertPath == "" { return fmt.Errorf("%w: user and password must both be set for password authentication", protocol.ErrValidationFailed) } + if c.Password != "" && !c.UseHTTPS && !c.Insecure { + return fmt.Errorf("%w: a password would be sent over plain HTTP, where any observer recovers it; set useHTTPS, or insecure to send it anyway", protocol.ErrValidationFailed) + } return nil } diff --git a/protocol/winrm/config_test.go b/protocol/winrm/config_test.go new file mode 100644 index 00000000..251c4328 --- /dev/null +++ b/protocol/winrm/config_test.go @@ -0,0 +1,70 @@ +package winrm + +import ( + "errors" + "strings" + "testing" + + "github.com/k0sproject/rig/v2/protocol" +) + +// A password on a transport that does not protect it is refused unless the +// caller set insecure. Certificate authentication is only selected over +// HTTPS, so it is never reached by the rule. +func TestValidateRejectsExposedPassword(t *testing.T) { + for _, tc := range []struct { + name string + cfg Config + reject bool + }{ + { + name: "password over plain HTTP", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2"}, + reject: true, + }, + { + name: "NTLM does not protect the body", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseNTLM: true}, + reject: true, + }, + { + name: "plain HTTP accepted once insecure is set", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", Insecure: true}, + }, + { + name: "HTTPS", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseHTTPS: true}, + }, + { + name: "HTTPS without certificate verification", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseHTTPS: true, Insecure: true}, + }, + { + name: "certificate authentication carries no password", + cfg: Config{Address: "10.0.0.1", CertPath: "/dev/null", KeyPath: "/dev/null"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + cfg := tc.cfg + cfg.SetDefaults() + err := cfg.Validate() + + if !tc.reject { + if err != nil { + t.Fatalf("Validate() = %v, want nil", err) + } + return + } + + if err == nil { + t.Fatal("Validate() = nil, want a validation error") + } + if !errors.Is(err, protocol.ErrValidationFailed) { + t.Errorf("Validate() = %v, want it to wrap protocol.ErrValidationFailed", err) + } + if strings.Contains(err.Error(), cfg.Password) { + t.Error("the error repeats the password") + } + }) + } +} From 023cfbc2e3c7eb2c74363ef336bfd7d209fb5759 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Wed, 11 Nov 2026 13:31:07 +0200 Subject: [PATCH 3/6] fix(winrm): enforce the plain-HTTP password rule on the connection path Validate is optional for callers and nothing in the connection path calls it, so the rule only reached consumers who validated first. NewConnection now applies the same check after SetDefaults, so rig.NewClient(WithConnectionFactory(...)) and direct NewConnection callers are refused before any request is sent. The error also wraps ErrNonRetryable, since no retry can change the configuration. Config.Connection returned a typed-nil *Connection inside the interface on error; it now returns a plain nil. A client certificate is only presented over TLS, so certPath/keyPath without useHTTPS is now a validation error instead of being silently ignored. The insecure field's schema description now states both effects, and the schemas are regenerated. Signed-off-by: Kimmo Lehto --- client_test.go | 34 +++++++++++++++++++ protocol/winrm/config.go | 45 +++++++++++++++---------- protocol/winrm/config_test.go | 56 ++++++++++++++++++++++++++++--- protocol/winrm/connection.go | 8 ++++- protocol/winrm/connection_test.go | 1 + schemas/winrm.json | 2 +- schemas/winrm.yaml | 2 +- 7 files changed, 122 insertions(+), 26 deletions(-) diff --git a/client_test.go b/client_test.go index 3b115a02..dbe58030 100644 --- a/client_test.go +++ b/client_test.go @@ -4,8 +4,13 @@ import ( "context" "errors" "io" + "net/http" + "net/http/httptest" + "net/url" + "strconv" "strings" "sync" + "sync/atomic" "testing" "github.com/k0sproject/rig/v2" @@ -13,6 +18,7 @@ import ( "github.com/k0sproject/rig/v2/os" "github.com/k0sproject/rig/v2/packagemanager" "github.com/k0sproject/rig/v2/protocol" + "github.com/k0sproject/rig/v2/protocol/winrm" "github.com/k0sproject/rig/v2/remotefs" "github.com/k0sproject/rig/v2/rigtest" "github.com/stretchr/testify/assert" @@ -37,6 +43,34 @@ func TestClientWithConnectionFactory(t *testing.T) { require.Equal(t, "hello", out) } +func TestClientWithConnectionFactoryRefusesWinRMPasswordOverHTTP(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + + u, err := url.Parse(srv.URL) + require.NoError(t, err) + port, err := strconv.Atoi(u.Port()) + require.NoError(t, err) + + cc := &rig.CompositeConfig{ + WinRM: &winrm.Config{ + Address: u.Hostname(), + Port: port, + User: "Administrator", + Password: "hunter2", + }, + } + client, err := rig.NewClient(rig.WithConnectionFactory(cc)) + require.ErrorIs(t, err, protocol.ErrValidationFailed) + require.Nil(t, client) + require.NotContains(t, err.Error(), "hunter2") + require.Zero(t, requests.Load(), "a rejected configuration must not reach the host") +} + func TestClient(t *testing.T) { conn := rigtest.NewMockConnection() conn.AddCommandOutput(rigtest.Match("echo hello"), "hello") diff --git a/protocol/winrm/config.go b/protocol/winrm/config.go index 0fbb0d7f..da4be712 100644 --- a/protocol/winrm/config.go +++ b/protocol/winrm/config.go @@ -19,7 +19,7 @@ type Config struct { Port int `yaml:"port" json:"port,omitempty" default:"5985" validate:"gt=0,lte=65535" jsonschema:"minimum=1,maximum=65535,description=WinRM port (default 5985; 5986 when useHTTPS is true)"` Password string `yaml:"password,omitempty" json:"password,omitempty" jsonschema:"minLength=1,description=Password for WinRM authentication"` UseHTTPS bool `yaml:"useHTTPS" json:"useHTTPS,omitempty" default:"false" jsonschema:"default=false,description=Use HTTPS for WinRM"` - Insecure bool `yaml:"insecure" json:"insecure,omitempty" default:"false" jsonschema:"default=false,description=Accept invalid TLS certificates"` + Insecure bool `yaml:"insecure" json:"insecure,omitempty" default:"false" jsonschema:"default=false,description=Accept invalid TLS certificates when useHTTPS is set; without useHTTPS allow sending the password over plain HTTP"` UseNTLM bool `yaml:"useNTLM" json:"useNTLM,omitempty" default:"false" jsonschema:"default=false,description=Use NTLM authentication"` CACertPath string `yaml:"caCertPath,omitempty" json:"caCertPath,omitempty" validate:"omitempty,file" jsonschema:"description=Path to CA certificate"` CertPath string `yaml:"certPath,omitempty" json:"certPath,omitempty" validate:"omitempty,file" jsonschema:"minLength=1,description=Path to client certificate"` @@ -55,22 +55,8 @@ func (c *Config) SetDefaults() { } } -// validateAuth checks that a complete authentication method is configured, and -// that the configured transport does not hand the password to a third party. -// -// Insecure is read as "do not refuse to expose the credential", widening it -// from "accept invalid TLS certificates": both senses describe a caller who -// has decided the transport does not have to protect what it carries. -// -// Plain HTTP is refused because the default transport authenticates with HTTP -// basic, putting the password on the wire base64-encoded. NTLM does not earn -// an exemption: the WinRM client authenticates with it but sends the SOAP -// envelopes unencrypted, so the commands, their output and any credential -// inside them stay readable. -// -// TLS with Insecure is permitted by the same rule that permits plain HTTP -// with it -- the caller asked for it explicitly. Certificate authentication -// is unaffected: it is only selected over HTTPS, which this never rejects. +// validateAuth checks that a complete authentication method is configured and +// that the transport can carry it. func (c *Config) validateAuth() error { if c.User == "" && c.Password == "" && c.CertPath == "" && c.KeyPath == "" { return fmt.Errorf("%w: no authentication method set (provide user+password or certPath+keyPath)", protocol.ErrValidationFailed) @@ -78,9 +64,29 @@ func (c *Config) validateAuth() error { if (c.CertPath == "") != (c.KeyPath == "") { return fmt.Errorf("%w: certPath and keyPath must both be set for certificate authentication", protocol.ErrValidationFailed) } + // The client certificate is only presented in a TLS handshake; over plain + // HTTP it would be silently ignored. + if c.CertPath != "" && !c.UseHTTPS { + return fmt.Errorf("%w: certificate authentication requires useHTTPS", protocol.ErrValidationFailed) + } if (c.User == "") != (c.Password == "") && c.CertPath == "" { return fmt.Errorf("%w: user and password must both be set for password authentication", protocol.ErrValidationFailed) } + return c.validatePasswordTransport() +} + +// validatePasswordTransport refuses a password on plain HTTP unless Insecure +// is set. NewConnection runs it too, because callers are not required to call +// Validate before connecting. +// +// Insecure is read as "do not refuse to expose the credential", widening it +// from "accept invalid TLS certificates": both senses describe a caller who +// has decided the transport does not have to protect what it carries. +// +// NTLM does not earn an exemption: the WinRM client authenticates with it but +// sends the SOAP envelopes unencrypted, so the commands, their output and any +// credential inside them stay readable. +func (c *Config) validatePasswordTransport() error { if c.Password != "" && !c.UseHTTPS && !c.Insecure { return fmt.Errorf("%w: a password would be sent over plain HTTP, where any observer recovers it; set useHTTPS, or insecure to send it anyway", protocol.ErrValidationFailed) } @@ -109,7 +115,10 @@ func (c *Config) Validate() error { // Connection returns a new WinRM Connection based on the configuration. func (c *Config) Connection() (protocol.Connection, error) { conn, err := NewConnection(*c, WithLogger(c.Log())) - return conn, err + if err != nil { + return nil, err + } + return conn, nil } // String returns a string representation of the configuration. diff --git a/protocol/winrm/config_test.go b/protocol/winrm/config_test.go index 251c4328..ec51211e 100644 --- a/protocol/winrm/config_test.go +++ b/protocol/winrm/config_test.go @@ -9,8 +9,8 @@ import ( ) // A password on a transport that does not protect it is refused unless the -// caller set insecure. Certificate authentication is only selected over -// HTTPS, so it is never reached by the rule. +// caller set insecure, and a client certificate is refused where it would be +// ignored. func TestValidateRejectsExposedPassword(t *testing.T) { for _, tc := range []struct { name string @@ -35,13 +35,31 @@ func TestValidateRejectsExposedPassword(t *testing.T) { name: "HTTPS", cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseHTTPS: true}, }, + { + name: "HTTPS inferred from port 5986", + cfg: Config{Address: "10.0.0.1", Port: 5986, User: "Administrator", Password: "hunter2"}, + }, { name: "HTTPS without certificate verification", cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseHTTPS: true, Insecure: true}, }, { - name: "certificate authentication carries no password", - cfg: Config{Address: "10.0.0.1", CertPath: "/dev/null", KeyPath: "/dev/null"}, + name: "certificate authentication over HTTPS", + cfg: Config{Address: "10.0.0.1", CertPath: "/dev/null", KeyPath: "/dev/null", UseHTTPS: true}, + }, + { + name: "certificate and password over HTTPS", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", CertPath: "/dev/null", KeyPath: "/dev/null", UseHTTPS: true}, + }, + { + name: "certificate over plain HTTP would be ignored", + cfg: Config{Address: "10.0.0.1", CertPath: "/dev/null", KeyPath: "/dev/null"}, + reject: true, + }, + { + name: "certificate over plain HTTP with insecure would still be ignored", + cfg: Config{Address: "10.0.0.1", CertPath: "/dev/null", KeyPath: "/dev/null", Insecure: true}, + reject: true, }, } { t.Run(tc.name, func(t *testing.T) { @@ -62,9 +80,37 @@ func TestValidateRejectsExposedPassword(t *testing.T) { if !errors.Is(err, protocol.ErrValidationFailed) { t.Errorf("Validate() = %v, want it to wrap protocol.ErrValidationFailed", err) } - if strings.Contains(err.Error(), cfg.Password) { + if cfg.Password != "" && strings.Contains(err.Error(), cfg.Password) { t.Error("the error repeats the password") } }) } } + +// Validate is optional for callers, so the connection path must refuse the +// exposure on its own. +func TestConnectionRejectsExposedPassword(t *testing.T) { + cfg := Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2"} + + conn, err := cfg.Connection() + if err == nil { + t.Fatal("Connection() error = nil, want a validation error") + } + if conn != nil { + t.Errorf("Connection() returned a connection alongside error %v", err) + } + if !errors.Is(err, protocol.ErrValidationFailed) { + t.Errorf("Connection() error = %v, want it to wrap protocol.ErrValidationFailed", err) + } + if !errors.Is(err, protocol.ErrNonRetryable) { + t.Errorf("Connection() error = %v, want it to wrap protocol.ErrNonRetryable", err) + } + if strings.Contains(err.Error(), cfg.Password) { + t.Error("the error repeats the password") + } + + cfg.Insecure = true + if _, err := cfg.Connection(); err != nil { + t.Errorf("Connection() with insecure error = %v, want nil", err) + } +} diff --git a/protocol/winrm/connection.go b/protocol/winrm/connection.go index 98b7d0c4..6cbb07c4 100644 --- a/protocol/winrm/connection.go +++ b/protocol/winrm/connection.go @@ -45,11 +45,17 @@ type Connection struct { type dialFunc func(network, addr string) (net.Conn, error) -// NewConnection creates a new WinRM connection. Error is currently always nil. +// NewConnection creates a new WinRM connection. It does not contact the host. +// The error wraps [protocol.ErrValidationFailed] and [protocol.ErrNonRetryable] +// when the configuration would send a password over plain HTTP without +// [Config.Insecure] set. func NewConnection(cfg Config, opts ...Option) (*Connection, error) { options := NewOptions(opts...) options.InjectLoggerTo(&cfg, log.KeyProtocol, "winrm-config") cfg.SetDefaults() + if err := cfg.validatePasswordTransport(); err != nil { + return nil, fmt.Errorf("%w: %w", protocol.ErrNonRetryable, err) + } c := &Connection{Config: cfg} options.InjectLoggerTo(c, log.KeyProtocol, "winrm") diff --git a/protocol/winrm/connection_test.go b/protocol/winrm/connection_test.go index 44bc00ae..b5d207de 100644 --- a/protocol/winrm/connection_test.go +++ b/protocol/winrm/connection_test.go @@ -192,6 +192,7 @@ func TestConnect_probeClassification(t *testing.T) { Port: port, User: "user", Password: "pass", + Insecure: true, }) if err != nil { t.Fatalf("NewConnection() error = %v", err) diff --git a/schemas/winrm.json b/schemas/winrm.json index b75fd2df..c8017132 100644 --- a/schemas/winrm.json +++ b/schemas/winrm.json @@ -118,7 +118,7 @@ }, "insecure": { "type": "boolean", - "description": "Accept invalid TLS certificates", + "description": "Accept invalid TLS certificates when useHTTPS is set; without useHTTPS allow sending the password over plain HTTP", "default": false }, "useNTLM": { diff --git a/schemas/winrm.yaml b/schemas/winrm.yaml index cda51455..e4fa1ced 100644 --- a/schemas/winrm.yaml +++ b/schemas/winrm.yaml @@ -80,7 +80,7 @@ $defs: default: false insecure: type: boolean - description: Accept invalid TLS certificates + description: Accept invalid TLS certificates when useHTTPS is set; without useHTTPS allow sending the password over plain HTTP default: false useNTLM: type: boolean From 8f84193ca832cc33669ba268165d6d5372805ef5 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Wed, 11 Nov 2026 13:59:48 +0200 Subject: [PATCH 4/6] fix(winrm): recheck the password rule in Connect and honor port-inferred HTTPS Connection.Config is exported and mutable, and a Connection can be built without NewConnection, so a password added after NewConnection reached the plain-HTTP transport unchecked. Connect now applies the same rule before any request. SetDefaults turns UseHTTPS on for port 5986, but Validate is often called before SetDefaults, so a password or client certificate on 5986 was rejected there while NewConnection accepted it. usesHTTPS treats port 5986 as HTTPS in the validation rules and in Connect's endpoint, so every path judges the transport the connection will actually use. The endpoint construction moves into newEndpoint to keep Connect within the cyclomatic complexity limit. Signed-off-by: Kimmo Lehto --- protocol/winrm/config.go | 16 ++++++--- protocol/winrm/config_test.go | 29 +++++++++++++--- protocol/winrm/connection.go | 31 +++++++++++------ protocol/winrm/connection_test.go | 58 +++++++++++++++++++++++++++++++ 4 files changed, 116 insertions(+), 18 deletions(-) diff --git a/protocol/winrm/config.go b/protocol/winrm/config.go index da4be712..0ff9a97f 100644 --- a/protocol/winrm/config.go +++ b/protocol/winrm/config.go @@ -66,7 +66,7 @@ func (c *Config) validateAuth() error { } // The client certificate is only presented in a TLS handshake; over plain // HTTP it would be silently ignored. - if c.CertPath != "" && !c.UseHTTPS { + if c.CertPath != "" && !c.usesHTTPS() { return fmt.Errorf("%w: certificate authentication requires useHTTPS", protocol.ErrValidationFailed) } if (c.User == "") != (c.Password == "") && c.CertPath == "" { @@ -75,9 +75,17 @@ func (c *Config) validateAuth() error { return c.validatePasswordTransport() } +// usesHTTPS reports whether the connection will use TLS. SetDefaults turns +// UseHTTPS on for port 5986; the port is honored here too so that a Config +// validated or connected before SetDefaults runs is judged by the transport +// it will actually get. +func (c *Config) usesHTTPS() bool { + return c.UseHTTPS || c.Port == 5986 +} + // validatePasswordTransport refuses a password on plain HTTP unless Insecure -// is set. NewConnection runs it too, because callers are not required to call -// Validate before connecting. +// is set. NewConnection and Connect run it too, because callers are not +// required to call Validate and the Config of a Connection is mutable. // // Insecure is read as "do not refuse to expose the credential", widening it // from "accept invalid TLS certificates": both senses describe a caller who @@ -87,7 +95,7 @@ func (c *Config) validateAuth() error { // sends the SOAP envelopes unencrypted, so the commands, their output and any // credential inside them stay readable. func (c *Config) validatePasswordTransport() error { - if c.Password != "" && !c.UseHTTPS && !c.Insecure { + if c.Password != "" && !c.usesHTTPS() && !c.Insecure { return fmt.Errorf("%w: a password would be sent over plain HTTP, where any observer recovers it; set useHTTPS, or insecure to send it anyway", protocol.ErrValidationFailed) } return nil diff --git a/protocol/winrm/config_test.go b/protocol/winrm/config_test.go index ec51211e..44e5bd58 100644 --- a/protocol/winrm/config_test.go +++ b/protocol/winrm/config_test.go @@ -13,9 +13,12 @@ import ( // ignored. func TestValidateRejectsExposedPassword(t *testing.T) { for _, tc := range []struct { - name string - cfg Config - reject bool + name string + cfg Config + // noDefaults validates without SetDefaults, as a caller validating + // a freshly unmarshaled config would. + noDefaults bool + reject bool }{ { name: "password over plain HTTP", @@ -39,6 +42,22 @@ func TestValidateRejectsExposedPassword(t *testing.T) { name: "HTTPS inferred from port 5986", cfg: Config{Address: "10.0.0.1", Port: 5986, User: "Administrator", Password: "hunter2"}, }, + { + name: "HTTPS inferred from port 5986 before SetDefaults", + cfg: Config{Address: "10.0.0.1", Port: 5986, User: "Administrator", Password: "hunter2"}, + noDefaults: true, + }, + { + name: "certificate over port 5986 before SetDefaults", + cfg: Config{Address: "10.0.0.1", Port: 5986, CertPath: "/dev/null", KeyPath: "/dev/null"}, + noDefaults: true, + }, + { + name: "password over port 5985 before SetDefaults", + cfg: Config{Address: "10.0.0.1", Port: 5985, User: "Administrator", Password: "hunter2"}, + noDefaults: true, + reject: true, + }, { name: "HTTPS without certificate verification", cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseHTTPS: true, Insecure: true}, @@ -64,7 +83,9 @@ func TestValidateRejectsExposedPassword(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { cfg := tc.cfg - cfg.SetDefaults() + if !tc.noDefaults { + cfg.SetDefaults() + } err := cfg.Validate() if !tc.reject { diff --git a/protocol/winrm/connection.go b/protocol/winrm/connection.go index 6cbb07c4..4716391e 100644 --- a/protocol/winrm/connection.go +++ b/protocol/winrm/connection.go @@ -204,18 +204,13 @@ func (c *Connection) bastionDialer(ctx context.Context) (*ssh.Connection, dialFu return bastionSSH, bastionSSH.Dial, nil } -// Connect opens the WinRM connection. Any existing connection is torn down first. -func (c *Connection) Connect(ctx context.Context) error { - c.Disconnect() - - if err := c.loadCertificates(); err != nil { - return fmt.Errorf("%w: failed to load certificates: %w", protocol.ErrNonRetryable, err) - } - +// newEndpoint builds the WinRM endpoint from the configuration and the +// certificates loaded by loadCertificates. +func (c *Connection) newEndpoint(ctx context.Context) *winrm.Endpoint { endpoint := &winrm.Endpoint{ Host: c.Address, Port: c.Port, - HTTPS: c.UseHTTPS, + HTTPS: c.usesHTTPS(), Insecure: c.Insecure, TLSServerName: c.TLSServerName, Timeout: endpointTimeout(ctx), @@ -233,6 +228,22 @@ func (c *Connection) Connect(ctx context.Context) error { endpoint.Key = c.key } + return endpoint +} + +// Connect opens the WinRM connection. Any existing connection is torn down first. +func (c *Connection) Connect(ctx context.Context) error { + c.Disconnect() + + if err := c.validatePasswordTransport(); err != nil { + return fmt.Errorf("%w: %w", protocol.ErrNonRetryable, err) + } + + if err := c.loadCertificates(); err != nil { + return fmt.Errorf("%w: failed to load certificates: %w", protocol.ErrNonRetryable, err) + } + + endpoint := c.newEndpoint(ctx) params := winrm.DefaultParameters if c.Bastion != nil { @@ -250,7 +261,7 @@ func (c *Connection) Connect(ctx context.Context) error { params.TransportDecorator = func() winrm.Transporter { return &winrm.ClientNTLM{} } } - if c.UseHTTPS && len(c.cert) > 0 { + if c.usesHTTPS() && len(c.cert) > 0 { params.TransportDecorator = func() winrm.Transporter { return &winrm.ClientAuthRequest{} } } diff --git a/protocol/winrm/connection_test.go b/protocol/winrm/connection_test.go index b5d207de..186fc55a 100644 --- a/protocol/winrm/connection_test.go +++ b/protocol/winrm/connection_test.go @@ -222,6 +222,64 @@ func TestConnect_probeClassification(t *testing.T) { } } +// Connection.Config is exported and mutable, and a Connection can be built +// without NewConnection, so Connect must recheck the plain-HTTP password rule +// against the configuration it is about to transmit. +func TestConnectRefusesPasswordOverPlainHTTP(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.WriteHeader(http.StatusUnauthorized) + })) + t.Cleanup(srv.Close) + + u, err := url.Parse(srv.URL) + if err != nil { + t.Fatalf("url.Parse(%q) error = %v", srv.URL, err) + } + port, err := strconv.Atoi(u.Port()) + if err != nil { + t.Fatalf("strconv.Atoi(%q) error = %v", u.Port(), err) + } + + passwordAdded, err := NewConnection(Config{Address: u.Hostname(), Port: port, User: "Administrator"}) + if err != nil { + t.Fatalf("NewConnection() without a password error = %v", err) + } + passwordAdded.Password = "hunter2" + + for _, tc := range []struct { + name string + conn *Connection + }{ + {name: "password set after NewConnection", conn: passwordAdded}, + { + name: "constructed without NewConnection", + conn: &Connection{Config: Config{Address: u.Hostname(), Port: port, User: "Administrator", Password: "hunter2"}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := tc.conn.Connect(context.Background()) + if err == nil { + t.Fatal("Connect() error = nil, want a validation error") + } + if !errors.Is(err, protocol.ErrValidationFailed) { + t.Errorf("Connect() error = %v, want it to wrap protocol.ErrValidationFailed", err) + } + if !errors.Is(err, protocol.ErrNonRetryable) { + t.Errorf("Connect() error = %v, want it to wrap protocol.ErrNonRetryable", err) + } + if strings.Contains(err.Error(), "hunter2") { + t.Error("the error repeats the password") + } + }) + } + + if got := requests.Load(); got != 0 { + t.Errorf("server received %d requests, want 0", got) + } +} + type errWriter struct{ err error } func (e errWriter) Write(_ []byte) (int, error) { return 0, e.err } From bd6c136506e68e7b0c0964b8d5c655cb0ada8223 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Wed, 11 Nov 2026 14:30:26 +0200 Subject: [PATCH 5/6] test(winrm): pin the endpoint scheme for port-inferred HTTPS Validation treats port 5986 as HTTPS before SetDefaults, so a directly constructed Connection on 5986 must build an HTTPS endpoint; otherwise a password accepted on that basis would travel over plain HTTP. Covers 5985, 5986 without useHTTPS, and custom ports with and without it. Signed-off-by: Kimmo Lehto --- protocol/winrm/connection_test.go | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/protocol/winrm/connection_test.go b/protocol/winrm/connection_test.go index 186fc55a..d0628939 100644 --- a/protocol/winrm/connection_test.go +++ b/protocol/winrm/connection_test.go @@ -280,6 +280,33 @@ func TestConnectRefusesPasswordOverPlainHTTP(t *testing.T) { } } +// The validation rules treat port 5986 as HTTPS even before SetDefaults, so +// the endpoint must too; otherwise a password accepted on that basis would +// travel over plain HTTP. +func TestNewEndpointScheme(t *testing.T) { + for _, tc := range []struct { + name string + cfg Config + wantHTTPS bool + }{ + {name: "port 5985", cfg: Config{Address: "10.0.0.1", Port: 5985}}, + {name: "port 5986 without useHTTPS", cfg: Config{Address: "10.0.0.1", Port: 5986}, wantHTTPS: true}, + {name: "custom port with useHTTPS", cfg: Config{Address: "10.0.0.1", Port: 8443, UseHTTPS: true}, wantHTTPS: true}, + {name: "custom port without useHTTPS", cfg: Config{Address: "10.0.0.1", Port: 8080}}, + } { + t.Run(tc.name, func(t *testing.T) { + conn := &Connection{Config: tc.cfg} + endpoint := conn.newEndpoint(context.Background()) + if endpoint.HTTPS != tc.wantHTTPS { + t.Errorf("newEndpoint().HTTPS = %v, want %v", endpoint.HTTPS, tc.wantHTTPS) + } + if endpoint.Port != tc.cfg.Port { + t.Errorf("newEndpoint().Port = %d, want %d", endpoint.Port, tc.cfg.Port) + } + }) + } +} + type errWriter struct{ err error } func (e errWriter) Write(_ []byte) (int, error) { return 0, e.err } From 61035e267e267b08fe8aa081b793cea5d6998d86 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Wed, 11 Nov 2026 15:27:30 +0200 Subject: [PATCH 6/6] fix(winrm): do not claim NTLM over plain HTTP discloses the password NTLM never sends the password itself, so the basic-auth message about the password being sent over plain HTTP was wrong for useNTLM. NTLM over plain HTTP is still refused: the WinRM client does not seal the session, so commands, output and any secret inside them are readable, and the captured challenge response can be cracked offline or relayed. It now gets its own message saying so. Signed-off-by: Kimmo Lehto --- protocol/winrm/config.go | 18 +++++++++++------- protocol/winrm/config_test.go | 30 ++++++++++++++++++++++++------ 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/protocol/winrm/config.go b/protocol/winrm/config.go index 0ff9a97f..80339eb4 100644 --- a/protocol/winrm/config.go +++ b/protocol/winrm/config.go @@ -87,18 +87,22 @@ func (c *Config) usesHTTPS() bool { // is set. NewConnection and Connect run it too, because callers are not // required to call Validate and the Config of a Connection is mutable. // -// Insecure is read as "do not refuse to expose the credential", widening it +// Insecure is read as "do not refuse an unprotected transport", widening it // from "accept invalid TLS certificates": both senses describe a caller who // has decided the transport does not have to protect what it carries. // -// NTLM does not earn an exemption: the WinRM client authenticates with it but -// sends the SOAP envelopes unencrypted, so the commands, their output and any -// credential inside them stay readable. +// NTLM does not earn an exemption. It never sends the password itself, but +// the WinRM client does not seal the session, so the commands, their output +// and any secret inside them stay readable, and the captured challenge +// response can be cracked offline or relayed. func (c *Config) validatePasswordTransport() error { - if c.Password != "" && !c.usesHTTPS() && !c.Insecure { - return fmt.Errorf("%w: a password would be sent over plain HTTP, where any observer recovers it; set useHTTPS, or insecure to send it anyway", protocol.ErrValidationFailed) + if c.Password == "" || c.usesHTTPS() || c.Insecure { + return nil } - return nil + if c.UseNTLM { + return fmt.Errorf("%w: NTLM over plain HTTP leaves the session unencrypted and exposes the challenge response to capture; set useHTTPS, or insecure to connect anyway", protocol.ErrValidationFailed) + } + return fmt.Errorf("%w: a password would be sent over plain HTTP, where any observer recovers it; set useHTTPS, or insecure to send it anyway", protocol.ErrValidationFailed) } // Validate checks the configuration for any invalid values. diff --git a/protocol/winrm/config_test.go b/protocol/winrm/config_test.go index 44e5bd58..bf40da2a 100644 --- a/protocol/winrm/config_test.go +++ b/protocol/winrm/config_test.go @@ -19,16 +19,28 @@ func TestValidateRejectsExposedPassword(t *testing.T) { // a freshly unmarshaled config would. noDefaults bool reject bool + // wantMsg, when set, must appear in the error; notMsg must not. + wantMsg string + notMsg string }{ { - name: "password over plain HTTP", - cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2"}, - reject: true, + name: "password over plain HTTP", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2"}, + reject: true, + wantMsg: "a password would be sent over plain HTTP", }, { - name: "NTLM does not protect the body", - cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseNTLM: true}, - reject: true, + // NTLM never sends the password itself, so the error must not + // claim it does. + name: "NTLM does not protect the session", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseNTLM: true}, + reject: true, + wantMsg: "NTLM over plain HTTP leaves the session unencrypted", + notMsg: "a password would be sent", + }, + { + name: "NTLM over plain HTTP accepted once insecure is set", + cfg: Config{Address: "10.0.0.1", User: "Administrator", Password: "hunter2", UseNTLM: true, Insecure: true}, }, { name: "plain HTTP accepted once insecure is set", @@ -104,6 +116,12 @@ func TestValidateRejectsExposedPassword(t *testing.T) { if cfg.Password != "" && strings.Contains(err.Error(), cfg.Password) { t.Error("the error repeats the password") } + if tc.wantMsg != "" && !strings.Contains(err.Error(), tc.wantMsg) { + t.Errorf("Validate() = %v, want it to contain %q", err, tc.wantMsg) + } + if tc.notMsg != "" && strings.Contains(err.Error(), tc.notMsg) { + t.Errorf("Validate() = %v, want it not to contain %q", err, tc.notMsg) + } }) } }