Skip to content

Add the sush proxy to the switch zone - #11253

Draft
plotnick wants to merge 15 commits into
sush-sled-agentfrom
sush-switch-proxy
Draft

Add the sush proxy to the switch zone#11253
plotnick wants to merge 15 commits into
sush-sled-agentfrom
sush-switch-proxy

Conversation

@plotnick

@plotnick plotnick commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Continues the integration of the Support Shell (RFD 620), building on the sled-agent embedding: a new switch zone service, sush-proxy, terminates technician-port connections and routes each request to a sled's sush server. The proxy finds sush servers by probing the bootstrap and underlay prefixes given by DDM, and polls MGS for the cubby map so a request may name its target sled by cubby.

For security review:

On real hardware the proxy serves TLS backed by the sled's platform identity. At zone startup, sled-agent generates an ephemeral key and has the RoT sign its certificate with the TQ key (Ed25519 over the SHA3-256 digest of the TBS certificate), and the sush client verifies that the chain ends at a platform identity root. Simulated and emulated environments have no RoT, so their proxies serve plaintext. This is described in the transport security section of RFD 620.

This continues the integration of the
[Support Shell](https://github.com/oxidecomputer/sush)
([RFD 620](https://rfd.shared.oxide.computer/rfd/0620)), building on
the sled-agent embedding: a new switch zone service, `sush-proxy`,
terminates technician-port connections and routes each request to a
sled's sush server. The proxy finds sush servers by probing the bootstrap
and underlay prefixes given by DDM, and polls MGS for the cubby map
so a request may name its target sled by cubby.

**For security review:**

On real hardware the proxy serves TLS backed by the sled's platform
identity. At zone startup, sled-agent generates an ephemeral key and
has the RoT sign its certificate with the TQ key (Ed25519 over the
SHA3-256 digest of the TBS certificate), and the sush client verifies
that the chain ends at a platform identity root. Simulated and emulated
environments have no RoT, so their proxies serve plaintext.

Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
@plotnick

plotnick commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

A heads-up: @rmustacc identified a very serious problem in the security story here, which is that we are forbidden by policy (which we're currently ignoring) from signing certificates with the platform id leaf certificate. I will be sketching a work-around tomorrow (Wed, Sep 9), and it will have some knock-on effects (mostly positive, I think).

@plotnick

Copy link
Copy Markdown
Contributor Author

Heads-up follow-up: I did not get to implement the previously mentioned work-around today (Wed), but I did document it in the transport security section of RFD 620. Feedback would be welcome, and I'll try again to prototype it tomorrow (Thu) based on the sketch there.

…h-proxy

Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>

# Conflicts:
#	workspace-hack/Cargo.toml
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>

# Conflicts:
#	workspace-hack/Cargo.toml
@plotnick

Copy link
Copy Markdown
Contributor Author

Follow-up follow-up: the "voucher" workaround is now implemented in oxidecomputer/sush#86. New commits will be pushed here for the Omicron side.

The proxy's ephemeral cert is now self-signed which carries the RoT's
voucher for its key (a Trust Quorum signature over the tagged SPKI
digest; see RFD 620 §4.6.2.1), rather than claiming tq-dhe as its issuer
(which violates critical constraints). The TLS mode is renamed `Platform`
→ `Vouched` to match.

Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>

@internet-diglett internet-diglett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good to me from the networking and switch zone perspective!

@jmpesp jmpesp left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I haven't had a chance to review sush_server::ProxyServer yet, but here's a first round of feedback

.add_property(
"address",
"astring",
&format!("[::]:{SUSH_PROXY_PORT}"),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do we want to restrict this at all?

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.

Assuming "this" is [::], no, because we need to listen on both the tech port and underlay addresses (for the Nexus tunnel) as they become available. Restriction here would mean plumbing addresses up dynamically for no real win that I can see.

Comment thread sled-agent/src/sush.rs Outdated
Comment on lines +109 to +110
const SUSH_PROXY_CERT_VALIDITY: Duration =
Duration::from_secs(365 * 24 * 60 * 60);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is regenerated every sled-agent restart?

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.

Every switch zone startup, which includes sled-agent restarts, yes.

Comment thread sush-proxy/src/lib.rs
}
}
}
targets.send_modify(|t| t.cubbies.extend(cubbies));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The same comment from the previous PR applies here, w.r.t clobbering the map.

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.

This map has different semantics from the one in '52. There, each round is one complete response from MGS; here, each round polls every SP separately, and we don't want to drop a cubby's entry just because it missed a round. Eventually we'll want to have some kind of stale-entry expiration, but a sled not in this map can't accept jobs targeted by cubby, so we'd like to keep the entries even for a flaky sled (or while it's rebooting, or whatever).

Comment thread sush-proxy/src/lib.rs
warn!(log, "unable to fetch underlay prefixes"; "error" => %err)
}
}
targets.send_modify(|t| t.sleds.extend(sleds));

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

and here

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.

Same situation as above, I think merging into the map is correct here.

Comment thread .cargo/xtask.toml Outdated
Comment thread sled-agent/src/services.rs Outdated
Comment thread sled-agent/src/sush.rs
Comment on lines +101 to +105
/// Path inside the switch zone to the sush proxy's TLS private key.
pub const SUSH_PROXY_KEY_PATH: &str = "/etc/sush-proxy/key.pem";

/// Path inside the switch zone to the sush proxy's TLS certificate chain.
pub const SUSH_PROXY_CERT_CHAIN_PATH: &str = "/etc/sush-proxy/chain.pem";

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.

what, if anything, can or should reasonably done to restrict the accessibility of these paths to to processes other than the sush proxy?

EDIT: oh, cool, I see that we have a write_private function which ensures the key has the correct permissions. maybe worth noting that in the comment?

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.

Sure, noted in b237150.

Comment thread sled-agent/src/sush.rs
Comment thread sled-agent/src/sush.rs Outdated
Comment thread sled-agent/src/sush.rs
.create(true)
.write(true)
.truncate(true)
.mode(0o600)

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.

is this sufficient to ensure that only sush-proxy can read the file? what user do other switch zone processes run as? and all SSH sessions presently log in as root, right?

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.

As far as I can tell, everything in the switch zone currently runs as root, yes. You are correct that this proxy could run as a dedicated non-privileged user, and probably drop other capabilities and permissions. I would be happy to take that as a follow-up if you think it's worth it (it may be, I'm not sure). But stealing this key would only let an attacker impersonate a proxy; it would not grant any access to run or examine jobs.

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.

