From d8eb198e97847d74d510ef61f549aca177934ba8 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 31 Dec 2022 18:47:39 +0000 Subject: [PATCH 1/3] Move certificate parsing into `Metadata::extract` --- src/builder.rs | 1 - src/key.rs | 3 +-- src/main.rs | 14 ++++---------- src/util.rs | 10 ++++++---- 4 files changed, 11 insertions(+), 17 deletions(-) diff --git a/src/builder.rs b/src/builder.rs index f01f131..72c91d7 100644 --- a/src/builder.rs +++ b/src/builder.rs @@ -134,7 +134,6 @@ impl IdentityBuilder { )], )?; - let (_, cert) = x509_parser::parse_x509_certificate(cert.as_ref()).unwrap(); let metadata = Metadata::extract(yubikey, slot, &cert, false).unwrap(); Ok(( diff --git a/src/key.rs b/src/key.rs index 8654b3e..a53d4e4 100644 --- a/src/key.rs +++ b/src/key.rs @@ -542,9 +542,8 @@ impl Connection { ) -> io::Result> { // Check if we can skip requesting a PIN. if self.cached_metadata.is_none() { - let (_, cert) = x509_parser::parse_x509_certificate(self.cert.as_ref()).unwrap(); self.cached_metadata = - match Metadata::extract(&mut self.yubikey, self.slot, &cert, true) { + match Metadata::extract(&mut self.yubikey, self.slot, &self.cert, true) { None => { return Ok(Err(identity::Error::Identity { index: self.identity_index, diff --git a/src/main.rs b/src/main.rs index 491fa9d..6817f12 100644 --- a/src/main.rs +++ b/src/main.rs @@ -211,10 +211,7 @@ fn print_single( .ok_or(Error::SlotHasNoIdentity(slot))?; let stub = key::Stub::new(yubikey.serial(), slot, &recipient); - let metadata = x509_parser::parse_x509_certificate(key.certificate().as_ref()) - .ok() - .and_then(|(_, cert)| util::Metadata::extract(&mut yubikey, slot, &cert, true)) - .unwrap(); + let metadata = util::Metadata::extract(&mut yubikey, slot, key.certificate(), true).unwrap(); printer(stub, recipient, metadata); @@ -252,9 +249,7 @@ fn print_multiple( }; let stub = key::Stub::new(yubikey.serial(), slot, &recipient); - let metadata = match x509_parser::parse_x509_certificate(key.certificate().as_ref()) - .ok() - .and_then(|(_, cert)| util::Metadata::extract(&mut yubikey, slot, &cert, all)) + let metadata = match util::Metadata::extract(&mut yubikey, slot, key.certificate(), all) { Some(res) => res, None => continue, @@ -479,10 +474,9 @@ fn main() -> Result<(), Error> { .interact()? { let stub = key::Stub::new(yubikey.serial(), slot, &recipient); - let (_, cert) = - x509_parser::parse_x509_certificate(key.certificate().as_ref()).unwrap(); let metadata = - util::Metadata::extract(&mut yubikey, slot, &cert, true).unwrap(); + util::Metadata::extract(&mut yubikey, slot, key.certificate(), true) + .unwrap(); ((stub, recipient, metadata), false) } else { diff --git a/src/util.rs b/src/util.rs index ce6460d..f907c5e 100644 --- a/src/util.rs +++ b/src/util.rs @@ -4,7 +4,7 @@ use std::iter; use x509_parser::{certificate::X509Certificate, der_parser::oid::Oid}; use yubikey::{ piv::{RetiredSlotId, SlotId}, - PinPolicy, Serial, TouchPolicy, YubiKey, + Certificate, PinPolicy, Serial, TouchPolicy, YubiKey, }; use crate::fl; @@ -112,9 +112,11 @@ impl Metadata { pub(crate) fn extract( yubikey: &mut YubiKey, slot: RetiredSlotId, - cert: &X509Certificate, + cert: &Certificate, all: bool, ) -> Option { + let (_, cert) = x509_parser::parse_x509_certificate(cert.as_ref()).ok()?; + // We store the PIN and touch policies for identities in their certificates // using the same certificate extension as PIV attestations. // https://developers.yubico.com/PIV/Introduction/PIV_attestation.html @@ -143,10 +145,10 @@ impl Metadata { .unwrap_or((None, None)) }; - extract_name(cert, all) + extract_name(&cert, all) .map(|(name, ours)| { if ours { - let (pin_policy, touch_policy) = policies(cert); + let (pin_policy, touch_policy) = policies(&cert); (name, pin_policy, touch_policy) } else { // We can extract the PIN and touch policies via an attestation. This From fc66d9f6fd29887c0530e1d51164a4065d1b998c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 31 Dec 2022 19:31:18 +0000 Subject: [PATCH 2/3] Add helper methods for filtering available keys --- src/key.rs | 28 +++++++++++++++++++++++++++- src/main.rs | 46 +++++++++------------------------------------- 2 files changed, 36 insertions(+), 38 deletions(-) diff --git a/src/key.rs b/src/key.rs index a53d4e4..47e0ea0 100644 --- a/src/key.rs +++ b/src/key.rs @@ -18,7 +18,7 @@ use yubikey::{ certificate::Certificate, piv::{decrypt_data, AlgorithmId, RetiredSlotId, SlotId}, reader::{Context, Reader}, - MgmKey, PinPolicy, Serial, TouchPolicy, YubiKey, + Key, MgmKey, PinPolicy, Serial, TouchPolicy, YubiKey, }; use crate::{ @@ -303,6 +303,32 @@ pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { Ok(()) } +/// Returns an iterator of keys that are occupying plugin-compatible slots, along with the +/// corresponding recipient if the key is compatible with this plugin. +pub(crate) fn list_slots( + yubikey: &mut YubiKey, +) -> Result)>, Error> { + Ok(Key::list(yubikey)?.into_iter().filter_map(|key| { + // We only use the retired slots. + match key.slot() { + SlotId::Retired(slot) => { + // Only P-256 keys are compatible with us. + let recipient = Recipient::from_certificate(key.certificate()); + Some((key, slot, recipient)) + } + _ => None, + } + })) +} + +/// Returns an iterator of keys that are compatible with this plugin. +pub(crate) fn list_compatible( + yubikey: &mut YubiKey, +) -> Result, Error> { + list_slots(yubikey) + .map(|iter| iter.filter_map(|(key, slot, res)| res.map(|recipient| (key, slot, recipient)))) +} + /// A reference to an age key stored in a YubiKey. #[derive(Debug)] pub struct Stub { diff --git a/src/main.rs b/src/main.rs index 6817f12..5687a2a 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,11 +12,7 @@ use i18n_embed::{ }; use lazy_static::lazy_static; use rust_embed::RustEmbed; -use yubikey::{ - piv::{RetiredSlotId, SlotId}, - reader::Context, - Key, PinPolicy, Serial, TouchPolicy, -}; +use yubikey::{piv::RetiredSlotId, reader::Context, PinPolicy, Serial, TouchPolicy}; mod builder; mod error; @@ -195,18 +191,7 @@ fn print_single( ) -> Result<(), Error> { let mut yubikey = key::open(serial)?; - let mut keys = Key::list(&mut yubikey)?.into_iter().filter_map(|key| { - // - We only use the retired slots. - // - Only P-256 keys are compatible with us. - match key.slot() { - SlotId::Retired(slot) => { - p256::Recipient::from_certificate(key.certificate()).map(|r| (key, slot, r)) - } - _ => None, - } - }); - - let (key, slot, recipient) = keys + let (key, slot, recipient) = key::list_compatible(&mut yubikey)? .find(|(_, s, _)| s == &slot) .ok_or(Error::SlotHasNoIdentity(slot))?; @@ -235,19 +220,7 @@ fn print_multiple( } } - for key in Key::list(&mut yubikey)? { - // We only use the retired slots. - let slot = match key.slot() { - SlotId::Retired(slot) => slot, - _ => continue, - }; - - // Only P-256 keys are compatible with us. - let recipient = match p256::Recipient::from_certificate(key.certificate()) { - Some(recipient) => recipient, - None => continue, - }; - + for (key, slot, recipient) in key::list_compatible(&mut yubikey)? { let stub = key::Stub::new(yubikey.serial(), slot, &recipient); let metadata = match util::Metadata::extract(&mut yubikey, slot, key.certificate(), all) { @@ -405,16 +378,16 @@ fn main() -> Result<(), Error> { None => return Ok(()), }; - let keys = Key::list(&mut yubikey)?; + let keys = key::list_slots(&mut yubikey)?.collect::>(); // Identify slots that we can't allow the user to select. let slot_details: Vec<_> = USABLE_SLOTS .iter() .map(|&slot| { keys.iter() - .find(|key| key.slot() == SlotId::Retired(slot)) - .map(|key| { - p256::Recipient::from_certificate(key.certificate()).map(|_| { + .find(|(_, s, _)| s == &slot) + .map(|(key, _, recipient)| { + recipient.as_ref().map(|_| { // Cache the details we need to display to the user. let (_, cert) = x509_parser::parse_x509_certificate(key.certificate().as_ref()) @@ -465,9 +438,8 @@ fn main() -> Result<(), Error> { } }; - if let Some(key) = keys.iter().find(|key| key.slot() == SlotId::Retired(slot)) { - let recipient = p256::Recipient::from_certificate(key.certificate()) - .expect("We checked this above"); + if let Some((key, _, recipient)) = keys.into_iter().find(|(_, s, _)| s == &slot) { + let recipient = recipient.expect("We checked this above"); if Confirm::new() .with_prompt(fl!("cli-setup-use-existing", slot_index = slot_index)) From 1dfadc7e272d52dcff97a960aa45e1acf9eb56ad Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 1 Jan 2023 13:29:30 +0000 Subject: [PATCH 3/3] Clean up `key::filter_connected` --- src/key.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/src/key.rs b/src/key.rs index 47e0ea0..532837b 100644 --- a/src/key.rs +++ b/src/key.rs @@ -39,21 +39,16 @@ pub(crate) fn is_connected(reader: Reader) -> bool { 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!( - "{}", - fl!("warn-yk-not-connected", yubikey_name = reader.name()) - ); - false - } else { - true - } + Err(yubikey::Error::PcscError { + inner: Some(pcsc::Error::RemovedCard), + }) => { + warn!( + "{}", + fl!("warn-yk-not-connected", yubikey_name = reader.name()) + ); + false } + _ => true, } }