Skip to content

fix(start-os): reflect the upstream's ALPN across an add_ssl rewrap - #3777

Open
helix-nine wants to merge 19 commits into
masterfrom
fix/vhost-upstream-alpn-reflection
Open

fix(start-os): reflect the upstream's ALPN across an add_ssl rewrap#3777
helix-nine wants to merge 19 commits into
masterfrom
fix/vhost-upstream-alpn-reflection

Conversation

@helix-nine

@helix-nine helix-nine commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

What was wrong

ProxyTarget::preprocess read the backend's negotiated ALPN from the closure it
passed to TlsConnector::connect_with. rustls runs that closure as a configuration
hook — connect_impl builds the ClientConnection, calls the closure, and only then
returns the future — so it ran before a single handshake byte moved,
conn.alpn_protocol() was unconditionally None, and the client-facing
ServerConfig was left with an empty ALPN list on every connection.

git log dates the closure to 9ff0128fb (2023-07-14). It has been inert since the
day 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, offered
nothing, settles on HTTP/1.1. run_http_proxy keys both legs off the client's
negotiation, 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_chose
    drives a real ClientHello through TlsListener into the real preprocess, in front
    of a TLS backend advertising [h2, http/1.1]. On master it fails
    left: None, right: Some("h2") — the backend picked h2, the client got nothing.
  • net::http::tests::the_negotiated_alpn_frames_the_upstream_leg shows what that
    None then writes upstream: GET / HTTP/1.1, where Some("h2") writes
    PRI * HTTP/2.0.

The fix

connect instead of connect_with, and read alpn_protocol() off the connected
stream. The client handshakes on this config only after preprocess returns, so the
backend'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_protocols rather than appending. rustls picks
by the server list's order, so an entry inherited from the base config would outrank
whatever the branch chose. Reflect and Specified need that guard as much as the
rewrap does — more, in fact, since every http and ws binding resolves to
Specified. No config on this path carries an entry today, so those two commits are
guards, not behavior changes; f695cc2f3 is the only one that alters what goes
over the wire. a_protocol_on_the_base_config_does_not_outrank_the_backend pins it
and 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: NoApplicationProtocol with no address,
service or SNI, while the client got an unrecognized_name alert — a failure whose
entire 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:

  • Backend selects h2 — previously the proxy wrote HTTP/1 framing onto it, so the
    binding was already broken for all traffic. Now both legs frame h2.
  • Backend selects http/1.1 or nothing — the client-facing handshake lands on the
    same protocol it did before. No change on the wire.

Two gaps this makes reachable, neither introduced here

run_http2_proxy was previously all but unreachable for https/wss bindings and now
becomes their primary path. Review turned up two pre-existing defects in it, filed
separately rather than folded in here:

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.md told package authors that a rewrap
negotiates no ALPN and pointed them at passthrough for that reason. It now describes
what the rewrap carries, and warns that advertising h2 commits the container to
serving 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.alpn is 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.ssl is
the 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.alpn no longer decides how the container is dialled

Raised by @dr-bonez on #3778, where this was originally being written down as a
documentation gap.

connect_ssl carried two unrelated decisions in one Result: Ok meant dial over
TLS
and also carry the ALPN the container negotiated; 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
got cleartext on a port expecting TLS, and upstreamCertValidation was discarded with
it.

They are now separate fields: connect_ssl: Option<Arc<ClientConfig>> says only how to
dial and is derived from secure.ssl alone, and alpn says only what the client is
offered, 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 alpn on a TLS-serving container, which could not work.

setting_alpn_does_not_stop_the_container_being_dialled_over_tls watches the
container'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.alpn narrows the protocols the binding puts forward: a dialled container
chooses 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_offered reports what the container settled
on, 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 meets
    this option in their editor.
  • An upstream ALPN refusal logs at error!, and this PR documents that refusal as
    expected. 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 types addSsl.alpn as string. docs(start-sdk): correct the addSsl.alpn type #3778 owns that
    row, against live-docs; it is left alone here so the two do not collide.

Closes #3739

`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.
helix-nine added a commit that referenced this pull request Aug 21, 2026
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.
Comment thread shared-libs/crates/start-core/src/net/host/binding.rs Outdated
Comment thread shared-libs/crates/start-core/src/net/vhost.rs
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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if null == "reflect" we shouldn't have both

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 reflectknownProtocols 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

search all packages in Start9Labs ans Start9Community org. is anyone using this field?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 are knownProtocols (http/ws), the embassy shim, and the OS's own admin binding.
  • Two false positives worth naming so the numbers reconcile: public-pool-web-startos matched a base64 PNG inside an SVG, and lnd's two specified hits 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

vhost: upstream ALPN reflection in ProxyTarget::preprocess is a no-op

2 participants