diff --git a/src/hal/common/led.rs b/src/hal/common/led.rs new file mode 100644 index 0000000..70586ec --- /dev/null +++ b/src/hal/common/led.rs @@ -0,0 +1,116 @@ +//! Shared parsing for the RS-Key LED status block. +//! +//! Both the CCID Vendor/LED applet (`GET LED 0x11`) and the FIDO CTAPHID +//! `0x41 CONFIG_READ` (target LED) return the same `EF_LED_CONF` block. Its +//! current layout is `[steady, (effect, color, brightness, speed) × 4]` +//! (17 bytes); older firmware returns a 13-byte (pre-speed) or 9-byte +//! (pre-effect) block. The per-status stride is `(len - 1) / 4`, mirroring +//! RS-Key's own `rsk_led::load_block`. Only `color` and `brightness` are +//! surfaced to the config UI. + +/// Number of device-status slots (idle, processing, touch, boot). +const N_STATUS: usize = 4; + +/// Parse an `EF_LED_CONF` block into `(steady, [(color, brightness); 4])`. +/// +/// `data` is the raw config block with no CBOR wrapper or status-word suffix. +/// Returns `None` when the block is too short to hold four status records +/// (stride `< 2`), so a malformed/truncated read fails cleanly rather than +/// reporting garbage colours. +pub fn parse_led_block(data: &[u8]) -> Option<(bool, [(u8, u8); N_STATUS])> { + if data.is_empty() { + return None; + } + + let stride = (data.len() - 1) / N_STATUS; + if stride < 2 { + return None; + } + + // color then brightness sit right after the optional leading effect byte: + // stride >= 3 (with effect) puts them at record offset +1/+2, the pre-effect + // stride-2 block starts with colour at offset +0. + let color_off = if stride >= 3 { 1 } else { 0 }; + let steady = data[0] != 0; + let mut statuses = [(0u8, 0u8); N_STATUS]; + for (i, slot) in statuses.iter_mut().enumerate() { + let base = 1 + stride * i + color_off; + *slot = (*data.get(base)?, *data.get(base + 1)?); + } + + Some((steady, statuses)) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_current_17_byte_block() { + // [steady, (effect, color, brightness, speed) × 4] + let block = [ + 0x01, // steady + 0x00, 0x02, 0x40, 0x00, // idle: green, br 0x40 + 0x01, 0x03, 0x20, 0x05, // proc: blue, br 0x20 + 0x02, 0x04, 0x10, 0x0F, // touch: yellow, br 0x10 + 0x00, 0x01, 0x08, 0x00, // boot: red, br 0x08 + ]; + let (steady, statuses) = parse_led_block(&block).unwrap(); + assert!(steady); + assert_eq!(statuses, [(2, 0x40), (3, 0x20), (4, 0x10), (1, 0x08)]); + } + + #[test] + fn effect_byte_is_not_mistaken_for_colour() { + // Regression: a 17-byte block whose effect bytes differ from the colours. + // The old stride-2 parse read the effect byte as the colour. + let mut block = [0u8; 17]; + block[0] = 0; // steady = false + for i in 0..4 { + block[1 + 4 * i] = 0x03; // effect (would be misread as colour) + block[2 + 4 * i] = (i as u8) + 1; // colour 1..=4 + block[3 + 4 * i] = 0x11 * (i as u8 + 1); // brightness + block[4 + 4 * i] = 0x00; // speed + } + let (steady, statuses) = parse_led_block(&block).unwrap(); + assert!(!steady); + assert_eq!(statuses, [(1, 0x11), (2, 0x22), (3, 0x33), (4, 0x44)]); + } + + #[test] + fn parses_pre_speed_13_byte_block() { + // [steady, (effect, color, brightness) × 4] + let block = [ + 0x00, // steady + 0x00, 0x02, 0x40, // idle + 0x00, 0x03, 0x20, // proc + 0x00, 0x04, 0x10, // touch + 0x00, 0x01, 0x08, // boot + ]; + let (steady, statuses) = parse_led_block(&block).unwrap(); + assert!(!steady); + assert_eq!(statuses, [(2, 0x40), (3, 0x20), (4, 0x10), (1, 0x08)]); + } + + #[test] + fn parses_pre_effect_9_byte_block() { + // [steady, (color, brightness) × 4] + let block = [ + 0x01, // steady + 0x02, 0x40, // idle + 0x03, 0x20, // proc + 0x04, 0x10, // touch + 0x01, 0x08, // boot + ]; + let (steady, statuses) = parse_led_block(&block).unwrap(); + assert!(steady); + assert_eq!(statuses, [(2, 0x40), (3, 0x20), (4, 0x10), (1, 0x08)]); + } + + #[test] + fn rejects_blocks_too_short_for_four_statuses() { + assert!(parse_led_block(&[]).is_none()); + assert!(parse_led_block(&[0x01]).is_none()); + assert!(parse_led_block(&[0x01, 0x02, 0x40, 0x03, 0x20, 0x04, 0x10, 0x01]).is_none()); + } +} diff --git a/src/hal/common/mod.rs b/src/hal/common/mod.rs index 350507a..e3c84cf 100644 --- a/src/hal/common/mod.rs +++ b/src/hal/common/mod.rs @@ -1,6 +1,9 @@ -//! Shared COSE algorithm/curve/key-parameter definitions and firmware-version parsing. +//! Shared COSE algorithm/curve/key-parameter definitions, firmware-version +//! parsing, and the RS-Key LED status-block codec. pub mod cose; +pub mod led; pub mod version; +pub use led::parse_led_block; pub use version::FirmwareVersion; diff --git a/src/hal/fido/constants.rs b/src/hal/fido/constants.rs index 595f9b2..2508bf7 100644 --- a/src/hal/fido/constants.rs +++ b/src/hal/fido/constants.rs @@ -718,8 +718,9 @@ pub const RSKEY_CTAPHID_VENDOR_CMD: u8 = 0x41; /// RS-Key CONFIG_READ sub-command ID (0x0D). /// -/// Reads device configuration over FIDO. Supports DEV_CONF (0x00), -/// PHY (0x01), and LED (0x02) targets. Ungated — no PIN needed. +/// Reads device configuration over FIDO. Supports PHY (0x01) and LED (0x02) +/// targets only; DEV_CONF (0x00) is write-only over FIDO (read it via the CCID +/// Management applet). Returns a CBOR `{1: blob}` map. Ungated — no PIN needed. pub const RSKEY_CONFIG_READ: u8 = 0x0D; /// RS-Key CONFIG_WRITE sub-command ID (0x0C). diff --git a/src/hal/fido/mod.rs b/src/hal/fido/mod.rs index b8b43f0..10aee06 100644 --- a/src/hal/fido/mod.rs +++ b/src/hal/fido/mod.rs @@ -598,31 +598,12 @@ pub(crate) struct ManagementInfo { pub(crate) fn read_rskey_management_info( transport: &HidTransport, ) -> Result { - match transport.rs_key_config_read(RSKEY_CFG_TARGET_DEV_CONF) { - Ok(raw) if raw.len() > 1 => { - let data = if raw.first().copied() == Some(raw.len().saturating_sub(1) as u8) { - &raw[1..] - } else { - &raw[..] - }; - parse_management_info(data).map_err(|e| { - PFError::Device(format!("Failed to parse RS-Key management config: {e}")) - }) - } - Ok(_) => Err(PFError::Device( - "RS-Key FIDO management config response too short".to_string(), - )), - Err(_) => { - // DEV_CONF target not readable — fall back to legacy - // 0xC2 management info read (same as pico-fido). - read_management_info(transport).ok_or_else(|| { - PFError::Device( - "Failed to read management info over FIDO (0x41 and 0xC2 both rejected)" - .to_string(), - ) - }) - } - } + // Enabled-apps info is NOT readable over the 0x41 CONFIG_READ path — the + // firmware exposes only PHY/LED there and rejects DEV_CONF. Both RS-Key and + // pico-fido answer the CTAPHID vendor command 0xC2 (logical READ_CONFIG) + // with the Management DeviceInfo TLV, so read it there directly. + read_management_info(transport) + .ok_or_else(|| PFError::Device("Failed to read management info over FIDO".to_string())) } /// Read full device status (info, config, security flags) over the FIDO HID transport. @@ -908,27 +889,16 @@ fn read_legacy_physical_config(transport: &HidTransport, mut config: AppConfig) /// Read PHY configuration from an RS-Key via CTAPHID 0x41 CONFIG_READ. /// /// Falls back to returning the unchanged config if the command is not -/// supported by the device. +/// supported by the device (pre-v0.3.1 firmware) or the transport errors. fn read_rskey_physical_config(transport: &HidTransport, mut config: AppConfig) -> AppConfig { - let Ok(raw) = transport.rs_key_config_read(RSKEY_CFG_TARGET_PHY) else { - log::info!("RS-Key FIDO config read unavailable (transport error)"); + // `rs_key_config_read` unwraps the CBOR `{1: blob}` and returns the raw + // `EF_PHY` TLV record — a bare `TAG LEN VALUE` sequence, no length prefix. + let Ok(data) = transport.rs_key_config_read(RSKEY_CFG_TARGET_PHY) else { + log::info!("RS-Key FIDO config read unavailable (pre-v0.3.1 firmware or transport error)"); return config; }; - if raw.len() <= 1 { - log::info!( - "RS-Key FIDO config read unavailable (response len={}, likely pre-v0.3.1 firmware)", - raw.len() - ); - return config; - } - - let data = if raw.first().copied() == Some(raw.len().saturating_sub(1) as u8) { - &raw[1..] - } else { - &raw[..] - }; - + let data = &data[..]; let mut i = 0; while i + 1 < data.len() { if i + 2 > data.len() { @@ -948,13 +918,13 @@ fn read_rskey_physical_config(transport: &HidTransport, mut config: AppConfig) - config.pid = format!("{:04X}", u16::from_be_bytes([field_data[2], field_data[3]])); } RSKEY_PHY_TAG_LED_GPIO if !field_data.is_empty() => { - config.led_gpio = field_data[0]; + config.led_gpio = Some(field_data[0]); } RSKEY_PHY_TAG_LED_BRIGHTNESS if !field_data.is_empty() => { - config.led_brightness = field_data[0]; + config.led_brightness = Some(field_data[0]); } RSKEY_PHY_TAG_PRESENCE_TIMEOUT if !field_data.is_empty() => { - config.touch_timeout = field_data[0]; + config.touch_timeout = Some(field_data[0]); } RSKEY_PHY_TAG_USB_PRODUCT => { let product_str = std::str::from_utf8(field_data) @@ -969,12 +939,14 @@ fn read_rskey_physical_config(transport: &HidTransport, mut config: AppConfig) - config.led_steady = opts & RSKEY_OPT_LED_STEADY != 0; } RSKEY_PHY_TAG_CURVES if field_data.len() == 4 => { - config.raw_curves_mask = Some(u32::from_be_bytes([ + let mask = u32::from_be_bytes([ field_data[0], field_data[1], field_data[2], field_data[3], - ])); + ]); + config.raw_curves_mask = Some(mask); + config.enable_secp256k1 = mask & 0x08 != 0; // SECP256K1, mirrors the CCID read } RSKEY_PHY_TAG_LED_DRIVER if !field_data.is_empty() => { config.led_driver = Some(field_data[0]); @@ -999,8 +971,9 @@ fn read_rskey_physical_config(transport: &HidTransport, mut config: AppConfig) - /// Build a PHY TLV blob from `AppConfigInput` for RS-Key CONFIG_WRITE. /// /// The TLV format matches the Rescue PHY record and is sent as-is -/// to the RS-Key 0x41 CONFIG_WRITE handler. -fn build_rskey_phy_tlv(config: &AppConfigInput) -> Vec { +/// to the RS-Key 0x41 CONFIG_WRITE handler. Errors if a field can't fit +/// the record (e.g. an over-long product name). +fn build_rskey_phy_tlv(config: &AppConfigInput) -> Result, PFError> { let mut tlv = Vec::new(); if let (Some(vid_str), Some(pid_str)) = (&config.vid, &config.pid) @@ -1055,6 +1028,11 @@ fn build_rskey_phy_tlv(config: &AppConfigInput) -> Vec { if let Some(name) = config.product_name.as_deref().filter(|n| !n.is_empty()) { let bytes = name.as_bytes(); + // Firmware accepts a product record of 1..=33 bytes (name + trailing NUL), + // so the name must be <= 32 bytes — reject rather than emit a wrapped length. + if bytes.len() + 1 > 33 { + return Err(PFError::Device("Product name too long (max 32 bytes).".into())); + } tlv.push(RSKEY_PHY_TAG_USB_PRODUCT); tlv.push((bytes.len() + 1) as u8); tlv.extend_from_slice(bytes); @@ -1099,7 +1077,7 @@ fn build_rskey_phy_tlv(config: &AppConfigInput) -> Vec { tlv.push(val); } - tlv + Ok(tlv) } /// Write PHY config to an RS-Key via CTAPHID 0x41 CONFIG_WRITE. @@ -1108,23 +1086,25 @@ fn write_rskey_config( config: &AppConfigInput, pin: &str, ) -> Result { - let tlv = build_rskey_phy_tlv(config); + let tlv = build_rskey_phy_tlv(config)?; if tlv.is_empty() { return Ok("No RS-Key configuration changes were needed.".to_string()); } - // Probe: CONFIG_READ (0x41 subcommand 0x0D) is ungated and confirms - // the device supports the 0x41 CONFIG_WRITE/CONFIG_READ commands - // (RS-Key v0.3.1+). Pre-v0.3.1 devices return a CTAP error byte, which - // we detect as a response with len <= 1. - let cfg_read_resp = transport.rs_key_config_read(RSKEY_CFG_TARGET_PHY)?; - if cfg_read_resp.len() <= 1 { - return Err(PFError::Device( - "This RS-Key firmware does not support FIDO configuration. \ - Please use Rescue mode (CCID/PCSC) or update to RS-Key v0.3.1+." - .into(), - )); - } + // Probe: CONFIG_READ (0x41 subcommand 0x0D) is ungated and, on success, + // confirms the device supports the 0x41 CONFIG_WRITE/CONFIG_READ commands + // (RS-Key v0.3.1+). Pre-v0.3.1 firmware rejects it with a CTAP error, which + // surfaces here as `Err`. A supported device may legitimately return an + // empty PHY blob, so success alone is the signal — never the blob length. + transport + .rs_key_config_read(RSKEY_CFG_TARGET_PHY) + .map_err(|_| { + PFError::Device( + "This RS-Key firmware does not support FIDO configuration. \ + Please use Rescue mode (CCID/PCSC) or update to RS-Key v0.3.1+." + .into(), + ) + })?; let pin_token = transport .get_pin_token_with_permission(pin, PinUvAuthTokenPermissions::AUTHENTICATOR_CONFIG, None) @@ -1215,6 +1195,12 @@ fn is_empty_config_input(config: &AppConfigInput) -> bool { && config.power_cycle_on_reset.is_none() && config.led_steady.is_none() && config.enable_secp256k1.is_none() + // RS-Key-only PHY fields — `build_rskey_phy_tlv` writes these, so a + // change to any of them alone must not be treated as a no-op. + && config.raw_curves_mask.is_none() + && config.led_order.is_none() + && config.enabled_usb_itf.is_none() + && config.led_num.is_none() } fn validate_fido_config_changes( @@ -1477,47 +1463,12 @@ const RSKEY_LED_CONF_LEN: usize = 17; /// [`LedStatusConfig`] type, keeping only color and brightness /// for backward compatibility with the Rescue LED UI. pub(crate) fn read_rskey_led_config(transport: &HidTransport) -> Result { - let raw = transport.rs_key_config_read(RSKEY_CFG_TARGET_LED)?; - if raw.len() < RSKEY_LED_CONF_LEN { - return Err(PFError::Device(format!( - "LED config response too short: {} bytes (expected {})", - raw.len(), - RSKEY_LED_CONF_LEN, - ))); - } - - let data = if raw.first().copied() == Some(raw.len().saturating_sub(1) as u8) { - &raw[1..] - } else { - &raw[..] - }; - - if data.len() < 9 { - return Err(PFError::Device(format!( - "LED config payload too short: {} bytes", - data.len(), - ))); - } - - let steady = data[0] != 0; - let statuses = if data.len() >= RSKEY_LED_CONF_LEN { - // Full block: [steady, (effect, color, brightness, speed) × N] - let mut s = [(0u8, 0u8); 4]; - for (i, slot) in s.iter_mut().enumerate() { - *slot = (data[2 + 4 * i], data[3 + 4 * i]); // color, brightness - } - s - } else { - // Legacy 9-byte block: [steady, (color, brightness) × N] - let mut s = [(0u8, 0u8); 4]; - for (i, slot) in s.iter_mut().enumerate() { - let off = 1 + 2 * i; - if off + 1 < data.len() { - *slot = (data[off], data[off + 1]); - } - } - s - }; + // `rs_key_config_read` unwraps the CBOR `{1: blob}`, so `data` is the raw + // `EF_LED_CONF` block. `parse_led_block` handles the 17/13/9-byte layouts. + let data = transport.rs_key_config_read(RSKEY_CFG_TARGET_LED)?; + let (steady, statuses) = crate::hal::common::parse_led_block(&data).ok_or_else(|| { + PFError::Device(format!("LED config response too short: {} bytes", data.len())) + })?; log::info!( "RS-Key FIDO LED config: steady={}, statuses={:?}", @@ -1539,14 +1490,20 @@ pub(crate) fn write_rskey_led_config( config: &LedStatusConfig, pin: &str, ) -> Result { + // Read-modify-write: overwrite only steady/color/brightness so a WS2812 effect + // + speed set out-of-band (e.g. `rsk led`) survives a colour change. Fall back + // to a fresh solid block (effect/speed = 0) if the current one can't be read. let mut block = [0u8; RSKEY_LED_CONF_LEN]; + if let Ok(current) = transport.rs_key_config_read(RSKEY_CFG_TARGET_LED) + && current.len() >= RSKEY_LED_CONF_LEN + { + block.copy_from_slice(¤t[..RSKEY_LED_CONF_LEN]); + } block[0] = if config.steady { 0x01 } else { 0x00 }; for (i, &(color, brightness)) in config.statuses.iter().enumerate() { - let off = 1 + 4 * i; - block[off] = 0x00; // effect = solid - block[off + 1] = color & 0x07; - block[off + 2] = brightness; - block[off + 3] = 0x00; // speed = default + // effect (1+4i) and speed (4+4i) are left untouched. + block[2 + 4 * i] = color & 0x07; + block[3 + 4 * i] = brightness; } let pin_token = transport.get_pin_token_with_permission( @@ -1971,4 +1928,54 @@ mod tests { c.vid = Some("FEFF".to_string()); assert!(!is_empty_config_input(&c)); } + + #[test] + fn test_is_empty_config_input_counts_rskey_only_fields() { + // A change to any RS-Key-only PHY field alone must not be a no-op, + // otherwise write_config drops it before build_rskey_phy_tlv runs. + for c in [ + { + let mut c = empty_config_input(); + c.raw_curves_mask = Some(0x08); + c + }, + { + let mut c = empty_config_input(); + c.led_order = Some(1); + c + }, + { + let mut c = empty_config_input(); + c.enabled_usb_itf = Some(0x05); + c + }, + { + let mut c = empty_config_input(); + c.led_num = Some(3); + c + }, + ] { + assert!(!is_empty_config_input(&c)); + } + } + + #[test] + fn test_build_rskey_phy_tlv_rejects_overlong_product_name() { + let mut c = empty_config_input(); + c.product_name = Some("x".repeat(32)); // 32 + NUL = 33, the firmware max + assert!(build_rskey_phy_tlv(&c).is_ok()); + c.product_name = Some("x".repeat(33)); // 33 + NUL = 34, over the limit + assert!(build_rskey_phy_tlv(&c).is_err()); + } + + #[test] + fn test_build_rskey_phy_tlv_emits_rskey_only_tags() { + let mut c = empty_config_input(); + c.led_num = Some(3); + c.led_order = Some(1); + let tlv = build_rskey_phy_tlv(&c).unwrap(); + // tag 0x0E len 0x01 val 0x03, and tag 0x0D len 0x01 val 0x01 + assert!(tlv.windows(3).any(|w| w == [0x0E, 0x01, 0x03])); + assert!(tlv.windows(3).any(|w| w == [0x0D, 0x01, 0x01])); + } } diff --git a/src/hal/fido/ops.rs b/src/hal/fido/ops.rs index 56133f2..2cc0d41 100644 --- a/src/hal/fido/ops.rs +++ b/src/hal/fido/ops.rs @@ -1432,15 +1432,16 @@ impl FidoOperations for HidTransport { Ok(()) } - /// Read physical configuration from an RS-Key via CTAPHID 0x41 CONFIG_READ. + /// Read a device-config record from an RS-Key via CTAPHID 0x41 CONFIG_READ. /// /// Sends `{1: 0x0D, 2: {1: target}}` CBOR payload to the RS-Key vendor - /// command handler inside a CTAPHID_CBOR message with the vendor sub-command - /// prefix. Returns raw TLV bytes for the requested target. - /// Ungated — no PIN needed. + /// command handler inside a CTAPHID_CBOR message. The firmware answers with + /// a CBOR map `{1: blob}`; this unwraps key 1 and returns the raw record + /// bytes. Ungated — no PIN needed. /// - /// Targets: `RSKEY_CFG_TARGET_DEV_CONF` (0x00), `RSKEY_CFG_TARGET_PHY` (0x01), - /// `RSKEY_CFG_TARGET_LED` (0x02). + /// Targets: `RSKEY_CFG_TARGET_PHY` (0x01) and `RSKEY_CFG_TARGET_LED` (0x02). + /// `DEV_CONF` (0x00) is write-only over FIDO — the firmware rejects it here + /// (readable only via the CCID Management applet), so this returns an error. fn rs_key_config_read(&self, target: u8) -> Result, PFError> { let mut params = BTreeMap::new(); params.insert(Value::Integer(1), Value::Integer(RSKEY_CONFIG_READ as i128)); @@ -1453,7 +1454,20 @@ impl FidoOperations for HidTransport { let mut full_payload = vec![RSKEY_CTAPHID_VENDOR_CMD]; full_payload.extend(inner); - self.send_cbor(CTAPHID_CBOR, &full_payload) + let resp = self.send_cbor(CTAPHID_CBOR, &full_payload)?; + + // Response is CBOR `{1: blob(bstr)}` — unwrap key 1 to the raw record. + match from_slice::(&resp) { + Ok(Value::Map(m)) => match m.get(&Value::Integer(1)) { + Some(Value::Bytes(b)) => Ok(b.clone()), + _ => Err(PFError::Device( + "CONFIG_READ response missing blob (key 1)".into(), + )), + }, + _ => Err(PFError::Device( + "CONFIG_READ response is not a CBOR map".into(), + )), + } } /// Write physical configuration to an RS-Key via CTAPHID 0x41 CONFIG_WRITE. diff --git a/src/hal/io.rs b/src/hal/io.rs index c83ae1f..04789a6 100644 --- a/src/hal/io.rs +++ b/src/hal/io.rs @@ -70,8 +70,8 @@ pub fn read_device_details() -> Result { } else { fido.config.pid }, - led_gpio: rescue.config.led_gpio, - led_brightness: rescue.config.led_brightness, + led_gpio: rescue.config.led_gpio.or(fido.config.led_gpio), + led_brightness: rescue.config.led_brightness.or(fido.config.led_brightness), led_dimmable: rescue.config.led_dimmable, power_cycle_on_reset: rescue.config.power_cycle_on_reset, led_steady: rescue.config.led_steady, @@ -83,8 +83,14 @@ pub fn read_device_details() -> Result { None } }), - product_name: rescue.config.product_name, - touch_timeout: rescue.config.touch_timeout, + // Prefer the phy record's product override; fall back to the + // FIDO transport's USB product string when it has none. + product_name: if !rescue.config.product_name.is_empty() { + rescue.config.product_name + } else { + fido.config.product_name + }, + touch_timeout: rescue.config.touch_timeout.or(fido.config.touch_timeout), raw_curves_mask: rescue.config.raw_curves_mask, led_order: rescue.config.led_order, enabled_usb_itf: rescue.config.enabled_usb_itf, diff --git a/src/hal/rescue/constants.rs b/src/hal/rescue/constants.rs index 6679bf6..64d0bba 100644 --- a/src/hal/rescue/constants.rs +++ b/src/hal/rescue/constants.rs @@ -279,8 +279,8 @@ pub enum PhyTag { /// Touch presence timeout. /// - /// Data format: `[TIMEOUT_MS_LSB, TIMEOUT_MS_MSB]` (2 bytes). - /// Timeout in milliseconds for user presence verification. + /// Data format: `[TIMEOUT_SECONDS]` (1 byte); `0`/absent = firmware default. + /// How long the device waits for a user touch before giving up. PresenceTimeout = 0x08, /// USB product string. diff --git a/src/hal/rescue/ops.rs b/src/hal/rescue/ops.rs index 53cd80e..81f25f1 100644 --- a/src/hal/rescue/ops.rs +++ b/src/hal/rescue/ops.rs @@ -327,17 +327,17 @@ impl RescueOperations for PcscTransport { } PhyTag::LedGpio => { if !field_data.is_empty() { - config.led_gpio = field_data[0]; + config.led_gpio = Some(field_data[0]); } } PhyTag::LedBrightness => { if !field_data.is_empty() { - config.led_brightness = field_data[0]; + config.led_brightness = Some(field_data[0]); } } PhyTag::PresenceTimeout => { if !field_data.is_empty() { - config.touch_timeout = field_data[0]; + config.touch_timeout = Some(field_data[0]); } } PhyTag::UsbProduct => { @@ -553,6 +553,14 @@ impl RescueOperations for PcscTransport { tlv.push(val | UsbInterfaces::CCID.bits()); } + // LED count (Tag 0x0E) — RS-Key extension; the rescue write is full-replace, + // so emit it here too or a CCID write silently drops the configured count. + if let Some(val) = config.led_num { + tlv.push(PhyTag::LedNum as u8); + tlv.push(0x01); + tlv.push(val); + } + // 2. Connect and Send if tlv.is_empty() { log::warn!("No configuration changes to apply"); @@ -689,20 +697,16 @@ impl RescueOperations for PcscTransport { let mut rx_buf = [0; 256]; let rx = self.transmit(&apdu, &mut rx_buf)?; - if !rx.ends_with(&SW_SUCCESS) || rx.len() < 11 { + if !rx.ends_with(&SW_SUCCESS) { return Err(PFError::Device("Failed to read LED config".into())); } + // The applet returns the raw `EF_LED_CONF` block (17 bytes on current + // firmware); `parse_led_block` reads colour/brightness at the right + // stride instead of assuming the legacy 9-byte layout. let data = &rx[..rx.len() - 2]; - if data.len() < 9 { - return Err(PFError::Device("LED config response too short".into())); - } - - let steady = data[0] != 0; - let mut statuses = [(0u8, 0u8); 4]; - for s in 0..4 { - statuses[s] = (data[1 + 2 * s], data[2 + 2 * s]); - } + let (steady, statuses) = crate::hal::common::parse_led_block(data) + .ok_or_else(|| PFError::Device("LED config response too short".into()))?; log::info!("LED config: steady={}, statuses={:?}", steady, statuses); Ok(LedStatusConfig { steady, statuses }) diff --git a/src/hal/types.rs b/src/hal/types.rs index 3e30528..3d5409c 100644 --- a/src/hal/types.rs +++ b/src/hal/types.rs @@ -34,11 +34,17 @@ pub struct AppConfig { pub vid: String, pub pid: String, pub product_name: String, - /// GPIO pin the status LED is connected to. - pub led_gpio: u8, - pub led_brightness: u8, - /// Touch-button press timeout in seconds. - pub touch_timeout: u8, + /// GPIO pin the status LED is connected to. `None` = no phy override, i.e. + /// the firmware's build-time default (which the device doesn't report back). + #[serde(skip_serializing_if = "Option::is_none")] + pub led_gpio: Option, + /// Global LED brightness cap. `None` = no phy override (firmware default). + #[serde(skip_serializing_if = "Option::is_none")] + pub led_brightness: Option, + /// Touch-button press timeout in seconds. `None` = no phy override; the + /// firmware then uses its built-in default (30 s), and `0` means the same. + #[serde(skip_serializing_if = "Option::is_none")] + pub touch_timeout: Option, /// LED driver type identifier (e.g. PWM direct vs external driver). #[serde(skip_serializing_if = "Option::is_none")] pub led_driver: Option, diff --git a/src/ui/screens/config/view.rs b/src/ui/screens/config/view.rs index 7aaafc0..dfa0403 100644 --- a/src/ui/screens/config/view.rs +++ b/src/ui/screens/config/view.rs @@ -224,71 +224,6 @@ impl ConfigViewModel { .child(content) } - fn render_curves_card(&mut self, cx: &mut Context, is_fido: bool) -> impl IntoElement { - let theme = cx.theme(); - let mut rows = v_flex().gap_4(); - - let curves = [ - ("curve-p256", "P-256 (secp256r1)", self.curve_p256), - ("curve-p384", "P-384 (secp384r1)", self.curve_p384), - ("curve-p521", "P-521 (secp521r1)", self.curve_p521), - ("curve-k1", "secp256k1 (Bitcoin)", self.curve_secp256k1), - ("curve-bp256", "Brainpool 256r1", self.curve_bp256), - ("curve-bp384", "Brainpool 384r1", self.curve_bp384), - ("curve-bp512", "Brainpool 512r1", self.curve_bp512), - ("curve-ed25519", "Ed25519", self.curve_ed25519), - ("curve-ed448", "Ed448", self.curve_ed448), - ("curve-x25519", "X25519", self.curve_x25519), - ("curve-x448", "X448", self.curve_x448), - ]; - - for (id, label, checked) in curves { - let toggle_listener = cx.listener(move |this, checked, _, cx| { - match id { - "curve-p256" => this.curve_p256 = *checked, - "curve-p384" => this.curve_p384 = *checked, - "curve-p521" => this.curve_p521 = *checked, - "curve-k1" => this.curve_secp256k1 = *checked, - "curve-bp256" => this.curve_bp256 = *checked, - "curve-bp384" => this.curve_bp384 = *checked, - "curve-bp512" => this.curve_bp512 = *checked, - "curve-ed25519" => this.curve_ed25519 = *checked, - "curve-ed448" => this.curve_ed448 = *checked, - "curve-x25519" => this.curve_x25519 = *checked, - "curve-x448" => this.curve_x448 = *checked, - _ => {} - } - cx.notify(); - }); - - rows = rows.child( - h_flex() - .items_center() - .justify_between() - .child( - v_flex().gap_0p5().child(label).child( - div() - .text_sm() - .text_color(theme.muted_foreground) - .child("Cryptographic curve"), - ), - ) - .child( - Switch::new(id) - .checked(checked) - .disabled(is_fido) - .on_click(toggle_listener), - ), - ); - } - - Card::new() - .title("Supported Curves") - .description("Enable or disable cryptographic curves for RS-Key") - .icon(Icon::default().path("icons/shield.svg")) - .child(rows) - } - fn render_rskey_led_card(&mut self, cx: &mut Context, is_fido: bool) -> impl IntoElement { let theme = cx.theme(); let mut rows = v_flex().gap_4(); @@ -652,8 +587,9 @@ impl Render for ConfigViewModel { .child(options_card); if is_rskey { + // No curves card: RS-Key's firmware ignores the phy ENABLED_CURVES + // tag (curve support is compile-time), so exposing it would only mislead. inner = inner - .child(self.render_curves_card(cx, false)) .child(self.render_rskey_led_card(cx, false)) .child(self.render_rskey_apps_card(cx, false)) .child(self.render_rskey_usb_itf_card(cx, false)); diff --git a/src/ui/screens/config/view_model.rs b/src/ui/screens/config/view_model.rs index 375c7fc..f160a97 100644 --- a/src/ui/screens/config/view_model.rs +++ b/src/ui/screens/config/view_model.rs @@ -13,6 +13,11 @@ use gpui_component::input::InputState; use gpui_component::select::{SelectItem, SelectState}; use gpui_component::slider::SliderState; +/// Slider position shown for LED brightness when the device has no phy override. +/// Purely cosmetic: an unmoved slider is treated as "no override" on save, so this +/// value is never written unless the user actually drags the slider. +const DEFAULT_BRIGHTNESS: u8 = 8; + /// Known USB vendor/product identity presets for various security keys. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum UsbIdentityPreset { @@ -260,13 +265,20 @@ impl ConfigViewModel { let current_product_name: SharedString = config .map(|c| c.product_name.clone().into()) .unwrap_or_else(|| "My Key".into()); + // `None` (no phy override) → blank input, so it reads as "firmware default" + // rather than a bogus "0" and isn't written back on save. let current_led_gpio: SharedString = config - .map(|c| c.led_gpio.to_string().into()) - .unwrap_or_else(|| "25".into()); + .and_then(|c| c.led_gpio) + .map(|g| g.to_string().into()) + .unwrap_or_default(); let current_touch_timeout: SharedString = config - .map(|c| c.touch_timeout.to_string().into()) - .unwrap_or_else(|| "10".into()); - let current_brightness = config.map(|c| c.led_brightness as f32).unwrap_or(8.0); + .and_then(|c| c.touch_timeout) + .map(|t| t.to_string().into()) + .unwrap_or_default(); + let current_brightness = config + .and_then(|c| c.led_brightness) + .map(|b| b as f32) + .unwrap_or(DEFAULT_BRIGHTNESS as f32); let led_dimmable = config.map(|c| c.led_dimmable).unwrap_or(true); let led_steady = config.map(|c| c.led_steady).unwrap_or(false); @@ -337,8 +349,11 @@ impl ConfigViewModel { let product_name_input = cx.new(|cx| InputState::new(window, cx).default_value(current_product_name.clone())); - let led_gpio_input = - cx.new(|cx| InputState::new(window, cx).default_value(current_led_gpio.clone())); + let led_gpio_input = cx.new(|cx| { + InputState::new(window, cx) + .placeholder("Firmware default") + .default_value(current_led_gpio.clone()) + }); let initial_driver_idx = LedDriverType::all() .iter() @@ -384,8 +399,11 @@ impl ConfigViewModel { .default_value(current_brightness) }); - let touch_timeout_input = - cx.new(|cx| InputState::new(window, cx).default_value(current_touch_timeout.clone())); + let touch_timeout_input = cx.new(|cx| { + InputState::new(window, cx) + .placeholder("Firmware default (30s)") + .default_value(current_touch_timeout.clone()) + }); Self { device, @@ -647,6 +665,7 @@ impl ConfigViewModel { let raw_curves_mask = status.config.raw_curves_mask; let led_order = status.config.led_order; let method = status.method.clone(); + let is_rskey = status.firmware_type == crate::ui::models::device::FirmwareType::RSKey; let mut has_changes = false; @@ -665,40 +684,65 @@ impl ConfigViewModel { has_changes = true; } - let mut final_led_gpio = current_led_gpio; + // LED GPIO: an empty input means "no phy override" (firmware default) and + // is not written; a value is written only when the user typed one. let led_gpio_str = self.led_gpio_input.read(cx).text().to_string(); - if let Ok(parsed_gpio) = led_gpio_str.parse::() { - if parsed_gpio != current_led_gpio { - has_changes = true; - } - final_led_gpio = parsed_gpio; - } - - let mut final_led_driver = current_led_driver; - let driver_idx = self.led_driver_select.read(cx).selected_index(cx); - if let Some(idx) = driver_idx - && let Some(driver) = LedDriverType::all().get(idx.row) - { - let driver_value = driver.value(); - let current_driver_value = current_led_driver.unwrap_or(1); - if driver_value != current_driver_value { - has_changes = true; - } - final_led_driver = Some(driver_value); - } - - let brightness = self.led_brightness_slider.read(cx).value().start() as u8; - if brightness != current_led_brightness { + let trimmed_gpio = led_gpio_str.trim(); + let final_led_gpio = if trimmed_gpio.is_empty() { + None + } else { + trimmed_gpio.parse::().ok().or(current_led_gpio) + }; + if final_led_gpio != current_led_gpio { has_changes = true; } - let mut final_touch_timeout = current_touch_timeout; + // LED driver: preserve the device's value (None = firmware default) unless + // the user picks a different entry than the one it booted with — an + // untouched select must not clobber a virgin phy with a bogus driver. + let init_driver_idx = LedDriverType::all() + .iter() + .position(|d| Some(d.value()) == current_led_driver) + .unwrap_or(0); + let sel_driver_idx = self + .led_driver_select + .read(cx) + .selected_index(cx) + .map(|p| p.row) + .unwrap_or(init_driver_idx); + let final_led_driver = if sel_driver_idx != init_driver_idx { + LedDriverType::all().get(sel_driver_idx).map(|d| d.value()) + } else { + current_led_driver + }; + if final_led_driver != current_led_driver { + has_changes = true; + } + + // LED brightness: an unmoved slider preserves the device's value + // (None = firmware default) instead of writing its placeholder position. + let init_brightness = current_led_brightness.unwrap_or(DEFAULT_BRIGHTNESS); + let slider_brightness = self.led_brightness_slider.read(cx).value().start() as u8; + let final_led_brightness = if slider_brightness != init_brightness { + Some(slider_brightness) + } else { + current_led_brightness + }; + if final_led_brightness != current_led_brightness { + has_changes = true; + } + + // Touch timeout: empty input = "firmware default" (no override written). + // `0` firmware-side also means the 30 s default, so an empty field is honest. let touch_timeout_str = self.touch_timeout_input.read(cx).text().to_string(); - if let Ok(val) = touch_timeout_str.parse::() { - if val != current_touch_timeout { - has_changes = true; - } - final_touch_timeout = val; + let trimmed_tt = touch_timeout_str.trim(); + let final_touch_timeout = if trimmed_tt.is_empty() { + None + } else { + trimmed_tt.parse::().ok().or(current_touch_timeout) + }; + if final_touch_timeout != current_touch_timeout { + has_changes = true; } if (self.led_dimmable != current_led_dimmable) @@ -708,9 +752,17 @@ impl ConfigViewModel { has_changes = true; } - let new_curves_mask = Self::curves_mask_from_toggles(self); - let has_curve_changes = Some(new_curves_mask) != raw_curves_mask - || (raw_curves_mask.is_none() && new_curves_mask != 0); + // RS-Key ignores the phy ENABLED_CURVES tag (its curves card is hidden), + // so preserve the device's mask rather than rebuilding it from the hidden + // toggles — otherwise Apply Changes would write a meaningless curves tag. + let (has_curve_changes, built_curves_mask) = if is_rskey { + (false, raw_curves_mask) + } else { + let new_curves_mask = Self::curves_mask_from_toggles(self); + let changed = Some(new_curves_mask) != raw_curves_mask + || (raw_curves_mask.is_none() && new_curves_mask != 0); + (changed, if changed { Some(new_curves_mask) } else { raw_curves_mask }) + }; if has_curve_changes { has_changes = true; } @@ -726,18 +778,13 @@ impl ConfigViewModel { return; } - let built_curves_mask = if has_curve_changes { - Some(new_curves_mask) - } else { - raw_curves_mask - }; let changes = AppConfigInput { vid: Some(vid), pid: Some(pid), product_name: Some(product_name), - led_gpio: Some(final_led_gpio), - led_brightness: Some(brightness), - touch_timeout: Some(final_touch_timeout), + led_gpio: final_led_gpio, + led_brightness: final_led_brightness, + touch_timeout: final_touch_timeout, led_driver: final_led_driver, led_dimmable: Some(self.led_dimmable), power_cycle_on_reset: Some(self.power_cycle), @@ -750,7 +797,6 @@ impl ConfigViewModel { }; if method == DeviceMethod::Fido { - let is_rskey = status.firmware_type == crate::ui::models::device::FirmwareType::RSKey; if Self::status_supports_legacy_fido_config(status) || is_rskey { self.open_pin_dialog(changes, window, cx); } else { @@ -835,18 +881,23 @@ impl ConfigViewModel { .map(|c| c.product_name.clone()) .unwrap_or_else(|| "My Key".into()); let new_gpio = config - .map(|c| c.led_gpio.to_string()) - .unwrap_or_else(|| "25".into()); + .and_then(|c| c.led_gpio) + .map(|g| g.to_string()) + .unwrap_or_default(); let new_timeout = config - .map(|c| c.touch_timeout.to_string()) - .unwrap_or_else(|| "10".into()); + .and_then(|c| c.touch_timeout) + .map(|t| t.to_string()) + .unwrap_or_default(); self.led_dimmable = config.map(|c| c.led_dimmable).unwrap_or(true); self.led_steady = config.map(|c| c.led_steady).unwrap_or(false); self.power_cycle = config.map(|c| c.power_cycle_on_reset).unwrap_or(false); Self::sync_curve_toggles(self, config); - let brightness = config.map(|c| c.led_brightness as f32).unwrap_or(8.0); + let brightness = config + .and_then(|c| c.led_brightness) + .map(|b| b as f32) + .unwrap_or(DEFAULT_BRIGHTNESS as f32); let new_driver_val = config.and_then(|c| c.led_driver).unwrap_or(1); diff --git a/src/ui/screens/home/view.rs b/src/ui/screens/home/view.rs index f7cebd7..aebadf4 100644 --- a/src/ui/screens/home/view.rs +++ b/src/ui/screens/home/view.rs @@ -292,7 +292,12 @@ impl HomeViewModel { .text_color(theme.muted_foreground) .child("LED GPIO Pin"), ) - .child(format!("GPIO {}", config.led_gpio)), + .child( + config + .led_gpio + .map(|g| format!("GPIO {}", g)) + .unwrap_or_else(|| "Firmware default".into()), + ), ) .child( h_flex() @@ -302,7 +307,12 @@ impl HomeViewModel { .text_color(theme.muted_foreground) .child("LED Brightness"), ) - .child(config.led_brightness.to_string()), + .child( + config + .led_brightness + .map(|b| b.to_string()) + .unwrap_or_else(|| "Firmware default".into()), + ), ) .child( h_flex() @@ -312,7 +322,12 @@ impl HomeViewModel { .text_color(theme.muted_foreground) .child("Presence Touch Timeout"), ) - .child(format!("{}s", config.touch_timeout)), + .child( + config + .touch_timeout + .map(|t| format!("{}s", t)) + .unwrap_or_else(|| "Firmware default".into()), + ), ) .child( h_flex()