diff --git a/docs/design.md b/docs/design.md
new file mode 100644
index 0000000000000000000000000000000000000000..c29d3fb422a8c19db59f47748d06851e70c5ec51
--- /dev/null
+++ b/docs/design.md
@@ -0,0 +1,69 @@
+This documents various design decisions, also to be read as _lessons learned_.
+At the time you read this they may, or may not, still be relevant.
+Apologies if I seem overly critical of any projects - I have huge respect for
+all of the dependencies I've tried.
+
+## Error type
+
+Snafu seems to do the job for an error type.
+
+Initially the `Error` type had two 35 byte `UnknownName`s in one of the variants.
+Removing that reduced the arm thumb binary size by 16kB! The reason appears to be that
+when the error variant of `Result` is larger than the `Ok` item, it can't be used
+to construct a struct in-place (it has to write to temporary larger stack space,
+then copy the item out). That matters a lot for the `sshwire` deserialisation
+that recursively creates lots of structs.
+
+`.trap()` is there as an alternative to `.unwrap()` - if a server handles multiple connections
+you don't want them all going away panicking if there's some un-thought-of edge case.
+Each call to `.trap()` seems on the order of perhaps 100 bytes larger than a plain `panic!()` -
+perhaps there should be a feature to just panic. In debug builds it panics too so it's quick
+to get a backtrace.
+
+## Serialisation/deserialisation
+
+Previously the code used serde with a custom `Serializer`/`Deserializer`. That worked
+fine for serializing and `derive(Deserialize)`, but deserializing enums was a pain.
+For types like `packets::ChannelRequest`
+the deserializer had to stash the channel request type string from the parent struct somewhere,
+then use that later when deserialising the `ChannelReqType` enum. Other response packets like number 60 `Userauth60`
+mean completely different things depending on the previous request, but that state is hard to pass through
+the deserializer.
+
+[`serde_state`](https://docs.rs/serde_state/latest/serde_state/) solves some of it, but was still a bit awkward,
+and didn't work well with manually written `Deserialize` implementations.
+
+serde is fairly flexible design,
+but for our purposes we can use something much simpler - the packet format is not self describing
+(in most parts), we just take bytes off the wire exactly as we expect them.
+
+Eventually I gave up fighting and wrote a new `sshwire_derive` with
+[virtue](https://github.com/bincode-org/virtue) crate. That is about 10kB smaller
+(arm thumb) and easier to customise as required. For example `UnknownName` has a simple attribute to indicate
+that's the variant that should store an unmatched string.
+
+It was easier than I expected - the virtue syntax seems a bit verbose but feels more intuitive than
+`syn` or `quote` crates which are the alternative. I think this is because it's still written in
+normal Rust, not a new macro language to learn.
+
+## Ring vs RustCrypto
+
+Initially the code was written using `ring`, mainly because it already had
+[`chacha20_poly1305_openssh`](https://docs.rs/ring/latest/ring/aead/chacha20_poly1305_openssh/index.html).
+That worked great until I tried to build for a `thumbv7em` platform. Some of the code wouldn't build
+(ARM assembly issues?, possibly fixable),
+but the bigger problem is there's no way to insert a custom random number generator.
+
+Instead switching to RustCrypto crates worked fairly easily, though perhaps a bit messier having
+to deal with `GenericArray` and `DynamicDigest` and types like that. At the time of writing `curve25519-dalek` etc
+are a bit behind on dependencies which is messy, but I assume that will sort itself out.
+
+## `Behaviour`
+
+At some points in packet handling some custom behaviour from the application is required. For example
+"is this hostkey valid?", "is this user's password correct?". Those need an immediate response,
+so `.await` fits well.
+
+The problem is that `async_trait` requires `Box`, won't work on `no_std`. The `Behaviour` struct has a `cfg` to switch
+between async and non-async traits, hiding that from the main code. Eventually `async fn` [should work OK](https://github.com/rust-lang/rust/issues/91611) in static traits on `no_std`, and then it can be unified.
+
diff --git a/smol/examples/con1.rs b/smol/examples/con1.rs
index 9eb2fe9fb28062f281a1d08cca0cb0b22467806e..74f3d30cb1275a2bb705010af69117ba13b4a2f9 100644
--- a/smol/examples/con1.rs
+++ b/smol/examples/con1.rs
@@ -71,7 +71,7 @@ fn main() -> Result<()> {
// time crate won't read TZ if we're threaded, in case someone
// tries to mutate shared state with setenv.
// https://github.com/rust-lang/rust/issues/90308 etc
- // So we can't use async main.
+ // logging uses the timezone, so we can't use async main.
setup_log(&args);
tokio::runtime::Builder::new_multi_thread()
@@ -104,7 +104,7 @@ fn setup_log(args: &Args) {
.add_filter_allow_str("con1")
// not debugging these bits of the stack at present
// .add_filter_ignore_str("door_sshproto::traffic")
- .add_filter_ignore_str("door_sshproto::runner")
+ // .add_filter_ignore_str("door_sshproto::runner")
.add_filter_ignore_str("door_smol::async_door")
.set_time_offset_to_local().expect("Couldn't get local timezone")
.build();
diff --git a/smol/src/async_client.rs b/smol/src/async_client.rs
index ec699d455249e478a2e2b772cd42b41178760ee2..ec2dc9538125c9f36c3997db5a0f5a8345527afc 100644
--- a/smol/src/async_client.rs
+++ b/smol/src/async_client.rs
@@ -34,6 +34,8 @@ impl SimpleClient {
pub fn add_authkey(&mut self, k: SignKey) {
self.authkeys.push_back(k)
}
+
+ fn write_stdout
}
#[async_trait(?Send)]
diff --git a/sshproto/src/encrypt.rs b/sshproto/src/encrypt.rs
index 8700bd507e9aa699fbdce575867f0ec34a075848..cfb018417d46438d7749f33971e470e216fd5398 100644
--- a/sshproto/src/encrypt.rs
+++ b/sshproto/src/encrypt.rs
@@ -453,7 +453,7 @@ impl fmt::Display for Cipher {
impl Cipher {
/// Creates a cipher key by algorithm name. Must be passed a known name.
- pub fn from_name(name: &str) -> Result<Self, Error> {
+ pub fn from_name(name: &'static str) -> Result<Self, Error> {
match name {
SSH_NAME_CHAPOLY => Ok(Cipher::ChaPoly),
SSH_NAME_AES256_CTR => Ok(Cipher::Aes256Ctr),
@@ -571,7 +571,7 @@ pub(crate) enum Integ {
impl Integ {
/// Matches a MAC name. Should not be called for AEAD ciphers, instead use [`EncKey::integ`] etc
- pub fn from_name(name: &str) -> Result<Self, Error> {
+ pub fn from_name(name: &'static str) -> Result<Self, Error> {
match name {
SSH_NAME_HMAC_SHA256 => Ok(Integ::HmacSha256),
_ => Err(Error::bug()),
diff --git a/sshproto/src/error.rs b/sshproto/src/error.rs
index 78d2760b607e18950fa4f37966cba75f916c709f..3a7b8b8b30ed700fc5b66c812bcb0418f379c0cb 100644
--- a/sshproto/src/error.rs
+++ b/sshproto/src/error.rs
@@ -11,44 +11,9 @@ use heapless::String;
use crate::behaviour::BhError;
-// RFC4251 defines a maximum of 64, but 35 is probably enough to identify
-// a problem.
-#[derive(Debug)]
-pub struct UnknownName(pub String<35>);
-
- impl fmt::Display for UnknownName {
- fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
- self.0.fmt(f)
- }
-}
-
-impl From<&str> for UnknownName {
-
- /// Indicates truncation
- fn from(from: &str) -> Self {
- let mut s = String::new();
- // +10 to avoid wasteful iteration on untrusted input
- let need = from.escape_default().take(s.capacity()+10).count();
- let used = if need > s.capacity() {
- s.capacity() - 4
- } else {
- need
- };
- for e in from.escape_default().take(used) {
- s.push(e).unwrap()
- }
-
- if need > used {
- s.push_str(" ...").unwrap()
- }
- UnknownName(s)
- }
-}
-
-
-
// TODO: can we make Snafu not require Debug?
// TODO: maybe split this into a list of public vs private errors?
+
#[non_exhaustive]
#[derive(Snafu, Debug)]
pub enum Error {
@@ -74,8 +39,8 @@ pub enum Error {
/// Key exchange incorrect
BadKex,
- #[snafu(display("Signature \"{sig}\" doesn't match key type \"{key}\""))]
- SignatureMismatch { key: UnknownName, sig: UnknownName },
+ // Signature doesn't match key type
+ SignatureMismatch,
/// Error in received SSH protocol
SSHProtoError,
@@ -111,8 +76,8 @@ pub enum Error {
/// An unknown SSH name is provided, for a key type, signature type,
/// channel name etc.
- #[snafu(display("Unknown {kind} method {name}"))]
- UnknownMethod { kind: &'static str, name: UnknownName },
+ #[snafu(display("Unknown {kind} method"))]
+ UnknownMethod { kind: &'static str},
/// Implementation behaviour error
#[snafu(display("Failure from application: {msg}"))]
@@ -263,17 +228,6 @@ pub(crate) mod tests {
use crate::packets::Unknown;
use proptest::prelude::*;
- proptest! {
- #[test]
- fn unknown_name_from_pt(s: std::string::String) {
- let u: UnknownName = s.as_str().into();
- let cap = u.0.capacity();
- if s.escape_default().count() > cap {
- assert_eq!(&u.0[cap-4..], " ...");
- }
- }
-
- }
}
diff --git a/sshproto/src/kex.rs b/sshproto/src/kex.rs
index 0d19ac1088cfcb537028d12c0ade44abd90ed302..c934b6c7317738ed302bb62ec2b24b187ee033cb 100644
--- a/sshproto/src/kex.rs
+++ b/sshproto/src/kex.rs
@@ -383,7 +383,7 @@ impl fmt::Display for SharedSecret {
}
impl SharedSecret {
- pub fn from_name(name: &str) -> Result<Self> {
+ pub fn from_name(name: &'static str) -> Result<Self> {
match name {
SSH_NAME_CURVE25519 | SSH_NAME_CURVE25519_LIBSSH => {
Ok(SharedSecret::KexCurve25519(KexCurve25519::new()?))
diff --git a/sshproto/src/namelist.rs b/sshproto/src/namelist.rs
index 79bb17af5d39a3f1802257b04852ef894c8ed3e6..318017283b9a983d5d396b36a6f12f914b8ec512 100644
--- a/sshproto/src/namelist.rs
+++ b/sshproto/src/namelist.rs
@@ -93,7 +93,7 @@ impl<'a> NameList<'a> {
/// Must only be called on [`StringNames`], will fail if called with self as [`LocalNames`].
pub fn first_match(
&self, is_client: bool, our_options: &LocalNames,
- ) -> Result<Option<&str>> {
+ ) -> Result<Option<&'static str>> {
match self {
NameList::String(s) => Ok(if is_client {
s.first_options_match(our_options)
@@ -126,11 +126,11 @@ impl<'a> NameList<'a> {
impl<'a> StringNames<'a> {
/// Returns the first name in this namelist that matches one of the provided options
- fn first_string_match(&self, options: &LocalNames) -> Option<&str> {
+ fn first_string_match(&self, options: &LocalNames) -> Option<&'static str> {
for n in self.0.split(',') {
for o in options.0.iter() {
if n == *o {
- return Some(n);
+ return Some(*o);
}
}
}
@@ -138,11 +138,11 @@ impl<'a> StringNames<'a> {
}
/// Returns the first of "options" that is in this namelist
- fn first_options_match(&self, options: &LocalNames) -> Option<&str> {
+ fn first_options_match(&self, options: &LocalNames) -> Option<&'static str> {
for o in options.0.iter() {
for n in self.0.split(',') {
if n == *o {
- return Some(n);
+ return Some(*o);
}
}
}
diff --git a/sshproto/src/packets.rs b/sshproto/src/packets.rs
index cbdd41a4f46b333d4036c0baad0c3b27099afad4..2d4341a3694be7693c4f387617276c180bd8dd8a 100644
--- a/sshproto/src/packets.rs
+++ b/sshproto/src/packets.rs
@@ -303,8 +303,10 @@ impl<'a> Signature<'a> {
match pubkey {
PubKey::Ed25519(_) => Ok(SSH_NAME_ED25519),
PubKey::RSA(_) => Ok(SSH_NAME_RSA_SHA256),
- PubKey::Unknown(u) => Err(Error::UnknownMethod {kind: "key",
- name: u.0.into() })
+ PubKey::Unknown(u) => {
+ warn!("Unknown key type \"{}\"", u);
+ Err(Error::UnknownMethod {kind: "key"})
+ }
}
}
@@ -313,8 +315,8 @@ impl<'a> Signature<'a> {
Signature::Ed25519(_) => Ok(SigType::Ed25519),
Signature::RSA256(_) => Ok(SigType::RSA256),
Signature::Unknown(u) => {
- Err(Error::UnknownMethod {kind: "signature",
- name: u.0.into() })
+ warn!("Unknown signature type \"{}\"", u);
+ Err(Error::UnknownMethod {kind: "signature" })
}
}
}
@@ -547,6 +549,13 @@ pub struct DirectTcpip<'a> {
#[derive(Debug, Clone, PartialEq)]
pub struct Unknown<'a>(pub &'a str);
+impl core::fmt::Display for Unknown<'_> {
+ fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+ self.0.escape_default().fmt(f)
+ }
+
+}
+
/// State to be passed to decoding.
/// Use this so the parser can select the correct enum variant to decode.
#[derive(Default, Clone, Debug)]
diff --git a/sshproto/src/sign.rs b/sshproto/src/sign.rs
index fce77ee5e66c0aa5c8e37ec7d06fd941a37e60a3..7c30fcfeaa5f757e97cd3e053f7a6850dc5eab30 100644
--- a/sshproto/src/sign.rs
+++ b/sshproto/src/sign.rs
@@ -28,7 +28,7 @@ pub enum SigType {
impl SigType {
/// Must be a valid name
- pub fn from_name(name: &str) -> Result<Self> {
+ pub fn from_name(name: &'static str) -> Result<Self> {
match name {
SSH_NAME_ED25519 => Ok(SigType::Ed25519),
SSH_NAME_RSA_SHA256 => Ok(SigType::RSA256),
@@ -85,10 +85,11 @@ impl SigType {
}
_ => {
- Err(Error::SignatureMismatch {
- key: pubkey.algorithm_name().into(),
- sig: sig.algorithm_name().into(),
- })
+ warn!("Signature \"{}\" doesn't match key type \"{}\"",
+ sig.algorithm_name(),
+ pubkey.algorithm_name(),
+ );
+ Err(Error::SignatureMismatch)
}
}
}