From bef7854b69e78e0604fa8fe682c80eecde129705 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 11:02:13 -0700 Subject: [PATCH 1/6] Print hashes in hex in debug output Also build Codephrase's debug output with debug_tuple, so both nest properly in pretty-printed output. Co-authored-by: Claude --- common/src/codephrases.rs | 4 +++- common/src/hash.rs | 10 +++++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/common/src/codephrases.rs b/common/src/codephrases.rs index 3fa05929..6254e06e 100644 --- a/common/src/codephrases.rs +++ b/common/src/codephrases.rs @@ -125,7 +125,9 @@ impl fmt::Display for Codephrase { impl fmt::Debug for Codephrase { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "Codephrase({self})") + f.debug_tuple("Codephrase") + .field(&format_args!("{self}")) + .finish() } } diff --git a/common/src/hash.rs b/common/src/hash.rs index 203160a0..c7b502c2 100644 --- a/common/src/hash.rs +++ b/common/src/hash.rs @@ -16,7 +16,7 @@ use crate::wire::ExactBytes; pub const OUT_LEN: usize = 32; /// A SHA3-256 digest. -#[derive(Clone, Copy, Debug, Eq, PartialEq)] +#[derive(Clone, Copy, Eq, PartialEq)] pub struct Hash([u8; OUT_LEN]); impl Hash { @@ -43,6 +43,14 @@ impl fmt::Display for Hash { } } +impl fmt::Debug for Hash { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("Hash") + .field(&format_args!("{self}")) + .finish() + } +} + /// Hex for humans, raw bytes for binary formats. impl Serialize for Hash { fn serialize(&self, serializer: S) -> Result { From 2efbb8542a789665750207f10a9d147868f9f2fb Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 11:18:49 -0700 Subject: [PATCH 2/6] Trim trailing slashes from URL arguments A trailing slash on `--url` made every request path start with `//`, which the proxy doesn't match. Trim the permslip URL the same way. Co-authored-by: Claude --- client/src/commands.rs | 35 +++++++++++++++++++++++++++++++---- 1 file changed, 31 insertions(+), 4 deletions(-) diff --git a/client/src/commands.rs b/client/src/commands.rs index d5cc2dd5..e78fcbfb 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -7,6 +7,7 @@ //! May be executed via either the main CLI or the interactive REPL. use std::collections::BTreeSet; +use std::convert::Infallible; use std::env::args_os; use std::fs::{File, OpenOptions, read}; use std::io::ErrorKind; @@ -206,7 +207,7 @@ pub struct GlobalArgs { pub text: bool, /// Support Shell HTTP API address for online mode - #[arg(short, long, env = SUSH_URL)] + #[arg(short, long, env = SUSH_URL, value_parser = parse_url_arg)] #[clap(global = true)] pub url: Option, @@ -364,6 +365,12 @@ fn parallel_chunks(s: &str) -> Result { Ok(n) } +/// Parse a URL argument for clap, trimming whitespace and trailing +/// slashes. +fn parse_url_arg(s: &str) -> Result { + Ok(s.trim().trim_end_matches('/').to_string()) +} + /// Support Shell job management command #[derive(Clone, Debug, Default, Subcommand)] pub enum ClientCommand { @@ -499,7 +506,7 @@ pub enum SessionCommand { permslip_key: Option, /// The `permslip` server to contact for signing. - #[arg(long, env = PERMSLIP_URL, value_name = "URL")] + #[arg(long, env = PERMSLIP_URL, value_name = "URL", value_parser = parse_url_arg)] permslip_url: Option, }, @@ -533,7 +540,7 @@ pub enum SessionCommand { /// The `permslip` server to contact for signing. #[cfg(feature = "permslip")] - #[arg(long, env = PERMSLIP_URL, value_name = "URL")] + #[arg(long, env = PERMSLIP_URL, value_name = "URL", value_parser = parse_url_arg)] permslip_url: Option, /// Supersede an active session without asking. @@ -691,7 +698,7 @@ pub struct JobStartArgs { /// The `permslip` server to contact for signing. #[cfg(feature = "permslip")] - #[arg(long, env = PERMSLIP_URL, value_name = "URL")] + #[arg(long, env = PERMSLIP_URL, value_name = "URL", value_parser = parse_url_arg)] permslip_url: Option, /// Terminal type for interactive jobs. @@ -3090,6 +3097,26 @@ mod tests { } } + /// URL arguments lose surrounding whitespace and trailing slashes. + #[test] + fn url_args() { + let url = |url: &str| { + ClientArgs::try_parse_from(["sush", "--url", url]) + .ok() + .and_then(|args| args.globals.url) + }; + assert_eq!( + url("https://[fe80::1%3]:12352"), + Some("https://[fe80::1%3]:12352".into()) + ); + assert_eq!( + url(" https://[fe80::1%3]:12352// "), + Some("https://[fe80::1%3]:12352".into()) + ); + assert_eq!(url("/"), Some("".into())); + assert_eq!(url(" "), Some("".into())); + } + #[test] fn login_refused_blames_build_skew() { let refused = || CommandError::Client(String::from("Authentication required, try `iam`")); From 5afed38ec26a64aa0e95f462b9ac9c4446a89833 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 11:28:35 -0700 Subject: [PATCH 3/6] Warn, don't fail, when no SSH identities are found Co-authored-by: Claude --- client/src/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/src/cli.rs b/client/src/cli.rs index bdcac1cb..e6ff4ce6 100644 --- a/client/src/cli.rs +++ b/client/src/cli.rs @@ -1132,7 +1132,7 @@ impl CommandContext for Cli { .collect::, CommandError>>()?; qprintln!("{}", json!({"identities": identities})); } - OutputFormat::Text if keys.is_empty() => self.note("❌ No SSH identities found"), + OutputFormat::Text if keys.is_empty() => self.note("❗ No SSH identities found"), OutputFormat::Text => { for key in keys { let key_id = key.key_id()?; From aa2850f0b92f9adf1fe615c0dfb0a423a7a5869c Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 12:10:15 -0700 Subject: [PATCH 4/6] Show unverified output of failed jobs Co-authored-by: Claude --- client/src/commands.rs | 245 +++++++++++++++++++++++++++-------------- 1 file changed, 165 insertions(+), 80 deletions(-) diff --git a/client/src/commands.rs b/client/src/commands.rs index e78fcbfb..e083f915 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -10,8 +10,7 @@ use std::collections::BTreeSet; use std::convert::Infallible; use std::env::args_os; use std::fs::{File, OpenOptions, read}; -use std::io::ErrorKind; -use std::io::{Seek as _, SeekFrom, Write as _, stdin}; +use std::io::{self, Seek as _, SeekFrom, Write as _, stdin}; use std::net::SocketAddr; use std::num::{NonZeroU8, NonZeroU64}; use std::path::{Path, PathBuf}; @@ -52,8 +51,8 @@ use sush_common::interactive::{InteractiveJobError, InteractiveJobMessage}; use sush_common::jobs::JobOutputStream::{self, Stderr, Stdout}; use sush_common::jobs::{ Access, JobId, JobLimits, JobMode, JobOutputHash, JobOutputState, JobStartRequest, JobStatus, - JobStatusMap, Session, SessionClientNonce, SessionId, SessionSeq, SessionSignerNonce, - SessionSushNonce, SignedJob, SkipReason, job_status_try_from_json_map, + JobStatusMap, ProcessError, Session, SessionClientNonce, SessionId, SessionSeq, + SessionSignerNonce, SessionSushNonce, SignedJob, SkipReason, job_status_try_from_json_map, }; use sush_common::keys::{KeyError, KeyId, Signer as _}; use sush_common::targets::{SledId, Target}; @@ -1746,40 +1745,62 @@ async fn job_start( }; ctx.job_watch_finished(&job_id); ctx.job_status(&job_id, &status, StatusDisplayStyle::Short); - if !status - .values() - .any(|s| matches!(s, JobStatus::Stopped { .. })) - { + + // Show the job output from every sled it ran on. + let ran: Vec<(&BaseboardId, &JobStatus)> = status + .iter() + .filter(|(_, s)| matches!(s, JobStatus::Stopped { .. } | JobStatus::Error { .. })) + .collect(); + if ran.is_empty() { return Err(if status.values().all(JobStatus::is_terminal) { CommandError::JobDidNotRun(job_id) } else { CommandError::JobStillRunning(job_id) }); } - // Show the output of every sled the job ran on. - let multiple = status.len() > 1; - for baseboard in status.keys() { - for stream in [Stdout, Stderr] { - match with_login(ctx, client, async || { - client - .job_output() - .job_id(job_id) - .target(baseboard.to_string()) - .stream(stream) - .send() - .await - }) - .await - { - Ok(byte_stream) => { - let output = byte_stream_to_vec(byte_stream.into_inner()).await?; - ctx.job_output(&job_id, baseboard, stream, &output, binary); + let args = JobOutput { + job_id: Some(job_id), + binary, + file: None, + force: false, + chunk_size: DEFAULT_CHUNK_SIZE, + parallel: PARALLEL_CHUNKS, + }; + let streams = [Stdout, Stderr]; + let mut errors = Vec::new(); + for (baseboard, sled_status) in ran { + if let JobStatus::Error { error, .. } = sled_status { + match failed_job_output(ctx, client, job_id, baseboard, error, streams).await { + Ok(outputs) => { + for (stream, output) in streams.into_iter().zip(outputs) { + ctx.job_output(&job_id, baseboard, stream, &output, binary); + } + } + Err(error) => errors.push(error), + } + } else { + for stream in streams { + let output = job_output_with_status( + ctx, + client, + job_id, + baseboard, + Some(sled_status), + stream, + args.clone(), + ); + if let Err(error) = output.await { + errors.push(error); } - Err(error) if multiple => ctx.command_error(&error), - Err(error) => return Err(error), } } } + if let Some(last) = errors.pop() { + for error in &errors { + ctx.command_error(error); + } + return Err(last); + } } else { let sleds = cancelable(start).await?.into_inner().sleds; ctx.job_started(&job, Some(&sleds), StartedDisplay::Full); @@ -1875,7 +1896,7 @@ async fn job_start_request( return Err(CommandError::StreamingNeedsFile); }; if !start_args.force && path.exists() { - return Err(CommandError::io(path, ErrorKind::AlreadyExists.into())); + return Err(CommandError::io(path, io::ErrorKind::AlreadyExists.into())); } JobMode::StreamOutput } else { @@ -2198,47 +2219,74 @@ async fn job_output_from( client: &Client, target: &BaseboardId, stream: JobOutputStream, - JobOutput { + args: JobOutput, +) -> Result<(), CommandError> { + let job_id = job_id_or_last(ctx, args.job_id)?; + let status = job_status_map(ctx, client, &job_id, Some(target)).await?; + job_output_with_status( + ctx, + client, job_id, + target, + status.get(target), + stream, + args, + ) + .await +} + +/// Download one stream of a job's output from `target`, where the job +/// has `status`. +async fn job_output_with_status( + ctx: &mut impl CommandContext, + client: &Client, + job_id: JobId, + target: &BaseboardId, + status: Option<&JobStatus>, + stream: JobOutputStream, + JobOutput { binary, file, force, chunk_size, parallel, + .. }: JobOutput, ) -> Result<(), CommandError> { - let job_id = job_id_or_last(ctx, job_id)?; - - // Fetch job status for output length and hash. - let status = job_status_map(ctx, client, &job_id, Some(target)).await?; - let JobOutputState { stdout_len, stderr_len, stdout_hash, stderr_hash, - } = match status.get(target) { + } = match status { None => { return Err(CommandError::NotFound(format!( "Job `{job_id}` not found on sled `{target}`" ))); } - Some(JobStatus::Cancelled { .. }) => { - return Err(CommandError::JobCancelled(job_id.to_owned())); - } - Some(JobStatus::Queued { job_id, .. }) => { - return Err(CommandError::JobNotYetRunning(job_id.to_owned())); + Some(JobStatus::Stopped { output, .. }) => output, + Some(JobStatus::Started { .. }) => return Err(CommandError::JobStillRunning(job_id)), + Some(JobStatus::Skipped { reason, .. }) => { + return Err(CommandError::JobSkipped(job_id, *reason)); } + Some(JobStatus::Cancelled { .. }) => return Err(CommandError::JobCancelled(job_id)), + Some(JobStatus::Queued { .. }) => return Err(CommandError::JobNotYetRunning(job_id)), Some(JobStatus::Error { error, .. }) => { - return Err(CommandError::Process(error.to_owned())); - } - Some(JobStatus::Started { job_id, .. }) => { - return Err(CommandError::JobStillRunning(job_id.to_owned())); - } - Some(JobStatus::Skipped { job_id, reason, .. }) => { - return Err(CommandError::JobSkipped(job_id.to_owned(), *reason)); + // A failed job's status has no hash or length, so we can't verify it + // or download it in chunks. Dump its output with a warning. + let [bytes] = failed_job_output(ctx, client, job_id, target, error, [stream]).await?; + match file { + Some(path) => { + let io_error = |error| CommandError::io(&path, error); + let mut file = open_output_file(&path, force).map_err(io_error)?; + file.write_all(&bytes) + .and_then(|()| file.flush()) + .map_err(io_error)?; + } + None => ctx.job_output(&job_id, target, stream, &bytes, binary), + } + return Ok(()); } - Some(JobStatus::Stopped { output, .. }) => output, }; let len = match stream { Stdout => stdout_len, @@ -2271,14 +2319,7 @@ async fn job_output_from( // Open the output file. let io_error = |error| CommandError::io(&path, error); - let mut options = OpenOptions::new(); - options.read(true).write(true); - if force { - options.create(true).truncate(true); - } else { - options.create_new(true); - } - let mut file = options.open(&path).map_err(io_error)?; + let mut file = open_output_file(&path, force).map_err(io_error)?; // Download and write the output in parallel (unordered) chunks. ctx.job_output_started(&job_id, stream, "Downloading", *len); @@ -2314,26 +2355,71 @@ async fn job_output_from( } else { // Download and print the output all at once. If hash verification // fails here, do not print any output. - let byte_stream = with_login(ctx, client, { - async || { - client - .job_output() - .job_id(job_id) - .target(target.to_string()) - .stream(stream) - .send() - .await - } - }) - .await? - .into_inner(); - let bytes = byte_stream_to_vec(byte_stream).await?; + let bytes = fetch_job_output(ctx, client, &job_id, target, stream).await?; check_hash!(hash(&bytes), None)?; ctx.job_output(&job_id, target, stream, &bytes, binary); Ok(()) } } +/// Fetch `streams` of a failed job's output from `target`, unverified, +/// warning once that it may be partial. If any stream is missing, +/// report the job's own `error` instead. +async fn failed_job_output( + ctx: &mut impl CommandContext, + client: &Client, + job_id: JobId, + target: &BaseboardId, + error: &ProcessError, + streams: [JobOutputStream; N], +) -> Result<[Vec; N], CommandError> { + let mut outputs = [const { Vec::new() }; N]; + for (output, stream) in outputs.iter_mut().zip(streams) { + *output = match fetch_job_output(ctx, client, &job_id, target, stream).await { + Ok(bytes) => bytes, + Err(CommandError::NotFound(_)) => return Err(CommandError::Process(error.to_owned())), + Err(other) => return Err(other), + }; + } + ctx.command_error(&CommandError::PartialOutput(job_id, error.to_owned())); + Ok(outputs) +} + +/// Open `path` to save job output in, replacing an existing file only +/// if `force` is set. +fn open_output_file(path: &Path, force: bool) -> io::Result { + let mut options = OpenOptions::new(); + options.read(true).write(true); + if force { + options.create(true).truncate(true); + } else { + options.create_new(true); + } + options.open(path) +} + +/// Download one stream of a job's output in a single request. +async fn fetch_job_output( + ctx: &mut impl CommandContext, + client: &Client, + job_id: &JobId, + target: &BaseboardId, + stream: JobOutputStream, +) -> Result, CommandError> { + let byte_stream = with_login(ctx, client, async || { + client + .job_output() + .job_id(job_id) + .target(target.to_string()) + .stream(stream) + .send() + .await + }) + .await? + .into_inner(); + byte_stream_to_vec(byte_stream).await +} + /// Prepare a vector of futures that fetch chunks of output. fn job_output_chunks<'a>( client: &'a Client, @@ -2698,12 +2784,9 @@ pub enum CommandError { #[error("Interactive jobs must target exactly one sled")] InteractiveTarget, #[error("Local I/O error accessing `{path}`: {error}")] - Io { - path: PathBuf, - error: std::io::Error, - }, + Io { path: PathBuf, error: io::Error }, #[error("Signal handling error: {0}")] - Signal(#[from] std::io::Error), + Signal(#[from] io::Error), #[error("Unauthorized, try `iam`")] InvalidAuthorization, #[error("Leaf certificate does not match key `{0}`")] @@ -2781,7 +2864,9 @@ pub enum CommandError { #[error("permslip error: {0}")] Permslip(#[from] PermslipError), #[error("Job process error: {0}")] - Process(#[from] sush_common::jobs::ProcessError), + Process(#[from] ProcessError), + #[error("Job `{0}` did not exit cleanly, output is unverified:\n {1}")] + PartialOutput(JobId, ProcessError), #[error("Proxy TLS error: {0}")] ProxyTls(#[from] tls::ProxyTlsError), #[error("Goodbye!")] @@ -2798,7 +2883,7 @@ pub enum CommandError { Signature(KeyId), #[error("Streaming jobs need `--file`")] StreamingNeedsFile, - #[error("Job `{0}` did not exit cleanly, streamed output is unverified")] + #[error("Job `{0}` did not exit cleanly, output is unverified")] StreamUnverified(JobId), #[error("Can't parse target baseboard ID: {0}")] BaseboardIdParseError(sled_hardware_types::BaseboardIdParseError), @@ -2826,7 +2911,7 @@ pub enum CommandError { impl CommandError { /// Report I/O errors with the corresponding path or stream name. - pub fn io(path: impl AsRef, error: std::io::Error) -> Self { + pub fn io(path: impl AsRef, error: io::Error) -> Self { Self::Io { path: path.as_ref().to_owned(), error, @@ -2841,7 +2926,7 @@ impl CommandError { fn icon(&self) -> &'static str { match self { Self::Clap(_) => "❓", - Self::JobNotStarted(..) => "❗", + Self::JobNotStarted(..) | Self::PartialOutput(..) => "❗", Self::JobSkipped(..) => "⏩", Self::Quit => "👋", Self::Recursive(error) => error.icon(), From 6a4588aa1872a9eec690e0349a380295d0d9d46a Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 13:22:05 -0700 Subject: [PATCH 5/6] Rename JobOutput to JobOutputArgs Co-authored-by: Claude --- client/src/commands.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/client/src/commands.rs b/client/src/commands.rs index e083f915..4f8c9611 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -320,7 +320,7 @@ impl ClientArgs { /// Arguments for `job-stdout` and `job-stderr`. #[derive(Clone, Debug, Parser)] -pub struct JobOutput { +pub struct JobOutputArgs { /// The job whose output should be fetched. #[clap(env = SUSH_JOB_ID)] job_id: Option, @@ -612,7 +612,7 @@ pub enum JobCommand { target: TargetArg, #[clap(flatten)] - output: JobOutput, + output: JobOutputArgs, }, /// Get the standard error of a job. @@ -623,7 +623,7 @@ pub enum JobCommand { target: TargetArg, #[clap(flatten)] - output: JobOutput, + output: JobOutputArgs, }, /// Attach to an interactive job. @@ -1572,7 +1572,7 @@ async fn job_start( match job_attach(ctx, client, &job_id, &target).await { Ok(()) => (), Err(CommandError::NotFound(_)) => { - let output = JobOutput { + let output = JobOutputArgs { job_id: Some(job_id), binary: true, file: None, @@ -1758,7 +1758,7 @@ async fn job_start( CommandError::JobStillRunning(job_id) }); } - let args = JobOutput { + let args = JobOutputArgs { job_id: Some(job_id), binary, file: None, @@ -2180,7 +2180,7 @@ async fn job_output( client: &Client, target: &TargetArg, stream: JobOutputStream, - args: JobOutput, + args: JobOutputArgs, ) -> Result<(), CommandError> { let job_id = job_id_or_last(ctx, args.job_id)?; let target = match target { @@ -2219,7 +2219,7 @@ async fn job_output_from( client: &Client, target: &BaseboardId, stream: JobOutputStream, - args: JobOutput, + args: JobOutputArgs, ) -> Result<(), CommandError> { let job_id = job_id_or_last(ctx, args.job_id)?; let status = job_status_map(ctx, client, &job_id, Some(target)).await?; @@ -2244,14 +2244,14 @@ async fn job_output_with_status( target: &BaseboardId, status: Option<&JobStatus>, stream: JobOutputStream, - JobOutput { + JobOutputArgs { binary, file, force, chunk_size, parallel, .. - }: JobOutput, + }: JobOutputArgs, ) -> Result<(), CommandError> { let JobOutputState { stdout_len, @@ -3025,7 +3025,7 @@ mod tests { async fn output_needs_target() { let client = Client::new_with_client("http://[::1]:1", reqwest::Client::new(), Default::default()); - let output = JobOutput::try_parse_from([ + let output = JobOutputArgs::try_parse_from([ "job-stdout", "--binary", "sea-say-sting-palm-tunnel-festival-pull-bid", From dd5c5b906cda2102edd35c2af911d65589b600e3 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Sat, 7 Nov 2026 12:45:02 -0700 Subject: [PATCH 6/6] Check the target sled before signing a job Co-authored-by: Claude --- api/src/lib.rs | 17 +++++++++----- client/src/cli.rs | 10 --------- client/src/commands.rs | 46 ++++++++++---------------------------- client/src/context.rs | 3 +-- client/src/repl.rs | 6 +---- proxy/src/lib.rs | 15 ++++++++++++- server/src/manager.rs | 17 ++++++++++---- server/src/server.rs | 16 ++++++++----- sush.json | 36 +++++++++++++++++------------ tests/src/manager_tests.rs | 13 ++++++++++- 10 files changed, 97 insertions(+), 82 deletions(-) diff --git a/api/src/lib.rs b/api/src/lib.rs index 98dde098..c10fe42d 100644 --- a/api/src/lib.rs +++ b/api/src/lib.rs @@ -75,14 +75,15 @@ pub trait SushApi { headers: Header, ) -> Result, HttpError>; - /// Check that this server would accept a job in a session. + /// Check that this server would accept a job in a session for a target. /// - /// Succeeds if the session is active or its start is pending. - #[endpoint { method = GET, path = "/sessions/{session_id}/check" }] - async fn session_check( + /// Succeeds if the session is active or its start is pending, and + /// the target resolves to at least one sled this server can reach. + #[endpoint { method = GET, path = "/sessions/{session_id}/preflight/{target}" }] + async fn job_preflight( ctx: RequestContext, headers: Header, - params: PathParams, + params: PathParams, ) -> Result; /// Get a fresh server nonce for the session ceremony. @@ -258,6 +259,12 @@ pub struct SessionIdParam { pub session_id: SessionId, } +#[derive(Deserialize, JsonSchema)] +pub struct SessionTargetParams { + pub session_id: SessionId, + pub target: String, +} + #[derive(Deserialize, JsonSchema)] pub struct SessionAndJobIds { pub session_id: SessionId, diff --git a/client/src/cli.rs b/client/src/cli.rs index e6ff4ce6..0d200e7f 100644 --- a/client/src/cli.rs +++ b/client/src/cli.rs @@ -1151,16 +1151,6 @@ impl CommandContext for Cli { Ok(()) } - fn really_target(&mut self, sled: &SledId) -> Result<(), CommandError> { - if read_bool(&format!( - "❓ Sled `{sled}` is not in the rack inventory. Proceed (yes/no)? " - ))? { - Ok(()) - } else { - Err(CommandError::Canceled) - } - } - fn please_touch(&mut self, identity: &SshPublicKey) -> Result<(), CommandError> { if identity.is_sk_algorithm() { eprintln!( diff --git a/client/src/commands.rs b/client/src/commands.rs index 4f8c9611..cdb5933c 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -1906,13 +1906,6 @@ async fn job_start_request( let Some(session_id) = ctx.session_id() else { return Err(CommandError::MissingSession); }; - if let Some(client) = client { - with_login(ctx, client, async || { - client.session_check().session_id(session_id).send().await - }) - .await?; - } - let target = match &start_args.target { TargetArg::Target(target) => target.clone(), abbreviated => { @@ -1928,12 +1921,22 @@ async fn job_start_request( return Err(CommandError::InteractiveTarget); }; Target::from(resolve_target(client, &target).await?) - } else if let Some(client) = client { - preflight_target(ctx, client, target).await? } else { target }; + if let Some(client) = client { + with_login(ctx, client, async || { + client + .job_preflight() + .session_id(session_id) + .target(target.to_string()) + .send() + .await + }) + .await?; + } + Ok(JobStartRequest::new( ctx.next_job_id()?, session_id, @@ -2659,31 +2662,6 @@ impl FromStr for TargetArg { } } -/// Confirm before signing a job for a sled that doesn't appear in inventory. -async fn preflight_target( - ctx: &mut impl CommandContext, - client: &Client, - target: Target, -) -> Result { - let Target::Sleds(sleds) = &target else { - return Ok(target); - }; - let Ok(inventory) = client.versions().send().await else { - return Ok(target); - }; - let inventory = inventory.into_inner(); - for sled in sleds { - let known = match sled { - SledId::Baseboard(baseboard) => inventory.iter().any(|s| &s.baseboard == baseboard), - SledId::Cubby(cubby) => inventory.iter().any(|s| s.cubby == Some(*cubby)), - }; - if !known { - ctx.really_target(sled)?; - } - } - Ok(target) -} - /// Resolve a target argument to a target, matching bare serial /// numbers against the rack's sled inventory. async fn resolve_target_arg(client: &Client, target: &TargetArg) -> Result { diff --git a/client/src/context.rs b/client/src/context.rs index 80532140..1b70ce59 100644 --- a/client/src/context.rs +++ b/client/src/context.rs @@ -19,7 +19,7 @@ use sush_common::jobs::{ SessionId, SessionSeq, SessionSignerNonce, SessionSushNonce, SignedJob, }; use sush_common::keys::{KeyId, SshPublicKey}; -use sush_common::targets::{SledId, SledVersion}; +use sush_common::targets::SledVersion; use sush_common::version::VersionInfo; use crate::AuthzSigner; @@ -189,7 +189,6 @@ pub trait CommandContext: Clone + Send + Sync { ) -> Result; // Job management - fn really_target(&mut self, sled: &SledId) -> Result<(), CommandError>; fn job_started( &mut self, job: &SignedJob, diff --git a/client/src/repl.rs b/client/src/repl.rs index 93e6a3d2..668f574e 100644 --- a/client/src/repl.rs +++ b/client/src/repl.rs @@ -25,7 +25,7 @@ use sush_common::jobs::{ SessionId, SessionSeq, SessionSignerNonce, SessionSushNonce, SignedJob, }; use sush_common::keys::{KeyId, SshPublicKey}; -use sush_common::targets::{SledId, SledVersion}; +use sush_common::targets::SledVersion; use sush_common::version::VersionInfo; use crate::cli::Cli; @@ -466,8 +466,4 @@ impl CommandContext for Repl { fn please_authn(&mut self, url: &str) { self.cli.please_authn(url) } - - fn really_target(&mut self, sled: &SledId) -> Result<(), CommandError> { - self.cli.really_target(sled) - } } diff --git a/proxy/src/lib.rs b/proxy/src/lib.rs index 69e50c35..ceeb0070 100644 --- a/proxy/src/lib.rs +++ b/proxy/src/lib.rs @@ -265,7 +265,12 @@ fn named_target(request: &Request) -> Option> { let uri = request.uri(); let segments: Vec<&str> = uri.path().trim_start_matches('/').split('/').collect(); let named = match segments.as_slice() { - ["jobs", _, "attach" | "output" | "start", target, ..] if *target != "*" => Some(*target), + ["jobs", _, "attach" | "output" | "start", target, ..] + | ["sessions", _, "preflight", target] + if *target != "*" => + { + Some(*target) + } _ => uri.query().and_then(|query| { query.split('&').find_map(|param| { param @@ -689,6 +694,14 @@ mod test { Some(Ok(target("test part:0000"))), ); assert_eq!(named_target(&request("/jobs/some-job/start/*")), None); + assert_eq!( + named_target(&request("/sessions/some-session/preflight/14")), + Some(Ok(target("14"))), + ); + assert_eq!( + named_target(&request("/sessions/some-session/preflight/*")), + None + ); assert_eq!( named_target(&request("/jobs/some-job/attach/test%20part:0000")), Some(Ok(target("test part:0000"))), diff --git a/server/src/manager.rs b/server/src/manager.rs index b62c3d0c..0da46b93 100644 --- a/server/src/manager.rs +++ b/server/src/manager.rs @@ -435,13 +435,12 @@ impl JobManager { // Check the session and target. let payload = job.payload(); - self.session_check(payload.session_id())?; if !payload.target_matches_mode() { return Err(JobError::InteractiveTarget); } + let sleds = self.job_preflight(payload.session_id(), payload.target())?; // Submit the job for execution. - let sleds = self.reachable_targets(payload.target())?; let wait = params.wait.to_owned(); self.job_request(authn, JobRequest::Start(job, params, sleds.clone())) .await?; @@ -449,8 +448,18 @@ impl JobManager { Ok(sleds) } - /// The sleds `target` names that this sled can reach: itself and - /// the sleds it holds live gossip links to. + /// Check that this sled would accept a job in `session_id` for `target`. + pub fn job_preflight( + &self, + session_id: SessionId, + target: &Target, + ) -> Result, JobError> { + self.session_check(session_id)?; + self.reachable_targets(target) + } + + /// Return the sleds in `target` that match either this sled or any of + /// its live gossip peers. fn reachable_targets(&self, target: &Target) -> Result, JobError> { let mut reachable = self.linked.borrow().clone(); reachable.insert(self.own_baseboard.clone()); diff --git a/server/src/server.rs b/server/src/server.rs index c2ae2d4e..26c9550e 100644 --- a/server/src/server.rs +++ b/server/src/server.rs @@ -24,14 +24,15 @@ use tokio_tungstenite::tungstenite::protocol::{Role, WebSocketConfig}; use sush_api::{ AccessParam, Authorization, AuthorizedRangeRequest, JobHistoryParams, JobIdParam, JobOutputParams, JobStartParams, JobStarted, JobStopParams, JobTargetParams, KeyIdParam, - RoutingParam, SessionAndJobIds, SessionAndKeyIds, SessionIdParam, SessionStartBody, SushApi, + RoutingParam, SessionAndJobIds, SessionAndKeyIds, SessionIdParam, SessionStartBody, + SessionTargetParams, SushApi, }; use sush_common::authn::{Authn, Identity}; use sush_common::jobs::{ JsonJobStatusMap, Session, SessionServerNonce, SignedJob, job_status_to_json_map, }; use sush_common::keys::{KeyId, SshPublicKey, pem_cert_chain}; -use sush_common::targets::SledVersion; +use sush_common::targets::{SledVersion, Target}; use sush_common::version::VersionInfo; use crate::error::JobError; @@ -104,16 +105,19 @@ impl SushApi for ApiServer { } } - async fn session_check( + async fn job_preflight( ctx: RequestContext, headers: Header, - params: PathParams, + params: PathParams, ) -> Result { let mgr = ctx.context(); let Authorization { authorization } = headers.into_inner(); let _authn = requester(authorization)?; - let SessionIdParam { session_id } = params.into_inner(); - mgr.session_check(session_id)?; + let SessionTargetParams { session_id, target } = params.into_inner(); + let target = target.parse::().map_err(|error| { + HttpError::for_client_error(None, ClientErrorStatusCode::BAD_REQUEST, error.to_string()) + })?; + mgr.job_preflight(session_id, &target)?; Ok(HttpResponseUpdatedNoContent()) } diff --git a/sush.json b/sush.json index d7cc0f89..31f9982c 100644 --- a/sush.json +++ b/sush.json @@ -686,11 +686,11 @@ } } }, - "/sessions/{session_id}/check": { - "get": { - "summary": "Check that this server would accept a job in a session.", - "description": "Succeeds if the session is active or its start is pending.", - "operationId": "session_check", + "/sessions/{session_id}/deny/{key_id}": { + "post": { + "summary": "Withdraw a key's attach access.", + "description": "Only the key that started the session may withdraw access. Already-attached connections keep the access they attached with.", + "operationId": "session_deny_attach", "parameters": [ { "in": "header", @@ -700,6 +700,14 @@ "type": "string" } }, + { + "in": "path", + "name": "key_id", + "required": true, + "schema": { + "$ref": "#/components/schemas/KeyId" + } + }, { "in": "path", "name": "session_id", @@ -722,11 +730,11 @@ } } }, - "/sessions/{session_id}/deny/{key_id}": { - "post": { - "summary": "Withdraw a key's attach access.", - "description": "Only the key that started the session may withdraw access. Already-attached connections keep the access they attached with.", - "operationId": "session_deny_attach", + "/sessions/{session_id}/preflight/{target}": { + "get": { + "summary": "Check that this server would accept a job in a session for a target.", + "description": "Succeeds if the session is active or its start is pending, and the target resolves to at least one sled this server can reach.", + "operationId": "job_preflight", "parameters": [ { "in": "header", @@ -738,18 +746,18 @@ }, { "in": "path", - "name": "key_id", + "name": "session_id", "required": true, "schema": { - "$ref": "#/components/schemas/KeyId" + "$ref": "#/components/schemas/SessionId" } }, { "in": "path", - "name": "session_id", + "name": "target", "required": true, "schema": { - "$ref": "#/components/schemas/SessionId" + "type": "string" } } ], diff --git a/tests/src/manager_tests.rs b/tests/src/manager_tests.rs index 62ae1990..c26541ce 100644 --- a/tests/src/manager_tests.rs +++ b/tests/src/manager_tests.rs @@ -1553,7 +1553,8 @@ async fn attribution() { /// A job runs only on the sleds its target names that the accepting /// sled can reach. A job left with none, because it names only an -/// unreachable baseboard or an unknown cubby, is refused. +/// unreachable baseboard or an unknown cubby, is refused, and the +/// preflight refuses it too. #[named] #[tokio::test] async fn job_targets() { @@ -1595,6 +1596,16 @@ async fn job_targets() { }; let elsewhere = "913-0000019:BRM00000000"; + let preflight = |target: &str| mgr.job_preflight(session_id, &target.parse().unwrap()); + assert!(matches!( + preflight(elsewhere), + Err(JobError::NoReachableTarget) + )); + assert!(matches!(preflight("14"), Err(JobError::UnknownCubby(14)))); + assert_eq!( + preflight(&format!("{elsewhere},{}", test_baseboard_id())).unwrap(), + BTreeSet::from([test_baseboard_id()]), + ); assert!(matches!( start("true", elsewhere).await, Err(JobError::NoReachableTarget),