From 3b63d4e91e395eaf96548996280310e044ac8b80 Mon Sep 17 00:00:00 2001
From: James Nesbitt <jnesbitt@mirantis.com>
Date: Thu, 5 Nov 2026 12:10:24 +0200
Subject: [PATCH 1/5] [PRODENG-3935] Warn on insecure WinRM with an inline
 password

winRM.insecure makes rig accept any server certificate, so a host with
useHTTPS: true and insecure: true has an encrypted but unauthenticated
channel: an on-path attacker can impersonate the host, recover the
password and read every command sent to it.

ClusterConfig.Validate now warns once per affected host. Validate is the
seam rather than Host.UnmarshalYAML because it runs once per config load
(pkg/product/mke/mke.go:37), while UnmarshalYAML re-runs on migrations
and round-trips and would emit duplicate warnings for one apply. The
trigger is UseHTTPS && Insecure && Password != "". It is a warning, not
an error, so an operator who accepts the risk can still apply. The
message names the host address and the two settings, never the
credential, its length, or a digest of it.

docs/usage/getting-started.md gains guidance on sourcing credentials
from the environment, covering the envsubst rules that apply: an unset
variable fails the load rather than substituting empty, a literal "$"
must be escaped as "$$", and "$" followed by a digit stays literal.

Covers acceptance criteria 2 and 3. Criterion 1, scrubbing the untracked
working-tree launchpad.yaml and rotating the referenced credentials, is
not addressed here.

Written by AI: claude-opus-5

Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
---
 docs/usage/getting-started.md          |  63 ++++++++++++
 pkg/product/mke/config/cluster.go      |  35 +++++++
 pkg/product/mke/config/cluster_test.go | 132 +++++++++++++++++++++++++
 3 files changed, 230 insertions(+)

