Isolate cached PINs for multiple devices from each other

The application supports multiple devices both plugged in at the same
time as well as when used after the other. However, the GPG cache ID we
use for storing and retrieving the respective PIN is effectively a
constant. This constraint can cause problems when devices have different
PINs, as the PIN of a previously plugged in device may be reused for an
operation on a different one.
To resolve this problem this change adds the respective device's model
and serial number to the cache ID. As each serial number is supposed to
be different, this will ensure that the correct PIN is used for each
device. With this change we also show the model and serial number of the
currently used device in the pinentry dialog.
Note that because we do not store the serial numbers of all previously
plugged in devices, the pin clear command will only clear the PIN for
the currently plugged in device. If a user wants to make sure that a
cached PIN is cleared, the pin clear command should be invoked before
unplugging the device.

6 files changed, 118 insertions, 66 deletions

diff --git a/nitrocli/CHANGELOG.md b/nitrocli/CHANGELOG.md
index 6236a77..fcb126e 100644
--- a/nitrocli/CHANGELOG.md
+++ b/nitrocli/CHANGELOG.md
@@ -1,5 +1,7 @@
 Unreleased
 ----------
+- Store cached PINs on a per-device basis to better support multi-device
+  scenarios
 - Further decreased binary size by using system allocator
 
 
diff --git a/nitrocli/doc/nitrocli.1 b/nitrocli/doc/nitrocli.1
index 9537439..dbbaf2b 100644
--- a/nitrocli/doc/nitrocli.1
+++ b/nitrocli/doc/nitrocli.1
@@ -1,4 +1,4 @@
-.TH NITROCLI 1 2019-01-10
+.TH NITROCLI 1 2019-01-11
 .SH NAME
 nitrocli \- access Nitrokey devices
 .SH SYNOPSIS
@@ -201,7 +201,9 @@ reset using \fBgpg\fR(1).
 Use the \fBstatus\fR command to check the retry counters.
 .TP
 .B nitrocli pin clear
-Clear the PINs cached by the other commands.
+Clear the PINs cached by the other commands. Note that cached PINs are
+associated with the device they belong to and the \fBclear\fR command will only
+clear the PIN for the currently used device, not all others.
 .TP
 \fBnitrocli pin set \fItype\fR
 Change a PIN.
diff --git a/nitrocli/doc/nitrocli.1.pdf b/nitrocli/doc/nitrocli.1.pdf
index f93e3b7..a823db0 100644
--- a/nitrocli/doc/nitrocli.1.pdf
+++ b/nitrocli/doc/nitrocli.1.pdf
diff --git a/nitrocli/src/args.rs b/nitrocli/src/args.rs
index f4e0758..b15ca65 100644
--- a/nitrocli/src/args.rs
+++ b/nitrocli/src/args.rs
@@ -631,7 +631,7 @@ fn pin_clear(ctx: &mut ExecCtx<'_>, args: Vec<String>) -> Result<()> {
   parser.set_description("Clears the cached PINs");
   parse(ctx, &parser, args)?;
 
-  commands::pin_clear()
+  commands::pin_clear(ctx)
 }
 
 /// Change a PIN.
diff --git a/nitrocli/src/commands.rs b/nitrocli/src/commands.rs
index 209fb8f..00a594f 100644
--- a/nitrocli/src/commands.rs
+++ b/nitrocli/src/commands.rs
@@ -81,13 +81,18 @@ fn get_storage_device(ctx: &mut args::ExecCtx<'_>) -> Result<nitrokey::Storage>
 }
 
 /// Open the password safe on the given device.
-fn get_password_safe<'dev>(
+fn get_password_safe<'dev, D>(
   ctx: &mut args::ExecCtx<'_>,
