Enable TLS automatically for Amazon RDS endpoints

[morgo]

A connection to an *.rds.amazonaws.com address now verifies against Amazon's RDS root bundle, embedded here, unless the DSN asked for something else.

Why in the driver

Three Block repositories have already written this independently:

	regexp	min TLS	checks AppendCertsFromPEM	bundle
strata pkg/mysqlrds	rds\.amazonaws\.com(:\d+)?$	1.2	no	121 roots
vitess go/vt/topo/mysqltopo	\.rds\.amazonaws\.com(:\d+)?$	Go default	yes	118 roots
spirit pkg/dbconn	\.rds\.amazonaws\.com(:\d+)?$	Go default	no	118 roots

They agree on what should happen and disagree on every detail — including the leading dot, which is the difference between matching RDS and matching anything ending in rds.amazonaws.com. Each also carries its own ~180KB copy of the bundle, and strata's is three ca-west-1 roots newer than the other two.

The driver is the only layer that sees every connection, and the endpoint address is the entire input this needs. Getting it wrong by omission produces an unencrypted connection to a production database rather than an error, which is the kind of default worth moving.

Design

• One line in Config.normalize, so it covers both entry points (ParseDSN and NewConnector) and everything downstream. All the logic is in rds.go, a file upstream does not have — the merge-forward cost is that single call.
• The DSN always wins. It fires only when neither cfg.TLS nor cfg.TLSConfig is set, so tls=false is a working opt-out, as are skip-verify and any registered config. normalize then fills ServerName as it does for any config, making this identity verification rather than just encryption.
• The regexp requires the leading dot. Without it notrds.amazonaws.com matches. That fails safely — verification against RDS roots fails rather than trusting the wrong CA — but a confusing handshake error is still worse than not matching.
• MinVersion is TLS 1.2, not the Go default, so a future change to that default cannot quietly weaken an RDS connection.
• The bundle is the newest of the three, and covers the aws partition only — which is why the regexp deliberately does not match the China (.amazonaws.com.cn) or other-partition endpoint forms, whose roots are not in it. RDSTLSConfig() is exported for an RDS instance reached under a name that doesn't look like one (a CNAME, or a proxy).

Tests

rds_test.go covers the endpoint patterns including the near-misses (notrds.amazonaws.com, an RDS label mid-domain, the cn partition), the precedence of each way a DSN can specify TLS, and the independence of per-connection configs — normalize writes ServerName into cfg.TLS, so a shared config would let one connection's expected identity overwrite another's.

TestRDSGlobalBundleParses guards the bundle itself: a truncated PEM parses into an empty pool and then fails every RDS connection at handshake time, a long way from the mistake. It also fails if every root has expired. Currently 121 roots, 93 unexpired.

Full suite passes against MySQL 8.0.44 with -race.

After this and the rejectReadOnly change merge

strata, vitess and spirit can each delete their copy — bundle, regexp, RegisterTLSConfig call and the branches around it.

[coveralls]

Coverage Report for CI Build 34064570862

Coverage increased (+0.2%) to 84.575%

Details

• Coverage increased (+0.2%) from the base build.
• Patch coverage: 37 of 37 lines across 2 files are fully covered (100%).
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	4227
Covered Lines:	3575
Line Coverage:	84.58%
Coverage Strength:	331445.44 hits per line

---

💛 - Coveralls

[Copilot]

🔵 Needs a closer look

It changes default transport security behavior in a database driver and embeds/depends on a large CA bundle, so it warrants final human review despite strong tests.

Pull request overview

This PR adds driver-level automatic TLS enablement for connections targeting Amazon RDS/Aurora endpoints by defaulting to an embedded AWS RDS root CA bundle when the DSN/Config does not explicitly specify a TLS mode, making verified encryption the default for *.rds.amazonaws.com addresses.

Changes:

• Add rds.go implementing RDS endpoint detection and an RDSTLSConfig() helper backed by an embedded global RDS bundle (TLS min 1.2).
• Invoke RDS auto-TLS from Config.normalize() when neither cfg.TLS nor cfg.TLSConfig is set.
• Add rds_test.go to validate endpoint matching, DSN precedence, and bundle parse/expiry sanity.

File summaries

