Skip to content

Commit caf2077

Browse files
mnenciamnencia
authored
fix: tighten recovery target validation (#10565)
TargetXID was previously unchecked and a malformed value surfaced only at PostgreSQL startup. Validate it as a non-negative 32-bit integer at admission, matching PG's TransactionId width so values that would silently truncate to a different XID are rejected up front. TargetName accepts arbitrary text in pg_create_restore_point, but ASCII control characters (NUL, newline, DEL) are never legitimate and would either break the line-oriented config file or signal a hostile spec. Reject them. Also enforce the 64-byte MAXFNAMELEN limit at admission rather than letting PG fail at recovery. Related #10515 Suggested-by: Koda Reef <[email protected]> Signed-off-by: Marco Nenciarini <[email protected]>
1 parent 490938f commit caf2077

2 files changed

Lines changed: 116 additions & 1 deletion

File tree

‎internal/webhook/v1/cluster_webhook.go‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1406,6 +1406,39 @@ func (v *ClusterCustomValidator) validateRecoveryTarget(r *apiv1.Cluster) field.
14061406
}
14071407
}
14081408

1409+
// PostgreSQL casts recovery_target_xid to TransactionId (uint32); a value
1410+
// above 2^32-1 would be silently truncated. Use bitSize 32 so we reject
1411+
// rather than admit a different XID than the user wrote.
1412+
if recoveryTarget.TargetXID != "" {
1413+
if _, err := strconv.ParseUint(recoveryTarget.TargetXID, 10, 32); err != nil {
1414+
result = append(result, field.Invalid(
1415+
field.NewPath("spec", "bootstrap", "recovery", "recoveryTarget", "targetXID"),
1416+
recoveryTarget.TargetXID,
1417+
"recovery target xid must be a non-negative 32-bit integer"))
1418+
}
1419+
}
1420+
1421+
// PostgreSQL enforces MAXFNAMELEN (64) on recovery_target_name and on
1422+
// pg_create_restore_point; mirror it at admission for a better error.
1423+
if len(recoveryTarget.TargetName) >= 64 {
1424+
result = append(result, field.Invalid(
1425+
field.NewPath("spec", "bootstrap", "recovery", "recoveryTarget", "targetName"),
1426+
recoveryTarget.TargetName,
1427+
"recovery target name must be shorter than 64 bytes"))
1428+
}
1429+
1430+
// pg_create_restore_point accepts arbitrary text, but a name with
1431+
// newlines or NUL is never legitimate and is a strong signal of a
1432+
// malformed or hostile spec.
1433+
if strings.ContainsFunc(recoveryTarget.TargetName, func(r rune) bool {
1434+
return r < 0x20 || r == 0x7F
1435+
}) {
1436+
result = append(result, field.Invalid(
1437+
field.NewPath("spec", "bootstrap", "recovery", "recoveryTarget", "targetName"),
1438+
recoveryTarget.TargetName,
1439+
"recovery target name must not contain ASCII control characters"))
1440+
}
1441+
14091442
// When using a backup catalog, we can identify the backup to be restored
14101443
// only if the PITR is time-based. If the PITR is not time-based, the user
14111444
// need to specify a backup ID.

‎internal/webhook/v1/cluster_webhook_test.go‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1562,7 +1562,7 @@ var _ = Describe("recovery target", func() {
15621562
RecoveryTarget: &apiv1.RecoveryTarget{
15631563
BackupID: "",
15641564
TargetTLI: "",
1565-
TargetXID: "1/1",
1565+
TargetXID: "1234",
15661566
TargetName: "",
15671567
TargetLSN: "",
15681568
TargetTime: "",
@@ -1779,6 +1779,88 @@ var _ = Describe("recovery target", func() {
17791779
Expect(v.validateRecoveryTarget(cluster)).To(HaveLen(1))
17801780
})
17811781
})
1782+
1783+
When("TargetXID is specified", func() {
1784+
recoveryTargetWith := func(xid string) *apiv1.Cluster {
1785+
return &apiv1.Cluster{
1786+
Spec: apiv1.ClusterSpec{
1787+
Bootstrap: &apiv1.BootstrapConfiguration{
1788+
Recovery: &apiv1.BootstrapRecovery{
1789+
RecoveryTarget: &apiv1.RecoveryTarget{
1790+
BackupID: "backup-id",
1791+
TargetXID: xid,
1792+
},
1793+
},
1794+
},
1795+
},
1796+
}
1797+
}
1798+
1799+
It("accepts a non-negative integer", func() {
1800+
Expect(v.validateRecoveryTarget(recoveryTargetWith("1234"))).To(BeEmpty())
1801+
})
1802+
1803+
It("rejects a non-numeric value", func() {
1804+
Expect(v.validateRecoveryTarget(recoveryTargetWith("not-a-number"))).To(HaveLen(1))
1805+
})
1806+
1807+
It("rejects an LSN-shaped value", func() {
1808+
Expect(v.validateRecoveryTarget(recoveryTargetWith("1/1"))).To(HaveLen(1))
1809+
})
1810+
1811+
It("rejects a negative value", func() {
1812+
Expect(v.validateRecoveryTarget(recoveryTargetWith("-1"))).To(HaveLen(1))
1813+
})
1814+
1815+
It("accepts the largest 32-bit XID", func() {
1816+
Expect(v.validateRecoveryTarget(recoveryTargetWith("4294967295"))).To(BeEmpty())
1817+
})
1818+
1819+
It("rejects a value above 2^32-1 to avoid silent epoch truncation", func() {
1820+
Expect(v.validateRecoveryTarget(recoveryTargetWith("4294967296"))).To(HaveLen(1))
1821+
})
1822+
})
1823+
1824+
When("TargetName is specified", func() {
1825+
recoveryTargetWith := func(name string) *apiv1.Cluster {
1826+
return &apiv1.Cluster{
1827+
Spec: apiv1.ClusterSpec{
1828+
Bootstrap: &apiv1.BootstrapConfiguration{
1829+
Recovery: &apiv1.BootstrapRecovery{
1830+
RecoveryTarget: &apiv1.RecoveryTarget{
1831+
BackupID: "backup-id",
1832+
TargetName: name,
1833+
},
1834+
},
1835+
},
1836+
},
1837+
}
1838+
}
1839+
1840+
It("accepts an arbitrary printable string", func() {
1841+
Expect(v.validateRecoveryTarget(recoveryTargetWith("my'restore point"))).To(BeEmpty())
1842+
})
1843+
1844+
It("rejects an embedded newline", func() {
1845+
Expect(v.validateRecoveryTarget(recoveryTargetWith("line1\nline2"))).To(HaveLen(1))
1846+
})
1847+
1848+
It("rejects an embedded NUL byte", func() {
1849+
Expect(v.validateRecoveryTarget(recoveryTargetWith("a\x00b"))).To(HaveLen(1))
1850+
})
1851+
1852+
It("rejects a DEL byte", func() {
1853+
Expect(v.validateRecoveryTarget(recoveryTargetWith("a\x7fb"))).To(HaveLen(1))
1854+
})
1855+
1856+
It("accepts a 63-byte name", func() {
1857+
Expect(v.validateRecoveryTarget(recoveryTargetWith(strings.Repeat("a", 63)))).To(BeEmpty())
1858+
})
1859+
1860+
It("rejects a 64-byte name", func() {
1861+
Expect(v.validateRecoveryTarget(recoveryTargetWith(strings.Repeat("a", 64)))).To(HaveLen(1))
1862+
})
1863+
})
17821864
})
17831865

17841866
var _ = Describe("primary update strategy", func() {

0 commit comments

Comments
 (0)