Feat: Allow certs for PostgreSQL connections#416
Conversation
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
Signed-off-by: Alexander Laye <alaye@microsoft.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens PostgreSQL connectivity for the DocumentDB Kubernetes Operator by tightening default pg_hba rules and introducing an API surface (spec.tls.postgres) to optionally supply CloudNative-PG PostgreSQL server/replication certificate Secrets—supporting mTLS for cross-cluster replication while keeping existing gateway loopback behavior.
Changes:
- Replace permissive PostgreSQL
pg_hbaentries with least-privilege defaults and replication rules (streaming_replica+ cert/TLS-aware behavior). - Introduce
spec.tls.postgresbacked by CloudNative-PGCertificatesConfiguration, with CRD CEL validation to enforce secret pairing invariants. - Propagate certificate configuration into CNPG Cluster spec (including patch/diff support) and update multi-region replication connection parameters to use TLS client certs when provided; update docs/playground and tests accordingly.
Reviewed changes
Copilot reviewed 22 out of 24 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| operator/src/internal/controller/physical_replication.go | Builds external cluster connection parameters with optional TLS client cert/root CA and updates replication patch logic. |
| operator/src/internal/controller/physical_replication_test.go | Adds tests covering external cluster detail patching and Postgres cert-related replication behavior. |
| operator/src/internal/controller/certificate_controller_test.go | Extends cert controller tests to ensure Postgres cert resources are not managed/created unexpectedly. |
| operator/src/internal/controller/backup_controller_test.go | Test formatting/indentation update (no behavioral change). |
| operator/src/internal/cnpg/cnpg_sync.go | Adds CNPG Cluster patching support for /spec/certificates changes. |
| operator/src/internal/cnpg/cnpg_patch.go | Adds JSON patch path constants for certificates and pg_hba. |
| operator/src/internal/cnpg/cnpg_cluster.go | Sets CNPG spec.certificates from spec.tls.postgres and hardens default pg_hba. |
| operator/src/internal/cnpg/cnpg_cluster_test.go | Updates/extends tests for Postgres certificate config presence and pg_hba expectations. |
| operator/src/config/crd/bases/documentdb.io_dbs.yaml | Updates generated CRD schema for spec.tls.postgres and adds CEL validation rule. |
| operator/src/api/preview/zz_generated.deepcopy.go | Regenerates deep-copies to support CNPG CertificatesConfiguration on TLS spec. |
| operator/src/api/preview/documentdb_types.go | Replaces placeholder PostgresTLS with CNPG CertificatesConfiguration + XValidation. |
| operator/documentdb-helm-chart/crds/documentdb.io_dbs.yaml | Mirrors CRD schema changes for Helm-managed CRDs. |
| documentdb-playground/aks-fleet-deployment/documentdb-resource-crp.yaml | Adds cert-manager resources and fleet Secret propagation for cross-region replication TLS. |
| documentdb-playground/aks-fleet-deployment/documentdb-operator-crp.yaml | Adjusts fleet placement affinity for operator resources. |
| documentdb-playground/aks-fleet-deployment/deploy-multi-region.sh | Adds SAN computation + more robust apply logic with webhook readiness/retries. |
| documentdb-playground/aks-fleet-deployment/deploy-fleet-bicep.sh | Hardens shell settings/quoting and pins external repos to tags; adds RBAC bindings and hub CA handling. |
| docs/operator-public-documentation/preview/multi-region-deployment/setup.md | Documents replication TLS options and SAN requirements; updates backup link. |
| docs/operator-public-documentation/preview/multi-region-deployment/overview.md | Updates replication traffic guidance to reference spec.tls.postgres behavior. |
| docs/operator-public-documentation/preview/getting-started/deploy-on-aks.md | Fixes/updates doc links for storage and TLS configuration. |
| docs/operator-public-documentation/preview/configuration/tls.md | Adds PostgreSQL certificate configuration documentation and examples. |
| docs/operator-public-documentation/preview/architecture/overview.md | Updates backup doc link location. |
| docs/operator-public-documentation/preview/api-reference.md | Updates API reference to reflect CertificatesConfiguration and removes placeholder PostgresTLS. |
| CHANGELOG.md | Adds security and feature entries describing pg_hba hardening and new Postgres TLS support. |
Files not reviewed (1)
- operator/src/api/preview/zz_generated.deepcopy.go: Generated file
|
🤖 Auto-triaged by documentdb-triage-tool. Applied: Reasoningcomponent from path globs (docs, controllers, playground, test, api, manifests); P0 keyword "security" in body; effort from diff stats (1190+172 LOC, 24 files) If a label is wrong, remove it manually and ping |
Signed-off-by: Alexander Laye <alaye@microsoft.com>
7fb40af to
2cb5c35
Compare
|
Thanks for tackling replication TLS — the hardened 1. Is
|
WentingWu666666
left a comment
There was a problem hiding this comment.
Marking this as request-changes — not because I think any specific answer is required, but because these four points concern the public API surface (hard to change once released) and the security posture of cross-region replication, so I'd like us to work through them deliberately before merging rather than after.
I've laid out the details and rationale in my earlier comment. The four things I'd ask you to think carefully about:
- Naming — does
spec.tls.postgresdescribe the real scope (replication only), or could it imply app→DB coverage it doesn't provide? - CNPG type leakage — is exposing
cnpgv1.CertificatesConfigurationdirectly in our CRD a coupling we want to commit to long-term? verify-fullguarantee — is thesslmode=requirepartial-cert path (MITM-exposed) something we should ship and call "mTLS," or should the two secrets be required as a pair?- Silent plaintext default — is the cert-less
trustfallback (and its Istio-mesh assumption) safe as an implicit default, or should it be an explicit opt-in?
No rush to agree with my leanings — I'd just like your reasoning on each before we lock the API in. Happy to pair or prototype if that helps move it forward.
Duplicate — superseded by the request-changes review directly below.
|
To address your questions from the comment above:
|
Removes the permissive pg_hba rules that allowed passwordless PostgreSQL access from any address and replaces them with least-privilege rules plus optional certificate-based mutual TLS for replication. Adds a new spec.tls.postgres field so operators can supply their own CA/certificate Secrets for PostgreSQL server and replication connections.
Motivation
Previously the operator emitted:
Anyone with pod-network reachability could connect to PostgreSQL unauthenticated. This PR closes that gap while keeping the gateway sidecar (loopback) and cross-region replication working.
Changes
PostgreSQL access hardening (cnpg_cluster.go)
Base pg_hba is now host all all localhost trust (gateway sidecar over pod loopback) + hostssl replication streaming_replica all cert.
New spec.tls.postgres API (documentdb_types.go)
spec.tls.postgres is now backed by CNPG's CertificatesConfiguration (replaces the empty PostgresTLS placeholder). Supports serverTLSSecret/serverCASecret and replicationTLSSecret/clientCASecret.
A CEL XValidation rule enforces pairing invariants: server and replication secrets must each be provided together, and serverTLSSecret requires replicationTLSSecret.
Certificates are propagated to the CNPG Cluster and diffed/patched in cnpg_sync.go (add/replace/remove).
Replication TLS (physical_replication.go)
When replication client certs are provided, cross-region externalClusters connect with sslmode=require (or verify-full when a server CA is present) and present the streaming_replica client certificate.
When spec.tls.postgres is unset in multi-region deployments, replication falls back to host replication streaming_replica all trust, relying on network-layer security (e.g. an Istio/service-mesh mTLS boundary).
Reworked replication patch logic to diff externalClusters, pg_hba, and synchronous independently (proper add/replace/remove ops) instead of only reacting to cluster-name changes.
Docs & playground
New "PostgreSQL certificates" section in the TLS configuration guide; multi-region setup docs updated.
deploy-multi-region.sh / deploy-fleet-bicep.sh generate replication certs with cross-region SANs; new documentdb-resource-crp.yaml propagates cert Secrets across the fleet.
CHANGELOG entries under Security and Major Features.
Tests
New physical_replication_test.go and certificate_controller_test.go coverage; updated CNPG cluster and backup controller tests.