From 8aae0a357e3bee427d3dac575d7704bbb3e1605c Mon Sep 17 00:00:00 2001
From: Heiko Schaefer <heiko@schaefer.name>
Date: Fri, 20 Aug 2021 13:17:08 +0200
Subject: [PATCH] Split the `Response` type into an internal `RawResponse` type
 which also contains the status bytes, and an external `Response` type that
 can only be generated from a RawResponse with status "ok".

This removes the need for external users of openpgp-card to check the status or operations.
That is, openpgp-card now always returns an `Err` if the status of a command is not ok.
---
 card-functionality/src/tests.rs   | 77 +++++++++++++++++--------------
 openpgp-card-sequoia/src/lib.rs   | 45 ++++++------------
 openpgp-card-sequoia/src/main.rs  |  4 --
 openpgp-card/src/apdu/mod.rs      | 10 ++--
 openpgp-card/src/apdu/response.rs | 55 ++++++++++++++--------
 openpgp-card/src/card_app.rs      | 38 ++++++++-------
 openpgp-card/src/errors.rs        |  2 +-
 openpgp-card/src/keys.rs          | 16 +++----
 pcsc/src/lib.rs                   |  4 +-
 9 files changed, 128 insertions(+), 123 deletions(-)

diff --git a/card-functionality/src/tests.rs b/card-functionality/src/tests.rs
index 4988824..ae3694a 100644
--- a/card-functionality/src/tests.rs
+++ b/card-functionality/src/tests.rs
@@ -24,7 +24,8 @@ use crate::util;
 
 #[derive(Debug)]
 pub enum TestResult {
-    Status([u8; 2]),
+    Status(OcErrorStatus),
+    StatusOk,
     Text(String),
 }
 
@@ -62,8 +63,7 @@ pub fn test_decrypt(
     let cert = Cert::from_str(param[0])?;
     let msg = param[1].to_string();
 
-    let res = ca.verify_pw1("123456")?;
-    res.check_ok()?;
+    ca.verify_pw1("123456")?;
 
     let res = openpgp_card_sequoia::decrypt(&mut ca, &cert, msg.into_bytes())?;
     let plain = String::from_utf8_lossy(&res);
@@ -80,8 +80,7 @@ pub fn test_sign(
 ) -> Result<TestOutput, TestError> {
     assert_eq!(param.len(), 1, "test_sign needs a filename for 'cert'");
 
-    let res = ca.verify_pw1_for_signing("123456")?;
-    res.check_ok()?;
+    ca.verify_pw1_for_signing("123456")?;
 
     let cert = Cert::from_str(param[0])?;
 
@@ -190,8 +189,7 @@ pub fn test_upload_keys(
         "test_upload_keys needs a filename for 'cert'"
     );
 
-    let verify = ca.verify_pw3("12345678")?;
-    verify.check_ok()?;
+    ca.verify_pw3("12345678")?;
 
     let cert = Cert::from_file(param[0])?;
 
@@ -211,8 +209,7 @@ pub fn test_keygen(
     ca: &mut CardApp,
     param: &[&str],
 ) -> Result<TestOutput, TestError> {
-    let verify = ca.verify_pw3("12345678")?;
-    verify.check_ok()?;
+    ca.verify_pw3("12345678")?;
 
     // Generate all three subkeys on card
     let algo = param[0];
@@ -328,24 +325,19 @@ pub fn test_set_user_data(
     ca: &mut CardApp,
     _param: &[&str],
 ) -> Result<TestOutput, TestError> {
-    let res = ca.verify_pw3("12345678")?;
-    res.check_ok()?;
+    ca.verify_pw3("12345678")?;
 
     // name
-    let res = ca.set_name("Bar<<Foo")?;
-    res.check_ok()?;
+    ca.set_name("Bar<<Foo")?;
 
     // lang
-    let res = ca.set_lang("deen")?;
-    res.check_ok()?;
+    ca.set_lang("deen")?;
 
     // sex
-    let res = ca.set_sex(Sex::Female)?;
-    res.check_ok()?;
+    ca.set_sex(Sex::Female)?;
 
     // url
-    let res = ca.set_url("https://duckduckgo.com/")?;
-    res.check_ok()?;
+    ca.set_url("https://duckduckgo.com/")?;
 
     // read all the fields back again, expect equal data
     let ch = ca.get_cardholder_related_data()?;
@@ -382,31 +374,46 @@ pub fn test_verify(
     let mut out = vec![];
 
     // try to set name without verify, assert result is not ok!
-    let res = ca.set_name("Notverified<<Hello")?;
-    assert_eq!(res.status(), [0x69, 0x82]); // "Security status not satisfied"
+    let res = ca.set_name("Notverified<<Hello");
 
-    let res = ca.verify_pw3("12345678")?;
-    res.check_ok()?;
+    if let Err(OpenpgpCardError::OcStatus(s)) = res {
+        assert_eq!(s, OcErrorStatus::SecurityStatusNotSatisfied);
+    } else {
+        panic!();
+    }
 
-    let check = ca.check_pw3()?;
-    // don't "check_ok()" - yubikey5 returns an error code!
-    out.push(TestResult::Status(check.status()));
+    ca.verify_pw3("12345678")?;
 
-    let res = ca.set_name("Admin<<Hello")?;
-    res.check_ok()?;
+    match ca.check_pw3() {
+        Err(OpenpgpCardError::OcStatus(s)) => {
+            // e.g. yubikey5 returns an error status!
+            out.push(TestResult::Status(s));
+        }
+        Err(_) => {
+            panic!("unexpected error");
+        }
+        Ok(_) => out.push(TestResult::StatusOk),
+    }
+
+    ca.set_name("Admin<<Hello")?;
 
     let cardholder = ca.get_cardholder_related_data()?;
     assert_eq!(cardholder.name, Some("Admin<<Hello".to_string()));
 
-    let res = ca.verify_pw1("123456")?;
-    res.check_ok()?;
+    ca.verify_pw1("123456")?;
 
-    let check = ca.check_pw3()?;
-    // don't "check_ok()" - yubikey5 returns an error code
-    out.push(TestResult::Status(check.status()));
+    match ca.check_pw3() {
+        Err(OpenpgpCardError::OcStatus(s)) => {
+            // e.g. yubikey5 returns an error status!
+            out.push(TestResult::Status(s));
+        }
+        Err(_) => {
+            panic!("unexpected error");
+        }
+        Ok(_) => out.push(TestResult::StatusOk),
+    }
 
-    let res = ca.set_name("There<<Hello")?;
-    res.check_ok()?;
+    ca.set_name("There<<Hello")?;
 
     let cardholder = ca.get_cardholder_related_data()?;
     assert_eq!(cardholder.name, Some("There<<Hello".to_string()));
diff --git a/openpgp-card-sequoia/src/lib.rs b/openpgp-card-sequoia/src/lib.rs
index cd7e946..fa12097 100644
--- a/openpgp-card-sequoia/src/lib.rs
+++ b/openpgp-card-sequoia/src/lib.rs
@@ -204,8 +204,7 @@ pub fn make_cert(
                 )?;
 
         // Allow signing on the card
-        let res = ca.verify_pw1_for_signing("123456")?;
-        res.check_ok()?;
+        ca.verify_pw1_for_signing("123456")?;
 
         // Card-backed signer for bindings
         let mut card_signer = CardSigner::with_pubkey(ca, key_sig.clone());
@@ -230,8 +229,7 @@ pub fn make_cert(
                 .set_key_flags(KeyFlags::empty().set_authentication())?;
 
         // Allow signing on the card
-        let res = ca.verify_pw1_for_signing("123456")?;
-        res.check_ok()?;
+        ca.verify_pw1_for_signing("123456")?;
 
         // Card-backed signer for bindings
         let mut card_signer = CardSigner::with_pubkey(ca, key_sig.clone());
@@ -263,8 +261,7 @@ pub fn make_cert(
                 )?;
 
         // Allow signing on the card
-        let res = ca.verify_pw1_for_signing("123456")?;
-        res.check_ok()?;
+        ca.verify_pw1_for_signing("123456")?;
 
         // Card-backed signer for bindings
         let mut card_signer = CardSigner::with_pubkey(ca, key_sig);
@@ -714,15 +711,11 @@ impl CardBase {
     ) -> Result<CardSign, CardBase> {
         assert!(pin.len() >= 6); // FIXME: Err
 
-        let res = self.card_app.verify_pw1_for_signing(pin);
-
-        if let Ok(resp) = res {
-            if resp.is_ok() {
-                return Ok(CardSign { oc: self });
-            }
+        if self.card_app.verify_pw1_for_signing(pin).is_ok() {
+            Ok(CardSign { oc: self })
+        } else {
+            Err(self)
         }
-
-        Err(self)
     }
 
     pub fn check_pw1(&mut self) -> Result<Response, OpenpgpCardError> {
@@ -732,15 +725,11 @@ impl CardBase {
     pub fn verify_pw1(mut self, pin: &str) -> Result<CardUser, CardBase> {
         assert!(pin.len() >= 6); // FIXME: Err
 
-        let res = self.card_app.verify_pw1(pin);
-
-        if let Ok(resp) = res {
-            if resp.is_ok() {
-                return Ok(CardUser { oc: self });
-            }
+        if self.card_app.verify_pw1(pin).is_ok() {
+            Ok(CardUser { oc: self })
+        } else {
+            Err(self)
         }
-
-        Err(self)
     }
 
     pub fn check_pw3(&mut self) -> Result<Response, OpenpgpCardError> {
@@ -750,15 +739,11 @@ impl CardBase {
     pub fn verify_pw3(mut self, pin: &str) -> Result<CardAdmin, CardBase> {
         assert!(pin.len() >= 8); // FIXME: Err
 
-        let res = self.card_app.verify_pw3(pin);
-
-        if let Ok(resp) = res {
-            if resp.is_ok() {
-                return Ok(CardAdmin { oc: self });
-            }
+        if self.card_app.verify_pw3(pin).is_ok() {
+            Ok(CardAdmin { oc: self })
+        } else {
+            Err(self)
         }
-
-        Err(self)
     }
 }
 
diff --git a/openpgp-card-sequoia/src/main.rs b/openpgp-card-sequoia/src/main.rs
index d76cd33..75e851c 100644
--- a/openpgp-card-sequoia/src/main.rs
+++ b/openpgp-card-sequoia/src/main.rs
@@ -108,20 +108,16 @@ fn main() -> Result<(), Box<dyn Error>> {
 
                 let res = oc_admin.set_name("Bar<<Foo")?;
                 println!("set name {:x?}", res);
-                res.check_ok()?;
 
                 let res =
                     oc_admin.set_sex(openpgp_card::Sex::NotApplicable)?;
                 println!("set sex {:x?}", res);
-                res.check_ok()?;
 
                 let res = oc_admin.set_lang("en")?;
                 println!("set lang {:x?}", res);
-                res.check_ok()?;
 
                 let res = oc_admin.set_url("https://keys.openpgp.org")?;
                 println!("set url {:x?}", res);
-                res.check_ok()?;
 
                 let cert = Cert::from_file(TEST_KEY_PATH)?;
 
diff --git a/openpgp-card/src/apdu/mod.rs b/openpgp-card/src/apdu/mod.rs
index 5b5c07e..7bdfbe4 100644
--- a/openpgp-card/src/apdu/mod.rs
+++ b/openpgp-card/src/apdu/mod.rs
@@ -11,7 +11,7 @@ use anyhow::Result;
 use std::convert::TryFrom;
 
 use crate::apdu::command::Command;
-use crate::apdu::response::Response;
+use crate::apdu::response::RawResponse;
 use crate::errors::{OcErrorStatus, OpenpgpCardError};
 use crate::CardClientBox;
 
@@ -33,20 +33,20 @@ pub(crate) fn send_command(
     card_client: &mut CardClientBox,
     cmd: Command,
     expect_reply: bool,
-) -> Result<Response, OpenpgpCardError> {
-    let mut resp = Response::try_from(send_command_low_level(
+) -> Result<RawResponse, OpenpgpCardError> {
+    let mut resp = RawResponse::try_from(send_command_low_level(
         card_client,
         cmd,
         expect_reply,
     )?)?;
 
-    while resp.status()[0] == 0x61 {
+    while resp.status().0 == 0x61 {
         // More data is available for this command from the card
 
         log::debug!(" response was truncated, getting more data");
 
         // Get additional data
-        let next = Response::try_from(send_command_low_level(
+        let next = RawResponse::try_from(send_command_low_level(
             card_client,
             commands::get_response(),
             expect_reply,
diff --git a/openpgp-card/src/apdu/response.rs b/openpgp-card/src/apdu/response.rs
index 86ddac8..5f32718 100644
--- a/openpgp-card/src/apdu/response.rs
+++ b/openpgp-card/src/apdu/response.rs
@@ -1,19 +1,38 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-use crate::errors::OcErrorStatus;
+use crate::errors::{OcErrorStatus, OpenpgpCardError};
 use std::convert::TryFrom;
 
 /// APDU Response
 #[allow(unused)]
 #[derive(Clone, Debug)]
-pub struct Response {
+pub(crate) struct RawResponse {
     data: Vec<u8>,
     sw1: u8,
     sw2: u8,
 }
 
-impl Response {
+#[derive(Debug)]
+pub struct Response {
+    data: Vec<u8>,
+}
+
+impl TryFrom<RawResponse> for Response {
+    type Error = OpenpgpCardError;
+
+    fn try_from(value: RawResponse) -> Result<Self, Self::Error> {
+        if value.is_ok() {
+            Ok(Response { data: value.data })
+        } else {
+            Err(OpenpgpCardError::OcStatus(OcErrorStatus::from(
+                value.status(),
+            )))
+        }
+    }
+}
+
+impl RawResponse {
     pub fn check_ok(&self) -> Result<(), OcErrorStatus> {
         if !self.is_ok() {
             Err(OcErrorStatus::from((self.sw1, self.sw2)))
@@ -36,22 +55,22 @@ impl Response {
         &mut self.data
     }
 
-    pub(crate) fn set_status(&mut self, new_status: [u8; 2]) {
-        self.sw1 = new_status[0];
-        self.sw2 = new_status[1];
+    pub(crate) fn set_status(&mut self, new_status: (u8, u8)) {
+        self.sw1 = new_status.0;
+        self.sw2 = new_status.1;
     }
 
-    pub fn status(&self) -> [u8; 2] {
-        [self.sw1, self.sw2]
+    pub fn status(&self) -> (u8, u8) {
+        (self.sw1, self.sw2)
     }
 
-    /// Is the response status "ok"? [0x90, 0x00]
+    /// Is the response status "ok"? (0x90, 0x00)
     pub fn is_ok(&self) -> bool {
-        self.status() == [0x90, 0x00]
+        self.status() == (0x90, 0x00)
     }
 }
 
-impl<'a> TryFrom<&[u8]> for Response {
+impl<'a> TryFrom<&[u8]> for RawResponse {
     type Error = OcErrorStatus;
 
     fn try_from(buf: &[u8]) -> Result<Self, OcErrorStatus> {
@@ -59,7 +78,7 @@ impl<'a> TryFrom<&[u8]> for Response {
         if n < 2 {
             return Err(OcErrorStatus::ResponseLength(buf.len()));
         }
-        Ok(Response {
+        Ok(RawResponse {
             data: buf[..n - 2].into(),
             sw1: buf[n - 2],
             sw2: buf[n - 1],
@@ -67,7 +86,7 @@ impl<'a> TryFrom<&[u8]> for Response {
     }
 }
 
-impl TryFrom<Vec<u8>> for Response {
+impl TryFrom<Vec<u8>> for RawResponse {
     type Error = OcErrorStatus;
 
     fn try_from(mut data: Vec<u8>) -> Result<Self, OcErrorStatus> {
@@ -78,32 +97,32 @@ impl TryFrom<Vec<u8>> for Response {
             .pop()
             .ok_or_else(|| OcErrorStatus::ResponseLength(data.len()))?;
 
-        Ok(Response { data, sw1, sw2 })
+        Ok(RawResponse { data, sw1, sw2 })
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use crate::apdu::response::Response;
+    use crate::apdu::response::RawResponse;
     use std::convert::TryFrom;
 
     #[test]
     fn test_two_bytes_data_response() {
-        let res = Response::try_from(vec![0x01, 0x02, 0x90, 0x00]).unwrap();
+        let res = RawResponse::try_from(vec![0x01, 0x02, 0x90, 0x00]).unwrap();
         assert_eq!(res.is_ok(), true);
         assert_eq!(res.data, vec![0x01, 0x02]);
     }
 
     #[test]
     fn test_no_data_response() {
-        let res = Response::try_from(vec![0x90, 0x00]).unwrap();
+        let res = RawResponse::try_from(vec![0x90, 0x00]).unwrap();
         assert_eq!(res.is_ok(), true);
         assert_eq!(res.data, vec![]);
     }
 
     #[test]
     fn test_more_data_response() {
-        let res = Response::try_from(vec![0xAB, 0x61, 0x02]).unwrap();
+        let res = RawResponse::try_from(vec![0xAB, 0x61, 0x02]).unwrap();
         assert_eq!(res.is_ok(), false);
         assert_eq!(res.data, vec![0xAB]);
     }
diff --git a/openpgp-card/src/card_app.rs b/openpgp-card/src/card_app.rs
index 8f6a86b..9a05c17 100644
--- a/openpgp-card/src/card_app.rs
+++ b/openpgp-card/src/card_app.rs
@@ -11,6 +11,7 @@
 
 use std::borrow::BorrowMut;
 use std::convert::TryFrom;
+use std::convert::TryInto;
 use std::time::SystemTime;
 
 use anyhow::{anyhow, Result};
@@ -90,7 +91,8 @@ impl CardApp {
     /// "Select" the OpenPGP card application
     pub fn select(&mut self) -> Result<Response, OpenpgpCardError> {
         let select_openpgp = commands::select_openpgp();
-        apdu::send_command(&mut self.card_client, select_openpgp, false)
+        apdu::send_command(&mut self.card_client, select_openpgp, false)?
+            .try_into()
     }
 
     // --- application data ---
@@ -323,8 +325,8 @@ impl CardApp {
             let verify = commands::verify_pw1_81([0x40; 8].to_vec());
             let resp =
                 apdu::send_command(&mut self.card_client, verify, false)?;
-            if !(resp.status() == [0x69, 0x82]
-                || resp.status() == [0x69, 0x83])
+            if !(resp.status() == (0x69, 0x82)
+                || resp.status() == (0x69, 0x83))
             {
                 return Err(anyhow!("Unexpected status for reset, at pw1."));
             }
@@ -337,8 +339,8 @@ impl CardApp {
             let resp =
                 apdu::send_command(&mut self.card_client, verify, false)?;
 
-            if !(resp.status() == [0x69, 0x82]
-                || resp.status() == [0x69, 0x83])
+            if !(resp.status() == (0x69, 0x82)
+                || resp.status() == (0x69, 0x83))
             {
                 return Err(anyhow!("Unexpected status for reset, at pw3."));
             }
@@ -367,12 +369,12 @@ impl CardApp {
         assert!(pin.len() >= 6); // FIXME: Err
 
         let verify = commands::verify_pw1_81(pin.as_bytes().to_vec());
-        apdu::send_command(&mut self.card_client, verify, false)
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
     pub fn check_pw1(&mut self) -> Result<Response, OpenpgpCardError> {
         let verify = commands::verify_pw1_82(vec![]);
-        apdu::send_command(&mut self.card_client, verify, false)
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
     pub fn verify_pw1(
@@ -382,12 +384,12 @@ impl CardApp {
         assert!(pin.len() >= 6); // FIXME: Err
 
         let verify = commands::verify_pw1_82(pin.as_bytes().to_vec());
-        apdu::send_command(&mut self.card_client, verify, false)
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
     pub fn check_pw3(&mut self) -> Result<Response, OpenpgpCardError> {
         let verify = commands::verify_pw3(vec![]);
-        apdu::send_command(&mut self.card_client, verify, false)
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
     pub fn verify_pw3(
@@ -397,7 +399,7 @@ impl CardApp {
         assert!(pin.len() >= 8); // FIXME: Err
 
         let verify = commands::verify_pw3(pin.as_bytes().to_vec());
-        apdu::send_command(&mut self.card_client, verify, false)
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
     // --- decrypt ---
@@ -503,7 +505,7 @@ impl CardApp {
         name: &str,
     ) -> Result<Response, OpenpgpCardError> {
         let put_name = commands::put_name(name.as_bytes().to_vec());
-        apdu::send_command(&mut self.card_client, put_name, false)
+        apdu::send_command(&mut self.card_client, put_name, false)?.try_into()
     }
 
     pub fn set_lang(
@@ -511,12 +513,14 @@ impl CardApp {
         lang: &str,
     ) -> Result<Response, OpenpgpCardError> {
         let put_lang = commands::put_lang(lang.as_bytes().to_vec());
-        apdu::send_command(self.card_client.borrow_mut(), put_lang, false)
+        apdu::send_command(self.card_client.borrow_mut(), put_lang, false)?
+            .try_into()
     }
 
     pub fn set_sex(&mut self, sex: Sex) -> Result<Response, OpenpgpCardError> {
         let put_sex = commands::put_sex((&sex).into());
-        apdu::send_command(self.card_client.borrow_mut(), put_sex, false)
+        apdu::send_command(self.card_client.borrow_mut(), put_sex, false)?
+            .try_into()
     }
 
     pub fn set_url(
@@ -524,7 +528,7 @@ impl CardApp {
         url: &str,
     ) -> Result<Response, OpenpgpCardError> {
         let put_url = commands::put_url(url.as_bytes().to_vec());
-        apdu::send_command(&mut self.card_client, put_url, false)
+        apdu::send_command(&mut self.card_client, put_url, false)?.try_into()
     }
 
     pub fn set_creation_time(
@@ -545,7 +549,7 @@ impl CardApp {
             time_value,
         );
 
-        apdu::send_command(&mut self.card_client, time_cmd, false)
+        apdu::send_command(&mut self.card_client, time_cmd, false)?.try_into()
     }
 
     pub fn set_fingerprint(
@@ -558,7 +562,7 @@ impl CardApp {
             fp.to_vec(),
         );
 
-        apdu::send_command(self.card(), fp_cmd, false)
+        apdu::send_command(self.card(), fp_cmd, false)?.try_into()
     }
 
     /// Set algorithm attributes [4.4.3.9 Algorithm Attributes]
@@ -582,7 +586,7 @@ impl CardApp {
         // Command to PUT the algorithm attributes
         let cmd = commands::put_data(&[key_type.get_algorithm_tag()], data);
 
-        apdu::send_command(&mut self.card_client, cmd, false)
+        apdu::send_command(&mut self.card_client, cmd, false)?.try_into()
     }
 
     fn rsa_algo_attrs(algo_attrs: &RsaAttrs) -> Result<Vec<u8>> {
diff --git a/openpgp-card/src/errors.rs b/openpgp-card/src/errors.rs
index 62d885e..e4313f4 100644
--- a/openpgp-card/src/errors.rs
+++ b/openpgp-card/src/errors.rs
@@ -41,7 +41,7 @@ impl From<anyhow::Error> for OpenpgpCardError {
 }
 
 /// OpenPGP card "Status Byte" errors
-#[derive(Error, Debug)]
+#[derive(Error, Debug, PartialEq)]
 pub enum OcErrorStatus {
     #[error("Selected file or DO in termination state")]
     TerminationState,
diff --git a/openpgp-card/src/keys.rs b/openpgp-card/src/keys.rs
index 954696f..b7a7c40 100644
--- a/openpgp-card/src/keys.rs
+++ b/openpgp-card/src/keys.rs
@@ -32,9 +32,7 @@ pub(crate) fn gen_key_with_metadata(
 ) -> Result<(PublicKeyMaterial, u32), OpenpgpCardError> {
     // set algo on card if it's Some
     if let Some(algo) = algo {
-        card_app
-            .set_algorithm_attributes(key_type, algo)?
-            .check_ok()?;
+        card_app.set_algorithm_attributes(key_type, algo)?;
     }
 
     // algo
@@ -58,11 +56,11 @@ pub(crate) fn gen_key_with_metadata(
         .map_err(|e| OpenpgpCardError::InternalError(anyhow!(e)))?
         .as_secs() as u32;
 
-    card_app.set_creation_time(ts, key_type)?.check_ok()?;
+    card_app.set_creation_time(ts, key_type)?;
 
     // calculate/store fingerprint
     let fp = fp_from_pub(&pubkey, time, key_type)?;
-    card_app.set_fingerprint(fp, key_type)?.check_ok()?;
+    card_app.set_fingerprint(fp, key_type)?;
 
     Ok((pubkey, ts))
 }
@@ -438,15 +436,13 @@ fn copy_key_to_card(
 
     // FIXME: Only write algo attributes to the card if "extended
     // capabilities" show that they are changeable!
-    card_app
-        .set_algorithm_attributes(key_type, algo)?
-        .check_ok()?;
+    card_app.set_algorithm_attributes(key_type, algo)?;
 
     apdu::send_command(card_app.card(), key_cmd, false)?.check_ok()?;
 
-    card_app.set_fingerprint(fp, key_type)?.check_ok()?;
+    card_app.set_fingerprint(fp, key_type)?;
 
-    card_app.set_creation_time(ts, key_type)?.check_ok()?;
+    card_app.set_creation_time(ts, key_type)?;
 
     Ok(())
 }
diff --git a/pcsc/src/lib.rs b/pcsc/src/lib.rs
index 080a47b..fc35415 100644
--- a/pcsc/src/lib.rs
+++ b/pcsc/src/lib.rs
@@ -99,9 +99,7 @@ impl PcscClient {
         let ccb = Box::new(card_client) as CardClientBox;
 
         let mut ca = CardApp::new(ccb);
-        let resp = ca.select()?;
-
-        if resp.is_ok() {
+        if ca.select().is_ok() {
             Ok(ca)
         } else {
             Err(OpenpgpCardError::Smartcard(