File	Description
README.md	Documents the new “RDS auto-TLS” capability and how it fits the fork’s goals.
dsn.go	Adds a Config.normalize() hook to apply RDS auto-TLS before existing TLS selection logic.
rds.go	Implements RDS endpoint matching, embedded bundle parsing, and auto-TLS application logic.
rds_test.go	Adds tests for address matching, precedence rules, and embedded bundle validity checks.
rdsGlobalBundle.pem	Adds the embedded Amazon RDS global trust bundle used for verification.

Review details

• Files reviewed: 4/5 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[aparajon]

🤖 Adversarial correctness review — ecb9188c (+3381/-4, 5 files)

This is the right shape for the problem: address-derived rather than DSN-derived means the safe default can't be lost by a ParseDSN/FormatDSN round trip inside the fork, and one line in normalize() keeps the upstream merge near-mechanical. The placement is also exactly right — before the TLSConfig switch, so the DSN still wins, and before the ServerName block, so auto-TLS gets identity verification rather than bare encryption. TestRDSAutoTLS covers the override matrix properly, including the NewConnector struct path.

Also worth saying, since it's the reason this matters beyond convenience: this makes spirit's dbconn.EnhanceDSNWithTLS + RegisterTLSConfig("rds", …) unnecessary. That pair is currently the one exported spirit API that hands a caller a DSN referring to a TLS name in spirit's driver registry, which is the cross-module break I flagged on block/spirit#1219 — and deleting it in favour of this is a cleaner close than documenting it.

Two things I'd want resolved before merge, both measured.

#	Sev	Where	What
1	med	rds.go:35, rds_test.go:31	GovCloud endpoints match rdsAddr but the embedded bundle contains zero GovCloud roots, so a working plaintext connection becomes a failing handshake — and a test pins the match as intended
2	med	rds.go:52, rds.go:66	Every RDSTLSConfig() shares one *x509.CertPool, while its doc comment invites callers to modify the result — appending one CA widens trust for every RDS connection in the process
3	low	rds.go:35	The match is case-sensitive, so an uppercased hostname silently connects in plaintext
4	low	dsn.go:234, README	FormatDSN() on an auto-TLS'd config emits no tls=, so that string handed to a non-fork driver is a plaintext RDS connection

---

1 — GovCloud matches the pattern and isn't in the bundle (med)

{"mydb.cxyz123.us-gov-west-1.rds.amazonaws.com:3306", true},

I parsed the embedded bundle and looked for what would have to be there for that to be safe:

certs in bundle: 121
GovCloud region roots: 0
China region roots:    0

China is excluded by hostname and documented — amazonaws.com.cn doesn't match, which is the whole reason the suffix check works there. GovCloud is the case that has no such escape: its RDS endpoints are <name>.<hash>.us-gov-west-1.rds.amazonaws.com, ordinary amazonaws.com, so they match rdsAddr exactly, and AWS publishes GovCloud's RDS roots in a separate truststore that isn't concatenated into global-bundle.pem.

The consequence is a behaviour change in the direction this file is trying to avoid. Before: a GovCloud DSN with no tls= connects in plaintext, and works. After: it attempts TLS, verifies against 121 commercial roots, and fails with an x509 error naming none of this. That's the notrds.amazonaws.com failure mode the rdsAddr comment already calls out — "a confusing handshake error is still worse than not matching" — arrived at from the other direction.

Copilot's review comment is pointing at the same seam from the doc side. The revision in ecb9188 resolved the inconsistency by dropping GovCloud from the prose and describing China instead, which makes the comment self-consistent but leaves rdsAddr, the bundle, and rds_test.go:31 disagreeing with each other. Three ways out, in my order of preference:

• Exclude it, the way China is excluded: \.rds\.amazonaws\.com$ with a us-gov- region guard, flip the test expectation, and let RDSTLSConfig() be the documented GovCloud path (it already is the escape hatch for "an RDS instance under a name that doesn't look like one" — this is the mirror case, a name that looks like one but needs a different bundle). Preserves today's behaviour for GovCloud instead of breaking it.
• Embed the GovCloud bundle too and pick by region. Correct, but it's a second embedded blob with its own refresh cadence, and the region has to be parsed out of the hostname.
• Keep it and say so — but then rds_test.go:31 should carry a comment saying a GovCloud connection is expected to fail verification until the operator registers their own config, because as written that line reads as coverage confirming GovCloud works.

