Escape auth passwords in generated Valkey config (#8)

Author: attilaolah

internal/controller/builders_coverage_test.go

@@ -96,7 +96,7 @@ func TestRenderValkeyConfProfiles(t *testing.T) {
 
 	// Password → requirepass + masterauth.
 	withPass := renderValkeyConf(withMem(cachev1beta1.ProfileCache, "1Gi"), "s3cr3t")
-	if !strings.Contains(withPass, "requirepass s3cr3t") || !strings.Contains(withPass, "masterauth s3cr3t") {
+	if !strings.Contains(withPass, `requirepass "s3cr3t"`) || !strings.Contains(withPass, `masterauth "s3cr3t"`) {
 		t.Errorf("password conf must set requirepass + masterauth\n%s", withPass)
 	}
 
@@ -140,12 +140,15 @@ func TestRenderSentinelConf(t *testing.T) {
 	}
 
 	// Password → sentinel auth-user (dedicated ACL user) + auth-pass + requirepass.
-	if c := renderSentinelConf(sentinelCR(), "pw"); !strings.Contains(c, "auth-pass mymaster pw") || !strings.Contains(c, "requirepass pw") {
+	if c := renderSentinelConf(sentinelCR(), "pw"); !strings.Contains(c, `auth-pass mymaster "pw"`) || !strings.Contains(c, `requirepass "pw"`) {
 		t.Errorf("sentinel auth not rendered\n%s", c)
 	}
 	if c := renderSentinelConf(sentinelCR(), "pw"); !strings.Contains(c, "auth-user mymaster sentinel-user") {
 		t.Errorf("sentinel must authenticate to master as the dedicated ACL user\n%s", c)
 	}
+	if c := renderSentinelConf(sentinelCR(), `abc"def`); !strings.Contains(c, `auth-pass mymaster "abc\"def"`) || !strings.Contains(c, `requirepass "abc\"def"`) {
+		t.Errorf("sentinel auth password must be escaped\n%s", c)
+	}
 
 	// TLS → tls-port + plaintext disabled + tls-replication.
 	tlsCR := sentinelCR()

internal/controller/resources.go

@@ -155,6 +155,13 @@ func passwordSecretName(vc *cachev1beta1.ValkeyCluster) string {
 	return fmt.Sprintf("%s-auth", vc.Name)
 }
 
+func authSecretName(vc *cachev1beta1.ValkeyCluster) string {
+	if vc.Spec.Auth != nil && vc.Spec.Auth.ExistingSecret != "" {
+		return vc.Spec.Auth.ExistingSecret
+	}
+	return passwordSecretName(vc)
+}
+
 // hibernated reports whether the cluster is requested to be hibernated
 // (scaled to zero while keeping PVCs) via the hibernate annotation.
 func hibernated(vc *cachev1beta1.ValkeyCluster) bool {
@@ -196,6 +203,36 @@ func generatePassword(n int) (string, error) {
 	return base64.RawURLEncoding.EncodeToString(b)[:n], nil
 }
 
+func valkeyConfigArg(value string) string {
+	var b strings.Builder
+	b.WriteByte('"')
+	for i := range len(value) {
+		switch c := value[i]; c {
+		case '\\', '"':
+			b.WriteByte('\\')
+			b.WriteByte(c)
+		case '\a':
+			b.WriteString(`\a`)
+		case '\b':
+			b.WriteString(`\b`)
+		case '\n':
+			b.WriteString(`\n`)
+		case '\r':
+			b.WriteString(`\r`)
+		case '\t':
+			b.WriteString(`\t`)
+		default:
+			if c < 0x20 || c == 0x7f {
+				fmt.Fprintf(&b, `\x%02x`, c)
+				continue
+			}
+			b.WriteByte(c)
+		}
+	}
+	b.WriteByte('"')
+	return b.String()
+}
+
 func buildHeadlessService(vc *cachev1beta1.ValkeyCluster) *corev1.Service {
 	port := valkeyPort
 	if tlsEnabled(vc) {
@@ -345,8 +382,9 @@ func renderValkeyConf(vc *cachev1beta1.ValkeyCluster, password string) string {
 	fmt.Fprintf(&b, "protected-mode no\n")
 	fmt.Fprintf(&b, "dir %s\n", dataMountPath)
 	if password != "" {
-		fmt.Fprintf(&b, "requirepass %s\n", password)
-		fmt.Fprintf(&b, "masterauth %s\n", password)
+		quotedPassword := valkeyConfigArg(password)
+		fmt.Fprintf(&b, "requirepass %s\n", quotedPassword)
+		fmt.Fprintf(&b, "masterauth %s\n", quotedPassword)
 	}
 
 	// Persist ACL state on the data PVC so users survive pod restarts.
@@ -554,20 +592,23 @@ func renderInitScript(vc *cachev1beta1.ValkeyCluster) string {
 	// follow-up that needs e2e failover validation.)
 	sentinelACL := ""
 	if vc.Spec.Topology == cachev1beta1.TopologySentinel {
-		sentinelACL = fmt.Sprintf("    echo \"user %s on >$PW &* +@all\" >> %s/users.acl\n",
+		sentinelACL = fmt.Sprintf("    echo \"user %s on #$PW_HASH &* +@all\" >> %s/users.acl\n",
 			sentinelACLUser, dataMountPath)
 	}
 	common := fmt.Sprintf(`set -eu
 cp %[1]s/valkey.conf %[2]s/runtime.conf
 if [ ! -s %[2]s/users.acl ]; then
-  PW=$(sed -n 's/^requirepass //p' %[2]s/runtime.conf | head -n1)
-  if [ -n "$PW" ]; then
-    echo "user default on >$PW ~* &* +@all" > %[2]s/users.acl
+  if [ -n "${VALKEY_PASSWORD:-}" ]; then
+    PW_HASH=$(printf '%%s' "$VALKEY_PASSWORD" | sha256sum | awk '{print $1}')
+    echo "user default on #$PW_HASH ~* &* +@all" > %[2]s/users.acl
 %[3]s  else
     : > %[2]s/users.acl
   fi
 fi
-`, configMountPath, dataMountPath, sentinelACL)
+valkey_config_arg() {
+  awk 'BEGIN { printf "\"" } { if (NR > 1) printf "\\n"; gsub(/\\/, "\\\\"); gsub(/"/, "\\\""); printf "%%s", $0 } END { printf "\"\n" }'
+}
+	`, configMountPath, dataMountPath, sentinelACL)
 
 	// Multi-region: every pod (including pod-0) replicates from an external
 	// primary. Local primary/replica entrypoint logic is bypassed.
@@ -594,7 +635,8 @@ fi
 		}
 		return common + fmt.Sprintf(`echo "replicaof %[1]s %[2]d" >> %[3]s/runtime.conf
 if [ -n "${SOURCE_PASSWORD:-}" ]; then
-  echo "masterauth ${SOURCE_PASSWORD}" >> %[3]s/runtime.conf
+  SOURCE_PASSWORD_ARG=$(printf '%%s' "$SOURCE_PASSWORD" | valkey_config_arg)
+  echo "masterauth ${SOURCE_PASSWORD_ARG}" >> %[3]s/runtime.conf
 fi
 %[4]s%[5]s`, vc.Spec.ReplicateFrom.Host, extPort, dataMountPath, tlsLine, caMergeLine)
 	}
@@ -707,6 +749,13 @@ func buildStatefulSet(vc *cachev1beta1.ValkeyCluster, configHash string, proacti
 			},
 		})
 	}
+	configInitEnv := append([]corev1.EnvVar(nil), envVars...)
+	if vc.Spec.Auth != nil && vc.Spec.Auth.Enabled {
+		configInitEnv = append(configInitEnv, corev1.EnvVar{
+			Name:      envValkeyPassword,
+			ValueFrom: secretRef(authSecretName(vc), secretKeyPassword),
+		})
+	}
 
 	containerPorts := []corev1.ContainerPort{{
 		Name:          appValkey,
@@ -748,7 +797,7 @@ func buildStatefulSet(vc *cachev1beta1.ValkeyCluster, configHash string, proacti
 		Image:           vc.Spec.Image,
 		ImagePullPolicy: vc.Spec.ImagePullPolicy,
 		Command:         []string{shellCmd, "-c", renderInitScript(vc)},
-		Env:             envVars,
+		Env:             configInitEnv,
 		VolumeMounts:    configInitMounts,
 	})
 

internal/controller/resources_test.go

@@ -232,6 +232,27 @@ func TestConfigHashFromDataIsDeterministicAndSensitive(t *testing.T) {
 	}
 }
 
+func TestValkeyConfigArgQuotesSpecialCharacters(t *testing.T) {
+	tests := []struct {
+		name  string
+		value string
+		want  string
+	}{
+		{name: "plain", value: "secret", want: `"secret"`},
+		{name: "double quote", value: `abc"def`, want: `"abc\"def"`},
+		{name: "space backslash hash", value: `a b\c#d`, want: `"a b\\c#d"`},
+		{name: "leading hash", value: `#comment`, want: `"#comment"`},
+		{name: "newline", value: "line\nnext", want: `"line\nnext"`},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := valkeyConfigArg(tc.value); got != tc.want {
+				t.Errorf("valkeyConfigArg(%q) = %q, want %q", tc.value, got, tc.want)
+			}
+		})
+	}
+}
+
 func TestRenderValkeyConfHasProfileDefaults(t *testing.T) {
 	tests := []struct {
 		name           string
@@ -275,14 +296,17 @@ func TestRenderInitScriptSeedsDefaultUserACL(t *testing.T) {
 
 	for _, want := range []string{
 		"users.acl",
-		"requirepass",                              // password is read back from runtime.conf
-		"user default on >$PW ~* &* +@all",         // seeded default user carries the password
+		`printf '%s' "$VALKEY_PASSWORD"`,           // hash is computed from the exact Secret value
+		"user default on #$PW_HASH ~* &* +@all",    // seeded default user carries the password hash
 		"[ ! -s " + dataMountPath + "/users.acl ]", // only seed when empty (don't clobber ACL SAVE)
 	} {
 		if !strings.Contains(script, want) {
 			t.Errorf("init script missing %q\n%s", want, script)
 		}
 	}
+	if strings.Contains(script, "sed -n 's/^requirepass //p'") {
+		t.Errorf("init script must not parse the escaped config password back out of runtime.conf\n%s", script)
+	}
 	// It must NOT blindly create an empty file in the auth case.
 	if strings.Contains(script, "touch "+dataMountPath+"/users.acl") {
 		t.Errorf("init script still touches an empty users.acl (the bug)\n%s", script)
@@ -303,11 +327,11 @@ func TestRenderInitScriptSeedsSentinelACLUser(t *testing.T) {
 	vc.Spec.Auth = &cachev1beta1.AuthSpec{Enabled: true}
 	script := renderInitScript(vc)
 
-	if !strings.Contains(script, "user sentinel-user on >$PW &* +@all") {
+	if !strings.Contains(script, "user sentinel-user on #$PW_HASH &* +@all") {
 		t.Errorf("Sentinel init script must seed the sentinel ACL user\n%s", script)
 	}
 	// No key glob (~) for the sentinel user — it must not read/write data.
-	if strings.Contains(script, "user sentinel-user on >$PW ~* &* +@all") {
+	if strings.Contains(script, "user sentinel-user on #$PW_HASH ~* &* +@all") {
 		t.Errorf("sentinel-user must not have key access (~*)\n%s", script)
 	}
 }
@@ -335,6 +359,24 @@ func TestRenderValkeyConfMutualTLS(t *testing.T) {
 	}
 }
 
+func TestRenderValkeyConfEscapesPasswordArguments(t *testing.T) {
+	vc := minimalCR()
+	password := `abc" def\#ghi`
+	conf := renderValkeyConf(vc, password)
+	quoted := valkeyConfigArg(password)
+	for _, want := range []string{
+		"requirepass " + quoted,
+		"masterauth " + quoted,
+	} {
+		if !strings.Contains(conf, want) {
+			t.Errorf("missing escaped password directive %q\n%s", want, conf)
+		}
+	}
+	if strings.Contains(conf, `requirepass abc"`) {
+		t.Errorf("password must not be rendered as an unquoted config argument\n%s", conf)
+	}
+}
+
 func TestInternalEndpoint(t *testing.T) {
 	vc := minimalCR()
 	vc.Name = "web"
@@ -360,8 +402,8 @@ func TestRenderValkeyConfClusterDirectives(t *testing.T) {
 		"cluster-config-file " + dataMountPath + "/nodes.conf",
 		"cluster-node-timeout 5000",
 		"cluster-require-full-coverage yes", // Durable requires it
-		"requirepass secret",
-		"masterauth secret",
+		`requirepass "secret"`,
+		`masterauth "secret"`,
 	} {
 		if !strings.Contains(conf, want) {
 			t.Errorf("missing %q\n%s", want, conf)
@@ -464,6 +506,35 @@ func TestBuildExporterAuthSecret(t *testing.T) {
 	}
 }
 
+func TestBuildStatefulSetConfigInitGetsAuthPasswordEnv(t *testing.T) {
+	passwordSecretRef := func(vc *cachev1beta1.ValkeyCluster) string {
+		sts := buildStatefulSet(vc, "h", false)
+		for _, e := range sts.Spec.Template.Spec.InitContainers[0].Env {
+			if e.Name == envValkeyPassword && e.ValueFrom != nil && e.ValueFrom.SecretKeyRef != nil {
+				return e.ValueFrom.SecretKeyRef.Name
+			}
+		}
+		return ""
+	}
+
+	gen := minimalCR()
+	gen.Spec.Auth = &cachev1beta1.AuthSpec{Enabled: true}
+	if got := passwordSecretRef(gen); got != "test-auth" {
+		t.Errorf("config-init password secret = %q, want generated test-auth", got)
+	}
+
+	ext := minimalCR()
+	ext.Spec.Auth = &cachev1beta1.AuthSpec{Enabled: true, ExistingSecret: "my-auth"}
+	if got := passwordSecretRef(ext); got != "my-auth" {
+		t.Errorf("config-init password secret = %q, want existingSecret my-auth", got)
+	}
+
+	disabled := minimalCR()
+	if got := passwordSecretRef(disabled); got != "" {
+		t.Errorf("config-init should not get VALKEY_PASSWORD when auth is disabled, got secret %q", got)
+	}
+}
+
 func TestBuildHeadlessServiceHasGossipPortOnlyForCluster(t *testing.T) {
 	vc := minimalCR()
 	svc := buildHeadlessService(vc)
@@ -630,6 +701,8 @@ func TestSourceCAMergeRendersCombinedBundle(t *testing.T) {
 		"cat " + tlsMountPath + "/ca.crt " + sourceCAMountPath + "/ca.crt > " + dataMountPath + "/ca-bundle.crt",
 		"replicaof src-primary.dc2.svc.cluster.local 6380",
 		"tls-replication yes",
+		"SOURCE_PASSWORD_ARG=$(printf '%s' \"$SOURCE_PASSWORD\" | valkey_config_arg)",
+		"masterauth ${SOURCE_PASSWORD_ARG}",
 	} {
 		if !strings.Contains(script, want) {
 			t.Errorf("init script missing %q\n%s", want, script)

internal/controller/sentinel.go

@@ -164,8 +164,9 @@ sentinel announce-hostnames yes
 		// Authenticate to the monitored master as the dedicated least-data-exposure
 		// ACL user (seeded on the data nodes) rather than the default user, and
 		// keep requirepass for client/inter-sentinel auth on the Sentinel port.
+		quotedPassword := valkeyConfigArg(password)
 		conf += fmt.Sprintf("sentinel auth-user %s %s\nsentinel auth-pass %s %s\nrequirepass %s\n",
-			sentinelMasterName, sentinelACLUser, sentinelMasterName, password, password)
+			sentinelMasterName, sentinelACLUser, sentinelMasterName, quotedPassword, quotedPassword)
 	}
 	if tlsEnabled(vc) {
 		conf += fmt.Sprintf(`tls-port %d