[james-nesbitt]

What

winrm now refuses a password on a transport that does not protect it, and insecure is widened from "accept invalid TLS certificates" to "do not refuse an unprotected transport".

Config	Before	After
password, plain HTTP	connects	ErrValidationFailed
password + useNTLM, plain HTTP	connects	ErrValidationFailed
password, plain HTTP, insecure	connects	connects
password, useHTTPS (or port 5986)	connects	connects
password, useHTTPS + insecure	connects	connects
certPath/keyPath, useHTTPS	connects	connects
certPath/keyPath, plain HTTP	cert silently ignored	ErrValidationFailed

insecure carries the decision because both readings describe the same caller: one who has accepted that the transport need not protect what it carries. On plain HTTP it previously did nothing, so nobody who set it for TLS gets the new meaning by surprise. Its schema description now states both effects.

Where it is enforced

Nothing in rig calls a protocol config's Validate() on the connection path, so the rule runs in three places:

• Config.Validate() (and through it CompositeConfig.Validate())
• NewConnection, after SetDefaults, so rig.NewClient(WithConnectionFactory(...)) fails before any request
• Connection.Connect, because Connection.Config is exported and mutable and a Connection can be built without NewConnection

On the connection path the error wraps both ErrValidationFailed and ErrNonRetryable.

Port 5986 counts as HTTPS everywhere (validation and the endpoint Connect builds), matching what SetDefaults does, so a config validated or connected before SetDefaults is judged by the transport it will actually use.

NTLM is not an exemption

NTLM never sends the password itself, but the WinRM client does not seal the session: commands, output and any secret inside them stay readable, and the captured challenge response can be cracked offline or relayed. useNTLM therefore does not change the outcome, and gets its own error message rather than the basic-auth one about the password being sent.

Breaking changes

• A config with a password, no useHTTPS (or port 5986) and no insecure now fails instead of connecting. Setting insecure: true keeps the previous behaviour.
• certPath/keyPath without HTTPS now fails validation. The client certificate is only presented in a TLS handshake, so over plain HTTP it was silently ignored and the connection fell back to basic auth. The only config that worked before and fails now is certificate + password + insecure over plain HTTP, which authenticated with the password and never used the certificate.

Drive-by

fix(log): inject the logger into a Config pointer, not a copy: InjectLoggerTo was handed a Config value, whose method set excludes the pointer-receiver SetLogger, so the injection silently did nothing in both winrm and ssh. No symptom today because no Config method logs.

Also: Config.Connection() no longer returns a typed-nil *Connection inside a non-nil interface on error.

Testing

• TestValidateRejectsExposedPassword: the matrix above, including port-5986 cases validated without SetDefaults, the NTLM message, and that no error repeats the password
• TestConnectionRejectsExposedPassword: Config.Connection() refuses with both sentinels
• TestConnectRefusesPasswordOverPlainHTTP: password set after NewConnection, and a directly constructed Connection; the stub server receives no requests
• TestNewEndpointScheme: endpoint scheme for 5985, 5986 without useHTTPS, and custom ports
• TestClientWithConnectionFactoryRefusesWinRMPasswordOverHTTP: through rig.NewClient, no request reaches the stub server
• TestNewConnectionInjectsLoggerIntoConfig in protocol/ssh and protocol/winrm

Written by AI: claude-opus-5

[Copilot]

🟡 Changes recommended

The security guidance incorrectly claims NTLM encrypts SOAP bodies, so its recommended remedy can leave commands and output unencrypted.

4 open issues

• NTLM recommendation does not encrypt WinRM SOAP traffic · New
• Do not warn about passwords when certificate authentication is selected · New
• Cancelled connection tests can still dial a real host · New
• Warning rationale still claims Config loses the supplied logger · New

What changed in this PR

This PR adds advisory warnings for potentially exposed WinRM passwords without rejecting existing configurations. It also fixes configuration logger injection in both WinRM and SSH.

Changes:

• Warn once per connection for password authentication over plain HTTP without NTLM or TLS with certificate verification disabled.
• Inject loggers into configuration pointers rather than values.
• Add tests for warning behavior, password redaction, and logger wiring.

File	Description
protocol/​winrm/​connection.go	Emits connection-time warnings once and fixes logger injection.
protocol/​winrm/​config.go	Classifies password-exposure configurations and supplies warning messages.
protocol/​winrm/​config_test.go	Tests exposure classification, warning behavior, and logger injection.
protocol/​ssh/​logger_test.go	Tests logger injection into SSH configuration and connection objects.
protocol/​ssh/​connection.go	Passes the configuration pointer to logger injection.

