[james-nesbitt]

What

ClusterConfig.Validate warns once per host that combines winRM.useHTTPS: true, winRM.insecure: true and a non-empty winRM.password. docs/usage/getting-started.md gains two sections: sourcing credentials from the environment via envsubst, and the risk of winRM.insecure.

Why

PRODENG-3935, finding secf_ad4234c93364c313eba5a63f. winRM.insecure is rig's accept-any-certificate option (protocol/winrm/config.go:22), so the channel is encrypted but the peer is unauthenticated. An on-path attacker can impersonate the host, recover the password and read every command sent to it.

How

• Validate is the seam, not Host.UnmarshalYAML: Validate runs once per config load (pkg/product/mke/mke.go:37), while UnmarshalYAML re-runs on migrations, Init round-trips and tests, emitting duplicate warnings for a single apply. The existing WinRM port fix at host.go:169 lives in UnmarshalYAML because it must mutate the struct before rig reads it; a log line has no such constraint.
• Trigger is UseHTTPS && Insecure && Password != "", the finding as written. Field names taken from protocol/winrm/config.go:20-22, not from the ticket's YAML spelling.
• Warning, not error: an operator who accepts the risk can still apply.
• The message contains the host address and the two setting names only — never the credential, its length, or a digest.

Testing

• TestInsecureWinRMWarning (5 cases) and TestInsecureWinRMWarningIsPerHost: fail before the change, pass after. Negative cases cover insecure+HTTPS with no password, HTTPS+password with insecure: false, insecure+password without HTTPS, and a bare host. Every case asserts the password string never reaches the log.
• make lint: 0 issues. make unit-test: all packages ok.
• Built make local and ran apply against a throwaway config with a fake password. Warning printed at config load, before the Open Remote Connection phase, with no credential in it.
• envsubst behaviour in the new guidance verified against the binary: unset variable fails with variable ${WINRM_PASSWORD} not set; abc$def fails with variable ${def} not set; abc$5986 is left literal.

Links

https://mirantis.jira.com/browse/PRODENG-3935

Checklist

• Commit signed
• Lint and unit tests pass
• Change exercised against a built binary
• Review findings addressed
• Completes the ticket

Acceptance criteria status

All three criteria are satisfied.

Criterion 2 (envsubst guidance) and criterion 3 (warning at config load) are delivered by this PR.

Criterion 1 is complete, and was settled outside this PR:

• Working-tree file: scrubbed. The credential-bearing launchpad.yaml was untracked in the root checkout (.gitignore:19 **/launchpad.yaml) and never reachable from a worktree. Its four credential values were replaced in place with ${WINRM_PASSWORD} and ${MKE_ADMIN_PASSWORD} placeholders, the same pattern the new documentation teaches.
• Rotation: assessed and not required. The hosts referenced by that file no longer exist, so the criterion's "rotated if those hosts existed" clause is satisfied vacuously. This is a user determination, not an inference drawn here.
• Git history: clean, no rewrite needed. The root launchpad.yaml was never committed on any branch (git log --all --diff-filter=A returns nothing for it). Two other paths existed historically and are both removed: test/launchpad.yaml (added in f7c06ed) and examples/footloose/launchpad.yaml (added in cae9335). Each holds exactly one credential key, --admin-password=, carrying the long-standing orcaorcaorca MKE test string against 127.0.0.1 loopback addresses in footloose fixtures - a dummy consistent with those examples' own conventions, not a real credential. The private key those fixtures reference was never committed either.

Review findings

• 6b302cc: environment-sourced values are substituted into the configuration text and then re-parsed as YAML, so a value containing # or : is truncated or aborts the load. The guidance documents this, replacing an earlier claim that such values are substituted verbatim.
• 6b302cc: the report that port: 5986 without an explicit useHTTPS escapes the warning did not reproduce. creasty/defaults invokes rig's winrm.Config.SetDefaults from Host.UnmarshalYAML, so UseHTTPS is already true when Validate runs. A test case pins that behaviour.
• 80216f5: the warning previously advised keeping the password out of the file by using an environment variable. Substitution at pkg/config/config.go:79 precedes parsing, so that advice also reached users who had already done exactly that. The message now leads with the certificate problem and names winRM.caCertPath (confirmed against protocol/winrm/config.go:24).
• 80216f5: the tests asserted the password never appears in entry.Message; entry.Data is now asserted too, closing a leak route through structured log fields.