diff --git a/docs/usage/getting-started.md b/docs/usage/getting-started.md
index b59c6c48e..7cbce6228 100644
--- a/docs/usage/getting-started.md
+++ b/docs/usage/getting-started.md
@@ -29,6 +29,69 @@ Create a `launchpad.yaml` to define your cluster. A minimal configuration includ
 
 For a full reference, see the [public documentation](https://docs.mirantis.com/mke/3.8/launchpad.html).
 
+### Keeping credentials out of `launchpad.yaml`
+
+Do not write passwords into `launchpad.yaml`. The file sits in plaintext on
+the disk of whichever machine runs Launchpad, is read by every `apply` and
+`reset`, and is easy to paste into a ticket or a chat window by accident.
+
+Launchpad expands environment variables in the configuration file before
+parsing it, so reference the credential instead of inlining it:
+
+```yaml
+spec:
+  hosts:
+    - role: manager
+      winRM:
+        address: 10.0.0.1
+        user: Administrator
+        password: ${WINRM_PASSWORD}
+        useHTTPS: true
+  mke:
+    adminUsername: admin
+    adminPassword: ${MKE_ADMIN_PASSWORD}
+```
+
+Supply the values from your shell, a secret manager, or a CI secret store:
+
+```bash
+export WINRM_PASSWORD="$(my-secret-tool read winrm/administrator)"
+export MKE_ADMIN_PASSWORD="$(my-secret-tool read mke/admin)"
+launchpad apply --config launchpad.yaml
+```
+
+Both `${VAR}` and `$VAR` are recognised. Note the substitution rules, because
+they bite in exactly this situation:
+
+- **An unset variable is an error, not an empty string.** If
+  `WINRM_PASSWORD` is not exported, `launchpad apply` fails with a
+  substitution error rather than quietly trying to authenticate with an empty
+  password. An exported-but-empty variable is accepted as empty.
+- **A literal `$` in a value must be escaped as `$$`.** A password of
+  `Pa$$word` has to be written `Pa$$$$word`, and a password of `abc$def` has
+  to be written `abc$$def` — otherwise `$def` is read as a variable
+  reference. This applies to any value in the file, not just passwords.
+- **`$` followed by a digit is left alone.** `$5986` is never a valid
+  variable name, so it stays literal and needs no escaping.
+
+Escaping is why sourcing the credential from the environment is safer than
+inlining it even when the file is private: the value of `$WINRM_PASSWORD` is
+substituted verbatim and is never re-scanned for `$`.
+
+### WinRM over TLS
+
+`winRM.insecure: true` tells Launchpad to accept any certificate the host
+presents. The channel is then encrypted but the peer is not authenticated, so
+an attacker on the network path can impersonate the host, capture the password
+and read every command sent to it — `useHTTPS: true` does not protect you on
+its own.
+
+Launchpad logs a warning at config load for every host that combines
+`winRM.insecure: true` with a `winRM.password`. Treat it as a finding:
+install a certificate the client trusts (pin it with `winRM.caCertPath`) and
+drop `winRM.insecure`. Leaving `insecure` set is only defensible on a trusted
+network you control, and never with a password you would mind losing.
+
 ## Running Launchpad
 
 Use the `apply` command to start the installation:
diff --git a/pkg/product/mke/config/cluster.go b/pkg/product/mke/config/cluster.go
index 7d4c28d72..78699561f 100644
--- a/pkg/product/mke/config/cluster.go
+++ b/pkg/product/mke/config/cluster.go
@@ -9,6 +9,7 @@ import (
 	validator "github.com/go-playground/validator/v10"
 	rig "github.com/k0sproject/rig/v2"
 	"github.com/k0sproject/rig/v2/protocol/ssh"
+	log "github.com/sirupsen/logrus"
 )
 
 // ClusterMeta defines cluster metadata.
@@ -44,6 +45,8 @@ func (c *ClusterConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 // Validate validates that everything in the config makes sense
 // Currently we do only very "static" validation using https://github.com/go-playground/validator
 func (c *ClusterConfig) Validate() error {
+	c.warnInsecureWinRM()
+
 	validator := validator.New(validator.WithRequiredStructEnabled())
 	validator.RegisterStructValidation(roleChecks, ClusterSpec{})
 	if err := validator.Struct(c); err != nil {
@@ -52,6 +55,38 @@ func (c *ClusterConfig) Validate() error {
 	return nil
 }
 
+// warnInsecureWinRM warns about every host that combines winRM.insecure with a
+// password written into the configuration file. winRM.insecure makes rig accept
+// any server certificate, so the channel is encrypted but the peer is never
+// authenticated: an on-path attacker can terminate the TLS session, recover the
+// password and read every command issued to that host. This is deliberately a
+// warning and not an error, because an operator may have accepted that risk
+// knowingly and must still be able to apply. The message names the host and the
+// settings only -- never the credential, nor its length or any digest of it.
+func (c *ClusterConfig) warnInsecureWinRM() {
+	if c.Spec == nil {
+		return
+	}
+
+	for _, h := range c.Spec.Hosts {
+		if h == nil || h.WinRM == nil {
+			continue
+		}
+		wrm := h.WinRM
+		if !wrm.UseHTTPS || !wrm.Insecure || wrm.Password == "" {
+			continue
+		}
+
+		log.Warnf(
+			"%s: winRM.insecure is true, so the host's TLS certificate is not verified, and winRM.password is set in the configuration file. "+
+				"An on-path attacker can impersonate this host, capture the password and read every command sent to it. "+
+				"Remove winRM.insecure once the host presents a trusted certificate, and keep the password out of the file by using an environment variable "+
+				"(see docs/usage/getting-started.md, \"Keeping credentials out of launchpad.yaml\").",
+			wrm.Address,
+		)
+	}
+}
+
 func roleChecks(sl validator.StructLevel) {
 	spec, ok := sl.Current().Interface().(ClusterSpec)
 	if !ok {
diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go
index fccaaa41a..32966706c 100644
--- a/pkg/product/mke/config/cluster_test.go
+++ b/pkg/product/mke/config/cluster_test.go
@@ -28,6 +28,8 @@ import (
 	_ "github.com/Mirantis/launchpad/pkg/config/migration/v1beta3"
 	"github.com/Mirantis/launchpad/pkg/constant"
 	validator "github.com/go-playground/validator/v10"
+	"github.com/sirupsen/logrus"
+	logrustest "github.com/sirupsen/logrus/hooks/test"
 	"github.com/stretchr/testify/require"
 	"gopkg.in/yaml.v2"
 )
@@ -510,6 +512,136 @@ spec:
 
 }
 
+func TestInsecureWinRMWarning(t *testing.T) {
+	// An obviously fake value: the point of the assertions below is that this
+	// string never reaches the log, so it must be recognisable if it ever does.
+	const fakePassword = "NOT-A-REAL-PASSWORD"
+
+	configWithWinRM := func(settings string) string {
+		return `
+apiVersion: launchpad.mirantis.com/mke/v1.6
+kind: mke
+spec:
+  mcr:
+    channel: stable
+  mke:
+    version: 3.3.7
+  hosts:
+    - role: manager
+      winRM:
+        address: 10.0.0.1
+        user: Administrator
+` + settings
+	}
+
+	tests := []struct {
+		name     string
+		settings string
+		wantWarn bool
+	}{
+		{
+			name:     "insecure https with an inline password warns",
+			settings: "        useHTTPS: true\n        insecure: true\n        password: " + fakePassword + "\n",
+			wantWarn: true,
+		},
+		{
+			name:     "insecure https without a password does not warn",
+			settings: "        useHTTPS: true\n        insecure: true\n",
+			wantWarn: false,
+		},
+		{
+			name:     "verified https with an inline password does not warn",
+			settings: "        useHTTPS: true\n        insecure: false\n        password: " + fakePassword + "\n",
+			wantWarn: false,
+		},
+		{
+			name:     "insecure without https does not warn",
+			settings: "        useHTTPS: false\n        insecure: true\n        password: " + fakePassword + "\n",
+			wantWarn: false,
+		},
+		{
+			name:     "plain winrm host does not warn",
+			settings: "",
+			wantWarn: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hook := logrustest.NewGlobal()
+			defer hook.Reset()
+
+			c := loadYaml(t, configWithWinRM(tt.settings))
+			require.NoError(t, c.Validate())
+
+			var warnings []string
+			for _, entry := range hook.AllEntries() {
+				require.NotContains(t, entry.Message, fakePassword, "the warning must never print the password")
+				if entry.Level == logrus.WarnLevel {
+					warnings = append(warnings, entry.Message)
+				}
+			}
+
+			if !tt.wantWarn {
+				require.Empty(t, warnings)
+				return
+			}
+
+			require.Len(t, warnings, 1)
+			require.Contains(t, warnings[0], "10.0.0.1")
+			require.Contains(t, warnings[0], "winRM.insecure")
+			require.Contains(t, warnings[0], "winRM.password")
+		})
+	}
+}
+
+func TestInsecureWinRMWarningIsPerHost(t *testing.T) {
+	hook := logrustest.NewGlobal()
+	defer hook.Reset()
+
+	data := `
+apiVersion: launchpad.mirantis.com/mke/v1.6
+kind: mke
+spec:
+  mcr:
+    channel: stable
+  mke:
+    version: 3.3.7
+  hosts:
+    - role: manager
+      winRM:
+        address: 10.0.0.1
+        user: Administrator
+        useHTTPS: true
+        insecure: true
+        password: NOT-A-REAL-PASSWORD
+    - role: worker
+      winRM:
+        address: 10.0.0.2
+        user: Administrator
+        useHTTPS: true
+        insecure: true
+        password: ALSO-NOT-REAL
+    - role: worker
+      winRM:
+        address: 10.0.0.3
+        user: Administrator
+`
+	c := loadYaml(t, data)
+	require.NoError(t, c.Validate())
+
+	var warnings []string
+	for _, entry := range hook.AllEntries() {
+		if entry.Level == logrus.WarnLevel {
+			warnings = append(warnings, entry.Message)
+		}
+	}
+
+	require.Len(t, warnings, 2)
+	require.Contains(t, warnings[0], "10.0.0.1")
+	require.Contains(t, warnings[1], "10.0.0.2")
+}
+
 // Just a small helper to load the config struct from yaml to get defaults etc. in place.
 func loadYaml(t *testing.T, data string) *ClusterConfig {
 	c := &ClusterConfig{}

From 6b302cc1e9bdb2a212945c284226285989faa1e1 Mon Sep 17 00:00:00 2001
From: James Nesbitt <jnesbitt@mirantis.com>
Date: Thu, 5 Nov 2026 12:18:12 +0200
Subject: [PATCH 2/5] [PRODENG-3935] Address review: YAML re-parse docs, port
 5986 test

Two findings from the automated review on PR #682.

Finding on docs/usage/getting-started.md: an environment-sourced value is
substituted into the configuration text, which is then parsed as YAML, so
metacharacters in the value are interpreted. Confirmed against the built
binary: WINRM_PASSWORD='abc: def' aborts the load with "yaml: line 11:
mapping values are not allowed in this context", while a plain value loads.
Quoting the placeholder in the source file does not help, because
pkg/config/config.go re-marshals the configuration before substitution and
that discards the quoting. The guidance now documents the re-parse, the
truncation of an unquoted "#", and two ways to avoid it.

Finding on pkg/product/mke/config/cluster.go: the claim was that a host with
port 5986, insecure and a password, but no explicit useHTTPS, escapes the
warning. It does not. rig's winrm.Config.SetDefaults sets UseHTTPS for port
5986, and creasty/defaults invokes it through the Setter interface from
Host.UnmarshalYAML (defaults.go:42,164), so UseHTTPS is already true when
Validate runs. Verified by running the binary against such a config: the
warning fires. No code change; a test case pins the behaviour so a future
change to the defaults chain cannot regress it silently.

Written by AI: claude-opus-5

Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
---
 docs/usage/getting-started.md          | 26 ++++++++++++++++++++++----
 pkg/product/mke/config/cluster_test.go | 11 +++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/docs/usage/getting-started.md b/docs/usage/getting-started.md
index 7cbce6228..e6e97ad3f 100644
--- a/docs/usage/getting-started.md
+++ b/docs/usage/getting-started.md
@@ -73,10 +73,28 @@ they bite in exactly this situation:
   reference. This applies to any value in the file, not just passwords.
 - **`$` followed by a digit is left alone.** `$5986` is never a valid
   variable name, so it stays literal and needs no escaping.
-
-Escaping is why sourcing the credential from the environment is safer than
-inlining it even when the file is private: the value of `$WINRM_PASSWORD` is
-substituted verbatim and is never re-scanned for `$`.
+- **The substituted value is parsed as YAML afterwards.** Substitution
+  rewrites the file's text and the result is then re-parsed, so YAML
+  metacharacters inside the value are interpreted rather than taken
+  literally. A password of `abc #def` is truncated to `abc`, because
+  everything from the unquoted `#` is a comment; a password of `abc: def`
+  aborts the load with `yaml: line 11: mapping values are not allowed in
+  this context`. Quoting the placeholder in your own file does not help:
+  the configuration is re-marshalled before substitution, which discards
+  that quoting.
+
+A value containing `#`, `:`, `"`, `'`, a leading `{`, `[`, `*`, `&` or `%`,
+or a trailing space therefore needs care. Two ways to stay safe:
+
+- Prefer generated passwords restricted to alphanumerics and `-`, `_`, `.`,
+  which have no YAML meaning in any position.
+- If the value is outside your control, pass it base64-encoded and decode it
+  when you set the variable, so the text that reaches the configuration file
+  is always inert.
+
+Sourcing the credential from the environment is still better than inlining
+it, because the value lives in the process environment rather than on disk —
+but it is not a way to smuggle arbitrary bytes past the parser.
 
 ### WinRM over TLS
 
diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go
index 32966706c..5e6b9a5fc 100644
--- a/pkg/product/mke/config/cluster_test.go
+++ b/pkg/product/mke/config/cluster_test.go
@@ -544,6 +544,17 @@ spec:
 			settings: "        useHTTPS: true\n        insecure: true\n        password: " + fakePassword + "\n",
 			wantWarn: true,
 		},
+		{
+			// port 5986 implies TLS without an explicit useHTTPS: rig's
+			// winrm.Config.SetDefaults flips UseHTTPS for that port, and
+			// creasty/defaults invokes it from Host.UnmarshalYAML, so the
+			// flag is already true by the time Validate runs. This case
+			// locks that in: the warning must not depend on the user
+			// having spelled useHTTPS out.
+			name:     "insecure with implicit https via port 5986 warns",
+			settings: "        port: 5986\n        insecure: true\n        password: " + fakePassword + "\n",
+			wantWarn: true,
+		},
 		{
 			name:     "insecure https without a password does not warn",
 			settings: "        useHTTPS: true\n        insecure: true\n",

From 80216f5a649d94bed3002bf76fdaeced7d4bd5b4 Mon Sep 17 00:00:00 2001
From: James Nesbitt <jnesbitt@mirantis.com>
Date: Thu, 5 Nov 2026 12:49:44 +0200
Subject: [PATCH 3/5] [PRODENG-3935] Review: reword warning, assert password
 absent from log fields

Two items from the review of PR #682.

The warning told the user to keep the password out of the configuration
file by using an environment variable. Substitution happens at
pkg/config/config.go:79, before parsing, so Password is populated
identically whether the value was inlined or came from ${WINRM_PASSWORD}.
A user who had already followed the guidance was told to follow it. The
message now leads with the certificate problem, which is actionable in
every case, and names winRM.caCertPath as the fix. That field name is
confirmed against protocol/winrm/config.go:24, not taken from the review.

The tests asserted the password never appears in entry.Message but did
not check entry.Data, so a change that moved the value into a logrus
field would have passed. Both are now asserted.

Written by AI: claude-opus-5

Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
---
 pkg/product/mke/config/cluster.go      | 14 +++++++++++---
 pkg/product/mke/config/cluster_test.go |  7 ++++++-
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/pkg/product/mke/config/cluster.go b/pkg/product/mke/config/cluster.go
index 78699561f..f2b0766b1 100644
--- a/pkg/product/mke/config/cluster.go
+++ b/pkg/product/mke/config/cluster.go
@@ -77,10 +77,18 @@ func (c *ClusterConfig) warnInsecureWinRM() {
 			continue
 		}
 
+		// The message cannot tell whether the password was inlined or came
+		// from an environment variable: substitution happens at
+		// pkg/config/config.go:79, before the config is parsed, so the field
+		// is populated identically either way. It therefore leads with the
+		// certificate problem, which is always actionable, and mentions
+		// sourcing the password from the environment as secondary advice
+		// rather than implying the user has inlined it.
 		log.Warnf(
-			"%s: winRM.insecure is true, so the host's TLS certificate is not verified, and winRM.password is set in the configuration file. "+
-				"An on-path attacker can impersonate this host, capture the password and read every command sent to it. "+
-				"Remove winRM.insecure once the host presents a trusted certificate, and keep the password out of the file by using an environment variable "+
+			"%s: winRM.insecure is true, so the host's TLS certificate is not verified. "+
+				"An on-path attacker can impersonate this host, capture the WinRM password and read every command sent to it. "+
+				"Remove winRM.insecure once the host presents a trusted certificate (pin it with winRM.caCertPath), "+
+				"and source the password from the environment rather than inlining it "+
 				"(see docs/usage/getting-started.md, \"Keeping credentials out of launchpad.yaml\").",
 			wrm.Address,
 		)
diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go
index 5e6b9a5fc..83681f25b 100644
--- a/pkg/product/mke/config/cluster_test.go
+++ b/pkg/product/mke/config/cluster_test.go
@@ -3,6 +3,7 @@ package config
 import (
 	"encoding/json"
 	"errors"
+	"fmt"
 	"os"
 	"strings"
 	"testing"
@@ -588,6 +589,10 @@ spec:
 			var warnings []string
 			for _, entry := range hook.AllEntries() {
 				require.NotContains(t, entry.Message, fakePassword, "the warning must never print the password")
+				// Structured fields are a separate leak route from the
+				// formatted message: a change that moved the value into a
+				// logrus field would otherwise go unnoticed here.
+				require.NotContains(t, fmt.Sprint(entry.Data), fakePassword, "the password must never reach a log field")
 				if entry.Level == logrus.WarnLevel {
 					warnings = append(warnings, entry.Message)
 				}
@@ -601,7 +606,7 @@ spec:
 			require.Len(t, warnings, 1)
 			require.Contains(t, warnings[0], "10.0.0.1")
 			require.Contains(t, warnings[0], "winRM.insecure")
-			require.Contains(t, warnings[0], "winRM.password")
+			require.Contains(t, warnings[0], "winRM.caCertPath")
 		})
 	}
 }

From 432094850295a8de62b7228839b9f0ff91ffbcca Mon Sep 17 00:00:00 2001
From: James Nesbitt <jnesbitt@mirantis.com>
Date: Thu, 5 Nov 2026 12:55:23 +0200
Subject: [PATCH 4/5] [PRODENG-3935] Warn when a WinRM password is sent over
 plain HTTP

Added by user ruling, beyond the two findings named in the ticket.

rig installs a WinRM transport decorator only for NTLM, or for HTTPS with a
loaded client certificate (protocol/winrm/connection.go:242-250). With
useHTTPS false and useNTLM false, both of which are the defaults
(protocol/winrm/config.go:21,23), neither applies, so go-winrm's default
transport calls req.SetBasicAuth (masterzen/winrm/http.go:96) and the
password crosses the network base64-encoded in an Authorization header with
no encryption.

The two exposures are reported as separate messages rather than one blended
warning, because the remedy differs and a reader must be able to tell which
applies: plain HTTP says the password is sent in clear, insecure HTTPS says
the certificate is not verified. A host configured with useNTLM is not
reported, since NTLM negotiates its own message-level encryption.

warnInsecureWinRM is renamed warnWinRMPasswordExposure to match what it now
covers. Tests assert the distinguishing phrase of each message, assert the
other message's phrase is absent so the two cannot blend, and keep the
existing negative cases and the entry.Data leak assertion.

Written by AI: claude-opus-5

Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
---
 docs/usage/getting-started.md          | 29 +++++----
 pkg/product/mke/config/cluster.go      | 64 ++++++++++++++------
 pkg/product/mke/config/cluster_test.go | 82 +++++++++++++++++++-------
 3 files changed, 123 insertions(+), 52 deletions(-)

diff --git a/docs/usage/getting-started.md b/docs/usage/getting-started.md
index e6e97ad3f..27bdf94bd 100644
--- a/docs/usage/getting-started.md
+++ b/docs/usage/getting-started.md
@@ -98,17 +98,24 @@ but it is not a way to smuggle arbitrary bytes past the parser.
 
 ### WinRM over TLS
 
-`winRM.insecure: true` tells Launchpad to accept any certificate the host
-presents. The channel is then encrypted but the peer is not authenticated, so
-an attacker on the network path can impersonate the host, capture the password
-and read every command sent to it — `useHTTPS: true` does not protect you on
-its own.
-
-Launchpad logs a warning at config load for every host that combines
-`winRM.insecure: true` with a `winRM.password`. Treat it as a finding:
-install a certificate the client trusts (pin it with `winRM.caCertPath`) and
-drop `winRM.insecure`. Leaving `insecure` set is only defensible on a trusted
-network you control, and never with a password you would mind losing.
+Launchpad warns at config load for every host whose WinRM password is exposed
+by the transport it is configured to use. There are two separate failures.
+
+**`winRM.useHTTPS: false` with a password sends it in clear.** Without TLS,
+and without `winRM.useNTLM: true`, the password is placed in an HTTP
+`Authorization` header as base64 — an encoding, not encryption. Anyone on the
+network path reads the password and every command sent to the host. This is
+the default transport: `useHTTPS` defaults to `false`.
+
+**`winRM.insecure: true` accepts any certificate the host presents.** The
+channel is encrypted but the peer is not authenticated, so an attacker on the
+network path can impersonate the host, capture the password and read every
+command sent to it. `useHTTPS: true` does not protect you on its own.
+
+Treat either warning as a finding. Set `winRM.useHTTPS: true`, install a
+certificate the client trusts, pin it with `winRM.caCertPath`, and leave
+`winRM.insecure` unset. Leaving `insecure` set is only defensible on a
+trusted network you control, and never with a password you would mind losing.
 
 ## Running Launchpad
 
diff --git a/pkg/product/mke/config/cluster.go b/pkg/product/mke/config/cluster.go
index f2b0766b1..371e32cbc 100644
--- a/pkg/product/mke/config/cluster.go
+++ b/pkg/product/mke/config/cluster.go
@@ -45,7 +45,7 @@ func (c *ClusterConfig) UnmarshalYAML(unmarshal func(interface{}) error) error {
 // Validate validates that everything in the config makes sense
 // Currently we do only very "static" validation using https://github.com/go-playground/validator
 func (c *ClusterConfig) Validate() error {
-	c.warnInsecureWinRM()
+	c.warnWinRMPasswordExposure()
 
 	validator := validator.New(validator.WithRequiredStructEnabled())
 	validator.RegisterStructValidation(roleChecks, ClusterSpec{})
@@ -55,15 +55,24 @@ func (c *ClusterConfig) Validate() error {
 	return nil
 }
 
-// warnInsecureWinRM warns about every host that combines winRM.insecure with a
-// password written into the configuration file. winRM.insecure makes rig accept
-// any server certificate, so the channel is encrypted but the peer is never
-// authenticated: an on-path attacker can terminate the TLS session, recover the
-// password and read every command issued to that host. This is deliberately a
-// warning and not an error, because an operator may have accepted that risk
-// knowingly and must still be able to apply. The message names the host and the
-// settings only -- never the credential, nor its length or any digest of it.
-func (c *ClusterConfig) warnInsecureWinRM() {
+// warnWinRMPasswordExposure warns about every host whose WinRM password is
+// exposed by the transport it is configured to use. Two distinct failures are
+// reported separately, because the remedy differs:
+//
+//   - plain HTTP with a password. rig installs a transport decorator only for
+//     NTLM, or for HTTPS with a loaded client certificate
+//     (protocol/winrm/connection.go:242-250). With neither, go-winrm's default
+//     transport calls req.SetBasicAuth (masterzen/winrm/http.go:96), so the
+//     password crosses the network base64-encoded in an Authorization header
+//     with no encryption at all.
+//   - HTTPS with winRM.insecure. The channel is encrypted but the peer is
+//     never authenticated, so an on-path attacker can impersonate the host,
+//     recover the password and read every command issued to it.
+//
+// Both are warnings rather than errors: an operator may have accepted either
+// risk knowingly and must still be able to apply. The messages name the host
+// and the settings only -- never the credential, nor its length or any digest.
+func (c *ClusterConfig) warnWinRMPasswordExposure() {
 	if c.Spec == nil {
 		return
 	}
@@ -73,7 +82,22 @@ func (c *ClusterConfig) warnInsecureWinRM() {
 			continue
 		}
 		wrm := h.WinRM
-		if !wrm.UseHTTPS || !wrm.Insecure || wrm.Password == "" {
+		if wrm.Password == "" {
+			continue
+		}
+
+		// No message-level protection either: NTLM negotiates its own
+		// encryption, so it is not cleartext even over plain HTTP.
+		if !wrm.UseHTTPS && !wrm.UseNTLM {
+			log.Warnf(
+				"%s: winRM.useHTTPS is false, so the WinRM password is sent in clear. "+
+					"It is base64-encoded in an HTTP Authorization header, which is encoding, not encryption, "+
+					"and anyone on the network path can read it along with every command sent to this host. "+
+					"Set winRM.useHTTPS and give the host a certificate your client trusts "+
+					"(see docs/usage/getting-started.md, \"WinRM over TLS\").",
+				wrm.Address,
+			)
+
 			continue
 		}
 
@@ -84,14 +108,16 @@ func (c *ClusterConfig) warnInsecureWinRM() {
 		// certificate problem, which is always actionable, and mentions
 		// sourcing the password from the environment as secondary advice
 		// rather than implying the user has inlined it.
-		log.Warnf(
-			"%s: winRM.insecure is true, so the host's TLS certificate is not verified. "+
-				"An on-path attacker can impersonate this host, capture the WinRM password and read every command sent to it. "+
-				"Remove winRM.insecure once the host presents a trusted certificate (pin it with winRM.caCertPath), "+
-				"and source the password from the environment rather than inlining it "+
-				"(see docs/usage/getting-started.md, \"Keeping credentials out of launchpad.yaml\").",
-			wrm.Address,
-		)
+		if wrm.UseHTTPS && wrm.Insecure {
+			log.Warnf(
+				"%s: winRM.insecure is true, so the host's TLS certificate is not verified. "+
+					"An on-path attacker can impersonate this host, capture the WinRM password and read every command sent to it. "+
+					"Remove winRM.insecure once the host presents a trusted certificate (pin it with winRM.caCertPath), "+
+					"and source the password from the environment rather than inlining it "+
+					"(see docs/usage/getting-started.md, \"Keeping credentials out of launchpad.yaml\").",
+				wrm.Address,
+			)
+		}
 	}
 }
 
diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go
index 83681f25b..ec3a9a0aa 100644
--- a/pkg/product/mke/config/cluster_test.go
+++ b/pkg/product/mke/config/cluster_test.go
@@ -513,11 +513,19 @@ spec:
 
 }
 
-func TestInsecureWinRMWarning(t *testing.T) {
+func TestWinRMPasswordExposureWarning(t *testing.T) {
 	// An obviously fake value: the point of the assertions below is that this
 	// string never reaches the log, so it must be recognisable if it ever does.
 	const fakePassword = "NOT-A-REAL-PASSWORD"
 
+	// Markers that tell the two warnings apart. A reader must be able to see
+	// which failure applies, so the tests assert on the distinguishing phrase
+	// rather than on "a warning happened".
+	const (
+		cleartextMarker = "sent in clear"
+		unverifiedCert  = "winRM.caCertPath"
+	)
+
 	configWithWinRM := func(settings string) string {
 		return `
 apiVersion: launchpad.mirantis.com/mke/v1.6
@@ -538,12 +546,14 @@ spec:
 	tests := []struct {
 		name     string
 		settings string
-		wantWarn bool
+		// want is the phrase the single warning must contain, or "" when no
+		// warning may be emitted at all.
+		want string
 	}{
 		{
-			name:     "insecure https with an inline password warns",
+			name:     "insecure https with a password warns about the certificate",
 			settings: "        useHTTPS: true\n        insecure: true\n        password: " + fakePassword + "\n",
-			wantWarn: true,
+			want:     unverifiedCert,
 		},
 		{
 			// port 5986 implies TLS without an explicit useHTTPS: rig's
@@ -552,29 +562,45 @@ spec:
 			// flag is already true by the time Validate runs. This case
 			// locks that in: the warning must not depend on the user
 			// having spelled useHTTPS out.
-			name:     "insecure with implicit https via port 5986 warns",
+			name:     "insecure with implicit https via port 5986 warns about the certificate",
 			settings: "        port: 5986\n        insecure: true\n        password: " + fakePassword + "\n",
-			wantWarn: true,
+			want:     unverifiedCert,
+		},
+		{
+			// Added by user ruling, beyond the ticket's two named findings.
+			// rig installs no transport decorator for plain HTTP without
+			// NTLM, so go-winrm falls back to SetBasicAuth and the password
+			// crosses the network with no encryption.
+			name:     "plain http with a password warns about cleartext",
+			settings: "        useHTTPS: false\n        password: " + fakePassword + "\n",
+			want:     cleartextMarker,
+		},
+		{
+			name:     "plain http with a password and insecure warns about cleartext, not the certificate",
+			settings: "        useHTTPS: false\n        insecure: true\n        password: " + fakePassword + "\n",
+			want:     cleartextMarker,
+		},
+		{
+			// NTLM negotiates its own message-level encryption, so the
+			// password is not cleartext even over plain HTTP.
+			name:     "plain http with ntlm does not warn",
+			settings: "        useHTTPS: false\n        useNTLM: true\n        password: " + fakePassword + "\n",
+			want:     "",
 		},
 		{
 			name:     "insecure https without a password does not warn",
 			settings: "        useHTTPS: true\n        insecure: true\n",
-			wantWarn: false,
+			want:     "",
 		},
 		{
-			name:     "verified https with an inline password does not warn",
+			name:     "verified https with a password does not warn",
 			settings: "        useHTTPS: true\n        insecure: false\n        password: " + fakePassword + "\n",
-			wantWarn: false,
-		},
-		{
-			name:     "insecure without https does not warn",
-			settings: "        useHTTPS: false\n        insecure: true\n        password: " + fakePassword + "\n",
-			wantWarn: false,
+			want:     "",
 		},
 		{
-			name:     "plain winrm host does not warn",
+			name:     "host with no password does not warn",
 			settings: "",
-			wantWarn: false,
+			want:     "",
 		},
 	}
 
@@ -598,20 +624,28 @@ spec:
 				}
 			}
 
-			if !tt.wantWarn {
+			if tt.want == "" {
 				require.Empty(t, warnings)
 				return
 			}
 
 			require.Len(t, warnings, 1)
 			require.Contains(t, warnings[0], "10.0.0.1")
-			require.Contains(t, warnings[0], "winRM.insecure")
-			require.Contains(t, warnings[0], "winRM.caCertPath")
+			require.Contains(t, warnings[0], tt.want)
+
+			// The two warnings must stay distinguishable: a host is told
+			// either that its password crosses the wire unencrypted, or that
+			// its peer is unauthenticated, never a blend of the two.
+			other := cleartextMarker
+			if tt.want == cleartextMarker {
+				other = unverifiedCert
+			}
+			require.NotContains(t, warnings[0], other)
 		})
 	}
 }
 
-func TestInsecureWinRMWarningIsPerHost(t *testing.T) {
+func TestWinRMPasswordExposureWarningIsPerHost(t *testing.T) {
 	hook := logrustest.NewGlobal()
 	defer hook.Reset()
 
@@ -635,8 +669,7 @@ spec:
       winRM:
         address: 10.0.0.2
         user: Administrator
-        useHTTPS: true
-        insecure: true
+        useHTTPS: false
         password: ALSO-NOT-REAL
     - role: worker
       winRM:
@@ -653,9 +686,14 @@ spec:
 		}
 	}
 
+	// One warning per affected host, each diagnosing that host's own
+	// transport: 10.0.0.1 has an unverified certificate, 10.0.0.2 sends the
+	// password in clear, and 10.0.0.3 has no password so is not reported.
 	require.Len(t, warnings, 2)
 	require.Contains(t, warnings[0], "10.0.0.1")
+	require.Contains(t, warnings[0], "winRM.caCertPath")
 	require.Contains(t, warnings[1], "10.0.0.2")
+	require.Contains(t, warnings[1], "sent in clear")
 }
 
 // Just a small helper to load the config struct from yaml to get defaults etc. in place.

From 1df53c93e9815b623b5b9bcde390ba3499b86705 Mon Sep 17 00:00:00 2001
From: James Nesbitt <jnesbitt@mirantis.com>
Date: Thu, 5 Nov 2026 14:34:06 +0200
Subject: [PATCH 5/5] [PRODENG-3935] Reject WinRM hosts without a complete
 credential

Added by user ruling, beyond the two findings named in the ticket.

ClusterConfig.Validate now runs rig's CompositeConfig.Validate for every
host. rig already requires a user and a password together
(protocol/winrm/config.go:66-68, and the SSH equivalent), but launchpad
never reached that guard: it connects through CompositeConfig.Connection,
which does not validate, so the check was dead code on this path.

This closes a hole in the warning shipped earlier on this branch. The
warning fires where the password survives, and was silent where it vanished
entirely: an empty Password reads as "no password configured", so a value
truncated during YAML parsing produced no warning and launchpad then sent an
HTTP Basic header with an empty password, measured as
scheme=Basic user_len=13 password_len=0 against a server whose certificate
it had not verified.

Two configuration shapes that load today stop loading. A winRM host with a
user and no password, which includes a host relying on the Administrator
default, now fails with "user and password must both be set for password
authentication". A host entry with no connection block fails with "no
protocol configuration". Neither could authenticate before; the observable
change is load-time rejection rather than connect-time failure.
Certificate authentication is unaffected.

TestHostWinRMDefaults gains a password rather than having its assertion
inverted. git log -L shows the function predates the rig refactor, so its
require.NoError was written when Validate did not consult connection
configuration and never asserted anything about authentication. Inverting it
would convert a defaults test into a validation test and drop the default
coverage.

.changes/PRODENG-3935.md records the breaking change, using the fragment
convention from PRODENG-3595.

Written by AI: claude-opus-5

Signed-off-by: James Nesbitt <jnesbitt@mirantis.com>
---
 .changes/PRODENG-3935.md               |   9 ++
 pkg/product/mke/config/cluster.go      |  35 +++++++
 pkg/product/mke/config/cluster_test.go | 138 +++++++++++++++++++++++--
 3 files changed, 174 insertions(+), 8 deletions(-)
 create mode 100644 .changes/PRODENG-3935.md

diff --git a/.changes/PRODENG-3935.md b/.changes/PRODENG-3935.md
new file mode 100644
index 000000000..629ec550e
--- /dev/null
+++ b/.changes/PRODENG-3935.md
@@ -0,0 +1,9 @@
+**Breaking: WinRM hosts without a complete credential are now rejected at config load.**
+
+`launchpad` now runs rig's own connection validation for every host when the configuration is loaded. A `winRM` host with a `user` but no `password`, or with a password that is empty after environment substitution, now fails with `user and password must both be set for password authentication` instead of loading. A host entry with no connection block at all fails with `no protocol configuration`. Certificate authentication (`certPath` plus `keyPath`) is unaffected.
+
+Such a configuration could not authenticate before this change either — it sent an HTTP Basic header with an empty password and failed at connect. The change is that it now fails at load, with a message naming the cause.
+
+Rejection was chosen over substituting a default or warning and continuing, because the failure it replaces was silent: a password that vanished during YAML parsing left launchpad authenticating as the configured user with **no password at all**, against a host whose TLS certificate it may also not have verified. A warning would have been missed in exactly the runs where it mattered.
+
+If you are affected, supply the password. Note that values are substituted into the configuration and then parsed as YAML, so a literal `$` must be written `$$` and a value containing an unquoted `#` is truncated at the `#` — which is how a password silently becomes empty. Full guidance: [Keeping credentials out of launchpad.yaml](https://github.com/Mirantis/launchpad/blob/main/docs/usage/getting-started.md).
diff --git a/pkg/product/mke/config/cluster.go b/pkg/product/mke/config/cluster.go
index 371e32cbc..5b0430d35 100644
--- a/pkg/product/mke/config/cluster.go
+++ b/pkg/product/mke/config/cluster.go
@@ -52,6 +52,41 @@ func (c *ClusterConfig) Validate() error {
 	if err := validator.Struct(c); err != nil {
 		return fmt.Errorf("cluster config validation failed: %w", err)
 	}
+
+	if err := c.validateConnections(); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// validateConnections runs rig's own connection validation for every host.
+//
+// rig already enforces that a password-authenticated connection has both a
+// user and a password (protocol/winrm/config.go:66-68 and the SSH
+// equivalent), but launchpad never reached that guard: it connects through
+// CompositeConfig.Connection(), which does not validate, so the check was
+// dead code on this path. Without it a password that is present in the file
+// but empty by the time it is parsed -- a value YAML-truncated to nothing,
+// for instance -- authenticated as the configured user with no password at
+// all, with no warning and no error.
+//
+// This also closes the gap in warnWinRMPasswordExposure, which fires where
+// the password survives and was silent where it vanished entirely.
+func (c *ClusterConfig) validateConnections() error {
+	if c.Spec == nil {
+		return nil
+	}
+
+	for _, h := range c.Spec.Hosts {
+		if h == nil {
+			continue
+		}
+		if err := h.Validate(); err != nil {
+			return fmt.Errorf("host connection configuration is invalid: %w", err)
+		}
+	}
+
 	return nil
 }
 
diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go
index ec3a9a0aa..e416c5b91 100644
--- a/pkg/product/mke/config/cluster_test.go
+++ b/pkg/product/mke/config/cluster_test.go
@@ -447,9 +447,15 @@ spec:
       winRM:
         address: 10.0.0.1
         user: User
+        password: NOT-A-REAL-PASSWORD
 `
 	c := loadYaml(t, data)
 
+	// The password is here only so the host has a complete credential for
+	// Validate to accept. The assertions below are about default
+	// application, which is what this test is for; inverting it to expect a
+	// validation error would convert a defaults test into a validation test
+	// and drop that coverage.
 	require.NoError(t, c.Validate())
 
 	require.Equal(t, c.Spec.Hosts[0].WinRM.User, "User")
@@ -549,6 +555,11 @@ spec:
 		// want is the phrase the single warning must contain, or "" when no
 		// warning may be emitted at all.
 		want string
+		// wantRejected marks a host that Validate now refuses outright,
+		// because rig requires a user and a password together. Such a host
+		// must also emit no warning: the exposure warning describes a
+		// password that reaches the wire, and there is no password here.
+		wantRejected bool
 	}{
 		{
 			name:     "insecure https with a password warns about the certificate",
@@ -588,9 +599,10 @@ spec:
 			want:     "",
 		},
 		{
-			name:     "insecure https without a password does not warn",
-			settings: "        useHTTPS: true\n        insecure: true\n",
-			want:     "",
+			name:         "insecure https without a password is rejected and does not warn",
+			settings:     "        useHTTPS: true\n        insecure: true\n",
+			want:         "",
+			wantRejected: true,
 		},
 		{
 			name:     "verified https with a password does not warn",
@@ -598,9 +610,10 @@ spec:
 			want:     "",
 		},
 		{
-			name:     "host with no password does not warn",
-			settings: "",
-			want:     "",
+			name:         "host with no password is rejected and does not warn",
+			settings:     "",
+			want:         "",
+			wantRejected: true,
 		},
 	}
 
@@ -610,7 +623,12 @@ spec:
 			defer hook.Reset()
 
 			c := loadYaml(t, configWithWinRM(tt.settings))
-			require.NoError(t, c.Validate())
+			err := c.Validate()
+			if tt.wantRejected {
+				require.Error(t, err, "a host with a user and no password must not validate")
+			} else {
+				require.NoError(t, err)
+			}
 
 			var warnings []string
 			for _, entry := range hook.AllEntries() {
@@ -675,6 +693,8 @@ spec:
       winRM:
         address: 10.0.0.3
         user: Administrator
+        useHTTPS: true
+        password: SAFE-NOT-REAL
 `
 	c := loadYaml(t, data)
 	require.NoError(t, c.Validate())
@@ -688,7 +708,7 @@ spec:
 
 	// One warning per affected host, each diagnosing that host's own
 	// transport: 10.0.0.1 has an unverified certificate, 10.0.0.2 sends the
-	// password in clear, and 10.0.0.3 has no password so is not reported.
+	// password in clear, and 10.0.0.3 uses verified TLS so is not reported.
 	require.Len(t, warnings, 2)
 	require.Contains(t, warnings[0], "10.0.0.1")
 	require.Contains(t, warnings[0], "winRM.caCertPath")
@@ -740,3 +760,105 @@ func getAllErrorFields(err error) []string {
 
 	return fields
 }
+
+// TestEmptyWinRMPasswordIsRejected covers the defect the exposure warning
+// could not see. The warning fires where the password survives; it was silent
+// where the password vanished entirely, because an empty Password reads as
+// "no password configured". rig has the right guard but launchpad never
+// reached it, connecting through CompositeConfig.Connection() rather than
+// Validate(), so such a host authenticated as the configured user with no
+// password at all.
+func TestEmptyWinRMPasswordIsRejected(t *testing.T) {
+	tests := []struct {
+		name        string
+		host        string
+		wantErr     bool
+		wantWarning bool
+	}{
+		{
+			// The YAML-truncation shape: a password beginning with "#" is
+			// consumed as a comment during substitution, so the key parses
+			// as null and the field is empty by the time Validate runs.
+			name:    "password present in the file but empty after parsing",
+			host:    "        address: 10.0.0.1\n        user: Administrator\n        password:\n",
+			wantErr: true,
+		},
+		{
+			name:    "explicit empty string password",
+			host:    "        address: 10.0.0.1\n        user: Administrator\n        password: \"\"\n",
+			wantErr: true,
+		},
+		{
+			name:    "no winRM password key at all",
+			host:    "        address: 10.0.0.1\n        user: Administrator\n",
+			wantErr: true,
+		},
+		{
+			name:        "a real password still validates and still warns about cleartext",
+			host:        "        address: 10.0.0.1\n        user: Administrator\n        password: NOT-A-REAL-PASSWORD\n",
+			wantErr:     false,
+			wantWarning: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			hook := logrustest.NewGlobal()
+			defer hook.Reset()
+
+			data := `
+apiVersion: launchpad.mirantis.com/mke/v1.6
+kind: mke
+spec:
+  mcr:
+    channel: stable
+  mke:
+    version: 3.3.7
+  hosts:
+    - role: manager
+      winRM:
+` + tt.host
+
+			c := loadYaml(t, data)
+			err := c.Validate()
+
+			if tt.wantErr {
+				require.Error(t, err, "an empty password must be rejected, not used to authenticate")
+				require.ErrorContains(t, err, "user and password must both be set")
+			} else {
+				require.NoError(t, err)
+			}
+
+			var warned bool
+			for _, entry := range hook.AllEntries() {
+				if entry.Level == logrus.WarnLevel {
+					warned = true
+				}
+			}
+			require.Equal(t, tt.wantWarning, warned,
+				"the warning describes a password that reaches the wire; an absent one has nothing to expose")
+		})
+	}
+}
+
+// TestHostWithNoProtocolIsRejected covers the other shape rig's validation
+// catches: a host entry with no connection block at all previously validated
+// and failed later at connect time.
+func TestHostWithNoProtocolIsRejected(t *testing.T) {
+	data := `
+apiVersion: launchpad.mirantis.com/mke/v1.6
+kind: mke
+spec:
+  mcr:
+    channel: stable
+  mke:
+    version: 3.3.7
+  hosts:
+    - role: manager
+`
+	c := loadYaml(t, data)
+	err := c.Validate()
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "no protocol configuration")
+}