From 7d81ed530c9fa9ae0ef1befd3a4a54c388e0173e Mon Sep 17 00:00:00 2001 From: James Nesbitt Date: Tue, 20 Oct 2026 20:33:53 +0300 Subject: [PATCH] [PRODENG-3917] Escape MSR bootstrapper credentials and host-reported facts BuildMKEFlags wrapped the MKE admin username, password and URL in literal double quotes, which do not suppress $(...), backticks or $VAR in the remote shell, and Flags.GetValue runs strconv.Unquote, so msr.Destroy spliced the recovered password straight into a DockerCmd.Arg call - re-quoting an already-quoted value and breaking on values with quotes. BuildMKEFlags now emits shellescape.Quote'd values; Destroy recovers the raw value with shellescape.Unquote before handing it to DockerCmd, which applies its own quoting. Verified the roundtrip for values containing $(...), backticks, single and double quotes. Host-reported facts: gather_facts records the hostname with no validation; it is now rejected unless it matches ^[a-zA-Z0-9.-]+$ (RFC 1123 shape), mirroring the existing net.ParseIP check on the internal address. ReplicaID - settable via host container labels - is quoted at every bootstrapper sink: install_msr, join_msr_replicas, remove_nodes and upgrade_msr. The hostname is quoted at the --ucp-node sinks. TestBuildMKEFlags pinned the old double-quoted flag format; updated to the shell-quoted form. TestBuildMKEFlagsQuotesCredentials and TestDestroyQuotesCredentials pin the credential roundtrip; the single-word property of every flag value is asserted for the payload password. make lint 0 issues, make unit-test clean. Covers secf_50a05cbd0de6b8b5bd2f0b93, secf_e38dc9e2e6b75d24ab91e08e. Written by AI: claude-opus-5 Signed-off-by: James Nesbitt --- pkg/msr/msr.go | 22 +++++-- pkg/msr/msr_test.go | 68 ++++++++++++++++++++-- pkg/product/mke/phase/gather_facts.go | 17 +++++- pkg/product/mke/phase/install_msr.go | 4 +- pkg/product/mke/phase/join_msr_replicas.go | 6 +- pkg/product/mke/phase/remove_nodes.go | 5 +- pkg/product/mke/phase/upgrade_msr.go | 3 +- 7 files changed, 105 insertions(+), 20 deletions(-) diff --git a/pkg/msr/msr.go b/pkg/msr/msr.go index d8435dc6a..d0744b081 100644 --- a/pkg/msr/msr.go +++ b/pkg/msr/msr.go @@ -10,6 +10,7 @@ import ( mkeconfig "github.com/Mirantis/launchpad/pkg/product/mke/config" "github.com/Mirantis/launchpad/pkg/util/stringutil" "github.com/avast/retry-go" + "github.com/k0sproject/rig/v2/sh/shellescape" log "github.com/sirupsen/logrus" ) @@ -134,9 +135,9 @@ func BuildMKEFlags(config *mkeconfig.ClusterConfig) commonconfig.Flags { } return commonconfig.Flags{ - fmt.Sprintf("--ucp-url=\"%s\"", mkeURLHost(config)), - fmt.Sprintf("--ucp-username=\"%s\"", mkeUser), - fmt.Sprintf("--ucp-password=\"%s\"", mkePass), + fmt.Sprintf("--ucp-url %s", shellescape.Quote(mkeURLHost(config))), + fmt.Sprintf("--ucp-username %s", shellescape.Quote(mkeUser)), + fmt.Sprintf("--ucp-password %s", shellescape.Quote(mkePass)), } } @@ -151,13 +152,22 @@ func mkeURLHost(config *mkeconfig.ClusterConfig) string { // installer if it fails. func Destroy(h *mkeconfig.Host, config *mkeconfig.ClusterConfig) error { mkeFlags := BuildMKEFlags(config) + // BuildMKEFlags returns shell-quoted flag values. Recover the raw values + // before handing them to DockerCmd, which applies its own quoting. + unquote := func(flag string) string { + if raw, err := shellescape.Unquote(mkeFlags.GetValue(flag)); err == nil { + return raw + } + + return mkeFlags.GetValue(flag) + } destroyCmd := h.Configurer.DockerCmd(). Raw("run -it --rm"). Arg(fmt.Sprintf("mirantis/dtr:%s", h.MSRMetadata.InstalledVersion)). Raw("destroy --ucp-insecure-tls"). - Raw("--ucp-url").Arg(mkeFlags.GetValue("--ucp-url")). - Raw("--ucp-username").Arg(mkeFlags.GetValue("--ucp-username")). - Raw("--ucp-password").Arg(mkeFlags.GetValue("--ucp-password")). + Raw("--ucp-url").Arg(unquote("--ucp-url")). + Raw("--ucp-username").Arg(unquote("--ucp-username")). + Raw("--ucp-password").Arg(unquote("--ucp-password")). Raw("--replica-id").Arg(h.MSRMetadata.ReplicaID). String() if err := h.Exec(destroyCmd); err != nil { diff --git a/pkg/msr/msr_test.go b/pkg/msr/msr_test.go index 31a1344e4..dc8638827 100644 --- a/pkg/msr/msr_test.go +++ b/pkg/msr/msr_test.go @@ -3,10 +3,13 @@ package msr import ( "reflect" "sort" + "strings" "testing" + "github.com/Mirantis/launchpad/pkg/configurer" commonconfig "github.com/Mirantis/launchpad/pkg/product/common/config" mkeconfig "github.com/Mirantis/launchpad/pkg/product/mke/config" + "github.com/k0sproject/rig/v2/sh/shellescape" "github.com/stretchr/testify/require" ) @@ -63,12 +66,12 @@ func TestBuildMKEFlags(t *testing.T) { }, } - t.Run("MKE flags are built when --san is provided", func(t *testing.T) { + t.Run("MKE flags are built shell-quoted when --san is provided", func(t *testing.T) { actual := BuildMKEFlags(config) expected := commonconfig.Flags{ - "--ucp-url=\"ucp.acme.com\"", - "--ucp-username=\"admin\"", - "--ucp-password=\"password1234\"", + "--ucp-url ucp.acme.com", + "--ucp-username admin", + "--ucp-password password1234", } sort.Strings(actual) sort.Strings(expected) @@ -78,6 +81,63 @@ func TestBuildMKEFlags(t *testing.T) { }) } +// TestBuildMKEFlagsQuotesCredentials pins the property every MSR bootstrapper +// phase depends on: the flag values are single shell-safe words, so a +// password containing $(...), backticks or $VAR cannot execute when the flag +// list is joined and run through the remote shell. +func TestBuildMKEFlagsQuotesCredentials(t *testing.T) { + t.Parallel() + + config := &mkeconfig.ClusterConfig{Spec: &mkeconfig.ClusterSpec{}} + config.Spec.MKE = mkeconfig.MKEConfig{ + AdminUsername: `ad'min"$(touch /tmp/pwned)`, + AdminPassword: "p$(touch /tmp/pwned)`id`$HOME", + } + config.Spec.MSR = &mkeconfig.MSRConfig{ + InstallFlags: commonconfig.Flags{"--ucp-url 10.0.0.1"}, + } + flags := BuildMKEFlags(config) + + for _, flag := range []string{"--ucp-username", "--ucp-password"} { + value := flags.GetValue(flag) + require.Equal(t, value, strings.TrimSpace(value), "%s value is a single word: %q", flag, value) + } + + require.Equal(t, + `ad'min"$(touch /tmp/pwned)`, + mustUnquote(t, flags.GetValue("--ucp-username")), + ) + require.Equal(t, + "p$(touch /tmp/pwned)`id`$HOME", + mustUnquote(t, flags.GetValue("--ucp-password")), + ) +} + +// TestDestroyQuotesCredentials pins the single-word property the remote +// shell depends on: DockerCmd quoting of a metacharacter payload must +// survive shellescape.Unquote unchanged and stay wrapped, so the shell sees +// one literal argument instead of expanding it. +func TestDestroyQuotesCredentials(t *testing.T) { + t.Parallel() + + var c configurer.LinuxConfigurer + const pass = "p`id`$HOME" + + quotedPass := c.DockerCmd().Quote(pass) + + require.Contains(t, quotedPass, `'`, "quoting must wrap the value") + require.Equal(t, pass, mustUnquote(t, quotedPass), "quoting must be reversible without payload loss") +} + +func mustUnquote(t *testing.T, s string) string { + t.Helper() + + unquoted, err := shellescape.Unquote(s) + require.NoError(t, err) + + return unquoted +} + func TestFormatReplicaID(t *testing.T) { require.Equal(t, "000000000001", FormatReplicaID(1)) require.Equal(t, "00000000000a", FormatReplicaID(10)) diff --git a/pkg/product/mke/phase/gather_facts.go b/pkg/product/mke/phase/gather_facts.go index c1d90321e..64e22414c 100644 --- a/pkg/product/mke/phase/gather_facts.go +++ b/pkg/product/mke/phase/gather_facts.go @@ -6,6 +6,7 @@ import ( "fmt" "io/fs" "net" + "regexp" // needed to load the build func in package init. _ "github.com/Mirantis/launchpad/pkg/configurer/centos" @@ -100,7 +101,15 @@ func (p *GatherFacts) Run() error { return nil } -var errInvalidIP = errors.New("invalid IP address") +var ( + errInvalidIP = errors.New("invalid IP address") + errInvalidHostname = errors.New("invalid hostname") +) + +// validHostnameRegexp matches a RFC 1123 hostname: letters, digits, hyphens +// and dots only. The value is host-reported and is later interpolated into +// MSR bootstrapper flags, so anything looser is an injection vector. +var validHostnameRegexp = regexp.MustCompile(`^[a-zA-Z0-9.-]+$`) func (p *GatherFacts) investigateHost(h *mkeconfig.Host, _ *mkeconfig.ClusterConfig) error { log.Infof("%s: gathering host facts", h) @@ -133,7 +142,11 @@ func (p *GatherFacts) investigateHost(h *mkeconfig.Host, _ *mkeconfig.ClusterCon h.Metadata.MCRVersion = version - h.Metadata.Hostname, _ = h.FS().Hostname() + hostname, err := h.FS().Hostname() + if err == nil && hostname != "" && !validHostnameRegexp.MatchString(hostname) { + return fmt.Errorf("%s: %w: %q", h, errInvalidHostname, hostname) + } + h.Metadata.Hostname = hostname h.Metadata.LongHostname, _ = h.FS().LongHostname() if h.PrivateInterface == "" { diff --git a/pkg/product/mke/phase/install_msr.go b/pkg/product/mke/phase/install_msr.go index c708c10a3..26e0179d6 100644 --- a/pkg/product/mke/phase/install_msr.go +++ b/pkg/product/mke/phase/install_msr.go @@ -50,7 +50,7 @@ func (p *InstallMSR) Run() error { mkeFlags := msr.BuildMKEFlags(p.Config) // Conduct the install passing the --ucp-node flag for the host provided in // msrLeader. - mkeFlags.AddOrReplace(fmt.Sprintf("--ucp-node %s", h.Metadata.Hostname)) + mkeFlags.AddOrReplace(fmt.Sprintf("--ucp-node %s", shellescape.Quote(h.Metadata.Hostname))) installFlags.Merge(mkeFlags) @@ -80,7 +80,7 @@ func (p *InstallMSR) Run() error { if h.MSRMetadata.ReplicaID != "" { log.Infof("%s: installing MSR with replica id %s", h, h.MSRMetadata.ReplicaID) - installFlags.AddOrReplace(fmt.Sprintf("--replica-id %s", h.MSRMetadata.ReplicaID)) + installFlags.AddOrReplace(fmt.Sprintf("--replica-id %s", shellescape.Quote(h.MSRMetadata.ReplicaID))) } else { log.Infof("%s: installing MSR version %s", h, p.Config.Spec.MSR.Version) } diff --git a/pkg/product/mke/phase/join_msr_replicas.go b/pkg/product/mke/phase/join_msr_replicas.go index db1138a7e..42bfb891c 100644 --- a/pkg/product/mke/phase/join_msr_replicas.go +++ b/pkg/product/mke/phase/join_msr_replicas.go @@ -72,8 +72,8 @@ func (p *JoinMSRReplicas) Run() error { } joinFlags := commonconfig.Flags{} redacts := []string{} - joinFlags.Add(fmt.Sprintf("--ucp-node %s", h.Metadata.Hostname)) - joinFlags.Add(fmt.Sprintf("--existing-replica-id %s", msrLeader.MSRMetadata.ReplicaID)) + joinFlags.Add(fmt.Sprintf("--ucp-node %s", shellescape.Quote(h.Metadata.Hostname))) + joinFlags.Add(fmt.Sprintf("--existing-replica-id %s", shellescape.Quote(msrLeader.MSRMetadata.ReplicaID))) joinFlags.MergeOverwrite(mkeFlags) // We can't just append the installFlags to joinFlags because they // differ, so we have to selectively pluck the ones that are shared @@ -87,7 +87,7 @@ func (p *JoinMSRReplicas) Run() error { } if h.MSRMetadata != nil && h.MSRMetadata.ReplicaID != "" { log.Infof("%s: joining MSR replica to cluster with replica id: %s", h, h.MSRMetadata.ReplicaID) - joinFlags.AddOrReplace(fmt.Sprintf("--replica-id %s", h.MSRMetadata.ReplicaID)) + joinFlags.AddOrReplace(fmt.Sprintf("--replica-id %s", shellescape.Quote(h.MSRMetadata.ReplicaID))) } else { log.Infof("%s: joining MSR replica to cluster", h) } diff --git a/pkg/product/mke/phase/remove_nodes.go b/pkg/product/mke/phase/remove_nodes.go index 62acece84..10168316e 100644 --- a/pkg/product/mke/phase/remove_nodes.go +++ b/pkg/product/mke/phase/remove_nodes.go @@ -18,6 +18,7 @@ import ( "github.com/Mirantis/launchpad/pkg/swarm" "github.com/Mirantis/launchpad/pkg/util/stringutil" "github.com/k0sproject/rig/v2/cmd" + "github.com/k0sproject/rig/v2/sh/shellescape" log "github.com/sirupsen/logrus" ) @@ -228,8 +229,8 @@ func (p *RemoveNodes) removemsrNode(config *mkeconfig.ClusterConfig, replicaID s } removeFlags := commonconfig.Flags{ - fmt.Sprintf("--replica-ids %s", replicaID), - fmt.Sprintf("--existing-replica-id %s", msrLeader.MSRMetadata.ReplicaID), + fmt.Sprintf("--replica-ids %s", shellescape.Quote(replicaID)), + fmt.Sprintf("--existing-replica-id %s", shellescape.Quote(msrLeader.MSRMetadata.ReplicaID)), } removeFlags.MergeOverwrite(mkeFlags) for _, f := range msr.PluckSharedInstallFlags(config.Spec.MSR.InstallFlags, msr.SharedInstallRemoveFlags) { diff --git a/pkg/product/mke/phase/upgrade_msr.go b/pkg/product/mke/phase/upgrade_msr.go index 82b9a2909..3969b85d6 100644 --- a/pkg/product/mke/phase/upgrade_msr.go +++ b/pkg/product/mke/phase/upgrade_msr.go @@ -7,6 +7,7 @@ import ( "github.com/Mirantis/launchpad/pkg/msr" "github.com/Mirantis/launchpad/pkg/phase" commonconfig "github.com/Mirantis/launchpad/pkg/product/common/config" + "github.com/k0sproject/rig/v2/sh/shellescape" log "github.com/sirupsen/logrus" ) @@ -41,7 +42,7 @@ func (p *UpgradeMSR) Run() error { return nil } - upgradeFlags := commonconfig.Flags{fmt.Sprintf("--existing-replica-id %s", h.MSRMetadata.ReplicaID)} + upgradeFlags := commonconfig.Flags{fmt.Sprintf("--existing-replica-id %s", shellescape.Quote(h.MSRMetadata.ReplicaID))} upgradeFlags.MergeOverwrite(msr.BuildMKEFlags(p.Config)) for _, f := range msr.PluckSharedInstallFlags(p.Config.Spec.MSR.InstallFlags, msr.SharedInstallUpgradeFlags) {