2 — one CertPool behind every config, and the doc invites mutating it (med)

var rdsRootCAs = sync.OnceValue(func() *x509.CertPool { ... })

func RDSTLSConfig() *tls.Config {
	return &tls.Config{RootCAs: rdsRootCAs(), MinVersion: tls.VersionTLS12}
}

Caching the parse is right — parsing 121 certs per connection would be silly. But the doc comment three lines up says:

Each call returns a new config, so it can be modified freely — to trust a different partition's bundle, for instance.

The config is new; the pool inside it is not, and "trust a different partition's bundle" is precisely the operation that reaches through into it. *x509.CertPool has no copy-on-write:

append to a.RootCAs succeeded: true
b.RootCAs (never touched):        121 -> 122
a fresh RDSTLSConfig() pool size: 122
auto-TLS pool for a NEW connection: 122

One caller following the documented advice widens the trust store for every RDS connection in the process, including auto-TLS connections that never asked. And because AppendCertsFromPEM mutates the pool's internals while other goroutines are handshaking against it, it's also a data race, not only a policy leak.

RootCAs: rdsRootCAs().Clone() is the whole fix — (*CertPool).Clone is a shallow copy of the index, so it stays cheap next to a handshake, and the doc comment then means what it says. If you'd rather keep the sharing, the comment needs to say "replace RootCAs, don't append to it," which is a worse contract to hand an operator who is already in an unusual situation.

TestRDSAutoTLS/"each config is independent" is the natural home for the regression: it currently asserts a.TLS != b.TLS, and the field that isn't independent is one line below.

3 — the match is case-sensitive (low)

mydb.cxyz123.us-east-1.rds.amazonaws.com:3306  IsRDSAddr=true   TLS applied=true
mydb.cxyz123.us-east-1.RDS.amazonaws.com:3306  IsRDSAddr=false  TLS applied=false
MYDB.CXYZ123.US-EAST-1.RDS.AMAZONAWS.COM:3306  IsRDSAddr=false  TLS applied=false

DNS is case-insensitive and nothing upstream lowercases cfg.Addr, so a hostname that arrives uppercased from a config file, an env var, or a copy-paste out of a console gets no auto-TLS. This is the failure direction that matters: the README's argument is that "an unencrypted RDS connection is a silent omission rather than an error," and a case variant reintroduces exactly that omission, silently. (?i) on the pattern is the fix, plus one row in TestIsRDSAddr — which is otherwise a good table, including the notrds and .example.test suffix attacks.

4 — the DSN doesn't carry the TLS it got (low)

in  = user:pass@tcp(mydb.cxyz123.us-east-1.rds.amazonaws.com:3306)/db
out = user:pass@tcp(mydb.cxyz123.us-east-1.rds.amazonaws.com:3306)/db   (TLS applied: true, TLSConfig="")

Inside the fork this is a feature — the property is derived from the address, so it can't be dropped by a round trip, which is strictly better than the tls=<name> scheme it replaces. The hazard is only at the boundary: a consumer that parses, edits and re-formats a DSN (the correct way to manipulate one, and what block/schemabot's own conventions mandate) and then hands the string to something built on upstream go-sql-driver gets a plaintext RDS connection with nothing in the string to suggest otherwise. hotswap-dsn-driver is a live instance of that — it imports upstream directly.

Not something to fix in code; the useful version is one line in the README next to "Anything the DSN specifies still wins": auto-TLS is a property of the address, not of the DSN string, so a DSN produced here carries no tls= and only reproduces the behaviour when opened with block-mysql.

---

Verified — the runs, the bundle, and three attacks that dissolved

Local. go build ./..., go vet ./..., gofmt -l clean. The server-independent tests (-run 'RDS|DSN|TLS|Config') pass. CI green on all ten matrix jobs plus the OSS check.

The bundle is what it claims to be. 121 certificates, all parsing, TestRDSGlobalBundleParses asserts a floor rather than an exact count — right call, since a refresh only adds. The curl line in the doc comment is the canonical AWS location, and "adding a root is backwards compatible, so refreshing early costs nothing" is the correct operational instruction to leave behind.

