diff --git a/crates/kerykeion/src/config.rs b/crates/kerykeion/src/config.rs index 7849a5c..c809d65 100644 --- a/crates/kerykeion/src/config.rs +++ b/crates/kerykeion/src/config.rs @@ -98,9 +98,39 @@ pub struct ChannelPsk { /// - 0 bytes: no encryption (use only for public channels). /// - 16 bytes: AES-128-CTR. /// - 32 bytes: AES-256-CTR. + /// + /// WHY(#229) this is checked at deserialization: the rule above was a + /// comment, and any length was accepted. A mistyped key in a config file + /// then travelled all the way to AES, which rejected it, and the channel was + /// skipped as though it had simply been unencrypted. + /// + /// The single-byte channel-index form that [`crate::crypto::resolve_psk`] + /// also accepts is deliberately not permitted here: that shorthand comes off + /// the wire, and an operator writing a config means a key. + #[serde(deserialize_with = "deserialize_psk")] pub psk: Vec, } +/// Accept only the PSK lengths [`ChannelPsk::psk`] documents. +fn deserialize_psk<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + use serde::Deserialize as _; + + let bytes = Vec::::deserialize(deserializer)?; + if matches!( + bytes.len(), + 0 | crate::crypto::AES128_KEY_LEN | crate::crypto::AES256_KEY_LEN + ) { + return Ok(bytes); + } + Err(serde::de::Error::invalid_length( + bytes.len(), + &"0, 16, or 32 bytes", + )) +} + /// Store-and-forward server configuration. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(default, deny_unknown_fields)] @@ -438,6 +468,37 @@ impl Default for MessageConfig { mod tests { use super::*; + /// WHY(#229): the length rule was a doc comment and nothing enforced it, so + /// a mistyped key reached AES, was rejected there, and the channel was + /// skipped as though it had been unencrypted on purpose. + #[test] + fn a_psk_of_the_wrong_length_is_refused_at_the_config_boundary() { + for len in [1usize, 7, 15, 17, 31, 33, 64] { + let toml = format!( + "[[channel_psk]]\nindex = 0\nname = \"x\"\npsk = [{}]\n", + vec!["1"; len].join(", ") + ); + let parsed: Result = toml::from_str(&toml); + assert!( + parsed.is_err(), + "a {len}-byte PSK must be refused, not carried to AES" + ); + } + } + + /// Anti-vacuity: the documented lengths must still parse. + #[test] + fn the_documented_psk_lengths_parse() { + for len in [0usize, 16, 32] { + let toml = format!( + "[[channel_psk]]\nindex = 0\nname = \"x\"\npsk = [{}]\n", + vec!["1"; len].join(", ") + ); + let parsed: Result = toml::from_str(&toml); + assert!(parsed.is_ok(), "a {len}-byte PSK is documented as valid"); + } + } + const SAMPLE_TOML: &str = r#" [[connections]] Serial = { port = "/dev/ttyUSB0", baud = 115200 } diff --git a/crates/kerykeion/src/crypto.rs b/crates/kerykeion/src/crypto.rs index 5129766..f615a8a 100644 --- a/crates/kerykeion/src/crypto.rs +++ b/crates/kerykeion/src/crypto.rs @@ -52,20 +52,57 @@ pub(crate) fn build_nonce(packet_id: u32, from_node: u32) -> [u8; 16] { /// Resolve a PSK to its full-length key bytes. /// -/// - Empty slice → `None` (channel has no encryption). +/// Length of an AES-128 key, in bytes. +/// +/// WHY named here rather than written inline: the accepted PSK lengths are one +/// fact used by both this module and the config boundary that validates what an +/// operator writes. Two literals would be free to disagree. +pub(crate) const AES128_KEY_LEN: usize = 16; + +/// Length of an AES-256 key, in bytes. See [`AES128_KEY_LEN`]. +pub(crate) const AES256_KEY_LEN: usize = 32; + +/// What a channel's PSK bytes resolve to. +/// +/// WHY(#229) three states rather than an `Option`: a PSK of an undefined shape +/// used to be indistinguishable from a channel that carries no encryption. Both +/// ended in the same silent `continue`, so a misconfigured channel looked +/// exactly like a public one and nothing said otherwise. +#[derive(Debug, PartialEq, Eq)] +pub(crate) enum PskResolution { + /// The channel carries no encryption. + Unencrypted, + /// A usable AES key. + Key(Vec), + /// The bytes match no shape this protocol defines. + Undefined { + /// Length of the rejected material. The bytes themselves are key + /// material and are not carried into a log line. + len: usize, + }, +} + +/// - Empty slice → [`PskResolution::Unencrypted`]. /// - Single byte `n` (1–10) → [`DEFAULT_PSK`] with byte 15 SET to `n`. /// - 16 or 32 bytes → used as-is. -/// -/// Returns `None` if the PSK is empty (unencrypted channel), otherwise `Some(key)`. -pub(crate) fn resolve_psk(psk: &[u8]) -> Option> { +/// - Anything else → [`PskResolution::Undefined`]. +pub(crate) fn resolve_psk(psk: &[u8]) -> PskResolution { match psk { - [] => None, + [] => PskResolution::Unencrypted, [n] if *n >= 1 && *n <= 10 => { let mut key = DEFAULT_PSK; key[15] = *n; // kanon:ignore RUST/indexing-slicing -- key is fixed-size [u8; 16], index 15 is compile-time bounded - Some(key.to_vec()) + PskResolution::Key(key.to_vec()) } - _ => Some(psk.to_vec()), + // WHY(#229) the two accepted key lengths are named rather than left to a + // catch-all: the arm below used to be `_ => Some(psk.to_vec())`, which + // handed AES whatever it was given. A single byte of 0, or of 11, or a + // seven-byte blob, all became "keys" — the doc above said 16 or 32 and + // the code accepted every length. + [_, ..] if psk.len() == AES128_KEY_LEN || psk.len() == AES256_KEY_LEN => { + PskResolution::Key(psk.to_vec()) + } + other => PskResolution::Undefined { len: other.len() }, } } @@ -126,9 +163,20 @@ pub fn encrypt( from_node: u32, psk: &[u8], ) -> Result, Error> { - let Some(key) = resolve_psk(psk) else { + let key = match resolve_psk(psk) { + PskResolution::Key(key) => key, // Unencrypted channel: return plaintext unchanged. - return Ok(plaintext.to_vec()); + PskResolution::Unencrypted => return Ok(plaintext.to_vec()), + // WHY(#229) an error rather than a pass-through: sending is the + // dangerous direction. A misconfigured PSK previously became a bad key, + // and returning the plaintext instead would transmit in the clear on a + // channel the operator believes is encrypted. + PskResolution::Undefined { len } => { + return EncryptionSnafu { + detail: format!("channel PSK is {len} bytes, which defines no key"), + } + .fail(); + } }; let mut buf = plaintext.to_vec(); apply_aes_ctr(&mut buf, packet_id, from_node, &key)?; @@ -152,9 +200,22 @@ pub fn decrypt( channel_psks: &[(usize, Vec)], ) -> Result<(Vec, usize), Error> { for (channel_idx, psk) in channel_psks { - let Some(key) = resolve_psk(psk) else { + let key = match resolve_psk(psk) { + PskResolution::Key(key) => key, // Skip unencrypted-channel PSKs. - continue; + PskResolution::Unencrypted => continue, + // WHY(#229) logged rather than skipped silently: this used to be + // indistinguishable from an unencrypted channel, so a channel that + // simply could not decrypt anything looked like one that was never + // meant to. + PskResolution::Undefined { len } => { + tracing::warn!( + channel = channel_idx, + psk_len = len, + "skipping channel whose PSK defines no key" + ); + continue; + } }; let mut candidate = ciphertext.to_vec(); @@ -178,6 +239,19 @@ pub fn decrypt( mod tests { use super::*; + /// The key a PSK resolves to, if it resolves to one. + /// + /// Returns an `Option` rather than panicking so the call sites keep their + /// existing `unwrap` — and with it the `#[expect(clippy::unwrap_used)]` + /// attributes that would otherwise go unfulfilled, which is itself an error + /// under this workspace's lints. + fn key_of(psk: &[u8]) -> Option> { + match resolve_psk(psk) { + PskResolution::Key(key) => Some(key), + PskResolution::Unencrypted | PskResolution::Undefined { .. } => None, + } + } + // ── Nonce construction ────────────────────────────────────────────────── #[test] @@ -221,20 +295,58 @@ mod tests { #[test] fn psk_empty_returns_none() { - assert!(resolve_psk(&[]).is_none()); + assert_eq!(resolve_psk(&[]), PskResolution::Unencrypted); + } + + /// WHY(#229): the arm these fall into used to be `_ => Some(psk.to_vec())`, + /// so each of these became an AES "key" of its own length. A single byte of + /// 0 or 11 is an index this protocol does not define, not a one-byte key. + #[test] + fn a_psk_of_no_defined_shape_is_not_a_key() { + for psk in [ + vec![0x00], // index 0 + vec![0x0B], // index 11, one past the defined range + vec![0xFF], // index 255 + vec![0xAA; 7], // neither key length + vec![0xAA; 15], // one short of AES-128 + vec![0xAA; 31], // one short of AES-256 + vec![0xAA; 33], // one over AES-256 + ] { + assert_eq!( + resolve_psk(&psk), + PskResolution::Undefined { len: psk.len() }, + "a {}-byte PSK defines no key", + psk.len() + ); + } + } + + /// Anti-vacuity: the shapes the protocol does define must still resolve, or + /// the case above would pass against a function that rejects everything. + #[test] + fn the_defined_psk_shapes_still_resolve() { + assert_eq!(resolve_psk(&[]), PskResolution::Unencrypted); + for index in 1..=10u8 { + assert!( + key_of(&[index]).is_some(), + "index {index} is within the defined range" + ); + } + assert!(key_of(&[0xAA; AES128_KEY_LEN]).is_some()); + assert!(key_of(&[0xAA; AES256_KEY_LEN]).is_some()); } #[test] fn psk_0x01_resolves_to_default_key() { #[expect(clippy::unwrap_used, reason = "test-only")] - let key = resolve_psk(&[0x01]).unwrap(); + let key = key_of(&[0x01]).unwrap(); assert_eq!(key, DEFAULT_PSK.to_vec()); } #[test] fn psk_0x05_sets_last_byte() { #[expect(clippy::unwrap_used, reason = "test-only")] - let key = resolve_psk(&[0x05]).unwrap(); + let key = key_of(&[0x05]).unwrap(); assert_eq!(key.len(), 16); assert_eq!(key.last().copied(), Some(0x05u8)); // key and DEFAULT_PSK are both 16 bytes; get(..15) never returns None. @@ -247,7 +359,7 @@ mod tests { #[test] fn psk_0x0a_sets_last_byte() { #[expect(clippy::unwrap_used, reason = "test-only")] - let key = resolve_psk(&[0x0A]).unwrap(); + let key = key_of(&[0x0A]).unwrap(); assert_eq!(key.last().copied(), Some(0x0Au8)); } @@ -255,7 +367,7 @@ mod tests { fn psk_16_bytes_used_as_is() { let raw = [0xAAu8; 16]; #[expect(clippy::unwrap_used, reason = "test-only")] - let key = resolve_psk(&raw).unwrap(); + let key = key_of(&raw).unwrap(); assert_eq!(key, raw.to_vec()); } @@ -263,7 +375,7 @@ mod tests { fn psk_32_bytes_used_as_is() { let raw = [0xBBu8; 32]; #[expect(clippy::unwrap_used, reason = "test-only")] - let key = resolve_psk(&raw).unwrap(); + let key = key_of(&raw).unwrap(); assert_eq!(key, raw.to_vec()); }