Skip to content

Commit d65da47

Browse files
armrumnenciaNiccoloFei
authored
fix: escape backslashes and control chars in PostgreSQL config values (#10515)
Values written into custom.conf were only escaped for single quotes. Backslashes and control characters (LF, CR, TAB, BS, FF) passed through unescaped, which either corrupted the configuration (e.g. a user-supplied recovery_target_name containing ' or a newline) or silently mangled the runtime value (a \ in log_line_prefix stripped by PostgreSQL's config lexer). Introduce configfile.RenderPostgresConfiguration as the single canonical helper that emits a postgresql.conf-style fragment from a map[string]string, with values escaped according to the guc-file.l rules (\, ', \n, \r, \t, \b, \f). Migrate every config-file-emitting site to it: pkg/configfile.UpdateConfigurationContents (replacing pq.QuoteLiteral, which emits an E'...' SQL-style literal that the config lexer rejects when the value contains a backslash), pkg/postgres.CreatePostgresqlConfFile, api/v1.(*RecoveryTarget).BuildPostgresOptions (previously did no escaping at all for recovery_target_name/_xid/_lsn/_timeline/_time), and pkg/management/postgres.getRestoreWalConfig and RestoreSnapshot's restore_command construction. After this change the inner escapePostgresConfLiteral is private to pkg/configfile. Upgrade impact: clusters whose PostgreSQL parameters contain \ or a control character (most commonly a literal tab in log_line_prefix) will see one pg_reload_conf() on the first reconcile after upgrade because the config-file SHA256 changes. No restart, no connection drops. Clusters with only default or quote-only special values are unaffected. Related #10518 Closes #10506 Reported-by: Koda Reef <kodareef5@users.noreply.github.com> Signed-off-by: Armando Ruocco <armando.ruocco@enterprisedb.com> Signed-off-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com> Signed-off-by: Niccolò Fei <niccolo.fei@enterprisedb.com> Co-authored-by: Marco Nenciarini <marco.nenciarini@enterprisedb.com> Co-authored-by: Niccolò Fei <niccolo.fei@enterprisedb.com>
1 parent f8283b0 commit d65da47

8 files changed

Lines changed: 315 additions & 87 deletions

File tree

‎api/v1/cluster_funcs.go‎

Lines changed: 12 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@ import (
4141
"k8s.io/apimachinery/pkg/types"
4242

4343
"github.com/cloudnative-pg/cloudnative-pg/internal/configuration"
44+
"github.com/cloudnative-pg/cloudnative-pg/pkg/configfile"
4445
"github.com/cloudnative-pg/cloudnative-pg/pkg/system"
4546
"github.com/cloudnative-pg/cloudnative-pg/pkg/utils"
4647
contextutils "github.com/cloudnative-pg/cloudnative-pg/pkg/utils/context"
@@ -1472,47 +1473,36 @@ func (cluster *Cluster) EnsureGVKIsPresent() {
14721473
// should be added to the PostgreSQL configuration to
14731474
// recover given a certain target
14741475
func (target *RecoveryTarget) BuildPostgresOptions() string {
1475-
result := ""
1476-
14771476
if target == nil {
1478-
return result
1477+
return ""
14791478
}
14801479

1480+
options := map[string]string{}
14811481
if target.TargetTLI != "" {
1482-
result += fmt.Sprintf(
1483-
"recovery_target_timeline = '%v'\n",
1484-
target.TargetTLI)
1482+
options["recovery_target_timeline"] = target.TargetTLI
14851483
}
14861484
if target.TargetXID != "" {
1487-
result += fmt.Sprintf(
1488-
"recovery_target_xid = '%v'\n",
1489-
target.TargetXID)
1485+
options["recovery_target_xid"] = target.TargetXID
14901486
}
14911487
if target.TargetName != "" {
1492-
result += fmt.Sprintf(
1493-
"recovery_target_name = '%v'\n",
1494-
target.TargetName)
1488+
options["recovery_target_name"] = target.TargetName
14951489
}
14961490
if target.TargetLSN != "" {
1497-
result += fmt.Sprintf(
1498-
"recovery_target_lsn = '%v'\n",
1499-
target.TargetLSN)
1491+
options["recovery_target_lsn"] = target.TargetLSN
15001492
}
15011493
if target.TargetTime != "" {
1502-
result += fmt.Sprintf(
1503-
"recovery_target_time = '%v'\n",
1504-
pgTime.ConvertToPostgresFormat(target.TargetTime))
1494+
options["recovery_target_time"] = pgTime.ConvertToPostgresFormat(target.TargetTime)
15051495
}
15061496
if target.TargetImmediate != nil && *target.TargetImmediate {
1507-
result += "recovery_target = immediate\n"
1497+
options["recovery_target"] = "immediate"
15081498
}
15091499
if target.Exclusive != nil && *target.Exclusive {
1510-
result += "recovery_target_inclusive = false\n"
1500+
options["recovery_target_inclusive"] = "false"
15111501
} else {
1512-
result += "recovery_target_inclusive = true\n"
1502+
options["recovery_target_inclusive"] = "true"
15131503
}
15141504

1515-
return result
1505+
return configfile.RenderPostgresConfiguration(options)
15161506
}
15171507

15181508
// ApplyInto applies the content of the probe configuration in a Kubernetes

‎api/v1/cluster_funcs_test.go‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1824,3 +1824,45 @@ var _ = Describe("GetServiceAccountName", func() {
18241824
Expect(cluster.GetServiceAccountName()).To(Equal("my-cluster"))
18251825
})
18261826
})
1827+
1828+
var _ = Describe("RecoveryTarget.BuildPostgresOptions", func() {
1829+
It("returns an empty string for a nil receiver", func() {
1830+
var target *RecoveryTarget
1831+
Expect(target.BuildPostgresOptions()).To(Equal(""))
1832+
})
1833+
1834+
It("escapes single quotes in the target name", func() {
1835+
target := &RecoveryTarget{TargetName: "my'restore'point"}
1836+
Expect(target.BuildPostgresOptions()).To(ContainSubstring(
1837+
"recovery_target_name = 'my''restore''point'\n",
1838+
))
1839+
})
1840+
1841+
It("escapes backslashes in the target name", func() {
1842+
target := &RecoveryTarget{TargetName: `path\to\restore`}
1843+
Expect(target.BuildPostgresOptions()).To(ContainSubstring(
1844+
`recovery_target_name = 'path\\to\\restore'` + "\n",
1845+
))
1846+
})
1847+
1848+
It("escapes literal newlines in the target name", func() {
1849+
// pg_create_restore_point accepts any string, including one with a
1850+
// literal newline. The config file must stay line-oriented.
1851+
target := &RecoveryTarget{TargetName: "line1\nline2"}
1852+
Expect(target.BuildPostgresOptions()).To(ContainSubstring(
1853+
`recovery_target_name = 'line1\nline2'` + "\n",
1854+
))
1855+
})
1856+
1857+
It("escapes every other target field", func() {
1858+
target := &RecoveryTarget{
1859+
TargetTLI: `l\a'test`,
1860+
TargetXID: "1'2",
1861+
TargetLSN: `0/1\6`,
1862+
}
1863+
out := target.BuildPostgresOptions()
1864+
Expect(out).To(ContainSubstring(`recovery_target_timeline = 'l\\a''test'` + "\n"))
1865+
Expect(out).To(ContainSubstring(`recovery_target_xid = '1''2'` + "\n"))
1866+
Expect(out).To(ContainSubstring(`recovery_target_lsn = '0/1\\6'` + "\n"))
1867+
})
1868+
})

‎pkg/configfile/configfile.go‎

Lines changed: 45 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,30 @@ import (
2727

2828
"github.com/cloudnative-pg/machinery/pkg/fileutils"
2929
"github.com/cloudnative-pg/machinery/pkg/stringset"
30-
"github.com/lib/pq"
3130
)
3231

32+
// postgresConfLiteralReplacer escapes every character the PostgreSQL config
33+
// file lexer (`guc-file.l`) interprets specially inside a single-quoted value.
34+
var postgresConfLiteralReplacer = strings.NewReplacer(
35+
`\`, `\\`,
36+
`'`, `''`,
37+
"\n", `\n`,
38+
"\r", `\r`,
39+
"\t", `\t`,
40+
"\b", `\b`,
41+
"\f", `\f`,
42+
)
43+
44+
// escapePostgresConfLiteral escapes an arbitrary string so it can be used as a
45+
// value in a PostgreSQL configuration file, wrapped in single quotes.
46+
//
47+
// pq.QuoteLiteral is unsuitable here: it emits ` E'...'` when the value
48+
// contains a backslash, and the PostgreSQL config parser does not recognise
49+
// the `E'...'` syntax.
50+
func escapePostgresConfLiteral(value string) string {
51+
return "'" + postgresConfLiteralReplacer.Replace(value) + "'"
52+
}
53+
3354
// UpdatePostgresConfigurationFile search and replace options in a Postgres configuration file.
3455
// If any managedOptions is passed, it will be removed unless present in the options map.
3556
// If the configuration file doesn't exist, it will be written.
@@ -50,17 +71,13 @@ func UpdatePostgresConfigurationFile(
5071
}
5172
}
5273
lines = RemoveOptionsFromConfigurationContents(lines, optionsToRemove...)
53-
54-
lines, err = UpdateConfigurationContents(lines, options)
55-
if err != nil {
56-
return false, fmt.Errorf("error while updating configuration from %v: %w", fileName, err)
57-
}
74+
lines = UpdateConfigurationContents(lines, options)
5875
return fileutils.WriteLinesToFile(fileName, lines)
5976
}
6077