_{Review effort: Balanced}

[kke]

Maybe go a step further and make it an error? insecure defaults to false, so how about expanding it from "verify the certificate" to "don't send passwords over unprotected transports". Plain HTTP with basic auth would then be rejected unless insecure: true is set.

[james-nesbitt]

Done — made it an error, and dropped the warning machinery entirely. insecure now reads as "do not refuse to expose the credential" rather than "accept invalid TLS certificates", so plain HTTP with a password is rejected unless insecure: true is set. It is three lines in validateAuth alongside the other auth-shape rules; passwordExposure, the sync.Once and the Connect-time hook are gone.

The only reason it was a warning in the first place was to avoid a breaking behaviour change — that was the whole of the rationale, and it is not a good enough one. A warning arrives at the moment the connection is about to be made anyway, which is too late for the caller to act on it, and the cost of saying so was a function returning a message string plus the plumbing to decide when to emit it.

While reworking it I also dropped NTLM's exemption. The previous revision excused NTLM over plain HTTP on the grounds that it encrypts at the message level; it does not — the client authenticates with NTLM and still sends the SOAP envelopes unencrypted, so commands and their output stay readable. useNTLM no longer changes the outcome.

One thing worth flagging, since it is now arguably out of scope: the branch still carries the InjectLoggerTo(&cfg, ...) fix as its first commit, and the password check no longer depends on it. That fix existed because the warning needed a logger on the Config. Nothing on the branch calls Log() any more, so it is now an unrelated drive-by: InjectLoggerTo is handed a Config value whose method set excludes the pointer-receiver SetLogger, the type assertion in log.InjectLogger fails, and the injection silently does nothing in both winrm and ssh. There is no symptom today because no Config method logs anything — it is only a trap for whoever adds the first one.

I have kept it as a separate commit so it is trivial to drop if you would rather it went in on its own, or not at all. Happy either way.

Written by AI: claude-opus-5

[james-nesbitt]

@kke let me know if you'd like me to drop that commit with the log reference injection.

[Copilot]

🟡 Changes recommended

The connection path does not enforce validation, allowing passwords to be sent over plaintext HTTP without an explicit insecure opt-in.

2 open issues

• Validate connection security before sending HTTP requests · New
• Document both effects of Config.Insecure in the schema · New

4 resolved since last review

• NTLM recommendation does not encrypt WinRM SOAP traffic
• Cancelled connection tests can still dial a real host
• Do not warn about passwords when certificate authentication is selected
• Warning rationale still claims Config loses the supplied logger

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Mutable connection configuration can bypass the password-transport guard and send credentials over HTTP despite Insecure being false, while validation also incorrectly rejects port-inferred HTTPS configurations.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

Revalidate password transport security before connecting

protocol/​winrm/​connection.go:58

Constructing an HTTP connection without a password, assigning conn.Password, and calling Connect bypasses this check and sends the password over HTTP with Insecure false. The exported, mutable Connection.Config also allows direct construction that skips NewConnection. Recheck validatePasswordTransport at the start of Connect, wrapping both validation and non-retryable sentinels, so the protection applies to the configuration used for transmission.

Validate authentication using normalized HTTPS settings

protocol/​winrm/​config.go:90

With Port: 5986, a user/password and UseHTTPS left false, Config.Validate() and CompositeConfig.Validate() reject a configuration that NewConnection accepts as HTTPS after applying defaults. Validate authentication against the same normalized configuration so both this guard and the certificate guard at line 69 honor port-inferred HTTPS. Add a regression case that calls validation without first applying defaults.

2 resolved since last review

• Validate connection security before sending HTTP requests
• Document both effects of Config.Insecure in the schema

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

Missing endpoint-construction coverage for directly constructed connections could let a regression send passwords over HTTP on port 5986 without failing tests.

1 open issue

• Add endpoint scheme tests for directly constructed connections · New

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed to evaluate the additional HTTPS requirement for certificate configurations and the NTLM security rationale’s inaccurate claim of direct password disclosure.

No open issues 🎉

1 resolved since last review

• Add endpoint scheme tests for directly constructed connections

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Human assessment is needed because the normal connection path can bypass certificate/HTTPS validation and silently fall back to password authentication.

No open issues 🎉

_{Review effort: Balanced}