fix: an address family is a choice, not a setting, and a pinned CA is the CA - #47
Merged
Conversation
…s the CA
Two things 1.0 got wrong about the options a caller hands in.
`socket_options => [inet]` was refused. `inet` and `inet6` are not settings a
socket takes, they choose which address a name resolves to, so asking
`setopt/2` about them was the wrong question and the answer was
`{error, {unsupported_socket_option, inet}}` - after the TCP connect, which a
pool reports as `disconnected` rather than as a bad option. Every deployment
that names its address family, which is the documented way to configure one,
could not open a connection. The accept list was drawn from what a socket can
be set to instead of from what callers pass.
They are now read before connecting and honoured: `inet6` resolves AAAA or
fails, rather than falling back to v4 and leaving the option accepted and not
honoured.
`ssl_options => [{cacertfile, Path}]` did not replace the default
`{cacerts, public_key:cacerts_get()}`, because a default was only superseded by
an option spelled the same way. Both reached `m:ssl` and the public bundle won,
so a caller pinning a private CA silently got the wider trust set. Defaults are
now superseded by the option that answers the same question.
Both cases are covered by tests that fail without the fix.
🟡 Code Coverage — 88.8%1758 of 1980 lines covered. ✅ ELP LintNo diagnostics. |
A send with no deadline waits on a peer that has stopped reading for as long as that peer likes, holding the pool slot it borrowed. The driver has `send_timeout` for exactly that, and the socket transport had nowhere to put it, so the mitigation existed for the transport that is not the default. The deadline covers the whole write rather than each attempt, because a peer accepting one byte at a time would otherwise never reach it. It is read once when the connection opens rather than off the socket per write: a `getopt` per send is a NIF call per send, which is the cost this transport exists to avoid. `send_timeout_close` is honoured too. A write that timed out has left the server half a message, so the connection is finished either way, and a caller who asked for it to be closed should not have to notice that for themselves.
Common Test's default is thirty minutes, which is longer than CI gives the whole step, so a case that blocks arrives as a killed job with no case name and no stack. Every operation in this suite is bounded by a five second connect or a ten second read, so sixty seconds means stuck somewhere with no deadline of its own - and the timetrap's stack trace is the thing worth having.
Taure
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two defects in what 1.0 does with caller-supplied options. The first is a production outage; the second is a quiet weakening of a CA pin.
socket_options => [inet]was refused1.0 made the socket transport the default and made
minato_socket:setopts/2refuse options it cannot honour instead of silently dropping them. The policy is right. The accept list was not: I drew it from what a socket can be set to rather than from what callers actually pass, andinet/inet6are neither - they choose which address a hostname resolves to, andgen_tcpconsumes them before connecting.So
{socket_options, [inet]}answered{error, {socket, {unsupported_socket_option, inet}}}, after the TCP handshake. Through a pool that surfaces as{error, disconnected}with a pool that started cleanly, so a container boots, migrations fail, readiness never flips, and nothing crashloops.This is the documented way to configure a family. asobi's own configuration guide lists
inet,inet6andinet, {nodelay, true}as the supported values ofASOBI_DB_SOCKET_OPTS, and both asobi and asobi_engine bakeinetas their image default. Every one of those deployments would have failed to reach Postgres.The family is now read before connecting and used for resolution.
inet6means AAAA or an error - a fall back to v4 would be the same defect one layer down, the option accepted and not honoured.{cacertfile, Path}did not replace{cacerts, _}tls_options/2merged caller options over defaults by option name, so a caller pinning a private CA by file was handed the public bundle as well, and the wider set decided. It fails closed rather than open, but the shape is a pin bypass: pin a stricter private CA for a host whose certificate also chains to a public root and you silently get the wider trust set.Defaults are now superseded by the option that answers the same question, with
cacertfileandcacertsbeing one question.Tests
Both are covered by cases that fail without the fix - I checked by reverting each and watching them go red (
unknown_cafor the CA case). 36 in the transport suite across both transports, 12 in the TLS suite.Full pipeline green: 1398 eunit, 178 ct, xref, dialyzer, eqwalize-all, elp lint, ex_doc, fmt.
Provenance
Found by an architecture-guardian and a security review run against widgrensit/asobi_engine#103, the relock that would have carried 1.0.1 into deployed environments. That PR is a draft until this ships.
Marked
fix!because the family is now honoured rather than ignored: a caller who passedinet6to a dual-stack host was getting v4 and will now get v6.