Merge pull request #118 from str4d/correctly-handle-short-pins

Enforce correct PIN lengths during YubiKey setup

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

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}]).

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",

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<Serial>) -> Result<YubiKey, Error> {
     Ok(yubikey)
 }
 
+fn request_pin<E>(
+    mut prompt: impl FnMut(Option<String>) -> io::Result<Result<SecretString, E>>,
+    serial: Serial,
+) -> io::Result<Result<SecretString, E>> {
+    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()
+        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()?;
+                    .interact()
-        if new_pin.len() > 8 {
+                    .map(|pin| Result::<_, Infallible>::Ok(SecretString::new(pin)))
-            return Err(Error::InvalidPinLength);
+            },
-        }
+            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,29 +615,21 @@ 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!(
+        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(),
-        );
+                    )
-        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),
             },
+            self.yubikey.serial(),
+        )? {
+            Ok(pin) => pin,
             Err(_) => {
                 return Ok(Err(identity::Error::Identity {
                     index: self.identity_index,
@@ -614,7 +640,6 @@ impl Connection {
                 }))
             }
         };
-        };
         if let Err(e) = self.yubikey.verify_pin(pin.expose_secret().as_bytes()) {
             return Ok(Err(identity::Error::Identity {
                 index: self.identity_index,