6178
// UpdateConfigurationContents search and replace options in a configuration file whose
6279
// content is passed
63-
func UpdateConfigurationContents(lines []string, options map[string]string) ([]string, error) {
80+
func UpdateConfigurationContents(lines []string, options map[string]string) []string {
6481
foundKeys := stringset.New()
6582
index := 0
6683
for _, line := range lines {
@@ -77,7 +94,7 @@ func UpdateConfigurationContents(lines []string, options map[string]string) ([]s
7794
}
7895

7996
foundKeys.Put(key)
80-
lines[index] = fmt.Sprintf("%s = %s", key, pq.QuoteLiteral(value))
97+
lines[index] = fmt.Sprintf("%s = %s", key, escapePostgresConfLiteral(value))
8198
index++
8299
continue
83100
}
@@ -92,11 +109,28 @@ func UpdateConfigurationContents(lines []string, options map[string]string) ([]s
92109
for _, key := range keysList {
93110
if !foundKeys.Has(key) {
94111
value := options[key]
95-
lines = append(lines, fmt.Sprintf("%s = %s", key, pq.QuoteLiteral(value)))
112+
lines = append(lines, fmt.Sprintf("%s = %s", key, escapePostgresConfLiteral(value)))
96113
}
97114
}
98115

99-
return lines, nil
116+
return lines
117+
}
118+
119+
// RenderPostgresConfiguration returns a PostgreSQL configuration fragment as
120+
// a single string: one `key = 'escaped value'\n` line per entry in options,
121+
// sorted by key. The returned string is safe to concatenate with other
122+
// fragments and to embed directly into a `postgresql.conf`-style file.
123+
//
124+
// This is the canonical way to produce a config fragment in-memory: callers
125+
// that build strings by hand can forget to escape values, while this funnels
126+
// every value through the same escaping that the on-disk parser uses.
127+
func RenderPostgresConfiguration(options map[string]string) string {
128+
var b strings.Builder
129+
for _, l := range UpdateConfigurationContents(nil, options) {
130+
b.WriteString(l)
131+
b.WriteByte('\n')
132+
}
133+
return b.String()
100134
}
101135

102136
// WritePostgresConfiguration replaces the content of a PostgreSQL configuration
@@ -105,11 +139,7 @@ func WritePostgresConfiguration(
105139
fileName string,
106140
options map[string]string,
107141
) (changed bool, err error) {
108-
lines, err := UpdateConfigurationContents(nil, options)
109-
if err != nil {
110-
return false, fmt.Errorf("error while writing configuration to %v: %w", fileName, err)
111-
}
112-
return fileutils.WriteLinesToFile(fileName, lines)
142+
return fileutils.WriteLinesToFile(fileName, UpdateConfigurationContents(nil, options))
113143
}
114144

115145
// RemoveOptionsFromConfigurationContents deletes all the lines containing one of the given options

‎pkg/configfile/configfile_test.go‎

Lines changed: 125 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ var _ = Describe("Update configuration files", func() {
207207
"recovery_target_timeline = 'latest'",
208208
}
209209

210-
updatedContent, _ := UpdateConfigurationContents(initialContent, map[string]string{
210+
updatedContent := UpdateConfigurationContents(initialContent, map[string]string{
211211
"test.key": "test.value",
212212
})
213213

@@ -233,7 +233,7 @@ var _ = Describe("Update configuration files", func() {
233233
"primary_conninfo = 'host=someHost2 user=someUser2 application_name=nodeName2'",
234234
}
235235

236-
updatedContent, _ := UpdateConfigurationContents(initialContent, map[string]string{
236+
updatedContent := UpdateConfigurationContents(initialContent, map[string]string{
237237
"primary_conninfo": "host=someHost user=someUser application_name=nodeName",
238238
})
239239

@@ -249,6 +249,129 @@ var _ = Describe("Update configuration files", func() {
249249
})
250250
})
251251

252+
var _ = Describe("escapePostgresConfLiteral", func() {
253+
DescribeTable("produces a value that round-trips through the PostgreSQL config file parser",
254+
func(input, expected string) {
255+
Expect(escapePostgresConfLiteral(input)).To(Equal(expected))
256+
},
257+
Entry("plain string", "hello", "'hello'"),
258+
Entry("empty string", "", "''"),
259+
Entry("single quote", "hello'world", "'hello''world'"),
260+
Entry("backslash", `hello\world`, `'hello\\world'`),
261+
Entry("backslash followed by n (two chars)", `a\nb`, `'a\\nb'`),
262+
Entry("literal newline", "a\nb", `'a\nb'`),
263+
Entry("literal carriage return", "a\rb", `'a\rb'`),
264+
Entry("literal tab", "a\tb", `'a\tb'`),
265+
Entry("literal backspace", "a\bb", `'a\bb'`),
266+
Entry("literal form feed", "a\fb", `'a\fb'`),
267+
Entry("backslash and quote together",
268+
`a'\b`,
269+
`'a''\\b'`),
270+
Entry("everything at once",
271+
"a'b\\c\nd\te",
272+
`'a''b\\c\nd\te'`),
273+
)
274+
275+
It("produces a quoted literal that PostgreSQL reverses back to the original", func() {
276+
// The config file parser applies these rules inside single-quoted strings:
277+
// '' -> '
278+
// \\ -> \
279+
// \n -> newline
280+
// \r -> CR
281+
// \t -> tab
282+
// \b -> backspace
283+
// \f -> form feed
284+
// \X -> X (backslash stripped for any other X)
285+
reverse := func(quoted string) string {
286+
// Expect surrounding single quotes
287+
Expect(quoted[:1]).To(Equal("'"))
288+
Expect(quoted[len(quoted)-1:]).To(Equal("'"))
289+
inner := quoted[1 : len(quoted)-1]
290+
var b []byte
291+
for i := 0; i < len(inner); i++ {
292+
switch inner[i] {
293+
case '\'':
294+
Expect(i+1 < len(inner)).To(BeTrue())
295+
Expect(inner[i+1]).To(Equal(byte('\'')))
296+
b = append(b, '\'')
297+
i++
298+
case '\\':
299+
Expect(i+1 < len(inner)).To(BeTrue())
300+
switch inner[i+1] {
301+
case '\\':
302+
b = append(b, '\\')
303+
case 'n':
304+
b = append(b, '\n')
305+
case 'r':
306+
b = append(b, '\r')
307+
case 't':
308+
b = append(b, '\t')
309+
case 'b':
310+
b = append(b, '\b')
311+
case 'f':
312+
b = append(b, '\f')
313+
default:
314+
b = append(b, inner[i+1])
315+
}
316+
i++
317+
default:
318+
b = append(b, inner[i])
319+
}
320+
}
321+
return string(b)
322+
}
323+
324+
cases := []string{
325+
"hello",
326+
"",
327+
"hello'world",
328+
`hello\world`,
329+
`a\nb`,
330+
"a\nb",
331+
"a\rb",
332+
"a\tb",
333+
"a'b\\c\nd\te",
334+
`multiple 'quotes' and \back\slashes`,
335+
}
336+
for _, value := range cases {
337+
Expect(reverse(escapePostgresConfLiteral(value))).To(Equal(value),
338+
"round-trip failed for input %q", value)
339+
}
340+
})
341+
})
342+
343+
var _ = Describe("UpdateConfigurationContents with special characters", func() {
344+
It("escapes single quotes in values", func() {
345+
updated := UpdateConfigurationContents(nil, map[string]string{
346+
"recovery_target_name": "my'restore'point",
347+
})
348+
Expect(updated).To(ConsistOf(
349+
"recovery_target_name = 'my''restore''point'",
350+
))
351+
})
352+
353+
It("escapes backslashes without generating E-strings", func() {
354+
// pq.QuoteLiteral generates `E'...'` when the input contains a backslash,
355+
// which the postgresql.conf lexer does not understand. We must produce
356+
// the plain single-quoted form with doubled backslashes.
357+
updated := UpdateConfigurationContents(nil, map[string]string{
358+
"archive_command": `test \ command`,
359+
})
360+
Expect(updated).To(ConsistOf(
361+
`archive_command = 'test \\ command'`,
362+
))
363+
})
364+
365+
It("escapes literal newlines", func() {
366+
updated := UpdateConfigurationContents(nil, map[string]string{
367+
"recovery_target_name": "line1\nline2",
368+
})
369+
Expect(updated).To(ConsistOf(
370+
`recovery_target_name = 'line1\nline2'`,
371+
))
372+
})
373+
})
374+
252375
var _ = Describe("Remove configuration files option", func() {
253376
It("keeps the initial input if the option to be removed is not matched", func() {
254377
initialContent := []string{

‎pkg/management/postgres/restore.go‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -148,10 +148,10 @@ func (info InitInfo) RestoreSnapshot(ctx context.Context, cli client.Client, imm
148148
restoreCmd := fmt.Sprintf(
149149
"/controller/manager wal-restore --log-destination %s/%s.json %%f %%p",
150150
postgresSpec.LogPath, postgresSpec.LogFileName)
151-
config := fmt.Sprintf(
152-
"recovery_target_action = promote\n"+
153-
"restore_command = '%s'\n",
154-
restoreCmd)
151+
config := configfile.RenderPostgresConfiguration(map[string]string{
152+
"recovery_target_action": "promote",
153+
"restore_command": restoreCmd,
154+
})
155155

156156
if pluginConfiguration := cluster.GetRecoverySourcePlugin(); pluginConfiguration == nil {
157157
server, found := cluster.ExternalCluster(cluster.Spec.Bootstrap.Recovery.Source)
@@ -639,12 +639,10 @@ func getRestoreWalConfig(ctx context.Context, backup *apiv1.Backup) (string, err
639639

640640
cmd = append(cmd, "%f", "%p")
641641

642-
recoveryFileContents := fmt.Sprintf(
643-
"recovery_target_action = promote\n"+
644-
"restore_command = '%s'\n",
645-
strings.Join(cmd, " "))
646-
647-
return recoveryFileContents, nil
642+
return configfile.RenderPostgresConfiguration(map[string]string{
643+
"recovery_target_action": "promote",
644+
"restore_command": strings.Join(cmd, " "),
645+
}), nil
648646
}
649647

650648
func (info InitInfo) writeRecoveryConfiguration(cluster *apiv1.Cluster, recoveryFileContents string) error {

0 commit comments

Comments
 (0)