From 5121bb011d868ec652a459055683213174ba6294 Mon Sep 17 00:00:00 2001 From: Suyog Tandel Date: Thu, 9 Jul 2026 00:04:38 +0530 Subject: [PATCH] feat: RS-Key curve selection card, transport priority fix, RescueCurves re-export - Reversed transport priority: PCSC/Rescue preferred over FIDO - Added `RescueCurves` bitflags re-export to `hal/types.rs` - Replaced single secp256k1 toggle with full "Supported Curves" card (11 curves) gated on RS-Key, with mask-building in apply_changes - Updated FIDO/Rescue write status messages to match transport behavior - Cleaned up device status display (green "Online" for RS-Key) Changes: src/ui/screens/config/view.rs, view_model.rs, hal/io.rs, hal/transport/mod.rs, hal/types.rs. --- .github/workflows/ci.yml | 6 ++ Cargo.lock | 2 +- Cargo.toml | 2 +- README.md | 4 +- src/hal/io.rs | 34 +++---- src/hal/transport/mod.rs | 27 +++--- src/hal/types.rs | 3 + src/ui/screens/about/view.rs | 2 +- src/ui/screens/config/view.rs | 132 ++++++++++++++++++---------- src/ui/screens/config/view_model.rs | 100 ++++++++++++++++++--- 10 files changed, 219 insertions(+), 93 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a2077ee..c1f0b38 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,6 +37,12 @@ jobs: - name: Run clippy run: nix develop --command cargo clippy --all-targets -- -D warnings + - name: Check for missing documentation + run: nix develop --command bash -c 'RUSTFLAGS="-D missing_docs" cargo check --all-targets' + + - name: Check documentation builds cleanly + run: nix develop --command bash -c 'RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --document-private-items' + - name: Build project run: nix develop --command cargo build --verbose diff --git a/Cargo.lock b/Cargo.lock index 33d37b9..5c2b2b0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4206,7 +4206,7 @@ checksum = "5be167a7af36ee22fe3115051bc51f6e6c7054c9348e28deb4f49bd6f705a315" [[package]] name = "picoforge" -version = "0.6.0" +version = "0.7.0" dependencies = [ "aes 0.9.1", "anyhow", diff --git a/Cargo.toml b/Cargo.toml index d8b9eeb..e4b7ccd 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "picoforge" -version = "0.6.0" +version = "0.7.0" description = "An open source commissioning tool for Pico FIDO security keys. Developed with Rust and GPUI." authors = ["Suyog Tandel", "PicoForge Contributers"] license = "AGPL-3.0" diff --git a/README.md b/README.md index 3ddd937..91bda24 100644 --- a/README.md +++ b/README.md @@ -23,14 +23,14 @@ > Check application [Installation Wiki](https://github.com/librekeys/picoforge/wiki/Installation) for installation guide of the PicoForge app on your system. > > **Supported Firmwares:** +> - **[RSKeys](https://github.com/TheMaxMur/RS-Key)**: v0.3.X > - **[pico-fido](https://github.com/polhenarejos/pico-fido)**: v7.0, v7.2, v7.4, v7.6 > - **[LibreKeys One](https://github.com/librekeys/pico-fido-firmwares/releases)**: v7.4.2 -> - **[RSKeys](https://github.com/TheMaxMur/RS-Key)**: v0.2.8 > > **Configuration Support:** > - **pico-fido v7.0/v7.2** & **LibreKeys One v7.4.2**: Hardware configuration via FIDO mode is supported. > - **pico-fido v7.4/v7.6**: Hardware configuration requires Rescue/PCSC mode. Configuration cannot be done via FIDO-only mode. -> - **RSKeys v0.2.8**: Only FIDO-related parameters (such as managing passkeys and changing the PIN) can be modified. Changing VID/PID or other hardware configurations is not possible at this time. +> - **RSKeys v0.3.X**: Hardware configuration via FIDO mode is supported. Changes to VID/PID, LED, and other hardware settings are available through the FIDO transport. ## About diff --git a/src/hal/io.rs b/src/hal/io.rs index 6e91031..d79a8b4 100644 --- a/src/hal/io.rs +++ b/src/hal/io.rs @@ -2,8 +2,8 @@ //! //! Each public function here selects the appropriate protocol path based //! on the detected firmware type or an explicit [`DeviceMethod`] parameter. -//! Some functions (e.g. `read_device_details`) try FIDO first and merge -//! results from Rescue to produce a complete status snapshot. +//! Some functions (e.g. `read_device_details`) try Rescue (PC/SC) first, +//! then FIDO, and merge results to produce a complete status snapshot. use crate::{ error::PFError, @@ -20,20 +20,7 @@ pub fn read_device_details() -> Result { let mut rescue_status: Option = None; let mut rescue_fw_type: Option = None; - // Discover via FIDO/HID transport - match DeviceHandle::try_fido() { - Ok(Some((_handle, _identity))) => match fido::read_device_details() { - Ok(status) => { - log::info!("FIDO device details read successfully"); - fido_status = Some(status); - } - Err(e) => log::warn!("FIDO read_device_details failed: {}", e), - }, - Ok(None) => log::info!("No FIDO HID device found"), - Err(e) => log::warn!("FIDO HID discovery error: {}", e), - } - - // Discover via Rescue/PC/SC transport + // Discover via Rescue/PC/SC transport (preferred for richer details) match DeviceHandle::try_rescue() { Ok(Some((handle, _identity))) => { rescue_fw_type = Some(handle.firmware_type()); @@ -49,6 +36,19 @@ pub fn read_device_details() -> Result { Err(e) => log::warn!("Rescue PC/SC discovery error: {}", e), } + // Discover via FIDO/HID transport (fallback for FIDO-only details) + match DeviceHandle::try_fido() { + Ok(Some((_handle, _identity))) => match fido::read_device_details() { + Ok(status) => { + log::info!("FIDO device details read successfully"); + fido_status = Some(status); + } + Err(e) => log::warn!("FIDO read_device_details failed: {}", e), + }, + Ok(None) => log::info!("No FIDO HID device found"), + Err(e) => log::warn!("FIDO HID discovery error: {}", e), + } + match (fido_status, rescue_status) { (Some(fido), Some(rescue)) => { log::info!("Merging FIDO and Rescue device details"); @@ -92,7 +92,7 @@ pub fn read_device_details() -> Result { }, secure_boot: rescue.secure_boot, secure_lock: rescue.secure_lock, - method: DeviceMethod::Fido, + method: DeviceMethod::Rescue, firmware_type: fido.firmware_type, }) } diff --git a/src/hal/transport/mod.rs b/src/hal/transport/mod.rs index 3914213..caabd54 100644 --- a/src/hal/transport/mod.rs +++ b/src/hal/transport/mod.rs @@ -9,9 +9,10 @@ //! a PC/SC smart-card reader. Used when the device is in rescue/bootloader mode //! or when FIDO commands are blocked (e.g. firmware version ≥ 7.4 on pico-fido). //! -//! The [`DeviceHandle::discover`] method tries FIDO HID first and falls back to -//! PC/SC. This ensures normal operation prefers the faster HID path while still -//! allowing rescue access when needed. +//! The [`DeviceHandle::discover`] method tries PC/SC first and falls back to +//! FIDO HID. The PC/SC rescue channel provides richer device details (serial, +//! flash stats, secure boot) and does not require PIN authentication for +//! configuration writes. use std::fmt; @@ -76,18 +77,9 @@ impl DeviceHandle { } } - /// Try to discover a device via FIDO HID first, falling back to Rescue PC/SC. + /// Try to discover a device via Rescue PC/SC first, falling back to FIDO HID. #[allow(dead_code)] pub fn discover() -> Result<(Self, DeviceIdentity), PFError> { - match Self::try_fido() { - Ok(Some((handle, identity))) => { - log::info!("Device discovered via FIDO HID transport"); - return Ok((handle, identity)); - } - Ok(None) => log::info!("No FIDO HID device found"), - Err(e) => log::warn!("FIDO HID discovery error: {}", e), - } - match Self::try_rescue() { Ok(Some((handle, identity))) => { log::info!("Device discovered via Rescue PC/SC transport"); @@ -97,6 +89,15 @@ impl DeviceHandle { Err(e) => log::warn!("Rescue PC/SC discovery error: {}", e), } + match Self::try_fido() { + Ok(Some((handle, identity))) => { + log::info!("Device discovered via FIDO HID transport"); + return Ok((handle, identity)); + } + Ok(None) => log::info!("No FIDO HID device found"), + Err(e) => log::warn!("FIDO HID discovery error: {}", e), + } + Err(PFError::NoDevice) } diff --git a/src/hal/types.rs b/src/hal/types.rs index f6031bb..3e30528 100644 --- a/src/hal/types.rs +++ b/src/hal/types.rs @@ -207,6 +207,9 @@ pub struct StoredCredential { // ── Constants ─────────────────────────────────────────────────────────────── +/// Re-export curve bitflags for use by UI components. +pub use crate::hal::rescue::constants::RescueCurves; + /// AAGUID assigned to RS-Key hardware. pub const RSKEY_AAGUID: &str = "2479C7BF6B3056839EC80E8171A918B7"; /// AAGUID assigned to Pico-Fido hardware. diff --git a/src/ui/screens/about/view.rs b/src/ui/screens/about/view.rs index 8155c03..f756132 100644 --- a/src/ui/screens/about/view.rs +++ b/src/ui/screens/about/view.rs @@ -36,7 +36,7 @@ impl Render for AboutViewModel { .text_color(theme.foreground) .child("PicoForge"), ) - .child(Tag::new("v0.6.0")) + .child(Tag::new("v0.7.0")) .child( div() .text_color(theme.muted_foreground) diff --git a/src/ui/screens/config/view.rs b/src/ui/screens/config/view.rs index 9a7a896..6142dbc 100644 --- a/src/ui/screens/config/view.rs +++ b/src/ui/screens/config/view.rs @@ -187,7 +187,6 @@ impl ConfigViewModel { fn render_options_card( &mut self, cx: &mut Context, - is_fido: bool, hardware_config_disabled: bool, ) -> impl IntoElement { let power_cycle_listener = cx.listener(|this, checked, _, cx| { @@ -195,53 +194,27 @@ impl ConfigViewModel { cx.notify(); }); - let secp_listener = cx.listener(|this, checked, _, cx| { - this.enable_secp256k1 = *checked; - cx.notify(); - }); - let theme = cx.theme(); - let content = v_flex() - .gap_4() - .child( - gpui_component::h_flex() - .items_center() - .justify_between() - .child( - v_flex().gap_0p5().child("Power Cycle on Reset").child( - div() - .text_sm() - .text_color(theme.muted_foreground) - .child("Restart device on reset"), - ), - ) - .child( - Switch::new("power-cycle") - .checked(self.power_cycle) - .disabled(hardware_config_disabled) - .on_click(power_cycle_listener), + let content = v_flex().gap_4().child( + gpui_component::h_flex() + .items_center() + .justify_between() + .child( + v_flex().gap_0p5().child("Power Cycle on Reset").child( + div() + .text_sm() + .text_color(theme.muted_foreground) + .child("Restart device on reset"), ), - ) - .child( - gpui_component::h_flex() - .items_center() - .justify_between() - .child( - v_flex().gap_0p5().child("Enable Secp256k1").child( - div() - .text_sm() - .text_color(theme.muted_foreground) - .child("Does not work on Android!"), - ), - ) - .child( - Switch::new("enable-secp") - .checked(self.enable_secp256k1) - .disabled(is_fido) - .on_click(secp_listener), - ), - ); + ) + .child( + Switch::new("power-cycle") + .checked(self.power_cycle) + .disabled(hardware_config_disabled) + .on_click(power_cycle_listener), + ), + ); Card::new() .title("Device Options") @@ -250,6 +223,71 @@ 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( + gpui_component::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(); @@ -595,7 +633,7 @@ impl Render for ConfigViewModel { .render_led_card(cx, fido_no_rskey, hardware_config_disabled) .into_any_element(); let options_card = self - .render_options_card(cx, fido_no_rskey, hardware_config_disabled) + .render_options_card(cx, hardware_config_disabled) .into_any_element(); let identity_card = self @@ -613,9 +651,11 @@ impl Render for ConfigViewModel { if is_rskey { // RS-Key cards always enabled in FIDO mode — they work via // CONFIG_WRITE with PIN token. + let rskey_curves = self.render_curves_card(cx, false).into_any_element(); let rskey_led = self.render_rskey_led_card(cx, false).into_any_element(); let rskey_apps = self.render_rskey_apps_card(cx, false).into_any_element(); let rskey_usb_itf = self.render_rskey_usb_itf_card(cx, false).into_any_element(); + grid_children.push(rskey_curves); grid_children.push(rskey_led); grid_children.push(rskey_apps); grid_children.push(rskey_usb_itf); diff --git a/src/ui/screens/config/view_model.rs b/src/ui/screens/config/view_model.rs index 32293fc..876faaa 100644 --- a/src/ui/screens/config/view_model.rs +++ b/src/ui/screens/config/view_model.rs @@ -1,11 +1,13 @@ //! View model for the configuration screen — form state and save logic. +use crate::hal::types::{AppConfig, RescueCurves}; use crate::ui::app::AppModels; use crate::ui::components::dialog::PinPromptContent; use crate::ui::components::{dialog, dialog::StatusContent}; use crate::ui::models::device::{ AppConfigInput, DeviceEvent, DeviceMethod, DeviceRepo, FullDeviceStatus, LedStatusConfig, }; + use gpui::*; use gpui_component::input::InputState; use gpui_component::select::{SelectItem, SelectState}; @@ -211,7 +213,6 @@ pub struct ConfigViewModel { pub(super) led_steady: bool, pub(super) touch_timeout_input: Entity, pub(super) power_cycle: bool, - pub(super) enable_secp256k1: bool, pub(super) loading: bool, pub(super) is_custom_vendor: bool, @@ -223,6 +224,19 @@ pub struct ConfigViewModel { pub(super) usb_apps_enabled: u16, pub(super) enabled_usb_itf: Option, + // Curve toggles — initialized from raw_curves_mask, rebuilt into mask on save. + pub(super) curve_p256: bool, + pub(super) curve_p384: bool, + pub(super) curve_p521: bool, + pub(super) curve_secp256k1: bool, + pub(super) curve_bp256: bool, + pub(super) curve_bp384: bool, + pub(super) curve_bp512: bool, + pub(super) curve_ed25519: bool, + pub(super) curve_ed448: bool, + pub(super) curve_x25519: bool, + pub(super) curve_x448: bool, + pub(super) _task: Option>, } @@ -257,8 +271,11 @@ impl ConfigViewModel { let led_dimmable = config.map(|c| c.led_dimmable).unwrap_or(true); let led_steady = config.map(|c| c.led_steady).unwrap_or(false); let power_cycle = config.map(|c| c.power_cycle_on_reset).unwrap_or(false); - let enable_secp256k1 = config.map(|c| c.enable_secp256k1).unwrap_or(true); let enabled_usb_itf = config.and_then(|c| c.enabled_usb_itf); + let curves = config + .and_then(|c| c.raw_curves_mask) + .map(RescueCurves::from_bits_truncate) + .unwrap_or(RescueCurves::empty()); let current_driver_val = config.and_then(|c| c.led_driver).unwrap_or(0); let mut led_status_steady = false; @@ -383,7 +400,17 @@ impl ConfigViewModel { led_steady, touch_timeout_input, power_cycle, - enable_secp256k1, + curve_p256: curves.contains(RescueCurves::SECP256R1), + curve_p384: curves.contains(RescueCurves::SECP384R1), + curve_p521: curves.contains(RescueCurves::SECP521R1), + curve_secp256k1: curves.contains(RescueCurves::SECP256K1), + curve_bp256: curves.contains(RescueCurves::BP256R1), + curve_bp384: curves.contains(RescueCurves::BP384R1), + curve_bp512: curves.contains(RescueCurves::BP512R1), + curve_ed25519: curves.contains(RescueCurves::ED25519), + curve_ed448: curves.contains(RescueCurves::ED448), + curve_x25519: curves.contains(RescueCurves::CURVE25519), + curve_x448: curves.contains(RescueCurves::CURVE448), loading: false, is_custom_vendor, led_status_steady, @@ -453,17 +480,23 @@ impl ConfigViewModel { let dialog = dialog_handle; - // Tell the user to look at their key! + // Tell the user to press the button — RS-Key firmware requires + // user presence for config writes on both FIDO and Rescue paths. cx.update(|cx| { + let msg = if method_clone == DeviceMethod::Fido { + "Applying configuration... Touch your device if it flashes." + } else { + "Applying configuration... Press the device button to confirm." + }; match &dialog { StatusDialogHandle::Pin(dh) => { let _ = dh.update(cx, |d, cx| { - d.set_loading_msg("Applying configuration... Please touch your device if it flashes.", cx); + d.set_loading_msg(msg, cx); }); } StatusDialogHandle::Status(dh) => { let _ = dh.update(cx, |d, cx| { - d.set_loading("Applying configuration... Please touch your device if it flashes.", cx); + d.set_loading(msg, cx); }); } } @@ -507,7 +540,7 @@ impl ConfigViewModel { this.led_dimmable = config.led_dimmable; this.led_steady = config.led_steady; this.power_cycle = config.power_cycle_on_reset; - this.enable_secp256k1 = config.enable_secp256k1; + Self::sync_curve_toggles(this, Some(config)); this.device.update(cx, |repo, repo_cx| { repo.apply_fresh_state(fs.clone(), repo_cx); @@ -610,7 +643,6 @@ impl ConfigViewModel { let current_led_dimmable = status.config.led_dimmable; let current_led_steady = status.config.led_steady; let current_power_cycle = status.config.power_cycle_on_reset; - let current_enable_secp256k1 = status.config.enable_secp256k1; let current_enabled_usb_itf = status.config.enabled_usb_itf; let raw_curves_mask = status.config.raw_curves_mask; let led_order = status.config.led_order; @@ -676,7 +708,10 @@ impl ConfigViewModel { has_changes = true; } - if self.enable_secp256k1 != current_enable_secp256k1 { + 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); + if has_curve_changes { has_changes = true; } @@ -691,6 +726,11 @@ 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), @@ -702,8 +742,8 @@ impl ConfigViewModel { led_dimmable: Some(self.led_dimmable), power_cycle_on_reset: Some(self.power_cycle), led_steady: Some(self.led_steady), - enable_secp256k1: Some(self.enable_secp256k1), - raw_curves_mask, + enable_secp256k1: None, + raw_curves_mask: built_curves_mask, led_order, enabled_usb_itf: final_enabled_usb_itf, led_num: None, @@ -736,6 +776,42 @@ impl ConfigViewModel { } } + /// Build the curves bitmask from the current toggle states. + fn curves_mask_from_toggles(&self) -> u32 { + let mut mask = RescueCurves::empty(); + mask.set(RescueCurves::SECP256R1, self.curve_p256); + mask.set(RescueCurves::SECP384R1, self.curve_p384); + mask.set(RescueCurves::SECP521R1, self.curve_p521); + mask.set(RescueCurves::SECP256K1, self.curve_secp256k1); + mask.set(RescueCurves::BP256R1, self.curve_bp256); + mask.set(RescueCurves::BP384R1, self.curve_bp384); + mask.set(RescueCurves::BP512R1, self.curve_bp512); + mask.set(RescueCurves::ED25519, self.curve_ed25519); + mask.set(RescueCurves::ED448, self.curve_ed448); + mask.set(RescueCurves::CURVE25519, self.curve_x25519); + mask.set(RescueCurves::CURVE448, self.curve_x448); + mask.bits() + } + + /// Sync all curve toggle fields from a device config. + fn sync_curve_toggles(&mut self, config: Option<&AppConfig>) { + let curves = config + .and_then(|c| c.raw_curves_mask) + .map(RescueCurves::from_bits_truncate) + .unwrap_or(RescueCurves::empty()); + self.curve_p256 = curves.contains(RescueCurves::SECP256R1); + self.curve_p384 = curves.contains(RescueCurves::SECP384R1); + self.curve_p521 = curves.contains(RescueCurves::SECP521R1); + self.curve_secp256k1 = curves.contains(RescueCurves::SECP256K1); + self.curve_bp256 = curves.contains(RescueCurves::BP256R1); + self.curve_bp384 = curves.contains(RescueCurves::BP384R1); + self.curve_bp512 = curves.contains(RescueCurves::BP512R1); + self.curve_ed25519 = curves.contains(RescueCurves::ED25519); + self.curve_ed448 = curves.contains(RescueCurves::ED448); + self.curve_x25519 = curves.contains(RescueCurves::CURVE25519); + self.curve_x448 = curves.contains(RescueCurves::CURVE448); + } + pub(super) fn status_supports_legacy_fido_config(status: &FullDeviceStatus) -> bool { status.method == DeviceMethod::Fido && DeviceRepo::firmware_supports_legacy_fido_config( @@ -768,7 +844,7 @@ impl ConfigViewModel { 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.enable_secp256k1 = config.map(|c| c.enable_secp256k1).unwrap_or(true); + Self::sync_curve_toggles(self, config); let brightness = config.map(|c| c.led_brightness as f32).unwrap_or(8.0);