Repository navigation
fix: tighten recovery target validation - #10565
Conversation
dosubot
Bot
added
size:M
cnpg-bot
added
backport-requested ◀️
|
❗ By default, the pull request is configured to backport to all release branches.
|
|
/test |
|
@mnencia, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/25059531741 |
d5c51c2 to
fc47025
Compare
fc47025 to
dc524dd
Compare
|
/test |
|
@mnencia, here's the link to the E2E on CNPG workflow run: https://github.com/cloudnative-pg/cloudnative-pg/actions/runs/25179673809 |
|
@mnencia why not doing the verification also in the API? This because the webhook can be skipped but not the API |
|
Fair point about API-level validation. The other recovery target fields are already validated in the webhook, so this PR follows the existing pattern. Moving these checks to CEL would be a good improvement, but I'd do it as a separate patch that also wires up envtest in this package so the rules get test coverage. |
dc524dd to
3018b1f
Compare
|
@mnencia I'm creating the issue for it then |
|
Follow up issue created here #10621 |
6a7e0a3 to
c580c2e
Compare
Isn't this a broader issue, unrelated to this specific ticket? My understanding is that we have normally relied on the webhook for this kind of validation. |
|
@gbartolini @mnencia I don't think we should merge this before handling the situation described in #10621 since it will be a partial fix. Probably the PR for #10621 can be merged into this PR to have a full fix? @gbartolini yes, normally we relied on it, but webhook can be skipped and there has been discussions about having these webhook removed in some configurations, so we should also start adding these verification on the API. On the other hand, having the validation on the API helps any GUI to validated objects before sinding it to the kubernetes clsuter. I think it's not necessary to say that if webhook can be skipped, all the validations there are useless and will arrive to the PostgreSQL configuration. |
|
I believe we should merge this and we should merge #10621 too. You can skip the webhook but, doing that:
About webhooks being not installed... I worked on a patch a while ago to prevent the operator and the instance manager from receiving invalid CR definitions: 09c0ade It is a separate topic, but would something like this help? |
|
Thanks for the follow-up, @sxd. I want to address the premise here, because I think it affects the conclusion. My understanding and position is that currently webhooks are not optional in a supported CloudNativePG installation. The operator itself treats their absence as an anomaly: in The So, in my opinion, the validation in this PR is not partial. It is complete relative to the architecture we support and the existing pattern across all other recovery target fields. @mnencia's response on 4 May already addressed this correctly: CEL is a worthwhile future improvement but belongs in a separate patch with proper envtest coverage, not folded into this fix. Coupling this PR to #10621 would block a well-scoped, tested fix on work that is substantially larger in scope and not yet designed. Unless there is a specific technical concern about the validation logic itself, I think this is ready to merge. |
|
@sxd I think the framing is off. The real fix for malformed recovery target values is #10515, which is already merged. That PR makes the operator escape whatever the user provides, so it cannot corrupt This PR is a UX improvement on top: reject obviously-bad input at admission, so users get a clear error instead of seeing PostgreSQL fail to start later. #10621 would be another UX layer (CEL on the API for clients that bypass the webhook). Neither is essential for safety; both just improve the UX at different surfaces. So this is not a partial fix. The real fix already shipped. Let's land this UX improvement now and tackle #10621 separately when we plan CEL coverage and envtest setup for the package. |
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. The escape layers introduced in #10515 already neutralise these values, so this is admission-time defense-in-depth: it surfaces the same errors earlier and with a clearer message. Signed-off-by: Marco Nenciarini <[email protected]>
c580c2e to
5668bb9
Compare
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]> (cherry picked from commit caf2077)
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]> (cherry picked from commit caf2077)
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]> (cherry picked from commit caf2077)
##### [\`v1.29.1\`](https://github.com/cloudnative-pg/cloudnative-pg/releases/tag/v1.29.1) **Release date:** May 8, 2026 ##### Security and Supply Chain - **`CVE-2026-44477` / `GHSA-423p-g724-fr39`: metrics exporter privilege escalation**: the metrics exporter no longer authenticates as the `postgres` superuser. It now uses a dedicated `cnpg_metrics_exporter` role with `pg_monitor` privileges only, closing a chain that let a low-privilege database user gain PostgreSQL superuser. ([`GHSA-423p-g724-fr39`](GHSA-423p-g724-fr39)) <!-- 1.29 1.28 1.25 --> Upgrade impact: custom monitoring queries that read user-owned tables, or use `target_databases: '*'` against databases where `PUBLIC CONNECT` has been revoked, need explicit `GRANT` statements to `cnpg_metrics_exporter`. See ["Custom query privileges and safety"](../monitoring.md#custom-query-privileges-and-safety) and ["Manually creating the metrics exporter role"](../monitoring.md#manually-creating-the-metrics-exporter-role) in the monitoring documentation. For replica clusters, upgrade the source primary cluster before any replica clusters that consume from it. The `cnpg_metrics_exporter` role is created on the source primary and replicates downstream; a replica cluster upgraded first will scrape against a missing role until the source primary upgrades. The manual-recovery section linked above also covers replica clusters. - **Schema-qualified catalog references in default monitoring queries**: hardened the shipped monitoring configuration and documentation samples by qualifying every `pg_catalog` object explicitly. Unqualified references resolve through `search_path`, which a database user can manipulate to shadow built-in objects. ([#10576](cloudnative-pg/cloudnative-pg#10576)) <!-- 1.29 1.28 1.25 --> - **Discoverable SBOM and provenance attestations**: SBOM and SLSA provenance attached to operator container images now follow the OCI 1.1 Referrers spec, so standard registry tooling and supply-chain scanners can discover them automatically. ([#10601](cloudnative-pg/cloudnative-pg#10601)) <!-- 1.29 1.28 1.25 --> - **CVE remediation in `github.com/jackc/pgx/v5`**: bumped to v5.9.2 to pick up upstream fixes for `CVE-2026-33816` (memory-safety in `pgproto3`) and `GHSA-j88v-2chj-qfwx` (SQL injection via simple-protocol dollar-quoted string handling). ([#10437](cloudnative-pg/cloudnative-pg#10437), [#10499](cloudnative-pg/cloudnative-pg#10499)) - **CVE remediation in the Go runtime**: built with Go 1.26.3 to pick up upstream fixes in `crypto/x509`, `crypto/tls`, `net/http`, and `net` (CVE-2026-32280, CVE-2026-32281, CVE-2026-33810, CVE-2026-33814, CVE-2026-33811, CVE-2026-39825). ([#10463](cloudnative-pg/cloudnative-pg#10463), [#10647](cloudnative-pg/cloudnative-pg#10647)) <!-- 1.29 1.28 1.25 --> - **Build pipeline hardening**: the Go 1.26.3 bump also addresses CVE-2026-42501 (`cmd/go` module-checksum validation), reducing supply-chain exposure during release builds. The affected code paths are not reachable from the running operator. ([#10647](cloudnative-pg/cloudnative-pg#10647)) <!-- 1.29 1.28 1.25 --> ##### Changes - Switched TLS peer verification from `VerifyPeerCertificate` to `VerifyConnection`, which runs on every completed handshake (the former is skipped on resumed TLS 1.3 sessions). Session resumption is not enabled in CloudNativePG today, so this has no observable effect, but it future-proofs verification if session caching is introduced later. ([#10478](cloudnative-pg/cloudnative-pg#10478)) <!-- 1.29 1.28 1.25 --> ##### Fixes - Fixed a failover window where the former primary kept its primary label. If it returned during failover (for example, after a transient network partition), the `-rw` service kept routing to it, replicas could reconnect, and committed writes were lost to `pg_rewind`. The old primary is now labeled `unhealthy` to isolate it from service traffic during failover. ([#10409](cloudnative-pg/cloudnative-pg#10409)) <!-- 1.29 1.28 1.25 --> - Fixed failover not being triggered when the node hosting the primary becomes unreachable. The operator now reads the pod's `Ready` condition (flipped to `False` by the node controller when the kubelet stops reporting) instead of `ContainersReady`, which stays stale as `True` in that scenario. Combined with the spurious-failover guard ([#10445](cloudnative-pg/cloudnative-pg#10445)), failover triggers only when Kubernetes itself marks the pod not Ready. ([#10448](cloudnative-pg/cloudnative-pg#10448)) <!-- 1.29 1.28 1.25 --> - Fixed spurious failovers caused by transient failures on the primary's HTTP status endpoint. ([#10445](cloudnative-pg/cloudnative-pg#10445)) <!-- 1.29 1.28 1.25 --> - Fixed escaping of backslashes and control characters in PostgreSQL configuration values. Previously, such characters in parameters like `log_line_prefix` could corrupt the configuration file or be silently stripped at runtime. ([#10515](cloudnative-pg/cloudnative-pg#10515)) <!-- 1.29 1.28 1.25 --> - Fixed `restore_command` construction to shell-quote each argument. Values such as a `destinationPath` containing whitespace (for example, `s3://my bucket/wal`) were word-split by the POSIX shell and passed to the WAL restore tool as separate arguments. ([#10518](cloudnative-pg/cloudnative-pg#10518)) <!-- 1.29 1.28 1.25 --> - Tightened `recoveryTarget` validation in the admission webhook: `targetXID` must now be a non-negative 32-bit integer, and `targetName` must be shorter than 64 bytes and free of ASCII control characters. Malformed values are rejected at admission instead of failing later during PostgreSQL recovery. ([#10565](cloudnative-pg/cloudnative-pg#10565)) <!-- 1.29 1.28 1.25 --> - Fixed snapshot restores failing when leftover `pgsql_tmp*` directories were present in the data directory. ([#10447](cloudnative-pg/cloudnative-pg#10447)) <!-- 1.29 1.28 1.25 --> - Fixed a deadlock occurring when PVC storage size and resource requests are changed simultaneously. ([#10427](cloudnative-pg/cloudnative-pg#10427)) <!-- 1.29 1.28 1.25 -->
5 participants
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]