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 diff --git a/ota/src/handler.rs b/ota/src/handler.rs index b7dcd08..46191f4 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); + } + 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 } => { + 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); + 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> { + 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); + 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); + + debug!("Starting OTA update"); + self.state = UpdateProcessorState::Downloading { + total_received_size: 0, + }; + debug!("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()?; + debug!("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() { + debug!("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 b8946c2..a7ae6c3 100644 --- a/ota/src/tlv.rs +++ b/ota/src/tlv.rs @@ -19,13 +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 = 0x73736873; // 'sshs' big endian in ASCII +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 = (core::mem::size_of::() - + core::mem::size_of::() - + u8::MAX as usize) as u32; // type + length + value +/// +/// 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( @@ -35,7 +35,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 +50,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 +64,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 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,11 +114,13 @@ 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]; - for element in checksum.iter_mut() { + for element in &mut checksum { *element = u8::dec(s)?; } Ok(Tlv::Sha256Checksum { checksum }) @@ -124,10 +130,8 @@ 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 Err(sunset::sshwire::WireError::UnknownPacket { number: tlv_type }) @@ -136,7 +140,7 @@ impl<'de> SSHDecode<'de> for Tlv { } } -/// 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 +149,7 @@ pub struct TlvsSource<'a> { } impl<'a> TlvsSource<'a> { + #[must_use] pub fn new(buf: &'a [u8]) -> Self { Self { remaining_buf: buf, @@ -153,17 +158,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 +181,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 +275,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 +306,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 +318,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 +356,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 8ad1a6f..728da7c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -27,7 +27,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? @@ -69,11 +69,14 @@ 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 } /// 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() @@ -84,6 +87,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)?; @@ -116,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) @@ -126,7 +130,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) @@ -156,7 +160,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(()); @@ -186,7 +190,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), } } @@ -200,9 +204,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> @@ -214,7 +221,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)?; @@ -224,27 +231,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(|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(()) } #[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(()) } @@ -267,7 +274,7 @@ where Ok(StaticConfigV4 { address: Ipv4Cidr::new(ad, prefix), gateway, - dns_servers: Default::default(), + dns_servers: heapless::Vec::new(), }) }) .transpose() @@ -292,7 +299,7 @@ where Ok(StaticConfigV6 { address: Ipv6Cidr::new(ad, prefix), gateway, - dns_servers: Vec::new(), + dns_servers: Default::default(), }) }) .transpose() @@ -302,17 +309,17 @@ impl SSHEncode for SSHStampConfig { fn enc(&self, s: &mut dyn SSHSink) -> WireResult<()> { enc_signkey(&self.hostkey, s)?; - for k in self.pubkeys.iter() { - enc_option(k, s)?; + for k in &self.pubkeys { + 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)?; @@ -333,7 +340,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..c80bf42 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,18 +55,18 @@ 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]; - 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}"); @@ -73,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 @@ -101,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. @@ -141,14 +142,14 @@ impl Default for BufferedUart { } } -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; -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 @@ -170,7 +171,7 @@ pub struct UartPins<'a> { pub static UART_BUF: StaticCell = StaticCell::new(); pub static UART_SIGNAL: Signal = Signal::new(); -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) } @@ -178,9 +179,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. @@ -204,5 +207,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 2b35f69..3a6436b 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 @@ -66,7 +73,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); } @@ -77,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); } } @@ -105,17 +112,17 @@ 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; let net_config = embassy_net::Config::ipv4_static(StaticConfigV4 { address: Ipv4Cidr::new(gw_ip_addr_ipv4, 24), gateway: Some(gw_ip_addr_ipv4), - dns_servers: Default::default(), + dns_servers: heapless::Vec::new(), }); - 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( @@ -137,21 +144,18 @@ 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) } -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 } -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 @@ -172,27 +176,32 @@ pub async fn accept_requests<'a>( }) .await { - error!("connect error: {:?}", e); + error!("connect error: {e:?}"); // continue; - tcp_socket_disable().await; + tcp_socket_disable(); } debug!("Connected, port 22"); 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") } -/// 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 +/// 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 { - 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()"), @@ -204,17 +213,17 @@ 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:?}"), } } - _ => { + sunset::SignKey::AgentEd25519(_) => { warn!("Unsupported key type for fingerprint"); } } } -/// 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] @@ -238,17 +247,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; } @@ -258,7 +267,7 @@ pub async fn wifi_up( } } -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 @@ -268,11 +277,11 @@ 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"); - runner.run().await + runner.run().await; } #[embassy_executor::task] diff --git a/src/espressif/rng.rs b/src/espressif/rng.rs index e7ba71e..bc08f18 100644 --- a/src/espressif/rng.rs +++ b/src/espressif/rng.rs @@ -23,6 +23,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/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(()) +} 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/main.rs b/src/main.rs index 4693afd..1e73144 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; @@ -49,7 +52,7 @@ cfg_if::cfg_if! { } } -pub async fn peripherals_disable() -> () { +pub fn peripherals_disable() { // drop peripherals debug!("Disabling peripherals: WIP"); } @@ -115,7 +118,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"); @@ -197,14 +200,14 @@ async fn main(spawner: Spawner) -> ! { uart_buf, }; - match peripherals_enabled(peripherals_enabled_struct).await { - Ok(_) => (), + match Box::pin(peripherals_enabled(peripherals_enabled_struct)).await { + Ok(()) => (), Err(e) => { - error!("Peripheral error: {}", e); + error!("Peripheral error: {e}"); } } - peripherals_disable().await; + peripherals_disable(); // loop {} warn!("End of Main... Reset!!"); software_reset(); @@ -231,14 +234,14 @@ 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 { - Ok(_) => (), + match Box::pin(wifi_controller_enabled(peripherals_enabled_struct)).await { + Ok(()) => (), Err(e) => { - error!("Wifi controller error: {}", e); + error!("Wifi controller error: {e}"); } } - net::wifi_controller_disable().await; + net::wifi_controller_disable(); Ok(()) // todo!() return relevant value } @@ -249,6 +252,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?; @@ -259,13 +266,13 @@ 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 { - Ok(_) => (), + match Box::pin(tcp_enabled(wifi_controller_enabled_stack)).await { + Ok(()) => (), Err(e) => { - error!("AP Stack error: {}", e); + error!("AP Stack error: {e}"); } } - net::ap_stack_disable().await; + net::ap_stack_disable(); Ok(()) // todo!() return relevant value } @@ -276,7 +283,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]; @@ -294,10 +301,10 @@ async fn tcp_enabled<'a>(s: WifiControllerEnabled<'a>) -> Result<(), sunset::Err }) .await { - info!("connect error: {:?}", e); - net::tcp_socket_disable().await; + 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; } @@ -308,13 +315,13 @@ 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 { - Ok(_) => (), + match Box::pin(socket_enabled(tcp_enabled_struct)).await { + Ok(()) => (), Err(e) => { - error!("TCP socket error: {}", e); + error!("TCP socket error: {e}"); } } - net::tcp_socket_disable().await; + net::tcp_socket_disable(); } // Ok(()) // todo!() return relevant value } @@ -326,14 +333,14 @@ 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() 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 { @@ -343,13 +350,13 @@ 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}"); } } - serve::ssh_disable().await; + serve::ssh_disable(); // } Ok(()) // todo!() return relevant value } @@ -366,7 +373,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"); @@ -384,13 +391,13 @@ 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}"); } } - serve::connection_disable().await; + serve::connection_disable(); // } Ok(()) // todo!() return relevant value } @@ -423,13 +430,13 @@ 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}"); } } - serve::bridge_disable().await; + serve::bridge_disable(); Ok(()) } @@ -451,9 +458,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 a447f93..244075a 100644 --- a/src/serial.rs +++ b/src/serial.rs @@ -4,47 +4,49 @@ 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; /// 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; + 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(()) } 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 { 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?; + 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 e7a2f51..1b31efa 100644 --- a/src/serve.rs +++ b/src/serve.rs @@ -6,10 +6,10 @@ 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; -use heapless::String; use storage::flash; use core::fmt::Debug; @@ -21,7 +21,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}; @@ -33,26 +32,44 @@ 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; + } -fn is_valid_wifi_credential(s: &str) -> bool { - !s.is_empty() && s.bytes().all(|b| b.is_ascii_graphic()) + 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. +/// +/// # Errors +/// Returns an error if SSH protocol operations fail. +/// +/// # Panics +/// Panics if flash storage lock cannot be acquired when saving configuration. 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 { @@ -62,125 +79,54 @@ 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).await; - 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" - ); - a.reject()?; - } + ServEvent::FirstAuth(a) => { + handle_first_auth(a, config).await?; } 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::SessionPty(a) => { + handle_session_pty(a, config, auth_checked).await?; + } 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 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. - let sig_checked = key_signature_ok(&a); - auth_checked = sig_checked; + auth_checked = true; a.allow()?; } else { debug!("No matching pubkey slot found"); a.reject()?; } } - _ => { - // Only Ed25519 keys supported + sunset::packets::PubKey::Unknown(_) => { a.reject()?; } } @@ -193,195 +139,209 @@ pub async fn connection_loop( a.reject(ChanFail::SSH_OPEN_ADMINISTRATIVELY_PROHIBITED)?; } None => { - // Track the session 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) { - warn!( - "SSH_STAMP_WIFI_SSID env received but not authenticated; rejecting" - ); - a.fail()?; - } else { - let value = a.value()?; - if !is_valid_wifi_credential(value) { - warn!( - "SSH_STAMP_WIFI_SSID contains invalid characters (null bytes or non-printable ASCII)" - ); - a.fail()?; - } else { - let mut s = String::<32>::new(); - if s.push_str(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()?; - } - } - } - } - "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 { - let value = a.value()?; - if !is_valid_wifi_credential(value) { - warn!( - "SSH_STAMP_WIFI_PSK contains invalid characters (null bytes or non-printable ASCII)" - ); - a.fail()?; - } else 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()?; - } - } - } - } - "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 { - 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 { - 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: {:02X?}", mac); - a.succeed()?; - config_changed = true; - needs_reset = true; - } else { - warn!("SSH_STAMP_WIFI_MAC_ADDRESS invalid format"); - 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 { - config_guard.mac = [0xFF; 6]; - debug!("Set MAC address to random mode"); - a.succeed()?; - config_changed = true; - needs_reset = true; - } - } - _ => { - debug!("Ignoring unknown environment variable: {}", a.name()?); - a.succeed()?; - } - } - } - 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()?; - } + handle_session_env( + a, + config, + &mut config_changed, + &mut needs_reset, + &mut auth_checked, + ) + .await?; } ServEvent::SessionExec(a) => { a.fail()?; } ServEvent::Defunct => { debug!("Expected caller to handle event"); - error::BadUsage.fail()? + error::BadUsage.fail()?; } ServEvent::PollAgain => {} } } } -pub async fn connection_disable() -> () { +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<'_, '_>, + 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" => { + a.succeed()?; + } + "SSH_STAMP_PUBKEY" => { + handle::pubkey_env(a, config, config_changed, auth_checked).await?; + } + "SSH_STAMP_WIFI_SSID" => { + handle::wifi_ssid_env(a, config, config_changed, needs_reset, *auth_checked).await?; + } + "SSH_STAMP_WIFI_PSK" => { + handle::wifi_psk_env(a, config, config_changed, needs_reset, *auth_checked).await?; + } + "SSH_STAMP_WIFI_MAC_ADDRESS" => { + handle::wifi_mac_address_env(a, config, config_changed, needs_reset, *auth_checked) + .await?; + } + "SSH_STAMP_WIFI_MAC_RANDOM" => { + handle::wifi_mac_random_env(a, config, config_changed, needs_reset, *auth_checked) + .await?; + } + _ => { + 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 } -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) } -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 } @@ -390,6 +350,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>, @@ -402,9 +366,9 @@ pub async fn handle_ssh_client<'a, 'b>( SessionType::Bridge(ch) => { info!("Handling bridge session"); let stdio: ChanInOut<'_> = ssh_server.stdio(ch).await?; - let (stdin, stdout) = stdio.split(); + let (input, output) = stdio.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) => { @@ -416,11 +380,11 @@ pub async fn handle_ssh_client<'a, 'b>( ota::run_ota_server::(stdio, ota_writer).await? } } - }; + } Ok(()) } -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 78a5270..a6c6526 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,9 +73,12 @@ fn config_hash(config: &SSHStampConfig) -> Result<[u8; 32], SunsetError> { Ok(h.finalize().into()) } -/// Loads a SSHConfig at startup. Good for persisting hostkeys. -pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result { - match load(flash).await { +/// Loads a `SSHConfig` at startup. Good for persisting hostkeys. +/// +/// # Errors +/// 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); @@ -83,27 +86,35 @@ pub async fn load_or_create(flash: &mut FlashBuffer<'_>) -> Result debug!("Existing config bad, making new. {e}"), } - create(flash).await + create(flash) } -pub async fn create(flash: &mut FlashBuffer<'_>) -> Result { +/// Creates a new `SSHStampConfig` and saves it to flash. +/// +/// # Errors +/// Returns an error if config creation or flash write fails. +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) } -pub async fn load(fl: &mut FlashBuffer<'_>) -> Result { +/// Loads `SSHStampConfig` from flash. +/// +/// # Errors +/// Returns an error if flash read fails, config is invalid, or hash mismatch. +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"))?; @@ -128,7 +139,11 @@ pub async fn load(fl: &mut FlashBuffer<'_>) -> Result, config: &SSHStampConfig) -> Result<(), SunsetError> { +/// Saves `SSHStampConfig` to flash. +/// +/// # Errors +/// Returns an error if flash write fails or config serialization fails. +pub fn save(fl: &mut FlashBuffer<'_>, config: &SSHStampConfig) -> Result<(), SunsetError> { let sc = FlashConfig { version: SSHStampConfig::CURRENT_VERSION, config: OwnOrBorrow::Borrow(config), @@ -156,18 +171,18 @@ 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| { - error!("flash write error: {:?}", e); + fl.flash.write(offset, &fl.buf).map_err(|e| { + error!("flash write error: {e:?}"); SunsetError::msg("flash write error") })?; 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));