Attack that dissolved: the fork addition landing in the wrong half of normalize(). applyRDSAutoTLS runs after ensureHavePort (so the regex's optional-port arm is what actually matches), before the TLSConfig switch (so tls=false, tls=skip-verify, and a registered name all win — each covered by a subtest), and before the ServerName fill (so auto-TLS verifies identity, not just encryption, which the test pins explicitly). Every one of those orderings is load-bearing and every one is correct.

Attack that dissolved: normalize() mutating a shared *tls.Config. The ServerName block writes into cfg.TLS in place with no clone. That's safe for the upstream branches because each allocates fresh or goes through getTLSConfigClone, and safe for the new branch because RDSTLSConfig() returns a new struct per call — "each config is independent" pins it. It's only the RootCAs pointer inside that struct that's shared, which is finding 2 and is a different failure.

Attack that dissolved: the empty-pool failure mode. AppendCertsFromPEM's return value is discarded, so a malformed bundle would yield an empty pool. That fails closed — every RDS connection fails verification rather than trusting anything — the comment says so explicitly, and TestRDSGlobalBundleParses catches it in CI before it can ship. Right trade for a sync.OnceValue on a call path with nowhere to return an error.

Leak check: clean. The one internal reference is "Block had three separate copies of exactly that before this existed" in applyRDSAutoTLS's comment, which names no system, repo, or environment and is the honest motivation for a fork addition. Nothing else in the diff is internal.

The probe files were moved out of the tree; the worktree is clean at ecb9188c.

This review was generated by Claude Code (claude-opus-5).

[aparajon]

🤖 Approving — the shape is right and the normalize() placement is correct on all three orderings that matter. Two measured findings worth resolving first, in the review comment above: GovCloud endpoints match the pattern but have zero roots in the embedded bundle (a working plaintext connection becomes a failing handshake), and every RDSTLSConfig() shares one CertPool while the doc invites mutating it.

This stamp was left by Claude Code (claude-opus-5).

[morgo]

🤖 All four addressed in 7875855. Each of the three code fixes has a test that fails without it — I applied and reverted every mutation to check:

Mutation	Result
drop the GovCloud guard	killed — 3 rows in TestIsRDSAddr
drop .Clone()	killed — each config is independent, appending to a returned pool does not leak
drop (?i)	killed — 2 rows in TestIsRDSAddr

1 — GovCloud excluded. I confirmed the bundle independently before changing anything: 121 certificates, 0 GovCloud roots, 0 China roots. Took your first option, for the reason you gave — matching it turns a working plaintext connection into a failing handshake, which is a regression, and RDSTLSConfig is already the documented path for "an RDS endpoint this bundle cannot verify". govCloudAddr is a second pattern rather than a lookahead, since RE2 has none. Added a row pinning that the exclusion does not over-reach: mydb.gov-thing.us-east-1.rds.amazonaws.com still matches.

2 — pool cloned. RootCAs: rdsRootCAs().Clone(), with the doc updated to say the returned pool is the caller's. The regression test needed a foreign CA rather than a re-append of rdsGlobalBundle — CertPool deduplicates by raw certificate, so re-appending the same bundle is a no-op that would have passed vacuously. It generates a throwaway ed25519 CA, appends it, and asserts a fresh RDSTLSConfig() and a new auto-TLS connection both still equal the pristine pool. It also asserts the append actually changed the mutated pool, so the test cannot pass by doing nothing.

3 — (?i). Both your uppercase cases are now rows in the table.

4 — README. Added the paragraph: auto-TLS is a property of the address, not the DSN string, so FormatDSN output carries no tls= and only reproduces the behaviour when reopened with block-mysql. Also documented both excluded partitions there, since that is where someone in GovCloud will look.

On your framing point — agreed, and it is now on the retirement list: this makes spirit's EnhanceDSNWithTLS + RegisterTLSConfig("rds", …) deletable, which closes the cross-module hazard from block/spirit#1219 properly rather than by documentation. dbconn.IsRDSHost is exported and called by block/schemabot, so it will forward to mysql.IsRDSAddr rather than being deleted outright.