Yeah, for what it's worth, I totally agree that not having everything run as root is very much out of scope for this PR. But, it feels like something we should be thinking about separately. My point was more that the fact that everything, including SSH sessions, runs as root means that restricting who can access the file is currently mostly ceremonial, since basically every process will be running as root anyway. I agree that it's better than not doing it though!

Comment thread sush-proxy/src/lib.rs Outdated
Comment on lines +42 to +44
/// A vouched proxy identity: an ephemeral key that sled-agent
/// generates and the RoT vouches for, served from local files
/// along with the platform identity chain that validates it.

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.

would like this to be declauded

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.

Thanks, rewritten in 71a9dc8.

Comment thread sush-proxy/src/lib.rs Outdated
Comment thread sush-proxy/src/lib.rs Outdated
Comment thread sush-proxy/src/lib.rs
Comment on lines +82 to +83
tokio::join!(sleds(log, ddm, &tx_targets), cubbies(log, mgs, &tx_targets),);
unreachable!("discovery loops never return");

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 they never return, why bother joining them? perhaps they should be spawned instead?

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.

(what i am actually saying here is in part that i am a bit concerned about these running in the same task rather than in true parallelism, especially since they both contend the watch channel sender which is morally equivalent to a mutex)

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.

in fact, upon thinking through this further, i don't really get why Targets must be a single watch channel, given that there are two distinct senders which both only touch half of it. It seems like Targets should really be internally a watch channel of cubbies and separately a watch channel of sleds, and then consumers would select either of them having changed

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.

Ok, in order: we join so that panics propagate, kill the proxy, and SMF will restart it. The alternative is to continue serving with stale routes, which could wedge it closed.

On contention: yes, it's morally a mutex, but the write locks are once per 30-second poll round, and the reads are one borrow per request, which consults both maps. I don't think splitting it would really improve much, though we certainly can if you feel strongly about it.

On concurrency: there's no real work happening in either loop, they're just waiting for timeouts and I/O.

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.

Ok, in order: we join so that panics propagate, kill the proxy, and SMF will restart it. The alternative is to continue serving with stale routes, which could wedge it closed.

Are we building with panic = "abort"? If so, this shouldn't be necessary.

On contention: yes, it's morally a mutex, but the write locks are once per 30-second poll round, and the reads are one borrow per request, which consults both maps. I don't think splitting it would really improve much, though we certainly can if you feel strongly about it.

On concurrency: there's no real work happening in either loop, they're just waiting for timeouts and I/O.

My concern here is just that having two separate concurrent futures in the same task which may both try to write lock the same mutex feels sketchy. I agree that there is no risk of deadlock in the current code, but if some future thing comes along and makes one of the poll loops also try to read from the watch channel, it could become a problem. I don't feel that strongly about it, it's just mildly concerning for the future.

Comment thread sush-proxy/src/lib.rs Outdated
Comment thread sush-proxy/src/lib.rs Outdated
Comment thread sush-proxy/src/lib.rs Outdated
Comment thread sush-proxy/src/main.rs Outdated
plotnick and others added 5 commits September 12, 2026 08:01
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-authored-by: Eliza Weisman <eliza@elizas.website>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
plotnick and others added 4 commits September 12, 2026 08:43
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
Co-Authored-By: Claude Mythos 5 <noreply@anthropic.com>
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.

4 participants