From b1710e8d6907b819617aef120a15adae1359f7f6 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 29 Jan 2023 15:07:25 +0000 Subject: [PATCH 1/4] Enforce correct PIN lengths during YubiKey setup The behaviour of `age-plugin-yubikey` during setup now matches its behaviour during plugin usage. --- CHANGELOG.md | 4 ++ i18n/en-US/age_plugin_yubikey.ftl | 4 +- src/error.rs | 2 - src/key.rs | 107 ++++++++++++++++++------------ 4 files changed, 72 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7da3a75..b6ca3b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to Rust's notion of to 0.3.0 are beta releases. ## [Unreleased] +### Fixed +- When `age-plugin-yubikey` assists the user in changing their PIN from the + default PIN, it no longer tells the user that PINs shorter than 6 characters + are allowed, and instead loops until the user enters a PIN of valid length. ## [0.3.2] - 2023-01-01 ### Changed diff --git a/i18n/en-US/age_plugin_yubikey.ftl b/i18n/en-US/age_plugin_yubikey.ftl index 2ec1d7a..b5315ac 100644 --- a/i18n/en-US/age_plugin_yubikey.ftl +++ b/i18n/en-US/age_plugin_yubikey.ftl @@ -127,7 +127,8 @@ mgr-change-default-pin = ✨ Your {-yubikey} is using the default PIN. Let's change it! ✨ We'll also set the PUK equal to the PIN. - 🔐 The PIN is up to 8 numbers, letters, or symbols. Not just numbers! + 🔐 The PIN can be numbers, letters, or symbols. Not just numbers! + 📏 The PIN must be at least 6 and at most 8 characters in length. ❌ Your keys will be lost if the PIN and PUK are locked after 3 incorrect tries. mgr-enter-current-puk = Enter current PUK (default is {$default_puk}) @@ -180,7 +181,6 @@ rec-custom-mgmt-key = err-invalid-flag-command = Flag '{$flag}' cannot be used with '{$command}'. err-invalid-flag-tui = Flag '{$flag}' cannot be used with the interactive interface. -err-invalid-pin-length = The PIN needs to be 1-8 characters. err-invalid-pin-policy = Invalid PIN policy '{$policy}' (expected [{$expected}]). err-invalid-slot = Invalid slot '{$slot}' (expected number between 1 and 20). err-invalid-touch-policy = Invalid touch policy '{$policy}' (expected [{$expected}]). diff --git a/src/error.rs b/src/error.rs index 5b4372c..9b2ff2c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -17,7 +17,6 @@ pub enum Error { CustomManagementKey, InvalidFlagCommand(String, String), InvalidFlagTui(String), - InvalidPinLength, InvalidPinPolicy(String), InvalidSlot(u8), InvalidTouchPolicy(String), @@ -63,7 +62,6 @@ impl fmt::Debug for Error { command = command.as_str(), )?, Error::InvalidFlagTui(flag) => wlnfl!(f, "err-invalid-flag-tui", flag = flag.as_str())?, - Error::InvalidPinLength => wlnfl!(f, "err-invalid-pin-length")?, Error::InvalidPinPolicy(s) => wlnfl!( f, "err-invalid-pin-policy", diff --git a/src/key.rs b/src/key.rs index 532837b..813de5a 100644 --- a/src/key.rs +++ b/src/key.rs @@ -3,12 +3,13 @@ use age_core::{ format::{FileKey, FILE_KEY_BYTES}, primitives::{aead_decrypt, hkdf}, - secrecy::ExposeSecret, + secrecy::{ExposeSecret, SecretString}, }; use age_plugin::{identity, Callbacks}; use bech32::{ToBase32, Variant}; use dialoguer::Password; use log::{debug, error, warn}; +use std::convert::Infallible; use std::fmt; use std::io; use std::iter; @@ -236,6 +237,31 @@ pub(crate) fn open(serial: Option) -> Result { Ok(yubikey) } +fn request_pin( + mut prompt: impl FnMut(Option) -> io::Result>, + serial: Serial, +) -> io::Result> { + let mut prev_error = None; + loop { + prev_error = Some(match prompt(prev_error)? { + Ok(pin) => match pin.expose_secret().len() { + // A PIN must be between 6 and 8 characters. + 6..=8 => break Ok(Ok(pin)), + // If the string is 44 bytes and starts with the YubiKey's serial + // encoded as 12-byte modhex, the user probably touched the YubiKey + // early and "typed" an OTP. + 44 if pin.expose_secret().starts_with(&otp_serial_prefix(serial)) => { + fl!("plugin-err-accidental-touch") + } + // Otherwise, the PIN is either too short or too long. + 0..=5 => fl!("plugin-err-pin-too-short"), + _ => fl!("plugin-err-pin-too-long"), + }, + Err(e) => break Ok(Err(e)), + }); + } +} + pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { const DEFAULT_PIN: &str = "123456"; const DEFAULT_PUK: &str = "12345678"; @@ -258,13 +284,21 @@ pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { let current_puk = Password::new() .with_prompt(fl!("mgr-enter-current-puk", default_puk = DEFAULT_PUK)) .interact()?; - let new_pin = Password::new() - .with_prompt(fl!("mgr-choose-new-pin")) - .with_confirmation(fl!("mgr-repeat-new-pin"), fl!("mgr-pin-mismatch")) - .interact()?; - if new_pin.len() > 8 { - return Err(Error::InvalidPinLength); - } + let new_pin = request_pin( + |prev_error| { + if let Some(err) = prev_error { + eprintln!("{}", err); + } + Password::new() + .with_prompt(fl!("mgr-choose-new-pin")) + .with_confirmation(fl!("mgr-repeat-new-pin"), fl!("mgr-pin-mismatch")) + .interact() + .map(|pin| Result::<_, Infallible>::Ok(SecretString::new(pin))) + }, + yubikey.serial(), + )? + .unwrap(); + let new_pin = new_pin.expose_secret(); yubikey.change_puk(current_puk.as_bytes(), new_pin.as_bytes())?; yubikey.change_pin(pin.as_bytes(), new_pin.as_bytes())?; } @@ -581,39 +615,30 @@ impl Connection { // The policy requires a PIN, so request it. // Note that we can't distinguish between PinPolicy::Once and PinPolicy::Always // because this plugin is ephemeral, so we always request the PIN. - let enter_pin_msg = fl!( - "plugin-enter-pin", - yubikey_serial = self.yubikey.serial().to_string(), - ); - let mut message = enter_pin_msg.clone(); - let pin = loop { - message = match callbacks.request_secret(&message)? { - Ok(pin) => match pin.expose_secret().len() { - // A PIN must be between 6 and 8 characters. - 6..=8 => break pin, - // If the string is 44 bytes and starts with the YubiKey's serial - // encoded as 12-byte modhex, the user probably touched the YubiKey - // early and "typed" an OTP. - 44 if pin - .expose_secret() - .starts_with(&otp_serial_prefix(self.yubikey.serial())) => - { - format!("{} {}", fl!("plugin-err-accidental-touch"), enter_pin_msg) - } - // Otherwise, the PIN is either too short or too long. - 0..=5 => format!("{} {}", fl!("plugin-err-pin-too-short"), enter_pin_msg), - _ => format!("{} {}", fl!("plugin-err-pin-too-long"), enter_pin_msg), - }, - Err(_) => { - return Ok(Err(identity::Error::Identity { - index: self.identity_index, - message: fl!( - "plugin-err-pin-required", - yubikey_serial = self.yubikey.serial().to_string(), - ), - })) - } - }; + let pin = match request_pin( + |prev_error| { + callbacks.request_secret(&format!( + "{}{}{}", + prev_error.as_deref().unwrap_or(""), + prev_error.as_deref().map(|_| " ").unwrap_or(""), + fl!( + "plugin-enter-pin", + yubikey_serial = self.yubikey.serial().to_string(), + ) + )) + }, + self.yubikey.serial(), + )? { + Ok(pin) => pin, + Err(_) => { + return Ok(Err(identity::Error::Identity { + index: self.identity_index, + message: fl!( + "plugin-err-pin-required", + yubikey_serial = self.yubikey.serial().to_string(), + ), + })) + } }; if let Err(e) = self.yubikey.verify_pin(pin.expose_secret().as_bytes()) { return Ok(Err(identity::Error::Identity { From ff3e8e37c9c3885657817feb82772dd3e1497d50 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sun, 29 Jan 2023 14:27:20 +0000 Subject: [PATCH 2/4] Treat `pcsc::Error::NoSmartcard` as a "YubiKey disconnected" error Some SmartCard readers report this error when no SmartCard is inserted, so we need to check for it when filtering for connected YubiKeys (along with `pcsc::Error::RemovedCard` which some _other_ SmartCard readers report instead). Closes str4d/age-plugin-yubikey#81. --- CHANGELOG.md | 2 ++ src/key.rs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6ca3b2..31680cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ to 0.3.0 are beta releases. - When `age-plugin-yubikey` assists the user in changing their PIN from the default PIN, it no longer tells the user that PINs shorter than 6 characters are allowed, and instead loops until the user enters a PIN of valid length. +- More kinds of SmartCard readers are ignored when they have no SmartCard + inserted. ## [0.3.2] - 2023-01-01 ### Changed diff --git a/src/key.rs b/src/key.rs index 813de5a..0eb47a5 100644 --- a/src/key.rs +++ b/src/key.rs @@ -41,7 +41,7 @@ pub(crate) fn is_connected(reader: Reader) -> bool { pub(crate) fn filter_connected(reader: &Reader) -> bool { match reader.open() { Err(yubikey::Error::PcscError { - inner: Some(pcsc::Error::RemovedCard), + inner: Some(pcsc::Error::NoSmartcard | pcsc::Error::RemovedCard), }) => { warn!( "{}", From d2132b4ac2c560b6b00fe36baf321361e38323c9 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 11 Feb 2023 02:45:30 +0000 Subject: [PATCH 3/4] Prevent changing the default PIN to itself Closes str4d/age-plugin-yubikey#120. --- CHANGELOG.md | 2 ++ i18n/en-US/age_plugin_yubikey.ftl | 1 + src/key.rs | 35 ++++++++++++++++++------------- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31680cb..63f3c65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ to 0.3.0 are beta releases. - When `age-plugin-yubikey` assists the user in changing their PIN from the default PIN, it no longer tells the user that PINs shorter than 6 characters are allowed, and instead loops until the user enters a PIN of valid length. + It also now prevents the user from setting their PIN to the default PIN, to + avoid creating a cycle. - More kinds of SmartCard readers are ignored when they have no SmartCard inserted. diff --git a/i18n/en-US/age_plugin_yubikey.ftl b/i18n/en-US/age_plugin_yubikey.ftl index b5315ac..a84e19c 100644 --- a/i18n/en-US/age_plugin_yubikey.ftl +++ b/i18n/en-US/age_plugin_yubikey.ftl @@ -135,6 +135,7 @@ mgr-enter-current-puk = Enter current PUK (default is {$default_puk}) mgr-choose-new-pin = Choose a new PIN/PUK mgr-repeat-new-pin = Repeat the PIN/PUK mgr-pin-mismatch = PINs don't match +mgr-nope-default-pin = You entered the default PIN again. You need to change it. mgr-changing-mgmt-key = ✨ Your {-yubikey} is using the default management key. diff --git a/src/key.rs b/src/key.rs index 0eb47a5..9aa81ed 100644 --- a/src/key.rs +++ b/src/key.rs @@ -284,20 +284,27 @@ pub(crate) fn manage(yubikey: &mut YubiKey) -> Result<(), Error> { let current_puk = Password::new() .with_prompt(fl!("mgr-enter-current-puk", default_puk = DEFAULT_PUK)) .interact()?; - let new_pin = request_pin( - |prev_error| { - if let Some(err) = prev_error { - eprintln!("{}", err); - } - Password::new() - .with_prompt(fl!("mgr-choose-new-pin")) - .with_confirmation(fl!("mgr-repeat-new-pin"), fl!("mgr-pin-mismatch")) - .interact() - .map(|pin| Result::<_, Infallible>::Ok(SecretString::new(pin))) - }, - yubikey.serial(), - )? - .unwrap(); + let new_pin = loop { + let pin = request_pin( + |prev_error| { + if let Some(err) = prev_error { + eprintln!("{}", err); + } + Password::new() + .with_prompt(fl!("mgr-choose-new-pin")) + .with_confirmation(fl!("mgr-repeat-new-pin"), fl!("mgr-pin-mismatch")) + .interact() + .map(|pin| Result::<_, Infallible>::Ok(SecretString::new(pin))) + }, + yubikey.serial(), + )? + .unwrap(); + if pin.expose_secret() == DEFAULT_PIN { + eprintln!("{}", fl!("mgr-nope-default-pin")); + } else { + break pin; + } + }; let new_pin = new_pin.expose_secret(); yubikey.change_puk(current_puk.as_bytes(), new_pin.as_bytes())?; yubikey.change_pin(pin.as_bytes(), new_pin.as_bytes())?; From cd03e7bda3dc66e204514d061924c78fa7acc6ad Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Sat, 11 Feb 2023 04:28:16 +0000 Subject: [PATCH 4/4] Release 0.3.3 --- CHANGELOG.md | 2 ++ Cargo.lock | 2 +- Cargo.toml | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63f3c65..3301c89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,8 @@ and this project adheres to Rust's notion of to 0.3.0 are beta releases. ## [Unreleased] + +## [0.3.3] - 2023-02-11 ### Fixed - When `age-plugin-yubikey` assists the user in changing their PIN from the default PIN, it no longer tells the user that PINs shorter than 6 characters diff --git a/Cargo.lock b/Cargo.lock index eccee10..0b15c6e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -49,7 +49,7 @@ dependencies = [ [[package]] name = "age-plugin-yubikey" -version = "0.3.2" +version = "0.3.3" dependencies = [ "age-core", "age-plugin", diff --git a/Cargo.toml b/Cargo.toml index b1eed07..db38619 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "age-plugin-yubikey" description = "YubiKey plugin for age clients" -version = "0.3.2" +version = "0.3.3" authors = ["Jack Grigg "] repository = "https://github.com/str4d/age-plugin-yubikey" readme = "README.md"