Scope added by user ruling

The plain-HTTP warning is a widening beyond the two findings named in the ticket, added by user ruling. A reviewer checking acceptance criteria against the diff will not find it in the ticket text; it is intentional, not scope creep.

winRM.useHTTPS and winRM.useNTLM both default to false (protocol/winrm/config.go:21,23). rig installs a transport decorator only for NTLM, or for HTTPS with a loaded client certificate (connection.go:242-250), so with the defaults neither applies and go-winrm's default transport calls req.SetBasicAuth (masterzen/winrm/http.go:96). The password therefore crosses the network base64-encoded in an Authorization header with no encryption — arguably worse than the unverified-certificate case the ticket named, since there is no TLS at all.

The two exposures are reported as separate messages rather than blended into one, because the remedies differ:

Configuration	Message
useHTTPS: false, no NTLM, password set	the password is sent in clear
useHTTPS: true plus insecure: true, password set	the certificate is not verified
useHTTPS: false, useNTLM: true	no warning; NTLM negotiates message-level encryption

AC-1 is unaffected by this addition and remains complete.

Empty-password rejection — added by user ruling

The third widening beyond this ticket's two named findings, added by user ruling and not present in the ticket text. A reviewer checking acceptance criteria against the diff will not find it there; it is intentional.

The hole it closes. The warning this PR already shipped fires where the password survives. It was silent where the password vanished entirely, because an empty Password reads as "no password configured". A value truncated during YAML parsing — a password beginning with #, consumed as a comment — produced no warning, and launchpad then authenticated as the configured user with no password at all. Measured against a local TLS listener: scheme=Basic user_len=13 password_len=0, against a server whose certificate it had not verified.

ClusterConfig.Validate now runs rig's CompositeConfig.Validate 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 it: it connects through CompositeConfig.Connection(), which does not validate, so the guard was dead code on this path.

Configurations that load today will stop loading. Two shapes:

Shape	Now fails with
winRM host with a user and no password — including one relying on the Administrator default	user and password must both be set for password authentication
host entry with no connection block	no protocol configuration

Neither could authenticate before this change: the first sent an HTTP Basic header with an empty password and failed at connect. The observable difference is load-time rejection instead of connect-time failure. Certificate authentication (certPath plus keyPath) is unaffected and still validates.

No passwordless WinRM authentication mode is lost. validateAuth is the complete exemption set, and there is no Kerberos, Negotiate or credential-from-environment path in rig's WinRM protocol; NTLM uses the same user and password.

TestHostWinRMDefaults gained a password rather than an inverted assertion. git log -L on that function shows it predates the rig refactor, so its require.NoError was written when Validate did not consult connection configuration at all and never asserted anything about authentication. Inverting it to expect an error would convert a defaults test into a validation test and drop the default coverage it exists for.

Context, not a change in this PR: windows_password defaults to "" in examples/terraform/aws-simple and aws-complete, so our own examples can generate a WinRM host with an empty password — exactly what this validator now rejects. The generator is fixed separately in #689. A reviewer seeing only this diff could not otherwise tell whether we ship configuration we reject.

AC-1 is not reopened. It remains complete per the rotation ruling.

The breaking change is recorded in .changes/PRODENG-3935.md, using the fragment convention introduced in #692.

Testing gaps

make functional-test was not run: it fails fleet-wide on an unauthenticated GitHub API rate limit and reproduces on untouched main at ce4a91d. The Aikido scan gate was not run; it is parked by a separate ruling and blocks nothing here.

Written by AI: claude-opus-5

LOCAL VALIDATION, not a CI smoke result

Run on 2026-11-06 against the binary built from bfa5ee1 in this worktree. Re-run after merging main at e095a0b (PRODENG-3931), which changed ClusterConfig.Validate() — the function both of these cases exercise. The earlier run at 1df53c9 was stale and has been replaced rather than carried forward. No AWS, no terraform, no hosts. Config load completes in pkg/config before any host is contacted, so these cases are decided and the process exits before provisioning. CI's smoke harness cannot produce them: it asserts an apply succeeds, and two of these three are reject paths.

