Clean up Command, simplify serialization.

openpgp-card/src/apdu.rs

@@ -14,16 +14,9 @@ use std::convert::TryFrom;
 use crate::apdu::{command::Command, response::RawResponse};
 use crate::{CardClientBox, Error, StatusBytes};
 
-// "Maximum amount of bytes in a short APDU command or response" (from pcsc)
+/// "Maximum amount of bytes in a short APDU command or response" (from pcsc)
 const MAX_BUFFER_SIZE: usize = 264;
 
-#[derive(Clone, Copy, PartialEq, Debug)]
-pub(crate) enum Le {
-    None,
-    Short,
-    Long,
-}
-
 /// Send a Command and return the result as a Response.
 ///
 /// If the reply is truncated, this fn assembles all the parts and returns
@@ -78,7 +71,7 @@ pub(crate) fn send_command(
 fn send_command_low_level(
     card_client: &mut CardClientBox,
     cmd: Command,
-    expect_reply: bool,
+    expect_response: bool,
 ) -> Result<Vec<u8>, Error> {
     let (ext_support, chaining_support, mut max_cmd_bytes, max_rsp_bytes) =
         if let Some(caps) = card_client.get_caps() {
@@ -114,20 +107,19 @@ fn send_command_low_level(
         max_rsp_bytes
     );
 
-    // Set Le to 'long', if we're using an extended chunk size.
+    // Decide if we want to use "extended length fields".
     //
-    // According to the Card spec 3.4.1, pg 47,
+    // Current approach: we only use extended length if the card supports it,
-    // 255 is the maximum value for 'Lc' in short mode:
+    // and only if the current command has more than 255 bytes of data.
-    // "A short Lc field consists of one byte not set to '00' (1 to 255 dec.)"
+    //
-    let ext = match (expect_reply, ext_support && max_cmd_bytes > 0xFF) {
+    // (This could be a problem with cards that don't support chained
-        (false, _) => Le::None,
+    // responses, when a response if >255 bytes long - e.g. getting public
-        (_, true) => Le::Long,
+    // key data from cards?)
-        _ => Le::Short,
+    let ext_len = ext_support && (max_cmd_bytes > 0xFF);
-    };
 
     log::debug!(" -> full APDU command: {:x?}", cmd);
 
-    let buf_size = if !ext_support || ext == Le::Short {
+    let buf_size = if !ext_len {
         MAX_BUFFER_SIZE
     } else {
         max_rsp_bytes
@@ -135,28 +127,24 @@ fn send_command_low_level(
 
     log::trace!("buf_size {}", buf_size);
 
-    if chaining_support && !cmd.get_data().is_empty() {
+    if chaining_support && !cmd.data().is_empty() {
         // Send command in chained mode
 
         log::debug!("chained command mode");
 
         // Break up payload into chunks that fit into one command, each
-        let chunks: Vec<_> = cmd.get_data().chunks(max_cmd_bytes).collect();
+        let chunks: Vec<_> = cmd.data().chunks(max_cmd_bytes).collect();
 
         for (i, d) in chunks.iter().enumerate() {
             let last = i == chunks.len() - 1;
 
             let cla = if last { 0x00 } else { 0x10 };
-            let partial = Command::new(
+            let partial =
-                cla,
+                Command::new(cla, cmd.ins(), cmd.p1(), cmd.p2(), d.to_vec());
-                cmd.get_ins(),
-                cmd.get_p1(),
-                cmd.get_p2(),
-                d.to_vec(),
-            );
 
-            let serialized =
+            let serialized = partial
-                partial.serialize(ext).map_err(Error::InternalError)?;
+                .serialize(ext_len, expect_response)
+                .map_err(Error::InternalError)?;
 
             log::debug!(" -> chained APDU command: {:x?}", &serialized);
 
@@ -193,7 +181,7 @@ fn send_command_low_level(
         }
         unreachable!("This state should be unreachable");
     } else {
-        let serialized = cmd.serialize(ext)?;
+        let serialized = cmd.serialize(ext_len, expect_response)?;
 
         // Can't send this command to the card, because it is too long and
         // the card doesn't support command chaining.

openpgp-card/src/apdu/command.rs

@@ -4,10 +4,8 @@
 //! Data structure for APDU Commands
 //! (Commands get sent to the card, which will usually send back a `Response`)
 
-use crate::apdu::Le;
 use anyhow::Result;
 
-#[allow(clippy::upper_case_acronyms)]
 #[derive(Clone, Debug)]
 pub(crate) struct Command {
     // Class byte (CLA)
@@ -43,66 +41,84 @@ impl Command {
         }
     }
 
-    pub(crate) fn get_ins(&self) -> u8 {
+    pub(crate) fn ins(&self) -> u8 {
         self.ins
     }
 
-    pub(crate) fn get_p1(&self) -> u8 {
+    pub(crate) fn p1(&self) -> u8 {
         self.p1
     }
 
-    pub(crate) fn get_p2(&self) -> u8 {
+    pub(crate) fn p2(&self) -> u8 {
         self.p2
     }
 
-    pub(crate) fn get_data(&self) -> &[u8] {
+    pub(crate) fn data(&self) -> &[u8] {
         &self.data
     }
 
-    fn encode_len(len: u16, ext: Le) -> Vec<u8> {
+    /// Serialize a Command (for sending to a card).
-        if len > 0xff || ext == Le::Long {
+    ///
-            vec![0, (len as u16 >> 8) as u8, (len as u16 & 255) as u8]
+    /// See OpenPGP card spec, chapter 7 (pg 47)
-        } else {
+    pub(crate) fn serialize(
-            vec![len as u8]
+        &self,
-        }
+        ext_len: bool,
-    }
+        expect_response: bool,
+    ) -> Result<Vec<u8>> {
+        // FIXME? (from scd/apdu.c):
+        //  T=0 does not allow the use of Lc together with Le;
+        //  thus disable Le in this case.
 
-    pub(crate) fn serialize(&self, ext: Le) -> Result<Vec<u8>> {
+        // "number of bytes in the command data field"
-        // See OpenPGP card spec, chapter 7 (pg 47)
+        let nc = self.data.len() as u16;
-
-        // FIXME: 1) get "ext" information (how long can commands and
-        // responses be),
-        // FIXME: 2) decide on long vs. short encoding for both Lc and Le
-        // (must be the same)
-
-        let data_len = Self::encode_len(self.data.len() as u16, ext);
 
         let mut buf = vec![self.cla, self.ins, self.p1, self.p2];
-
+        buf.extend(Self::make_lc(nc, ext_len));
-        if !self.data.is_empty() {
+        buf.extend(&self.data);
-            buf.extend_from_slice(&data_len);
+        buf.extend(Self::make_le(nc, ext_len, expect_response));
-            buf.extend_from_slice(&self.data[..]);
-        }
-
-        // Le
-        match ext {
-            // FIXME? (from scd/apdu.c):
-            // /* T=0 does not allow the use of Lc together with Le;
-            //                  thus disable Le in this case.  */
-            //               if (reader_table[slot].is_t0)
-            //                 le = -1;
-            Le::None => (),
-
-            Le::Short => buf.push(0),
-            Le::Long => {
-                buf.push(0);
-                buf.push(0);
-                if self.data.is_empty() {
-                    buf.push(0)
-                }
-            }
-        }
 
         Ok(buf)
     }
+
+    /// Encode len for Lc field
+    fn make_lc(len: u16, ext_len: bool) -> Vec<u8> {
+        if !ext_len {
+            assert!(len <= 0xff, "unexpected: len > 0xff, but ext says Short");
+        }
+
+        if len == 0 {
+            vec![]
+        } else if !ext_len {
+            vec![len as u8]
+        } else {
+            vec![0, (len as u16 >> 8) as u8, (len as u16 & 255) as u8]
+        }
+    }
+
+    /// Encode value for Le field
+    /// ("maximum number of bytes expected in the response data field").
+    fn make_le(nc: u16, ext_len: bool, expect_response: bool) -> Vec<u8> {
+        match (ext_len, expect_response) {
+            (_, false) => {
+                // No response data expected.
+                // "If the Le field is absent, then Ne is zero"
+                vec![]
+            }
+            (false, true) => {
+                // A short Le field consists of one byte with any value
+                vec![0]
+            }
+            (true, true) => {
+                if nc == 0 {
+                    // "three bytes (one byte set to '00' followed by two
+                    // bytes with any value) if the Lc field is absent"
+                    vec![0, 0, 0]
+                } else {
+                    // "two bytes (with any value) if an extended Lc field
+                    // is present"
+                    vec![0, 0]
+                }
+            }
+        }
+    }
 }