From 30b0023d0c636ed9a6f77cc8c3e6ceb64edd27c4 Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 12:30:02 +0200 Subject: [PATCH 01/10] create new Codephrase wrapper --- common/src/codephrases.rs | 180 +++++++++++++++++++++++++++++++++++++- common/src/lib.rs | 1 + 2 files changed, 180 insertions(+), 1 deletion(-) diff --git a/common/src/codephrases.rs b/common/src/codephrases.rs index 1fac573..7b39dd3 100644 --- a/common/src/codephrases.rs +++ b/common/src/codephrases.rs @@ -9,11 +9,21 @@ //! that must be readily transmissible over low bandwidth channels (e.g., //! email, voice, printed or handwritten notes, etc.). -use crypto_bigint::{CheckedAdd as _, CheckedMul as _, Limb, Random as _, Reciprocal, U256}; +use std::str::FromStr; + +use crypto_bigint::{ + ArrayEncoding as _, CheckedAdd as _, CheckedMul as _, Encoding as _, Limb, Random as _, + Reciprocal, U256, +}; use rand_core::OsRng; +use schemars::JsonSchema; +use serde::de::Error as _; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; use thiserror::Error; use crate::wordlist::{WORDLIST, WORDLIST_LEN}; +use borsh::{BorshDeserialize, BorshSerialize}; +use std::io::prelude::{Read, Write}; /// Entropy is treated as an integer whose base is to be changed /// to 2048, which gives us indexes into the BIP-39 word list. @@ -35,6 +45,174 @@ pub const PHRASE_WORDS_ID: usize = 8; /// will also accept arbitrary ASCII whitespace as word separators. pub const WORD_SEPARATOR: &str = "-"; +const TRUNCATED_MASK: U256 = U256::from_u128(2u128.pow(88) - 1); + +/// Random code phrase like `abstract misery favorite ordinary moon talk`. +#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct Codephrase(U256); + +impl Codephrase { + /// Generate a new random codephrase. + pub fn random() -> Self { + Self(U256::random(&mut OsRng)) + } + + pub fn truncate(self) -> Self { + Self(self.0.bitand(&TRUNCATED_MASK)) + } + + pub fn from_be_bytes(bytes: [u8; 32]) -> Self { + Self(U256::from_be_bytes(bytes)) + } + + /// Get the underlying big-endian byte representation of the codephrase. + pub fn to_be_bytes(&self) -> [u8; 32] { + self.0.to_be_byte_array().into() + } +} + +impl std::fmt::Display for Codephrase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&codephrase(self.0).join(WORD_SEPARATOR)) + } +} + +impl std::fmt::Debug for Codephrase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "Codephrase({self})") + } +} + +impl FromStr for Codephrase { + type Err = InvalidCodephrase; + + fn from_str(value: &str) -> Result { + Ok(Self(decode_phrase(value)?)) + } +} + +impl Serialize for Codephrase { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&self.to_string()) + } +} + +impl<'de> Deserialize<'de> for Codephrase { + fn deserialize>(deserializer: D) -> Result { + let string = ::deserialize(deserializer)?; + Ok(Self(decode_phrase(&string).map_err(D::Error::custom)?)) + } +} + +impl BorshSerialize for Codephrase { + fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + <[u8; 32] as BorshSerialize>::serialize(&self.to_be_bytes(), writer) + } +} + +impl BorshDeserialize for Codephrase { + fn deserialize_reader(reader: &mut R) -> std::io::Result { + let bytes = <[u8; 32] as BorshDeserialize>::deserialize_reader(reader)?; + Ok(Self(U256::from_be_byte_array(bytes.into()))) + } +} + +// Treat a codephrase as a string in JSON schemas. +impl JsonSchema for Codephrase { + fn schema_name() -> String { + String::schema_name() + } + + fn json_schema(generator: &mut schemars::r#gen::SchemaGenerator) -> schemars::schema::Schema { + String::json_schema(generator) + } + + fn is_referenceable() -> bool { + String::is_referenceable() + } + + fn schema_id() -> std::borrow::Cow<'static, str> { + String::schema_id() + } +} + +#[macro_export] +macro_rules! codephrase_newtype { + ($(#[$meta:meta])* $vis:vis struct $name:ident = $len:ident;) => { + $(#[$meta])* + $vis struct $name($crate::codephrases::Codephrase); + + impl $name { + #[allow(unused)] + $vis fn random() -> Self { + let mut codephrase = $crate::codephrases::Codephrase::random(); + match $crate::codephrases::CodephraseLength::$len { + $crate::codephrases::CodephraseLength::Full => {} + $crate::codephrases::CodephraseLength::Truncated => { + codephrase = codephrase.truncate(); + } + } + Self(codephrase) + } + + #[allow(unused)] + $vis fn from_hash(hash: blake3::Hash) -> Self { + let mut codephrase = $crate::codephrases::Codephrase::from_be_bytes(*hash.as_bytes()); + match $crate::codephrases::CodephraseLength::$len { + $crate::codephrases::CodephraseLength::Full => {} + $crate::codephrases::CodephraseLength::Truncated => { + codephrase = codephrase.truncate(); + } + } + Self(codephrase) + } + + #[allow(unused)] + $vis fn from_be_bytes(bytes: [u8; 32]) -> Self { + let mut codephrase = $crate::codephrases::Codephrase::from_be_bytes(bytes); + match $crate::codephrases::CodephraseLength::$len { + $crate::codephrases::CodephraseLength::Full => {} + $crate::codephrases::CodephraseLength::Truncated => { + codephrase = codephrase.truncate(); + } + } + Self(codephrase) + } + + #[allow(unused)] + $vis fn to_be_bytes(&self) -> [u8; 32] { + self.0.to_be_bytes() + } + } + + impl std::fmt::Debug for $name { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}({})", stringify!($name), self.0) + } + } + + impl std::fmt::Display for $name { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + <$crate::codephrases::Codephrase as std::fmt::Display>::fmt(&self.0, f) + } + } + + impl std::str::FromStr for $name { + type Err = $crate::codephrases::InvalidCodephrase; + + fn from_str(s: &str) -> Result<$name, Self::Err> { + Ok($name(s.parse()?)) + } + } + } +} + +#[derive(Clone, Copy)] +pub enum CodephraseLength { + Full, + Truncated, +} + /// Decoding a phrase failed. #[derive(Debug, Error)] #[error("invalid or non-canonical code phrase")] diff --git a/common/src/lib.rs b/common/src/lib.rs index a88bc26..da9e5b3 100644 --- a/common/src/lib.rs +++ b/common/src/lib.rs @@ -6,6 +6,7 @@ pub mod authn; pub mod borsh; +#[macro_use] pub mod codephrases; pub mod interactive; pub mod jobs; From 2dfc110ef8943018ad7a0ec41469f9dc49023a0a Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 12:51:51 +0200 Subject: [PATCH 02/10] switch auth nonces to the newtype --- common/src/authn.rs | 88 +++++++----------- common/src/keys.rs | 8 +- server/src/manager.rs | 2 +- server/src/messages.rs | 10 +- server/tests/common/mod.rs | 2 +- .../tests/output/identity-login-request.bin | Bin 199 -> 241 bytes tests/src/manager_tests.rs | 5 +- tests/src/test_utils.rs | 2 +- 8 files changed, 49 insertions(+), 68 deletions(-) diff --git a/common/src/authn.rs b/common/src/authn.rs index 3ec3cdb..6a1be55 100644 --- a/common/src/authn.rs +++ b/common/src/authn.rs @@ -41,9 +41,8 @@ use schemars::JsonSchema; use serde::{Deserialize, Serialize}; use thiserror::Error; -use crate::codephrases::{ - InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase, generate_id, -}; +use crate::codephrase_newtype; +use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase}; use crate::keys::{EncodedSignature, KeyError, KeyId, Signed, SshPublicKey, ToBeSigned, Verified}; /// The name of our custom HTTP authentication scheme. @@ -63,43 +62,22 @@ pub const IDENTITY_MAX_TTL: Duration = Duration::from_secs(8 * 3600); /// The time-to-live of a nonce. pub const NONCE_TTL: Duration = Duration::from_secs(60); -/// A unique random string. Authentication credentials have two -/// of these: one generated by the server, and one by the client. -/// This structure is agnostic to the syntax of the string. -#[derive( - BorshDeserialize, - BorshSerialize, - Clone, - Debug, - Deserialize, - Eq, - Hash, - JsonSchema, - PartialEq, - Serialize, -)] -pub struct Nonce(String); - -impl Nonce { - pub fn generate() -> Self { - Self(generate_id()) - } - - pub fn as_bytes(&self) -> &[u8] { - self.0.as_bytes() - } -} - -impl From for Nonce { - fn from(nonce: String) -> Self { - Self(nonce) - } -} - -impl fmt::Display for Nonce { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } +codephrase_newtype! { + /// A unique random string. Authentication credentials have two + /// of these: one generated by the server, and one by the client. + /// This structure is agnostic to the syntax of the string. + #[derive( + BorshDeserialize, + BorshSerialize, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + PartialEq, + Serialize, + )] + pub struct Nonce = Truncated; } /// Authentication challenge. @@ -135,7 +113,7 @@ impl FromStr for Challenge { /// Parse a server challenge, e.g., `Sush nonce=foo-bar-baz`. fn from_str(s: &str) -> Result { let mut params = AuthnParams::from_str(s)?; - let nonce = params.get("nonce")?; + let nonce = params.parse("nonce")?; params.finished()?; Ok(Self::new(Nonce(nonce))) } @@ -291,7 +269,7 @@ impl BoundRequest { } = self; hash(Self::TYPE_NAME); hash(key_id.as_bytes()); - hash(nonce.as_bytes()); + hash(&nonce.to_be_bytes()); hash(method.as_bytes()); hash(target.as_bytes()); hash(&seq.to_be_bytes()); @@ -334,7 +312,7 @@ impl FromStr for BoundCredentials { let mut params = AuthnParams::from_str(s)?; let creds = Self { key_id: params.get("key-id")?, - nonce: params.get("nonce")?, + nonce: params.parse("nonce")?, seq: params.parse("seq")?, signature: EncodedSignature { r: params.get("r")?, @@ -432,7 +410,7 @@ impl ChallengeResponse { pub fn new(challenge: Challenge, epk: RequestVerifier) -> Self { Self { nonce: challenge.nonce, - cnonce: Nonce::generate(), + cnonce: Nonce::random(), epk, } } @@ -453,8 +431,8 @@ impl ToBeSigned for ChallengeResponse { let Self { nonce, cnonce, epk } = self; hash(Self::TYPE_NAME); - hash(nonce.as_bytes()); - hash(cnonce.as_bytes()); + hash(&nonce.to_be_bytes()); + hash(&cnonce.to_be_bytes()); hash(epk.as_bytes()); hasher.finalize().as_bytes().to_vec() } @@ -543,8 +521,8 @@ impl FromStr for Credentials { fn from_str(s: &str) -> Result { let mut params = AuthnParams::from_str(s)?; let creds = Self { - nonce: params.get("nonce")?, - cnonce: params.get("cnonce")?, + nonce: params.parse("nonce")?, + cnonce: params.parse("cnonce")?, key_id: params.get("key-id")?, epk: params.parse("epk")?, signature: EncodedSignature { @@ -697,7 +675,7 @@ mod test { let mut signer = EphemeralKey::new_root(KeyType::Ed25519, name, validity).unwrap(); for _ in 0..10 { let key_id = signer.key_id().clone(); - let nonce = Nonce::generate(); + let nonce = Nonce::random(); let challenge = Challenge::new(nonce.clone()); let www_authenticate = challenge.to_string(); assert_eq!(www_authenticate, format!("Sush nonce={nonce}")); @@ -752,7 +730,7 @@ mod test { let key = RequestKey::new(); let verifier = key.verifier(); let request = BoundRequest::new("get", "/jobs?limit=1", 7); - let creds = key.bind(KeyId::from("baz".to_string()), Nonce::generate(), &request); + let creds = key.bind(KeyId::from("baz".to_string()), Nonce::random(), &request); assert_eq!( creds.to_string().parse::().unwrap(), creds @@ -786,7 +764,7 @@ mod test { }; assert!(verifier.verify(&request, &relabeled).is_err()); let relabeled = BoundCredentials { - nonce: Nonce::generate(), + nonce: Nonce::random(), ..creds }; assert!(verifier.verify(&request, &relabeled).is_err()); @@ -851,27 +829,27 @@ mod test { AuthnError::MissingParam(p) if p == "nonce" )); assert!(matches!( - Credentials::from_str("Sush nonce=foo").unwrap_err(), + Credentials::from_str("Sush nonce=abandon").unwrap_err(), AuthnError::MissingParam(p) if p == "cnonce" )); let epk = RequestKey::new().verifier(); assert!(matches!( Credentials::from_str( - "Sush nonce=foo,cnonce=bar,key-id=baz,epk=plugh,r=r,s=s,flags=0,counter=0" + "Sush nonce=abandon,cnonce=ability,key-id=baz,epk=plugh,r=r,s=s,flags=0,counter=0" ) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=foo,cnonce=bar,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=foo" + "Sush nonce=abandon,cnonce=ability,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=foo" )) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=foo,cnonce=bar,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=0,foo=bar" + "Sush nonce=abandon,cnonce=ability,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=0,foo=bar" )) .unwrap_err(), AuthnError::TooManyParams diff --git a/common/src/keys.rs b/common/src/keys.rs index d26b2ae..6cd2b65 100644 --- a/common/src/keys.rs +++ b/common/src/keys.rs @@ -1068,10 +1068,10 @@ mod test { .unwrap(); let mut signatures = HashSet::new(); for _ in 0..100 { - let nonce = Nonce::generate(); - let signed = key.sign(nonce.as_bytes()).await.unwrap(); + let nonce = Nonce::random(); + let signed = key.sign(nonce.to_be_bytes()).await.unwrap(); assert_eq!(signed.key_id(), key.key_id()); - assert_eq!(*signed.payload(), nonce.as_bytes()); + assert_eq!(*signed.payload(), nonce.to_be_bytes()); let signature = signed.signature(); assert!(signatures.insert(signature.clone()), "duplicate signature"); let signature_string = serde_json::to_string(&signature).unwrap(); @@ -1084,7 +1084,7 @@ mod test { ); let verified = signed.verify_with_cert(key.cert()).unwrap(); assert_eq!(verified.verified_by(), key.key_id()); - assert_eq!(verified.into_payload(), nonce.as_bytes()); + assert_eq!(verified.into_payload(), nonce.to_be_bytes()); } } } diff --git a/server/src/manager.rs b/server/src/manager.rs index 44563d5..70a0443 100644 --- a/server/src/manager.rs +++ b/server/src/manager.rs @@ -256,7 +256,7 @@ impl JobManager { macro_rules! unauthorized { ($error:expr) => {{ warn!(self.log, "authentication failed"; "error" => %$error); - let nonce = Nonce::generate(); + let nonce = Nonce::random(); self.nonces.lock().await.put(nonce.clone(), Instant::now()); return Err(JobError::unauthorized(nonce)); }}; diff --git a/server/src/messages.rs b/server/src/messages.rs index 37a3283..d84ad1e 100644 --- a/server/src/messages.rs +++ b/server/src/messages.rs @@ -240,6 +240,8 @@ mod wire_format { use super::v0::*; use super::*; + use std::str::FromStr as _; + use sush_common::authn::Nonce; /// Serialize a message and compare against the snapshot in /// `tests/output/`, or rewrite it under `EXPECTORATE=overwrite`. @@ -350,9 +352,11 @@ mod wire_format { // Craft deterministic evidence: nonces, then the ed25519 // basepoint as the verifier. let mut evidence = Vec::new(); - for nonce in ["abandon", "ability"] { - evidence.extend((nonce.len() as u32).to_le_bytes()); - evidence.extend(nonce.as_bytes()); + for nonce in [ + Nonce::from_str("abandon").unwrap(), + Nonce::from_str("ability").unwrap(), + ] { + evidence.extend(&nonce.to_be_bytes()); } evidence.push(0x58); evidence.extend([0x66; 31]); diff --git a/server/tests/common/mod.rs b/server/tests/common/mod.rs index 44dabd0..baacc3e 100644 --- a/server/tests/common/mod.rs +++ b/server/tests/common/mod.rs @@ -139,7 +139,7 @@ pub fn ephemeral_root() -> EphemeralKey { /// An authenticated identity for `key`, as `iam` would produce. pub async fn fake_identity(key: &mut EphemeralKey) -> Identity { - let challenge = Challenge::new(Nonce::generate()); + let challenge = Challenge::new(Nonce::random()); let response = ChallengeResponse::new(challenge, RequestKey::new().verifier()); let signed = key.sign(response).await.unwrap(); let verified = signed diff --git a/server/tests/output/identity-login-request.bin b/server/tests/output/identity-login-request.bin index cde4443c819a3da0d127bcab6002ec2c49d0b9d4..ae710271e52dfa00111a04a12650eeff9b367d8d 100644 GIT binary patch delta 84 ccmX@k_>pl!o+ATgfH7j?MB#}GSindf02p-$N&o-= delta 36 ncmey!c${%Uo;W)L14Cj`VqQvq9)y{hlUY(3F>#~p#51Y@%GwLq diff --git a/tests/src/manager_tests.rs b/tests/src/manager_tests.rs index eb7e53e..eff10d6 100644 --- a/tests/src/manager_tests.rs +++ b/tests/src/manager_tests.rs @@ -1214,7 +1214,7 @@ async fn gossiped_identities() { // A liar claims root's key with evidence signed by their own. let bogus_key = RequestKey::new(); - let bogus = ChallengeResponse::new(Challenge::new(Nonce::generate()), bogus_key.verifier()); + let bogus = ChallengeResponse::new(Challenge::new(Nonce::random()), bogus_key.verifier()); let signed_by_liar = liar.sign(bogus).await.unwrap(); let bogus_verified = signed_by_liar .clone() @@ -1233,8 +1233,7 @@ async fn gossiped_identities() { // Real evidence authorizes here without ever logging in here. let request_key = RequestKey::new(); - let response = - ChallengeResponse::new(Challenge::new(Nonce::generate()), request_key.verifier()); + let response = ChallengeResponse::new(Challenge::new(Nonce::random()), request_key.verifier()); let signed = root.sign(response).await.unwrap(); let verified = signed.clone().verify_with_ssh_public_key(&root_pk).unwrap(); let mut credentials = Credentials::new(verified); diff --git a/tests/src/test_utils.rs b/tests/src/test_utils.rs index 448f305..a159447 100644 --- a/tests/src/test_utils.rs +++ b/tests/src/test_utils.rs @@ -145,7 +145,7 @@ pub fn test_pki(prefix: &'static str) -> (TempDir, Utf8PathBuf) { } pub async fn fake_identity(key: &mut EphemeralKey) -> Identity { - let nonce = Nonce::generate(); + let nonce = Nonce::random(); let challenge = Challenge::new(nonce.clone()); let response = ChallengeResponse::new(challenge, RequestKey::new().verifier()); let signed = key.sign(response).await.unwrap(); From b4b9cc9a11b1d6b6c8dd7b26ab8c652ddaf5f2dc Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 12:59:17 +0200 Subject: [PATCH 03/10] switch session IDs to the newtype --- client/src/cli.rs | 2 +- client/src/commands.rs | 16 +- common/src/borsh.rs | 2 +- common/src/jobs.rs | 77 +++------ server/src/manager.rs | 7 +- server/src/messages.rs | 12 +- server/src/server.rs | 2 +- server/src/state.rs | 22 ++- server/tests/distributed.rs | 14 +- .../output/session-allow-attach-request.bin | Bin 50 -> 63 bytes .../output/session-deny-attach-request.bin | Bin 49 -> 62 bytes server/tests/output/session-start-request.bin | Bin 35 -> 48 bytes server/tests/output/session-stop-request.bin | Bin 35 -> 48 bytes tests/src/integration_tests.rs | 6 +- tests/src/manager_tests.rs | 157 +++++++----------- 15 files changed, 119 insertions(+), 198 deletions(-) diff --git a/client/src/cli.rs b/client/src/cli.rs index 06a95d8..76aad00 100644 --- a/client/src/cli.rs +++ b/client/src/cli.rs @@ -96,7 +96,7 @@ impl CommandContext for Cli { fn session_stopped(&mut self, session_id: &SessionId) -> Result<(), CommandError> { let mut session_guard = self.session.lock().unwrap(); if let Some(session) = session_guard.as_ref() - && session.session_id() == session_id + && session.session_id() == *session_id { let _ = session_guard.take(); } diff --git a/client/src/commands.rs b/client/src/commands.rs index c823874..4dc1ae2 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -759,7 +759,7 @@ async fn session( .await? .into_inner(); if let Some(session_id) = session_id - && *session.session_id() != session_id + && session.session_id() != session_id { return Err(CommandError::MissingSession); } @@ -781,7 +781,7 @@ async fn session( let session = if let Some(session_id) = session_id { Session::new(session_id) } else { - Session::new(SessionId::new()) + Session::new(SessionId::random()) }; with_login(ctx, client, async || { client @@ -809,7 +809,7 @@ async fn session( with_login(ctx, client, async || { client .session_allow_attach() - .session_id(session_id.clone()) + .session_id(session_id) .key_id(key_id.clone()) .access(access) .send() @@ -827,7 +827,7 @@ async fn session( with_login(ctx, client, async || { client .session_deny_attach() - .session_id(session_id.clone()) + .session_id(session_id) .key_id(key_id.clone()) .send() .await @@ -843,11 +843,7 @@ async fn session( return Err(CommandError::MissingSession); }; with_login(ctx, client, async || { - client - .session_stop() - .session_id(session_id.clone()) - .send() - .await + client.session_stop().session_id(*session_id).send().await }) .await?; ctx.session_stopped(session_id)?; @@ -920,7 +916,7 @@ async fn job( { Ok(resp) => resp.into_inner(), Err(CommandError::NotFound) => { - let session = Session::new(SessionId::new()); + let session = Session::new(SessionId::random()); with_login(ctx, client, async || { client .session_start() diff --git a/common/src/borsh.rs b/common/src/borsh.rs index e1966a9..97e9133 100644 --- a/common/src/borsh.rs +++ b/common/src/borsh.rs @@ -109,7 +109,7 @@ mod test { let sneaky = borsh::to_vec(&"ALPHA-BRAVO".to_string()).unwrap(); assert!(borsh::from_slice::(&sneaky).is_err()); - let good = borsh::to_vec(&SessionId::new().first_job_id().to_string()).unwrap(); + let good = borsh::to_vec(&SessionId::random().first_job_id().to_string()).unwrap(); assert!(borsh::from_slice::(&good).is_ok()); } } diff --git a/common/src/jobs.rs b/common/src/jobs.rs index 78fbf42..3fcd830 100644 --- a/common/src/jobs.rs +++ b/common/src/jobs.rs @@ -27,9 +27,7 @@ use crate::borsh::{ borsh_de_datetime, borsh_de_hash, borsh_de_job_id, borsh_de_target, borsh_ser_datetime, borsh_ser_hash, borsh_ser_target, }; -use crate::codephrases::{ - InvalidCodephrase, WORD_SEPARATOR, decode_phrase, generate_id, id_phrase, -}; +use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, decode_phrase, id_phrase}; use crate::interactive::InteractiveJobError; use crate::keys::{KeyId, Signed, ToBeSigned, Verified}; use crate::targets::Target; @@ -110,37 +108,34 @@ impl slog::Value for JobId { } } -/// A globally unique identifier for a session. -#[derive( - BorshDeserialize, - BorshSerialize, - Clone, - Debug, - Deserialize, - Eq, - Hash, - JsonSchema, - Ord, - PartialEq, - PartialOrd, - Serialize, -)] -pub struct SessionId(String); +codephrase_newtype! { + /// A globally unique identifier for a session. + #[derive( + BorshDeserialize, + BorshSerialize, + Copy, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + Ord, + PartialEq, + PartialOrd, + Serialize, + )] + pub struct SessionId = Truncated; +} -#[allow(clippy::new_without_default)] impl SessionId { - pub fn new() -> Self { - Self(generate_id()) - } - pub fn first_job_id(&self) -> JobId { - U256::from_be_slice(hash(self.0.as_bytes()).as_bytes()).into() + U256::from_be_slice(hash(&self.0.to_be_bytes()).as_bytes()).into() } pub fn next_job_id(&self, last_job: &LastJob) -> JobId { U256::from_be_slice( match last_job { - LastJob::None => hash(&[b"None", self.0.as_bytes()].concat()), + LastJob::None => hash(&[b"None", self.0.to_be_bytes().as_slice()].concat()), LastJob::Some(job) => hash(&[b"Some", job.to_be_signed().as_slice()].concat()), LastJob::Burned(job_id) => hash(&[b"Burned", job_id.as_bytes()].concat()), } @@ -150,32 +145,6 @@ impl SessionId { } } -impl Deref for SessionId { - type Target = str; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl fmt::Display for SessionId { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl From<&Self> for SessionId { - fn from(other: &Self) -> Self { - other.to_owned() - } -} - -impl> From for SessionId { - fn from(s: S) -> Self { - Self(s.as_ref().to_string()) - } -} - #[derive(Clone, Debug, Default, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] pub enum LastJob { #[default] @@ -210,8 +179,8 @@ impl Session { } } - pub fn session_id(&self) -> &SessionId { - &self.session_id + pub fn session_id(&self) -> SessionId { + self.session_id } pub fn started_by(&self) -> Option<&KeyId> { diff --git a/server/src/manager.rs b/server/src/manager.rs index 70a0443..3fc0ad4 100644 --- a/server/src/manager.rs +++ b/server/src/manager.rs @@ -433,7 +433,7 @@ impl JobManager { move |state| { state .session() - .is_some_and(|s| *s.session_id() == session_id) + .is_some_and(|s| s.session_id() == session_id) } } @@ -493,11 +493,10 @@ impl JobManager { session_id: SessionId, wait: bool, ) -> Result<(), JobError> { - self.session_request(authn, SessionRequest::Start(session_id.clone())) + self.session_request(authn, SessionRequest::Start(session_id)) .await?; if wait { - self.wait_for(self.wait_for_session(session_id.clone())) - .await?; + self.wait_for(self.wait_for_session(session_id)).await?; } Ok(()) } diff --git a/server/src/messages.rs b/server/src/messages.rs index d84ad1e..8efac53 100644 --- a/server/src/messages.rs +++ b/server/src/messages.rs @@ -266,11 +266,15 @@ mod wire_format { assert_eq!(decoded, message, "wire format should round-trip"); } + fn sid(name: &str) -> SessionId { + name.parse().unwrap() + } + #[test] fn session_start_request() { let msg: VersionedMessage = Message::Request(Request::session( KeyId::from("zoo-zero".to_string()), - SessionRequest::Start(SessionId::from("abandon-ability")), + SessionRequest::Start(sid("abandon-ability")), )) .into(); assert_wire_format("session-start-request", msg); @@ -280,7 +284,7 @@ mod wire_format { fn session_stop_request() { let msg: VersionedMessage = Message::Request(Request::session( KeyId::from("zoo-zero".to_string()), - SessionRequest::Stop(SessionId::from("abandon-ability")), + SessionRequest::Stop(sid("abandon-ability")), )) .into(); assert_wire_format("session-stop-request", msg); @@ -291,7 +295,7 @@ mod wire_format { let msg: VersionedMessage = Message::Request(Request::session( KeyId::from("zoo-zero".to_string()), SessionRequest::AllowAttach( - SessionId::from("abandon-ability"), + sid("abandon-ability"), KeyId::from("able-about".to_string()), Access::ReadWrite, ), @@ -305,7 +309,7 @@ mod wire_format { let msg: VersionedMessage = Message::Request(Request::session( KeyId::from("zoo-zero".to_string()), SessionRequest::DenyAttach( - SessionId::from("abandon-ability"), + sid("abandon-ability"), KeyId::from("able-about".to_string()), ), )) diff --git a/server/src/server.rs b/server/src/server.rs index 621464a..76bbb56 100644 --- a/server/src/server.rs +++ b/server/src/server.rs @@ -183,7 +183,7 @@ impl SushApi for ApiServer { .await?; let WaitParam { wait } = query.into_inner(); let SessionIdParam { session_id } = params.into_inner(); - mgr.session_start(&authn, session_id.clone(), wait).await?; + mgr.session_start(&authn, session_id, wait).await?; Ok(HttpResponseUpdatedNoContent()) } diff --git a/server/src/state.rs b/server/src/state.rs index b3fec2e..79ccf66 100644 --- a/server/src/state.rs +++ b/server/src/state.rs @@ -173,7 +173,7 @@ struct SessionGuard<'a> { } impl<'a> SessionGuard<'a> { - pub fn session_id(&self) -> &SessionId { + pub fn session_id(&self) -> SessionId { self.inner.session_id() } @@ -434,6 +434,7 @@ impl State { self.revoked_keys.peek(key_id).is_some() } + #[allow(clippy::result_large_err)] fn update( &mut self, log: &Logger, @@ -474,7 +475,7 @@ impl State { // Re-announcement of active session; absorb and ignore. Active { frontier, session, .. - } if session.session_id() == session_id => { + } if session.session_id() == *session_id => { *frontier |= incoming_version.clone(); info!(log, "duplicate session start"; "session_id" => %session_id); } @@ -494,10 +495,7 @@ impl State { self.session = Active { frontier: self.session.frontier() | incoming_version.clone(), started: incoming_version.clone(), - session: Box::new(Session::started( - session_id.clone(), - actor.clone(), - )), + session: Box::new(Session::started(*session_id, actor.clone())), queued_jobs: QueuedJobs::new(), attach_grants: BTreeMap::new(), } @@ -515,9 +513,9 @@ impl State { .. } if incoming_version.partial_cmp(frontier).is_none() => { let error = Error::ConcurrentSessions { - own_session: session.session_id().clone(), + own_session: session.session_id(), own_version: started.clone(), - incoming_session: session_id.clone(), + incoming_session: *session_id, incoming_version: incoming_version.clone(), }; self.session = Inactive { @@ -553,7 +551,7 @@ impl State { if let Active { frontier, session, .. } = &self.session - && session.session_id() == session_id + && session.session_id() == *session_id { info!( log, "session stopped"; @@ -570,7 +568,7 @@ impl State { attach_grants, .. } = &mut self.session - && session.session_id() == session_id + && session.session_id() == *session_id { if session.started_by() == Some(actor) { attach_grants.insert(key_id.clone(), *access); @@ -592,7 +590,7 @@ impl State { attach_grants, .. } = &mut self.session - && session.session_id() == session_id + && session.session_id() == *session_id { if session.started_by() == Some(actor) { attach_grants.remove(key_id); @@ -610,7 +608,7 @@ impl State { } (actor, SessionRequest::Skip(session_id, job_id)) => { if let Some(mut session) = self.session.active_session() - && session.session_id() == session_id + && session.session_id() == *session_id { info!( log, "job skipped"; diff --git a/server/tests/distributed.rs b/server/tests/distributed.rs index bf119ee..9ee6584 100644 --- a/server/tests/distributed.rs +++ b/server/tests/distributed.rs @@ -117,16 +117,16 @@ async fn jobs_gossip_between_sleds() { // A session started on sled A becomes B's active session too. let authn_a = fake_identity(&mut root).await; let authn_b = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); + let session_id = SessionId::random(); + let session = Session::new(session_id); a.mgr - .session_start(&authn_a, session_id.clone(), true) + .session_start(&authn_a, session_id, true) .await .unwrap(); eventually("session gossips to B", 60, async || { b.mgr .session(&authn_b) - .is_some_and(|s| *s.session_id() == session_id) + .is_some_and(|s| s.session_id() == session_id) }) .await; @@ -161,15 +161,15 @@ async fn jobs_gossip_between_sleds() { .await; // A session started on B supersedes A's everywhere. - let successor = SessionId::new(); + let successor = SessionId::random(); b.mgr - .session_start(&authn_b, successor.clone(), true) + .session_start(&authn_b, successor, true) .await .unwrap(); eventually("supersession gossips to A", 60, async || { a.mgr .session(&authn_a) - .is_some_and(|s| *s.session_id() == successor) + .is_some_and(|s| s.session_id() == successor) }) .await; diff --git a/server/tests/output/session-allow-attach-request.bin b/server/tests/output/session-allow-attach-request.bin index ffb4e3d22da4930a3a0c866742dada0c92b8df55..4291cca01f23cdeac6ec0c3d7eef6b4344f7975f 100644 GIT binary patch literal 63 pcmZQzVB}z6V5rK^*R4t|%4Y_$@c~9Iplo7NPO5HVQhsR(BLK&u2RZ-% literal 50 zcmZQzVB}z6V5rK^*R4t|%4g;WauSmg^HTEjbQ6;@b23XRxxhj>sX%^yX$d0$CA$qa diff --git a/server/tests/output/session-deny-attach-request.bin b/server/tests/output/session-deny-attach-request.bin index 9f90f5cc37a736f62ed2fe4bd4d3d66be73521da..c76ca2ddc3172cb444c6b0333db60015b23071c1 100644 GIT binary patch literal 62 ocmZQzVB}z6V5rK^*R4t|%4Y$x@c~9Iplo7NPO5HVQhsR(0Kk_AIsgCw literal 49 ycmZQzVB}z6V5rK^*R4t|%4gvRauSmg^HTEjbQ6;@b23XRxxhj>sX%^yX$b%rrwuj$ diff --git a/server/tests/output/session-start-request.bin b/server/tests/output/session-start-request.bin index af0a9096cb1bdb0f8098d29dc6cf7290c1635a76..b981582489ebbc1b220cd7bff00aaddf404769b7 100644 GIT binary patch literal 48 acmZQzVB}z6V5rK^*R4t|$_Fy>0!9FQY6DOJ literal 35 ocmZQzVB}z6V5rK^*R4t|%4gsQauSmg^HTEjbQ6;@b23XR0f5&DOaK4? diff --git a/server/tests/output/session-stop-request.bin b/server/tests/output/session-stop-request.bin index 635a8a02ab6abdbdb06f9f145b0cd184929b8218..3f0868b06e3a2404fdd85e8e9068e4a7fa65249b 100644 GIT binary patch literal 48 bcmZQzVB}z6V5rK^*R4t|%4Y&2"; @@ -456,11 +448,9 @@ async fn cubby_targets() { .await .unwrap(); let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let target: Target = "14".parse().unwrap(); // With the map empty, a cubby-targeted job records no status here. @@ -559,11 +549,9 @@ async fn root_certs_from_files() { // A job signed by the configured root runs. let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let job = root.sign_job_request(&job_id, "true", false).await; mgr.job_start( @@ -634,11 +622,9 @@ async fn job_output_dir_moves() { .unwrap(); let baseboard_id = mgr.own_baseboard(); let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); // Record a job's output under the first base. let first = session.next_job_id(); @@ -728,8 +714,8 @@ async fn universe_swap() { let authn = fake_identity(&mut root).await; async fn run_job(mgr: &JobManager, root: &mut EphemeralKey, authn: &Identity) -> JobId { - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); + let session_id = SessionId::random(); + let session = Session::new(session_id); mgr.session_start(authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let job = root.sign_job_request(&job_id, "true", false).await; @@ -774,11 +760,9 @@ async fn shutdown() { let log = test_logger(function_name!()); let (mut mgr, mut root, _dir, shutdown) = manager_and_test_root(log).await; let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let command = "sleep 30"; let job_id = session.next_job_id(); @@ -887,8 +871,8 @@ async fn cert_chain() { ); // Start a job signed with the child. - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); mgr.session_start(&authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let job = child.sign_job_request(&job_id, "true", false).await; @@ -915,11 +899,9 @@ async fn attribution() { let log = test_logger(function_name!()); let (mgr, mut root, _dir, _shutdown) = manager_and_test_root(log).await; let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); assert_eq!( mgr.session(&authn).unwrap().started_by(), Some(&authn.key_id) @@ -1137,8 +1119,8 @@ async fn job_targets() { let log = test_logger(function_name!()); let (mgr, mut root, _dir, _shutdown) = manager_and_test_root(log).await; let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); mgr.session_start(&authn, session_id, true).await.unwrap(); let mut start = async |command: &str, target: &str| { @@ -1283,11 +1265,9 @@ async fn attach_grants() { EphemeralKey::new_root(KeyType::P256, ephemeral_test_subject(), validity).unwrap(); let guest = fake_identity(&mut guest_key).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); - mgr.session_start(&owner, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let session = Session::new(session_id); + mgr.session_start(&owner, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let job = root.sign_job_request(&job_id, "sleep 10", false).await; mgr.job_start( @@ -1314,13 +1294,8 @@ async fn attach_grants() { Err(JobError::AttachDenied) )); assert!(matches!( - mgr.session_allow_attach( - &guest, - session_id.clone(), - owner.key_id.clone(), - Access::ReadOnly - ) - .await, + mgr.session_allow_attach(&guest, session_id, owner.key_id.clone(), Access::ReadOnly) + .await, Err(JobError::NotSessionStarter) )); @@ -1343,25 +1318,15 @@ async fn attach_grants() { .await .expect("grant never took effect") }; - mgr.session_allow_attach( - &owner, - session_id.clone(), - guest.key_id.clone(), - Access::ReadOnly, - ) - .await - .unwrap(); + mgr.session_allow_attach(&owner, session_id, guest.key_id.clone(), Access::ReadOnly) + .await + .unwrap(); granted(guest.clone(), Some(Access::ReadOnly)).await; - mgr.session_allow_attach( - &owner, - session_id.clone(), - guest.key_id.clone(), - Access::ReadWrite, - ) - .await - .unwrap(); + mgr.session_allow_attach(&owner, session_id, guest.key_id.clone(), Access::ReadWrite) + .await + .unwrap(); granted(guest.clone(), Some(Access::ReadWrite)).await; - mgr.session_deny_attach(&owner, session_id.clone(), guest.key_id.clone()) + mgr.session_deny_attach(&owner, session_id, guest.key_id.clone()) .await .unwrap(); granted(guest.clone(), None).await; @@ -1375,11 +1340,7 @@ async fn attach_grants() { peer.send( Message::Request(Request::session( guest.key_id.clone(), - SessionRequest::AllowAttach( - session_id.clone(), - guest.key_id.clone(), - Access::ReadWrite, - ), + SessionRequest::AllowAttach(session_id, guest.key_id.clone(), Access::ReadWrite), )) .into(), ); @@ -1650,11 +1611,9 @@ async fn too_much_cpu() { let (mgr, mut root, _dir, _shutdown) = manager_and_test_root(log).await; let baseboard_id = mgr.own_baseboard(); let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let command = "openssl speed sha1"; let job = root.sign_job_request(&job_id, command, false).await; @@ -1731,11 +1690,9 @@ async fn too_much_output() { let log = test_logger(function_name!()); let (mgr, mut root, _dir, _shutdown) = manager_and_test_root(log).await; let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let mut session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let mut session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); let command = "yes"; @@ -1823,11 +1780,9 @@ async fn output_ranges() { let (mgr, mut root, _dir, _shutdown) = manager_and_test_root(log).await; let baseboard_id = mgr.own_baseboard(); let authn = fake_identity(&mut root).await; - let session_id = SessionId::new(); - let session = Session::new(session_id.clone()); - mgr.session_start(&authn, session_id.clone(), true) - .await - .unwrap(); + let session_id = SessionId::random(); + let session = Session::new(session_id); + mgr.session_start(&authn, session_id, true).await.unwrap(); let job_id = session.next_job_id(); // Read some random bytes. From 006ccf97d4c0dc83d0d1e58162681f3d343e4681 Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 15:13:57 +0200 Subject: [PATCH 04/10] switch job IDs to the newtype --- client/src/commands.rs | 6 +- common/src/borsh.rs | 27 ------ common/src/jobs.rs | 112 +++++++--------------- server/src/executor.rs | 10 +- server/src/history.rs | 2 +- server/src/messages.rs | 1 + server/src/output.rs | 2 +- server/src/state.rs | 25 +++-- server/tests/output/job-start-request.bin | Bin 170 -> 135 bytes tests/src/integration_tests.rs | 22 ++--- tests/src/manager_tests.rs | 6 +- 11 files changed, 70 insertions(+), 143 deletions(-) diff --git a/client/src/commands.rs b/client/src/commands.rs index 4dc1ae2..aa26def 100644 --- a/client/src/commands.rs +++ b/client/src/commands.rs @@ -1132,7 +1132,7 @@ async fn job_start( match with_login_via(ctx, client, Some(&target), async || { client .job_output() - .job_id(&job_id) + .job_id(job_id) .target(target.to_string()) .stream(stream) .send() @@ -1238,7 +1238,7 @@ async fn job_output( // Fetch job status for output length and hash. let status = job_status_try_from_json_map( with_login_via(ctx, client, Some(target), async || { - client.job_status().job_id(&job_id).send().await + client.job_status().job_id(job_id).send().await }) .await? .into_inner(), @@ -1344,7 +1344,7 @@ async fn job_output( async || { client .job_output() - .job_id(&job_id) + .job_id(job_id) .target(target.to_string()) .stream(stream) .send() diff --git a/common/src/borsh.rs b/common/src/borsh.rs index 97e9133..9925ad0 100644 --- a/common/src/borsh.rs +++ b/common/src/borsh.rs @@ -12,7 +12,6 @@ use sled_hardware_types::BaseboardId; use x509_cert::Certificate; use x509_cert::der::{Decode as _, Encode as _}; -use crate::jobs::JobId; use crate::targets::Target; /// Borsh-encode a [`blake3::Hash`] as its 32 raw bytes. @@ -87,29 +86,3 @@ pub fn borsh_de_cert(reader: &mut R) -> Result { ) }) } - -pub fn borsh_de_job_id(reader: &mut R) -> Result { - Ok(String::deserialize_reader(reader)? - .parse::() - .map_err(|_| Error::new(ErrorKind::InvalidData, "failed to deserialize job ID"))? - .to_string()) -} - -#[cfg(test)] -mod test { - use crate::jobs::SessionId; - - use super::*; - - #[test] - fn borsh_de_job_id() { - let hostile = borsh::to_vec(&"../../etc/passwd".to_string()).unwrap(); - assert!(borsh::from_slice::(&hostile).is_err()); - - let sneaky = borsh::to_vec(&"ALPHA-BRAVO".to_string()).unwrap(); - assert!(borsh::from_slice::(&sneaky).is_err()); - - let good = borsh::to_vec(&SessionId::random().first_job_id().to_string()).unwrap(); - assert!(borsh::from_slice::(&good).is_ok()); - } -} diff --git a/common/src/jobs.rs b/common/src/jobs.rs index 3fcd830..78ff42c 100644 --- a/common/src/jobs.rs +++ b/common/src/jobs.rs @@ -14,7 +14,6 @@ use blake3::{Hash, Hasher, hash}; use borsh::{BorshDeserialize, BorshSerialize}; use bytesize::GB; use chrono::{DateTime, TimeDelta, Utc}; -use crypto_bigint::U256; use rlimit::Resource; use schemars::schema::{Schema, SchemaObject}; use schemars::{JsonSchema, SchemaGenerator}; @@ -24,77 +23,30 @@ use sled_hardware_types::{BaseboardId, BaseboardIdParseError}; use thiserror::Error; use crate::borsh::{ - borsh_de_datetime, borsh_de_hash, borsh_de_job_id, borsh_de_target, borsh_ser_datetime, - borsh_ser_hash, borsh_ser_target, + borsh_de_datetime, borsh_de_hash, borsh_de_target, borsh_ser_datetime, borsh_ser_hash, + borsh_ser_target, }; -use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, decode_phrase, id_phrase}; use crate::interactive::InteractiveJobError; use crate::keys::{KeyId, Signed, ToBeSigned, Verified}; use crate::targets::Target; -/// A globally unique identifier for a job within a session. -#[derive( - BorshDeserialize, - BorshSerialize, - Clone, - Debug, - Deserialize, - Eq, - Hash, - JsonSchema, - Ord, - PartialEq, - PartialOrd, - Serialize, -)] -#[serde(try_from = "String")] -pub struct JobId(#[borsh(deserialize_with = "borsh_de_job_id")] String); - -impl Deref for JobId { - type Target = str; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl fmt::Display for JobId { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl From<&Self> for JobId { - fn from(other: &Self) -> Self { - other.to_owned() - } -} - -impl From for JobId { - fn from(value: U256) -> Self { - Self(id_phrase(value).join(WORD_SEPARATOR)) - } -} - -impl FromStr for JobId { - type Err = InvalidCodephrase; - - fn from_str(s: &str) -> Result { - let p = id_phrase(decode_phrase(s)?).join(WORD_SEPARATOR); - if p == *s { - Ok(Self(p)) - } else { - Err(InvalidCodephrase) - } - } -} - -impl TryFrom for JobId { - type Error = InvalidCodephrase; - - fn try_from(s: String) -> Result { - JobId::from_str(&s) - } +codephrase_newtype! { + /// A globally unique identifier for a job within a session. + #[derive( + BorshDeserialize, + BorshSerialize, + Copy, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + Ord, + PartialEq, + PartialOrd, + Serialize, + )] + pub struct JobId = Truncated; } impl slog::Value for JobId { @@ -104,7 +56,13 @@ impl slog::Value for JobId { key: slog::Key, serializer: &mut dyn slog::Serializer, ) -> slog::Result { - serializer.emit_str(key, self) + serializer.emit_str(key, &self.0.to_string()) + } +} + +impl From<&JobId> for JobId { + fn from(value: &JobId) -> Self { + *value } } @@ -129,19 +87,15 @@ codephrase_newtype! { impl SessionId { pub fn first_job_id(&self) -> JobId { - U256::from_be_slice(hash(&self.0.to_be_bytes()).as_bytes()).into() + JobId::from_hash(hash(&self.0.to_be_bytes())) } pub fn next_job_id(&self, last_job: &LastJob) -> JobId { - U256::from_be_slice( - match last_job { - LastJob::None => hash(&[b"None", self.0.to_be_bytes().as_slice()].concat()), - LastJob::Some(job) => hash(&[b"Some", job.to_be_signed().as_slice()].concat()), - LastJob::Burned(job_id) => hash(&[b"Burned", job_id.as_bytes()].concat()), - } - .as_bytes(), - ) - .into() + JobId::from_hash(match last_job { + LastJob::None => hash(&[b"None", self.0.to_be_bytes().as_slice()].concat()), + LastJob::Some(job) => hash(&[b"Some", job.to_be_signed().as_slice()].concat()), + LastJob::Burned(job_id) => hash(&[b"Burned", job_id.to_be_bytes().as_slice()].concat()), + }) } } @@ -290,7 +244,7 @@ impl ToBeSigned for JobStartRequest { target, } = self; hash_with_len(Self::TYPE_NAME); - hash_with_len(job_id.as_bytes()); + hash_with_len(&job_id.to_be_bytes()); hash_with_len(command.as_bytes()); hash_with_len(if *interactive { &[1] } else { &[0] }); hash_with_len(target.to_string().as_bytes()); diff --git a/server/src/executor.rs b/server/src/executor.rs index 603d119..ab96f27 100644 --- a/server/src/executor.rs +++ b/server/src/executor.rs @@ -130,9 +130,9 @@ impl Executor { }; let stop = self.shutdown.child_token(); - self.stop.insert(job_id.clone(), stop.clone()); + self.stop.insert(job_id, stop.clone()); spawn(job_spawn( - self.log.new(o!("job_id" => job_id.clone())), + self.log.new(o!("job_id" => job_id)), events, self.output_dir.clone(), verified_request, @@ -372,7 +372,7 @@ async fn job_spawn( // Announce the birth of our new job! let _ = events - .send(Event::Job(JobEvent::Start(job_id.clone(), Utc::now()))) + .send(Event::Job(JobEvent::Start(job_id, Utc::now()))) .await; // Wait for the job to die and send an event when it does. @@ -381,7 +381,7 @@ async fn job_spawn( Ok((result, state)) => { let _ = events .send(Event::Job(JobEvent::Stop( - job_id.clone(), + job_id, Utc::now(), result, state, @@ -391,7 +391,7 @@ async fn job_spawn( Err(error) => { let _ = events .send(Event::Job(JobEvent::Error( - job_id.clone(), + job_id, Utc::now(), ProcessError::Join(error.to_string()), ))) diff --git a/server/src/history.rs b/server/src/history.rs index eead89f..8a4e08b 100644 --- a/server/src/history.rs +++ b/server/src/history.rs @@ -104,7 +104,7 @@ impl JobHistory { if let Some(queued) = queued { ineligible.extend(queued.keys().cloned()); } - ineligible.extend(running.iter().map(|((id, _), _)| id.clone())); + ineligible.extend(running.iter().map(|((id, _), _)| *id)); // Cache causal jobs. let causal = self diff --git a/server/src/messages.rs b/server/src/messages.rs index 8efac53..ff770e2 100644 --- a/server/src/messages.rs +++ b/server/src/messages.rs @@ -44,6 +44,7 @@ pub mod v0 { use super::*; #[derive(BorshDeserialize, BorshSerialize, Clone, Debug, Eq, PartialEq)] + #[allow(clippy::large_enum_variant)] pub enum Message { Request(Request), Event( diff --git a/server/src/output.rs b/server/src/output.rs index 58283f2..6fbf1e5 100644 --- a/server/src/output.rs +++ b/server/src/output.rs @@ -195,7 +195,7 @@ impl JobOutputDir { Ok(len) => Ok(len), Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(0), Err(err) => Err(ExecutionError::io( - job_id.clone(), + *job_id, format!("getting length of {}", path.display()), err, )), diff --git a/server/src/state.rs b/server/src/state.rs index 79ccf66..27da3ed 100644 --- a/server/src/state.rs +++ b/server/src/state.rs @@ -182,7 +182,7 @@ impl<'a> SessionGuard<'a> { } pub fn skip_job(&mut self, job_id: &JobId) { - self.inner.skip_job(job_id.clone()) + self.inner.skip_job(*job_id) } pub fn next_queued_job(&mut self) -> Option<(SignedJob, JobStartParams)> { @@ -201,7 +201,7 @@ impl<'a> SessionGuard<'a> { params: JobStartParams, actor: &KeyId, ) { - let job_id = job.job_id().clone(); + let job_id = *job.job_id(); let targeted = job.payload().target().includes(own_baseboard, cubbies); if history.contains(&job_id) { // Note but otherwise ignore the duplicate job. @@ -215,13 +215,13 @@ impl<'a> SessionGuard<'a> { // Insert the job into our queue. Every job joins the // queue to keep the causal chain whole, but only jobs // targeting this sled record a local status. - self.queued_jobs.insert(job_id.clone(), (job, params)); + self.queued_jobs.insert(job_id, (job, params)); if targeted { history.set_job_status( &job_id, own_baseboard, JobStatus::Queued { - job_id: job_id.clone(), + job_id, time_queued: Utc::now(), actor: actor.clone(), }, @@ -248,7 +248,7 @@ impl<'a> SessionGuard<'a> { None, |old_status| match old_status { None | Some(JobStatus::Queued { .. }) => Some(JobStatus::Cancelled { - job_id: job_id.clone(), + job_id: *job_id, time_cancelled: Utc::now(), actor: actor.clone(), }), @@ -285,7 +285,7 @@ impl<'a> SessionGuard<'a> { .unwrap_or(true) { executor.job_start(certs, request.clone(), params, tx_attachment); - attachments.insert(job_id.clone(), rx_attachment); + attachments.insert(job_id, rx_attachment); } self.job_started(request); } @@ -745,13 +745,12 @@ impl State { Event::Job(job_event) => match job_event { JobEvent::Start(job_id, when) => { info!(log, "job started"; "job_id" => %job_id, "when" => %when); - self.running - .insert((job_id.clone(), baseboard_id.clone()), *when); + self.running.insert((*job_id, baseboard_id.clone()), *when); self.history.set_job_status( job_id, baseboard_id, JobStatus::Started { - job_id: job_id.clone(), + job_id: *job_id, time_started: *when, }, Some(incoming_version.rank()), @@ -764,7 +763,7 @@ impl State { if baseboard_id == &self.own_baseboard { self.attachments.remove(job_id); } - self.running.remove(&(job_id.clone(), baseboard_id.clone())); + self.running.remove(&(*job_id, baseboard_id.clone())); self.history.transition_job_status( job_id, baseboard_id, @@ -772,7 +771,7 @@ impl State { |old_status| match old_status { Some(JobStatus::Started { time_started, .. }) => { Some(JobStatus::Stopped { - job_id: job_id.clone(), + job_id: *job_id, time_started: *time_started, time_stopped: *when, result: result.clone(), @@ -793,12 +792,12 @@ impl State { if baseboard_id == &self.own_baseboard { self.attachments.remove(job_id); } - self.running.remove(&(job_id.clone(), baseboard_id.clone())); + self.running.remove(&(*job_id, baseboard_id.clone())); self.history.set_job_status( job_id, baseboard_id, JobStatus::Error { - job_id: job_id.clone(), + job_id: *job_id, time_error: *when, error: error.clone(), }, diff --git a/server/tests/output/job-start-request.bin b/server/tests/output/job-start-request.bin index a6485051bcbd630779692b756e51a96b18f0b77b..ccaecd4d2209bdc74c1096ed23cecd6b9958914e 100644 GIT binary patch delta 56 icmZ3**v@Fcz`(@8z`#(IpRZe$T9glD;suNogTn#U76tDB literal 170 zcmZQzVB%n4V5rK^*R4t|%4e_#auSmg^HTEjbV;W(b23XRxqwEbCTHX;WTfWghGxk2up_Kt1~P%x14#x30X{Irzz}fl4kL*9ggb-@Dg*#p_bWdD diff --git a/tests/src/integration_tests.rs b/tests/src/integration_tests.rs index 2ab7a79..f11df1c 100644 --- a/tests/src/integration_tests.rs +++ b/tests/src/integration_tests.rs @@ -112,7 +112,7 @@ async fn client_server() { } = JobLimits::default(); client .job_start() - .job_id(&job_id) + .job_id(job_id) .max_cpu(max_cpu) .max_mem(max_mem) .max_fsize(max_fsize) @@ -125,7 +125,7 @@ async fn client_server() { // Check the job output. let mut output = client .job_output() - .job_id(&job_id) + .job_id(job_id) .stream(JobOutputStream::Stdout) .target("*") .send() @@ -209,7 +209,7 @@ async fn client_proxy_server() { } = JobLimits::default(); client .job_start() - .job_id(&job_id) + .job_id(job_id) .max_cpu(max_cpu) .max_mem(max_mem) .max_fsize(max_fsize) @@ -222,7 +222,7 @@ async fn client_proxy_server() { .expect("can't start job"); let socket = client .job_attach() - .job_id(&job_id) + .job_id(job_id) .target(test_baseboard_id().to_string()) .send() .await @@ -244,7 +244,7 @@ async fn client_proxy_server() { // A target the proxy cannot route is refused at the proxy. let Err(unrouted) = client .job_output() - .job_id(&job_id) + .job_id(job_id) .stream(JobOutputStream::Stdout) .target("913-0000019:BRM99999999") .send() @@ -552,7 +552,7 @@ async fn interactive_job() { } = JobLimits::default(); client .job_start() - .job_id(&job_id) + .job_id(job_id) .max_cpu(max_cpu) .max_mem(max_mem) .max_fsize(max_fsize) @@ -567,7 +567,7 @@ async fn interactive_job() { // Attach to the job. let socket1 = client .job_attach() - .job_id(&job_id) + .job_id(job_id) .target(test_baseboard_id().to_string()) .send() .await @@ -596,7 +596,7 @@ async fn interactive_job() { // and playback. let socket2 = client .job_attach() - .job_id(&job_id) + .job_id(job_id) .target(test_baseboard_id().to_string()) .send() .await @@ -637,7 +637,7 @@ async fn interactive_job() { let guest_attach = async || { guest_client .job_attach() - .job_id(&job_id) + .job_id(job_id) .target(test_baseboard_id().to_string()) .send() .await @@ -757,7 +757,7 @@ async fn interactive_job() { // Stop the job. client .job_stop() - .job_id(&job_id) + .job_id(job_id) .wait(JobWait::Stop) .send() .await @@ -767,7 +767,7 @@ async fn interactive_job() { // from the read-only guest. let mut output = client .job_output() - .job_id(&job_id) + .job_id(job_id) .stream(JobOutputStream::Stdout) .target(test_baseboard_id().to_string()) .send() diff --git a/tests/src/manager_tests.rs b/tests/src/manager_tests.rs index 2f0b052..743a084 100644 --- a/tests/src/manager_tests.rs +++ b/tests/src/manager_tests.rs @@ -251,8 +251,8 @@ async fn job_stop() { )); // Skip the cancelled job. - session.skip_job(job_id.clone()); - mgr.session_skip_job(&authn, session_id, job_id.clone()) + session.skip_job(job_id); + mgr.session_skip_job(&authn, session_id, job_id) .await .expect("should be able to skip cancelled job"); @@ -1125,7 +1125,7 @@ async fn job_targets() { let mut start = async |command: &str, target: &str| { let job_id = session.next_job_id(); - let request = JobStartRequest::new(job_id.clone(), command, false, target.parse().unwrap()); + let request = JobStartRequest::new(job_id, command, false, target.parse().unwrap()); let job = root .sign(request) .await From 32040567217a2b1e32bcd801fb9daba51db7cadd Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 17:18:06 +0200 Subject: [PATCH 05/10] switch key IDs to the newtype --- client/src/identity.rs | 5 +- common/src/authn.rs | 20 +++--- common/src/keys.rs | 68 ++++++------------ server/src/messages.rs | 21 +++--- .../tests/output/identity-login-request.bin | Bin 241 -> 281 bytes server/tests/output/job-start-request.bin | Bin 135 -> 175 bytes .../output/session-allow-attach-request.bin | Bin 63 -> 101 bytes .../output/session-deny-attach-request.bin | Bin 62 -> 100 bytes server/tests/output/session-start-request.bin | Bin 48 -> 68 bytes server/tests/output/session-stop-request.bin | Bin 48 -> 68 bytes tests/src/manager_tests.rs | 4 +- 11 files changed, 49 insertions(+), 69 deletions(-) diff --git a/client/src/identity.rs b/client/src/identity.rs index 3763126..bb89387 100644 --- a/client/src/identity.rs +++ b/client/src/identity.rs @@ -171,7 +171,7 @@ impl IdentityError { #[cfg(test)] mod test { - use sush_common::codephrases::{PHRASE_WORDS_ID, WORD_SEPARATOR, generate_id}; + use sush_common::codephrases::generate_id; use tempfile::TempDir; use tokio::process::Command; @@ -214,9 +214,6 @@ mod test { let mut agent = SshAgentConnection::connect(&sock).await.unwrap(); for key in agent.list_identities().await.unwrap() { - let key_id = key.key_id().unwrap(); - assert_eq!(key_id.split(WORD_SEPARATOR).count(), PHRASE_WORDS_ID); - let nonce = generate_id(); let signature = agent.sign_with(&key, nonce.as_bytes()).await.unwrap(); key.verify(nonce.as_bytes(), &signature).unwrap(); diff --git a/common/src/authn.rs b/common/src/authn.rs index 6a1be55..609bc79 100644 --- a/common/src/authn.rs +++ b/common/src/authn.rs @@ -268,7 +268,7 @@ impl BoundRequest { seq, } = self; hash(Self::TYPE_NAME); - hash(key_id.as_bytes()); + hash(&key_id.to_be_bytes()); hash(&nonce.to_be_bytes()); hash(method.as_bytes()); hash(target.as_bytes()); @@ -311,7 +311,7 @@ impl FromStr for BoundCredentials { fn from_str(s: &str) -> Result { let mut params = AuthnParams::from_str(s)?; let creds = Self { - key_id: params.get("key-id")?, + key_id: params.parse("key-id")?, nonce: params.parse("nonce")?, seq: params.parse("seq")?, signature: EncodedSignature { @@ -523,7 +523,7 @@ impl FromStr for Credentials { let creds = Self { nonce: params.parse("nonce")?, cnonce: params.parse("cnonce")?, - key_id: params.get("key-id")?, + key_id: params.parse("key-id")?, epk: params.parse("epk")?, signature: EncodedSignature { r: params.get("r")?, @@ -730,7 +730,11 @@ mod test { let key = RequestKey::new(); let verifier = key.verifier(); let request = BoundRequest::new("get", "/jobs?limit=1", 7); - let creds = key.bind(KeyId::from("baz".to_string()), Nonce::random(), &request); + let creds = key.bind( + KeyId::from_str("abandon").unwrap(), + Nonce::random(), + &request, + ); assert_eq!( creds.to_string().parse::().unwrap(), creds @@ -759,7 +763,7 @@ mod test { // The identity is part of the signed material: credentials // re-labeled with another key ID or nonce do not verify. let relabeled = BoundCredentials { - key_id: KeyId::from("qux".to_string()), + key_id: KeyId::from_str("ability").unwrap(), ..creds.clone() }; assert!(verifier.verify(&request, &relabeled).is_err()); @@ -835,21 +839,21 @@ mod test { let epk = RequestKey::new().verifier(); assert!(matches!( Credentials::from_str( - "Sush nonce=abandon,cnonce=ability,key-id=baz,epk=plugh,r=r,s=s,flags=0,counter=0" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk=plugh,r=r,s=s,flags=0,counter=0" ) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=abandon,cnonce=ability,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=foo" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=r,s=s,flags=0,counter=foo" )) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=abandon,cnonce=ability,key-id=baz,epk={epk},r=r,s=s,flags=0,counter=0,foo=bar" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=r,s=s,flags=0,counter=0,foo=bar" )) .unwrap_err(), AuthnError::TooManyParams diff --git a/common/src/keys.rs b/common/src/keys.rs index 6cd2b65..5153b8d 100644 --- a/common/src/keys.rs +++ b/common/src/keys.rs @@ -29,7 +29,6 @@ use rand_core::OsRng; use schemars::schema::Schema; use schemars::{JsonSchema, SchemaGenerator}; use serde::{Deserialize, Serialize}; -use sha2::{Digest as _, Sha256}; use signature::Verifier; use ssh_key::{ Algorithm as SshAlgorithm, EcdsaCurve, Error as SshKeyError, Mpint, Signature as SshSignature, @@ -46,44 +45,25 @@ use x509_cert::spki::{AlgorithmIdentifierOwned, SubjectPublicKeyInfo}; use x509_cert::time::Validity; use x509_cert::{Certificate, TbsCertificate, Version}; -use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase, id_phrase}; - -/// SHA-256 of a certificate subject or an identity public key, -/// encoded as a pseudorandom code phrase for storage & transport. -#[derive( - BorshDeserialize, - BorshSerialize, - Clone, - Debug, - Deserialize, - Eq, - Hash, - JsonSchema, - Ord, - PartialEq, - PartialOrd, - Serialize, -)] -pub struct KeyId(String); - -impl Deref for KeyId { - type Target = str; - - fn deref(&self) -> &Self::Target { - &self.0 - } -} - -impl fmt::Display for KeyId { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}", self.0) - } -} - -impl From for KeyId { - fn from(s: String) -> Self { - Self(s) - } +use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase}; + +codephrase_newtype! { + /// SHA-256 of a certificate subject or an identity public key, + /// encoded as a pseudorandom code phrase for storage & transport. + #[derive( + BorshDeserialize, + BorshSerialize, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + Ord, + PartialEq, + PartialOrd, + Serialize, + )] + pub struct KeyId = Truncated; } impl From<&Self> for KeyId { @@ -97,9 +77,8 @@ impl TryFrom<&Certificate> for KeyId { type Error = KeyError; fn try_from(cert: &Certificate) -> Result { - let hash = Sha256::digest(cert.tbs_certificate.subject_public_key_info.to_der()?); - let phrase = id_phrase(U256::from_be_slice(hash.as_slice())); - Ok(KeyId(phrase.join(WORD_SEPARATOR))) + let hash = blake3::hash(&cert.tbs_certificate.subject_public_key_info.to_der()?); + Ok(KeyId::from_hash(hash)) } } @@ -107,9 +86,8 @@ impl TryFrom<&ssh_key::PublicKey> for KeyId { type Error = KeyError; fn try_from(public_key: &ssh_key::PublicKey) -> Result { - let hash = Sha256::digest(public_key.to_bytes()?); - let phrase = id_phrase(U256::from_be_slice(hash.as_slice())); - Ok(KeyId(phrase.join(WORD_SEPARATOR))) + let hash = blake3::hash(&public_key.to_bytes()?); + Ok(KeyId::from_hash(hash)) } } diff --git a/server/src/messages.rs b/server/src/messages.rs index ff770e2..3aa7cae 100644 --- a/server/src/messages.rs +++ b/server/src/messages.rs @@ -159,6 +159,7 @@ pub mod v0 { } #[derive(BorshDeserialize, BorshSerialize, Clone, Debug, Eq, PartialEq)] + #[allow(clippy::large_enum_variant)] pub enum JobRequest { Start(SignedJob, JobStartParams), Stop(JobId), @@ -274,7 +275,7 @@ mod wire_format { #[test] fn session_start_request() { let msg: VersionedMessage = Message::Request(Request::session( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), SessionRequest::Start(sid("abandon-ability")), )) .into(); @@ -284,7 +285,7 @@ mod wire_format { #[test] fn session_stop_request() { let msg: VersionedMessage = Message::Request(Request::session( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), SessionRequest::Stop(sid("abandon-ability")), )) .into(); @@ -294,10 +295,10 @@ mod wire_format { #[test] fn session_allow_attach_request() { let msg: VersionedMessage = Message::Request(Request::session( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), SessionRequest::AllowAttach( sid("abandon-ability"), - KeyId::from("able-about".to_string()), + KeyId::from_str("able-about").unwrap(), Access::ReadWrite, ), )) @@ -308,10 +309,10 @@ mod wire_format { #[test] fn session_deny_attach_request() { let msg: VersionedMessage = Message::Request(Request::session( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), SessionRequest::DenyAttach( sid("abandon-ability"), - KeyId::from("able-about".to_string()), + KeyId::from_str("able-about").unwrap(), ), )) .into(); @@ -333,7 +334,7 @@ mod wire_format { ); let signed = Signed::new( request, - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), EncodedSignature { r: "abandon".to_string(), s: "zoo".to_string(), @@ -342,7 +343,7 @@ mod wire_format { }, ); let msg: VersionedMessage = Message::Request(Request::job( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), JobRequest::Start(signed, JobStartParams::default()), )) .into(); @@ -375,7 +376,7 @@ mod wire_format { let signed = Signed::new( response, - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), EncodedSignature { r: "abandon".to_string(), s: "zoo".to_string(), @@ -384,7 +385,7 @@ mod wire_format { }, ); let msg: VersionedMessage = Message::Request(Request::identity( - KeyId::from("zoo-zero".to_string()), + KeyId::from_str("zoo-zero").unwrap(), IdentityRequest::Login(public_key, signed), )) .into(); diff --git a/server/tests/output/identity-login-request.bin b/server/tests/output/identity-login-request.bin index ae710271e52dfa00111a04a12650eeff9b367d8d..af1475a92b41a0ec7a21a531fbd170f0eb33e6b1 100644 GIT binary patch delta 48 zcmey!IFpH)fq{8qp)9lg|GyLY_XAl{Ko&a#14Cj`VqQvq9#Ed4DnB1cGB5xDf%Fae delta 59 ycmbQq^pTODfq|KWfq|haKVP>hwJ3k0@_uDxK08n@F)1-GB|ncDq6|nfFaQ9%jSd9> diff --git a/server/tests/output/job-start-request.bin b/server/tests/output/job-start-request.bin index ccaecd4d2209bdc74c1096ed23cecd6b9958914e..76326d2793b4d58cdd3fd68f6de2732bf4e95068 100644 GIT binary patch delta 31 gcmZo?T+hhNz`!(7P?p*L|KEv%auao>VVn?S0EiR`=>Px# delta 45 pcmZ3_*v`n$z`(@8z`#(IpRZe$T9iLAQDR~N$3$mI9u(0UV*vNP3@`uy diff --git a/server/tests/output/session-allow-attach-request.bin b/server/tests/output/session-allow-attach-request.bin index 4291cca01f23cdeac6ec0c3d7eef6b4344f7975f..9ce5e5adc332300e2a7300f22af052dd94b2b5aa 100644 GIT binary patch literal 101 bcmZQzU}V4t?En8|#wQPy!*2kC05c;1nAHMR literal 63 pcmZQzVB}z6V5rK^*R4t|%4Y_$@c~9Iplo7NPO5HVQhsR(BLK&u2RZ-% diff --git a/server/tests/output/session-deny-attach-request.bin b/server/tests/output/session-deny-attach-request.bin index c76ca2ddc3172cb444c6b0333db60015b23071c1..0eacba1b005a68342015be89179d1215390543eb 100644 GIT binary patch literal 100 acmZQzU}V4t?En8|!6y%t!*2kC05brV!2(qP literal 62 ocmZQzVB}z6V5rK^*R4t|%4Y$x@c~9Iplo7NPO5HVQhsR(0Kk_AIsgCw diff --git a/server/tests/output/session-start-request.bin b/server/tests/output/session-start-request.bin index b981582489ebbc1b220cd7bff00aaddf404769b7..cd0df6996ed5a9fe9c340a7b370674cdb542916b 100644 GIT binary patch literal 68 VcmZQzU}V4t?EnA8Ck>Kg1OQFi0zLo$ literal 48 acmZQzVB}z6V5rK^*R4t|$_Fy>0!9FQY6DOJ diff --git a/server/tests/output/session-stop-request.bin b/server/tests/output/session-stop-request.bin index 3f0868b06e3a2404fdd85e8e9068e4a7fa65249b..469491adab5244c316d18154fa603b52e5356728 100644 GIT binary patch literal 68 VcmZQzU}V4t?EnA8uYiGp5dcm50zUu% literal 48 bcmZQzVB}z6V5rK^*R4t|%4Y Date: Tue, 11 Aug 2026 17:44:40 +0200 Subject: [PATCH 06/10] switch encoded signatures to the newtype --- common/src/authn.rs | 29 +++--- common/src/keys.rs | 83 ++++++++++++------ server/src/messages.rs | 8 +- .../tests/output/identity-login-request.bin | Bin 281 -> 327 bytes server/tests/output/job-start-request.bin | Bin 175 -> 221 bytes 5 files changed, 74 insertions(+), 46 deletions(-) diff --git a/common/src/authn.rs b/common/src/authn.rs index 609bc79..89a84f1 100644 --- a/common/src/authn.rs +++ b/common/src/authn.rs @@ -43,7 +43,9 @@ use thiserror::Error; use crate::codephrase_newtype; use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase}; -use crate::keys::{EncodedSignature, KeyError, KeyId, Signed, SshPublicKey, ToBeSigned, Verified}; +use crate::keys::{ + EccR, EccS, EncodedSignature, KeyError, KeyId, Signed, SshPublicKey, ToBeSigned, Verified, +}; /// The name of our custom HTTP authentication scheme. pub const AUTHN_SCHEME: &str = "Sush"; @@ -139,14 +141,13 @@ impl RequestKey { /// Sign `request`, yielding the credentials that authorize it. pub fn bind(&self, key_id: KeyId, nonce: Nonce, request: &BoundRequest) -> BoundCredentials { let signature = self.0.sign(&request.to_be_signed(&key_id, &nonce)); - let phrase = |bytes: &[u8]| codephrase(U256::from_be_slice(bytes)).join(WORD_SEPARATOR); BoundCredentials { key_id, nonce, seq: request.seq, signature: EncodedSignature { - r: phrase(&signature.r_bytes()[..]), - s: phrase(&signature.s_bytes()[..]), + r: EccR::from_be_bytes(*signature.r_bytes()), + s: EccS::from_be_bytes(*signature.s_bytes()), flags: 0, counter: 0, }, @@ -172,11 +173,8 @@ impl RequestVerifier { if request.seq != credentials.seq { return Err(AuthnError::InvalidSignature); } - let half = |phrase: &str| -> Result<[u8; 32], AuthnError> { - Ok(decode_phrase(phrase)?.to_be_byte_array().into()) - }; let EncodedSignature { r, s, .. } = &credentials.signature; - let signature = Ed25519Signature::from_components(half(r)?, half(s)?); + let signature = Ed25519Signature::from_components(r.to_be_bytes(), s.to_be_bytes()); self.0 .verify_strict( &request.to_be_signed(&credentials.key_id, &credentials.nonce), @@ -315,8 +313,9 @@ impl FromStr for BoundCredentials { nonce: params.parse("nonce")?, seq: params.parse("seq")?, signature: EncodedSignature { - r: params.get("r")?, - s: params.get("s")?, + r: params.parse("r")?, + + s: params.parse("s")?, flags: 0, counter: 0, }, @@ -526,8 +525,8 @@ impl FromStr for Credentials { key_id: params.parse("key-id")?, epk: params.parse("epk")?, signature: EncodedSignature { - r: params.get("r")?, - s: params.get("s")?, + r: params.parse("r")?, + s: params.parse("s")?, flags: params.parse("flags")?, counter: params.parse("counter")?, }, @@ -839,21 +838,21 @@ mod test { let epk = RequestKey::new().verifier(); assert!(matches!( Credentials::from_str( - "Sush nonce=abandon,cnonce=ability,key-id=able,epk=plugh,r=r,s=s,flags=0,counter=0" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk=plugh,r=burger,s=burst,flags=0,counter=0" ) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=r,s=s,flags=0,counter=foo" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=burger,s=burst,flags=0,counter=foo" )) .unwrap_err(), AuthnError::InvalidParam )); assert!(matches!( Credentials::from_str(&format!( - "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=r,s=s,flags=0,counter=0,foo=bar" + "Sush nonce=abandon,cnonce=ability,key-id=able,epk={epk},r=burger,s=burst,flags=0,counter=0,foo=bar" )) .unwrap_err(), AuthnError::TooManyParams diff --git a/common/src/keys.rs b/common/src/keys.rs index 5153b8d..b35b380 100644 --- a/common/src/keys.rs +++ b/common/src/keys.rs @@ -18,7 +18,7 @@ use borsh::io::{Error as BorshError, ErrorKind as BorshErrorKind, Read, Write}; use borsh::{BorshDeserialize, BorshSerialize}; use bytes::{Buf as _, BufMut as _, BytesMut}; use chrono::{DateTime, Utc}; -use crypto_bigint::{ArrayEncoding as _, Random as _, U128, U256}; +use crypto_bigint::{ArrayEncoding as _, Random as _, U128}; use ed25519_dalek::{ Signature as Ed25519Signature, Signer as _, SigningKey as Ed25519SigningKey, VerifyingKey as Ed25519VerifyingKey, @@ -45,7 +45,7 @@ use x509_cert::spki::{AlgorithmIdentifierOwned, SubjectPublicKeyInfo}; use x509_cert::time::Validity; use x509_cert::{Certificate, TbsCertificate, Version}; -use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase}; +use crate::codephrases::InvalidCodephrase; codephrase_newtype! { /// SHA-256 of a certificate subject or an identity public key, @@ -165,6 +165,42 @@ impl JsonSchema for SshPublicKey { } } +codephrase_newtype! { + /// The component `r` of a signature _(r, s)_ over a 256 bit elliptic curve. + #[derive( + BorshDeserialize, + BorshSerialize, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + Ord, + PartialEq, + PartialOrd, + Serialize, + )] + pub struct EccR = Full; +} + +codephrase_newtype! { + /// The component `s` of a signature _(r, s)_ over a 256 bit elliptic curve. + #[derive( + BorshDeserialize, + BorshSerialize, + Clone, + Deserialize, + Eq, + Hash, + JsonSchema, + Ord, + PartialEq, + PartialOrd, + Serialize, + )] + pub struct EccS = Full; +} + /// Code phrase encoded signature. /// /// The decoded phrases `r` and `s` together comprise a signature @@ -188,8 +224,8 @@ impl JsonSchema for SshPublicKey { Serialize, )] pub struct EncodedSignature { - pub r: String, - pub s: String, + pub r: EccR, + pub s: EccS, #[serde(default, skip_serializing_if = "is_zero_flags")] pub flags: u8, @@ -219,8 +255,8 @@ impl EncodedSignature { flags: _, counter: _, } = self; - let r = decode_phrase(r)?.to_be_byte_array(); - let s = decode_phrase(s)?.to_be_byte_array(); + let r = r.to_be_bytes(); + let s = s.to_be_bytes(); match signature_algorithm { AlgorithmIdentifierOwned { oid: ECDSA_WITH_SHA_256, @@ -231,10 +267,7 @@ impl EncodedSignature { AlgorithmIdentifierOwned { oid: ID_ED_25519, parameters: None, - } => Ok(Signature::Ed25519(Ed25519Signature::from_components( - r.into(), - s.into(), - ))), + } => Ok(Signature::Ed25519(Ed25519Signature::from_components(r, s))), _ => Err(KeyError::InvalidPublicKeyAlgorithm), } } @@ -248,16 +281,13 @@ impl EncodedSignature { flags, counter, } = self; - let r = decode_phrase(r)?.to_be_byte_array(); - let s = decode_phrase(s)?.to_be_byte_array(); + let r = r.to_be_bytes(); + let s = s.to_be_bytes(); match algorithm { Ecdsa { curve: NistP256 } => Ok(Signature::EcdsaSha256( ecdsa::Signature::from_scalars(r, s)?, )), - Ed25519 => Ok(Signature::Ed25519(Ed25519Signature::from_components( - r.into(), - s.into(), - ))), + Ed25519 => Ok(Signature::Ed25519(Ed25519Signature::from_components(r, s))), SkEcdsaSha2NistP256 => { let mut bytes = BytesMut::new(); put_mpint(&mut bytes, r)?; @@ -306,17 +336,16 @@ impl Signature { } pub fn encode(&self) -> Result { - let codephrase = |x: U256| codephrase(x).join(WORD_SEPARATOR); match self { Self::EcdsaSha256(signature) => Ok(EncodedSignature { - r: codephrase(U256::from_be_byte_array(signature.r().to_bytes())), - s: codephrase(U256::from_be_byte_array(signature.s().to_bytes())), + r: EccR::from_be_bytes(signature.r().to_bytes().into()), + s: EccS::from_be_bytes(signature.s().to_bytes().into()), flags: 0, counter: 0, }), Self::Ed25519(signature) => Ok(EncodedSignature { - r: codephrase(U256::from_be_slice(signature.r_bytes())), - s: codephrase(U256::from_be_slice(signature.s_bytes())), + r: EccR::from_be_bytes(*signature.r_bytes()), + s: EccS::from_be_bytes(*signature.s_bytes()), flags: 0, counter: 0, }), @@ -327,8 +356,8 @@ impl Signature { let s = get_mpint(&mut signature)?; let e = || KeyError::InvalidSignatureEncoding; Ok(EncodedSignature { - r: codephrase(u256_be(r.as_positive_bytes().ok_or_else(e)?)?), - s: codephrase(u256_be(s.as_positive_bytes().ok_or_else(e)?)?), + r: EccR::from_be_bytes(u256_be(r.as_positive_bytes().ok_or_else(e)?)?), + s: EccS::from_be_bytes(u256_be(s.as_positive_bytes().ok_or_else(e)?)?), flags, counter, }) @@ -337,8 +366,8 @@ impl Signature { let (signature, flags, counter) = sk_split(signature.as_bytes())?; let signature = Ed25519Signature::from_slice(signature)?; Ok(EncodedSignature { - r: codephrase(U256::from_be_slice(signature.r_bytes())), - s: codephrase(U256::from_be_slice(signature.s_bytes())), + r: EccR::from_be_bytes(*signature.r_bytes()), + s: EccS::from_be_bytes(*signature.s_bytes()), flags, counter, }) @@ -399,13 +428,13 @@ impl Signature { /// A `U256` from up to 32 big-endian bytes. An mpint's minimal /// encoding may carry fewer, which `U256::from_be_slice` refuses. -fn u256_be(bytes: &[u8]) -> Result { +fn u256_be(bytes: &[u8]) -> Result<[u8; 32], KeyError> { let Some(pad) = 32usize.checked_sub(bytes.len()) else { return Err(KeyError::InvalidSignatureEncoding); }; let mut buf = [0; 32]; buf[pad..].copy_from_slice(bytes); - Ok(U256::from_be_slice(&buf)) + Ok(buf) } /// Append one SSH mpint, encoded from positive big-endian bytes. diff --git a/server/src/messages.rs b/server/src/messages.rs index 3aa7cae..9ed896a 100644 --- a/server/src/messages.rs +++ b/server/src/messages.rs @@ -336,8 +336,8 @@ mod wire_format { request, KeyId::from_str("zoo-zero").unwrap(), EncodedSignature { - r: "abandon".to_string(), - s: "zoo".to_string(), + r: "abandon".parse().unwrap(), + s: "zoo".parse().unwrap(), flags: 0, counter: 0, }, @@ -378,8 +378,8 @@ mod wire_format { response, KeyId::from_str("zoo-zero").unwrap(), EncodedSignature { - r: "abandon".to_string(), - s: "zoo".to_string(), + r: "abandon".parse().unwrap(), + s: "zoo".parse().unwrap(), flags: 0, counter: 0, }, diff --git a/server/tests/output/identity-login-request.bin b/server/tests/output/identity-login-request.bin index af1475a92b41a0ec7a21a531fbd170f0eb33e6b1..90e9242db401d94f5834ec05e2de012a4385cfc7 100644 GIT binary patch delta 21 acmbQqbew4e6XV2*1``+Xv9teY0096@c?Grr delta 31 jcmX@kG?Qrq6Qejg0|P^1Qes|8ejYQBS(Tp;BpDb0b}9yB diff --git a/server/tests/output/job-start-request.bin b/server/tests/output/job-start-request.bin index 76326d2793b4d58cdd3fd68f6de2732bf4e95068..3fa78f1f74a6d66051890cb05ae893aae70d4d47 100644 GIT binary patch delta 27 ecmZ3_c$aZP%|vB|i3OYx#LB?H!2W+?pCSN-Xb9K< delta 28 icmcc1xSnxBjSxEn14Cj`VqQvq9y5?xm7hOxk0JnecnDAc From 2a176166747a254ce50017a4c77dfd1af5146175 Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 18:02:34 +0200 Subject: [PATCH 07/10] switch request verifier to the newtype --- common/src/authn.rs | 80 +++++++++++++-------------------------------- 1 file changed, 22 insertions(+), 58 deletions(-) diff --git a/common/src/authn.rs b/common/src/authn.rs index 89a84f1..3ccd184 100644 --- a/common/src/authn.rs +++ b/common/src/authn.rs @@ -30,10 +30,9 @@ use std::str::FromStr; use std::time::Duration; use blake3::Hasher; -use borsh::io::{Error, ErrorKind, Read, Write}; use borsh::{BorshDeserialize, BorshSerialize}; use chrono::{DateTime, Utc}; -use crypto_bigint::{ArrayEncoding as _, U256}; +use crypto_bigint::U256; use ed25519_dalek::{Signature as Ed25519Signature, Signer as _, SigningKey, VerifyingKey}; use http::header::{HeaderValue, InvalidHeaderValue}; use rand_core::OsRng; @@ -42,7 +41,7 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use crate::codephrase_newtype; -use crate::codephrases::{InvalidCodephrase, WORD_SEPARATOR, codephrase, decode_phrase}; +use crate::codephrases::InvalidCodephrase; use crate::keys::{ EccR, EccS, EncodedSignature, KeyError, KeyId, Signed, SshPublicKey, ToBeSigned, Verified, }; @@ -135,7 +134,7 @@ impl RequestKey { } pub fn verifier(&self) -> RequestVerifier { - RequestVerifier(self.0.verifying_key()) + RequestVerifier::from_be_bytes(*self.0.verifying_key().as_bytes()) } /// Sign `request`, yielding the credentials that authorize it. @@ -155,15 +154,14 @@ impl RequestKey { } } -/// The server half of an ephemeral request-signing key. -#[derive(Clone, Debug, Eq, PartialEq)] -pub struct RequestVerifier(VerifyingKey); +codephrase_newtype! { + /// The server half of an ephemeral request-signing key. + #[derive(Clone, Eq, PartialEq, BorshSerialize, BorshDeserialize)] + pub struct RequestVerifier = Full; -impl RequestVerifier { - fn as_bytes(&self) -> &[u8] { - self.0.as_bytes() - } +} +impl RequestVerifier { /// Verify that `credentials` sign `request`. pub fn verify( &self, @@ -175,53 +173,20 @@ impl RequestVerifier { } let EncodedSignature { r, s, .. } = &credentials.signature; let signature = Ed25519Signature::from_components(r.to_be_bytes(), s.to_be_bytes()); - self.0 - .verify_strict( - &request.to_be_signed(&credentials.key_id, &credentials.nonce), - &signature, - ) - .map_err(|_| AuthnError::InvalidSignature) - } -} -impl BorshSerialize for RequestVerifier { - fn serialize(&self, writer: &mut W) -> borsh::io::Result<()> { - writer.write_all(self.as_bytes()) - } -} - -impl BorshDeserialize for RequestVerifier { - fn deserialize_reader(reader: &mut R) -> borsh::io::Result { - let mut bytes = [0; 32]; - reader.read_exact(&mut bytes)?; - VerifyingKey::from_bytes(&bytes) - .map(Self) - .map_err(|err| Error::new(ErrorKind::InvalidData, err)) - } -} - -impl fmt::Display for RequestVerifier { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "{}", - codephrase(U256::from_be_slice(self.as_bytes())).join(WORD_SEPARATOR) - ) - } -} - -impl FromStr for RequestVerifier { - type Err = AuthnError; - - fn from_str(s: &str) -> Result { - let bytes = decode_phrase(s)?.to_be_byte_array(); - let key = VerifyingKey::from_bytes(&bytes.into()).map_err(|_| AuthnError::InvalidKey)?; + let key = + VerifyingKey::from_bytes(&self.to_be_bytes()).map_err(|_| AuthnError::InvalidKey)?; // A small-order key verifies anything, degrading the session // back to a bearer credential. if key.is_weak() { return Err(AuthnError::InvalidKey); } - Ok(Self(key)) + + key.verify_strict( + &request.to_be_signed(&credentials.key_id, &credentials.nonce), + &signature, + ) + .map_err(|_| AuthnError::InvalidSignature) } } @@ -432,7 +397,7 @@ impl ToBeSigned for ChallengeResponse { hash(Self::TYPE_NAME); hash(&nonce.to_be_bytes()); hash(&cnonce.to_be_bytes()); - hash(epk.as_bytes()); + hash(&epk.to_be_bytes()); hasher.finalize().as_bytes().to_vec() } } @@ -768,19 +733,18 @@ mod test { assert!(verifier.verify(&request, &relabeled).is_err()); let relabeled = BoundCredentials { nonce: Nonce::random(), - ..creds + ..creds.clone() }; assert!(verifier.verify(&request, &relabeled).is_err()); // A small-order ephemeral key is refused at parse. - let weak = codephrase(U256::from_be_slice(&{ + let weak = RequestVerifier::from_be_bytes({ let mut identity = [0u8; 32]; identity[0] = 1; identity - })) - .join(WORD_SEPARATOR); + }); assert!(matches!( - weak.parse::().unwrap_err(), + weak.verify(&request, &creds).unwrap_err(), AuthnError::InvalidKey )); } From 991ea72c05c1f047a21e9d8609b0e13780052693 Mon Sep 17 00:00:00 2001 From: Emily Albini Date: Tue, 11 Aug 2026 18:16:39 +0200 Subject: [PATCH 08/10] remove public functions to create codephrases --- client/src/identity.rs | 8 +- common/src/codephrases.rs | 206 +++++++++++++++++-------------------- server/tests/common/mod.rs | 4 +- tests/src/test_utils.rs | 4 +- 4 files changed, 100 insertions(+), 122 deletions(-) diff --git a/client/src/identity.rs b/client/src/identity.rs index bb89387..25d290f 100644 --- a/client/src/identity.rs +++ b/client/src/identity.rs @@ -171,7 +171,7 @@ impl IdentityError { #[cfg(test)] mod test { - use sush_common::codephrases::generate_id; + use sush_common::codephrases::Codephrase; use tempfile::TempDir; use tokio::process::Command; @@ -214,9 +214,9 @@ mod test { let mut agent = SshAgentConnection::connect(&sock).await.unwrap(); for key in agent.list_identities().await.unwrap() { - let nonce = generate_id(); - let signature = agent.sign_with(&key, nonce.as_bytes()).await.unwrap(); - key.verify(nonce.as_bytes(), &signature).unwrap(); + let nonce = Codephrase::random(); + let signature = agent.sign_with(&key, &nonce.to_be_bytes()).await.unwrap(); + key.verify(&nonce.to_be_bytes(), &signature).unwrap(); } agent_process diff --git a/common/src/codephrases.rs b/common/src/codephrases.rs index 7b39dd3..8f25cf3 100644 --- a/common/src/codephrases.rs +++ b/common/src/codephrases.rs @@ -29,7 +29,12 @@ use std::io::prelude::{Read, Write}; /// to 2048, which gives us indexes into the BIP-39 word list. /// Since 204823 < 2256 < 204824, /// 24 words suffice to represent 256 bits with no redundancy. -pub const PHRASE_WORDS_256: usize = 24; +const PHRASE_WORDS_256: usize = 24; + +/// The BIP-39 word list contains no punctuation of any kind, so we are +/// free to use ASCII hyphen (`-`) as the default word separator. Decoding +/// will also accept arbitrary ASCII whitespace as word separators. +const WORD_SEPARATOR: &str = "-"; /// With 2048 words, an 8 word code phrase has ~88 bits of entropy, /// making it suitable for use as a unique, hard-to-guess identifier, @@ -38,14 +43,7 @@ pub const PHRASE_WORDS_256: usize = 24; /// Note that the security of the Support Shell protocol (RFD 620) does /// *not* rely on such identifiers being unguessable; it relies only on /// the strength of the signatures produced over these phrases. -pub const PHRASE_WORDS_ID: usize = 8; - -/// The BIP-39 word list contains no punctuation of any kind, so we are -/// free to use ASCII hyphen (`-`) as the default word separator. Decoding -/// will also accept arbitrary ASCII whitespace as word separators. -pub const WORD_SEPARATOR: &str = "-"; - -const TRUNCATED_MASK: U256 = U256::from_u128(2u128.pow(88) - 1); +const TRUNCATED_BITS: u32 = 88; /// Random code phrase like `abstract misery favorite ordinary moon talk`. #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -57,10 +55,16 @@ impl Codephrase { Self(U256::random(&mut OsRng)) } + /// Turn 256 bits of entropy into a reasonably unique code phrase. + /// We use the low-order words of the full codephrase, and pad to + /// the canonical length; these phrases are intended for machine + /// consumption, and are short enough for easy transmission. pub fn truncate(self) -> Self { - Self(self.0.bitand(&TRUNCATED_MASK)) + let mask = U256::from_u128(2u128.pow(TRUNCATED_BITS) - 1); + Self(self.0.bitand(&mask)) } + /// Construct a codephrase from its big-endian byte representation. pub fn from_be_bytes(bytes: [u8; 32]) -> Self { Self(U256::from_be_bytes(bytes)) } @@ -73,7 +77,18 @@ impl Codephrase { impl std::fmt::Display for Codephrase { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&codephrase(self.0).join(WORD_SEPARATOR)) + // Turn 256 bits of entropy into an un-padded big-endian code phrase. + let b = Reciprocal::new(Limb(WORDLIST_LEN as u64)).expect("should have some words"); + let mut n = self.0; + let mut r; + let mut words = Vec::with_capacity(PHRASE_WORDS_256); + while n > U256::ZERO { + (n, r) = n.ct_div_rem_limb_with_reciprocal(&b); + words.push(word(r.0 as usize)); // accumulate little-endian + } + words.reverse(); // emit big-endian + + f.write_str(&words.join(WORD_SEPARATOR)) } } @@ -87,7 +102,34 @@ impl FromStr for Codephrase { type Err = InvalidCodephrase; fn from_str(value: &str) -> Result { - Ok(Self(decode_phrase(value)?)) + // Decode a big endian code phrase into 256 bits of entropy. + // + // Decoding is non-injective, or "liberal" in the sense that it will + // accept non-canonical codephrases, e.g., ones with spaces instead of + // separators, words in mixed case, or without leading zeros. This is + // for the comfort of humans that may have to transmit such phrases. + // It rejects phrases that no entropy encodes to: unknown words, more + // than [`PHRASE_WORDS_256`] words, or a value of 256 bits or more. + // The empty phrase decodes to zero. Callers that must distinguish + // absent input from zero should check before decoding. + + let b = U256::from_u64(WORDLIST_LEN as u64); + let mut n = U256::ZERO; + let mut words = 0; + for word in value + .to_ascii_lowercase() + .replace(WORD_SEPARATOR, " ") + .split_ascii_whitespace() + { + words += 1; + if words > PHRASE_WORDS_256 { + return Err(InvalidCodephrase); + } + let r = U256::from_u64(index(word)? as u64); + n = Option::from(n.checked_mul(&b)).ok_or(InvalidCodephrase)?; + n = Option::from(n.checked_add(&r)).ok_or(InvalidCodephrase)?; + } + Ok(Self(n)) } } @@ -100,7 +142,7 @@ impl Serialize for Codephrase { impl<'de> Deserialize<'de> for Codephrase { fn deserialize>(deserializer: D) -> Result { let string = ::deserialize(deserializer)?; - Ok(Self(decode_phrase(&string).map_err(D::Error::custom)?)) + Self::from_str(&string).map_err(D::Error::custom) } } @@ -230,80 +272,16 @@ fn index(word: &str) -> Result { WORDLIST.binary_search(&word).map_err(|_| InvalidCodephrase) } -/// Turn 256 bits of entropy into an un-padded big-endian code phrase. -pub fn codephrase(value: U256) -> Vec<&'static str> { - let b = Reciprocal::new(Limb(WORDLIST_LEN as u64)).expect("should have some words"); - let mut n = value; - let mut r; - let mut words = Vec::with_capacity(PHRASE_WORDS_256); - while n > U256::ZERO { - (n, r) = n.ct_div_rem_limb_with_reciprocal(&b); - words.push(word(r.0 as usize)); // accumulate little-endian - } - words.reverse(); // emit big-endian - words -} - -/// Turn 256 bits of entropy into a reasonably unique code phrase. -/// We use the low-order words of the full codephrase, and pad to -/// the canonical length; these phrases are intended for machine -/// consumption, and are short enough for easy transmission. -pub fn id_phrase(value: U256) -> Vec<&'static str> { - let mut phrase = codephrase(value); - let n = phrase.len().saturating_sub(PHRASE_WORDS_ID); - let mut phrase = phrase.split_off(n); - while phrase.len() < PHRASE_WORDS_ID { - phrase.insert(0, word(0)); // pad w/leading zeros - } - phrase -} - -/// Generate a code phrase for use as an identifier. -pub fn generate_id() -> String { - id_phrase(U256::random(&mut OsRng)).join(WORD_SEPARATOR) -} - -/// Decode a big endian code phrase into 256 bits of entropy. -/// -/// Decoding is non-injective, or "liberal" in the sense that it will -/// accept non-canonical codephrases, e.g., ones with spaces instead of -/// separators, words in mixed case, or without leading zeros. This is -/// for the comfort of humans that may have to transmit such phrases. -/// It rejects phrases that no entropy encodes to: unknown words, more -/// than [`PHRASE_WORDS_256`] words, or a value of 256 bits or more. -/// The empty phrase decodes to zero. Callers that must distinguish -/// absent input from zero should check before decoding. -pub fn decode_phrase(phrase: &str) -> Result { - let b = U256::from_u64(WORDLIST_LEN as u64); - let mut n = U256::ZERO; - let mut words = 0; - for word in phrase - .to_ascii_lowercase() - .replace(WORD_SEPARATOR, " ") - .split_ascii_whitespace() - { - words += 1; - if words > PHRASE_WORDS_256 { - return Err(InvalidCodephrase); - } - let r = U256::from_u64(index(word)? as u64); - n = Option::from(n.checked_mul(&b)).ok_or(InvalidCodephrase)?; - n = Option::from(n.checked_add(&r)).ok_or(InvalidCodephrase)?; - } - Ok(n) -} - #[cfg(test)] mod test { use super::*; - use sha2::{Digest as _, Sha256}; use std::collections::HashSet; fn round_trip(entropy: U256) -> String { - let codephrase = codephrase(entropy).join(WORD_SEPARATOR); - let decoded = decode_phrase(&codephrase).unwrap(); - assert_eq!(entropy, decoded); - codephrase + let codephrase = Codephrase::from_be_bytes(entropy.to_be_bytes()); + let decoded: Codephrase = codephrase.to_string().parse().unwrap(); + assert_eq!(codephrase, decoded); + codephrase.to_string() } #[test] @@ -336,44 +314,49 @@ mod test { #[test] fn non_phrases() { // Unknown words, wherever they appear. - assert!(decode_phrase("plugh").is_err()); - assert!(decode_phrase("abandon-plugh").is_err()); + assert!(Codephrase::from_str("plugh").is_err()); + assert!(Codephrase::from_str("abandon-plugh").is_err()); // More words than any entropy encodes to. let long = [word(0); PHRASE_WORDS_256 + 1].join(WORD_SEPARATOR); - assert!(decode_phrase(&long).is_err()); + assert!(Codephrase::from_str(&long).is_err()); // 24-word values of 256 bits or more: the smallest, exactly // 2^256, and the largest. let mut smallest = vec![word(8)]; smallest.extend([word(0); PHRASE_WORDS_256 - 1]); - assert!(decode_phrase(&smallest.join(WORD_SEPARATOR)).is_err()); + assert!(Codephrase::from_str(&smallest.join(WORD_SEPARATOR)).is_err()); let largest = [word(WORDLIST_LEN - 1); PHRASE_WORDS_256].join(WORD_SEPARATOR); - assert!(decode_phrase(&largest).is_err()); + assert!(Codephrase::from_str(&largest).is_err()); } #[test] fn constant_codephrases() { assert_eq!( - round_trip(U256::from_be_slice(&Sha256::digest("test phrase one"))), - "abstract-summer-orange-gown-urge-model-\ - exact-gorilla-outside-common-this-pepper-\ - pear-dust-minimum-black-double-recipe-\ - castle-crystal-clog-logic-delay-hamster" + round_trip(U256::from_be_slice( + blake3::hash(b"test phrase one").as_slice() + )), + "able-wet-frame-clown-gauge-gather-curious-\ + stereo-moment-moral-mirror-net-laptop-square-\ + toe-skill-upper-credit-cancel-flag-what-powder-\ + guide-hold" ); assert_eq!( - round_trip(U256::from_be_slice(&Sha256::digest("another test phrase"))), - "abstract-misery-favorite-ordinary-moon-talk-\ - write-coffee-digital-slogan-spray-angry-\ - once-jazz-random-income-garage-regret-accident-\ - file-release-deny-reward-drastic" + round_trip(U256::from_be_slice( + blake3::hash(b"another test phrase").as_slice() + )), + "about-item-skill-author-expose-assume-language-\ + mix-tornado-undo-dolphin-obtain-good-quarter-\ + poem-under-system-hybrid-foil-person-together-\ + output-exhaust-today" ); assert_eq!( - round_trip(U256::from_be_slice(&Sha256::digest("one more for luck!"))), - "able-dismiss-cost-scheme-amazing-slogan-\ - service-current-protect-feed-length-text-\ - cruise-wisdom-beauty-angle-regret-truck-\ - prosper-album-decline-wheel-pause-legend" + round_trip(U256::from_be_slice( + blake3::hash(b"one more for luck!").as_slice() + )), + "able-hazard-bread-decade-elegant-omit-ensure-\ + sudden-beef-voice-remove-nut-wish-bind-birth-b\ + leak-brush-joke-seven-amused-sunny-kite-flee-tape" ); } @@ -414,22 +397,17 @@ mod test { fn id_phrases() { let mut seen = HashSet::new(); for _ in 0..10_000 { - let id = generate_id(); - let n = id.split(WORD_SEPARATOR).count(); - assert_eq!(n, PHRASE_WORDS_ID); - assert!(seen.insert(id.clone()), "duplicate ID {id}"); - - let entropy = decode_phrase(&id).unwrap(); - round_trip(entropy); - assert_eq!(id, id_phrase(entropy).join(WORD_SEPARATOR)); + let id = Codephrase::random().truncate(); + assert!(seen.insert(id), "duplicate ID {id}"); + + let roundtrip = Codephrase::from_str(&id.to_string()).unwrap(); + assert_eq!(id, roundtrip); } } #[test] - fn id_leading_zero() { - let v = U256::ONE.shl_vartime(66); // 7 digits, since 2048⁶ = 2⁶⁶ - let id = id_phrase(v); - assert_eq!(id.len(), PHRASE_WORDS_ID); - assert_eq!(decode_phrase(&id.join(WORD_SEPARATOR)).unwrap(), v); + fn leading_zeros_after_truncation() { + let truncated = Codephrase::random().truncate(); + assert_eq!(U256::from_u8(0), truncated.0.shr(TRUNCATED_BITS as _)); } } diff --git a/server/tests/common/mod.rs b/server/tests/common/mod.rs index baacc3e..1a62c12 100644 --- a/server/tests/common/mod.rs +++ b/server/tests/common/mod.rs @@ -26,7 +26,7 @@ use sprockets_tls_test_utils::{ private_key_path, root_prefix, sprockets_auth_prefix, }; use sush_common::authn::{Challenge, ChallengeResponse, Identity, Nonce, RequestKey}; -use sush_common::codephrases::generate_id; +use sush_common::codephrases::Codephrase; use sush_common::jobs::{JobId, JobStartRequest, SignedJob}; use sush_common::keys::{EphemeralKey, KeyType, Signer as _}; use sush_common::targets::Target; @@ -126,7 +126,7 @@ pub async fn eventually(what: &str, secs: u64, mut condition: impl AsyncFnMut() pub fn ephemeral_root() -> EphemeralKey { let mut buf = [0; 8]; OsRng.fill_bytes(&mut buf); - let id = generate_id(); + let id = Codephrase::random().truncate(); EphemeralKey::new_root( KeyType::P256, format!("CN=Ephemeral Test Key {id},O=Oxide Computer Company,C=US") diff --git a/tests/src/test_utils.rs b/tests/src/test_utils.rs index a159447..7f7a87e 100644 --- a/tests/src/test_utils.rs +++ b/tests/src/test_utils.rs @@ -25,7 +25,7 @@ use x509_cert::time::Validity; use sush_client::context::Authz; use sush_client::{Client, ResponseValue}; use sush_common::authn::{Challenge, ChallengeResponse, Credentials, Identity, Nonce, RequestKey}; -use sush_common::codephrases::generate_id; +use sush_common::codephrases::Codephrase; use sush_common::jobs::{JobId, JobStartRequest, VerifiedJob}; use sush_common::keys::{EphemeralKey, KeyType, Signer}; use sush_common::targets::{Cubbies, Target}; @@ -110,7 +110,7 @@ impl SignJobRequest for EphemeralKey { pub fn ephemeral_test_subject() -> Name { let mut buf = [0; 8]; OsRng.fill_bytes(&mut buf); - let id = generate_id(); + let id = Codephrase::random().truncate(); format!("CN=Ephemeral Test Key {id},O=Oxide Computer Company,C=US") .parse() .unwrap() From b0db7e33d7e3c9a8fed511792a4d33322e2962cc Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 12 Aug 2026 01:45:57 +0000 Subject: [PATCH 09/10] Group codephrase imports Also self-qualify the newtype macro's formatter types, so call sites need no imports of their own. --- common/src/codephrases.rs | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/common/src/codephrases.rs b/common/src/codephrases.rs index 8f25cf3..47aa45b 100644 --- a/common/src/codephrases.rs +++ b/common/src/codephrases.rs @@ -9,8 +9,12 @@ //! that must be readily transmissible over low bandwidth channels (e.g., //! email, voice, printed or handwritten notes, etc.). +use std::borrow::Cow; +use std::fmt; +use std::io::{self, Read, Write}; use std::str::FromStr; +use borsh::{BorshDeserialize, BorshSerialize}; use crypto_bigint::{ ArrayEncoding as _, CheckedAdd as _, CheckedMul as _, Encoding as _, Limb, Random as _, Reciprocal, U256, @@ -22,8 +26,6 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; use thiserror::Error; use crate::wordlist::{WORDLIST, WORDLIST_LEN}; -use borsh::{BorshDeserialize, BorshSerialize}; -use std::io::prelude::{Read, Write}; /// Entropy is treated as an integer whose base is to be changed /// to 2048, which gives us indexes into the BIP-39 word list. @@ -75,8 +77,8 @@ impl Codephrase { } } -impl std::fmt::Display for Codephrase { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Display for Codephrase { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { // Turn 256 bits of entropy into an un-padded big-endian code phrase. let b = Reciprocal::new(Limb(WORDLIST_LEN as u64)).expect("should have some words"); let mut n = self.0; @@ -92,8 +94,8 @@ impl std::fmt::Display for Codephrase { } } -impl std::fmt::Debug for Codephrase { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { +impl fmt::Debug for Codephrase { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "Codephrase({self})") } } @@ -147,13 +149,13 @@ impl<'de> Deserialize<'de> for Codephrase { } impl BorshSerialize for Codephrase { - fn serialize(&self, writer: &mut W) -> std::io::Result<()> { + fn serialize(&self, writer: &mut W) -> io::Result<()> { <[u8; 32] as BorshSerialize>::serialize(&self.to_be_bytes(), writer) } } impl BorshDeserialize for Codephrase { - fn deserialize_reader(reader: &mut R) -> std::io::Result { + fn deserialize_reader(reader: &mut R) -> io::Result { let bytes = <[u8; 32] as BorshDeserialize>::deserialize_reader(reader)?; Ok(Self(U256::from_be_byte_array(bytes.into()))) } @@ -173,7 +175,7 @@ impl JsonSchema for Codephrase { String::is_referenceable() } - fn schema_id() -> std::borrow::Cow<'static, str> { + fn schema_id() -> Cow<'static, str> { String::schema_id() } } @@ -228,13 +230,13 @@ macro_rules! codephrase_newtype { } impl std::fmt::Debug for $name { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { write!(f, "{}({})", stringify!($name), self.0) } } impl std::fmt::Display for $name { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { <$crate::codephrases::Codephrase as std::fmt::Display>::fmt(&self.0, f) } } From 6071f9eef0b1dd835b0d95a3735e7483ec2a7280 Mon Sep 17 00:00:00 2001 From: Alex Plotnick Date: Wed, 12 Aug 2026 01:49:36 +0000 Subject: [PATCH 10/10] Say what EccR and EccS hold for Ed25519 --- common/src/keys.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/common/src/keys.rs b/common/src/keys.rs index b35b380..7dcdc84 100644 --- a/common/src/keys.rs +++ b/common/src/keys.rs @@ -166,7 +166,10 @@ impl JsonSchema for SshPublicKey { } codephrase_newtype! { - /// The component `r` of a signature _(r, s)_ over a 256 bit elliptic curve. + /// The first half of a 256 bit elliptic-curve signature: the scalar + /// `r` of an ECDSA pair, or the encoded point `R` of an Ed25519 + /// signature. It is carried as an opaque 256 bit value to which only + /// the signature algorithm assigns meaning. #[derive( BorshDeserialize, BorshSerialize, @@ -184,7 +187,10 @@ codephrase_newtype! { } codephrase_newtype! { - /// The component `s` of a signature _(r, s)_ over a 256 bit elliptic curve. + /// The second half of a 256 bit elliptic-curve signature: the scalar + /// `s` of an ECDSA pair, or the little-endian scalar `S` of an + /// Ed25519 signature. It is carried as an opaque 256 bit value like + /// [`EccR`]. #[derive( BorshDeserialize, BorshSerialize,