The distinction asserted throughout is failed at load versus failed at connect — not merely "it errored". Accept cases use the unreachable address 198.51.100.10, so reaching Running phase proves the config was accepted.

Case	Config	Result
Warning fires	useHTTPS: true + insecure: true + inline password	warning emitted once, then Running phase ×2 — accepted and warned
Empty-password fold	password #secret via ${V682PW}	rejected at load, no phase reached
Accept	verified TLS, real password	no warning, Running phase ×2

Verbatim, case 2:

FATA failed to load product config: failed to parse MKE configuration: failed to
validate cluster config: host connection configuration is invalid: validate
*winrm.Config: validation failed: user and password must both be set for
password authentication

Case 3 emitted no winRM.insecure line and no FATA, reaching ==> Running phase: Check For Upgrades and ==> Running phase: Open Remote Connection.

Written by AI: claude-opus-5

[Copilot]

🟡 Changes recommended

The insecure WinRM warning misses HTTPS implicitly enabled by port 5986, and the documentation omits YAML-escaping limitations that can alter environment-sourced passwords or cause parsing errors.

2 open issues

• Document YAML parsing limits for environment-sourced credentials · New
• Warn for insecure TLS using the port-based HTTPS default · New

What changed in this PR

This PR adds a configuration-time warning for risky WinRM settings in Launchpad and documents safer credential handling. It warns operators without blocking cluster operations.

Changes:

• Warns per host when HTTPS, insecure certificate verification, and a non-empty WinRM password are configured.
• Adds tests for warning conditions, per-host reporting, and password omission from logs.
• Documents environment-variable substitution and WinRM certificate risks.

File	Description
pkg/​product/​mke/​config/​cluster.go	Adds the WinRM warning during configuration validation.
pkg/​product/​mke/​config/​cluster_test.go	Tests warning behavior and credential-safe logging.
docs/​usage/​getting-started.md	Adds credential-sourcing and WinRM TLS guidance.

_{Review effort: Balanced}

[james-nesbitt]

Should this fix be considered a workaround?

Perhaps what we need is to submit an upstream change that better handles insecure winrm checking?

[james-nesbitt]

Partly, and the measurement changed the answer mid-investigation — so here is what is actually true rather than what we assumed.

The condition is invisible to rig today. protocol/winrm/config.go validates auth symmetry — it rejects a user without a password and a password without a user — but Insecure is never examined in Validate(), validateAuth() or anywhere else in that file. UseHTTPS && Insecure && Password != "" is accepted in silence, and the connection then authenticates with HTTP Basic over a TLS session whose peer was never verified.

But rig cannot carry the warning, and not for the reason we expected. protocol/winrm/connection.go calls options.InjectLoggerTo(cfg, ...) with cfg as a Config value, while the logger interface needs pointer receivers — so the type assertion fails and the injection is a silent no-op. Probed directly: after NewConnection, the connection has a logger and its embedded Config does not. A winrm.Config therefore never carries a logger at any point, for any rig caller, so a warning emitted from Validate() would go to the null logger for everybody — not just for us. The "make rig warn" shape fails on efficacy before any API argument starts.

So the warning stays here, and it is not a workaround: it is the only place the message can reach the operator.

What is worth sending upstream is the no-op logger injection itself, which looks like a plain bug independent of this ticket — a caller who injects a logger silently does not get one on the config. The same line exists at protocol/ssh/connection.go. That has been written: k0sproject/rig#516, which carries two commits, the insecure-transport warning and the logger pointer fix. It is currently a draft, parked at your request, not awaiting review.

One thing this PR is not a workaround for either: validateConnections() is the first place launchpad has ever called rig's own connection validation. rig already rejects a credential with an empty password; launchpad never reached that check, because it connects through CompositeConfig.Connection(), which does not validate. That is why a password destroyed by YAML parsing — one beginning with #, consumed as a comment — previously authenticated as the configured user with no password at all.

Written by AI: claude-opus-5