From 50b873c89ff57d03d98f70cc14c56331b43df79a Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 14 Apr 2021 00:33:39 +1200 Subject: [PATCH 1/6] Reliably ignore PIV devices that are not connected This is primarily to ignore smart card readers that don't have cards plugged in. --- Cargo.lock | 1 + Cargo.toml | 1 + src/main.rs | 41 +++++++++-------------------------------- src/yubikey.rs | 50 +++++++++++++++++++++++++++++++++++++++----------- 4 files changed, 50 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6e6651f..1b4c380 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -60,6 +60,7 @@ dependencies = [ "log", "man", "p256", + "pcsc", "rand 0.7.3", "secrecy", "sha2", diff --git a/Cargo.toml b/Cargo.toml index ab6dc84..869a7e0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,6 +33,7 @@ gumdrop = "0.8" hex = "0.4" log = "0.4" p256 = { version = "0.7", features = ["ecdh"] } +pcsc = "2.4" rand = "0.7" secrecy = "0.7" sha2 = "0.9" diff --git a/src/main.rs b/src/main.rs index 7080d18..3705070 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,7 +1,6 @@ use age_plugin::run_state_machine; use dialoguer::{Confirm, Select}; use gumdrop::Options; -use log::warn; use yubikey_piv::{ certificate::PublicKeyInfo, key::{RetiredSlotId, SlotId}, @@ -184,20 +183,8 @@ fn identity(opts: PluginOptions) -> Result<(), Error> { fn list(all: bool) -> Result<(), Error> { let mut readers = Readers::open()?; - for reader in readers.iter()? { - let mut yubikey = match reader.open() { - Ok(yk) => yk, - Err(e) => { - use std::error::Error; - let reason = if let Some(inner) = e.source() { - format!("{}: {}", e, inner) - } else { - e.to_string() - }; - warn!("Ignoring {}: {}", reader.name(), reason); - continue; - } - }; + for reader in readers.iter()?.filter(yubikey::filter_connected) { + let mut yubikey = reader.open()?; for key in Key::list(&mut yubikey)? { // We only use the retired slots. @@ -294,28 +281,18 @@ fn main() -> Result<(), Error> { eprintln!("make your choice, or press [Esc] or [q] to quit."); eprintln!(); - if Readers::open()?.iter()?.len() == 0 { + if Readers::open()? + .iter()? + .filter(yubikey::filter_connected) + .next() + .is_none() + { eprintln!("⏳ Please insert the YubiKey you want to set up."); }; let mut readers = yubikey::wait_for_readers()?; // Filter out readers we can't connect to. - let readers_list: Vec<_> = readers - .iter()? - .filter(|reader| match reader.open() { - Ok(_) => true, - Err(e) => { - use std::error::Error; - let reason = if let Some(inner) = e.source() { - format!("{}: {}", e, inner) - } else { - e.to_string() - }; - warn!("Ignoring {}: {}", reader.name(), reason); - false - } - }) - .collect(); + let readers_list: Vec<_> = readers.iter()?.filter(yubikey::filter_connected).collect(); let reader_names = readers_list .iter() diff --git a/src/yubikey.rs b/src/yubikey.rs index 1bb9816..f8c8f0c 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -7,15 +7,18 @@ use age_core::{ use age_plugin::{identity, Callbacks}; use bech32::{ToBase32, Variant}; use dialoguer::Password; +use log::warn; use secrecy::ExposeSecret; use std::convert::TryInto; use std::fmt; use std::io; +use std::iter; use std::thread::sleep; use std::time::{Duration, SystemTime}; use yubikey_piv::{ certificate::{Certificate, PublicKeyInfo}, key::{decrypt_data, AlgorithmId, RetiredSlotId, SlotId}, + readers::Reader, yubikey::Serial, MgmKey, Readers, YubiKey, }; @@ -30,12 +33,29 @@ use crate::{ const ONE_SECOND: Duration = Duration::from_secs(1); const FIFTEEN_SECONDS: Duration = Duration::from_secs(15); +pub(crate) fn filter_connected(reader: &Reader) -> bool { + match reader.open() { + Ok(_) => true, + Err(e) => { + use std::error::Error; + if let Some(pcsc::Error::RemovedCard) = + e.source().and_then(|inner| inner.downcast_ref()) + { + warn!("Ignoring {}: not connected", reader.name()); + false + } else { + true + } + } + } +} + pub(crate) fn wait_for_readers() -> Result { // Start a 15-second timer waiting for a YubiKey to be inserted (if necessary). let start = SystemTime::now(); loop { let mut readers = Readers::open()?; - if readers.iter()?.len() > 0 { + if readers.iter()?.filter(filter_connected).next().is_some() { break Ok(readers); } @@ -47,7 +67,12 @@ pub(crate) fn wait_for_readers() -> Result { } pub(crate) fn open(serial: Option) -> Result { - if Readers::open()?.iter()?.len() == 0 { + if Readers::open()? + .iter()? + .filter(filter_connected) + .next() + .is_none() + { if let Some(serial) = serial { eprintln!("⏳ Please insert the YubiKey with serial {}.", serial); } else { @@ -55,22 +80,25 @@ pub(crate) fn open(serial: Option) -> Result { } } let mut readers = wait_for_readers()?; - let mut readers_iter = readers.iter()?; + let mut readers_iter = readers.iter()?.filter(filter_connected); // --serial selects the YubiKey to use. If not provided, and more than one YubiKey is // connected, an error is returned. - let yubikey = match (readers_iter.len(), serial) { - (0, _) => unreachable!(), - (1, None) => readers_iter.next().unwrap().open()?, - (1, Some(serial)) => { - let yubikey = readers_iter.next().unwrap().open()?; + let yubikey = match (readers_iter.next(), readers_iter.next(), serial) { + (None, _, _) => unreachable!(), + (Some(reader), None, None) => reader.open()?, + (Some(reader), None, Some(serial)) => { + let yubikey = reader.open()?; if yubikey.serial() != serial { return Err(Error::NoMatchingSerial(serial)); } yubikey } - (_, Some(serial)) => { - let reader = readers_iter + (Some(a), Some(b), Some(serial)) => { + let reader = iter::empty() + .chain(Some(a)) + .chain(Some(b)) + .chain(readers_iter) .find(|reader| match reader.open() { Ok(yk) => yk.serial() == serial, _ => false, @@ -78,7 +106,7 @@ pub(crate) fn open(serial: Option) -> Result { .ok_or(Error::NoMatchingSerial(serial))?; reader.open()? } - (_, None) => return Err(Error::MultipleYubiKeys), + (Some(_), Some(_), None) => return Err(Error::MultipleYubiKeys), }; Ok(yubikey) From dd8589811be643c00b3a2c219cbb1c46fb00a083 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Wed, 14 Apr 2021 00:49:17 +1200 Subject: [PATCH 2/6] Improve UI messages for YubiKey management In particular, we now print out the candidate management key if setting it as the PIN-protected management key fails. --- src/yubikey.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/yubikey.rs b/src/yubikey.rs index f8c8f0c..3fc3a5d 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -125,7 +125,7 @@ pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { // If the user is using the default PIN, help them to change it. if pin == "123456" { eprintln!(); - eprintln!("✨ Your key is using the default PIN. Let's change it!"); + eprintln!("✨ Your YubiKey is using the default PIN. Let's change it!"); eprintln!("✨ We'll also set the PUK equal to the PIN."); eprintln!(); eprintln!("🔐 The PIN is up to 8 numbers, letters, or symbols. Not just numbers!"); @@ -157,7 +157,17 @@ pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { // Migrate to a PIN-protected management key. let mgm_key = MgmKey::generate()?; - mgm_key.set_protected(yubikey)?; + eprintln!(); + eprintln!("✨ Your YubiKey is using the default management key."); + eprintln!("✨ We'll migrate it to a PIN-protected management key."); + eprint!("... "); + mgm_key.set_protected(yubikey).map_err(|e| { + eprintln!("An error occurred while setting the new management key."); + eprintln!("⚠️ SAVE THIS MANAGEMENT KEY - YOU MAY NEED IT TO MANAGE YOUR YubiKey! ⚠️"); + eprintln!(" {}", hex::encode(mgm_key.as_ref())); + e + })?; + eprintln!("Success!"); } Ok(()) From 9fb8cd5f865db76d625672f890edaac26a76afac Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 15 Apr 2021 21:52:43 +1200 Subject: [PATCH 3/6] Add version flag --- src/main.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main.rs b/src/main.rs index 3705070..59a5c17 100644 --- a/src/main.rs +++ b/src/main.rs @@ -52,6 +52,9 @@ struct PluginOptions { #[options(help = "Print this help message and exit.")] help: bool, + #[options(help = "Print version info and exit.", short = "V")] + version: bool, + #[options( help = "Run the given age plugin state machine. Internal use only.", meta = "STATE-MACHINE", @@ -259,6 +262,9 @@ fn main() -> Result<(), Error> { plugin::IdentityPlugin::default, )?; Ok(()) + } else if opts.version { + println!("age-plugin-yubikey {}", env!("CARGO_PKG_VERSION")); + Ok(()) } else if opts.generate { generate(opts) } else if opts.identity { From b1249982deb5739c2654ba0083c05e4f8e9b94d3 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 15 Apr 2021 22:14:13 +1200 Subject: [PATCH 4/6] Add direct command flags to pretty CLI interface text --- src/main.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main.rs b/src/main.rs index 59a5c17..d03b366 100644 --- a/src/main.rs +++ b/src/main.rs @@ -278,10 +278,14 @@ fn main() -> Result<(), Error> { eprintln!(); eprintln!("This tool can create a new age identity in a free slot of your YubiKey."); eprintln!("It will generate an identity file that you can use with an age client,"); - eprintln!("along with the corresponding recipient."); + eprintln!("along with the corresponding recipient. You can also do this directly"); + eprintln!("with:"); + eprintln!(" age-plugin-yubikey --generate"); eprintln!(); eprintln!("If you are already using a YubiKey with age, you can select an existing"); - eprintln!("slot to recreate its corresponding identity file and recipient."); + eprintln!("slot to recreate its corresponding identity file and recipient. You can"); + eprintln!("also obtain this directly with:"); + eprintln!(" age-plugin-yubikey --identity"); eprintln!(); eprintln!("When asked below to select an option, use the up/down arrow keys to"); eprintln!("make your choice, or press [Esc] or [q] to quit."); From 9208719e8c70a4d82c3f23f607f561dcecfe338b Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 15 Apr 2021 22:16:05 +1200 Subject: [PATCH 5/6] Add issue templates --- .github/ISSUE_TEMPLATE/bug-report.md | 21 +++++++++++++++++++++ .github/ISSUE_TEMPLATE/ux-report.md | 21 +++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 .github/ISSUE_TEMPLATE/bug-report.md create mode 100644 .github/ISSUE_TEMPLATE/ux-report.md diff --git a/.github/ISSUE_TEMPLATE/bug-report.md b/.github/ISSUE_TEMPLATE/bug-report.md new file mode 100644 index 0000000..5ad9ae7 --- /dev/null +++ b/.github/ISSUE_TEMPLATE/bug-report.md @@ -0,0 +1,21 @@ +--- +name: Bug report +about: Create a report about a bug in this implementation. +title: '' +labels: '' +assignees: '' + +--- + +## Environment + +* OS: +* age-plugin-yubikey version: + +## What were you trying to do + +## What happened + +``` + +``` diff --git a/.github/ISSUE_TEMPLATE/ux-report.md b/.github/ISSUE_TEMPLATE/ux-report.md new file mode 100644 index 0000000..afb277d --- /dev/null +++ b/.github/ISSUE_TEMPLATE/ux-report.md @@ -0,0 +1,21 @@ +--- +name: UX report +about: Was age-plugin-yubikey hard to use? It's not you, it's us. We want to hear about it. +title: 'UX: ' +labels: 'UX report' +assignees: '' + +--- + + + +## What were you trying to do + +## What happened + +``` + +``` From f05c635d7b9d74d3240f6cab9cb52750b1ac7b15 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Thu, 15 Apr 2021 22:22:07 +1200 Subject: [PATCH 6/6] clippy fixes --- src/main.rs | 7 +------ src/yubikey.rs | 13 ++++++------- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/main.rs b/src/main.rs index d03b366..697387e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -291,12 +291,7 @@ fn main() -> Result<(), Error> { eprintln!("make your choice, or press [Esc] or [q] to quit."); eprintln!(); - if Readers::open()? - .iter()? - .filter(yubikey::filter_connected) - .next() - .is_none() - { + if !Readers::open()?.iter()?.any(yubikey::is_connected) { eprintln!("⏳ Please insert the YubiKey you want to set up."); }; let mut readers = yubikey::wait_for_readers()?; diff --git a/src/yubikey.rs b/src/yubikey.rs index 3fc3a5d..101b0b5 100644 --- a/src/yubikey.rs +++ b/src/yubikey.rs @@ -33,6 +33,10 @@ use crate::{ const ONE_SECOND: Duration = Duration::from_secs(1); const FIFTEEN_SECONDS: Duration = Duration::from_secs(15); +pub(crate) fn is_connected(reader: Reader) -> bool { + filter_connected(&reader) +} + pub(crate) fn filter_connected(reader: &Reader) -> bool { match reader.open() { Ok(_) => true, @@ -55,7 +59,7 @@ pub(crate) fn wait_for_readers() -> Result { let start = SystemTime::now(); loop { let mut readers = Readers::open()?; - if readers.iter()?.filter(filter_connected).next().is_some() { + if readers.iter()?.any(is_connected) { break Ok(readers); } @@ -67,12 +71,7 @@ pub(crate) fn wait_for_readers() -> Result { } pub(crate) fn open(serial: Option) -> Result { - if Readers::open()? - .iter()? - .filter(filter_connected) - .next() - .is_none() - { + if !Readers::open()?.iter()?.any(is_connected) { if let Some(serial) = serial { eprintln!("⏳ Please insert the YubiKey with serial {}.", serial); } else {