From 6e47448560f5e28c839c3b4d52690c0a2aa4959e Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 31 Dec 2022 13:07:15 +0000 Subject: [PATCH 1/3] Generalise code for hunting agents that may be holding YubiKeys --- src/key.rs | 49 ++++++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 23 deletions(-) diff --git a/src/key.rs b/src/key.rs index aa7bab6..614a276 100644 --- a/src/key.rs +++ b/src/key.rs @@ -77,37 +77,40 @@ pub(crate) fn wait_for_readers() -> Result { } } -/// Stops `scdaemon` if it is running. +/// Looks for agent processes that might be holding exclusive access to a YubiKey, and +/// asks them as nicely as possible to release it. /// -/// Returns `true` if `scdaemon` was running and was successfully interrupted (or killed -/// if the platform doesn't support interrupts). -fn stop_scdaemon() -> bool { - debug!("Sharing violation encountered, looking for scdaemon processes to stop"); +/// Returns `true` if any known agent was running and was successfully interrupted (or +/// killed if the platform doesn't support interrupts). +fn hunt_agents() -> bool { + debug!("Sharing violation encountered, looking for agent processes"); - use sysinfo::{ - Process, ProcessExt, ProcessRefreshKind, RefreshKind, Signal, System, SystemExt, - }; + use sysinfo::{ProcessExt, ProcessRefreshKind, RefreshKind, Signal, System, SystemExt}; let mut interrupted = false; let sys = System::new_with_specifics(RefreshKind::new().with_processes(ProcessRefreshKind::new())); - for process in sys - .processes() - .values() - .filter(|val: &&Process| ["scdaemon", "scdaemon.exe"].contains(&val.name())) - { - if process - .kill_with(Signal::Interrupt) - .unwrap_or_else(|| process.kill()) - { - debug!("Stopped scdaemon (PID {})", process.pid()); - interrupted = true; + for process in sys.processes().values() { + match process.name() { + "scdaemon" | "scdaemon.exe" => { + // gpg-agent runs scdaemon to interact with smart cards. The canonical way + // to reload it is `gpgconf --reload scdaemon`, which kills and restarts + // the process. We emulate that here with SIGINT (which it listens to). + if process + .kill_with(Signal::Interrupt) + .unwrap_or_else(|| process.kill()) + { + debug!("Stopped scdaemon (PID {})", process.pid()); + interrupted = true; + } + } + _ => (), } } - // If we did interrupt `scdaemon`, pause briefly to allow it to exit. + // If we did interrupt an agent, pause briefly to allow it to finish up. if interrupted { sleep(Duration::from_millis(100)); } @@ -121,7 +124,7 @@ fn open_sesame( op().or_else(|e| match e { yubikey::Error::PcscError { inner: Some(pcsc::Error::SharingViolation), - } if stop_scdaemon() => op(), + } if hunt_agents() => op(), _ => Err(e), }) } @@ -129,7 +132,7 @@ fn open_sesame( /// Opens a connection to this reader, returning a `YubiKey` if successful. /// /// This is equivalent to [`Reader::open`], but additionally handles the presence of -/// `scdaemon` (which can indefinitely hold exclusive access to a YubiKey). +/// agents (which can indefinitely hold exclusive access to a YubiKey). pub(crate) fn open_connection(reader: &Reader) -> Result { open_sesame(|| reader.open()) } @@ -137,7 +140,7 @@ pub(crate) fn open_connection(reader: &Reader) -> Result Result { open_sesame(|| YubiKey::open_by_serial(serial)) } From 1913838f8ed4b30c756c3c20ea5fdf1680ea97ca Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 31 Dec 2022 13:22:56 +0000 Subject: [PATCH 2/3] Hunt for yubikey-agent --- CHANGELOG.md | 3 +++ src/key.rs | 14 ++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index dc35f00..ec1d535 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,9 @@ and this project adheres to Rust's notion of to 0.3.0 are beta releases. ## [Unreleased] +### Changed +- The "sharing violation" logic now also sends SIGHUP to any `yubikey-agent` + that is running, to have them release any YubiKey locks they are holding. ## [0.3.1] - 2022-12-30 ### Changed diff --git a/src/key.rs b/src/key.rs index 614a276..b5b371f 100644 --- a/src/key.rs +++ b/src/key.rs @@ -106,6 +106,20 @@ fn hunt_agents() -> bool { interrupted = true; } } + "yubikey-agent" | "yubikey-agent.exe" => { + // yubikey-agent releases all YubiKey locks when it receives a SIGHUP. + match process.kill_with(Signal::Hangup) { + Some(true) => { + debug!("Sent SIGHUP to yubikey-agent (PID {})", process.pid()); + interrupted = true; + } + Some(false) => (), + None => debug!( + "Found yubikey-agent (PID {}) but platform doesn't support SIGHUP", + process.pid(), + ), + } + } _ => (), } } From 3597d963324137c33aba7454704312e72a8a3918 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 1 Jan 2023 13:10:58 +0000 Subject: [PATCH 3/3] Correctly hunt agents in plugin mode --- CHANGELOG.md | 4 ++++ src/key.rs | 46 ++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 48 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec1d535..05461ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,10 @@ to 0.3.0 are beta releases. - The "sharing violation" logic now also sends SIGHUP to any `yubikey-agent` that is running, to have them release any YubiKey locks they are holding. +### Fixed +- The "sharing violation" logic now runs during plugin mode as intended. In the + previous release it only ran during direct `age-plugin-yubikey` usage. + ## [0.3.1] - 2022-12-30 ### Changed - If a "sharing violation" error is encountered while opening a connection to a diff --git a/src/key.rs b/src/key.rs index b5b371f..e036a36 100644 --- a/src/key.rs +++ b/src/key.rs @@ -8,7 +8,7 @@ use age_core::{ use age_plugin::{identity, Callbacks}; use bech32::{ToBase32, Variant}; use dialoguer::Password; -use log::{debug, warn}; +use log::{debug, error, warn}; use std::fmt; use std::io; use std::iter; @@ -156,7 +156,49 @@ pub(crate) fn open_connection(reader: &Reader) -> Result Result { - open_sesame(|| YubiKey::open_by_serial(serial)) + // `YubiKey::open_by_serial` has a bug where it ignores all opening errors, even if + // it potentially could have found a matching YubiKey if not for an error, and thus + // returns `Error::NotFound` if another agent is holding exclusive access to the + // required YubiKey. This gives misleading UX behaviour where age-plugin-yubikey asks + // the user to insert a YubiKey they have already inserted. + // + // For now, we instead implement the correct behaviour manually. Once MSRV has been + // raised to 1.60, we can upstream this into the `yubikey` crate. + open_sesame(|| { + let mut readers = Context::open()?; + + let mut open_error = None; + + for reader in readers.iter()? { + let yubikey = match reader.open() { + Ok(yk) => yk, + Err(e) => { + // Save the first error we see that indicates we might have been able + // to find a matching YubiKey. + if open_error.is_none() { + if let yubikey::Error::PcscError { + inner: Some(pcsc::Error::SharingViolation), + } = e + { + open_error = Some(e); + } + } + continue; + } + }; + + if serial == yubikey.serial() { + return Ok(yubikey); + } + } + + Err(if let Some(e) = open_error { + e + } else { + error!("no YubiKey detected with serial: {}", serial); + yubikey::Error::NotFound + }) + }) } pub(crate) fn open(serial: Option) -> Result {