Merge pull request #35 from str4d/ux-improvements

UX improvements

src/format.rs

@@ -106,7 +106,7 @@ impl RecipientLine {
         let epk = esk.public_key();
         let epk_bytes = EphemeralKeyBytes::from_public_key(&epk);
 
-        let shared_secret = esk.diffie_hellman(&pk.public_key());
+        let shared_secret = esk.diffie_hellman(pk.public_key());
 
         let mut salt = vec![];
         salt.extend_from_slice(epk_bytes.as_bytes());

src/p256.rs

@@ -48,7 +48,7 @@ impl Recipient {
     /// This accepts both compressed (as used by the plugin) and uncompressed (as used in
     /// the YubiKey certificate) encodings.
     pub(crate) fn from_encoded(encoded: &p256::EncodedPoint) -> Option<Self> {
-        p256::PublicKey::from_encoded_point(&encoded).map(Recipient)
+        p256::PublicKey::from_encoded_point(encoded).map(Recipient)
     }
 
     /// Returns the compressed SEC-1 encoding of this recipient.

src/plugin.rs

@@ -23,7 +23,7 @@ impl RecipientPluginV1 for RecipientPlugin {
         bytes: &[u8],
     ) -> Result<(), recipient::Error> {
         if let Some(pk) = if plugin_name == PLUGIN_NAME {
-            Recipient::from_bytes(&bytes)
+            Recipient::from_bytes(bytes)
         } else {
             None
         } {
@@ -44,7 +44,7 @@ impl RecipientPluginV1 for RecipientPlugin {
         bytes: &[u8],
     ) -> Result<(), recipient::Error> {
         if let Some(stub) = if plugin_name == PLUGIN_NAME {
-            yubikey::Stub::from_bytes(&bytes, index)
+            yubikey::Stub::from_bytes(bytes, index)
         } else {
             None
         } {
@@ -88,7 +88,7 @@ impl RecipientPluginV1 for RecipientPlugin {
                     self.recipients
                         .iter()
                         .chain(yk_recipients.iter())
-                        .map(|pk| format::RecipientLine::wrap_file_key(&file_key, &pk).into())
+                        .map(|pk| format::RecipientLine::wrap_file_key(&file_key, pk).into())
                         .collect()
                 })
                 .collect())
@@ -111,7 +111,7 @@ impl IdentityPluginV1 for IdentityPlugin {
         bytes: &[u8],
     ) -> Result<(), identity::Error> {
         if let Some(stub) = if plugin_name == PLUGIN_NAME {
-            yubikey::Stub::from_bytes(&bytes, index)
+            yubikey::Stub::from_bytes(bytes, index)
         } else {
             None
         } {
@@ -145,7 +145,7 @@ impl IdentityPluginV1 for IdentityPlugin {
         for (file, stanzas) in files.iter().enumerate() {
             for (stanza_index, stanza) in stanzas.iter().enumerate() {
                 match (
-                    format::RecipientLine::from_stanza(&stanza).map(|res| {
+                    format::RecipientLine::from_stanza(stanza).map(|res| {
                         res.map_err(|_| identity::Error::Stanza {
                             file_index: file,
                             stanza_index,
@@ -213,6 +213,11 @@ impl IdentityPluginV1 for IdentityPlugin {
                 }
             };
 
+            if let Err(e) = conn.request_pin_if_necessary(&mut callbacks)? {
+                callbacks.error(e)?.unwrap();
+                continue;
+            }
+
             for (&file_index, stanzas) in files {
                 if file_keys.contains_key(&file_index) {
                     // We decrypted this file with an earlier YubiKey.
@@ -220,7 +225,7 @@ impl IdentityPluginV1 for IdentityPlugin {
                 }
 
                 for (stanza_index, line) in stanzas.iter().enumerate() {
-                    match conn.unwrap_file_key(&line) {
+                    match conn.unwrap_file_key(line) {
                         Ok(file_key) => {
                             // We've managed to decrypt this file!
                             file_keys.entry(file_index).or_insert(Ok(file_key));

src/util.rs

@@ -96,7 +96,7 @@ pub(crate) struct Metadata {
     slot: RetiredSlotId,
     name: String,
     created: String,
-    pin_policy: Option<PinPolicy>,
+    pub(crate) pin_policy: Option<PinPolicy>,
     touch_policy: Option<TouchPolicy>,
 }
 
@@ -138,7 +138,7 @@ impl Metadata {
         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

src/yubikey.rs

@@ -18,6 +18,7 @@ use std::time::{Duration, SystemTime};
 use yubikey_piv::{
     certificate::{Certificate, PublicKeyInfo},
     key::{decrypt_data, AlgorithmId, RetiredSlotId, SlotId},
+    policy::PinPolicy,
     readers::Reader,
     yubikey::Serial,
     MgmKey, Readers, YubiKey,
@@ -27,6 +28,7 @@ use crate::{
     error::Error,
     format::{RecipientLine, STANZA_KEY_LABEL},
     p256::{Recipient, TAG_BYTES},
+    util::Metadata,
     IDENTITY_PREFIX,
 };
 
@@ -299,12 +301,12 @@ impl Stub {
         };
 
         // Read the pubkey from the YubiKey slot and check it still matches.
-        let pk = match Certificate::read(&mut yubikey, SlotId::Retired(self.slot))
+        let (cert, pk) = match Certificate::read(&mut yubikey, SlotId::Retired(self.slot))
             .ok()
             .and_then(|cert| match cert.subject_pki() {
-                PublicKeyInfo::EcP256(pubkey) => {
+                PublicKeyInfo::EcP256(pubkey) => Recipient::from_encoded(pubkey)
-                    Recipient::from_encoded(pubkey).filter(|pk| pk.tag() == self.tag)
+                    .filter(|pk| pk.tag() == self.tag)
-                }
+                    .map(|pk| (cert, pk)),
                 _ => None,
             }) {
             Some(pk) => pk,
@@ -316,39 +318,24 @@ impl Stub {
             }
         };
 
-        let pin = match callbacks.request_secret(&format!(
-            "Enter PIN for YubiKey with serial {}",
-            self.serial
-        ))? {
-            Ok(pin) => pin,
-            Err(_) => {
-                return Ok(Err(identity::Error::Identity {
-                    index: self.identity_index,
-                    message: format!("A PIN is required for YubiKey with serial {}", self.serial),
-                }))
-            }
-        };
-        if yubikey.verify_pin(pin.expose_secret().as_bytes()).is_err() {
-            return Ok(Err(identity::Error::Identity {
-                index: self.identity_index,
-                message: "Invalid YubiKey PIN".to_owned(),
-            }));
-        }
-
         Ok(Ok(Connection {
             yubikey,
+            cert,
             pk,
             slot: self.slot,
             tag: self.tag,
+            identity_index: self.identity_index,
         }))
     }
 }
 
 pub(crate) struct Connection {
     yubikey: YubiKey,
+    cert: Certificate,
     pk: Recipient,
     slot: RetiredSlotId,
     tag: [u8; 4],
+    identity_index: usize,
 }
 
 impl Connection {
@@ -356,6 +343,54 @@ impl Connection {
         &self.pk
     }
 
+    pub(crate) fn request_pin_if_necessary<E>(
+        &mut self,
+        callbacks: &mut dyn Callbacks<E>,
+    ) -> io::Result<Result<(), identity::Error>> {
+        // Check if we can skip requesting a PIN.
+        let (_, cert) = x509_parser::parse_x509_certificate(self.cert.as_ref()).unwrap();
+        match Metadata::extract(&mut self.yubikey, self.slot, &cert, true) {
+            Some(metadata) => {
+                if let Some(PinPolicy::Never) = metadata.pin_policy {
+                    return Ok(Ok(()));
+                }
+            }
+            None => {
+                return Ok(Err(identity::Error::Identity {
+                    index: self.identity_index,
+                    message: "Certificate for YubiKey identity contains an invalid PIN policy"
+                        .to_string(),
+                }))
+            }
+        }
+
+        // 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 pin = match callbacks.request_secret(&format!(
+            "Enter PIN for YubiKey with serial {}",
+            self.yubikey.serial()
+        ))? {
+            Ok(pin) => pin,
+            Err(_) => {
+                return Ok(Err(identity::Error::Identity {
+                    index: self.identity_index,
+                    message: format!(
+                        "A PIN is required for YubiKey with serial {}",
+                        self.yubikey.serial()
+                    ),
+                }))
+            }
+        };
+        if let Err(e) = self.yubikey.verify_pin(pin.expose_secret().as_bytes()) {
+            return Ok(Err(identity::Error::Identity {
+                index: self.identity_index,
+                message: format!("{:?}", Error::YubiKey(e)),
+            }));
+        }
+        Ok(Ok(()))
+    }
+
     pub(crate) fn unwrap_file_key(&mut self, line: &RecipientLine) -> Result<FileKey, ()> {
         assert_eq!(self.tag, line.tag);