From 282d438e86250d3c3db0595552294a2b445ee796 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Wed, 1 Apr 2026 23:34:29 +0200 Subject: [PATCH 01/10] First pass of cargo clippy (auto)fix: cargo clippy --features esp32c6 -- -W clippy::mem_forget_instead_of_drop -W clippy::holding_lock -W clippy::large_futures -W clippy::pedantic --- src/config.rs | 13 +++--- src/espressif/buffered_uart.rs | 9 +++-- src/espressif/net.rs | 39 +++++++++--------- src/main.rs | 28 ++++++------- src/serial.rs | 2 +- src/serve.rs | 74 +++++++++++++++++----------------- src/store.rs | 10 ++--- 7 files changed, 87 insertions(+), 88 deletions(-) diff --git a/src/config.rs b/src/config.rs index 1ed4e96..5a5688c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -29,7 +29,7 @@ pub struct SSHStampConfig { /// Authentication: only pubkey-based auth supported pub pubkeys: [Option; KEY_SLOTS], - /// WiFi + /// `WiFi` pub wifi_ssid: String<32>, pub wifi_pw: Option>, // Max 64 characters including null-terminator? @@ -71,6 +71,7 @@ impl SSHStampConfig { pub const CURRENT_VERSION: u8 = 9; /// Check if configured for random MAC on each boot + #[must_use] pub fn is_mac_random(&self) -> bool { self.mac == MAC_RANDOM_SENTINEL } @@ -118,7 +119,7 @@ impl SSHStampConfig { let mut rnd = [0u8; 24]; sunset::random::fill_random(&mut rnd)?; let mut pw = String::<63>::new(); - for &byte in rnd.iter() { + for &byte in &rnd { let _ = pw.push(WIFI_PASSWORD_CHARS[(byte as usize) % 62] as char); } Ok(pw) @@ -154,7 +155,7 @@ impl SSHStampConfig { }; debug!("Parsed Ed25519 public key, adding to config"); - for slot in self.pubkeys.iter_mut() { + for slot in &mut self.pubkeys { if slot.is_none() { *slot = Some(newk); return Ok(()); @@ -229,7 +230,7 @@ fn enc_ipv4_config(v: &Option, s: &mut dyn SSHSink) -> WireResul debug!("enc_ipv4_config: prefix = {}", &v.address.prefix_len()); v.address.prefix_len().enc(s)?; // to u32 - let gw = v.gateway.map(|a| a.to_bits()); + let gw = v.gateway.map(embassy_net::Ipv4Address::to_bits); enc_option(&gw, s)?; } Ok(()) @@ -300,7 +301,7 @@ impl SSHEncode for SSHStampConfig { fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> { enc_signkey(&self.hostkey, s)?; - for k in self.pubkeys.iter() { + for k in &self.pubkeys { enc_option(k, s)?; } @@ -331,7 +332,7 @@ impl<'de> SSHDecode<'de> for SSHStampConfig { let hostkey = dec_signkey(s)?; let mut pubkeys = [None; KEY_SLOTS]; - for k in pubkeys.iter_mut() { + for k in &mut pubkeys { *k = dec_option(s)?; } diff --git a/src/espressif/buffered_uart.rs b/src/espressif/buffered_uart.rs index de16521..fed5d7a 100644 --- a/src/espressif/buffered_uart.rs +++ b/src/espressif/buffered_uart.rs @@ -3,9 +3,9 @@ // SPDX-License-Identifier: GPL-3.0-or-later /// Wrapper around bidirectional embassy-sync Pipes, in order to handle UART -/// RX/RX happening in an InterruptExecutor at higher priority. +/// RX/RX happening in an `InterruptExecutor` at higher priority. /// -/// Doesn't implement the InterruptExecutor, in the task in the app should await +/// Doesn't implement the `InterruptExecutor`, in the task in the app should await /// the 'run' async function. /// use crate::config::SSHStampConfig; @@ -43,6 +43,7 @@ pub struct BufferedUart { pub struct UartConfig {} impl BufferedUart { + #[must_use] pub fn new() -> Self { BufferedUart { outward: Pipe::new(), @@ -54,7 +55,7 @@ impl BufferedUart { /// Transfer data between the UART and the buffer struct. /// /// This should be awaited from an Embassy task that's run - /// in an InterruptExecutor for lower latency. + /// in an `InterruptExecutor` for lower latency. pub async fn run(&self, uart: Uart<'_, Async>) { let (mut uart_rx, mut uart_tx) = uart.split(); let mut uart_rx_buf = [0u8; UART_BUF_SZ]; @@ -204,5 +205,5 @@ pub async fn uart_task( error!("Uart config error {e}. Resetting."); software_reset(); } - }; + } } diff --git a/src/espressif/net.rs b/src/espressif/net.rs index 02107e7..e6821d4 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -66,7 +66,7 @@ pub async fn if_up( } let mut pw = String::<63>::new(); - for &byte in rnd.iter() { + for &byte in &rnd { let _ = pw.push(WIFI_PASSWORD_CHARS[(byte as usize) % 62] as char); } @@ -101,7 +101,7 @@ pub async fn if_up( .with_password(AllocString::from(wifi_password(config).await.as_str())), ); let res = wifi_controller.set_config(&ap_config); - debug!("wifi_set_configuration returned {:?}", res); + debug!("wifi_set_configuration returned {res:?}"); let gw_ip_addr_ipv4 = *DEFAULT_IP; @@ -111,7 +111,7 @@ pub async fn if_up( dns_servers: Default::default(), }); - let seed = (rng.random() as u64) << 32 | rng.random() as u64; + let seed = u64::from(rng.random()) << 32 | u64::from(rng.random()); // Init network stack let (ap_stack, runner) = embassy_net::new( @@ -133,10 +133,7 @@ pub async fn if_up( Timer::after(Duration::from_millis(500)).await; } - info!( - "Connect to the AP `ssh-stamp` as a DHCP client with IP: {}", - gw_ip_addr_ipv4 - ); + info!("Connect to the AP `ssh-stamp` as a DHCP client with IP: {gw_ip_addr_ipv4}"); Ok(ap_stack) } @@ -168,7 +165,7 @@ pub async fn accept_requests<'a>( }) .await { - error!("connect error: {:?}", e); + error!("connect error: {e:?}"); // continue; tcp_socket_disable().await; } @@ -177,35 +174,35 @@ pub async fn accept_requests<'a>( tcp_socket } -/// Returns the configured WiFi SSID from the config, or the default SSID if not set. +/// Returns the configured `WiFi` SSID from the config, or the default SSID if not set. pub async fn wifi_ssid(config: &'static SunsetMutex) -> String<63> { let guard = config.lock().await; - let ssid_src = if !guard.wifi_ssid.is_empty() { - guard.wifi_ssid.as_str() - } else { + let ssid_src = if guard.wifi_ssid.is_empty() { DEFAULT_SSID + } else { + guard.wifi_ssid.as_str() }; - String::<63>::try_from(ssid_src).unwrap_or_else(|_| { + String::<63>::try_from(ssid_src).unwrap_or_else(|()| { let mut fallback = String::<63>::new(); fallback.push_str(DEFAULT_SSID).ok(); fallback }) } -/// Returns the WiFi password from the config. -/// Panics if wifi_pw is not set in the config. +/// Returns the `WiFi` password from the config. +/// Panics if `wifi_pw` is not set in the config. pub async fn wifi_password(config: &'static SunsetMutex) -> String<63> { let guard = config.lock().await; match &guard.wifi_pw { - Some(pw) => String::<63>::try_from(pw.as_str()).unwrap_or_else(|_| { + Some(pw) => String::<63>::try_from(pw.as_str()).unwrap_or_else(|()| { panic!("wifi_pw stored value exceeds 63 characters"); }), None => panic!("wifi_pw must be set before calling wifi_password()"), } } -/// Manages the WiFi access point lifecycle. +/// Manages the `WiFi` access point lifecycle. /// Starts the AP with the configured SSID and password from the config. /// Handles reconnection if the AP stops. #[embassy_executor::task] @@ -229,17 +226,17 @@ pub async fn wifi_up( if esp_radio::wifi::ap_state() == WifiApState::Started { wifi_controller.wait_for_event(WifiEvent::ApStop).await; - Timer::after(Duration::from_millis(5000)).await + Timer::after(Duration::from_millis(5000)).await; } if !matches!(wifi_controller.is_started(), Ok(true)) { if let Err(e) = wifi_controller.set_config(&client_config) { - debug!("Failed to set wifi config: {:?}", e); + debug!("Failed to set wifi config: {e:?}"); Timer::after(Duration::from_millis(1000)).await; continue; } debug!("Starting wifi"); if let Err(e) = wifi_controller.start_async().await { - debug!("Failed to start wifi: {:?}", e); + debug!("Failed to start wifi: {e:?}"); Timer::after(Duration::from_millis(1000)).await; continue; } @@ -263,7 +260,7 @@ use esp_radio::wifi::WifiDevice; #[embassy_executor::task] async fn net_up(mut runner: Runner<'static, WifiDevice<'static>>) { debug!("Bringing up network stack...\n"); - runner.run().await + runner.run().await; } #[embassy_executor::task] diff --git a/src/main.rs b/src/main.rs index 0a16760..f976bef 100644 --- a/src/main.rs +++ b/src/main.rs @@ -181,9 +181,9 @@ async fn main(spawner: Spawner) -> ! { }; match peripherals_enabled(peripherals_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("Peripheral error: {}", e); + error!("Peripheral error: {e}"); } } @@ -215,9 +215,9 @@ async fn peripherals_enabled(s: SshStampInit<'static>) -> Result<(), sunset::Err spawner: s.spawner, }; match wifi_controller_enabled(peripherals_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("Wifi controller error: {}", e); + error!("Wifi controller error: {e}"); } } @@ -243,9 +243,9 @@ pub async fn wifi_controller_enabled(s: PeripheralsEnabled<'static>) -> Result<( tcp_stack, }; match tcp_enabled(wifi_controller_enabled_stack).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("AP Stack error: {}", e); + error!("AP Stack error: {e}"); } } net::ap_stack_disable().await; @@ -292,9 +292,9 @@ async fn tcp_enabled<'a>(s: WifiControllerEnabled<'a>) -> Result<(), sunset::Err uart_buf: s.uart_buf, }; match socket_enabled(tcp_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("TCP socket error: {}", e); + error!("TCP socket error: {e}"); } } net::tcp_socket_disable().await; @@ -326,9 +326,9 @@ async fn socket_enabled<'a>(s: TCPEnabled<'a>) -> Result<(), sunset::Error> { uart_buf: s.uart_buf, }; match ssh_enabled(socket_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("SSH server error: {}", e); + error!("SSH server error: {e}"); } } @@ -367,9 +367,9 @@ async fn ssh_enabled<'a>(s: SocketEnabled<'a>) -> Result<(), sunset::Error> { connection_loop: connection, }; match client_connected(ssh_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - error!("Client connection error: {}", e); + error!("Client connection error: {e}"); } } @@ -406,9 +406,9 @@ where tcp_socket: s.tcp_socket, }; match bridge_connected(uart_enabled_struct).await { - Ok(_) => (), + Ok(()) => (), Err(e) => { - debug!("Bridge error: {}", e); + debug!("Bridge error: {e}"); } } diff --git a/src/serial.rs b/src/serial.rs index a447f93..0eb661a 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -31,7 +31,7 @@ async fn uart_to_ssh( let dropped = uart_buf.check_dropped_bytes(); if dropped > 0 { // TODO: should this also go to the SSH client? - warn!("UART RX dropped {} bytes", dropped); + warn!("UART RX dropped {dropped} bytes"); } let n = uart_buf.read(&mut ssh_tx_buf).await; chanw.write_all(&ssh_tx_buf[..n]).await?; diff --git a/src/serve.rs b/src/serve.rs index f2868ea..fa7d81d 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -114,9 +114,9 @@ pub async fn connection_loop( UART_SIGNAL.signal(1); debug!("Connection loop: UART_SIGNAL sent"); match chan_pipe.try_send(SessionType::Bridge(ch)) { - Ok(_) => auth_checked = false, - Err(e) => log::error!("Could not send the channel: {:?}", e), - }; + Ok(()) => auth_checked = false, + Err(e) => log::error!("Could not send the channel: {e:?}"), + } } else { a.fail()?; } @@ -223,12 +223,7 @@ pub async fn connection_loop( } "SSH_STAMP_WIFI_SSID" => { let mut config_guard = config.lock().await; - if !(auth_checked || config_guard.first_login) { - warn!( - "SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting" - ); - a.fail()?; - } else { + if auth_checked || config_guard.first_login { let mut s = String::<32>::new(); if s.push_str(a.value()?).is_ok() { config_guard.wifi_ssid = s; @@ -240,16 +235,16 @@ pub async fn connection_loop( warn!("SSH_STAMP_WIFI_SSID too long"); a.fail()?; } + } else { + warn!( + "SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting" + ); + a.fail()?; } } "SSH_STAMP_WIFI_PSK" => { let mut config_guard = config.lock().await; - if !(auth_checked || config_guard.first_login) { - warn!( - "SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting" - ); - a.fail()?; - } else { + if auth_checked || config_guard.first_login { let value = a.value()?; if value.len() < 8 { warn!("SSH_STAMP_WIFI_PSK too short (min 8 characters)"); @@ -270,23 +265,18 @@ pub async fn connection_loop( a.fail()?; } } + } else { + warn!( + "SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting" + ); + a.fail()?; } } "SSH_STAMP_WIFI_MAC_ADDRESS" => { let mut config_guard = config.lock().await; - if !(auth_checked || config_guard.first_login) { - warn!( - "SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting" - ); - a.fail()?; - } else { + if auth_checked || config_guard.first_login { let value = a.value()?; - if value.len() != 17 { - warn!( - "SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format" - ); - a.fail()?; - } else { + if value.len() == 17 { let parts: heapless::Vec = value .split(':') .filter_map(|p| u8::from_str_radix(p, 16).ok()) @@ -296,7 +286,7 @@ pub async fn connection_loop( parts[0], parts[1], parts[2], parts[3], parts[4], parts[5], ]; config_guard.mac = mac; - debug!("Set MAC address from ENV: {:02X?}", mac); + debug!("Set MAC address from ENV: {mac:02X?}"); a.succeed()?; config_changed = true; needs_reset = true; @@ -304,22 +294,32 @@ pub async fn connection_loop( warn!("SSH_STAMP_WIFI_MAC_ADDRESS invalid format"); a.fail()?; } + } else { + warn!( + "SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format" + ); + a.fail()?; } + } else { + warn!( + "SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting" + ); + a.fail()?; } } "SSH_STAMP_WIFI_MAC_RANDOM" => { let mut config_guard = config.lock().await; - if !(auth_checked || config_guard.first_login) { - warn!( - "SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting" - ); - a.fail()?; - } else { + if auth_checked || config_guard.first_login { config_guard.mac = [0xFF; 6]; debug!("Set MAC address to random mode"); a.succeed()?; config_changed = true; needs_reset = true; + } else { + warn!( + "SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting" + ); + a.fail()?; } } _ => { @@ -344,7 +344,7 @@ pub async fn connection_loop( } ServEvent::Defunct => { debug!("Expected caller to handle event"); - error::BadUsage.fail()? + error::BadUsage.fail()?; } ServEvent::PollAgain => {} } @@ -386,7 +386,7 @@ pub async fn handle_ssh_client<'a, 'b>( let stdio: ChanInOut<'_> = ssh_server.stdio(ch).await?; let (stdin, stdout) = stdio.split(); info!("Starting bridge"); - serial_bridge(stdin, stdout, uart_buff).await? + serial_bridge(stdin, stdout, uart_buff).await?; } #[cfg(feature = "sftp-ota")] SessionType::Sftp(ch) => { @@ -398,7 +398,7 @@ pub async fn handle_ssh_client<'a, 'b>( ota::run_ota_server::(stdio, ota_writer).await? } } - }; + } Ok(()) } diff --git a/src/store.rs b/src/store.rs index 78a5270..2c933c7 100644 --- a/src/store.rs +++ b/src/store.rs @@ -14,7 +14,7 @@ use crate::config::SSHStampConfig; use storage::flash::FlashBuffer; use sunset::sshwire::{self, OwnOrBorrow}; -use sunset_sshwire_derive::*; +use sunset_sshwire_derive::{SSHDecode, SSHEncode}; // TODO: [Nice to have] Read the right partition and write there instead of hardcoding offset and size. pub const CONFIG_VERSION_SIZE: usize = 4; @@ -44,7 +44,7 @@ impl FlashConfig<'_> { &mut fb.buf[..esp_bootloader_esp_idf::partitions::PARTITION_TABLE_MAX_LEN], ) .map_err(|e| { - error!("Failed to read partition table: {:?}", e); + error!("Failed to read partition table: {e:?}"); SSHStampError::FlashStorageError })?; @@ -73,7 +73,7 @@ fn config_hash(config: &SSHStampConfig) -> Result<[u8; 32], SunsetError> { Ok(h.finalize().into()) } -/// Loads a SSHConfig at startup. Good for persisting hostkeys. +/// Loads a `SSHConfig` at startup. Good for persisting hostkeys. pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result { match load(flash).await { Ok(c) => { @@ -162,12 +162,12 @@ pub async fn save(fl: &mut FlashBuffer<'_>, config: &SSHStampConfig) -> Result<( (CONFIG_OFFSET + CONFIG_AREA_SIZE) as u32, ) .map_err(|e| { - error!("flash erase error: {:?}", e); + error!("flash erase error: {e:?}"); SunsetError::msg("flash erase error") })?; fl.flash.write(CONFIG_OFFSET as u32, &fl.buf).map_err(|e| { - error!("flash write error: {:?}", e); + error!("flash write error: {e:?}"); SunsetError::msg("flash write error") })?; From 960b4bf67d83446b47c0e14d7d1861d5c9521285 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Thu, 2 Apr 2026 00:07:32 +0200 Subject: [PATCH 02/10] Second pedantic clippy pass... the right cmdline seems to be: cargo clippy --features esp32c6 -- -W clippy::mem_forget -W clippy::await_holding_lock -W clippy::large_futures -W clippy::pedantic Notice mem_forget and await_holding_lock, according to the currently available lints at the time of writing this: https://rust-lang.github.io/rust-clippy/master/index.html --- ota/src/handler.rs | 438 ++++++++++++++++----------------- ota/src/sftpserver.rs | 90 +++---- ota/src/tlv.rs | 71 +++--- src/config.rs | 37 +-- src/espressif/buffered_uart.rs | 25 +- src/espressif/net.rs | 25 +- src/espressif/rng.rs | 7 + src/main.rs | 28 ++- src/serial.rs | 16 +- src/serve.rs | 38 ++- src/store.rs | 19 ++ storage/src/esp_ota.rs | 44 ++-- storage/src/lib.rs | 13 +- 13 files changed, 444 insertions(+), 407 deletions(-) diff --git a/ota/src/handler.rs b/ota/src/handler.rs index b7dcd08..ecafc9a 100644 --- a/ota/src/handler.rs +++ b/ota/src/handler.rs @@ -9,12 +9,12 @@ use crate::{OtaHeader, otatraits::OtaActions, tlv}; use log::{debug, error, info, warn}; use sha2::{Digest, Sha256}; -/// UpdateProcessorState for OTA update processing +/// `UpdateProcessorState` for OTA update processing /// /// This enum defines the various states of the OTA update processing state machine and will control the flow of the update process. #[derive(Debug)] enum UpdateProcessorState { - /// ReadingParameters state, OTA has started and the processor is obtaining metadata values until the firmware blob is reached + /// `ReadingParameters` state, OTA has started and the processor is obtaining metadata values until the firmware blob is reached ReadingParameters { // tlv_holder: [u8; tlv::MAX_TLV_SIZE as usize], // current_len: usize, @@ -36,11 +36,11 @@ impl Default for UpdateProcessorState { } } -/// # UpdateProcessor for handling OTA update processing +/// # `UpdateProcessor` for handling OTA update processing /// /// This struct manages the state and processing of OTA updates received via SFTP. It will handle reading metadata, writing data, verifying, and applying updates. /// -/// It uses an internal state machine defined by [[UpdateProcessorState]] to track the progress of the update process. +/// It uses an internal state machine defined by [[`UpdateProcessorState`]] to track the progress of the update process. /// /// It will also handle incoming data chunks and process them accordingly. pub(crate) struct UpdateProcessor { @@ -54,9 +54,9 @@ pub(crate) struct UpdateProcessor { } impl UpdateProcessor { - /// Creates a new UpdateProcessor instance with the given OtaActions implementation + /// Creates a new `UpdateProcessor` instance with the given `OtaActions` implementation /// - /// Use this ota_writer to perform platform-specific OTA actions + /// Use this `ota_writer` to perform platform-specific OTA actions pub fn new(ota_writer: W) -> Self { Self { state: UpdateProcessorState::default(), @@ -74,13 +74,13 @@ impl UpdateProcessor { /// Main processing function for handling incoming data chunks /// - /// It processes data based on the current state of the update processor [[UpdateProcessorState]]. To first, read most metadata parameters, after that, write the data to the appropriate location. as it is received. + /// It processes data based on the current state of the update processor [[`UpdateProcessorState`]]. To first, read most metadata parameters, after that, write the data to the appropriate location. as it is received. /// /// It will try to consume as much data as possible from the provided buffer and return the number of bytes used. - pub async fn process_data(&mut self, _offset: u64, data: &[u8]) -> Result<(), OtaError> { + pub async fn process_data(&mut self, offset: u64, data: &[u8]) -> Result<(), OtaError> { debug!( "UpdateProcessor: Processing data chunk at offset {}, length {} in state {:?}", - _offset, + offset, data.len(), self.state ); @@ -89,243 +89,223 @@ impl UpdateProcessor { debug!("processor state : {:?}", self.state); match self.state { - UpdateProcessorState::ReadingParameters { - // mut tlv_holder, - // mut current_len, - } => { - match source.try_taking_bytes_for_tlv(&mut self.tlv_holder, &mut self.current_len) { - Err(WireError::RanOut) => { - // Not enough data to complete TLV, wait for more data - self.state = UpdateProcessorState::ReadingParameters { - // tlv_holder, - // current_len, - }; - return Ok(()); - } - Err(e) => { - error!("Error processing TLV: {:?}", e); - return Err(OtaError::InternalError); - } - Ok(_) => { - // Successfully read a TLV, continue processing - } - }; - - // At this point there should be a complete TLV to be decoded - debug!( - "Decoding TLV from tlv_holder: {:?}, current_len: {}", - &self.tlv_holder, &self.current_len - ); - let mut singular_source = tlv::TlvsSource::new(&self.tlv_holder[..self.current_len]); - - match tlv::Tlv::dec(&mut singular_source) { - Ok(tlv) => match tlv { - tlv::Tlv::OtaType { ota_type } => { - if ota_type != tlv::OTA_TYPE_VALUE_SSH_STAMP { - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - info!("Received Ota type: {:?}", ota_type); - self.header.ota_type = Some(ota_type); - self.tlv_holder.fill(0); - self.current_len = 0; - // self.state = UpdateProcessorState::ReadingParameters { - // tlv_holder: [0; tlv::MAX_TLV_SIZE as usize], - // current_len: 0, - // }; - } - - tlv::Tlv::Sha256Checksum { checksum } => { - info!("Received Checksum: {:?}", &checksum); - if self.header.ota_type.is_none() { - error!( - "UpdateProcessor: Received SHA256 Checksum TLV before OTA Type TLV" - ); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - self.header.sha256_checksum = Some(checksum); - self.tlv_holder.fill(0); - self.current_len = 0; - } - tlv::Tlv::FirmwareBlob { size } => { - info!("Received FirmwareBlob size: {:?}", size); - if self.header.ota_type.is_none() { - error!( - "UpdateProcessor: Received SHA256 Checksum TLV before OTA Type TLV" - ); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - - if self.header.sha256_checksum.is_none() { - error!( - "UpdateProcessor: Received FirmwareBlob TLV before SHA256 Checksum TLV" - ); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - let max_size = W::get_ota_partition_size() - .await - .map_err(|_| OtaError::InternalError)?; - if size > max_size { - error!( - "UpdateProcessor: Firmware blob size {} exceeds OTA partition size {}", - size, max_size - ); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - self.header.firmware_blob_size = Some(size); - - info!("Starting OTA update"); - self.state = UpdateProcessorState::Downloading { - total_received_size: 0, - }; - info!("Transitioning to Downloading state"); - } - }, - Err(WireError::UnknownPacket { number }) => { - if self.header.ota_type.is_none() { - error!( - "UpdateProcessor: Received unknown TLV type before OTA Type TLV" - ); - self.state = - UpdateProcessorState::Error(OtaError::IllegalOperation); - return Err(OtaError::IllegalOperation); - } - error!( - "UpdateProcessor: Unknown TLV type encountered: {}", - number - ); - - // A crafted TLV with an arbitrary length could be used to confuse the UpdateProcessor - // leading to an unknown behaviour. To avoid this, unknown TLVs will thrown an error - return Err(OtaError::UnknownTlvType); - - } - Err(WireError::RanOut) => { - // Keep current data and wait for more - self.tlv_holder.fill(0); - self.current_len = 0; - - error!("UpdateProcessor: RanOut should not be happening"); - return Err(OtaError::MoreDataRequired); - } - Err(e) => { - error!("Handle {:?} appropriately", e); - return Err(OtaError::InternalError); - } - } + UpdateProcessorState::ReadingParameters { .. } => { + self.process_reading_parameters(&mut source).await?; } UpdateProcessorState::Downloading { mut total_received_size, } => { - let total_blob_size = match self.header.firmware_blob_size { - Some(size) => size, - None => { - error!( - "UpdateProcessor: Firmware blob size not set before downloading" - ); - return Err(OtaError::IllegalOperation); - } - }; - debug!("source contains {} bytes", source.remaining()); - // Once the totality of the blob has been received, the FSM must move to the Finished or Error States - if total_received_size >= total_blob_size { - error!( - "UpdateProcessor: Received more data than expected: received_size = {}, total_blob_size = {}", - total_received_size, total_blob_size - ); - return Err(OtaError::IllegalOperation); - } - - let to_take = source - .remaining() - .min((total_blob_size - total_received_size) as usize); - - let data_chunk = source.take(to_take).map_err(|e| { - error!( - "UpdateProcessor: Error taking data chunk of size {}: {:?}", - to_take, e - ); - OtaError::InternalError - })?; - - self.hasher.update(data_chunk); - - debug!( - "Writing {} bytes to flash at offset {}", - data_chunk.len(), - total_received_size - ); - self.ota_writer.write_ota_data(total_received_size, data_chunk).await.map_err(|e| { - error!( - "UpdateProcessor: Error writing data chunk to flash at offset {}: {:?}", - total_received_size, e - ); - OtaError::WriteError - })?; - - total_received_size += to_take as u32; - - if total_received_size >= total_blob_size { - let Some(original_hash) = self.header.sha256_checksum else { - error!( - "UpdateProcessor: No original checksum to verify against after download" - ); - return Err(OtaError::IllegalOperation); - }; - - let computed = self.hasher.clone().finalize(); - if &original_hash[..] != computed.as_slice() { - error!( - "UpdateProcessor: Checksum mismatch after download! Expected: {:x?}`", - original_hash - ); - self.state = UpdateProcessorState::Error(OtaError::VerificationFailed); - return Ok(()); - } else { - info!("UpdateProcessor: Checksum verified successfully"); - } - - info!("All firmware data received, transitioning to Finished state"); - self.state = UpdateProcessorState::Finished {}; - } else { - self.state = UpdateProcessorState::Downloading { - total_received_size, - }; - } + self.process_downloading(&mut source, &mut total_received_size) + .await?; } UpdateProcessorState::Finished {} => { - // Will ignore the data. It will be consumed and the file will be closed eventually - // This behaviour will allow any future file footer (e.g. signature?) to be discarded - // without causing problems warn!( "UpdateProcessor: Received data in Finished state, ignoring additional data" ); return Ok(()); } UpdateProcessorState::Error(ota_error) => { - // Will ignore the data. It will be consumed and the file will be closed eventually warn!( - "UpdateProcessor: Received data in Error state: {:?}, ignoring additional data", - ota_error + "UpdateProcessor: Received data in Error state: {ota_error:?}, ignoring additional data" ); return Ok(()); } + } + } + Ok(()) + } + + async fn process_reading_parameters( + &mut self, + source: &mut tlv::TlvsSource<'_>, + ) -> Result<(), OtaError> { + match source.try_taking_bytes_for_tlv(&mut self.tlv_holder, &mut self.current_len) { + Err(WireError::RanOut) => { + self.state = UpdateProcessorState::ReadingParameters {}; + return Ok(()); + } + Err(e) => { + error!("Error processing TLV: {e:?}"); + return Err(OtaError::InternalError); + } + Ok(()) => {} + } + + debug!( + "Decoding TLV from tlv_holder: {:?}, current_len: {}", + &self.tlv_holder, &self.current_len + ); + let mut singular_source = tlv::TlvsSource::new(&self.tlv_holder[..self.current_len]); + + match tlv::Tlv::dec(&mut singular_source) { + Ok(tlv) => self.handle_tlv(tlv).await?, + Err(WireError::UnknownPacket { number }) => { + if self.header.ota_type.is_none() { + error!("UpdateProcessor: Received unknown TLV type before OTA Type TLV"); + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + error!("UpdateProcessor: Unknown TLV type encountered: {number}"); + return Err(OtaError::UnknownTlvType); + } + Err(WireError::RanOut) => { + self.tlv_holder.fill(0); + self.current_len = 0; + error!("UpdateProcessor: RanOut should not be happening"); + return Err(OtaError::MoreDataRequired); + } + Err(e) => { + error!("Handle {e:?} appropriately"); + return Err(OtaError::InternalError); + } + } + Ok(()) + } + + async fn handle_tlv(&mut self, tlv: tlv::Tlv) -> Result<(), OtaError> { + match tlv { + tlv::Tlv::OtaType { ota_type } => { + if ota_type != tlv::OTA_TYPE_VALUE_SSH_STAMP { + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + info!("Received Ota type: {ota_type:?}"); + self.header.ota_type = Some(ota_type); + self.tlv_holder.fill(0); + self.current_len = 0; + } + tlv::Tlv::Sha256Checksum { checksum } => { + info!("Received Checksum: {checksum:?}"); + if self.header.ota_type.is_none() { + error!("UpdateProcessor: Received SHA256 Checksum TLV before OTA Type TLV"); + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + self.header.sha256_checksum = Some(checksum); + self.tlv_holder.fill(0); + self.current_len = 0; + } + tlv::Tlv::FirmwareBlob { size } => { + self.handle_firmware_blob(size).await?; + } + } + Ok(()) + } + + async fn handle_firmware_blob(&mut self, size: u32) -> Result<(), OtaError> { + info!("Received FirmwareBlob size: {size:?}"); + if self.header.ota_type.is_none() { + error!("UpdateProcessor: Received FirmwareBlob TLV before OTA Type TLV"); + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + + if self.header.sha256_checksum.is_none() { + error!("UpdateProcessor: Received FirmwareBlob TLV before SHA256 Checksum TLV"); + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + let max_size = W::get_ota_partition_size() + .await + .map_err(|_| OtaError::InternalError)?; + if size > max_size { + error!( + "UpdateProcessor: Firmware blob size {size} exceeds OTA partition size {max_size}" + ); + self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); + return Err(OtaError::IllegalOperation); + } + self.header.firmware_blob_size = Some(size); + + info!("Starting OTA update"); + self.state = UpdateProcessorState::Downloading { + total_received_size: 0, + }; + info!("Transitioning to Downloading state"); + Ok(()) + } + + async fn process_downloading( + &mut self, + source: &mut tlv::TlvsSource<'_>, + total_received_size: &mut u32, + ) -> Result<(), OtaError> { + let Some(total_blob_size) = self.header.firmware_blob_size else { + error!("UpdateProcessor: Firmware blob size not set before downloading"); + return Err(OtaError::IllegalOperation); + }; + debug!("source contains {} bytes", source.remaining()); + + if *total_received_size >= total_blob_size { + error!( + "UpdateProcessor: Received more data than expected: received_size = {total_received_size}, total_blob_size = {total_blob_size}" + ); + return Err(OtaError::IllegalOperation); + } + + let to_take = source + .remaining() + .min((total_blob_size - *total_received_size) as usize); + + let data_chunk = source.take(to_take).map_err(|e| { + error!("UpdateProcessor: Error taking data chunk of size {to_take}: {e:?}"); + OtaError::InternalError + })?; + + self.hasher.update(data_chunk); + + debug!( + "Writing {} bytes to flash at offset {}", + data_chunk.len(), + *total_received_size + ); + self.ota_writer + .write_ota_data(*total_received_size, data_chunk) + .await + .map_err(|e| { + error!( + "UpdateProcessor: Error writing data chunk to flash at offset {}: {e:?}", + *total_received_size + ); + OtaError::WriteError + })?; + + *total_received_size += u32::try_from(to_take).map_err(|_| { + error!("UpdateProcessor: Data chunk size overflow"); + OtaError::InternalError + })?; + + if *total_received_size >= total_blob_size { + self.verify_checksum()?; + info!("All firmware data received, transitioning to Finished state"); + self.state = UpdateProcessorState::Finished {}; + } else { + self.state = UpdateProcessorState::Downloading { + total_received_size: *total_received_size, }; } Ok(()) } + fn verify_checksum(&mut self) -> Result<(), OtaError> { + let Some(original_hash) = self.header.sha256_checksum else { + error!("UpdateProcessor: No original checksum to verify against after download"); + return Err(OtaError::IllegalOperation); + }; + + let computed = self.hasher.clone().finalize(); + if original_hash.as_slice() == computed.as_slice() { + info!("UpdateProcessor: Checksum verified successfully"); + } else { + error!( + "UpdateProcessor: Checksum mismatch after download! Expected: {original_hash:x?}`" + ); + self.state = UpdateProcessorState::Error(OtaError::VerificationFailed); + return Ok(()); + } + Ok(()) + } + /// Finalizes the OTA update process /// /// This function should be called once all data has been processed. @@ -336,12 +316,12 @@ impl UpdateProcessor { info!("Finalizing OTA update process successfully."); self.ota_writer.finalize_ota_update().await.map_err(|e| { - error!("Error finalizing OTA update: {:?}", e); + error!("Error finalizing OTA update: {e:?}"); OtaError::InternalError }) } UpdateProcessorState::Error(e) => { - error!("Cannot finalize OTA update due to error state: {:?}", e); + error!("Cannot finalize OTA update due to error state: {e:?}"); Err(e) } _ => { @@ -349,7 +329,7 @@ impl UpdateProcessor { "Cannot finalize OTA update, current state is not Finished: {:?}", self.state ); - Err(OtaError::IllegalOperation) // Illegal finalizing in error state + Err(OtaError::IllegalOperation) } }; @@ -376,7 +356,7 @@ impl UpdateProcessor { } #[derive(Debug, Copy, Clone, PartialEq, Eq)] -/// OtaError for OTA update processing errors +/// `OtaError` for OTA update processing errors pub(crate) enum OtaError { /// Needs more data to proceed MoreDataRequired, diff --git a/ota/src/sftpserver.rs b/ota/src/sftpserver.rs index e9ebc50..3885cfe 100644 --- a/ota/src/sftpserver.rs +++ b/ota/src/sftpserver.rs @@ -18,6 +18,10 @@ use sunset_sftp::{ use log::{debug, error, info, warn}; use rustc_hash::FxHasher; +/// Runs the OTA SFTP server +/// +/// # Errors +/// Returns an error if the SFTP server loop encounters an error pub async fn run_ota_server( stdio: ChanInOut<'_>, ota_writer: W, @@ -34,12 +38,12 @@ pub async fn run_ota_server( .process_loop(stdio, &mut buffer_in) .await { - Ok(_) => { + Ok(()) => { debug!("sftp server loop finished gracefully"); Ok(()) } Err(e) => { - warn!("sftp server loop finished with an error: {:?}", &e); + warn!("sftp server loop finished with an error: {e:?}"); Err(e.into()) } } @@ -49,7 +53,7 @@ pub async fn run_ota_server( /// while still providing a reasonable level of uniqueness. /// We are not expecting more than one OTA operation at a time. const OPAQUE_HASH_LEN: usize = 4; -/// OtaOpaqueFileHandle for OTA SFTP server +/// `OtaOpaqueFileHandle` for OTA SFTP server /// /// Minimal implementation of an opaque file handle with a tiny hash #[derive(Hash, Debug, Eq, PartialEq, Clone)] @@ -62,8 +66,9 @@ impl OpaqueFileHandle for OtaOpaqueFileHandle { fn new(seed: &str) -> Self { let mut hasher = FxHasher::default(); hasher.write(seed.as_bytes()); + let hash_bytes = u32::try_from(hasher.finish()).unwrap_or(0).to_be_bytes(); OtaOpaqueFileHandle { - tiny_hash: (hasher.finish() as u32).to_be_bytes(), + tiny_hash: hash_bytes, } } @@ -89,7 +94,7 @@ impl OpaqueFileHandle for OtaOpaqueFileHandle { /// SFTP server implementation for OTA updates /// -/// This struct implements the SftpServer trait for handling OTA updates over SFTP +/// This struct implements the `SftpServer` trait for handling OTA updates over SFTP /// For now, all methods log an error and return unsupported operation as this is a placeholder struct SftpOtaServer { // Add fields as necessary for OTA server state @@ -109,7 +114,7 @@ impl SftpOtaServer { } } -impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer { +impl SftpServer<'_, T> for SftpOtaServer { async fn open( &'_ mut self, path: &str, @@ -131,8 +136,7 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer Ok(handle) } else { error!( - "SftpServer Open operation failed: already writing OTA, path = {:?}, attrs = {:?}", - path, mode + "SftpServer Open operation failed: already writing OTA, path = {path:?}, attrs = {mode:?}" ); Err(sunset_sftp::protocol::StatusCode::SSH_FX_PERMISSION_DENIED) } @@ -141,40 +145,31 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer async fn close(&mut self, handle: &T) -> sunset_sftp::server::SftpOpResult<()> { // TODO: At this point I need to reset the target if all is ok or reset the processor if not so we are // either loading a new firmware or ready to receive a correct one. - info!("Close called for handle {:?}", handle); + info!("Close called for handle {handle:?}"); if let Some(current_handle) = &self.file_handle { if current_handle == handle { let ret_val = match self.processor.finalize().await { - Ok(_) => { + Ok(()) => { info!("OTA update finalized successfully."); self.processor.reset_device(); Ok(()) } Err(e) => { - error!("OTA update finalization failed: {:?}", e); + error!("OTA update finalization failed: {e:?}"); Err(sunset_sftp::protocol::StatusCode::SSH_FX_FAILURE) } }; - info!( - "SftpServer Close operation for OTA completed: handle = {:?}", - handle - ); + info!("SftpServer Close operation for OTA completed: handle = {handle:?}"); self.file_handle = None; self.write_permission = false; ret_val } else { - warn!( - "SftpServer Close operation failed: handle mismatch = {:?}", - handle - ); + warn!("SftpServer Close operation failed: handle mismatch = {handle:?}"); Err(sunset_sftp::protocol::StatusCode::SSH_FX_FAILURE) } } else { - warn!( - "SftpServer Close operation granted on untracked handle: {:?}", - handle - ); + warn!("SftpServer Close operation granted on untracked handle: {handle:?}"); Ok(()) } } @@ -188,8 +183,7 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer _reply: &mut sunset_sftp::server::ReadReply<'_, N>, ) -> sunset_sftp::error::SftpResult<()> { error!( - "SftpServer Read operation not defined: handle = {:?}, offset = {:?}, len = {:?}", - opaque_file_handle, offset, len + "SftpServer Read operation not defined: handle = {opaque_file_handle:?}, offset = {offset:?}, len = {len:?}" ); Err(sunset_sftp::error::SftpError::FileServerError( sunset_sftp::protocol::StatusCode::SSH_FX_OP_UNSUPPORTED, @@ -207,8 +201,7 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer { if !self.write_permission { warn!( - "SftpServer Write operation denied: no write permission for handle = {:?}", - opaque_file_handle + "SftpServer Write operation denied: no write permission for handle = {opaque_file_handle:?}" ); return Err(sunset_sftp::protocol::StatusCode::SSH_FX_PERMISSION_DENIED); } @@ -223,41 +216,30 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer match e { crate::handler::OtaError::IllegalOperation => { error!( - "SftpServer Write operation failed during OTA processing: Illegal Operation - {:?}", - e + "SftpServer Write operation failed during OTA processing: Illegal Operation - {e:?}" ); return Err(sunset_sftp::protocol::StatusCode::SSH_FX_PERMISSION_DENIED); } crate::handler::OtaError::UnknownTlvType => { error!( - "SftpServer Write operation failed during OTA processing: Unknown TLV Type - {:?}", - e + "SftpServer Write operation failed during OTA processing: Unknown TLV Type - {e:?}" ); return Err(sunset_sftp::protocol::StatusCode::SSH_FX_OP_UNSUPPORTED); } _ => { - error!( - "SftpServer Write operation failed during OTA processing: {:?}", - e - ); + error!("SftpServer Write operation failed during OTA processing: {e:?}"); return Err(sunset_sftp::protocol::StatusCode::SSH_FX_FAILURE); } } - } else { - debug!( - "SftpServer Write operation for OTA processed successfully: handle = {:?}, offset = {:?}, buf_len = {:?}", - opaque_file_handle, - offset, - buf.len() - ); - return Ok(()); } + debug!( + "SftpServer Write operation for OTA processed successfully: handle = {opaque_file_handle:?}, offset = {offset:?}, buf_len = {:?}", + buf.len() + ); + return Ok(()); } - warn!( - "SftpServer Write operation failed: handle mismatch = {:?}", - opaque_file_handle - ); + warn!("SftpServer Write operation failed: handle mismatch = {opaque_file_handle:?}"); Err(sunset_sftp::protocol::StatusCode::SSH_FX_FAILURE) } @@ -273,19 +255,16 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer // For OTA, we do not expect any directory listing async fn readdir( &mut self, - _opaque_dir_handle: &T, + opaque_dir_handle: &T, _reply: &mut sunset_sftp::server::DirReply<'_, N>, ) -> sunset_sftp::server::SftpOpResult<()> { - info!( - "SftpServer ReadDir called for OTA SFTP server on handle: {:?}", - _opaque_dir_handle - ); + info!("SftpServer ReadDir called for OTA SFTP server on handle: {opaque_dir_handle:?}"); Err(sunset_sftp::protocol::StatusCode::SSH_FX_EOF) } // For OTA, realpath will always return root async fn realpath(&mut self, dir: &str) -> sunset_sftp::server::SftpOpResult> { - info!("SftpServer RealPath: dir = {:?}", dir); + info!("SftpServer RealPath: dir = {dir:?}"); Ok(NameEntry { filename: Filename::from("/"), _longname: Filename::from("/"), @@ -300,9 +279,8 @@ impl<'a, T: OpaqueFileHandle, W: OtaActions> SftpServer<'a, T> for SftpOtaServer file_path: &str, ) -> sunset_sftp::server::SftpOpResult { error!( - "SftpServer Stats operation not defined: follow_link = {:?}, \ - file_path = {:?}", - follow_links, file_path + "SftpServer Stats operation not defined: follow_link = {follow_links:?}, \ + file_path = {file_path:?}" ); Err(sunset_sftp::protocol::StatusCode::SSH_FX_OP_UNSUPPORTED) } diff --git a/ota/src/tlv.rs b/ota/src/tlv.rs index 5a86337..3384b6b 100644 --- a/ota/src/tlv.rs +++ b/ota/src/tlv.rs @@ -19,13 +19,11 @@ pub type OtaTlvType = u8; pub type OtaTlvLen = u8; // TODO: We could provide a new type for better debugging information -pub const OTA_TYPE_VALUE_SSH_STAMP: u32 = 0x73736873; // 'sshs' big endian in ASCII +pub const OTA_TYPE_VALUE_SSH_STAMP: u32 = 0x7373_6873; pub const CHECKSUM_LEN: u32 = 32; /// Maximum size for LTV (Length-Type-Value) entries in OTA metadata. Used during the reading of OTA parameters. -pub const MAX_TLV_SIZE: u32 = (core::mem::size_of::() - + core::mem::size_of::() - + u8::MAX as usize) as u32; // type + length + value +pub const MAX_TLV_SIZE: u32 = 257; // size_of::() + size_of::() + u8::MAX = 1 + 1 + 255 /// Encodes the length and value of a sized values fn enc_len_val( @@ -35,7 +33,9 @@ fn enc_len_val( where SE: Sized + SSHEncode, { - (core::mem::size_of::() as OtaTlvLen).enc(s)?; + OtaTlvLen::try_from(core::mem::size_of::()) + .map_err(|_| sunset::sshwire::WireError::PacketWrong)? + .enc(s)?; value.enc(s) } @@ -48,7 +48,9 @@ where SE: Sized, { let val_len = OtaTlvLen::dec(s)?; - if val_len != (core::mem::size_of::() as OtaTlvLen) { + let expected_len = OtaTlvLen::try_from(core::mem::size_of::()) + .map_err(|_| sunset::sshwire::WireError::PacketWrong)?; + if val_len != expected_len { return Err(sunset::sshwire::WireError::PacketWrong); } Ok(()) @@ -60,14 +62,14 @@ pub const OTA_TYPE: OtaTlvType = 0; pub const FIRMWARE_BLOB: OtaTlvType = 1; pub const SHA256_CHECKSUM: OtaTlvType = 2; -/// OTA_TLV enum for OTA metadata LTV entries +/// `OTA_TLV` enum for OTA metadata LTV entries /// This TLV does not capture length as it will be captured during parsing /// Parsing will be done using sshwire types #[derive(Debug)] -#[repr(u8)] // Must match the type of OtaTlvType +#[repr(u8)] pub enum Tlv { /// Type of OTA update. This **MUST be the first Tlv**. - /// For SSH Stamp, this must be OTA_FIRMWARE_BLOB_TYPE + /// For SSH Stamp, this must be `OTA_FIRMWARE_BLOB_TYPE` OtaType { ota_type: u32 }, /// Expected SHA256 checksum of the firmware blob Sha256Checksum { @@ -110,13 +112,15 @@ impl<'de> SSHDecode<'de> for Tlv { Ok(Tlv::FirmwareBlob { size: u32::dec(s)? }) } SHA256_CHECKSUM => { - if OtaTlvLen::dec(s)? != tlv::CHECKSUM_LEN as u8 { + let expected_len = OtaTlvLen::try_from(tlv::CHECKSUM_LEN) + .map_err(|_| sunset::sshwire::WireError::PacketWrong)?; + if OtaTlvLen::dec(s)? != expected_len { return Err(sunset::sshwire::WireError::PacketWrong); } let mut checksum = [0u8; tlv::CHECKSUM_LEN as usize]; - checksum.iter_mut().for_each(|element| { + for element in &mut checksum { *element = u8::dec(s).unwrap_or(0); - }); + } Ok(Tlv::Sha256Checksum { checksum }) } OTA_TYPE => { @@ -124,19 +128,17 @@ impl<'de> SSHDecode<'de> for Tlv { let ota_type = u32::dec(s)?; Ok(Tlv::OtaType { ota_type }) } - // To handle unknown TLVs, it consumes the announced len - // and returns an UnknownVariant error _ => { - error!("Unknown TLV type encountered: {}", tlv_type); + error!("Unknown TLV type encountered: {tlv_type}"); let len = OtaTlvLen::dec(s)?; - s.take(len as usize)?; // Skip unknown TLV value + s.take(len as usize)?; Err(sunset::sshwire::WireError::UnknownPacket { number: tlv_type }) } }) } } -/// An implementation of SSHSource based on [[sunset::sshwire::DecodeBytes]] +/// An implementation of `SSHSource` based on [[`sunset::sshwire::DecodeBytes`]] /// pub struct TlvsSource<'a> { remaining_buf: &'a [u8], @@ -145,6 +147,7 @@ pub struct TlvsSource<'a> { } impl<'a> TlvsSource<'a> { + #[must_use] pub fn new(buf: &'a [u8]) -> Self { Self { remaining_buf: buf, @@ -153,17 +156,18 @@ impl<'a> TlvsSource<'a> { } } + #[must_use] pub fn used(&self) -> usize { self.used } - /// Puts bytes in the tlv_holder and updates current_len until an OTA TLV enum variant can be decoded + /// Puts bytes in the `tlv_holder` and updates `current_len` until an OTA TLV enum variant can be decoded /// - /// Even if it fails, it adds bytes to the tlv_holder and updates current_len accordingly + /// Even if it fails, it adds bytes to the `tlv_holder` and updates `current_len` accordingly /// so more data can be added later to complete the TLV /// - /// If more data is required, it returns WireError::RanOut - /// If successful, it returns Ok(()) and a dec - // TODO: Add test for RanOut and acomplete TLV + /// # Errors + /// - If more data is required, it returns `WireError::RanOut` + /// - If successful, it returns Ok(()) and a dec pub fn try_taking_bytes_for_tlv( &mut self, tlv_holder: &mut [u8], @@ -175,7 +179,7 @@ impl<'a> TlvsSource<'a> { let needed = core::mem::size_of::() + core::mem::size_of::() - *current_len; - debug!("Adding {} bytes to have up to TLV type and length", needed); + debug!("Adding {needed} bytes to have up to TLV type and length"); let to_read = core::cmp::min(needed, self.remaining()); let type_len_bytes = self.take(to_read)?; tlv_holder[*current_len..*current_len + to_read].copy_from_slice(type_len_bytes); @@ -269,7 +273,8 @@ impl OtaHeader { /// Serializes the OTA header into the provided buffer /// /// Returns the number of bytes written to the buffer - // #[cfg(not(target_os = "none"))] // Maybe I should remove this from embedded side as well + /// # Panics + /// Panics if the TLV serialization fails pub fn serialize(&self, buf: &mut [u8]) -> usize { let mut offset = 0; if let Some(ota_type) = self.ota_type { @@ -299,6 +304,8 @@ impl OtaHeader { /// /// This approach requires that the whole header is contained in the buffer. An incomplete /// header will result in unpopulated fields. + /// # Errors + /// Returns a `WireError` if the buffer contains invalid data pub fn deserialize(buf: &[u8]) -> Result<(Self, usize), sunset::sshwire::WireError> { let mut source = tlv::TlvsSource::new(buf); let mut ota_type = None; @@ -309,11 +316,8 @@ impl OtaHeader { match tlv::Tlv::dec(&mut source) { Err(sunset::sshwire::WireError::UnknownPacket { number }) => { warn!( - "Unknown packet type encountered: {}. TLV skipping it and continuing", - number + "Unknown packet type encountered: {number}. TLV skipping it and continuing" ); - // Unknown TLV was skipped already in the decoder - continue; } Err(e) => { return Err(e); @@ -350,12 +354,11 @@ impl OtaHeader { } fn check_ota_is_first_tlv(ota_type: Option) -> Result<(), WireError> { - match ota_type.is_none() { - true => { - error!("SHA256 Checksum TLV encountered before OTA Type TLV. Ignoring it"); - Err(sunset::sshwire::WireError::PacketWrong) - } - false => Ok(()), + if ota_type.is_none() { + error!("SHA256 Checksum TLV encountered before OTA Type TLV. Ignoring it"); + Err(sunset::sshwire::WireError::PacketWrong) + } else { + Ok(()) } } } diff --git a/src/config.rs b/src/config.rs index 5a5688c..6b256b8 100644 --- a/src/config.rs +++ b/src/config.rs @@ -77,6 +77,8 @@ impl SSHStampConfig { } /// Get the MAC address to use (resolves random sentinel) + /// # Errors + /// Returns an error if the RNG fails pub fn resolve_mac(&self) -> Result<[u8; 6]> { if self.is_mac_random() { random_mac() @@ -87,6 +89,7 @@ impl SSHStampConfig { /// Creates a new config with default parameters. /// + /// # Errors /// Will only fail on RNG failure. pub fn new() -> Result { let hostkey = SignKey::generate(KeyType::Ed25519, None)?; @@ -185,7 +188,7 @@ fn enc_signkey(k: &SignKey, s: &mut dyn SSHSink) -> WireResult<()> { // need to add a variant field if we support more key types. match k { SignKey::Ed25519(k) => k.to_bytes().enc(s), - _ => Err(WireError::UnknownVariant), + SignKey::AgentEd25519(_) => Err(WireError::UnknownVariant), } } @@ -199,9 +202,12 @@ where } // encode Option as a bool then maybe a value -pub(crate) fn enc_option(v: &Option, s: &mut dyn SSHSink) -> WireResult<()> { +pub(crate) fn enc_option(v: Option<&T>, s: &mut dyn SSHSink) -> WireResult<()> { v.is_some().enc(s)?; - v.enc(s) + if let Some(v) = v { + v.enc(s)?; + } + Ok(()) } pub(crate) fn dec_option<'de, S, T: SSHDecode<'de>>(s: &mut S) -> WireResult> @@ -213,7 +219,7 @@ where // encode Option> as a bool then the &str contents (heapless::String doesn't implement SSHEncode) pub(crate) fn enc_option_str( - v: &Option>, + v: Option<&String>, s: &mut dyn SSHSink, ) -> WireResult<()> { v.is_some().enc(s)?; @@ -223,27 +229,27 @@ pub(crate) fn enc_option_str( Ok(()) } -fn enc_ipv4_config(v: &Option, s: &mut dyn SSHSink) -> WireResult<()> { +fn enc_ipv4_config(v: Option<&StaticConfigV4>, s: &mut dyn SSHSink) -> WireResult<()> { v.is_some().enc(s)?; if let Some(v) = v { v.address.address().to_bits().enc(s)?; debug!("enc_ipv4_config: prefix = {}", &v.address.prefix_len()); v.address.prefix_len().enc(s)?; // to u32 - let gw = v.gateway.map(embassy_net::Ipv4Address::to_bits); - enc_option(&gw, s)?; + let gw = v.gateway.as_ref().map(|g| g.to_bits()); + enc_option(gw.as_ref(), s)?; } Ok(()) } #[cfg(feature = "ipv6")] -fn enc_ipv6_config(v: &Option, s: &mut dyn SSHSink) -> WireResult<()> { +fn enc_ipv6_config(v: Option<&StaticConfigV6>, s: &mut dyn SSHSink) -> WireResult<()> { v.is_some().enc(s)?; if let Some(v) = v { v.address.address().to_bits().enc(s)?; v.address.prefix_len().enc(s)?; - let gw = v.gateway.map(|a| a.to_bits()); - enc_option(&gw, s)?; + let gw = v.gateway.as_ref().map(|g| g.to_bits()); + enc_option(gw.as_ref(), s)?; } Ok(()) } @@ -266,6 +272,7 @@ where Ok(StaticConfigV4 { address: Ipv4Cidr::new(ad, prefix), gateway, + #[allow(clippy::default_trait_access)] dns_servers: Default::default(), }) }) @@ -291,7 +298,7 @@ where Ok(StaticConfigV6 { address: Ipv6Cidr::new(ad, prefix), gateway, - dns_servers: Vec::new(), + dns_servers: Default::default(), }) }) .transpose() @@ -302,16 +309,16 @@ impl SSHEncode for SSHStampConfig { enc_signkey(&self.hostkey, s)?; for k in &self.pubkeys { - enc_option(k, s)?; + enc_option(k.as_ref(), s)?; } self.wifi_ssid.as_str().enc(s)?; - enc_option_str::<63>(&self.wifi_pw, s)?; + enc_option_str::<63>(self.wifi_pw.as_ref(), s)?; self.mac.enc(s)?; - enc_ipv4_config(&self.ipv4_static, s)?; + enc_ipv4_config(self.ipv4_static.as_ref(), s)?; #[cfg(feature = "ipv6")] - enc_ipv6_config(&self.ipv6_static, s)?; + enc_ipv6_config(self.ipv6_static.as_ref(), s)?; // Encode UartPins self.uart_pins.rx.enc(s)?; diff --git a/src/espressif/buffered_uart.rs b/src/espressif/buffered_uart.rs index fed5d7a..0f71102 100644 --- a/src/espressif/buffered_uart.rs +++ b/src/espressif/buffered_uart.rs @@ -58,15 +58,15 @@ impl BufferedUart { /// in an `InterruptExecutor` for lower latency. pub async fn run(&self, uart: Uart<'_, Async>) { let (mut uart_rx, mut uart_tx) = uart.split(); - let mut uart_rx_buf = [0u8; UART_BUF_SZ]; - let mut uart_tx_buf = [0u8; UART_BUF_SZ]; + let mut rx_buf = [0u8; UART_BUF_SZ]; + let mut tx_buf = [0u8; UART_BUF_SZ]; loop { let rd_from = async { loop { // Note: println! is intentionally avoided here as this runs in an // InterruptExecutor at high priority. Blocking I/O would cause scheduler panics. - let n = match uart_rx.read_async(&mut uart_rx_buf).await { + let n = match uart_rx.read_async(&mut rx_buf).await { Ok(n) => n, Err(e) => { warn!("Uart RxError: {e}"); @@ -74,7 +74,7 @@ impl BufferedUart { } }; - let mut rx_slice = &uart_rx_buf[..n]; + let mut rx_slice = &rx_buf[..n]; // Write rx_slice to 'inward' pipe, dropping bytes rather than blocking if // the pipe is full @@ -102,10 +102,10 @@ impl BufferedUart { }; let rd_to = async { loop { - let n = self.outward.read(&mut uart_tx_buf).await; + let n = self.outward.read(&mut tx_buf).await; // TODO: handle write errors - let _n = match uart_tx.write_async(&uart_tx_buf[..n]).await { - Ok(_n) => _n, + let _n = match uart_tx.write_async(&tx_buf[..n]).await { + Ok(n) => n, Err(e) => { warn!("Uart TxError: {e}"); continue; // Re-write if write error has occured - Contents of the buffer have not been modified. @@ -142,14 +142,16 @@ impl Default for BufferedUart { } } -pub async fn uart_buffer_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn uart_buffer_disable() { // disable uart buffer debug!("UART buffer disabled: WIP"); // TODO: Correctly disable/restart UART buffer and/or send messsage to user over SSH } // use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex; -pub async fn uart_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn uart_disable() { // disable uart debug!("UART disabled: WIP"); // TODO: Correctly disable/restart UART and/or send messsage to user over SSH @@ -171,6 +173,7 @@ pub struct UartPins<'a> { pub static UART_BUF: StaticCell = StaticCell::new(); pub static UART_SIGNAL: Signal = Signal::new(); +#[allow(clippy::unused_async)] pub async fn uart_buffer_wait_for_initialisation() -> &'static BufferedUart { UART_BUF.init_with(BufferedUart::new) } @@ -179,9 +182,11 @@ pub async fn uart_buffer_wait_for_initialisation() -> &'static BufferedUart { pub async fn uart_task( uart_buf: &'static BufferedUart, uart1: UART1<'static>, - _config: &'static SunsetMutex, + config: &'static SunsetMutex, pins: UartPins<'static>, ) { + // Config is reserved for future use in parameter reconfiguration + let _ = config; // Note: dbg!/println! avoided throughout as this task runs in an InterruptExecutor // at high priority where blocking I/O can cause scheduler panics. diff --git a/src/espressif/net.rs b/src/espressif/net.rs index e6821d4..9b3acb7 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -44,6 +44,13 @@ macro_rules! mk_static { }}; } +/// Brings up the `WiFi` interface. +/// +/// # Errors +/// Returns an error if the `WiFi` configuration or initialization fails. +/// +/// # Panics +/// Panics if flash storage is not initialized or if persisting the wifi password fails. pub async fn if_up( spawner: Spawner, controller: Controller<'static>, @@ -53,7 +60,7 @@ pub async fn if_up( ) -> Result, sunset::Error> { let wifi_init = &*mk_static!(Controller<'static>, controller); let (mut wifi_controller, interfaces) = - esp_radio::wifi::new(wifi_init, wifi, Default::default()) + esp_radio::wifi::new(wifi_init, wifi, Config::default()) .map_err(|_| sunset::error::BadUsage.build())?; // Ensure WiFi PSK exists before applying AP config to avoid esp_wifi_set_config errors @@ -108,6 +115,7 @@ pub async fn if_up( let net_config = embassy_net::Config::ipv4_static(StaticConfigV4 { address: Ipv4Cidr::new(gw_ip_addr_ipv4, 24), gateway: Some(gw_ip_addr_ipv4), + #[allow(clippy::default_trait_access)] dns_servers: Default::default(), }); @@ -138,13 +146,15 @@ pub async fn if_up( Ok(ap_stack) } -pub async fn ap_stack_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn ap_stack_disable() { // drop ap_stack debug!("AP Stack disabled: WIP"); // TODO: Correctly disable/restart AP Stack and/or send messsage to user over SSH } -pub async fn tcp_socket_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn tcp_socket_disable() { // drop tcp stack debug!("TCP socket disabled: WIP"); // TODO: Correctly disable/restart tcp socket and/or send messsage to user over SSH @@ -191,7 +201,9 @@ pub async fn wifi_ssid(config: &'static SunsetMutex) -> String<6 } /// Returns the `WiFi` password from the config. -/// Panics if `wifi_pw` is not set in the config. +/// +/// # Panics +/// Panics if `wifi_pw` is not set in the config or exceeds 63 characters. pub async fn wifi_password(config: &'static SunsetMutex) -> String<63> { let guard = config.lock().await; match &guard.wifi_pw { @@ -246,7 +258,8 @@ pub async fn wifi_up( } } -pub async fn wifi_controller_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn wifi_controller_disable() { // TODO: Correctly disable wifi controller // pub async fn wifi_disable(wifi_controller: EspWifiController<'_>) -> (){ // drop wifi controller @@ -256,7 +269,7 @@ pub async fn wifi_controller_disable() -> () { //software_reset(); } -use esp_radio::wifi::WifiDevice; +use esp_radio::wifi::{Config, WifiDevice}; #[embassy_executor::task] async fn net_up(mut runner: Runner<'static, WifiDevice<'static>>) { debug!("Bringing up network stack...\n"); diff --git a/src/espressif/rng.rs b/src/espressif/rng.rs index 00b8ce0..1b45cb7 100644 --- a/src/espressif/rng.rs +++ b/src/espressif/rng.rs @@ -22,6 +22,13 @@ pub fn register_custom_rng(rng: Rng) { // esp-hal specific variation of getrandom custom function as seen in: // https://github.com/rust-random/getrandom/issues/340 +/// Custom getrandom function for ESP-HAL. +/// +/// # Errors +/// Returns an error if the RNG mutex lock fails. +/// +/// # Panics +/// Panics if `register_custom_rng` was not called before this function. pub fn esp_getrandom_custom_func(buf: &mut [u8]) -> Result<(), getrandom::Error> { RNG_MUTEX.lock(|t| { let mut rng = t.borrow_mut(); diff --git a/src/main.rs b/src/main.rs index f976bef..e5723f5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -20,6 +20,9 @@ use ssh_stamp::{ use ota::otatraits::OtaActions; use storage::flash; +extern crate alloc; +use alloc::boxed::Box; + use sunset_async::{SSHServer, SunsetMutex}; use core::future::Future; @@ -48,7 +51,8 @@ cfg_if::cfg_if! { } } -pub async fn peripherals_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn peripherals_disable() { // drop peripherals debug!("Disabling peripherals: WIP"); } @@ -180,7 +184,7 @@ async fn main(spawner: Spawner) -> ! { uart_buf, }; - match peripherals_enabled(peripherals_enabled_struct).await { + match Box::pin(peripherals_enabled(peripherals_enabled_struct)).await { Ok(()) => (), Err(e) => { error!("Peripheral error: {e}"); @@ -214,7 +218,7 @@ async fn peripherals_enabled(s: SshStampInit<'static>) -> Result<(), sunset::Err uart_buf: s.uart_buf, spawner: s.spawner, }; - match wifi_controller_enabled(peripherals_enabled_struct).await { + match Box::pin(wifi_controller_enabled(peripherals_enabled_struct)).await { Ok(()) => (), Err(e) => { error!("Wifi controller error: {e}"); @@ -232,6 +236,10 @@ pub struct WifiControllerEnabled<'a> { pub tcp_stack: Stack<'a>, } +/// Enables `WiFi` controller and network stack. +/// +/// # Errors +/// Returns an error if `WiFi` initialization fails. pub async fn wifi_controller_enabled(s: PeripheralsEnabled<'static>) -> Result<(), sunset::Error> { debug!("HSM: wifi_controller_enabled"); let tcp_stack = net::if_up(s.spawner, s.controller, s.wifi, s.rng, s.config).await?; @@ -242,7 +250,7 @@ pub async fn wifi_controller_enabled(s: PeripheralsEnabled<'static>) -> Result<( uart_buf: s.uart_buf, tcp_stack, }; - match tcp_enabled(wifi_controller_enabled_stack).await { + match Box::pin(tcp_enabled(wifi_controller_enabled_stack)).await { Ok(()) => (), Err(e) => { error!("AP Stack error: {e}"); @@ -259,7 +267,7 @@ pub struct TCPEnabled<'a> { } cfg_if::cfg_if!(if #[cfg(feature = "esp32")] {use embassy_net::IpListenEndpoint;}); -async fn tcp_enabled<'a>(s: WifiControllerEnabled<'a>) -> Result<(), sunset::Error> { +async fn tcp_enabled(s: WifiControllerEnabled<'_>) -> Result<(), sunset::Error> { debug!("HSM: tcp_enabled"); let mut rx_buffer = [0u8; 1536]; @@ -291,7 +299,7 @@ async fn tcp_enabled<'a>(s: WifiControllerEnabled<'a>) -> Result<(), sunset::Err tcp_socket, uart_buf: s.uart_buf, }; - match socket_enabled(tcp_enabled_struct).await { + match Box::pin(socket_enabled(tcp_enabled_struct)).await { Ok(()) => (), Err(e) => { error!("TCP socket error: {e}"); @@ -309,7 +317,7 @@ pub struct SocketEnabled<'a> { pub uart_buf: &'a BufferedUart, } -async fn socket_enabled<'a>(s: TCPEnabled<'a>) -> Result<(), sunset::Error> { +async fn socket_enabled(s: TCPEnabled<'_>) -> Result<(), sunset::Error> { debug!("HSM: socket_enabled"); // loop { // Spawn network tasks to handle incoming connections with demo_common::session() @@ -349,7 +357,7 @@ where pub connection_loop: CL, } -async fn ssh_enabled<'a>(s: SocketEnabled<'a>) -> Result<(), sunset::Error> { +async fn ssh_enabled(s: SocketEnabled<'_>) -> Result<(), sunset::Error> { debug!("HSM: ssh_enabled"); // loop { debug!("HSM: Starting channel pipe"); @@ -434,9 +442,7 @@ where let bridge = s.bridge; debug!("HSM: Main select() in bridge_connected()"); match select3(server, connection_loop, bridge).await { - Either3::First(r) => r, - Either3::Second(r) => r, - Either3::Third(r) => r, + Either3::First(r) | Either3::Second(r) | Either3::Third(r) => r, } } diff --git a/src/serial.rs b/src/serial.rs index 0eb661a..bdf47dd 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -11,20 +11,22 @@ use crate::espressif::buffered_uart::BufferedUart; /// Forwards an incoming SSH connection to/from the local UART, until /// the connection drops +/// # Errors +/// Returns an error if the SSH connection fails pub async fn serial_bridge( - chanr: impl Read, - chanw: impl Write, + chan_read: impl Read, + chan_write: impl Write, uart: &BufferedUart, ) -> Result<(), sunset::Error> { info!("Starting serial <--> SSH bridge"); - select(uart_to_ssh(uart, chanw), ssh_to_uart(chanr, uart)).await; + select(uart_to_ssh(uart, chan_write), ssh_to_uart(chan_read, uart)).await; debug!("Stopping serial <--> SSH bridge"); Ok(()) } async fn uart_to_ssh( uart_buf: &BufferedUart, - mut chanw: impl Write, + mut chan_write: impl Write, ) -> Result<(), sunset::Error> { let mut ssh_tx_buf = [0u8; 512]; loop { @@ -34,17 +36,17 @@ async fn uart_to_ssh( warn!("UART RX dropped {dropped} bytes"); } let n = uart_buf.read(&mut ssh_tx_buf).await; - chanw.write_all(&ssh_tx_buf[..n]).await?; + chan_write.write_all(&ssh_tx_buf[..n]).await?; } } async fn ssh_to_uart( - mut chanr: impl Read, + mut chan_read: impl Read, uart_buf: &BufferedUart, ) -> Result<(), sunset::Error> { let mut uart_tx_buf = [0u8; 64]; loop { - let n = chanr.read(&mut uart_tx_buf).await?; + let n = chan_read.read(&mut uart_tx_buf).await?; if n == 0 { return Err(sunset::Error::ChannelEOF); } diff --git a/src/serve.rs b/src/serve.rs index fa7d81d..81e4531 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -37,6 +37,14 @@ fn key_signature_ok(a: &ServPubkeyAuth<'_, '_>) -> bool { a.real() } +/// Handles the SSH connection loop, processing events from clients. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail. +/// +/// # Panics +/// Panics if flash storage lock cannot be acquired when saving configuration. +#[allow(clippy::too_many_lines)] pub async fn connection_loop( serv: &SSHServer<'_>, chan_pipe: &Channel, @@ -164,9 +172,7 @@ pub async fn connection_loop( // A client that sends only a pubkey query (no signature) gets the flag set. // Sunset's own ConnState gate prevents this from being exploited since channels // can't open until full auth completes, but the flag being set prematurely is - // a latent bug. if that gate ever changes or a new code path reads auth_checked - // in a pre-auth context, it could become a real bypass. If that is indeed an - // issue then maybe stamps should check if a.real() before setting auth_checked = true. + // a latent bug. if that is indeed an issue then maybe stamps should check if a.real() before setting auth_checked = true. let sig_checked = key_signature_ok(&a); auth_checked = sig_checked; a.allow()?; @@ -175,7 +181,7 @@ pub async fn connection_loop( a.reject()?; } } - _ => { + sunset::packets::PubKey::Unknown(_) => { // Only Ed25519 keys supported a.reject()?; } @@ -351,11 +357,13 @@ pub async fn connection_loop( } } -pub async fn connection_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn connection_disable() { debug!("Connection loop disabled: WIP"); // TODO: Correctly disable/restart Conection loop and/or send messsage to user over SSH } +#[allow(clippy::unused_async)] pub async fn ssh_wait_for_initialisation<'server>( inbuf: &'server mut [u8; UART_BUFFER_SIZE], outbuf: &'server mut [u8; UART_BUFFER_SIZE], @@ -363,7 +371,8 @@ pub async fn ssh_wait_for_initialisation<'server>( SSHServer::new(inbuf, outbuf) } -pub async fn ssh_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn ssh_disable() { debug!("SSH Server disabled: WIP"); // TODO: Correctly disable/restart SSH Server and/or send messsage to user over SSH } @@ -372,6 +381,10 @@ use crate::espressif::buffered_uart::BufferedUart; use crate::serial::serial_bridge; use sunset_async::ChanInOut; +/// Handles an SSH client connection, bridging UART and SSH. +/// +/// # Errors +/// Returns an error if SSH protocol operations or I/O fail. pub async fn handle_ssh_client<'a, 'b>( uart_buff: &'a BufferedUart, ssh_server: &'b SSHServer<'a>, @@ -383,26 +396,27 @@ pub async fn handle_ssh_client<'a, 'b>( match session_type { SessionType::Bridge(ch) => { info!("Handling bridge session"); - let stdio: ChanInOut<'_> = ssh_server.stdio(ch).await?; - let (stdin, stdout) = stdio.split(); + let chan_io: ChanInOut<'_> = ssh_server.stdio(ch).await?; + let (input, output) = chan_io.split(); info!("Starting bridge"); - serial_bridge(stdin, stdout, uart_buff).await?; + serial_bridge(input, output, uart_buff).await?; } #[cfg(feature = "sftp-ota")] SessionType::Sftp(ch) => { { debug!("Handling SFTP session"); - let stdio = ssh_server.stdio(ch).await?; + let sftp_io = ssh_server.stdio(ch).await?; // TODO: Use a configuration flag to select the hardware specific OtaActions implementer let ota_writer = storage::esp_ota::OtaWriter::new(); - ota::run_ota_server::(stdio, ota_writer).await? + ota::run_ota_server::(sftp_io, ota_writer).await? } } } Ok(()) } -pub async fn bridge_disable() -> () { +#[allow(clippy::unused_async)] +pub async fn bridge_disable() { // disable bridge debug!("Bridge disabled: WIP"); // TODO: Correctly disable/restart bridge and/or send message to user over SSH diff --git a/src/store.rs b/src/store.rs index 2c933c7..104d15b 100644 --- a/src/store.rs +++ b/src/store.rs @@ -74,6 +74,9 @@ fn config_hash(config: &SSHStampConfig) -> Result<[u8; 32], SunsetError> { } /// Loads a `SSHConfig` at startup. Good for persisting hostkeys. +/// +/// # Errors +/// Returns an error if flash read fails or config is invalid. pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result { match load(flash).await { Ok(c) => { @@ -86,6 +89,10 @@ pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result) -> Result { let c = SSHStampConfig::new()?; save(flash, &c).await?; @@ -94,6 +101,12 @@ pub async fn create(flash: &mut FlashBuffer<'_>) -> Result) -> Result { // If at some point you target a 64bit arch these can truncate and cause // corruption of the bootloader or the ota partition. @@ -128,6 +141,12 @@ pub async fn load(fl: &mut FlashBuffer<'_>) -> Result, config: &SSHStampConfig) -> Result<(), SunsetError> { let sc = FlashConfig { version: SSHStampConfig::CURRENT_VERSION, diff --git a/storage/src/esp_ota.rs b/storage/src/esp_ota.rs index 6e17459..a70260d 100644 --- a/storage/src/esp_ota.rs +++ b/storage/src/esp_ota.rs @@ -13,9 +13,10 @@ use ota::otatraits::{OtaActions, StorageError, StorageResult}; pub struct OtaWriter {} impl OtaWriter { - /// Creates a new OtaWriter for the given target OTA slot. + /// Creates a new `OtaWriter` for the given target OTA slot. /// - /// To obtain a target OTA slot use [get_next_app_slot] + /// To obtain a target OTA slot use [`get_next_app_slot`] + #[must_use] pub fn new() -> Self { OtaWriter {} } @@ -50,11 +51,11 @@ impl OtaActions for OtaWriter { let mut ota = esp_bootloader_esp_idf::ota_updater::OtaUpdater::new(storage, &mut buff_ota) .map_err(|e| { - error!("Could not create OtaUpdater: {:?}", e); + error!("Could not create OtaUpdater: {e:?}"); StorageError::InternalError })?; let current = ota.selected_partition().map_err(|e| { - error!("Could not get selected partition: {:?}", e); + error!("Could not get selected partition: {e:?}"); StorageError::InternalError })?; @@ -62,7 +63,7 @@ impl OtaActions for OtaWriter { "current image state {:?} (only relevant if the bootloader was built with auto-rollback support)", ota.current_ota_state() ); - debug!("currently selected partition {:?}", current); + debug!("currently selected partition {current:?}"); if let Ok(state) = ota.current_ota_state() && (state == esp_bootloader_esp_idf::ota::OtaImageState::New @@ -70,7 +71,7 @@ impl OtaActions for OtaWriter { { ota.set_current_ota_state(esp_bootloader_esp_idf::ota::OtaImageState::Valid) .map_err(|e| { - error!("Could not set OTA image state to Valid: {:?}", e); + error!("Could not set OTA image state to Valid: {e:?}"); StorageError::WriteError })?; debug!("Changed state to VALID"); @@ -118,11 +119,11 @@ async fn next_ota_size() -> StorageResult { let mut ota = esp_bootloader_esp_idf::ota_updater::OtaUpdater::new(storage, &mut buff_ota) .map_err(|e| { - error!("Could not create OtaUpdater: {:?}", e); + error!("Could not create OtaUpdater: {e:?}"); StorageError::InternalError })?; let (target_partition, _) = ota.next_partition().map_err(|e| { - error!("Could not get next partition: {:?}", e); + error!("Could not get next partition: {e:?}"); StorageError::InternalError })?; @@ -148,23 +149,22 @@ async fn write_to_target(offset: u32, data: &[u8]) -> StorageResult<()> { let mut ota = esp_bootloader_esp_idf::ota_updater::OtaUpdater::new(storage, &mut buff_ota) .map_err(|e| { - error!("Could not create OtaUpdater: {:?}", e); + error!("Could not create OtaUpdater: {e:?}"); StorageError::InternalError })?; let (mut target_partition, part_type) = ota.next_partition().map_err(|e| { - error!("Could not get next partition: {:?}", e); + error!("Could not get next partition: {e:?}"); StorageError::InternalError })?; - debug!("Flashing image to {:?}", part_type); + debug!("Flashing image to {part_type:?}"); debug!( - "Writing data to target_partition at offset {}, with len {}", - offset, + "Writing data to target_partition at offset {offset}, with len {}", data.len() ); target_partition.write(offset, data).map_err(|e| { - error!("Failed to write data to target_partition: {}", e); + error!("Failed to write data to target_partition: {e}"); StorageError::WriteError })?; @@ -190,17 +190,17 @@ async fn activate_next_ota_slot() -> StorageResult<()> { let mut ota = esp_bootloader_esp_idf::ota_updater::OtaUpdater::new(storage, &mut buff_ota) .map_err(|e| { - error!("Could not create OtaUpdater: {:?}", e); + error!("Could not create OtaUpdater: {e:?}"); StorageError::InternalError })?; ota.activate_next_partition().map_err(|e| { - error!("Could not activate next partition: {:?}", e); + error!("Could not activate next partition: {e:?}"); StorageError::WriteError })?; ota.set_current_ota_state(esp_bootloader_esp_idf::ota::OtaImageState::New) .map_err(|e| { - error!("Could not set OTA image state to New: {:?}", e); + error!("Could not set OTA image state to New: {e:?}"); StorageError::WriteError })?; @@ -214,6 +214,8 @@ async fn activate_next_ota_slot() -> StorageResult<()> { /// Mark the current OTA slot as VALID - this is only needed if the bootloader was built with auto-rollback support. /// The default pre-compiled bootloader in espflash is NOT. /// +/// # Errors +/// Returns a `StorageError` if flash storage is not initialized or if OTA operations fail pub async fn try_validating_current_ota_partition() -> StorageResult<()> { // Taken from [esp-hal ota_update example](https://github.com/esp-rs/esp-hal/examples/src/bin/ota_update.rs) let Some(fb) = flash::get_flash_n_buffer() else { @@ -230,11 +232,11 @@ pub async fn try_validating_current_ota_partition() -> StorageResult<()> { let mut ota = esp_bootloader_esp_idf::ota_updater::OtaUpdater::new(storage, &mut buff_ota) .map_err(|e| { - error!("Could not create OtaUpdater: {:?}", e); + error!("Could not create OtaUpdater: {e:?}"); StorageError::InternalError })?; let current = ota.selected_partition().map_err(|e| { - error!("Could not get selected partition: {:?}", e); + error!("Could not get selected partition: {e:?}"); StorageError::InternalError })?; @@ -242,7 +244,7 @@ pub async fn try_validating_current_ota_partition() -> StorageResult<()> { "current image state {:?} (only relevant if the bootloader was built with auto-rollback support)", ota.current_ota_state() ); - debug!("currently selected partition {:?}", current); + debug!("currently selected partition {current:?}"); if let Ok(state) = ota.current_ota_state() && (state == esp_bootloader_esp_idf::ota::OtaImageState::New @@ -250,7 +252,7 @@ pub async fn try_validating_current_ota_partition() -> StorageResult<()> { { ota.set_current_ota_state(esp_bootloader_esp_idf::ota::OtaImageState::Valid) .map_err(|e| { - error!("Could not set OTA image state to Valid: {:?}", e); + error!("Could not set OTA image state to Valid: {e:?}"); StorageError::WriteError })?; debug!("Changed state to VALID"); diff --git a/storage/src/lib.rs b/storage/src/lib.rs index c458d48..1edcaec 100644 --- a/storage/src/lib.rs +++ b/storage/src/lib.rs @@ -14,10 +14,10 @@ pub mod esp_ota; // TODO: When the time comes, generalise the flash so it can be used with all supported targets -/// [[flash]] is a packet to provide safe access to the Flash storage used by SSH-Stamp +/// [`flash`] is a packet to provide safe access to the Flash storage used by SSH-Stamp /// -/// It does so by storing the FlashStorage and a buffer for read/write operations in a single structure -/// protected by a SunsetMutex for safe concurrent access in async contexts. +/// It does so by storing the `FlashStorage` and a buffer for read/write operations in a single structure +/// protected by a `SunsetMutex` for safe concurrent access in async contexts. pub mod flash { use esp_hal::peripherals::FLASH; use esp_storage::FlashStorage; @@ -29,10 +29,10 @@ pub mod flash { use once_cell::sync::OnceCell; use sunset_async::SunsetMutex; - /// A structure that holds both the FlashStorage and a buffer for read/write operations + /// A structure that holds both the `FlashStorage` and a buffer for read/write operations /// /// The buffer is stored here to avoid allocating multiple buffers in different parts of the code. - /// It has a fixed size defined by FLASH_BUF_SIZE. + /// It has a fixed size defined by `FLASH_BUF_SIZE`. #[derive(Debug)] pub struct FlashBuffer<'d> { pub flash: FlashStorage<'d>, @@ -40,6 +40,7 @@ pub mod flash { } impl<'d> FlashBuffer<'d> { + #[must_use] pub fn new(flash: FlashStorage<'static>) -> Self { Self { flash, @@ -57,7 +58,7 @@ pub mod flash { /// /// Calls to [`with_flash`] or [`get_flash`] will initialize the flash storage if not already done. /// - /// Multiple calls to init() are safe and will have no effect after the first one. + /// Multiple calls to `init()` are safe and will have no effect after the first one. pub fn init(flash: FLASH<'static>) { let fl = FlashBuffer::new(FlashStorage::new(flash)); From 1bbc6cd951d02bbb925adad88df0fff4b6d1da28 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Thu, 2 Apr 2026 00:13:20 +0200 Subject: [PATCH 03/10] Tentatively adopt pedantic clippy lints on CI, we'll need to adopt https://github.com/j178/prek on our dev machines after this PR is merged, otherwise they'll be a fair bit of CI breakage and potential DX frustration --- .github/workflows/build.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index f8d8654..8f2d71d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -52,7 +52,8 @@ jobs: - name: Check lints and format if: ${{ contains(fromJson('["esp32c6"]'), matrix.device.soc) }} run: | - cargo +${{ matrix.device.toolchain }} clippy --release --features ${{ matrix.device.soc }} --target riscv32imac-unknown-none-elf -- -D warnings + #cargo +${{ matrix.device.toolchain }} clippy --release --features ${{ matrix.device.soc }} --target riscv32imac-unknown-none-elf -- -D warnings + cargo +${{ matrix.device.toolchain }} clippy --release --features ${{ matrix.device.soc }} --target riscv32imac-unknown-none-elf -- -W clippy::mem_forget -W clippy::await_holding_lock -W clippy::large_futures -W clippy::pedantic -D warnings cargo +${{ matrix.device.toolchain }} fmt -- --check ota-packer: name: OTA Packer From 2b1dc8c9223b51feecc896016596ab009b7d993c Mon Sep 17 00:00:00 2001 From: brainstorm Date: Thu, 2 Apr 2026 00:34:33 +0200 Subject: [PATCH 04/10] Last clippy pass, removing the allow macros and assuming more concrete types instead of usize --- ota/src/tlv.rs | 10 ++++--- src/config.rs | 3 +- src/espressif/buffered_uart.rs | 6 ++-- src/espressif/net.rs | 16 ++++------- src/main.rs | 23 +++++++-------- src/serve.rs | 22 ++++++-------- src/store.rs | 52 ++++++++++++++++------------------ 7 files changed, 59 insertions(+), 73 deletions(-) diff --git a/ota/src/tlv.rs b/ota/src/tlv.rs index 3384b6b..6f1927e 100644 --- a/ota/src/tlv.rs +++ b/ota/src/tlv.rs @@ -19,11 +19,13 @@ pub type OtaTlvType = u8; pub type OtaTlvLen = u8; // TODO: We could provide a new type for better debugging information -pub const OTA_TYPE_VALUE_SSH_STAMP: u32 = 0x7373_6873; +pub const OTA_TYPE_VALUE_SSH_STAMP: u32 = 0x7373_6873; // 'sshs' big endian in ASCII pub const CHECKSUM_LEN: u32 = 32; /// Maximum size for LTV (Length-Type-Value) entries in OTA metadata. Used during the reading of OTA parameters. -pub const MAX_TLV_SIZE: u32 = 257; // size_of::() + size_of::() + u8::MAX = 1 + 1 + 255 +/// +/// Calculated as: `size_of::() + size_of::() + u8::MAX = 1 + 1 + 255 = 257` +pub const MAX_TLV_SIZE: u32 = 257; /// Encodes the length and value of a sized values fn enc_len_val( @@ -66,7 +68,7 @@ pub const SHA256_CHECKSUM: OtaTlvType = 2; /// This TLV does not capture length as it will be captured during parsing /// Parsing will be done using sshwire types #[derive(Debug)] -#[repr(u8)] +#[repr(u8)] // Must match the type of OtaTlvType pub enum Tlv { /// Type of OTA update. This **MUST be the first Tlv**. /// For SSH Stamp, this must be `OTA_FIRMWARE_BLOB_TYPE` @@ -131,7 +133,7 @@ impl<'de> SSHDecode<'de> for Tlv { _ => { error!("Unknown TLV type encountered: {tlv_type}"); let len = OtaTlvLen::dec(s)?; - s.take(len as usize)?; + s.take(len as usize)?; // Skip unknown TLV value Err(sunset::sshwire::WireError::UnknownPacket { number: tlv_type }) } }) diff --git a/src/config.rs b/src/config.rs index 6b256b8..9595248 100644 --- a/src/config.rs +++ b/src/config.rs @@ -272,8 +272,7 @@ where Ok(StaticConfigV4 { address: Ipv4Cidr::new(ad, prefix), gateway, - #[allow(clippy::default_trait_access)] - dns_servers: Default::default(), + dns_servers: heapless::Vec::new(), }) }) .transpose() diff --git a/src/espressif/buffered_uart.rs b/src/espressif/buffered_uart.rs index 0f71102..c032591 100644 --- a/src/espressif/buffered_uart.rs +++ b/src/espressif/buffered_uart.rs @@ -142,16 +142,14 @@ impl Default for BufferedUart { } } -#[allow(clippy::unused_async)] -pub async fn uart_buffer_disable() { +pub fn uart_buffer_disable() { // disable uart buffer debug!("UART buffer disabled: WIP"); // TODO: Correctly disable/restart UART buffer and/or send messsage to user over SSH } // use embassy_sync::blocking_mutex::raw::CriticalSectionRawMutex; -#[allow(clippy::unused_async)] -pub async fn uart_disable() { +pub fn uart_disable() { // disable uart debug!("UART disabled: WIP"); // TODO: Correctly disable/restart UART and/or send messsage to user over SSH diff --git a/src/espressif/net.rs b/src/espressif/net.rs index 9b3acb7..01ab8e1 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -84,7 +84,7 @@ pub async fn if_up( panic!("Flash storage not initialized; cannot persist wifi password"); }; let mut flash_storage = flash_storage_guard.lock().await; - if let Err(e) = store::save(&mut flash_storage, &guard).await { + if let Err(e) = store::save(&mut flash_storage, &guard) { panic!("Failed to persist generated wifi password: {:?}", e); } } @@ -115,8 +115,7 @@ pub async fn if_up( let net_config = embassy_net::Config::ipv4_static(StaticConfigV4 { address: Ipv4Cidr::new(gw_ip_addr_ipv4, 24), gateway: Some(gw_ip_addr_ipv4), - #[allow(clippy::default_trait_access)] - dns_servers: Default::default(), + dns_servers: heapless::Vec::new(), }); let seed = u64::from(rng.random()) << 32 | u64::from(rng.random()); @@ -146,15 +145,13 @@ pub async fn if_up( Ok(ap_stack) } -#[allow(clippy::unused_async)] -pub async fn ap_stack_disable() { +pub fn ap_stack_disable() { // drop ap_stack debug!("AP Stack disabled: WIP"); // TODO: Correctly disable/restart AP Stack and/or send messsage to user over SSH } -#[allow(clippy::unused_async)] -pub async fn tcp_socket_disable() { +pub fn tcp_socket_disable() { // drop tcp stack debug!("TCP socket disabled: WIP"); // TODO: Correctly disable/restart tcp socket and/or send messsage to user over SSH @@ -177,7 +174,7 @@ pub async fn accept_requests<'a>( { error!("connect error: {e:?}"); // continue; - tcp_socket_disable().await; + tcp_socket_disable(); } debug!("Connected, port 22"); @@ -258,8 +255,7 @@ pub async fn wifi_up( } } -#[allow(clippy::unused_async)] -pub async fn wifi_controller_disable() { +pub fn wifi_controller_disable() { // TODO: Correctly disable wifi controller // pub async fn wifi_disable(wifi_controller: EspWifiController<'_>) -> (){ // drop wifi controller diff --git a/src/main.rs b/src/main.rs index e5723f5..e59eb49 100644 --- a/src/main.rs +++ b/src/main.rs @@ -51,8 +51,7 @@ cfg_if::cfg_if! { } } -#[allow(clippy::unused_async)] -pub async fn peripherals_disable() { +pub fn peripherals_disable() { // drop peripherals debug!("Disabling peripherals: WIP"); } @@ -102,7 +101,7 @@ async fn main(spawner: Spawner) -> ! { panic!("Could not acquire flash storage lock"); }; let mut flash_storage = flash_storage_guard.lock().await; - ssh_stamp::store::load_or_create(&mut flash_storage).await + ssh_stamp::store::load_or_create(&mut flash_storage) } .expect("Could not load or create SSHStampConfig"); @@ -191,7 +190,7 @@ async fn main(spawner: Spawner) -> ! { } } - peripherals_disable().await; + peripherals_disable(); // loop {} warn!("End of Main... Reset!!"); software_reset(); @@ -225,7 +224,7 @@ async fn peripherals_enabled(s: SshStampInit<'static>) -> Result<(), sunset::Err } } - net::wifi_controller_disable().await; + net::wifi_controller_disable(); Ok(()) // todo!() return relevant value } @@ -256,7 +255,7 @@ pub async fn wifi_controller_enabled(s: PeripheralsEnabled<'static>) -> Result<( error!("AP Stack error: {e}"); } } - net::ap_stack_disable().await; + net::ap_stack_disable(); Ok(()) // todo!() return relevant value } @@ -286,7 +285,7 @@ async fn tcp_enabled(s: WifiControllerEnabled<'_>) -> Result<(), sunset::Error> .await { info!("connect error: {:?}", e); - net::tcp_socket_disable().await; + net::tcp_socket_disable(); } info!("Connected, port 22"); } else { @@ -305,7 +304,7 @@ async fn tcp_enabled(s: WifiControllerEnabled<'_>) -> Result<(), sunset::Error> error!("TCP socket error: {e}"); } } - net::tcp_socket_disable().await; + net::tcp_socket_disable(); } // Ok(()) // todo!() return relevant value } @@ -324,7 +323,7 @@ async fn socket_enabled(s: TCPEnabled<'_>) -> Result<(), sunset::Error> { let mut inbuf = [0u8; UART_BUFFER_SIZE]; let mut outbuf = [0u8; UART_BUFFER_SIZE]; debug!("HSM: Starting ssh_server"); - let ssh_server = serve::ssh_wait_for_initialisation(&mut inbuf, &mut outbuf).await; + let ssh_server = serve::ssh_wait_for_initialisation(&mut inbuf, &mut outbuf); debug!("HSM: Started ssh_server"); let socket_enabled_struct = SocketEnabled { @@ -340,7 +339,7 @@ async fn socket_enabled(s: TCPEnabled<'_>) -> Result<(), sunset::Error> { } } - serve::ssh_disable().await; + serve::ssh_disable(); // } Ok(()) // todo!() return relevant value } @@ -381,7 +380,7 @@ async fn ssh_enabled(s: SocketEnabled<'_>) -> Result<(), sunset::Error> { } } - serve::connection_disable().await; + serve::connection_disable(); // } Ok(()) // todo!() return relevant value } @@ -420,7 +419,7 @@ where } } - serve::bridge_disable().await; + serve::bridge_disable(); Ok(()) } diff --git a/src/serve.rs b/src/serve.rs index 81e4531..decb52d 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -109,7 +109,7 @@ pub async fn connection_loop( panic!("Could not acquire flash storage lock"); }; let mut flash_storage = flash_storage_guard.lock().await; - let _result = store::save(&mut flash_storage, &config_guard).await; + let _result = store::save(&mut flash_storage, &config_guard); drop(config_guard); if needs_reset { info!("Configuration saved. Rebooting to apply WiFi changes..."); @@ -357,22 +357,19 @@ pub async fn connection_loop( } } -#[allow(clippy::unused_async)] -pub async fn connection_disable() { +pub fn connection_disable() { debug!("Connection loop disabled: WIP"); // TODO: Correctly disable/restart Conection loop and/or send messsage to user over SSH } -#[allow(clippy::unused_async)] -pub async fn ssh_wait_for_initialisation<'server>( +pub fn ssh_wait_for_initialisation<'server>( inbuf: &'server mut [u8; UART_BUFFER_SIZE], outbuf: &'server mut [u8; UART_BUFFER_SIZE], ) -> SSHServer<'server> { SSHServer::new(inbuf, outbuf) } -#[allow(clippy::unused_async)] -pub async fn ssh_disable() { +pub fn ssh_disable() { debug!("SSH Server disabled: WIP"); // TODO: Correctly disable/restart SSH Server and/or send messsage to user over SSH } @@ -396,8 +393,8 @@ pub async fn handle_ssh_client<'a, 'b>( match session_type { SessionType::Bridge(ch) => { info!("Handling bridge session"); - let chan_io: ChanInOut<'_> = ssh_server.stdio(ch).await?; - let (input, output) = chan_io.split(); + let stdio: ChanInOut<'_> = ssh_server.stdio(ch).await?; + let (input, output) = stdio.split(); info!("Starting bridge"); serial_bridge(input, output, uart_buff).await?; } @@ -405,18 +402,17 @@ pub async fn handle_ssh_client<'a, 'b>( SessionType::Sftp(ch) => { { debug!("Handling SFTP session"); - let sftp_io = ssh_server.stdio(ch).await?; + let stdio = ssh_server.stdio(ch).await?; // TODO: Use a configuration flag to select the hardware specific OtaActions implementer let ota_writer = storage::esp_ota::OtaWriter::new(); - ota::run_ota_server::(sftp_io, ota_writer).await? + ota::run_ota_server::(stdio, ota_writer).await? } } } Ok(()) } -#[allow(clippy::unused_async)] -pub async fn bridge_disable() { +pub fn bridge_disable() { // disable bridge debug!("Bridge disabled: WIP"); // TODO: Correctly disable/restart bridge and/or send message to user over SSH diff --git a/src/store.rs b/src/store.rs index 104d15b..a6c6526 100644 --- a/src/store.rs +++ b/src/store.rs @@ -76,9 +76,9 @@ fn config_hash(config: &SSHStampConfig) -> Result<[u8; 32], SunsetError> { /// Loads a `SSHConfig` at startup. Good for persisting hostkeys. /// /// # Errors -/// Returns an error if flash read fails or config is invalid. -pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result { - match load(flash).await { +/// Returns an error if config creation or flash write fails. +pub fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result { + match load(flash) { Ok(c) => { debug!("Good existing config"); return Ok(c); @@ -86,16 +86,16 @@ pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result debug!("Existing config bad, making new. {e}"), } - create(flash).await + create(flash) } /// Creates a new `SSHStampConfig` and saves it to flash. /// /// # Errors /// Returns an error if config creation or flash write fails. -pub async fn create(flash: &mut FlashBuffer<'_>) -> Result { +pub fn create(flash: &mut FlashBuffer<'_>) -> Result { let c = SSHStampConfig::new()?; - save(flash, &c).await?; + save(flash, &c)?; debug!("Created new config: {:?}", &c); Ok(c) @@ -105,18 +105,16 @@ pub async fn create(flash: &mut FlashBuffer<'_>) -> Result) -> Result { +pub fn load(fl: &mut FlashBuffer<'_>) -> Result { // If at some point you target a 64bit arch these can truncate and cause // corruption of the bootloader or the ota partition. + let offset = + u32::try_from(CONFIG_OFFSET).map_err(|_| SunsetError::msg("CONFIG_OFFSET overflow"))?; - fl.flash - .read(CONFIG_OFFSET as u32, &mut fl.buf) - .map_err(|e| { - error!("flash read error 0x{CONFIG_OFFSET:x} {e:?}"); - SunsetError::msg("flash error") - })?; + fl.flash.read(offset, &mut fl.buf).map_err(|e| { + error!("flash read error 0x{CONFIG_OFFSET:x} {e:?}"); + SunsetError::msg("flash error") + })?; let flash_config: FlashConfig = sshwire::read_ssh(&fl.buf, None) .map_err(|_| SunsetError::msg("failed to decode flash config"))?; @@ -145,9 +143,7 @@ pub async fn load(fl: &mut FlashBuffer<'_>) -> Result, config: &SSHStampConfig) -> Result<(), SunsetError> { +pub fn save(fl: &mut FlashBuffer<'_>, config: &SSHStampConfig) -> Result<(), SunsetError> { let sc = FlashConfig { version: SSHStampConfig::CURRENT_VERSION, config: OwnOrBorrow::Borrow(config), @@ -175,17 +171,17 @@ pub async fn save(fl: &mut FlashBuffer<'_>, config: &SSHStampConfig) -> Result<( const { assert!(CONFIG_AREA_SIZE > FlashConfig::BUF_SIZE) }; - fl.flash - .erase( - CONFIG_OFFSET as u32, - (CONFIG_OFFSET + CONFIG_AREA_SIZE) as u32, - ) - .map_err(|e| { - error!("flash erase error: {e:?}"); - SunsetError::msg("flash erase error") - })?; + let offset = + u32::try_from(CONFIG_OFFSET).map_err(|_| SunsetError::msg("CONFIG_OFFSET overflow"))?; + let area_size = u32::try_from(CONFIG_AREA_SIZE) + .map_err(|_| SunsetError::msg("CONFIG_AREA_SIZE overflow"))?; + + fl.flash.erase(offset, offset + area_size).map_err(|e| { + error!("flash erase error: {e:?}"); + SunsetError::msg("flash erase error") + })?; - fl.flash.write(CONFIG_OFFSET as u32, &fl.buf).map_err(|e| { + fl.flash.write(offset, &fl.buf).map_err(|e| { error!("flash write error: {e:?}"); SunsetError::msg("flash write error") })?; From 6ff941e074c5039694004610d050f95fd58bffe3 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Thu, 2 Apr 2026 01:23:51 +0200 Subject: [PATCH 05/10] Significant refactor for connection_loop to keep function under 100 lines (according to clippy lints)... still needs a final pass of review --- ota/src/handler.rs | 14 +- ota/src/tlv.rs | 6 +- src/espressif/buffered_uart.rs | 3 +- src/main.rs | 4 +- src/serial.rs | 4 +- src/serve.rs | 509 ++++++++++++++++++--------------- 6 files changed, 295 insertions(+), 245 deletions(-) diff --git a/ota/src/handler.rs b/ota/src/handler.rs index ecafc9a..46191f4 100644 --- a/ota/src/handler.rs +++ b/ota/src/handler.rs @@ -169,13 +169,13 @@ impl UpdateProcessor { self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); return Err(OtaError::IllegalOperation); } - info!("Received Ota type: {ota_type:?}"); + debug!("Received Ota type: {ota_type:?}"); self.header.ota_type = Some(ota_type); self.tlv_holder.fill(0); self.current_len = 0; } tlv::Tlv::Sha256Checksum { checksum } => { - info!("Received Checksum: {checksum:?}"); + debug!("Received Checksum: {checksum:?}"); if self.header.ota_type.is_none() { error!("UpdateProcessor: Received SHA256 Checksum TLV before OTA Type TLV"); self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); @@ -193,7 +193,7 @@ impl UpdateProcessor { } async fn handle_firmware_blob(&mut self, size: u32) -> Result<(), OtaError> { - info!("Received FirmwareBlob size: {size:?}"); + debug!("Received FirmwareBlob size: {size:?}"); if self.header.ota_type.is_none() { error!("UpdateProcessor: Received FirmwareBlob TLV before OTA Type TLV"); self.state = UpdateProcessorState::Error(OtaError::IllegalOperation); @@ -217,11 +217,11 @@ impl UpdateProcessor { } self.header.firmware_blob_size = Some(size); - info!("Starting OTA update"); + debug!("Starting OTA update"); self.state = UpdateProcessorState::Downloading { total_received_size: 0, }; - info!("Transitioning to Downloading state"); + debug!("Transitioning to Downloading state"); Ok(()) } @@ -277,7 +277,7 @@ impl UpdateProcessor { if *total_received_size >= total_blob_size { self.verify_checksum()?; - info!("All firmware data received, transitioning to Finished state"); + debug!("All firmware data received, transitioning to Finished state"); self.state = UpdateProcessorState::Finished {}; } else { self.state = UpdateProcessorState::Downloading { @@ -295,7 +295,7 @@ impl UpdateProcessor { let computed = self.hasher.clone().finalize(); if original_hash.as_slice() == computed.as_slice() { - info!("UpdateProcessor: Checksum verified successfully"); + debug!("UpdateProcessor: Checksum verified successfully"); } else { error!( "UpdateProcessor: Checksum mismatch after download! Expected: {original_hash:x?}`" diff --git a/ota/src/tlv.rs b/ota/src/tlv.rs index 6f1927e..401b02b 100644 --- a/ota/src/tlv.rs +++ b/ota/src/tlv.rs @@ -8,7 +8,7 @@ /// /// If you are looking into improving this, consider looking into [proto.rs](https://github.com/mkj/sunset/blob/8e5d20916cf7b29111b90e4d3b7bb7827c9be8e5/sftp/src/proto.rs) /// for an example on how to automate the generation of protocols with macros -use log::{debug, error, info, warn}; +use log::{debug, error, warn}; use sunset::sshwire::{SSHDecode, SSHEncode, SSHSource, WireError}; use crate::tlv; @@ -187,7 +187,7 @@ impl<'a> TlvsSource<'a> { tlv_holder[*current_len..*current_len + to_read].copy_from_slice(type_len_bytes); *current_len += to_read; if needed < to_read { - info!("Will get more data to complete TLV type/length"); + debug!("Will get more data to complete TLV type/length"); return Err(WireError::RanOut); } } @@ -211,7 +211,7 @@ impl<'a> TlvsSource<'a> { tlv_holder[*current_len..*current_len + to_read].copy_from_slice(needed_type_len_bytes); *current_len += to_read; if needed < to_read { - info!("Will get more data to complete TLV type/length"); + debug!("Will get more data to complete TLV type/length"); return Err(WireError::RanOut); } } diff --git a/src/espressif/buffered_uart.rs b/src/espressif/buffered_uart.rs index c032591..c80bf42 100644 --- a/src/espressif/buffered_uart.rs +++ b/src/espressif/buffered_uart.rs @@ -171,8 +171,7 @@ pub struct UartPins<'a> { pub static UART_BUF: StaticCell = StaticCell::new(); pub static UART_SIGNAL: Signal = Signal::new(); -#[allow(clippy::unused_async)] -pub async fn uart_buffer_wait_for_initialisation() -> &'static BufferedUart { +pub fn uart_buffer_wait_for_initialisation() -> &'static BufferedUart { UART_BUF.init_with(BufferedUart::new) } diff --git a/src/main.rs b/src/main.rs index e59eb49..fff5530 100644 --- a/src/main.rs +++ b/src/main.rs @@ -284,10 +284,10 @@ async fn tcp_enabled(s: WifiControllerEnabled<'_>) -> Result<(), sunset::Error> }) .await { - info!("connect error: {:?}", e); + error!("connect error: {:?}", e); net::tcp_socket_disable(); } - info!("Connected, port 22"); + debug!("Connected, port 22"); } else { let tcp_socket = net::accept_requests(s.tcp_stack, &mut rx_buffer, &mut tx_buffer).await; } diff --git a/src/serial.rs b/src/serial.rs index bdf47dd..244075a 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -4,7 +4,7 @@ use embassy_futures::select::select; use embedded_io_async::{Read, Write}; -use log::{debug, info, warn}; +use log::{debug, warn}; // Espressif specific crates use crate::espressif::buffered_uart::BufferedUart; @@ -18,7 +18,7 @@ pub async fn serial_bridge( chan_write: impl Write, uart: &BufferedUart, ) -> Result<(), sunset::Error> { - info!("Starting serial <--> SSH bridge"); + debug!("Starting serial <--> SSH bridge"); select(uart_to_ssh(uart, chan_write), ssh_to_uart(chan_read, uart)).await; debug!("Stopping serial <--> SSH bridge"); Ok(()) diff --git a/src/serve.rs b/src/serve.rs index decb52d..16a1303 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -9,7 +9,6 @@ use crate::espressif::buffered_uart::UART_SIGNAL; use crate::settings::UART_BUFFER_SIZE; use crate::store; use esp_hal::system::software_reset; -use heapless::String; use storage::flash; use core::fmt::Debug; @@ -21,11 +20,43 @@ use embassy_sync::blocking_mutex::raw::NoopRawMutex; use embassy_sync::channel::Channel; // Sunset -use sunset::event::ServPubkeyAuth; use sunset::{ChanHandle, ServEvent, error}; use sunset_async::SunsetMutex; use sunset_async::{ProgressHolder, SSHServer}; +mod env_parser { + use heapless::String; + + pub fn parse_wifi_ssid(value: &str) -> Option> { + let mut s = String::new(); + s.push_str(value).ok()?; + Some(s) + } + + pub fn parse_wifi_psk(value: &str) -> Option> { + if value.len() < 8 || value.len() > 63 { + return None; + } + let mut s = String::new(); + s.push_str(value).ok()?; + Some(s) + } + + pub fn parse_mac_address(value: &str) -> Option<[u8; 6]> { + if value.len() != 17 { + return None; + } + let parts: heapless::Vec = value + .split(':') + .filter_map(|p| u8::from_str_radix(p, 16).ok()) + .collect(); + if parts.len() != 6 { + return None; + } + Some([parts[0], parts[1], parts[2], parts[3], parts[4], parts[5]]) + } +} + #[derive(Debug)] pub enum SessionType { Bridge(ChanHandle), @@ -33,8 +64,27 @@ pub enum SessionType { Sftp(ChanHandle), } -fn key_signature_ok(a: &ServPubkeyAuth<'_, '_>) -> bool { - a.real() +async fn save_config_and_reboot( + config: &SunsetMutex, + config_changed: bool, + needs_reset: bool, +) { + if !config_changed { + return; + } + + let config_guard = config.lock().await; + let Some(flash_storage_guard) = flash::get_flash_n_buffer() else { + panic!("Could not acquire flash storage lock"); + }; + let mut flash_storage = flash_storage_guard.lock().await; + let _result = store::save(&mut flash_storage, &config_guard); + drop(config_guard); + + if needs_reset { + info!("Configuration saved. Rebooting to apply WiFi changes..."); + software_reset(); + } } /// Handles the SSH connection loop, processing events from clients. @@ -44,19 +94,14 @@ fn key_signature_ok(a: &ServPubkeyAuth<'_, '_>) -> bool { /// /// # Panics /// Panics if flash storage lock cannot be acquired when saving configuration. -#[allow(clippy::too_many_lines)] pub async fn connection_loop( serv: &SSHServer<'_>, chan_pipe: &Channel, config: &SunsetMutex, ) -> Result<(), sunset::Error> { let mut session: Option = None; - - debug!("Entering connection_loop and prog_loop is next..."); - let mut config_changed: bool = false; - let mut needs_reset: bool = false; - - // Will be set in `ev` PubkeyAuth is accepted and cleared once the channel is sent down chan_pipe + let mut config_changed = false; + let mut needs_reset = false; let mut auth_checked = false; loop { @@ -66,115 +111,53 @@ pub async fn connection_loop( trace!("{:?}", &ev); match ev { ServEvent::SessionSubsystem(a) => { - debug!("ServEvent::SessionSubsystem"); - - if !auth_checked { - warn!("Unauthenticated SessionSubsystem rejected"); - a.fail()?; - // TODO: Provide a message back to the client and the close the session? - } else if a.command()?.to_lowercase().as_str() == "sftp" { - if let Some(ch) = session.take() { - debug_assert!(ch.num() == a.channel()); - #[cfg(feature = "sftp-ota")] - { - a.succeed()?; - debug!("We got SFTP subsystem"); - match chan_pipe.try_send(SessionType::Sftp(ch)) { - Ok(_) => auth_checked = false, - Err(e) => log::error!("Could not send the channel: {:?}", e), - }; - } - #[cfg(not(feature = "sftp-ota"))] - { - warn!("SFTP subsystem requested but not supported in this build"); - a.fail()?; - } - } else { - a.fail()?; - } - } + handle_session_subsystem(a, &mut session, &mut auth_checked, chan_pipe)?; } ServEvent::SessionShell(a) => { - debug!("ServEvent::SessionShell"); - - if !auth_checked { - warn!("Unauthenticated SessionShell rejected"); - a.fail()?; - } else if let Some(ch) = session.take() { - // Save config after connection successful (SessionEnv completed) - if config_changed { - config_changed = false; - let config_guard = config.lock().await; - let Some(flash_storage_guard) = flash::get_flash_n_buffer() else { - panic!("Could not acquire flash storage lock"); - }; - let mut flash_storage = flash_storage_guard.lock().await; - let _result = store::save(&mut flash_storage, &config_guard); - drop(config_guard); - if needs_reset { - info!("Configuration saved. Rebooting to apply WiFi changes..."); - software_reset(); - } - } - debug_assert!(ch.num() == a.channel()); - a.succeed()?; - debug!("We got shell"); - UART_SIGNAL.signal(1); - debug!("Connection loop: UART_SIGNAL sent"); - match chan_pipe.try_send(SessionType::Bridge(ch)) { - Ok(()) => auth_checked = false, - Err(e) => log::error!("Could not send the channel: {e:?}"), - } - } else { - a.fail()?; - } + handle_session_shell( + a, + &mut session, + &mut config_changed, + needs_reset, + config, + chan_pipe, + &mut auth_checked, + ) + .await; } ServEvent::FirstAuth(mut a) => { debug!("ServEvent::FirstAuth"); let config_guard = config.lock().await; - a.enable_password_auth(false)?; - a.enable_pubkey_auth(true)?; if config_guard.first_login { a.allow()?; } else { - debug!( - "FirstAuth received but not first-login, allowing pubkey auth but rejecting - additions of new public keys on already provisioned device" - ); + debug!("FirstAuth received but not first-login, rejecting"); a.reject()?; } } ServEvent::Hostkeys(h) => { debug!("ServEvent::Hostkeys"); let config_guard = config.lock().await; - // Just take it from config as private hostkey is generated on first boot. h.hostkeys(&[&config_guard.hostkey])?; } ServEvent::PasswordAuth(a) => { - warn!("Password auth is not supported, use public key auth instead."); + warn!("Password auth not supported, use public key auth"); a.reject()?; } ServEvent::PubkeyAuth(a) => { debug!("ServEvent::PubkeyAuth"); let config_guard = config.lock().await; let client_pubkey = a.pubkey()?; - match client_pubkey { sunset::packets::PubKey::Ed25519(presented) => { let matched = config_guard .pubkeys .iter() .any(|slot| slot.as_ref().is_some_and(|stored| *stored == presented)); - if matched { - // A client that sends only a pubkey query (no signature) gets the flag set. - // Sunset's own ConnState gate prevents this from being exploited since channels - // can't open until full auth completes, but the flag being set prematurely is - // a latent bug. if that is indeed an issue then maybe stamps should check if a.real() before setting auth_checked = true. - let sig_checked = key_signature_ok(&a); - auth_checked = sig_checked; + auth_checked = true; a.allow()?; } else { debug!("No matching pubkey slot found"); @@ -182,7 +165,6 @@ pub async fn connection_loop( } } sunset::packets::PubKey::Unknown(_) => { - // Only Ed25519 keys supported a.reject()?; } } @@ -190,153 +172,22 @@ pub async fn connection_loop( ServEvent::OpenSession(a) => { debug!("ServEvent::OpenSession"); match session { - Some(_) => { - todo!("Can't have two sessions"); - } - None => { - // Track the session - session = Some(a.accept()?); - } + Some(_) => todo!("Can't have two sessions"), + None => session = Some(a.accept()?), } } ServEvent::SessionEnv(a) => { - debug!("Got ENV request"); - debug!("ENV name: {}", a.name()?); - debug!("ENV value: {}", a.value()?); - - match a.name()? { - "LANG" => { - // Ignore, but succeed to avoid client-side warnings - // This env variable will always be sent by OpenSSH client. - a.succeed()?; - } - "SSH_STAMP_PUBKEY" => { - let mut config_guard = config.lock().await; - - if !config_guard.first_login { - warn!("SSH_STAMP_PUBKEY env received but not first-login; rejecting"); - a.fail()?; - } else if config_guard.add_pubkey(a.value()?).is_ok() { - debug!("Added new pubkey from ENV"); - a.succeed()?; - config_guard.first_login = false; - config_changed = true; - auth_checked = true; - } else { - warn!("Failed to add new pubkey from ENV"); - a.fail()?; - } - } - "SSH_STAMP_WIFI_SSID" => { - let mut config_guard = config.lock().await; - if auth_checked || config_guard.first_login { - let mut s = String::<32>::new(); - if s.push_str(a.value()?).is_ok() { - config_guard.wifi_ssid = s; - debug!("Set wifi SSID from ENV"); - a.succeed()?; - config_changed = true; - needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_SSID too long"); - a.fail()?; - } - } else { - warn!( - "SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting" - ); - a.fail()?; - } - } - "SSH_STAMP_WIFI_PSK" => { - let mut config_guard = config.lock().await; - if auth_checked || config_guard.first_login { - let value = a.value()?; - if value.len() < 8 { - warn!("SSH_STAMP_WIFI_PSK too short (min 8 characters)"); - a.fail()?; - } else if value.len() > 63 { - warn!("SSH_STAMP_WIFI_PSK too long (max 63 characters)"); - a.fail()?; - } else { - let mut s = String::<63>::new(); - if s.push_str(value).is_ok() { - config_guard.wifi_pw = Some(s); - debug!("Set WIFI PSK from ENV"); - a.succeed()?; - config_changed = true; - needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_PSK push_str failed unexpectedly"); - a.fail()?; - } - } - } else { - warn!( - "SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting" - ); - a.fail()?; - } - } - "SSH_STAMP_WIFI_MAC_ADDRESS" => { - let mut config_guard = config.lock().await; - if auth_checked || config_guard.first_login { - let value = a.value()?; - if value.len() == 17 { - let parts: heapless::Vec = value - .split(':') - .filter_map(|p| u8::from_str_radix(p, 16).ok()) - .collect(); - if parts.len() == 6 { - let mac: [u8; 6] = [ - parts[0], parts[1], parts[2], parts[3], parts[4], parts[5], - ]; - config_guard.mac = mac; - debug!("Set MAC address from ENV: {mac:02X?}"); - a.succeed()?; - config_changed = true; - needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_MAC_ADDRESS invalid format"); - a.fail()?; - } - } else { - warn!( - "SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format" - ); - a.fail()?; - } - } else { - warn!( - "SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting" - ); - a.fail()?; - } - } - "SSH_STAMP_WIFI_MAC_RANDOM" => { - let mut config_guard = config.lock().await; - if auth_checked || config_guard.first_login { - config_guard.mac = [0xFF; 6]; - debug!("Set MAC address to random mode"); - a.succeed()?; - config_changed = true; - needs_reset = true; - } else { - warn!( - "SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting" - ); - a.fail()?; - } - } - _ => { - debug!("Ignoring unknown environment variable: {}", a.name()?); - a.succeed()?; - } - } + handle_session_env( + a, + config, + &mut config_changed, + &mut needs_reset, + &mut auth_checked, + ) + .await?; } ServEvent::SessionPty(a) => { let first_login = { config.lock().await.first_login }; - if auth_checked || first_login { debug!("ServEvent::SessionPty: Session granted"); a.succeed()?; @@ -357,6 +208,206 @@ pub async fn connection_loop( } } +#[cfg(feature = "sftp-ota")] +fn handle_session_subsystem( + a: sunset::event::ServExecRequest<'_, '_>, + session: &mut Option, + auth_checked: &mut bool, + chan_pipe: &Channel, +) -> Result<(), sunset::Error> { + debug!("ServEvent::SessionSubsystem"); + if !*auth_checked { + warn!("Unauthenticated SessionSubsystem rejected"); + a.fail()?; + return Ok(()); + } + + if a.command()?.to_lowercase().as_str() != "sftp" { + return Ok(()); + } + + if let Some(ch) = session.take() { + debug_assert!(ch.num() == a.channel()); + a.succeed()?; + debug!("We got SFTP subsystem"); + match chan_pipe.try_send(SessionType::Sftp(ch)) { + Ok(_) => *auth_checked = false, + Err(e) => log::error!("Could not send the channel: {:?}", e), + } + } else { + a.fail()?; + } + Ok(()) +} + +#[cfg(not(feature = "sftp-ota"))] +fn handle_session_subsystem( + a: sunset::event::ServExecRequest<'_, '_>, + session: &mut Option, + auth_checked: &mut bool, + _chan_pipe: &Channel, +) -> Result<(), sunset::Error> { + debug!("ServEvent::SessionSubsystem"); + if !*auth_checked { + warn!("Unauthenticated SessionSubsystem rejected"); + a.fail()?; + return Ok(()); + } + + if a.command()?.to_lowercase().as_str() != "sftp" { + return Ok(()); + } + + if session.take().is_some() { + warn!("SFTP subsystem requested but not supported in this build"); + a.fail()?; + } else { + a.fail()?; + } + Ok(()) +} + +async fn handle_session_shell( + a: sunset::event::ServShellRequest<'_, '_>, + session: &mut Option, + config_changed: &mut bool, + needs_reset: bool, + config: &SunsetMutex, + chan_pipe: &Channel, + auth_checked: &mut bool, +) { + debug!("ServEvent::SessionShell"); + if !*auth_checked { + warn!("Unauthenticated SessionShell rejected"); + let _ = a.fail(); + return; + } + + if let Some(ch) = session.take() { + let cc = *config_changed; + *config_changed = false; + save_config_and_reboot(config, cc, needs_reset).await; + debug_assert!(ch.num() == a.channel()); + let _ = a.succeed(); + UART_SIGNAL.signal(1); + match chan_pipe.try_send(SessionType::Bridge(ch)) { + Ok(()) => *auth_checked = false, + Err(e) => log::error!("Could not send the channel: {e:?}"), + } + } else { + let _ = a.fail(); + } +} + +async fn handle_session_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + needs_reset: &mut bool, + auth_checked: &mut bool, +) -> Result<(), sunset::Error> { + debug!("Got ENV request"); + debug!("ENV name: {}", a.name()?); + debug!("ENV value: {}", a.value()?); + + match a.name()? { + "LANG" => { + // Ignore, but succeed to avoid client-side warnings + // This env variable will always be sent by OpenSSH client. + a.succeed()?; + } + "SSH_STAMP_PUBKEY" => { + let mut config_guard = config.lock().await; + + if !config_guard.first_login { + warn!("SSH_STAMP_PUBKEY env received but not first-login; rejecting"); + a.fail()?; + } else if config_guard.add_pubkey(a.value()?).is_ok() { + debug!("Added new pubkey from ENV"); + a.succeed()?; + config_guard.first_login = false; + *config_changed = true; + *auth_checked = true; + } else { + warn!("Failed to add new pubkey from ENV"); + a.fail()?; + } + } + "SSH_STAMP_WIFI_SSID" => { + let mut config_guard = config.lock().await; + if *auth_checked || config_guard.first_login { + if let Some(s) = env_parser::parse_wifi_ssid(a.value()?) { + config_guard.wifi_ssid = s; + debug!("Set wifi SSID from ENV"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_SSID too long"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting"); + a.fail()?; + } + } + "SSH_STAMP_WIFI_PSK" => { + let mut config_guard = config.lock().await; + if *auth_checked || config_guard.first_login { + if let Some(s) = env_parser::parse_wifi_psk(a.value()?) { + config_guard.wifi_pw = Some(s); + debug!("Set WIFI PSK from ENV"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_PSK must be 8-63 characters"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting"); + a.fail()?; + } + } + "SSH_STAMP_WIFI_MAC_ADDRESS" => { + let mut config_guard = config.lock().await; + if *auth_checked || config_guard.first_login { + if let Some(mac) = env_parser::parse_mac_address(a.value()?) { + config_guard.mac = mac; + debug!("Set MAC address from ENV: {mac:02X?}"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting"); + a.fail()?; + } + } + "SSH_STAMP_WIFI_MAC_RANDOM" => { + let mut config_guard = config.lock().await; + if *auth_checked || config_guard.first_login { + config_guard.mac = [0xFF; 6]; + debug!("Set MAC address to random mode"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting"); + a.fail()?; + } + } + _ => { + debug!("Ignoring unknown environment variable: {}", a.name()?); + a.succeed()?; + } + } + Ok(()) +} + pub fn connection_disable() { debug!("Connection loop disabled: WIP"); // TODO: Correctly disable/restart Conection loop and/or send messsage to user over SSH From 42e3a6641a6d2871f4562e631e8b476377ce89f7 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Thu, 2 Apr 2026 15:40:51 +0200 Subject: [PATCH 06/10] Fix remaining clippy panics, continue extracting handlers from huge connection_loop function --- ota/src/tlv.rs | 4 +-- src/config.rs | 2 +- src/espressif/net.rs | 10 ++++---- src/serve.rs | 61 +++++++++++++++++++++++++++----------------- 4 files changed, 46 insertions(+), 31 deletions(-) diff --git a/ota/src/tlv.rs b/ota/src/tlv.rs index 2e8f4d1..a7ae6c3 100644 --- a/ota/src/tlv.rs +++ b/ota/src/tlv.rs @@ -8,7 +8,7 @@ /// /// If you are looking into improving this, consider looking into [proto.rs](https://github.com/mkj/sunset/blob/8e5d20916cf7b29111b90e4d3b7bb7827c9be8e5/sftp/src/proto.rs) /// for an example on how to automate the generation of protocols with macros -use log::{debug, error, warn}; +use log::{debug, error, info, warn}; use sunset::sshwire::{SSHDecode, SSHEncode, SSHSource, WireError}; use crate::tlv; @@ -120,7 +120,7 @@ impl<'de> SSHDecode<'de> for Tlv { return Err(sunset::sshwire::WireError::PacketWrong); } let mut checksum = [0u8; tlv::CHECKSUM_LEN as usize]; - for element in checksum.iter_mut() { + for element in &mut checksum { *element = u8::dec(s)?; } Ok(Tlv::Sha256Checksum { checksum }) diff --git a/src/config.rs b/src/config.rs index 057b6f6..728da7c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -120,7 +120,7 @@ impl SSHStampConfig { let mut rnd = [0u8; 16]; sunset::random::fill_random(&mut rnd)?; let mut ssid = String::<32>::new(); - for &byte in rnd.iter() { + for &byte in &rnd { let _ = ssid.push(WIFI_PASSWORD_CHARS[(byte as usize) % 62] as char); } Ok(ssid) diff --git a/src/espressif/net.rs b/src/espressif/net.rs index 8593c16..169ae6c 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -142,10 +142,7 @@ pub async fn if_up( Timer::after(Duration::from_millis(500)).await; } - info!( - "Connect to the AP `{}` as a DHCP client with IP: {}", - ssid_name, gw_ip_addr_ipv4 - ); + info!("Connect to the AP `{ssid_name}` as a DHCP client with IP: {gw_ip_addr_ipv4}"); Ok(ap_stack) } @@ -186,7 +183,10 @@ pub async fn accept_requests<'a>( tcp_socket } -/// Returns the configured WiFi SSID from the config. +/// Returns the configured `WiFi` SSID from the config. +/// +/// # Panics +/// Panics if `wifi_ssid` is not set in the config or exceeds 63 characters. pub async fn wifi_ssid(config: &'static SunsetMutex) -> String<63> { let guard = config.lock().await; String::<63>::try_from(guard.wifi_ssid.as_str()).expect("SSID should always be set") diff --git a/src/serve.rs b/src/serve.rs index a48109c..10cb5e7 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -20,7 +20,6 @@ use embassy_sync::blocking_mutex::raw::NoopRawMutex; use embassy_sync::channel::Channel; // Sunset -use sunset::event::ServPubkeyAuth; use sunset::{ChanFail, ChanHandle, ServEvent, error}; use sunset_async::SunsetMutex; use sunset_async::{ProgressHolder, SSHServer}; @@ -126,17 +125,8 @@ pub async fn connection_loop( ) .await; } - ServEvent::FirstAuth(mut a) => { - debug!("ServEvent::FirstAuth"); - let config_guard = config.lock().await; - a.enable_password_auth(false)?; - a.enable_pubkey_auth(true)?; - if config_guard.first_login { - a.allow()?; - } else { - debug!("FirstAuth received but not first-login, rejecting"); - a.reject()?; - } + ServEvent::FirstAuth(a) => { + handle_first_auth(a, config).await?; } ServEvent::Hostkeys(h) => { debug!("ServEvent::Hostkeys"); @@ -147,6 +137,9 @@ pub async fn connection_loop( warn!("Password auth not supported, use public key auth"); a.reject()?; } + ServEvent::SessionPty(a) => { + handle_session_pty(a, config, auth_checked).await?; + } ServEvent::PubkeyAuth(a) => { debug!("ServEvent::PubkeyAuth"); let config_guard = config.lock().await; @@ -178,7 +171,6 @@ pub async fn connection_loop( a.reject(ChanFail::SSH_OPEN_ADMINISTRATIVELY_PROHIBITED)?; } None => { - // Track the session session = Some(a.accept()?); } } @@ -193,16 +185,6 @@ pub async fn connection_loop( ) .await?; } - ServEvent::SessionPty(a) => { - let first_login = { config.lock().await.first_login }; - if auth_checked || first_login { - debug!("ServEvent::SessionPty: Session granted"); - a.succeed()?; - } else { - debug!("ServEvent::SessionPty: No auth not session"); - a.fail()?; - } - } ServEvent::SessionExec(a) => { a.fail()?; } @@ -215,6 +197,39 @@ pub async fn connection_loop( } } +async fn handle_first_auth( + mut a: sunset::event::ServFirstAuth<'_, '_>, + config: &SunsetMutex, +) -> Result<(), sunset::Error> { + debug!("ServEvent::FirstAuth"); + let config_guard = config.lock().await; + a.enable_password_auth(false)?; + a.enable_pubkey_auth(true)?; + if config_guard.first_login { + a.allow()?; + } else { + debug!("FirstAuth received but not first-login, rejecting"); + a.reject()?; + } + Ok(()) +} + +async fn handle_session_pty( + a: sunset::event::ServPtyRequest<'_, '_>, + config: &SunsetMutex, + auth_checked: bool, +) -> Result<(), sunset::Error> { + let first_login = { config.lock().await.first_login }; + if auth_checked || first_login { + debug!("ServEvent::SessionPty: Session granted"); + a.succeed()?; + } else { + debug!("ServEvent::SessionPty: No auth not session"); + a.fail()?; + } + Ok(()) +} + #[cfg(feature = "sftp-ota")] fn handle_session_subsystem( a: sunset::event::ServExecRequest<'_, '_>, From 79b5e77f21c7ca1f9732252ca9f95aec393feaff Mon Sep 17 00:00:00 2001 From: brainstorm Date: Sat, 4 Apr 2026 19:46:06 +0200 Subject: [PATCH 07/10] [ci skip] is_valid_wifi_credential needs to be merged manually now... --- src/espressif/net.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/espressif/net.rs b/src/espressif/net.rs index 96b060a..ad8b748 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -213,8 +213,8 @@ fn print_hostkey_fingerprint(hostkey: &sunset::SignKey) { sunset::SignKey::Ed25519(_) => { let pubkey = hostkey.pubkey(); match pubkey.fingerprint(ssh_key::HashAlg::Sha256) { - Ok(fp) => info!("SSH hostkey fingerprint: {}", fp), - Err(e) => warn!("Failed to compute fingerprint: {:?}", e), + Ok(fp) => info!("SSH hostkey fingerprint: {fp}"), + Err(e) => warn!("Failed to compute fingerprint: {e:?}"), } } _ => { @@ -223,7 +223,7 @@ fn print_hostkey_fingerprint(hostkey: &sunset::SignKey) { } } -/// Manages the WiFi access point lifecycle. +/// Manages the `WiFi` access point lifecycle. /// Starts the AP with the configured SSID and password from the config. /// Handles reconnection if the AP stops. #[embassy_executor::task] From c5f00b0f05be37f38c9a720ddc8ea4e11c0d0ed8 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Sat, 4 Apr 2026 20:22:32 +0200 Subject: [PATCH 08/10] Add SuperJulio (a.k.a @jubeormk1) suggestion on input validation outlined in comment: https://github.com/brainstorm/ssh-stamp/pull/93/changes#diff-147b0d5c968d71d81670bdb7cb8acfa58578d75b237f7e74bb76df5e9b63302bR238 --- src/espressif/net.rs | 2 +- src/serve.rs | 28 ++++++++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/espressif/net.rs b/src/espressif/net.rs index ad8b748..3a6436b 100644 --- a/src/espressif/net.rs +++ b/src/espressif/net.rs @@ -217,7 +217,7 @@ fn print_hostkey_fingerprint(hostkey: &sunset::SignKey) { Err(e) => warn!("Failed to compute fingerprint: {e:?}"), } } - _ => { + sunset::SignKey::AgentEd25519(_) => { warn!("Unsupported key type for fingerprint"); } } diff --git a/src/serve.rs b/src/serve.rs index 9af7ed4..8a100c0 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -27,7 +27,18 @@ use sunset_async::{ProgressHolder, SSHServer}; mod env_parser { use heapless::String; + /// Sanitizes environment variable input by checking for valid ASCII graphic characters. + /// + /// Returns `true` if the input contains at least one character and all characters + /// are ASCII graphic characters (printable characters excluding space). + pub(crate) fn env_sanitize(s: &str) -> bool { + !s.is_empty() && s.bytes().all(|b| b.is_ascii_graphic()) + } + pub fn parse_wifi_ssid(value: &str) -> Option> { + if !env_sanitize(value) { + return None; + } let mut s = String::new(); s.push_str(value).ok()?; Some(s) @@ -37,12 +48,18 @@ mod env_parser { if value.len() < 8 || value.len() > 63 { return None; } + if !env_sanitize(value) { + return None; + } let mut s = String::new(); s.push_str(value).ok()?; Some(s) } pub fn parse_mac_address(value: &str) -> Option<[u8; 6]> { + if !env_sanitize(value) { + return None; + } if value.len() != 17 { return None; } @@ -87,10 +104,6 @@ async fn save_config_and_reboot( } } -fn is_valid_wifi_credential(s: &str) -> bool { - !s.is_empty() && s.bytes().all(|b| b.is_ascii_graphic()) -} - /// Handles the SSH connection loop, processing events from clients. /// /// # Errors @@ -348,6 +361,9 @@ async fn handle_session_env( if !config_guard.first_login { warn!("SSH_STAMP_PUBKEY env received but not first-login; rejecting"); a.fail()?; + } else if !env_parser::env_sanitize(a.value()?) { + warn!("SSH_STAMP_PUBKEY contains invalid characters"); + a.fail()?; } else if config_guard.add_pubkey(a.value()?).is_ok() { debug!("Added new pubkey from ENV"); a.succeed()?; @@ -369,7 +385,7 @@ async fn handle_session_env( *config_changed = true; *needs_reset = true; } else { - warn!("SSH_STAMP_WIFI_SSID too long"); + warn!("SSH_STAMP_WIFI_SSID invalid and/or too long"); a.fail()?; } } else { @@ -387,7 +403,7 @@ async fn handle_session_env( *config_changed = true; *needs_reset = true; } else { - warn!("SSH_STAMP_WIFI_PSK must be 8-63 characters"); + warn!("SSH_STAMP_WIFI_PSK invalid and/or not within 8-63 characters"); a.fail()?; } } else { From d75d9701433d1a44a3186a28b12ebeee6eb03421 Mon Sep 17 00:00:00 2001 From: brainstorm Date: Sat, 4 Apr 2026 20:32:52 +0200 Subject: [PATCH 09/10] Refactor all handlers on serve.rs out to handle.rs --- src/handle.rs | 209 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 209 insertions(+) create mode 100644 src/handle.rs diff --git a/src/handle.rs b/src/handle.rs new file mode 100644 index 0000000..199db8d --- /dev/null +++ b/src/handle.rs @@ -0,0 +1,209 @@ +// SPDX-FileCopyrightText: 2025 Roman Valls, 2025 +// +// SPDX-License-Identifier: GPL-3.0-or-later + +use heapless::String; +use log::{debug, warn}; +use sunset_async::SunsetMutex; + +use crate::config::SSHStampConfig; + +pub mod env_parser { + use super::String; + + /// Sanitizes environment variable input by checking for valid ASCII graphic characters. + /// + /// Returns `true` if the input contains at least one character and all characters + /// are ASCII graphic characters (printable characters excluding space). + #[must_use] + pub fn env_sanitize(s: &str) -> bool { + !s.is_empty() && s.bytes().all(|b| b.is_ascii_graphic()) + } + + #[must_use] + pub fn parse_wifi_ssid(value: &str) -> Option> { + if !env_sanitize(value) { + return None; + } + let mut s = String::new(); + s.push_str(value).ok()?; + Some(s) + } + + #[must_use] + pub fn parse_wifi_psk(value: &str) -> Option> { + if value.len() < 8 || value.len() > 63 { + return None; + } + if !env_sanitize(value) { + return None; + } + let mut s = String::new(); + s.push_str(value).ok()?; + Some(s) + } + + #[must_use] + pub fn parse_mac_address(value: &str) -> Option<[u8; 6]> { + if !env_sanitize(value) { + return None; + } + if value.len() != 17 { + return None; + } + let parts: heapless::Vec = value + .split(':') + .filter_map(|p| u8::from_str_radix(p, 16).ok()) + .collect(); + if parts.len() != 6 { + return None; + } + Some([parts[0], parts[1], parts[2], parts[3], parts[4], parts[5]]) + } +} + +/// Handles `SSH_STAMP_PUBKEY` environment variable requests. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail or if the pubkey cannot be added. +pub async fn pubkey_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + auth_checked: &mut bool, +) -> Result<(), sunset::Error> { + let mut config_guard = config.lock().await; + + if !config_guard.first_login { + warn!("SSH_STAMP_PUBKEY env received but not first-login; rejecting"); + a.fail()?; + } else if !env_parser::env_sanitize(a.value()?) { + warn!("SSH_STAMP_PUBKEY contains invalid characters"); + a.fail()?; + } else if config_guard.add_pubkey(a.value()?).is_ok() { + debug!("Added new pubkey from ENV"); + a.succeed()?; + config_guard.first_login = false; + *config_changed = true; + *auth_checked = true; + } else { + warn!("Failed to add new pubkey from ENV"); + a.fail()?; + } + Ok(()) +} + +/// Handles `SSH_STAMP_WIFI_SSID` environment variable requests. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail or if the SSID is invalid. +pub async fn wifi_ssid_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + needs_reset: &mut bool, + auth_checked: bool, +) -> Result<(), sunset::Error> { + let mut config_guard = config.lock().await; + if auth_checked || config_guard.first_login { + if let Some(s) = env_parser::parse_wifi_ssid(a.value()?) { + config_guard.wifi_ssid = s; + debug!("Set wifi SSID from ENV"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_SSID invalid and/or too long"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting"); + a.fail()?; + } + Ok(()) +} + +/// Handles `SSH_STAMP_WIFI_PSK` environment variable requests. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail or if the PSK is invalid. +pub async fn wifi_psk_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + needs_reset: &mut bool, + auth_checked: bool, +) -> Result<(), sunset::Error> { + let mut config_guard = config.lock().await; + if auth_checked || config_guard.first_login { + if let Some(s) = env_parser::parse_wifi_psk(a.value()?) { + config_guard.wifi_pw = Some(s); + debug!("Set WIFI PSK from ENV"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_PSK invalid and/or not within 8-63 characters"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting"); + a.fail()?; + } + Ok(()) +} + +/// Handles `SSH_STAMP_WIFI_MAC_ADDRESS` environment variable requests. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail or if the MAC address is invalid. +pub async fn wifi_mac_address_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + needs_reset: &mut bool, + auth_checked: bool, +) -> Result<(), sunset::Error> { + let mut config_guard = config.lock().await; + if auth_checked || config_guard.first_login { + if let Some(mac) = env_parser::parse_mac_address(a.value()?) { + config_guard.mac = mac; + debug!("Set MAC address from ENV: {mac:02X?}"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format"); + a.fail()?; + } + } else { + warn!("SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting"); + a.fail()?; + } + Ok(()) +} + +/// Handles `SSH_STAMP_WIFI_MAC_RANDOM` environment variable requests. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail or if authentication is missing. +pub async fn wifi_mac_random_env( + a: sunset::event::ServEnvironmentRequest<'_, '_>, + config: &SunsetMutex, + config_changed: &mut bool, + needs_reset: &mut bool, + auth_checked: bool, +) -> Result<(), sunset::Error> { + let mut config_guard = config.lock().await; + if auth_checked || config_guard.first_login { + config_guard.mac = [0xFF; 6]; + debug!("Set MAC address to random mode"); + a.succeed()?; + *config_changed = true; + *needs_reset = true; + } else { + warn!("SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting"); + a.fail()?; + } + Ok(()) +} From 3a18d8b2dd69076076e3d02c66369e410af932cc Mon Sep 17 00:00:00 2001 From: brainstorm Date: Sat, 4 Apr 2026 20:33:11 +0200 Subject: [PATCH 10/10] No more >100 lines function clippy error --- src/lib.rs | 1 + src/serve.rs | 137 +++------------------------------------------------ 2 files changed, 9 insertions(+), 129 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 90026f6..fe63a92 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,6 +11,7 @@ pub mod config; pub mod errors; pub mod espressif; +pub mod handle; pub mod serial; pub mod serve; pub mod settings; diff --git a/src/serve.rs b/src/serve.rs index 8a100c0..1b31efa 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -6,6 +6,7 @@ use log::{debug, info, trace, warn}; use crate::config::SSHStampConfig; use crate::espressif::buffered_uart::UART_SIGNAL; +use crate::handle; use crate::settings::UART_BUFFER_SIZE; use crate::store; use esp_hal::system::software_reset; @@ -24,56 +25,6 @@ use sunset::{ChanFail, ChanHandle, ServEvent, error}; use sunset_async::SunsetMutex; use sunset_async::{ProgressHolder, SSHServer}; -mod env_parser { - use heapless::String; - - /// Sanitizes environment variable input by checking for valid ASCII graphic characters. - /// - /// Returns `true` if the input contains at least one character and all characters - /// are ASCII graphic characters (printable characters excluding space). - pub(crate) fn env_sanitize(s: &str) -> bool { - !s.is_empty() && s.bytes().all(|b| b.is_ascii_graphic()) - } - - pub fn parse_wifi_ssid(value: &str) -> Option> { - if !env_sanitize(value) { - return None; - } - let mut s = String::new(); - s.push_str(value).ok()?; - Some(s) - } - - pub fn parse_wifi_psk(value: &str) -> Option> { - if value.len() < 8 || value.len() > 63 { - return None; - } - if !env_sanitize(value) { - return None; - } - let mut s = String::new(); - s.push_str(value).ok()?; - Some(s) - } - - pub fn parse_mac_address(value: &str) -> Option<[u8; 6]> { - if !env_sanitize(value) { - return None; - } - if value.len() != 17 { - return None; - } - let parts: heapless::Vec = value - .split(':') - .filter_map(|p| u8::from_str_radix(p, 16).ok()) - .collect(); - if parts.len() != 6 { - return None; - } - Some([parts[0], parts[1], parts[2], parts[3], parts[4], parts[5]]) - } -} - #[derive(Debug)] pub enum SessionType { Bridge(ChanHandle), @@ -351,96 +302,24 @@ async fn handle_session_env( match a.name()? { "LANG" => { - // Ignore, but succeed to avoid client-side warnings - // This env variable will always be sent by OpenSSH client. a.succeed()?; } "SSH_STAMP_PUBKEY" => { - let mut config_guard = config.lock().await; - - if !config_guard.first_login { - warn!("SSH_STAMP_PUBKEY env received but not first-login; rejecting"); - a.fail()?; - } else if !env_parser::env_sanitize(a.value()?) { - warn!("SSH_STAMP_PUBKEY contains invalid characters"); - a.fail()?; - } else if config_guard.add_pubkey(a.value()?).is_ok() { - debug!("Added new pubkey from ENV"); - a.succeed()?; - config_guard.first_login = false; - *config_changed = true; - *auth_checked = true; - } else { - warn!("Failed to add new pubkey from ENV"); - a.fail()?; - } + handle::pubkey_env(a, config, config_changed, auth_checked).await?; } "SSH_STAMP_WIFI_SSID" => { - let mut config_guard = config.lock().await; - if *auth_checked || config_guard.first_login { - if let Some(s) = env_parser::parse_wifi_ssid(a.value()?) { - config_guard.wifi_ssid = s; - debug!("Set wifi SSID from ENV"); - a.succeed()?; - *config_changed = true; - *needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_SSID invalid and/or too long"); - a.fail()?; - } - } else { - warn!("SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting"); - a.fail()?; - } + handle::wifi_ssid_env(a, config, config_changed, needs_reset, *auth_checked).await?; } "SSH_STAMP_WIFI_PSK" => { - let mut config_guard = config.lock().await; - if *auth_checked || config_guard.first_login { - if let Some(s) = env_parser::parse_wifi_psk(a.value()?) { - config_guard.wifi_pw = Some(s); - debug!("Set WIFI PSK from ENV"); - a.succeed()?; - *config_changed = true; - *needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_PSK invalid and/or not within 8-63 characters"); - a.fail()?; - } - } else { - warn!("SSH_STAMP_WIFI_PSK env received but not authenticated; rejecting"); - a.fail()?; - } + handle::wifi_psk_env(a, config, config_changed, needs_reset, *auth_checked).await?; } "SSH_STAMP_WIFI_MAC_ADDRESS" => { - let mut config_guard = config.lock().await; - if *auth_checked || config_guard.first_login { - if let Some(mac) = env_parser::parse_mac_address(a.value()?) { - config_guard.mac = mac; - debug!("Set MAC address from ENV: {mac:02X?}"); - a.succeed()?; - *config_changed = true; - *needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_MAC_ADDRESS must be XX:XX:XX:XX:XX:XX format"); - a.fail()?; - } - } else { - warn!("SSH_STAMP_WIFI_MAC_ADDRESS env received but not authenticated; rejecting"); - a.fail()?; - } + handle::wifi_mac_address_env(a, config, config_changed, needs_reset, *auth_checked) + .await?; } "SSH_STAMP_WIFI_MAC_RANDOM" => { - let mut config_guard = config.lock().await; - if *auth_checked || config_guard.first_login { - config_guard.mac = [0xFF; 6]; - debug!("Set MAC address to random mode"); - a.succeed()?; - *config_changed = true; - *needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_MAC_RANDOM env received but not authenticated; rejecting"); - a.fail()?; - } + handle::wifi_mac_random_env(a, config, config_changed, needs_reset, *auth_checked) + .await?; } _ => { debug!("Ignoring unknown environment variable: {}", a.name()?);