fix(start-os): reflect the upstream's ALPN across an add_ssl rewrap - #3777
fix(start-os): reflect the upstream's ALPN across an add_ssl rewrap#3777helix-nine wants to merge 19 commits into
Conversation
`ProxyTarget::preprocess` read the backend's negotiated ALPN from `TlsConnector::connect_with`'s closure, which rustls runs before any handshake byte moves — so it read `None` on every connection and the client-facing config was left with an empty ALPN list. The client therefore settled on no protocol while the backend, offered the client's own list, was free to select `h2`; `run_http_proxy` keys both legs off the client's negotiation, so it wrote HTTP/1 framing onto a connection the backend was reading as HTTP/2. Read the protocol off the connected stream instead, which the client's handshake still follows. Closes #3739
rustls selects the client-facing protocol by the server list's order, so a base config that already carried a protocol the client offered would be chosen over the one the backend actually selected — reinstating exactly the mismatch this fixes. No config on this path carries one today, so assigning changes nothing now and makes the invariant local rather than inherited. Also correct the packaging guide: `addSsl.alpn` takes an `AlpnInfo`, not a string, and setting it dials the container over plain TCP — which the new paragraph on rewrap ALPN would otherwise send a reader straight into.
…he rewrap rustls picks the client-facing protocol by the server list's order, so an entry inherited from the base config outranks whatever the branch chose. The rewrap branch guarded against that; `Reflect` and `Specified` still appended, and between them they cover more bindings than the rewrap does — every `http` and `ws` binding resolves to `Specified`. All three now assign, so the invariant holds wherever the list is set. No config on this path carries an entry today, so this is a guard rather than a behavior change. Name the container address when the upstream handshake fails: a bare `log_err` reported `received fatal alert: NoApplicationProtocol` with no address, service or SNI, while the client saw an `unrecognized_name` alert — a mismatch whose whole diagnostic trail pointed at certificates. Tests: cover the refusal the packaging guide describes, drop an assertion that compared the two ends of one TLS session against each other, and stop writing an HTTP/1 request into the HTTP/2 leg, where it was an invalid connection preface that the test survived only by reading bytes the upstream handshake had already buffered.
…th plaintext arms `TlsConnector::with_alpn` threads the protocol list through the handshake, so the rewrap no longer deep-clones the whole `ClientConfig` and builds a fresh `Arc` for every accepted connection. `Reflect` and `Specified` had no test at all, though `Reflect` is the default and takes every plaintext container behind `add_ssl`. Both are covered now. Fix a test helper that primed the upstream leg whenever the ALPN was absent, when what decides the framing is whether it is `h2`: `upstream_framing` with an explicit `http/1.1` proxied as HTTP/1, wrote nothing, and failed on a timeout blaming the proxy. That case is the one the changelog is about, so it is now asserted. Rename the refused-upstream test for the guarantee it pins — the client is declined rather than served over the plaintext stream — since the alert it observes is the listener's generic decline and says nothing about ALPN. Scope the changelog to the clients that were actually affected: a client speaking only HTTP/1.1 negotiated HTTP/1.1 on both legs and was served normally. Say what makes the extended-CONNECT warning true — StartOS advertises it whether or not the container implements it — rather than leaving a reader to refute it against RFC 8441, which makes it opt-in. Drop the `addSsl.alpn` paragraph: it describes released behavior, so it ships on live-docs (#3778) with the corrected type for that option.
Both arm tests passed an empty base config, where appending to the list and replacing it produce the same result — so they passed against the code they were written to lock in. Each now starts from a base carrying the protocol the other side prefers, which wins on server order under an append and loses under a replace. Say what the failure depended on in the changelog: a connection was lost when the service chose HTTP/2 for it, not whenever a client asked for HTTP/2. A service listing HTTP/1.1 first served a browser normally, and the previous wording also claimed an HTTP/1.1-only client was always served, which is not true of a service that speaks only HTTP/2 — the case this branch declines and tests. Drop "serves its own TLS" from the heading too; the packaging guide uses that phrase for passthrough, which this does not touch. Restore the `addSsl.alpn` warning. Removing it left the section urging authors to think about which protocols to advertise directly above an option that looks like where to say so, and setting it drops the binding to a plaintext dial into a TLS listener.
…d as one `with_alpn` falls back to the dialling config's own protocol list when it is handed `None`, so offering it only for a non-empty list — which reads like an obvious simplification — would put that config's protocols to the backend on behalf of a client that asked for none. The backend would then choose one the client never offered, and every ALPN-less client would stop being able to open the binding. Nothing pinned it; the config the rewrap dials with carries no protocols today, so the fallback is silent. Also drop a claim that ALPN selection consults the client's preference, which it does not: rustls takes the first of the server's list that the client also offered, as the sibling test's own comment says. Say what the module covers now that it holds plaintext-strategy tests too, and stop promising a TLS alert in a doc whose test only observes that the handshake failed. Drop the changelog's "where it chose HTTP/1.1" clause: it paired with the failure case as though the two were exhaustive, when a service that advertises nothing is the common case and a service sharing no protocol with the client is refused outright, before and after.
…e one that prevents it `with_alpn` takes a plain list and always hands `connect_impl` a `Some`, so it is the one call that cannot fall back to the dialling config. The fallback is `connect_impl`'s own, reached by not going through `with_alpn` at all — which is exactly the refactor the test exists to catch, so the comment naming `with_alpn` as the culprit invited a reader to dismiss it as stale. Drop that test's `assert_eq!`: a client offering no ALPN gets no extension back whatever the config holds, so the comparison could not fail and only the `expect` was ever doing the work.
`connect_ssl` carried two unrelated decisions in one `Result`: `Ok` meant "dial over TLS" and also "carry the ALPN the container negotiated", while `Err` meant "dial plaintext" and also "apply this strategy". Setting `alpn` could only be expressed as `Err`, so it silently moved the binding onto the plaintext dial — a container serving its own TLS was handed cleartext on a port expecting TLS, and `upstreamCertValidation` was dropped with it. Split them: `connect_ssl` is now `Option<Arc<ClientConfig>>` and says only how to dial, derived from `secure.ssl` alone; `alpn` says only what the client is offered, and defaults to carrying the container's choice over a TLS leg or reflecting the client's list without one. Every existing shape resolves to what it did before — only `alpn` on a TLS-serving container changes, and that one could not work. `setting_alpn_does_not_stop_the_container_being_dialled_over_tls` observes the container's own handshake completing, since the client-facing list alone cannot tell the two dials apart.
The table gives the option as `string` with `'h2'` as the example. It takes an
`AlpnInfo` — `'reflect'` or `{ specified: [...] }` — so the documented value is
not one the field accepts.
The option's effect on how StartOS dials the container is a bug rather than
something to document; it is fixed in #3777.
…e client Splitting the dial from the ALPN made `connect_ssl: Some` with `alpn: Some` expressible for the first time, and in that state the two legs negotiated separately: the container was dialled with the client's list and picked out of that, while the client was handed the pinned list. One protocol frames both legs, so a container that chose `h2` was then sent HTTP/1 requests — the mismatch this branch exists to remove, reachable again through `alpn`. The pin now narrows what the container is put through, and a dialled container's own choice is what the client is offered. Every other shape is unchanged: without a TLS leg the pin is still what the client sees, and an unset `alpn` still puts the client's list forward. `a_pinned_protocol_is_what_the_container_is_offered` reports the protocol the container settled on, which is the only thing that separates the two designs — the client is told `http/1.1` either way. Also guard the `alpn` arm of `ProxyTarget`'s equality, which decides whether a binding that changes only its protocols re-registers, and retire the "dials plaintext" vocabulary the split made false.
…ent named it Pinning `alpn` put the binding's whole list to the container regardless of what the client asked for, and the container chooses by its own preference — so it could settle on a protocol the client never named. The client was then offered that one protocol alone, and rustls refused any client that could not speak it. A client naming one of the pinned protocols was turned away. The same gap reopened the mismatch this branch exists to close: a client that names no protocol at all still had the pin put to the container, so the container framed the connection `h2` while the client stayed on HTTP/1.1 — rustls skips ALPN selection entirely when the ClientHello carries no extension, so nothing on the client leg could correct it. The container is now put through the pin intersected with the client's list. Where the two share nothing the client is refused as it is without a TLS leg, and where the client names nothing the container is named nothing either. Also correct the packaging guide: a container that simply ignores ALPN leaves the client without a protocol rather than refusing it, so the setting is inert there, and a rewrap now hands the client whatever the container chose — which is no longer a reason to reach for passthrough.
…t it Deleting the arm that refuses a client sharing no protocol with the pin left every test passing. Without it such a client is served with no protocol at all, which makes `Specified` advisory rather than binding — the difference the arm exists to draw, and the one thing about a pin a package author relies on.
…ransport The derivation that sent plaintext to a container serving its own TLS had no test: every ALPN test hand-builds a `ProxyTarget`, so restoring `alpn`'s old precedence over `secure.ssl` broke nothing. Extract the predicate and table-test it across the bindings that reach it. Also correct the prose the intersection made wrong. `addSsl.alpn` is documented for the rewrap alone, but every `http` and `ws` binding sets it with no container TLS leg at all, where the list is simply what the client is offered.
Every assertion here reads the client-facing handshake, so `preprocess` could hand back the socket under the container's TLS session and nothing noticed — which is the shape of the two faults this branch already fixed. Write a probe through the returned stream and have the container read it back through its own session. Cover the refusal a plaintext binding owes a client that shares no protocol with its pin. `addSsl.alpn` without a container TLS leg is the setting's plainest deployment, and only the rewrap half was guarded.
`BindOptions` already answers `serves_own_tls`, so the complementary question belongs beside it rather than in the controller, where it also had to test `add_ssl` twice to reach the options it wanted. `AddSslOptions::alpn` is an exported surface whose meaning this branch changes, so document it there — the explanation reached the book and not the editor. Correct the guidance the change makes wrong. Where StartOS terminates alone the pinned list is the only thing holding a client to what the container speaks, so "leave it unset" is advice for the rewrap only, and a client refused for sharing no protocol is refused by the container, whose alert names the hostname.
Compressing the refusal comment left it claiming the socket was never dialled, when only the TLS leg is skipped, and dropped the reason its guard tests both lists for emptiness — rustls turns a client away only when both name something, so the comment now argued for removing the two clauses that keep an ALPN-less client off an unwrapped socket. Taking the test backend's report slot after its handshake rather than on accept kept the sender alive in a loop that never ends, so a container that fails its handshake no longer closed the channel and the test waited out ten seconds to blame a slow handshake instead. It bought nothing: each test opens one connection. The `addSsl.alpn` doc said the container is offered the pinned protocols, which is what this branch stopped doing — it is offered the ones the client also asked for. A pin that names nothing turns nobody away, so say that where the book promises a refusal.
The field's doc walked a reader through both arrangements before telling them what to set. Say what it is, and let the variants carry what `reflect` means.
It filters the client's list down to what the binding puts forward, so say that instead of walking through where the result is sent.
The phrase asked a reader to track a list being handed somewhere. Say what survives the filter and who gets it, and let the two arrangements be two sentences rather than one.
| * Filters the application protocols the client asks for. Unset filters | ||
| * none of them. | ||
| */ | ||
| alpn: AlpnInfo | null |
There was a problem hiding this comment.
if null == "reflect" we shouldn't have both
There was a problem hiding this comment.
Agreed — and they're only the same because of this PR. Before it, 'reflect' also moved the container onto a plaintext dial, so the two values did differ; the fix is what made one of them redundant.
Dropped reflect in 56c5fc2, keeping the value that needs no value:
export type AlpnInfo = { specified: Array<MaybeUtf8String> }so the option is a filter or nothing. null is the only way to say "filter nothing", and AlpnInfo::default() — which returned Reflect and had no callers on master either — goes with it.
Two things worth your eye:
It is a manifest-visible break. Nothing in-tree sets reflect — knownProtocols uses { specified: ['http/1.1'] } or null, the embassy shim uses { specified: [...] }, and the OS's own bindings pin a list — but a package that hand-wrote 'reflect' would now fail to deserialize its binding. Anything that did was on the plaintext-dial path this PR changes anyway. Logged it in the SDK changelog under 2.0.10 with the one-line migration.
The type row in the book needs your call on ordering. #3778 fixes interfaces.md:177 on live-docs, where 'reflect' is still a real value of the released software, and docs-backport.yml then pushes that commit to master — where, after this lands, the value will not exist. I have left row 177 alone here rather than write a second version of the same line and hand you a backport conflict. Either #3778 merges first and a follow-up narrows the row on master, or #3778 takes the narrowed type now and the published book is briefly ahead of the published OS. I would rather you pick than guess.
There was a problem hiding this comment.
search all packages in Start9Labs ans Start9Community org. is anyone using this field?
There was a problem hiding this comment.
Searched both orgs. Nobody sets it to anything but null, and nothing anywhere uses 'reflect'.
Method: enumerated every non-archived repo in Start9Labs and Start9-Community via the API — 204 repos, 144 of them *-startos — pulled each one's default-branch tarball and grepped the whole tree. Zero fetch failures. (Tarballs rather than gh search code, whose index has silently covered only part of the fleet before.)
Eight package repos mention alpn in startos/, and every one of them writes null:
| repo | |
|---|---|
Start9-Community/electrs-startos |
startos/interfaces.ts:14 alpn: null |
Start9-Community/frigate-startos |
startos/interfaces.ts:14 alpn: null |
Start9Labs/coturn-startos |
startos/interfaces.ts:33, :83 alpn: null |
Start9Labs/fulcrum-startos |
startos/interfaces.ts:14 alpn: null |
Start9Labs/lnd-startos |
startos/interfaces.ts:58 alpn: null |
Start9Labs/owntracks-recorder-startos |
startos/interfaces.ts:47 alpn: null |
Start9Labs/public-pool-startos |
startos/interfaces.ts:38 alpn: null |
All eight are hand-rolled addSsl blocks on a protocol: null binding, where the field has to be written out. The other 136 packages never name it — they go through protocol: 'http'/'https' and let knownProtocols fill it in.
'reflect': zero hits in either org. The only occurrence of the string in 204 repos is this repo's own Rust.{ specified: [...] }: zero hits in any package. The only in-tree writers areknownProtocols(http/ws), the embassy shim, and the OS's own admin binding.- Two false positives worth naming so the numbers reconcile:
public-pool-web-startosmatched a base64 PNG inside an SVG, and lnd's twospecifiedhits are i18n prose. The remaining eleven hits are vendored upstream TLS code (arti,async-acme,openwrt,DC-linux,fedimint,chama,start-wrt). - No open PR in either org touches the field except fix(start-os): reflect the upstream's ALPN across an add_ssl rewrap #3777 and docs(start-sdk): correct the addSsl.alpn type #3778.
So the removal breaks nothing we own. It also confirms the collapse went the right way round: those eight explicit alpn: null sites are exactly what would have broken had we kept 'reflect' and dropped the nullability instead.
Caveat on scope: this is default branches of the two orgs. A package in a contributor's personal repo that hand-wrote 'reflect' would be outside it — though on the released OS that value puts a TLS-serving container on a plaintext dial, so any binding relying on it is already broken in the way #3739 describes.
Once setting `alpn` stopped deciding how the container is dialled, `reflect` and leaving the option unset said the same thing. Keep the one that needs no value, so the type offers a filter or nothing.
What was wrong
ProxyTarget::preprocessread the backend's negotiated ALPN from the closure itpassed to
TlsConnector::connect_with. rustls runs that closure as a configurationhook —
connect_implbuilds theClientConnection, calls the closure, and only thenreturns the future — so it ran before a single handshake byte moved,
conn.alpn_protocol()was unconditionallyNone, and the client-facingServerConfigwas left with an empty ALPN list on every connection.git logdates the closure to9ff0128fb(2023-07-14). It has been inert since theday it landed; there is no tokio-rustls version under which it worked.
What that cost
The empty list is only half of it. The upstream leg is offered the client's own
ALPN list, so a backend that speaks HTTP/2 selects
h2— while the client, offerednothing, settles on HTTP/1.1.
run_http_proxykeys both legs off the client'snegotiation, so StartOS then wrote HTTP/1 requests onto a connection the backend was
reading as HTTP/2.
The issue flagged that combination as the part not yet reproduced. It is real, and
both halves are pinned by tests that fail on
master:net::vhost::upstream_alpn_tests::the_client_lands_on_the_protocol_the_backend_chosedrives a real ClientHello through
TlsListenerinto the realpreprocess, in frontof a TLS backend advertising
[h2, http/1.1]. Onmasterit failsleft: None, right: Some("h2")— the backend pickedh2, the client got nothing.net::http::tests::the_negotiated_alpn_frames_the_upstream_legshows what thatNonethen writes upstream:GET / HTTP/1.1, whereSome("h2")writesPRI * HTTP/2.0.The fix
connectinstead ofconnect_with, and readalpn_protocol()off the connectedstream. The client handshakes on this config only after
preprocessreturns, so thebackend's choice is still in time to reach it — and because the backend was offered
the client's own list, whatever it chose is something the client asked for. No extra
round trip: that handshake was already awaited.
Every branch now assigns
prev.alpn_protocolsrather than appending. rustls picksby the server list's order, so an entry inherited from the base config would outrank
whatever the branch chose.
ReflectandSpecifiedneed that guard as much as therewrap does — more, in fact, since every
httpandwsbinding resolves toSpecified. No config on this path carries an entry today, so those two commits areguards, not behavior changes;
f695cc2f3is the only one that alters what goesover the wire.
a_protocol_on_the_base_config_does_not_outrank_the_backendpins itand fails if the assignment goes back to
extend.A failed upstream handshake now names the container address. It was logged bare, so an
ALPN mismatch printed
received fatal alert: NoApplicationProtocolwith no address,service or SNI, while the client got an
unrecognized_namealert — a failure whoseentire diagnostic trail pointed at certificates.
No configuration that worked before stops working
The upstream leg's ALPN offer is unchanged by this diff, so the backend selects the
same protocol either way. That leaves two cases:
h2— previously the proxy wrote HTTP/1 framing onto it, so thebinding was already broken for all traffic. Now both legs frame
h2.http/1.1or nothing — the client-facing handshake lands on thesame protocol it did before. No change on the wire.
Two gaps this makes reachable, neither introduced here
run_http2_proxywas previously all but unreachable forhttps/wssbindings and nowbecomes their primary path. Review turned up two pre-existing defects in it, filed
separately rather than folded in here:
unconditionally, without checking
is_extended_connect_protocol_enabled()on thebackend, and its upstream-error path returns before
graceful_shutdown(), so thebrowser gets no GOAWAY.
tls.rs'sget_confighas no timeout despite a commentclaiming one, and
preprocessdoes an unbounded TCP connect plus TLS handshakeinside it.
Worth a look before merging, since they decide whether WebSockets survive on a binding
whose container advertises
h2.Docs
projects/start-sdk/docs/src/interfaces.mdtold package authors that a rewrapnegotiates no ALPN and pointed them at passthrough for that reason. It now describes
what the rewrap carries, and warns that advertising
h2commits the container toserving WebSockets over RFC 8441 extended CONNECT — which is what #3775 is about, and
the reason a container that only speaks HTTP/1.1 should say so.
The section keeps a warning that
addSsl.alpnis not where you configure this —setting it drops the binding to a plaintext dial into a TLS listener — because the new
prose above it urges authors to think about which protocols to advertise, and that
option looks like where to say so.
The option's type, and the rewrap section's claim that
addSsl+secure.sslisthe whole condition, are corrections to already-published text and go to
live-docs(#3778) instead, which deploys on merge and is backported to master unattended. They
touch different lines than the warning above, so there is no double-edit to conflict.
addSsl.alpnno longer decides how the container is dialledRaised by @dr-bonez on #3778, where this was originally being written down as a
documentation gap.
connect_sslcarried two unrelated decisions in oneResult:Okmeant dial overTLS and also carry the ALPN the container negotiated;
Errmeant dial plaintextand also apply this strategy. Setting
alpncould only be expressed asErr, so itsilently moved the binding onto the plaintext dial — a container serving its own TLS
got cleartext on a port expecting TLS, and
upstreamCertValidationwas discarded withit.
They are now separate fields:
connect_ssl: Option<Arc<ClientConfig>>says only how todial and is derived from
secure.sslalone, andalpnsays only what the client isoffered, defaulting to the container's choice over a TLS leg or the client's own list
without one. Every existing binding shape resolves to what it did before; the only
behaviour that changes is
alpnon a TLS-serving container, which could not work.setting_alpn_does_not_stop_the_container_being_dialled_over_tlswatches thecontainer's own handshake complete — the client-facing list alone cannot tell a TLS
dial from a plaintext one, so a test that only checked it passed either way.
addSsl.alpnnarrows the protocols the binding puts forward: a dialled containerchooses out of that narrower list, and the client is offered its choice, so one
protocol still frames both legs. Offering the container the client's list while telling
the client the pinned one would let them settle on different protocols — the mismatch
this PR exists to remove — and
a_pinned_protocol_is_what_the_container_is_offeredreports what the container settledon, which is the only thing that separates the two designs.
Follow-ups
Two things review surfaced that are deliberately not here:
AddSslOptions::alpn(net/host/binding.rs) is a bare field while its siblings carry///docs that ts-rs emits into the published binding. That is where an author meetsthis option in their editor.
error!, and this PR documents that refusal asexpected. It is client-triggerable, so a junk ALPN produces one error line per
connection. Pre-existing;
warn!would fit better.interfaces.md's options table still typesaddSsl.alpnasstring. docs(start-sdk): correct the addSsl.alpn type #3778 owns thatrow, against
live-docs; it is left alone here so the two do not collide.Closes #3739