dbconn: take RDS TLS from the driver, and drop rejectReadOnly - #1221
Conversation
block/mysql now carries the RDS certificate bundle and applies verified TLS
to an RDS address by itself, so spirit's copy of the same machinery is
duplication with a second refresh schedule. rdsGlobalBundle.pem (2952 lines),
the pool built from it, and the local rdsAddr regex go; NewTLSConfig and
IsRDSHost stay as thin forwarders and keep their behaviour.
They stay because two callers still need them, and neither is a database/sql
connection the driver would cover: GetTLSConfigForBinlog builds a config for
the go-mysql binlog client, and block/schemabot calls IsRDSHost to pick a TLS
mode. initRDSTLS keeps registering under "rds" for the same reason —
EnhanceDSNWithTLS returns DSNs naming it, and schemabot opens those.
Delegating changes two things. The host match is now case-insensitive, which
is a fix: DNS is case-insensitive, nothing normalizes the host, and an
uppercased hostname is the same endpoint. GovCloud and China endpoints now
report false, because the bundle holds no roots for either partition and
verifying against it could only ever fail. Also new for free: MinVersion is
pinned to TLS 1.2, which spirit's config did not set.
Two changes here are forced by the pin rather than chosen, and both are
behaviour, not cleanup.
--tls-mode=DISABLED now writes tls=false instead of leaving tls= empty. An
empty tls= is precisely what the driver's auto-TLS treats as "nothing was
asked for", so on an RDS host DISABLED would have produced a TLS connection —
the opposite of what was requested, from a dependency bump, with no error.
Measured, with the old assignment restored:
mode=DISABLED host=db.cxyz.us-east-1.rds.amazonaws.com effective_tls=true
mode=DISABLED host=mysql.internal effective_tls=false
It is RDS-only, so no test against a local MySQL could have seen it — and the
tests that covered DISABLED asserted the DSN string "should not contain tls=",
which is the spelling rather than the effect. Those now assert cfg.TLS is nil,
via a requireNoEffectiveTLS helper, and TestNewDSNDisabledMode covers an RDS
address explicitly.
AllowCleartextPasswords had to grow a second condition in the same breath. It
was `cfg.TLSConfig != ""`, which reads the new "false" as "TLS is on" and
would send passwords in the clear over a plaintext connection. Both guards
are load-bearing and each fails on its own mutation:
KILLED DISABLED writes empty tls= (the original bug)
KILLED cleartext guard drops the tlsDisabled check
KILLED NewTLSConfig returns an empty root pool
KILLED NewCustomTLSConfig ignores the empty-certData fallback
DBConfig.RejectReadOnly is deleted because the driver deleted the option it
mapped to: read-only errors now always recycle the connection. The field also
carried an opt-out that the sync runner set for its read-only source, and that
turned out to be guarding against an error the workload cannot raise —
1290/1792/1836 come from *writes*, not from connecting to a read-only server.
Verified against a super_read_only MySQL 8.0 as the restricted source user:
SELECT, SHOW TABLES, SHOW CREATE TABLE, SHOW MASTER STATUS, every session SET
newDSN adds, and the binlog client's FLUSH BINARY LOGS all succeed; INSERT is
what returns 1290. If sync ever does write to its source, that is a bug, and
the rejection now surfaces it rather than hiding it.
Full suite run against MySQL 8.0.45: identical pass/fail set to origin/main,
no new failures. pkg/dbconn green.
|
🤖 Adversarial correctness review — The two forced behaviour changes are correctly identified and correctly paired. The Two findings, both about what the retained surface promises callers.
1 — the registration's stated beneficiary is the consumer it breaks (med)// The registration survives the driver's own auto-TLS because the name is part
// of this package's contract, not an internal detail: EnhanceDSNWithTLS returns
// DSNs carrying tls=rds, and block/schemabot opens them.The first half is right and the retention is right. The second half is the part I'd change, because it reads as reassurance and the situation is the opposite. Measured, with a scratch module importing this branch's It fails at To be clear about blame: this is not introduced here. Spirit's move to So: keep the registration, and say what actually has to happen. Something like "the name is what The alternative worth considering, since 2 — the DISABLED fix landed on one of the two DSN producers (low-med)
case "DISABLED":
return dsn, nil // No TLS neededand Spirit's one internal caller is safe, and I checked rather than assumed: But the export is documented as producing a DSN to open, the reason it exists is to hand one to a caller that will open it themselves, and for Verified — the delegation, and three attacks that dissolvedThe delegation is sound at the pinned version. Attack that dissolved: the private-pool claim resting on an untested dependency property. Attack that dissolved: Attack that dissolved: the Local. Leak check: clean. The probe hostname is invented, and the schemabot paths cited are public. This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Approving — the two forced behaviour changes are correctly identified and correctly paired, and I re-ran the mutations that prove it. Findings are in the review comment above; the one worth acting on before the bump propagates is finding 1 — the tls=rds registration is retained on the grounds that schemabot opens those DSNs, and after the block/mysql switch that is exactly the case that fails (invalid value / unknown config name: rds, at the ParseDSN schemabot calls).
This stamp was left by Claude Code (claude-opus-5).
|
🤖 Review findings - created by Kiran's code review agent - for spirit/pull/1221, f224250. Verdict: 8 findings — 0 blocking, 6 non-blocking (one missed downstream symbol, and four unasserted security properties on paths this PR rewired), 2 suggestions. The DISABLED-mode fix is real and its new tests are individually load-bearing. Non-blocking1. The PR keeps three symbols for schemabot's sake and misses a fourth. 2. The binlog TLS path — the stated reason 3. 4. A 0-byte 5. The deleted bundle's per-certificate validation is not replaced. The old 6. The datasync source-config relaxation this PR rewrites is unpinned. Deleting General suggestions7. 8. These security assertions only ever run inside the Docker+MySQL matrix. The new TLS/DSN tests need no database — they pass standalone — but there is no unit-only job: The one thing that could have broken, verifiedThe whole PR rests on Verified correct
This review was generated by Claude Code (claude-opus-5). |
…S strength
Two behaviour fixes and the test coverage the review found missing.
EnhanceDSNWithTLS and addTLSParametersToDSN both returned an untouched DSN for
TLSMode=DISABLED. That is the same bug already fixed in newDSN: a DSN carrying
no tls= at all is exactly what the driver reads as permission to apply RDS
auto-TLS, so DISABLED handed RDS callers the opposite of what they asked for.
Both now write tls=false. An explicit tls= in the DSN still outranks the mode,
and a nil config is still a no-op — "nothing was said about TLS" is not the
same as "no TLS", and only the latter should write anything.
initCustomTLS now rejects a configured-but-empty certificate file.
NewCustomTLSConfig reads zero bytes as "use the RDS roots", which is correct
for a caller that named no path and wrong for one that named a path to a
truncated or unpopulated private CA: that operator asked to verify against
their own root and would have silently verified against Amazon's, on a
connection that succeeded. Only the path distinguishes the two intents, so the
check has to live at the read site.
New tls_strength_test.go asserts verification strength rather than struct
shape, which is what every weakening in this package preserves:
- VERIFY_CA is InsecureSkipVerify plus a VerifyPeerCertificate callback, so
the callback is the only thing verifying anything. It is now exercised
against real certificates — a leaf from the configured CA is accepted
despite a mismatched hostname, a leaf from an unknown CA is rejected. The
reviewer's `return nil` mutant degraded the mode to no verification at all
under a name promising the opposite; it now dies.
- InsecureSkipVerify is pinned false for VERIFY_IDENTITY, NewTLSConfig, and
the binlog path. That path is the stated reason NewTLSConfig survives the
retirement and is separate from the database/sql pool, so a weakening there
drops verification on the replication stream while the pool stays correct.
- A config registered under a mode's name is read back through ParseDSN and
checked for that mode's strength, closing the gap where initCustomTLS could
register skip-verify as "verify_identity" undetected.
- The RDS pool is checked for a known Amazon root, restoring what the deleted
bundle's per-certificate test used to cover: spirit no longer owns the
bundle, so what matters is that a future block/mysql bump cannot supply a
pool that is non-empty but not the RDS trust store.
Mutations, all killed:
KILLED VERIFY_CA callback body -> return nil
KILLED VERIFY_IDENTITY InsecureSkipVerify false -> true
KILLED empty-certificate-file check removed
KILLED EnhanceDSNWithTLS DISABLED -> untouched DSN
Three existing DISABLED assertions were asserting the DSN spelling rather than
the effect ("should not contain tls="), which is what made the original bug
invisible. The two table rows now pin tls=false and the replica test asserts no
effective TLS.
Added .github/workflows/unit-tls.yml. Every go test in this repo lived in the
mysql*-docker matrices, so these assertions were gated behind Docker, a MySQL
boot, and each workflow's documentation-only short-circuit. They need no
database and now run on every pull request on their own.
The initRDSTLS comment no longer cites schemabot as evidence the tls=rds
registration works, since after the block/mysql switch schemabot is precisely
what would break. It now states the requirement — open those DSNs with
DriverName — and notes schemabot is switching alongside this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🤖 Addressed. Both reviews converged on the same weak spot from different directions, and it turned out to be a real bug rather than a doc nit. @aparajon finding 2 / @Kiran01bm's framing — fixed, and it was the same bug twice. The three tests that broke were all asserting the DSN spelling ( @aparajon finding 1 — comment rewritten, and you were right that it read as an all-clear. It no longer cites schemabot as evidence the registration works, because after the switch schemabot is the thing that breaks. It now states the requirement (open those DSNs with @Kiran01bm 7 — best finding in either review, fixed. @Kiran01bm 2, 3, 5 — fixed, all as strength assertions rather than shape:
@Kiran01bm 4 — fixed, and it needed to move. A configured-but-empty CA file now errors. The check has to sit at the read site in @Kiran01bm 8 — fixed. Added Mutations, each killed: Not doing two, deliberately:
Full |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
zizmor/cache-poisoning: the job also runs on release tags, where a restored module cache could have been written from a lower-trust context. linter.yml disables the cache for the same reason. The job compiles a single package, so the cache was buying almost nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
TestReplicaTLSEnhancement was the fourth test asserting how a DISABLED DSN is spelled rather than what it does, and the one the new TLS/DSN job could not catch: that job runs pkg/dbconn only, because pkg/migration's TestMain provisions a server. Its table carried a shouldEnhance bool covering two properties that DISABLED has now pulled apart — "returned byte-identical" and "no TLS results". DISABLED writes tls=false, so its text changes while its effect does not; the row asserting equality had to fail. A bool cannot express both, so it is now a three-valued expectation and each row says which property it means. The row that preserves a DSN's own tls=skip-verify still demands byte-identity, because there the text *is* the contract. RequireNoEffectiveTLS moves to pkg/testutils, which both packages already import, and pkg/dbconn keeps a one-line local forwarder so its six call sites are untouched. Two copies of "what counts as no TLS" would let one be strengthened while the other silently stayed weak — the same trap the assertions above fell into. Also dropped expectedReplicaTLSMode from the table: nothing read it. It was formatted like an assertion and was not one. The workflow comment now says it covers pkg/dbconn only, so a green tick there is not mistaken for the whole TLS surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
🤖 Re-review — Both findings are addressed, and the two extras you found on the way are the better half of the commit. I re-ran your mutation table against the workflow's own filter rather than the package, so the result also answers "does the new job actually protect this": Finding 1 is closed properly rather than reworded — the comment now states the requirement, and The empty-CA-file catch is the one I'd have missed. "Named a path to a truncated private CA" and "named no path" are genuinely only distinguishable at the read site, and silently verifying against Amazon's roots on a connection that succeeds is the worst shape that class of bug comes in. One finding on the new workflow.
1 — the filter misses the repo's own naming convention (low)The comment above it is right about the failure mode and calls the shot: "a new TLS test whose name this misses is silently unprotected." That case already exists, because the filter's vocabulary is title-case and the mode names are not:
I'm calling it low rather than medium because neither test is uniquely protecting anything today: I could not construct a mutation that survives the current filter but dies under Residual after that: Also verifiedThe cache commit is a real fix, not a workaround. I looked at it expecting a test-masking change and it isn't one: Three DISABLED assertions moved from spelling to effect. That was the actual reason the original bug was invisible — The RDS-pool test earns its place for the reason given. Spirit no longer owns the bundle, so "non-empty" is not the property worth pinning — "is actually the Amazon trust store" is, and that is what a bad
This review was generated by Claude Code (claude-opus-5). |
aparajon
left a comment
aparajon
left a comment
There was a problem hiding this comment.
🤖 Re-approving on ff47088d so the approval points at what is actually there — the previous one was on f224250b.
Both findings from the first round are closed, and I re-ran your mutation table against the new workflow's own filter rather than the package, so all three kills also confirm the job protects them. One low finding on that filter in the re-review: it is case-sensitive, so Verify misses VERIFY_CA and Disabled misses DISABLED, dropping two pure tests. (?i) recovers both and costs nothing.
This stamp was left by Claude Code (claude-opus-5).
Both blockers landed, in both modules: github.com/block/spirit → 10804bbe (block/spirit#1221 merge commit) vitess.io/vitess → 88d15fda (block/vitess#23 merge commit, release-24.0) `go get github.com/block/spirit@main` resolved to an older revision than the branch pin it replaced — the proxy had not indexed the merge yet — so both are pinned to their merge commits explicitly. The consumer module's vitess replace is byte-identical to the parent's again, which is the invariant its own comment states and which was the source of the CI failure before this. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* feat: use block/mysql (driver name "block-mysql")
block/mysql is now a hard fork with its own module path rather than a
replace-target for go-sql-driver/mysql, so SchemaBot imports it directly and
opens pools under the name it registers, "block-mysql". This also unblocks the
spirit bump: spirit's pkg/dbconn moved to block/mysql and retired its own copy
of the RDS certificate bundle, and a tls=rds DSN from EnhanceDSNWithTLS only
resolves for a consumer using the same driver package's TLS registry.
Both MySQL drivers stay linked, and that is not incidental:
go-mysql/hotswap-dsn-driver embeds upstream go-sql-driver and cannot be pointed
at the fork, so the credential-reloading storage pool keeps returning upstream's
*mysql.MySQLError while every pool SchemaBot opens itself returns the fork's.
The names differ, so registration does not collide -- verified: sql.Drivers()
reports all three of block-mysql, mysql and mysql-hotswap-dsn.
What does not survive that split is errors.As. The two MySQLError structs are
field-identical but live in different packages, so asserting one type silently
returns false for the other -- and silently is the problem. A retry classifier
checking only one type does not fail loudly; it stops recognizing deadlocks and
starts surfacing them as permanent errors. So error codes are now read through
mysqlerr.Number/Is, which accepts either, and no call site asserts a driver's
error type:
- pkg/storage/internal/sqlstore/error_classifier.go -- the storage pool, which
is exactly the pool that can be opened either way
- pkg/mysqlerr.Reason
- pkg/engine/spirit.isLockWaitTimeout
pkg/mysqlerr/number_test.go pins both directions, including a test asserting
the two types are *not* interchangeable, so if a future dependency change ever
merges them the second branch is reported as dead rather than left looking
like superstition.
postgresconn's rdsRootPool took spirit's deleted GetEmbeddedRDSBundle. It now
takes mysql.RDSTLSConfig().RootCAs, which is the pool it wanted anyway: the
roots are the same because RDS issues from the same private Amazon CAs
regardless of engine, and RDSTLSConfig clones per call so nothing is aliased
with the MySQL side.
TestOpenNormalizesRDSDSNBeforeOpening asserted the driver name "mysql", which
now belongs to upstream -- opening under it would silently bypass the fork
rather than fail, so the assertion is worth keeping rather than deleting.
The sadscan annotations cover pre-existing false positives -- a doc-comment URI
shape with literal user:pass placeholders, obviously-fake test fixtures, and
three prose comments containing the word "PlanetScale" -- that this change put
into a commit for the first time by touching those files' imports. Each was
verified byte-identical on main.
Verified: go build ./..., go vet ./... and gofmt clean; go test ./pkg/...
fully green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Tidy the consumer module for the driver switch
Every red check traced to one cause: e2e/consumermodule is a second module,
and its go.mod was not regenerated after the parent's dependency graph changed.
CI runs `go test -race -run '^$' ./...` there and the toolchain refused with
"updates to go.mod needed". Unit Tests got through five minutes of real tests
before dying on the same step; Lint was cancelled behind the Build failure
rather than finding anything.
Its vitess replace was also pinned to an older block/vitess SHA than the
parent's, which the comment directly above it says must not happen ("Mirror the
parent module's replace directives; replaces do not propagate across module
boundaries"). Now matching, with block/mysql picked up and spirit moved onto the
same pin as the parent.
go-sql-driver/mysql stays in the graph as an indirect dependency, which is the
intended end state, not leftover: hotswap-dsn-driver embeds it, so both drivers
link, and pkg/mysqlerr is the only package importing it directly — for exactly
that reason.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Repoint spirit and vitess at merged revisions
Both blockers landed, in both modules:
github.com/block/spirit → 10804bbe (block/spirit#1221 merge commit)
vitess.io/vitess → 88d15fda (block/vitess#23 merge commit, release-24.0)
`go get github.com/block/spirit@main` resolved to an older revision than the
branch pin it replaced — the proxy had not indexed the merge yet — so both are
pinned to their merge commits explicitly.
The consumer module's vitess replace is byte-identical to the parent's again,
which is the invariant its own comment states and which was the source of the
CI failure before this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* mysqlerr: use slices.Contains in Is
golangci-lint's modernize check, on the new file. This is the first run where
lint actually got to execute — the earlier ones were cancelled behind the
consumer-module build failure, so it had never linted this code.
Full-repo golangci-lint v2 is clean locally.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Fix two driver names my sweep missed, and one that was wrong before it
Neither site was an inline sql.Open("mysql", …) literal, which is what the
original sweep grepped for, so both survived it:
pkg/testutil/mysql.go — wait.ForSQL's driver-name argument
pkg/namedlock/…_test.go — a driver name carried in a table field
The testutil one is what failed CI: it runs inside a testcontainers start hook,
so `unknown driver "mysql"` surfaced as `FAIL github.com/…/pkg/namedlock`
rather than as a bad driver name, before any test ran. It is now a named
constant next to the blank import it has to agree with.
Worth recording why only one of the six packages using that helper failed. Only
pkg/namedlock does not link upstream go-sql-driver:
pkg/namedlock upstream linked = 0 ← failed
pkg/pendingdrops upstream linked = 1
pkg/engine/spirit upstream linked = 1
pkg/storage/internal/sqlstore upstream linked = 1
Everywhere else upstream's init registers "mysql", so the readiness probe
resolved and passed — using upstream's driver, not the fork the blank import
declares. That was true before this PR too. So this is not only a fix for the
red package; it is the point at which all six actually probe with block/mysql.
Verified with real containers: namedlock 10.1s, pendingdrops 7.4s,
sqlstore 22.0s, all ok under -tags=integration. golangci-lint clean on all
three tag variants CI runs.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Drop the tidb parser replace
Spirit no longer needs the fork — it carries its own pkg/parser in-tree, and
its go.mod requires upstream github.com/pingcap/tidb/pkg/parser as a plain
indirect with no replace of its own. So the redirect this repo carried "for
SPATIAL index support in Spirit v0.13.0" no longer redirects anything anyone
reaches: `go mod why` now answers "main module does not need package
github.com/pingcap/tidb/pkg/parser".
Removed from both modules, since the consumer module mirrors the parent's
replaces. The upstream indirect requirement stays; it is only the fork
redirection that goes.
Verified: build clean, golangci-lint clean on all three tag variants, and
pkg/engine/spirit green under -tags=integration with real containers (20.2s) —
that being the package that would notice a parser regression.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Reimplement DSN credential reload in-repo; drop go-sql-driver/mysql
Retires github.com/go-mysql/hotswap-dsn-driver, the last thing in
SchemaBot's dependency graph that reached upstream go-sql-driver/mysql,
and with it the whole two-linked-drivers hazard this PR was working
around.
Fixes a startup break found in review. ConnectionDSN injects tls=rds,
and a tls= value is a *name* that only resolves inside the registry of
the driver package that registered it -- Spirit registers "rds" into
block/mysql. Open honoured that; OpenReloadable could not, because the
hot-swap driver embeds upstream and cannot be pointed at the fork. A
MySQL storage pool whose host resolves as RDS failed to open with
"unknown config name: rds", on the startup path, so the server did not
come up. No CI job points storage at an *.rds.amazonaws.com address, so
the break was host-shaped rather than code-shaped and nothing in the
suite could see it.
Mirroring the config into upstream's registry would have worked, but
having one registry is better than keeping two in sync, and dropping the
injection instead would have taken the pool from failing loudly to
connecting in the clear.
pkg/connreload holds the reload machinery, driver-independent: a caller
supplies Resolve (raw DSN -> driver.Connector) and Refused (does this
dial error mean the server rejected these credentials), and the package
owns everything about *when* to reload. That is the subtle part, and it
existed twice -- postgresconn had its own copy, which now goes away.
Both storage pools resolve their DSN through the same secrets
machinery, so a difference in how aggressively they re-resolve it would
have said nothing about either engine.
The reimplementation is not a port. The driver it replaces kept its
reload callback in a package-level variable, so opening a second
reloadable pool silently repointed the first one's reload at the second
one's secret; it had no cooldown, so a secrets-backend outage cost one
resolve per rejected dial; and it pinned the electing dial for the
duration of the reload, holding a pool connection slot. Each of those is
fixed and pinned by a test.
pkg/mysqlerr keeps Number/Is and loses its upstream branch. The helper
is still the right seam -- nothing about a second driver fails to
compile, and the way it breaks is silent -- so a depguard rule now
denies both import paths, with the reason in the message.
Also adds the regression test finding 2 asked for: a reloadable pool
against an RDS host, asserting the injected TLS name resolves *and*
that what the pool dials with verifies the server. Both halves are
needed -- block/mysql applies RDS TLS on its own, so the resolved-trust
assertions alone cannot tell "the name resolved" from "the driver
supplied TLS anyway".
github.com/go-sql-driver/mysql remains in go.mod as an indirect
requirement: testcontainers-go/modules/mysql test-imports it. No
SchemaBot package links it -- verified with go list -deps -test ./...
The sadscan annotations in postgresconn_test.go are incidental: the
scanner reports every finding in a file once the file is touched, and
these are the fake localhost DSNs the suite has always used.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
4 participants
What
Bumps
github.com/block/mysqlto merged master and retires the RDS TLS machinery spirit and the driver now both have. +228 / −3076, the bulk of itrdsGlobalBundle.pem.Deleted: the embedded bundle, the
x509.CertPoolbuilt from it, and the localrdsAddrregex.NewTLSConfigandIsRDSHoststay as thin forwarders tomysql.RDSTLSConfig()/mysql.IsRDSAddr, so there is one bundle to refresh instead of two that can drift.They stay rather than going away because two callers need them and neither is a
database/sqlconnection the driver covers:GetTLSConfigForBinlogbuilds a*tls.Configfor the go-mysql binlog client — Vitess-lineage code, not the driver.dbconn.IsRDSHostto pick a TLS mode (pkg/mysqlconn/mysqlconn.go:49).initRDSTLSkeeps registering under the name"rds"for the same reason:EnhanceDSNWithTLSreturns DSNs carryingtls=rds, and schemabot opens them.Two changes here are forced by the bump, not chosen
Both are behaviour. They are the reason this is not a pure deletion.
1.
--tls-mode=DISABLEDmust now saytls=falseout loudThe driver's auto-TLS fires when the DSN asked for nothing — which is exactly what
cfg.TLSConfig = ""meant. So on an RDS host,DISABLEDwould have produced a TLS connection: the opposite of what the operator asked for, arriving from a dependency bump, with no error to notice.Measured, with the old assignment restored:
It is RDS-only, so no test against a local MySQL could have caught it. And the tests that did cover
DISABLEDasserted the DSN string "should not containtls=" — the spelling, not the effect. Seven such assertions across four files now go through arequireNoEffectiveTLShelper that parses the DSN and checkscfg.TLS == nil, andTestNewDSNDisabledModecovers an RDS address explicitly.2.
AllowCleartextPasswordshad to grow a second condition in the same breathIt was
cfg.TLSConfig != "". With fix 1 in place that reads the new"false"as "TLS is on" — and starts sending passwords in the clear over a plaintext connection. Fixing the first without the second would have been a straight security regression.Each guard fails on its own mutation:
DBConfig.RejectReadOnlyis deletedNot a choice either: block/mysql#5 removed
Config.RejectReadOnlyand made the rejection unconditional, soconn.go:341stopped compiling the moment the pin moved.The interesting half is the opt-out.
pkg/datasync/runner.gosetRejectReadOnly = falsefor its read-only source, on the grounds that "sync's source is read-only by design, so it must not fire." That reasons from the wrong axis: 1290/1792/1836 are raised by writes, not by connecting to a read-only server. A pure reader cannot provoke them.Verified rather than argued — a
super_read_onlyMySQL 8.0, as the restricted source user (SELECT,REPLICATION SLAVE/CLIENT,RELOAD):SETnewDSNadds (sql_mode,time_zone,transaction_isolation, ...)SHOW TABLES,SHOW CREATE TABLE,SELECTSHOW MASTER STATUSFLUSH BINARY LOGS(the one that looked risky)INSERT— negative controlSo the probe can see a failure, and sync's actual workload does not trigger one. If sync ever does write to its source that is a bug, and the rejection now surfaces it instead of hiding it. The target side keeps the protection, which is where the Aurora-failover case actually lives.
Behaviour deltas from delegating the host match
false. The bundle holds no roots for either partition, so verifying against it could only ever fail. Reach them with--tls-certificate-pathand that partition's own bundle.MinVersion: tls.VersionTLS12is now pinned; spirit's config left it to the Go default.Not changed, but worth knowing
NewCustomTLSConfig(nil, ...)still falls back to the RDS roots for a non-RDS host with no--tls-certificate-path. That is inherited behaviour and it is close to useless — a non-RDS server will not present an RDS-issued certificate, soVERIFY_CA/VERIFY_IDENTITYfail by construction. Preserved deliberately: tightening it is a separate behaviour change and does not belong in a dependency bump. Flagged in the doc comment.Verification
go build ./...,go vet ./...,gofmt -l,go mod tidy— all cleango test ./...against MySQL 8.0.45: identical pass/fail set toorigin/main, no new failures.pkg/dbconnfully green.origin/maintree — local compose configuration, not this branch. Onepkg/statusflake appeared once and passes 5/5 in isolation.Related
Same retirement in the other two consumers: block/vitess#23, and strata's to follow. The driver side is block/mysql#4 and #5.
🤖 Generated with Claude Code