Skip to content

[dev] [Marfuen] mariano/rds-tls-followup - #2774

Closed
github-actions[bot] wants to merge 2 commits into
mainfrom
mariano/rds-tls-followup
Closed

github-actions[bot] wants to merge 2 commits into
mainfrom
mariano/rds-tls-followup

Conversation

@github-actions

@github-actions github-actions Bot commented May 6, 2026 •

Copy link
Copy Markdown
Contributor

This is an automated pull request to merge mariano/rds-tls-followup into dev.
It was created by the [Auto Pull Request] action.


Summary by cubic

Hardened Prisma TLS for Postgres via RDS Proxy/NLB by combining the RDS CA bundle with Node’s roots and adding a scoped hostname check that only accepts RDS endpoints. Applied across the db package and all app Prisma clients to fix issuer errors without disabling identity verification.

  • Bug Fixes

    • Verified TLS: combine RDS_CA_BUNDLE with tls.rootCertificates so direct RDS and Proxy chains validate.
    • Hostname verification: replace no-op with rdsServerIdentity that accepts CN/SAN ending in .rds.amazonaws.com or .rds.amazonaws.com.cn, with full chain validation.
    • Applied in packages/db/src/ssl-config.ts and inlined in apps (api, app, portal, framework-editor); updated tests for resolver and identity checks.
  • Dependencies

    • Bump @trycompai/db to 2.2.1.

Written for commit fc3b7a3. Summary will update on new commits.

… to RDS hosts

Two related fixes against the strict-TLS path introduced in #2772:

1. Combine `ssl.ca` with Node's default trust roots
   ----------------------------------------------------
   Setting `ssl.ca` *replaces* Node's trust store rather than augmenting it.
   Our `rds-global-bundle.pem` only contains the 108 RDS-specific regional
   self-signed CAs, NOT the public roots like Amazon Root CA 1, which is
   where AWS RDS Proxy chains terminate (and which lives in Node's default
   Mozilla bundle).

   Surfaced by comp-private/apps/trust during prerender of /sitemap.xml:
     Error opening a TLS connection: unable to get local issuer certificate

   apps/app and apps/portal didn't trip this because none of their
   prerendered routes hit the DB; latent issue if any do later. Combine
   `RDS_CA_BUNDLE` with `tls.rootCertificates` so direct-instance and
   RDS Proxy paths both validate.

2. Replace `checkServerIdentity: () => undefined` with a scoped check
   ----------------------------------------------------------------
   Disabling identity verification entirely lets an attacker who controls
   the NLB hostname's DNS substitute *any* chain-valid cert. The reason
   we skip is specifically that connections traverse an AWS NLB (TCP
   passthrough) → RDS Proxy, and the NLB hostname (`*.elb.amazonaws.com`)
   isn't in the RDS Proxy cert's SAN list.

   Replace the no-op with `rdsServerIdentity()` which asserts the cert's
   CN or SAN ends in `.rds.amazonaws.com[.cn]`. Combined with the pinned
   trust store + chain validation, an attacker would now need a forged
   or wrong-CA cert *for an RDS hostname*, both of which still fail. The
   NLB hostname mismatch is accepted, the substitute-arbitrary-cert
   weakness is not.

Applied at the source (`packages/db/src/ssl-config.ts`) and inlined into
the apps (apps/{app,portal,framework-editor,api}/prisma/client.ts) since
they don't import from `@trycompai/db/ssl-config` (Trigger.dev indexer
pins to the npm version, which lags behind workspace).

Bumps `@trycompai/db` to 2.2.1 so downstream consumers (comp-private)
get the corrected behavior at the source on their next dep bump.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented May 6, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
app Ready Ready Preview, Comment May 6, 2026 9:23pm
comp-framework-editor Ready Ready Preview, Comment May 6, 2026 9:23pm
portal Ready Ready Preview, Comment May 6, 2026 9:23pm

Request Review

…a client

Missed in the previous commit — apps/app/prisma/client.ts also needs the
combined trust store (RDS bundle + Node defaults) and the scoped
rdsServerIdentity check, since it doesn't import from
@trycompai/db/ssl-config (Trigger.dev indexer pinning concern).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot 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.

1 issue found across 6 files

Confidence score: 2/5

  • I’m scoring this as high risk because the issue is security-relevant (severity 6/10, confidence 7/10): TLS identity verification in apps/api/prisma/client.ts appears too permissive rather than strict.
  • The custom checkServerIdentity behavior could allow trusting any AWS RDS certificate without confirming it matches the intended endpoint host, which weakens hostname validation and increases MITM/regression risk.
  • This is likely merge-blocking until tightened, since certificate-host binding is a core transport security control for database connections.
  • Pay close attention to apps/api/prisma/client.ts - ensure checkServerIdentity validates the certificate identity against the exact target host, not just AWS RDS affiliation.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/api/prisma/client.ts">

<violation number="1" location="apps/api/prisma/client.ts:21">
P2: The custom `checkServerIdentity` is too permissive: it accepts any AWS RDS certificate and does not bind certificate identity to the intended endpoint host.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.

Comment thread apps/api/prisma/client.ts
.split(',')
.map((s) => s.trim().replace(/^DNS:/, ''));
const cn = (cert.subject as { CN?: string } | undefined)?.CN ?? '';
if (isRdsHostname(cn) || sans.some(isRdsHostname)) return undefined;

@cubic-dev-ai cubic-dev-ai Bot May 6, 2026 •

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.

P2: The custom checkServerIdentity is too permissive: it accepts any AWS RDS certificate and does not bind certificate identity to the intended endpoint host.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/prisma/client.ts, line 21:

<comment>The custom `checkServerIdentity` is too permissive: it accepts any AWS RDS certificate and does not bind certificate identity to the intended endpoint host.</comment>

<file context>
@@ -1,10 +1,29 @@
+    .split(',')
+    .map((s) => s.trim().replace(/^DNS:/, ''));
+  const cn = (cert.subject as { CN?: string } | undefined)?.CN ?? '';
+  if (isRdsHostname(cn) || sans.some(isRdsHostname)) return undefined;
+  return new Error(
+    `TLS hostname check: cert is not for an AWS RDS endpoint (CN=${cn}, SANs=${sans.join(',')})`,
</file context>
Fix with Cubic

@Marfuen

Marfuen commented May 6, 2026

Copy link
Copy Markdown
Contributor

Closing — over-engineered. The combine-with-rootCertificates and custom rdsServerIdentity were chasing narrow threat models inside an AWS VPC. The actual Cubic finding (silent rejectUnauthorized: false fallback) was already fixed by #2772. Beyond that, Node's default trust store includes Amazon Root CA 1 which is sufficient for RDS Proxy chain validation. Comp-private PR #276 was simplified accordingly.

@Marfuen Marfuen closed this May 6, 2026

This branch was successfully deployed

2 active and 1 inactive deployments
Preview – app — fc3b7a30 Deployed May 6, 2026 by vercel[bot]
Preview – comp-framework-editor — fc3b7a30 Deployed May 6, 2026 by vercel[bot]
Preview – portal — fc3b7a30 Deployed May 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant