Some cleanup/documentation.

ScdClient now produces the lower level CardClientBox objects (instead of CardBase)

scdc/README.md

@@ -0,0 +1,11 @@
+<!--
+SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
+SPDX-License-Identifier: MIT OR Apache-2.0
+-->
+
+**scdaemon client for the openpgp-card library**
+
+This crate provides `ScdClient`, which is an implementation of the 
+CardClient trait that uses an instance of GnuPG's
+[scdaemon](https://www.gnupg.org/documentation/manuals/gnupg/Invoking-SCDAEMON.html)
+to access OpenPGP cards.

scdc/src/lib.rs

@@ -1,6 +1,9 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! This crate provides `ScdClient`, which is an implementation of the
+//! CardClient trait that uses GnuPG's scdaemon to access OpenPGP cards.
+
 use anyhow::{anyhow, Result};
 use futures::StreamExt;
 use lazy_static::lazy_static;
@@ -9,34 +12,40 @@ use std::sync::Mutex;
 use tokio::runtime::Runtime;
 
 use openpgp_card::errors::OpenpgpCardError;
-use openpgp_card::{CardBase, CardCaps, CardClient, CardClientBox};
+use openpgp_card::{CardCaps, CardClient, CardClientBox};
 
 lazy_static! {
-    pub(crate) static ref RT: Mutex<Runtime> =
+    static ref RT: Mutex<Runtime> =
         Mutex::new(tokio::runtime::Runtime::new().unwrap());
 }
 
-/// The Assuan protocol which is used in GnuPG limits the length of commands.
+/// The Assuan protocol (in GnuPG) limits the length of commands.
-/// Currently there seems to be no way to send longer commands via Assuan.
 ///
 /// See:
 /// https://www.gnupg.org/documentation/manuals/assuan/Client-requests.html#Client-requests
 ///
-/// FIXME: This number is probably off by a few bytes (is "SCD " added in
+/// Currently there seems to be no way to send longer commands via Assuan,
+/// the functionality to break one command into multiple lines has
+/// apparently not yet been implemented.
+///
+/// NOTE: This number is probably off by a few bytes (is "SCD " added in
 /// communication within GnuPG? Are \r\n added?)
 const ASSUAN_LINELENGTH: usize = 1000;
 
-/// The maximum number of bytes for a command that can be sent via Assuan to
+/// The maximum number of bytes for a command that we will send to
-/// scdaemon.
+/// scdaemon (via Assuan).
-/// Each command byte gets sent via Assuan as a two-character hex string,
-/// and a few characters are used to send "APDU --exlen=abcd" (as a
-/// conservative limit, 20 characters are subtracted).
 ///
-/// In concrete terms, this limit means that with cards that do not support
+/// Each command byte gets sent via Assuan as a two-character hex string.
-/// command chaining (like the floss-shop OpenPGP Card 3.4), some commands
+///
-/// cannot be sent to the card. In particular, uploading rsa4096 keys will
+/// 18 characters are used to send "APDU --exlen=abcd "
-/// fail via scdaemon, with such cards.
+/// (So, as a defensive limit, 25 characters are subtracted).
-const CMD_SIZE_MAX: usize = ASSUAN_LINELENGTH / 2 - 20;
+///
+/// In concrete terms, this limit means that some commands (with big
+/// parameters) cannot be sent to the card, when the card doesn't support
+/// command chaining (like the floss-shop OpenPGP Card 3.4).
+///
+/// In particular, uploading rsa4096 keys fails via scdaemon, with such cards.
+const CMD_SIZE_MAX: usize = ASSUAN_LINELENGTH / 2 - 25;
 
 pub struct ScdClient {
     client: Client,
@@ -44,31 +53,9 @@ pub struct ScdClient {
 }
 
 impl ScdClient {
-    /// Create a CardBase object that uses an scdaemon instance as its
+    /// Initialize an ScdClient object that is connected to an scdaemon
-    /// backend.
+    /// instance via `socket`
-    pub fn open_scdc(socket: &str) -> Result<CardBase, OpenpgpCardError> {
+    fn new(socket: &str) -> Result<Self> {
-        let card_client = ScdClient::new(socket)?;
-        let card_client_box = Box::new(card_client) as CardClientBox;
-
-        CardBase::open_card(card_client_box)
-    }
-
-    /// Create a CardBase object that uses an scdaemon instance as its
-    /// backend, asking for a specific card by `serial`.
-    pub fn open_scdc_by_serial(
-        socket: &str,
-        serial: &str,
-    ) -> Result<CardBase, OpenpgpCardError> {
-        let mut card_client = ScdClient::new(socket)?;
-
-        card_client.select_card(serial)?;
-
-        let card_client_box = Box::new(card_client) as CardClientBox;
-
-        CardBase::open_card(card_client_box)
-    }
-
-    pub fn new(socket: &str) -> Result<Self> {
         let client = RT.lock().unwrap().block_on(Client::connect(socket))?;
         Ok(Self {
             client,
@@ -76,7 +63,29 @@ impl ScdClient {
         })
     }
 
-    pub fn select_card(&mut self, serial: &str) -> Result<()> {
+    /// Create a CardClientBox object that uses an scdaemon instance as its
+    /// backend. If multiple cards are available, scdaemon implicitly
+    /// selects one.
+    pub fn open(socket: &str) -> Result<CardClientBox, OpenpgpCardError> {
+        let card = ScdClient::new(socket)?;
+        Ok(Box::new(card) as CardClientBox)
+    }
+
+    /// Create a CardClientBox object that uses an scdaemon instance as its
+    /// backend. Requests the specific card `serial`.
+    pub fn open_by_serial(
+        socket: &str,
+        serial: &str,
+    ) -> Result<CardClientBox, OpenpgpCardError> {
+        let mut card = ScdClient::new(socket)?;
+        card.select_card(serial)?;
+
+        Ok(Box::new(card) as CardClientBox)
+    }
+
+    /// Ask scdameon to switch to using a specific OpenPGP card, based on
+    /// its `serial`.
+    fn select_card(&mut self, serial: &str) -> Result<()> {
         let send = format!("SERIALNO --demand={}\n", serial);
         self.client.send(send)?;
 
@@ -105,22 +114,20 @@ impl ScdClient {
 
 impl CardClient for ScdClient {
     fn transmit(&mut self, cmd: &[u8], _: usize) -> Result<Vec<u8>> {
-        log::debug!("SCDC cmd len {}", cmd.len());
+        log::trace!("SCDC cmd len {}", cmd.len());
 
         let hex = hex::encode(cmd);
 
-        let ex = if let Some(caps) = self.card_caps {
+        let ext = if self.card_caps.is_some()
-            if caps.ext_support {
+            && self.card_caps.unwrap().ext_support
-                format!("--exlen={} ", caps.max_rsp_bytes)
+        {
-            } else {
+            format!("--exlen={} ", self.card_caps.unwrap().max_rsp_bytes)
-                "".to_string()
-            }
         } else {
             "".to_string()
         };
 
-        let send = format!("APDU {}{}\n", ex, hex);
+        let send = format!("APDU {}{}\n", ext, hex);
-        log::debug!("send: '{}'", send);
+        log::debug!("SCDC command: '{}'", send);
 
         if send.len() > ASSUAN_LINELENGTH {
             return Err(anyhow!(
@@ -136,7 +143,10 @@ impl CardClient for ScdClient {
         while let Some(response) = rt.block_on(self.client.next()) {
             log::debug!("res: {:x?}", response);
             if response.is_err() {
-                unimplemented!("Err: {:x?}", response);
+                return Err(anyhow!(
+                    "Unexpected error response from SCD {:?}",
+                    response
+                ));
             }
 
             if let Ok(Response::Data { partial }) = response {
@@ -144,7 +154,7 @@ impl CardClient for ScdClient {
 
                 // drop remaining lines
                 while let Some(drop) = rt.block_on(self.client.next()) {
-                    log::debug!("drop: {:x?}", drop);
+                    log::trace!("drop: {:x?}", drop);
                 }
 
                 return Ok(res);