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/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/config.go b/protocol/winrm/config.go index 0e9ce6fe..80339eb4 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,7 +55,8 @@ 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 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) @@ -63,10 +64,45 @@ 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.usesHTTPS() { + 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 nil + 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 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 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. 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 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. @@ -91,7 +127,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 new file mode 100644 index 00000000..bf40da2a --- /dev/null +++ b/protocol/winrm/config_test.go @@ -0,0 +1,155 @@ +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, and a client certificate is refused where it would be +// ignored. +func TestValidateRejectsExposedPassword(t *testing.T) { + for _, tc := range []struct { + name string + cfg Config + // noDefaults validates without SetDefaults, as a caller validating + // 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, + wantMsg: "a password would be sent over plain HTTP", + }, + { + // 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", + 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 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}, + }, + { + 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) { + cfg := tc.cfg + if !tc.noDefaults { + 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 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) + } + }) + } +} + +// 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 b8ad0c99..4716391e 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") + 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") @@ -198,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), @@ -227,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 { @@ -244,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 44bc00ae..d0628939 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) @@ -221,6 +222,91 @@ 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) + } +} + +// 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 } 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") + } +} 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