-  device: &'dev dyn Device,
-) -> Result<nitrokey::PasswordSafe<'dev>> {
+  device: &'dev D,
+) -> Result<nitrokey::PasswordSafe<'dev>>
+where
+  D: Device,
+{
+  let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, device)?;
+
   try_with_pin_and_data(
     ctx,
-    pinentry::PinType::User,
+    &pin_entry,
     "Could not access the password safe",
     (),
     |_, pin| device.get_password_safe(pin).map_err(|err| ((), err)),
@@ -108,7 +113,9 @@ where
   D: Device,
   F: Fn(D, &str) -> result::Result<A, (D, nitrokey::CommandError)>,
 {
-  try_with_pin_and_data(ctx, pin_type, msg, device, op)
+  let pin_entry = pinentry::PinEntry::from(pin_type, &device)?;
+
+  try_with_pin_and_data(ctx, &pin_entry, msg, device, op)
 }
 
 /// Authenticate the given device with the user PIN.
@@ -167,7 +174,7 @@ fn get_volume_status(status: &nitrokey::VolumeStatus) -> &'static str {
 /// second or third try, it will call `op` with the data returned by the
 /// previous call to `op`.
 fn try_with_pin_and_data_with_pinentry<D, F, R>(
-  pin_type: pinentry::PinType,
+  pin_entry: &pinentry::PinEntry,
   msg: &'static str,
   data: D,
   op: F,
@@ -179,12 +186,12 @@ where
   let mut retry = 3;
   let mut error_msg = None;
   loop {
-    let pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Query, error_msg)?;
+    let pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Query, error_msg)?;
     match op(data, &pin) {
       Ok(result) => return Ok(result),
       Err((new_data, err)) => match err {
         nitrokey::CommandError::WrongPassword => {
-          pinentry::clear_pin(pin_type)?;
+          pinentry::clear_pin(pin_entry)?;
           retry -= 1;
 
           if retry > 0 {
@@ -203,7 +210,7 @@ where
 /// Try to execute the given function with a PIN.
 fn try_with_pin_and_data<D, F, R>(
   ctx: &mut args::ExecCtx<'_>,
-  pin_type: pinentry::PinType,
+  pin_entry: &pinentry::PinEntry,
   msg: &'static str,
   data: D,
   op: F,
@@ -211,7 +218,7 @@ fn try_with_pin_and_data<D, F, R>(
 where
   F: Fn(D, &str) -> result::Result<R, (D, nitrokey::CommandError)>,
 {
-  let pin = match pin_type {
+  let pin = match pin_entry.pin_type() {
     pinentry::PinType::Admin => &ctx.admin_pin,
     pinentry::PinType::User => &ctx.user_pin,
   };
@@ -225,7 +232,7 @@ where
     })?;
     op(data, &pin).map_err(|(_, err)| get_error(msg, err))
   } else {
-    try_with_pin_and_data_with_pinentry(pin_type, msg, data, op)
+    try_with_pin_and_data_with_pinentry(pin_entry, msg, data, op)
   }
 }
 
@@ -235,14 +242,14 @@ where
 /// it refrains from passing any data to it.
 fn try_with_pin<F>(
   ctx: &mut args::ExecCtx<'_>,
-  pin_type: pinentry::PinType,
+  pin_entry: &pinentry::PinEntry,
   msg: &'static str,
   op: F,
 ) -> Result<()>
 where
   F: Fn(&str) -> result::Result<(), nitrokey::CommandError>,
 {
-  try_with_pin_and_data(ctx, pin_type, msg, (), |data, pin| {
+  try_with_pin_and_data(ctx, pin_entry, msg, (), |data, pin| {
     op(pin).map_err(|err| (data, err))
   })
 }
@@ -287,12 +294,11 @@ pub fn status(ctx: &mut args::ExecCtx<'_>) -> Result<()> {
 /// Open the encrypted volume on the nitrokey.
 pub fn storage_open(ctx: &mut args::ExecCtx<'_>) -> Result<()> {
   let device = get_storage_device(ctx)?;
-  try_with_pin(
-    ctx,
-    pinentry::PinType::User,
-    "Opening encrypted volume failed",
-    |pin| device.enable_encrypted_volume(&pin),
-  )
+  let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, &device)?;
+
+  try_with_pin(ctx, &pin_entry, "Opening encrypted volume failed", |pin| {
+    device.enable_encrypted_volume(&pin)
+  })
 }
 
 /// Close the previously opened encrypted volume.
@@ -573,9 +579,14 @@ pub fn otp_status(ctx: &mut args::ExecCtx<'_>, all: bool) -> Result<()> {
 }
 
 /// Clear the PIN stored by various operations.
-pub fn pin_clear() -> Result<()> {
-  pinentry::clear_pin(pinentry::PinType::Admin)?;
-  pinentry::clear_pin(pinentry::PinType::User)?;
+pub fn pin_clear(ctx: &mut args::ExecCtx<'_>) -> Result<()> {
+  let device = get_device(ctx)?;
+
+  pinentry::clear_pin(&pinentry::PinEntry::from(
+    pinentry::PinType::Admin,
+    &device,
+  )?)?;
+  pinentry::clear_pin(&pinentry::PinEntry::from(pinentry::PinType::User, &device)?)?;
   Ok(())
 }
 
@@ -594,14 +605,14 @@ fn check_pin(pin_type: pinentry::PinType, pin: &str) -> Result<()> {
   }
 }
 
-fn choose_pin_with_pinentry(pin_type: pinentry::PinType) -> Result<String> {
-  pinentry::clear_pin(pin_type)?;
-  let new_pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Choose, None)?;
-  pinentry::clear_pin(pin_type)?;
-  check_pin(pin_type, &new_pin)?;
+fn choose_pin_with_pinentry(pin_entry: &pinentry::PinEntry) -> Result<String> {
+  pinentry::clear_pin(pin_entry)?;
+  let new_pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Choose, None)?;
+  pinentry::clear_pin(pin_entry)?;
+  check_pin(pin_entry.pin_type(), &new_pin)?;
 
-  let confirm_pin = pinentry::inquire_pin(pin_type, pinentry::Mode::Confirm, None)?;
-  pinentry::clear_pin(pin_type)?;
+  let confirm_pin = pinentry::inquire_pin(pin_entry, pinentry::Mode::Confirm, None)?;
+  pinentry::clear_pin(pin_entry)?;
 
   if new_pin != confirm_pin {
     Err(Error::from("Entered PINs do not match"))
@@ -616,10 +627,10 @@ fn choose_pin_with_pinentry(pin_type: pinentry::PinType) -> Result<String> {
 /// given PIN type, it will be used.
 fn choose_pin(
   ctx: &mut args::ExecCtx<'_>,
-  pin_type: pinentry::PinType,
+  pin_entry: &pinentry::PinEntry,
   new: bool,
 ) -> Result<String> {
-  let new_pin = match pin_type {
+  let new_pin = match pin_entry.pin_type() {
     pinentry::PinType::Admin => {
       if new {
         &ctx.new_admin_pin
@@ -642,17 +653,19 @@ fn choose_pin(
       .ok_or_else(|| Error::from("Failed to read PIN: invalid Unicode data found"))
       .map(ToOwned::to_owned)
   } else {
-    choose_pin_with_pinentry(pin_type)
+    choose_pin_with_pinentry(pin_entry)
   }
 }
 
 /// Change a PIN.
 pub fn pin_set(ctx: &mut args::ExecCtx<'_>, pin_type: pinentry::PinType) -> Result<()> {
   let device = get_device(ctx)?;
-  let new_pin = choose_pin(ctx, pin_type, true)?;
+  let pin_entry = pinentry::PinEntry::from(pin_type, &device)?;
+  let new_pin = choose_pin(ctx, &pin_entry, true)?;
+
   try_with_pin(
     ctx,
-    pin_type,
+    &pin_entry,
     "Could not change the PIN",
     |current_pin| match pin_type {
       pinentry::PinType::Admin => device.change_admin_pin(&current_pin, &new_pin),
@@ -664,10 +677,13 @@ pub fn pin_set(ctx: &mut args::ExecCtx<'_>, pin_type: pinentry::PinType) -> Resu
 /// Unblock and reset the user PIN.
 pub fn pin_unblock(ctx: &mut args::ExecCtx<'_>) -> Result<()> {
   let device = get_device(ctx)?;
-  let user_pin = choose_pin(ctx, pinentry::PinType::User, false)?;
+  let pin_entry = pinentry::PinEntry::from(pinentry::PinType::User, &device)?;
+  let user_pin = choose_pin(ctx, &pin_entry, false)?;
+  let pin_entry = pinentry::PinEntry::from(pinentry::PinType::Admin, &device)?;
+
   try_with_pin(
     ctx,
-    pinentry::PinType::Admin,
+    &pin_entry,
     "Could not unblock the user PIN",
     |admin_pin| device.unlock_user_pin(&admin_pin, &user_pin),
   )
diff --git a/nitrocli/src/pinentry.rs b/nitrocli/src/pinentry.rs
index d54a182..1eecdd0 100644
--- a/nitrocli/src/pinentry.rs
+++ b/nitrocli/src/pinentry.rs
@@ -46,34 +46,66 @@ impl str::FromStr for PinType {
   }
 }
 
-impl PinType {
-  fn cache_id(self) -> &'static str {
-    match self {
-      PinType::Admin => "nitrocli:admin",
-      PinType::User => "nitrocli:user",
+#[derive(Debug)]
+pub struct PinEntry {
+  pin_type: PinType,
+  model: nitrokey::Model,
+  serial: String,
+}
+
+impl PinEntry {
+  pub fn from<D>(pin_type: PinType, device: &D) -> crate::Result<Self>
+  where
+    D: nitrokey::Device,
+  {
+    let model = device.get_model();
+    let serial = device.get_serial_number()?;
+    Ok(Self {
+      pin_type,
+      model,
+      serial,
+    })
+  }
+
+  fn cache_id(&self) -> String {
+    let model = self.model.to_string().to_lowercase();
+    let suffix = format!("{}:{}", model, self.serial);
+
+    match self.pin_type {
+      PinType::Admin => format!("nitrocli:admin:{}", suffix),
+      PinType::User => format!("nitrocli:user:{}", suffix),
     }
   }
 
-  fn prompt(self) -> &'static str {
-    match self {
+  fn prompt(&self) -> &'static str {
+    match self.pin_type {
       PinType::Admin => "Admin PIN",
       PinType::User => "User PIN",
     }
   }
 
-  fn description(self, mode: Mode) -> &'static str {
-    match self {
-      PinType::Admin => match mode {
-        Mode::Choose => "Please enter a new admin PIN",
-        Mode::Confirm => "Please confirm the new admin PIN",
-        Mode::Query => "Please enter the admin PIN",
+  fn description(&self, mode: Mode) -> String {
+    format!(
+      "{} for\rNitrokey {} {}",
+      match self.pin_type {
+        PinType::Admin => match mode {
+          Mode::Choose => "Please enter a new admin PIN",
+          Mode::Confirm => "Please confirm the new admin PIN",
+          Mode::Query => "Please enter the admin PIN",
+        },
+        PinType::User => match mode {
+          Mode::Choose => "Please enter a new user PIN",
+          Mode::Confirm => "Please confirm the new user PIN",
+          Mode::Query => "Please enter the user PIN",
+        },
       },
-      PinType::User => match mode {
-        Mode::Choose => "Please enter a new user PIN",
-        Mode::Confirm => "Please confirm the new user PIN",
-        Mode::Query => "Please enter the user PIN",
-      },
-    }
+      self.model,
+      self.serial,
+    )
+  }
+
+  pub fn pin_type(&self) -> PinType {
+    self.pin_type
   }
 }
 
@@ -134,18 +166,18 @@ where
 /// description and to decide whether a quality bar is shown in the
 /// dialog.
 pub fn inquire_pin(
-  pin_type: PinType,
+  pin_entry: &PinEntry,
   mode: Mode,
   error_msg: Option<&str>,
 ) -> Result<String, Error> {
-  let cache_id = pin_type.cache_id();
+  let cache_id = pin_entry.cache_id();
   let error_msg = error_msg
     .map(|msg| msg.replace(" ", "+"))
     .unwrap_or_else(|| String::from("+"));
-  let prompt = pin_type.prompt().replace(" ", "+");
-  let description = pin_type.description(mode).replace(" ", "+");
+  let prompt = pin_entry.prompt().replace(" ", "+");
+  let description = pin_entry.description(mode).replace(" ", "+");
 
-  let args = vec![cache_id, &error_msg, &prompt, &description].join(" ");
+  let args = vec![cache_id, error_msg, prompt, description].join(" ");
   let mut command = "GET_PASSPHRASE --data ".to_string();
   if mode.show_quality_bar() {
     command += "--qualitybar ";
@@ -178,9 +210,9 @@ where
   Err(Error::Error(format!("Unexpected response: {}", string)))
 }
 
-/// Clear the cached pin of the given type.
-pub fn clear_pin(pin_type: PinType) -> Result<(), Error> {
-  let command = "CLEAR_PASSPHRASE ".to_string() + pin_type.cache_id();
+/// Clear the cached pin represented by the given entry.
+pub fn clear_pin(pin_entry: &PinEntry) -> Result<(), Error> {
+  let command = format!("CLEAR_PASSPHRASE {}", pin_entry.cache_id());
   let output = process::Command::new("gpg-connect-agent")
     .arg(command)
     .arg("/bye")