feat(winrm)!: refuse to send a password over plain HTTP

client_test.go

@@ -4,15 +4,21 @@ import (
 	"context"
 	"errors"
 	"io"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 
 	"github.com/k0sproject/rig/v2"
 	"github.com/k0sproject/rig/v2/cmd"
 	"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")

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
 	}

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")
+	}
+}

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,18 +55,54 @@ 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)
 	}
 	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.

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)
+	}
+}