MF-M05: fix(nitronode): enforce TLS by default for Postgres#733
Conversation
Replace the hardcoded `sslmode=disable` in postgresqlDbUrl with a configurable NITRONODE_DATABASE_SSLMODE (default `require`), validate the mode against the libpq list, and honor NITRONODE_DATABASE_URL verbatim when set so operators can supply a fully-qualified DSN. Wire the helm chart to propagate `config.database.sslmode` through NITRONODE_DATABASE_SSLMODE; chart default stays `disable` to avoid breaking existing deployments — operators must opt-in to TLS via values. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
The Go-layer fix is solid: the SSLMode field, URL-override path, validation map, and tests are all correct. One gap remains on the chart side — the template default in _common.tpl still ships disable, which means TLS is opt-in rather than opt-out for any deployment that doesn't explicitly set the value. Everything else looks good.
Flip the helm chart's `config.database.sslmode` default from `disable` to `require` so deployments that expose Postgres over an untrusted network get TLS without extra configuration, matching the binary default. Setups where Postgres is only reachable on a private network (e.g. an in-cluster pgbouncer or VPC-only Cloud SQL) can opt out by setting `sslmode: disable`. The stress-v1 profile does so explicitly, since its pgbouncer is cluster-internal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
approved for m-05 closure. the postgres path now defaults to tls in both the binary and helm, with explicit opt-out documented for private-network setups.
- MF-L01: fix(contracts/ChannelHub): cap ERC20 transfer returndata copy to 32 bytes (#726) - MF-H01: fix(nitronode): paginate get_last_key_states endpoints (#724) - MF-I01-I02: fix(contracts): address security audit findings I-01 and I-02 (#728) - MF-C01: rpc: cap inbound WebSocket frame size and rate-limit per connection (#723) - MF-L02: docs(protocol): qualify enforcement guarantee for intent-specific execution paths (#737) - MF-L02-I03-I04_I05: fix(contracts): add more Node trust assumptions and requirements (#738) - MF-M01: backfill state user_sig from on-chain events (#731) - MF-M02: fix(rpc): release Serve wait group on processSink overflow (#732) - Fix SDK acknowledgement before home channel creation (#734) - MF-I06: fix(nitronode): gate escrow transitions on home channel onchain materialization (#730) - MF-M05: fix(nitronode): enforce TLS by default for Postgres (#733) - MF-M07: Unblock receiver states after finalized escrow operations (#735) - MF-M04: feat: provide tooling for and enhance docs on ValidatorRegistered event (#744) - MF-L04: fix(contracts): reject redundant native value (#741) - MF-H02: bind session key registration to a single owner per kind (#739) - MF-I07: fix(contracts): enforce max challenge duration (#752) - MF-M08: fix(rpc): replace Origin label with application_id on connection gauge (#745) - MF-C02: fix(core): add ChannelStatusClosing to gate post-finalize state transitions (#746) - MF-L06: fix(contracts): clear stale challengeExpireAt on cooperative escrow finalization (#754) - MF-I08: docs: document ChannelClosed event orientation ambiguity during abandoned migration (#755) - MF-M09: fix(nitronode): auto-challenge home channel on withheld escrow finalize (#753) - MF-L09: fix(nitronode): validate parsed app session nonce (#751) - MF-L05: docs(contracts): document informational events not guaranteed to emit (#756) - MF-L08: fix(nitronode/api): default get_last_key_states to active-only with include_inactive opt-in (#749) - MF-L10: fix: emit escrowIds array in EscrowDepositsPurged event and handle it in Nitronode (#757)
3 participants
Summary
sslmode=disableinpostgresqlDbUrlwith configurableNITRONODE_DATABASE_SSLMODE(binary default:require, validated against the libpq list).NITRONODE_DATABASE_URLverbatim when set so operators can supply a fully-qualified DSN; individual host/user/password/sslmode fields are ignored in that case.config.database.sslmodeasNITRONODE_DATABASE_SSLMODE. Chart default isrequireso helm and the binary agree; deployments where Postgres is only reachable on a private network (e.g. cluster-internal pgbouncer, VPC-only Cloud SQL) can opt out by settingsslmode: disable. The stress-v1 profile does so explicitly.Why
Audit finding MF-M05: every Postgres connection (schema create, migrations, runtime) used
sslmode=disable, so credentials and protocol traffic could be observed or tampered with by anyone on the path. The chart documented ansslmodevalue but never propagated it.Notes
require(TLS without cert verification). Operators wanting strict cert checking should setverify-full.sslmode=disableintentionally.NITRONODE_DATABASE_SSLMODE=disableexplicitly. Helm users on a private-network Postgres must setconfig.database.sslmode: disableto keep the previous behavior.Test plan
go test ./nitronode/store/database/ -run TestPostgresqlDbUrl— covers default, explicit, invalid, schema append, URL precedence, unsupported drivergo vet ./nitronode/store/database/cleango build ./nitronode/...cleansslmodeset / unset (default →require, override →disable)🤖 Generated with Claude Code