Skip to content

Feat: Allow certs for PostgreSQL connections#416

Open
alaye-ms wants to merge 10 commits into
documentdb:mainfrom
alaye-ms:users/alaye/hardenPGConnection
Open

Feat: Allow certs for PostgreSQL connections#416
alaye-ms wants to merge 10 commits into
documentdb:mainfrom
alaye-ms:users/alaye/hardenPGConnection

Conversation

@alaye-ms

@alaye-ms alaye-ms commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

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.

alaye-ms added 6 commits July 1, 2026 11:10
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>
Copilot AI review requested due to automatic review settings July 7, 2026 14:51
Signed-off-by: Alexander Laye <alaye@microsoft.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_hba entries with least-privilege defaults and replication rules (streaming_replica + cert/TLS-aware behavior).
  • Introduce spec.tls.postgres backed by CloudNative-PG CertificatesConfiguration, 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

Comment thread operator/src/internal/controller/certificate_controller_test.go
Comment thread docs/operator-public-documentation/preview/multi-region-deployment/setup.md Outdated
Signed-off-by: Alexander Laye <alaye@microsoft.com>
@documentdb-triage-tool documentdb-triage-tool Bot added documentation Improvements or additions to documentation ecosystem enhancement New feature or request go Pull requests that update go code test labels Jul 7, 2026
@documentdb-triage-tool

Copy link
Copy Markdown

🤖 Auto-triaged by documentdb-triage-tool.

Applied: documentation, go, ecosystem, test, enhancement
Project fields suggested: Component docs · Priority P0 · Effort XL · Status Needs Review
Confidence: 0.95 (deterministic)

Reasoning

component 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 @patty-chow so the rules can be tuned. The bot will not re-label items that already have component labels.

Signed-off-by: Alexander Laye <alaye@microsoft.com>
@alaye-ms alaye-ms force-pushed the users/alaye/hardenPGConnection branch from 7fb40af to 2cb5c35 Compare July 7, 2026 19:37
@WentingWu666666

WentingWu666666 commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Thanks for tackling replication TLS — the hardened pg_hba and cross-cluster wiring look solid. I have a few questions about the public API shape and defaults that I think are worth thinking through before this locks in. None of these are blockers from me — more things I'd love your take on.

1. Is spec.tls.postgres the right name?

Its siblings under spec.tls (gateway, globalEndpoints) name the communication surface, whereas postgres names the software. As far as I can tell this field only secures replication (intra-cluster + cross-region) — apps reach Postgres through the gateway over loopback trust, not this path.

Worth considering: could a user read tls.postgres as securing their app → DB traffic and get a false sense of coverage? Would something like spec.tls.replication describe the actual scope more honestly? Curious whether you see a future where this widens beyond replication (e.g. direct Postgres access), which might justify keeping postgres.

2. Should we expose cnpgv1.CertificatesConfiguration directly?

The API currently surfaces the CNPG type as-is (4 fields: serverTLSSecret, serverCASecret, replicationTLSSecret, clientCASecret). Two things worth weighing:

  • Does coupling our public CRD to a CNPG struct risk a CNPG-side change becoming a breaking change to our API?

  • Since a kubernetes.io/tls Secret already bundles cert + key + ca.crt, could the four fields collapse to two? Your own example reuses one secret name per pair, which made me wonder if the split is redundant:

    spec:
      tls:
        replication:
          serverCertSecret: <secret>   # → serverTLSSecret + serverCASecret
          clientCertSecret: <secret>   # → replicationTLSSecret + clientCASecret

    Would deriving the CNPG fields from two bundled secrets be simpler for users?

3. What happens on the partial-cert path?

When only a client cert is present, the code uses sslmode=require, which encrypts the connection but doesn't verify the server's identity. That leaves it open to a man-in-the-middle (MITM) attack — an attacker on the network between the replica and primary can impersonate the primary, terminate the replica's TLS, and relay to the real primary while reading or altering the replication stream in between. The connection is encrypted, but potentially to the attacker. This path is currently described as "mTLS" in the docs.

Worth thinking through: in bidirectional replication (either cluster can become primary after failover), each cluster has to act as both server and client — so is there actually any coherent single-sided config? If not, would it make sense to require the two secrets as a pair (both-or-neither) so we always land on verify-full (which verifies the server cert against a trusted CA and closes the MITM gap)? And should the "mTLS" wording be reserved for the verify-full path only?

Note the two certs generally aren't interchangeable: the client cert needs CN=streaming_replica (Postgres maps the CN to that role) plus clientAuth EKU, while the server cert needs the endpoint host in its SAN plus serverAuth EKU. So a normal server cert can't double as the client cert. A single secret can serve both only if it's a purpose-built cert (CN=streaming_replica, host in SAN, both EKUs) — which is exactly why the two-field shape (serverCertSecret + clientCertSecret, each mapped by the operator onto the CNPG fields) fits the underlying reality better than a single field.

4. Is the plaintext fallback intentional?

When multi-region replication is set up without certs, pg_hba becomes host replication streaming_replica all trust cluster-wide — plaintext and unauthenticated across regions, and it also seems to relax intra-cluster enforcement. The code comment suggests this assumes an Istio mesh.

Is that assumption safe as a silent default? Would it be worth making the mesh-only path an explicit opt-in so the security posture is visible in the spec rather than implied by omission?


Happy to pair on any of these or prototype the type changes if that helps. Mostly wanted to surface them while the API is still easy to shape.

@WentingWu666666 WentingWu666666 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate — please see the request-changes review below.

@WentingWu666666 WentingWu666666 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Naming — does spec.tls.postgres describe the real scope (replication only), or could it imply app→DB coverage it doesn't provide?
  2. CNPG type leakage — is exposing cnpgv1.CertificatesConfiguration directly in our CRD a coupling we want to commit to long-term?
  3. verify-full guarantee — is the sslmode=require partial-cert path (MITM-exposed) something we should ship and call "mTLS," or should the two secrets be required as a pair?
  4. Silent plaintext default — is the cert-less trust fallback (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.

@WentingWu666666 WentingWu666666 dismissed their stale review July 8, 2026 15:42

Duplicate — superseded by the request-changes review directly below.

@alaye-ms

alaye-ms commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

To address your questions from the comment above:

  1. The spec.tls.postgres name was already present as a placeholder in the code, and I think changing that name is out of scope for this PR, though I do agree with the sentiment.

  2. A change to the CNPG implementation of certificates would in either case require a change from our side due to the direct implementation. I think simple pass throughs are better here and should cnpg make a change, we could address whether to keep our current surface or add an extra layer then.

  3. Good catch on the wording, I'll make sure to edit the docs to make clear that the client-only certs are not mutual TLS. As far as removing the feature altogether, I don't think that's needed once the documentation accurately reflects the reality of the path.

  4. I think explicit opt-in for the mesh path is a good idea, I'll add that now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation ecosystem enhancement New feature or request go Pull requests that update go code test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants