Factor Response into apdu module; improved debugging

This commit merges the `apdu` and `response` modules: the responses are
APDU responses, and so the two are related.

This also moves the `trace` logging into the APDU type, which allows it
to display `Debug` output for APDUs and responses, which makes it easier
to understand what's going on (and will be even better once instructions
are converted into an enum so you can actually see what's happening).

.github/workflows/rust.yml

@@ -149,7 +149,7 @@ jobs:
         uses: actions-rs/cargo@v1
         with:
           command: clippy
-          args: -- -D warnings
+          args: --all-features -- -D warnings
 
   # TODO: use actions-rs/audit-check
   security_audit:

README.md

@@ -112,15 +112,17 @@ To trace every message sent to/from the card i.e. the raw
 Application Protocol Data Unit (APDU) messages, use the `trace` log level:
 
 ```
-running 1 test
 [INFO  yubikey_piv::yubikey] trying to connect to reader 'Yubico YubiKey OTP+FIDO+CCID'
 [INFO  yubikey_piv::yubikey] connected to 'Yubico YubiKey OTP+FIDO+CCID' successfully
+[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 164, p1: 4, p2: 0, lc: 5, data: [160, 0, 0, 3, 8] }
 [TRACE yubikey_piv::transaction] >>> [0, 164, 4, 0, 5, 160, 0, 0, 3, 8]
-[TRACE yubikey_piv::transaction] <<< [97, 17, 79, 6, 0, 0, 16, 0, 1, 0, 121, 7, 79, 5, 160, 0, 0, 3, 8, 144, 0]
+[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [97, 17, 79, 6, 0, 0, 16, 0, 1, 0, 121, 7, 79, 5, 160, 0, 0, 3, 8] }
+[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 253, p1: 0, p2: 0, lc: 0, data: [] }
 [TRACE yubikey_piv::transaction] >>> [0, 253, 0, 0, 0]
-[TRACE yubikey_piv::transaction] <<< [5, 1, 2, 144, 0]
+[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [5, 1, 2] }
+[TRACE yubikey_piv::apdu] >>> APDU { cla: 0, ins: 248, p1: 0, p2: 0, lc: 0, data: [] }
 [TRACE yubikey_piv::transaction] >>> [0, 248, 0, 0, 0]
-[TRACE yubikey_piv::transaction] <<< [0, 115, 0, 178, 144, 0]
+[TRACE yubikey_piv::apdu] <<< Response { status_words: Success, data: [0, 115, 0, 178] }
 test connect ... ok
 ```
 

src/apdu.rs

@@ -30,7 +30,8 @@
 // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
-use crate::{error::Error, response::Response, transaction::Transaction, Buffer};
+use crate::{error::Error, transaction::Transaction, Buffer};
+use log::trace;
 use std::fmt::{self, Debug};
 use zeroize::{Zeroize, Zeroizing};
 
@@ -112,11 +113,13 @@ impl APDU {
 
     /// Transmit this APDU using the given card transaction
     pub fn transmit(&self, txn: &Transaction<'_>, recv_len: usize) -> Result<Response, Error> {
-        let response_bytes = txn.transmit(&self.to_bytes(), recv_len)?;
+        trace!(">>> {:?}", self);
-        Ok(Response::from_bytes(response_bytes))
+        let response = Response::from(txn.transmit(&self.to_bytes(), recv_len)?);
+        trace!("<<< {:?}", &response);
+        Ok(response)
     }
 
-    /// Consume this APDU and return a self-zeroizing buffer
+    /// Serialize this APDU as a self-zeroizing byte buffer
     pub fn to_bytes(&self) -> Buffer {
         let mut bytes = Vec::with_capacity(5 + self.data.len());
         bytes.push(self.cla);
@@ -159,3 +162,162 @@ impl Zeroize for APDU {
         self.data.zeroize();
     }
 }
+
+/// APDU responses
+#[derive(Debug)]
+pub(crate) struct Response {
+    /// Status words
+    status_words: StatusWords,
+
+    /// Buffer
+    data: Vec<u8>,
+}
+
+impl Response {
+    /// Create a new response from the given status words and buffer
+    #[cfg(feature = "untested")]
+    pub fn new(status_words: StatusWords, data: Vec<u8>) -> Response {
+        Response { status_words, data }
+    }
+
+    /// Get the [`StatusWords`] for this response.
+    pub fn status_words(&self) -> StatusWords {
+        self.status_words
+    }
+
+    /// Get the raw [`StatusWords`] code for this response.
+    #[cfg(feature = "untested")]
+    pub fn code(&self) -> u32 {
+        self.status_words.code()
+    }
+
+    /// Do the status words for this response indicate success?
+    pub fn is_success(&self) -> bool {
+        self.status_words.is_success()
+    }
+
+    /// Borrow the response data
+    pub fn data(&self) -> &[u8] {
+        self.data.as_ref()
+    }
+}
+
+impl AsRef<[u8]> for Response {
+    fn as_ref(&self) -> &[u8] {
+        self.data()
+    }
+}
+
+impl Drop for Response {
+    fn drop(&mut self) {
+        self.zeroize();
+    }
+}
+
+impl From<Vec<u8>> for Response {
+    fn from(mut bytes: Vec<u8>) -> Self {
+        if bytes.len() < 2 {
+            return Response {
+                status_words: StatusWords::None,
+                data: bytes,
+            };
+        }
+
+        let sw = StatusWords::from(
+            (bytes[bytes.len() - 2] as u32) << 8 | (bytes[bytes.len() - 1] as u32),
+        );
+
+        let len = bytes.len() - 2;
+        bytes.truncate(len);
+
+        Response {
+            status_words: sw,
+            data: bytes,
+        }
+    }
+}
+
+impl Zeroize for Response {
+    fn zeroize(&mut self) {
+        self.data.zeroize();
+    }
+}
+
+/// Status Words (SW) are 2-byte values returned by a card command.
+///
+/// The first byte of a status word is referred to as SW1 and the second byte
+/// of a status word is referred to as SW2.
+///
+/// See NIST special publication 800-73-4, section 5.6:
+/// <https://csrc.nist.gov/publications/detail/sp/800-73/4/final>
+#[derive(Copy, Clone, Debug, Eq, PartialEq)]
+pub(crate) enum StatusWords {
+    /// No status words present in response
+    None,
+
+    /// Successful execution
+    Success,
+
+    /// Security status not satisfied
+    SecurityStatusError,
+
+    /// Authentication method blocked
+    AuthBlockedError,
+
+    /// Incorrect parameter in command data field
+    IncorrectParamError,
+
+    //
+    // Custom Yubico Status Word extensions
+    //
+    /// Incorrect card slot error
+    IncorrectSlotError,
+
+    /// Not supported error
+    NotSupportedError,
+
+    /// Other/unrecognized status words
+    Other(u32),
+}
+
+impl StatusWords {
+    /// Get the numerical response code for these status words
+    pub fn code(self) -> u32 {
+        match self {
+            StatusWords::None => 0,
+            StatusWords::SecurityStatusError => 0x6982,
+            StatusWords::AuthBlockedError => 0x6983,
+            StatusWords::IncorrectParamError => 0x6a80,
+            StatusWords::IncorrectSlotError => 0x6b00,
+            StatusWords::NotSupportedError => 0x6d00,
+            StatusWords::Success => 0x9000,
+            StatusWords::Other(n) => n,
+        }
+    }
+
+    /// Do these status words indicate success?
+    pub fn is_success(self) -> bool {
+        self == StatusWords::Success
+    }
+}
+
+impl From<u32> for StatusWords {
+    fn from(sw: u32) -> Self {
+        match sw {
+            0x0000 => StatusWords::None,
+            0x6982 => StatusWords::SecurityStatusError,
+            0x6983 => StatusWords::AuthBlockedError,
+            0x6a80 => StatusWords::IncorrectParamError,
+            0x6b00 => StatusWords::IncorrectSlotError,
+            0x6d00 => StatusWords::NotSupportedError,
+            0x9000 => StatusWords::Success,
+            _ => StatusWords::Other(sw),
+        }
+    }
+}
+
+impl From<StatusWords> for u32 {
+    fn from(sw: StatusWords) -> u32 {
+        sw.code()
+    }
+}

src/key.rs

@@ -38,14 +38,14 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 use crate::{
+    apdu::StatusWords,
     certificate::{self, Certificate},
     consts::*,
     error::Error,
-    response::StatusWords,
     serialization::*,
     settings,
     yubikey::YubiKey,
-    AlgorithmId, ObjectId,
+    AlgorithmId, Buffer, ObjectId,
 };
 use log::{debug, error, warn};
 
@@ -309,7 +309,7 @@ pub fn generate(
         }
     }
 
-    let data = response.into_buffer();
+    let data = Buffer::new(response.data().into());
 
     match algorithm {
         YKPIV_ALGO_RSA1024 | YKPIV_ALGO_RSA2048 => {

src/lib.rs

@@ -155,7 +155,6 @@ mod metadata;
 pub mod mgm;
 #[cfg(feature = "untested")]
 pub mod msroots;
-mod response;
 #[cfg(feature = "untested")]
 mod serialization;
 #[cfg(feature = "untested")]

src/response.rs

@@ -1,186 +0,0 @@
-//! Responses to issued commands
-
-// Adapted from yubico-piv-tool:
-// <https://github.com/Yubico/yubico-piv-tool/>
-//
-// Copyright (c) 2014-2016 Yubico AB
-// All rights reserved.
-//
-// Redistribution and use in source and binary forms, with or without
-// modification, are permitted provided that the following conditions are
-// met:
-//
-//   * Redistributions of source code must retain the above copyright
-//     notice, this list of conditions and the following disclaimer.
-//
-//   * Redistributions in binary form must reproduce the above
-//     copyright notice, this list of conditions and the following
-//     disclaimer in the documentation and/or other materials provided
-//     with the distribution.
-//
-// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
-// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
-// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
-// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
-// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
-// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
-// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
-// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
-// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
-// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
-// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
-
-use crate::Buffer;
-
-/// Parsed response to a command
-pub(crate) struct Response {
-    /// Status words
-    status_words: StatusWords,
-
-    /// Buffer
-    buffer: Buffer,
-}
-
-impl Response {
-    /// Parse a response from the given buffer
-    pub fn from_bytes(mut buffer: Buffer) -> Self {
-        if buffer.len() >= 2 {
-            let sw = StatusWords::from(
-                (buffer[buffer.len() - 2] as u32) << 8 | (buffer[buffer.len() - 1] as u32),
-            );
-
-            let len = buffer.len() - 2;
-            buffer.truncate(len);
-            Response {
-                status_words: sw,
-                buffer,
-            }
-        } else {
-            Response {
-                status_words: StatusWords::None,
-                buffer,
-            }
-        }
-    }
-
-    /// Create a new response from the given status words and buffer
-    #[cfg(feature = "untested")]
-    pub fn new(status_words: StatusWords, buffer: Buffer) -> Response {
-        Response {
-            status_words,
-            buffer,
-        }
-    }
-
-    /// Get the [`StatusWords`] for this response.
-    pub fn status_words(&self) -> StatusWords {
-        self.status_words
-    }
-
-    /// Get the raw [`StatusWords`] code for this response.
-    #[cfg(feature = "untested")]
-    pub fn code(&self) -> u32 {
-        self.status_words.code()
-    }
-
-    /// Do the status words for this response indicate success?
-    pub fn is_success(&self) -> bool {
-        self.status_words.is_success()
-    }
-
-    /// Borrow the response buffer
-    pub fn buffer(&self) -> &[u8] {
-        self.buffer.as_ref()
-    }
-
-    /// Consume this response, returning its buffer
-    #[cfg(feature = "untested")]
-    pub fn into_buffer(self) -> Buffer {
-        self.buffer
-    }
-}
-
-impl AsRef<[u8]> for Response {
-    fn as_ref(&self) -> &[u8] {
-        self.buffer()
-    }
-}
-
-/// Status Words (SW) are 2-byte values returned by a card command.
-///
-/// The first byte of a status word is referred to as SW1 and the second byte
-/// of a status word is referred to as SW2.
-///
-/// See NIST special publication 800-73-4, section 5.6:
-/// <https://csrc.nist.gov/publications/detail/sp/800-73/4/final>
-#[derive(Copy, Clone, Debug, Eq, PartialEq)]
-pub(crate) enum StatusWords {
-    /// No status words present in response
-    None,
-
-    /// Successful execution
-    Success,
-
-    /// Security status not satisfied
-    SecurityStatusError,
-
-    /// Authentication method blocked
-    AuthBlockedError,
-
-    /// Incorrect parameter in command data field
-    IncorrectParamError,
-
-    //
-    // Custom Yubico Status Word extensions
-    //
-    /// Incorrect card slot error
-    IncorrectSlotError,
-
-    /// Not supported error
-    NotSupportedError,
-
-    /// Other/unrecognized status words
-    Other(u32),
-}
-
-impl StatusWords {
-    /// Get the numerical response code for these status words
-    pub fn code(self) -> u32 {
-        match self {
-            StatusWords::None => 0,
-            StatusWords::SecurityStatusError => 0x6982,
-            StatusWords::AuthBlockedError => 0x6983,
-            StatusWords::IncorrectParamError => 0x6a80,
-            StatusWords::IncorrectSlotError => 0x6b00,
-            StatusWords::NotSupportedError => 0x6d00,
-            StatusWords::Success => 0x9000,
-            StatusWords::Other(n) => n,
-        }
-    }
-
-    /// Do these status words indicate success?
-    pub fn is_success(self) -> bool {
-        self == StatusWords::Success
-    }
-}
-
-impl From<u32> for StatusWords {
-    fn from(sw: u32) -> Self {
-        match sw {
-            0x0000 => StatusWords::None,
-            0x6982 => StatusWords::SecurityStatusError,
-            0x6983 => StatusWords::AuthBlockedError,
-            0x6a80 => StatusWords::IncorrectParamError,
-            0x6b00 => StatusWords::IncorrectSlotError,
-            0x6d00 => StatusWords::NotSupportedError,
-            0x9000 => StatusWords::Success,
-            _ => StatusWords::Other(sw),
-        }
-    }
-}
-
-impl From<StatusWords> for u32 {
-    fn from(sw: StatusWords) -> u32 {
-        sw.code()
-    }
-}

src/transaction.rs

@@ -1,17 +1,18 @@
 //! YubiKey PC/SC transactions
 
-use crate::{apdu::APDU, consts::*, error::Error, yubikey::*, Buffer};
+use crate::{apdu::APDU, consts::*, error::Error, yubikey::*};
 #[cfg(feature = "untested")]
 use crate::{
+    apdu::{Response, StatusWords},
     mgm::MgmKey,
-    response::{Response, StatusWords},
     serialization::*,
-    ObjectId,
+    Buffer, ObjectId,
 };
 use log::{error, trace};
 use std::convert::TryInto;
 #[cfg(feature = "untested")]
 use std::ptr;
+#[cfg(feature = "untested")]
 use zeroize::Zeroizing;
 
 /// Exclusive transaction with the YubiKey's PC/SC card.
@@ -43,10 +44,10 @@ impl<'tx> Transaction<'tx> {
     /// single APDU messages at a time. For larger messages that need to be
     /// split into multiple APDUs, use the [`Transaction::transfer_data`]
     /// method instead.
-    pub fn transmit(&self, send_buffer: &[u8], recv_len: usize) -> Result<Buffer, Error> {
+    pub fn transmit(&self, send_buffer: &[u8], recv_len: usize) -> Result<Vec<u8>, Error> {
         trace!(">>> {:?}", send_buffer);
 
-        let mut recv_buffer = Zeroizing::new(vec![0u8; recv_len]);
+        let mut recv_buffer = vec![0u8; recv_len];
 
         let len = self
             .inner
@@ -54,9 +55,6 @@ impl<'tx> Transaction<'tx> {
             .len();
 
         recv_buffer.truncate(len);
-
-        trace!("<<< {:?}", recv_buffer.as_slice());
-
         Ok(recv_buffer)
     }
 
@@ -91,14 +89,14 @@ impl<'tx> Transaction<'tx> {
             return Err(Error::GenericError);
         }
 
-        if response.buffer().len() < 3 {
+        if response.data().len() < 3 {
             return Err(Error::SizeError);
         }
 
         Ok(Version {
-            major: response.buffer()[0],
+            major: response.data()[0],
-            minor: response.buffer()[1],
+            minor: response.data()[1],
-            patch: response.buffer()[2],
+            patch: response.data()[2],
         })
     }
 
@@ -157,7 +155,7 @@ impl<'tx> Transaction<'tx> {
             resp
         };
 
-        response.buffer()[..4]
+        response.data()[..4]
             .try_into()
             .map(|serial| Serial::from(u32::from_be_bytes(serial)))
             .map_err(|_| Error::SizeError)
@@ -363,7 +361,7 @@ impl<'tx> Transaction<'tx> {
             }
         }
 
-        let data = response.buffer();
+        let data = response.data();
 
         // skip the first 7c tag
         if data[0] != 0x7c {
@@ -404,7 +402,7 @@ impl<'tx> Transaction<'tx> {
         max_out: usize,
     ) -> Result<Response, Error> {
         let mut in_offset = 0;
-        let mut out_data = Zeroizing::new(vec![]);
+        let mut out_data = vec![];
         let mut sw = 0;
 
         loop {
@@ -432,17 +430,17 @@ impl<'tx> Transaction<'tx> {
 
             sw = response.status_words().code();
 
-            if out_data.len() - response.buffer().len() - 2 > max_out {
+            if out_data.len() - response.data().len() - 2 > max_out {
                 error!(
                     "output buffer too small: wanted to write {}, max was {}",
-                    out_data.len() - response.buffer().len() - 2,
+                    out_data.len() - response.data().len() - 2,
                     max_out
                 );
 
                 return Err(Error::SizeError);
             }
 
-            out_data.extend_from_slice(&response.buffer()[..response.buffer().len() - 2]);
+            out_data.extend_from_slice(&response.data()[..response.data().len() - 2]);
 
             in_offset += this_size;
             if in_offset >= in_data.len() {
@@ -460,20 +458,20 @@ impl<'tx> Transaction<'tx> {
             sw = response.status_words().code();
 
             if sw != StatusWords::Success.code() && (sw >> 8 != 0x61) {
-                return Ok(Response::new(sw.into(), Zeroizing::new(vec![])));
+                return Ok(Response::new(sw.into(), vec![]));
             }
 
-            if out_data.len() + response.buffer().len() - 2 > max_out {
+            if out_data.len() + response.data().len() - 2 > max_out {
                 error!(
                     "output buffer too small: wanted to write {}, max was {}",
-                    out_data.len() + response.buffer().len() - 2,
+                    out_data.len() + response.data().len() - 2,
                     max_out
                 );
 
                 return Err(Error::SizeError);
             }
 
-            out_data.extend_from_slice(&response.buffer()[..response.buffer().len() - 2]);
+            out_data.extend_from_slice(&response.data()[..response.data().len() - 2]);
         }
 
         Ok(Response::new(sw.into(), out_data))
@@ -495,7 +493,7 @@ impl<'tx> Transaction<'tx> {
             return Err(Error::GenericError);
         }
 
-        let data = response.into_buffer();
+        let data = Buffer::new(response.data().into());
         let mut outlen = 0;
 
         if data.len() < 2 || !has_valid_length(&data[1..], data.len() - 1) {

src/yubikey.rs

@@ -35,7 +35,11 @@
 
 #[cfg(feature = "untested")]
 use crate::{
-    apdu::APDU, key::SlotId, metadata, mgm::MgmKey, response::StatusWords, serialization::*,
+    apdu::{StatusWords, APDU},
+    key::SlotId,
+    metadata,
+    mgm::MgmKey,
+    serialization::*,
     ObjectId,
 };
 use crate::{consts::*, error::Error, transaction::Transaction, Buffer};
@@ -271,12 +275,12 @@ impl YubiKey {
             .data(&[0x7c, 0x02, 0x80, 0x00])
             .transmit(&txn, 261)?;
 
-        if !challenge.is_success() || challenge.buffer().len() < 12 {
+        if !challenge.is_success() || challenge.data().len() < 12 {
             return Err(Error::AuthenticationError);
         }
 
         // send a response to the cards challenge and a challenge of our own.
-        let response = mgm_key.decrypt(challenge.buffer()[4..12].try_into().unwrap());
+        let response = mgm_key.decrypt(challenge.data()[4..12].try_into().unwrap());
 
         let mut data = [0u8; 22];
         data[0] = 0x7c;
@@ -308,7 +312,7 @@ impl YubiKey {
         let response = mgm_key.encrypt(&challenge);
 
         use subtle::ConstantTimeEq;
-        if response.ct_eq(&authentication.buffer()[4..12]).unwrap_u8() != 1 {
+        if response.ct_eq(&authentication.data()[4..12]).unwrap_u8() != 1 {
             return Err(Error::AuthenticationError);
         }
 
@@ -803,11 +807,11 @@ impl YubiKey {
             }
         }
 
-        if response.buffer()[0] != 0x30 {
+        if response.data()[0] != 0x30 {
             return Err(Error::GenericError);
         }
 
-        Ok(response.into_buffer())
+        Ok(Buffer::new(response.data().into()))
     }
 
     /// Get an auth challenge
@@ -824,7 +828,7 @@ impl YubiKey {
             return Err(Error::AuthenticationError);
         }
 
-        Ok(response.buffer()[4..12].try_into().unwrap())
+        Ok(response.data()[4..12].try_into().unwrap())
     }
 
     /// Verify an auth response