diff --git a/.changes/PRODENG-3935.md b/.changes/PRODENG-3935.md new file mode 100644 index 00000000..629ec550 --- /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/docs/usage/getting-started.md b/docs/usage/getting-started.md index b59c6c48..27bdf94b 100644 --- a/docs/usage/getting-started.md +++ b/docs/usage/getting-started.md @@ -29,6 +29,94 @@ 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. +- **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 + +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 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 0060b976..948b9d2c 100644 --- a/pkg/product/mke/config/cluster.go +++ b/pkg/product/mke/config/cluster.go @@ -12,6 +12,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. @@ -62,6 +63,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.warnWinRMPasswordExposure() + validator := validator.New(validator.WithRequiredStructEnabled()) validator.RegisterStructValidation(roleChecks, ClusterSpec{}) // Defence in depth, not the control: the command sites quote. Validation @@ -82,9 +85,110 @@ 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 } +// 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 + } + + for _, h := range c.Spec.Hosts { + if h == nil || h.WinRM == nil { + continue + } + wrm := h.WinRM + 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 + } + + // 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. + 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, + ) + } + } +} + func isValidMCRChannel(fl validator.FieldLevel) bool { return common.IsValidMCRChannel(fl.Field().String()) } diff --git a/pkg/product/mke/config/cluster_test.go b/pkg/product/mke/config/cluster_test.go index b88ef691..bd9af184 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" @@ -28,6 +29,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" ) @@ -444,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") @@ -510,6 +519,203 @@ spec: } +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 +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 + // 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", + settings: " useHTTPS: true\n insecure: true\n password: " + fakePassword + "\n", + want: unverifiedCert, + }, + { + // 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 about the certificate", + settings: " port: 5986\n insecure: true\n password: " + fakePassword + "\n", + 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 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", + settings: " useHTTPS: true\n insecure: false\n password: " + fakePassword + "\n", + want: "", + }, + { + name: "host with no password is rejected and does not warn", + settings: "", + want: "", + wantRejected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + hook := logrustest.NewGlobal() + defer hook.Reset() + + c := loadYaml(t, configWithWinRM(tt.settings)) + 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() { + 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) + } + } + + 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], 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 TestWinRMPasswordExposureWarningIsPerHost(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: false + password: ALSO-NOT-REAL + - role: worker + winRM: + address: 10.0.0.3 + user: Administrator + useHTTPS: true + password: SAFE-NOT-REAL +` + 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) + } + } + + // 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 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") + 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. func loadYaml(t *testing.T, data string) *ClusterConfig { c := &ClusterConfig{} @@ -555,6 +761,108 @@ 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") +} + // TestImageRepoRejectsShellMetacharacters covers AC-3. spec.*.imageRepo is // expanded by envsubst and interpolated into image references that become // docker command arguments on every host, so control of one CI environment