Add documentation, normalize fn names.

card-functionality/src/tests.rs

@@ -168,7 +168,7 @@ pub fn test_print_algo_info(
 
     println!();
 
-    let algo = ca.list_supported_algo();
+    let algo = ca.get_algo_info();
     if let Ok(Some(algo)) = algo {
         println!("Card algorithm list:\n{}", algo);
     }

card-functionality/src/util.rs

@@ -50,7 +50,7 @@ pub(crate) fn upload_subkeys(
 
         // upload key
         let cuk = vka_as_uploadable_key(vka, None);
-        ca.upload_key(cuk, *kt)?;
+        ca.key_import(cuk, *kt)?;
     }
 
     Ok(out)

openpgp-card-sequoia/src/lib.rs

@@ -706,7 +706,7 @@ impl CardBase {
             return Ok(None);
         }
 
-        self.card_app.list_supported_algo()
+        self.card_app.get_algo_info()
     }
 
     // ----------
@@ -838,7 +838,7 @@ impl CardSign {
         &mut self,
         data: Vec<u8>,
     ) -> Result<Vec<u8>, OpenpgpCardError> {
-        self.card_app.compute_digital_signature(data)
+        self.card_app.pso_compute_digital_signature(data)
     }
 }
 
@@ -918,6 +918,6 @@ impl CardAdmin {
         key: Box<dyn CardUploadableKey>,
         key_type: KeyType,
     ) -> Result<(), OpenpgpCardError> {
-        self.card_app.upload_key(key, key_type)
+        self.card_app.key_import(key, key_type)
     }
 }

openpgp-card/src/algorithm.rs

@@ -134,7 +134,7 @@ impl AlgoSimple {
     }
 }
 
-/// Algorithm Information
+/// 4.4.3.11 Algorithm Information
 ///
 /// Modern cards (since OpenPGP card v3.4) provide a list of supported
 /// algorithms for each key type. This list specifies which "Algorithm
@@ -142,7 +142,7 @@ impl AlgoSimple {
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct AlgoInfo(pub(crate) Vec<(KeyType, Algo)>);
 
-/// Algorithm Attributes
+/// 4.4.3.9 Algorithm Attributes
 ///
 /// An `Algo` describes the algorithm settings for a key on the card.
 ///

openpgp-card/src/apdu/command.rs

@@ -1,6 +1,9 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 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;
 

openpgp-card/src/apdu/commands.rs

@@ -1,11 +1,12 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-//! Commands for the OpenPGP card application
+//! Pre-defined `Command` values for the OpenPGP card application
 
 use crate::apdu::command::Command;
 
-/// Select the OpenPGP application
+/// 7.2.1 SELECT
+/// (select the OpenPGP application on the card)
 pub(crate) fn select_openpgp() -> Command {
     Command::new(
         0x00,
@@ -90,16 +91,6 @@ pub(crate) fn verify_pw3(pin: Vec<u8>) -> Command {
     Command::new(0x00, 0x20, 0x00, 0x83, pin)
 }
 
-/// TERMINATE DF
-pub(crate) fn terminate_df() -> Command {
-    Command::new(0x00, 0xe6, 0x00, 0x00, vec![])
-}
-
-/// ACTIVATE FILE
-pub(crate) fn activate_file() -> Command {
-    Command::new(0x00, 0x44, 0x00, 0x00, vec![])
-}
-
 /// 7.2.8 PUT DATA,
 /// ('tag' must consist of either one or two bytes)
 pub(crate) fn put_data(tag: &[u8], data: Vec<u8>) -> Command {
@@ -162,30 +153,40 @@ pub(crate) fn change_pw3(oldpin: Vec<u8>, newpin: Vec<u8>) -> Command {
     Command::new(0x00, 0x24, 0x00, 0x83, fullpin)
 }
 
-/// Creates new APDU for decryption operation
+/// 7.2.10 PSO: COMPUTE DIGITAL SIGNATURE
-pub(crate) fn decryption(data: Vec<u8>) -> Command {
-    Command::new(0x00, 0x2A, 0x80, 0x86, data)
-}
-
-/// Creates new APDU for decryption operation
 pub(crate) fn signature(data: Vec<u8>) -> Command {
     Command::new(0x00, 0x2A, 0x9e, 0x9a, data)
 }
 
-/// Creates new APDU for "GENERATE ASYMMETRIC KEY PAIR"
+/// 7.2.11 PSO: DECIPHER (decryption)
+pub(crate) fn decryption(data: Vec<u8>) -> Command {
+    Command::new(0x00, 0x2A, 0x80, 0x86, data)
+}
+
+/// 7.2.14 GENERATE ASYMMETRIC KEY PAIR
 pub(crate) fn gen_key(data: Vec<u8>) -> Command {
     Command::new(0x00, 0x47, 0x80, 0x00, data)
 }
 
-/// Creates new APDU for "Reading of public key template"
+/// Read public key template (see 7.2.14)
 pub(crate) fn get_pub_key(data: Vec<u8>) -> Command {
     Command::new(0x00, 0x47, 0x81, 0x00, data)
 }
 
-/// Creates new APDU for key import
+/// key import (see 4.4.3.12, 7.2.8)
 pub(crate) fn key_import(data: Vec<u8>) -> Command {
     // The key import uses a PUT DATA command with odd INS (DB) and an
     // Extended header list (DO 4D) as described in ISO 7816-8
 
     Command::new(0x00, 0xDB, 0x3F, 0xFF, data)
 }
+
+/// 7.2.16 TERMINATE DF
+pub(crate) fn terminate_df() -> Command {
+    Command::new(0x00, 0xe6, 0x00, 0x00, vec![])
+}
+
+/// 7.2.17 ACTIVATE FILE
+pub(crate) fn activate_file() -> Command {
+    Command::new(0x00, 0x44, 0x00, 0x00, vec![])
+}

openpgp-card/src/apdu/mod.rs

@@ -1,7 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-//! Commands and responses to commands ("Application Protocol Data Unit")
+//! APDU "Application Protocol Data Unit"
+//! Commands and responses to commands
 
 pub(crate) mod command;
 pub(crate) mod commands;
@@ -69,7 +70,7 @@ pub(crate) fn send_command(
 /// return the response as a vector of `u8`.
 ///
 /// If the response is chained, this fn only returns one chunk, the caller
-/// needs take care of chained responses
+/// needs to re-assemble the chained response-parts.
 fn send_command_low_level(
     card_client: &mut CardClientBox,
     cmd: Command,

openpgp-card/src/apdu/response.rs

@@ -24,8 +24,8 @@ impl Response {
 /// "Raw" APDU Response, including the status bytes.
 ///
 /// This type is used for processing inside the openpgp-card crate
-/// (raw responses with a non-ok status sometimes need to be processed e.g.
+/// (raw responses with a non-ok status sometimes need to be processed,
-/// when a response is sent from the card in "chained" format).
+/// e.g. when a card sends a response in "chained" format).
 #[allow(unused)]
 #[derive(Clone, Debug)]
 pub(crate) struct RawResponse {

openpgp-card/src/card_app.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! CardApp exposes functionality of the "OpenPGP card" application.
+
 use std::borrow::BorrowMut;
 use std::convert::TryFrom;
 use std::convert::TryInto;
@@ -17,12 +19,12 @@ use crate::crypto_data::{
     CardUploadableKey, Cryptogram, EccType, Hash, PublicKeyMaterial,
 };
 use crate::errors::OpenpgpCardError;
-use crate::tlv::{tag::Tag, Tlv, Value};
+use crate::tlv::{tag::Tag, value::Value, Tlv};
 use crate::{apdu, keys, CardCaps, CardClientBox, KeyType};
 
 /// Low-level access to OpenPGP card functionality.
 ///
-/// No checks are performed here (e.g. for valid data lengths).
+/// Not many checks are performed here (e.g. for valid data lengths).
 /// Such checks should be performed on a higher layer, if needed.
 ///
 /// Also, no caching of data is done here. If necessary, caching should
@@ -210,7 +212,7 @@ impl CardApp {
     }
 
     /// DO "Algorithm Information" (0xFA)
-    pub fn list_supported_algo(&mut self) -> Result<Option<AlgoInfo>> {
+    pub fn get_algo_info(&mut self) -> Result<Option<AlgoInfo>> {
         let resp = apdu::send_command(
             &mut self.card_client,
             commands::get_algo_list(),
@@ -222,6 +224,9 @@ impl CardApp {
         Ok(Some(ai))
     }
 
+    /// 7.2.5 SELECT DATA
+    /// "select a DO in the current template"
+    /// (e.g. for cardholder certificate)
     pub fn select_data(
         &mut self,
         num: u8,
@@ -240,7 +245,14 @@ impl CardApp {
 
     // ----------
 
-    /// Delete all state on this OpenPGP card
+    /// Reset all state on this OpenPGP card.
+    ///
+    /// Note: the "factory reset" operation is not directly offered by the
+    /// card. It is composed of a series of steps:
+    /// - send 4 bad requests to verify pw1
+    /// - send 4 bad requests to verify pw3
+    /// - terminate_df
+    /// - activate_file
     pub fn factory_reset(&mut self) -> Result<()> {
         // send 4 bad requests to verify pw1
         // [apdu 00 20 00 81 08 40 40 40 40 40 40 40 40]
@@ -285,6 +297,12 @@ impl CardApp {
         Ok(())
     }
 
+    /// Verify pw1 (user) for signing operation (mode 81) and set an
+    /// appropriate access status.
+    ///
+    /// Depending on the PW1 status byte (see Extended Capabilities) this
+    /// access condition is only valid for one PSO:CDS command or remains
+    /// valid for several attempts.
     pub fn verify_pw1_for_signing(
         &mut self,
         pin: &str,
@@ -295,11 +313,21 @@ impl CardApp {
         apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
-    pub fn check_pw1(&mut self) -> Result<Response, OpenpgpCardError> {
+    /// Check the current access of PW1 for signing (mode 81).
-        let verify = commands::verify_pw1_82(vec![]);
+    ///
+    /// If verification is not required, an empty Ok Response is returned.
+    ///
+    /// (Note: some cards don't correctly implement this feature,
+    /// e.g. yubikey 5)
+    pub fn check_pw1_for_signing(
+        &mut self,
+    ) -> Result<Response, OpenpgpCardError> {
+        let verify = commands::verify_pw1_81(vec![]);
         apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
+    /// Verify PW1 (user) and set an appropriate access status.
+    /// (For operations except signing, mode 82).
     pub fn verify_pw1(
         &mut self,
         pin: &str,
@@ -310,11 +338,19 @@ impl CardApp {
         apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
-    pub fn check_pw3(&mut self) -> Result<Response, OpenpgpCardError> {
+    /// Check the current access of PW1.
-        let verify = commands::verify_pw3(vec![]);
+    /// (For operations except signing, mode 82).
+    ///
+    /// If verification is not required, an empty Ok Response is returned.
+    ///
+    /// (Note: some cards don't correctly implement this feature,
+    /// e.g. yubikey 5)
+    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)?.try_into()
     }
 
+    /// Verify PW3 (admin) and set an appropriate access status.
     pub fn verify_pw3(
         &mut self,
         pin: &str,
@@ -325,9 +361,23 @@ impl CardApp {
         apdu::send_command(&mut self.card_client, verify, false)?.try_into()
     }
 
+    /// Check the current access of PW3 (admin).
+    ///
+    /// If verification is not required, an empty Ok Response is returned.
+    ///
+    /// (Note: some cards don't correctly implement this feature,
+    /// e.g. yubikey 5)
+    pub fn check_pw3(&mut self) -> Result<Response, OpenpgpCardError> {
+        let verify = commands::verify_pw3(vec![]);
+        apdu::send_command(&mut self.card_client, verify, false)?.try_into()
+    }
+
     // --- decrypt ---
 
     /// Decrypt the ciphertext in `dm`, on the card.
+    ///
+    /// This is a convenience wrapper around `pso_decipher()` which builds
+    /// the required `data` field from `dm`.
     pub fn decrypt(
         &mut self,
         dm: Cryptogram,
@@ -372,6 +422,9 @@ impl CardApp {
     // --- sign ---
 
     /// Sign `hash`, on the card.
+    ///
+    /// This is a convenience wrapper around `pso_compute_digital_signature()`
+    /// which builds the required `data` field from `dm`.
     pub fn signature_for_hash(
         &mut self,
         hash: Hash,
@@ -402,12 +455,12 @@ impl CardApp {
             Hash::ECDSA(d) => d.to_vec(),
         };
 
-        self.compute_digital_signature(data)
+        self.pso_compute_digital_signature(data)
     }
 
     /// Run signing operation on the smartcard
     /// (7.2.10 PSO: COMPUTE DIGITAL SIGNATURE)
-    pub fn compute_digital_signature(
+    pub fn pso_compute_digital_signature(
         &mut self,
         data: Vec<u8>,
     ) -> Result<Vec<u8>, OpenpgpCardError> {
@@ -536,7 +589,8 @@ impl CardApp {
         apdu::send_command(&mut self.card_client, cmd, false)?.try_into()
     }
 
-    /// Set algorithm attributes [4.4.3.9 Algorithm Attributes]
+    /// Set algorithm attributes
+    /// (4.4.3.9 Algorithm Attributes)
     pub fn set_algorithm_attributes(
         &mut self,
         key_type: KeyType,
@@ -545,6 +599,9 @@ impl CardApp {
         // FIXME: caching?
         let ard = self.get_app_data()?;
 
+        // FIXME: Only write algo attributes to the card if "extended
+        // capabilities" show that they are changeable!
+
         // FIXME: reuse "e" from card, if no algo list is available
         let _cur_algo = ard.get_algorithm_attributes(key_type)?;
 
@@ -562,6 +619,7 @@ impl CardApp {
         apdu::send_command(&mut self.card_client, cmd, false)?.try_into()
     }
 
+    /// Helper: generate `data` for algorithm attributes with RSA
     fn rsa_algo_attrs(algo_attrs: &RsaAttrs) -> Result<Vec<u8>> {
         // Algorithm ID (01 = RSA (Encrypt or Sign))
         let mut algo_attributes = vec![0x01];
@@ -586,6 +644,7 @@ impl CardApp {
         Ok(algo_attributes)
     }
 
+    /// Helper: generate `data` for algorithm attributes with ECC
     fn ecc_algo_attrs(oid: &[u8], ecc_type: EccType) -> Vec<u8> {
         let algo_id = match ecc_type {
             EccType::EdDSA => 0x16,
@@ -600,23 +659,24 @@ impl CardApp {
         algo_attributes
     }
 
-    /// Upload an existing private key to the card.
+    /// Import an existing private key to the card.
     /// (This implicitly sets the algorithm info, fingerprint and timestamp)
-    pub fn upload_key(
+    pub fn key_import(
         &mut self,
         key: Box<dyn CardUploadableKey>,
         key_type: KeyType,
     ) -> Result<(), OpenpgpCardError> {
-        let algo_list = self.list_supported_algo();
+        let algo_list = self.get_algo_info();
 
         // An error is ok - it's fine if a card doesn't offer a list of
         // supported algorithms
         let algo_list = algo_list.unwrap_or(None);
 
-        keys::upload_key(self, key, key_type, algo_list)
+        keys::key_import(self, key, key_type, algo_list)
     }
 
     /// Generate a key on the card.
+    /// (7.2.14 GENERATE ASYMMETRIC KEY PAIR)
     ///
     /// If the `algo` parameter is Some, then this algorithm will be set on
     /// the card for "key_type".
@@ -634,7 +694,10 @@ impl CardApp {
     }
 
     /// Generate a key on the card.
-    /// Use a simplified algo selector enum.
+    /// (7.2.14 GENERATE ASYMMETRIC KEY PAIR)
+    ///
+    /// This is a convenience wrapper around generate_key() which allows
+    /// using the simplified `AlgoSimple` algorithm selector enum.
     pub fn generate_key_simple(
         &mut self,
         fp_from_pub: fn(
@@ -649,6 +712,12 @@ impl CardApp {
         self.generate_key(fp_from_pub, key_type, Some(&algo))
     }
 
+    /// Get public key material from the card.
+    ///
+    /// Note: this fn returns an uninterpreted set of raw values (not an
+    /// OpenPGP key data structure).
+    /// This data from the card is insufficient to create a typical
+    /// full public key.
     pub fn get_pub_key(
         &mut self,
         key_type: KeyType,

openpgp-card/src/card_do/algo_attrs.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 4.4.3.9 Algorithm Attributes
+
 use std::convert::TryFrom;
 
 use anyhow::Result;

openpgp-card/src/card_do/algo_info.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 4.4.3.11 Algorithm Information
+
 use std::convert::TryFrom;
 
 use anyhow::Result;

openpgp-card/src/card_do/application_id.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 4.2.1 Application Identifier (AID)
+
 use anyhow::Result;
 use nom::{bytes::complete as bytes, number::complete as number};
 use std::convert::TryFrom;

openpgp-card/src/card_do/cardholder.rs

@@ -1,12 +1,14 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Cardholder Related Data (see spec pg. 22)
+
 use std::convert::TryFrom;
 
 use anyhow::Result;
 
 use crate::card_do::{Cardholder, Sex};
-use crate::tlv::{Tlv, Value};
+use crate::tlv::{value::Value, Tlv};
 
 impl Cardholder {
     pub fn name(&self) -> Option<&str> {

openpgp-card/src/card_do/extended_cap.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 4.4.3.7 Extended Capabilities
+
 use anyhow::Result;
 use nom::{combinator, number::complete as number, sequence};
 use std::collections::HashSet;

openpgp-card/src/card_do/extended_length_info.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 4.1.3.1 Extended length information
+
 use anyhow::Result;
 use nom::{bytes::complete::tag, number::complete as number, sequence};
 

openpgp-card/src/card_do/fingerprint.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Fingerprint for a single key slot
+
 use anyhow::anyhow;
 use nom::{bytes::complete as bytes, combinator, sequence};
 use std::convert::TryFrom;

openpgp-card/src/card_do/historical.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! 6 Historical Bytes
+
 use crate::card_do::{CardCapabilities, CardServiceData, Historical};
 use crate::errors::OpenpgpCardError;
 use anyhow::{anyhow, Result};

openpgp-card/src/card_do/key_generation_times.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Generation date/time of key pair (see spec pg. 24)
+
 use anyhow::anyhow;
 use chrono::{DateTime, NaiveDateTime, Utc};
 use nom::{combinator, number::complete as number, sequence};

openpgp-card/src/card_do/mod.rs

@@ -24,14 +24,14 @@ mod historical;
 mod key_generation_times;
 mod pw_status;
 
-/// Application Related Data
+/// 4.4.3.1 Application Related Data
 ///
 /// The "application related data" DO contains a set of DOs.
 /// This struct offers read access to these DOs.
 ///
-/// Note that when any of the information in this DO changes on the card, you
+/// (Note: when any of the information in this DO changes on the card, you
-/// need to read ApplicationRelatedData from the card again to receive the
+/// need to re-read ApplicationRelatedData from the card to receive the
-/// current values.
+/// new values!)
 pub struct ApplicationRelatedData(pub(crate) Tlv);
 
 impl ApplicationRelatedData {
@@ -171,6 +171,7 @@ impl ApplicationRelatedData {
     }
 }
 
+/// Security support template (see spec pg. 24)
 #[derive(Debug)]
 pub struct SecuritySupportTemplate {
     // Digital signature counter [3 bytes]
@@ -184,7 +185,7 @@ impl SecuritySupportTemplate {
     }
 }
 
-/// An OpenPGP key generation Time
+/// An OpenPGP key generation Time (see spec pg. 24)
 #[derive(Clone, Copy, Eq, PartialEq, Debug)]
 pub struct KeyGenerationTime(u32);
 
@@ -194,7 +195,7 @@ impl KeyGenerationTime {
     }
 }
 
-/// Application identifier (AID)
+/// 4.2.1 Application Identifier (AID)
 #[derive(Debug, Eq, PartialEq)]
 pub struct ApplicationId {
     application: u8,
@@ -203,26 +204,7 @@ pub struct ApplicationId {
     serial: u32,
 }
 
-/// Card Capabilities (73)
+/// 6 Historical Bytes
-#[derive(Debug, PartialEq)]
-pub struct CardCapabilities {
-    command_chaining: bool,
-    extended_lc_le: bool,
-    extended_length_information: bool,
-}
-
-/// Card service data (31)
-#[derive(Debug, PartialEq)]
-pub struct CardServiceData {
-    select_by_full_df_name: bool,
-    select_by_partial_df_name: bool,
-    dos_available_in_ef_dir: bool,
-    dos_available_in_ef_atr_info: bool,
-    access_services: [bool; 3],
-    mf: bool,
-}
-
-/// Historical Bytes
 #[derive(Debug, PartialEq)]
 pub struct Historical {
     /// category indicator byte
@@ -238,7 +220,26 @@ pub struct Historical {
     sib: u8,
 }
 
-/// Extended Capabilities
+/// Card Capabilities (see 6 Historical Bytes)
+#[derive(Debug, PartialEq)]
+pub struct CardCapabilities {
+    command_chaining: bool,
+    extended_lc_le: bool,
+    extended_length_information: bool,
+}
+
+/// Card service data (see 6 Historical Bytes
+#[derive(Debug, PartialEq)]
+pub struct CardServiceData {
+    select_by_full_df_name: bool,
+    select_by_partial_df_name: bool,
+    dos_available_in_ef_dir: bool,
+    dos_available_in_ef_atr_info: bool,
+    access_services: [bool; 3],
+    mf: bool,
+}
+
+/// 4.4.3.7 Extended Capabilities
 #[derive(Debug, Eq, PartialEq)]
 pub struct ExtendedCap {
     features: HashSet<Features>,
@@ -250,7 +251,7 @@ pub struct ExtendedCap {
     mse_command_support: bool,
 }
 
-/// Features (first byte of Extended Capabilities)
+/// Features (first byte of Extended Capabilities, see 4.4.3.7)
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 pub enum Features {
     SecureMessaging,
@@ -263,14 +264,14 @@ pub enum Features {
     KdfDo,
 }
 
-/// Extended length information
+/// 4.1.3.1 Extended length information
 #[derive(Debug, Eq, PartialEq)]
 pub struct ExtendedLengthInfo {
     max_command_bytes: u16,
     max_response_bytes: u16,
 }
 
-/// Cardholder Related Data
+/// Cardholder Related Data (see spec pg. 22)
 #[derive(Debug)]
 pub struct Cardholder {
     name: Option<String>,
@@ -278,7 +279,7 @@ pub struct Cardholder {
     sex: Option<Sex>,
 }
 
-/// Sex (according to ISO 5218)
+/// 4.4.3.5 Sex (according to ISO 5218)
 #[derive(Debug, PartialEq, Clone, Copy)]
 pub enum Sex {
     NotKnown,
@@ -309,7 +310,7 @@ impl From<u8> for Sex {
     }
 }
 
-/// PW status Bytes
+/// PW status Bytes (see spec page 23)
 #[derive(Debug)]
 pub struct PWStatus {
     pub(crate) pw1_cds_multi: bool,
@@ -335,7 +336,7 @@ impl PWStatus {
     }
 }
 
-/// Fingerprint
+/// Fingerprint (see spec pg. 23)
 #[derive(Clone, Eq, PartialEq)]
 pub struct Fingerprint([u8; 20]);
 

openpgp-card/src/card_do/pw_status.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! PW status Bytes (see spec page 23)
+
 use anyhow::anyhow;
 
 use crate::card_do::PWStatus;

openpgp-card/src/crypto_data.rs

@@ -16,8 +16,8 @@ pub enum Hash<'a> {
     SHA256([u8; 0x20]),
     SHA384([u8; 0x30]),
     SHA512([u8; 0x40]),
-    EdDSA(&'a [u8]), // FIXME?
     ECDSA(&'a [u8]), // FIXME?
+    EdDSA(&'a [u8]), // FIXME?
 }
 
 impl Hash<'_> {
@@ -153,6 +153,6 @@ impl EccPub {
 #[derive(PartialEq, Eq, Debug, Clone, Copy)]
 pub enum EccType {
     ECDH,
-    EdDSA,
     ECDSA,
+    EdDSA,
 }

openpgp-card/src/keys.rs

@@ -17,11 +17,19 @@ use crate::crypto_data::{
     RSAKey, RSAPub,
 };
 use crate::errors::OpenpgpCardError;
-use crate::tlv::{Tlv, Value};
+use crate::tlv::{length::tlv_encode_length, value::Value, Tlv};
-use crate::{apdu, tlv, KeyType};
+use crate::{apdu, KeyType};
 
-/// `gen_key_with_metadata` calculates the fingerprint for a public key
+/// Generate asymmetric key pair on the card.
-/// data object
+///
+/// This is a convenience wrapper around gen_key() that:
+/// - sets algorithm attributes (if not None)
+/// - generates a key pair on the card
+/// - sets the creation time on the card to the current host time
+/// - calculates fingerprint for the key and sets it on the card
+///
+/// `fp_from_pub` calculates the fingerprint for a public key data object and
+/// creation timestamp
 pub(crate) fn gen_key_with_metadata(
     card_app: &mut CardApp,
     fp_from_pub: fn(
@@ -42,7 +50,7 @@ pub(crate) fn gen_key_with_metadata(
     let algo = ard.get_algorithm_attributes(key_type)?;
 
     // generate key
-    let tlv = gen_key(card_app, key_type)?;
+    let tlv = generate_asymmetric_key_pair(card_app, key_type)?;
 
     // derive pubkey
     let pubkey = tlv_to_pubkey(&tlv, &algo)?;
@@ -69,6 +77,7 @@ pub(crate) fn gen_key_with_metadata(
     Ok((pubkey, ts))
 }
 
+/// Transform a public key Tlv from the card into PublicKeyMaterial
 fn tlv_to_pubkey(tlv: &Tlv, algo: &Algo) -> Result<PublicKeyMaterial> {
     let n = tlv.find(&[0x81].into());
     let v = tlv.find(&[0x82].into());
@@ -96,7 +105,11 @@ fn tlv_to_pubkey(tlv: &Tlv, algo: &Algo) -> Result<PublicKeyMaterial> {
     }
 }
 
-pub(crate) fn gen_key(
+/// 7.2.14 GENERATE ASYMMETRIC KEY PAIR
+///
+/// This runs the low level key generation primitive on the card.
+/// (This does not set algorithm attributes, creation time or fingerprint)
+pub(crate) fn generate_asymmetric_key_pair(
     card_app: &mut CardApp,
     key_type: KeyType,
 ) -> Result<Tlv, OpenpgpCardError> {
@@ -114,12 +127,16 @@ pub(crate) fn gen_key(
     Ok(tlv)
 }
 
+/// Get the public key material for a key from the card.
+///
+/// ("Returns the public key of an asymmetric key pair previously generated
+/// in the card or imported")
+///
+/// (See 7.2.14 GENERATE ASYMMETRIC KEY PAIR)
 pub(crate) fn get_pub_key(
     card_app: &mut CardApp,
     key_type: KeyType,
 ) -> Result<PublicKeyMaterial, OpenpgpCardError> {
-    println!("get pub key for {:?}", key_type);
-
     // algo
     let ard = card_app.get_app_data()?; // FIXME: caching
     let algo = ard.get_algorithm_attributes(key_type)?;
@@ -137,10 +154,10 @@ pub(crate) fn get_pub_key(
     Ok(pubkey)
 }
 
-/// Upload an explicitly selected Key to the card as a specific KeyType.
+/// Import private key material to the card as a specific KeyType.
 ///
 /// The client needs to make sure that the key is suitable for `key_type`.
-pub(crate) fn upload_key(
+pub(crate) fn key_import(
     card_app: &mut CardApp,
     key: Box<dyn CardUploadableKey>,
     key_type: KeyType,
@@ -188,7 +205,7 @@ pub(crate) fn upload_key(
                 }
             };
 
-            let key_cmd = rsa_key_cmd(key_type, rsa_key, &rsa_attrs)?;
+            let key_cmd = rsa_key_import_cmd(key_type, rsa_key, &rsa_attrs)?;
 
             (Algo::Rsa(rsa_attrs), key_cmd)
         }
@@ -220,25 +237,25 @@ pub(crate) fn upload_key(
                 None,
             ));
 
-            let key_cmd = ecc_key_cmd(ecc_key, key_type)?;
+            let key_cmd = ecc_key_import_cmd(ecc_key, key_type)?;
 
             (algo, key_cmd)
         }
     };
 
-    copy_key_to_card(
+    let ts = key.get_ts();
-        card_app,
+    let fp = key.get_fp()?;
-        key_type,
+
-        key.get_ts(),
+    // Send all the commands
-        key.get_fp()?,
+    card_app.set_algorithm_attributes(key_type, &algo)?;
-        &algo,
+    apdu::send_command(card_app.card(), key_cmd, false)?.check_ok()?;
-        key_cmd,
+    card_app.set_fingerprint(fp, key_type)?;
-    )?;
+    card_app.set_creation_time(ts, key_type)?;
 
     Ok(())
 }
 
-// Look up RsaAttrs parameters in algo_list based on key_type and rsa_bits
+/// Look up RsaAttrs parameters in algo_list based on key_type and rsa_bits
 fn get_card_algo_rsa(
     algo_list: AlgoInfo,
     key_type: KeyType,
@@ -276,7 +293,7 @@ fn get_card_algo_rsa(
     }
 }
 
-// Check if `oid` is supported for `key_type` in algo_list.
+/// Check if `oid` is supported for `key_type` in algo_list.
 fn check_card_algo_ecc(
     algo_list: AlgoInfo,
     key_type: KeyType,
@@ -300,7 +317,8 @@ fn check_card_algo_ecc(
     ecc_algos.iter().any(|e| e.oid() == oid)
 }
 
-fn ecc_key_cmd(
+/// Create command for ECC key import
+fn ecc_key_import_cmd(
     ecc_key: Box<dyn EccKey>,
     key_type: KeyType,
 ) -> Result<Command, OpenpgpCardError> {
@@ -323,22 +341,8 @@ fn ecc_key_cmd(
     Ok(commands::key_import(ehl.serialize().to_vec()))
 }
 
-fn get_crt(key_type: KeyType) -> Result<Tlv, OpenpgpCardError> {
+/// Create command for RSA key import
-    // "Control Reference Template" (0xB8 | 0xB6 | 0xA4)
+fn rsa_key_import_cmd(
-    let tag = match key_type {
-        KeyType::Decryption => 0xB8,
-        KeyType::Signing => 0xB6,
-        KeyType::Authentication => 0xA4,
-        _ => {
-            return Err(OpenpgpCardError::InternalError(anyhow!(
-                "Unexpected KeyType"
-            )))
-        }
-    };
-    Ok(Tlv::new([tag], Value::S(vec![])))
-}
-
-fn rsa_key_cmd(
     key_type: KeyType,
     rsa_key: Box<dyn RSAKey>,
     algo_attrs: &RsaAttrs,
@@ -368,11 +372,11 @@ fn rsa_key_cmd(
 
     value.push(0x92);
     // len p in bytes, TLV-encoded
-    value.extend_from_slice(&tlv::tlv_encode_length(len_p_bytes));
+    value.extend_from_slice(&tlv_encode_length(len_p_bytes));
 
     value.push(0x93);
     // len q in bytes, TLV-encoded
-    value.extend_from_slice(&tlv::tlv_encode_length(len_q_bytes));
+    value.extend_from_slice(&tlv_encode_length(len_q_bytes));
 
     let cpkt = Tlv::new([0x7F, 0x48], Value::S(value));
 
@@ -404,25 +408,18 @@ fn rsa_key_cmd(
     Ok(commands::key_import(ehl.serialize().to_vec()))
 }
 
-fn copy_key_to_card(
+/// Get "Control Reference Template" Tlv for `key_type`
-    card_app: &mut CardApp,
+fn get_crt(key_type: KeyType) -> Result<Tlv, OpenpgpCardError> {
-    key_type: KeyType,
+    // "Control Reference Template" (0xB8 | 0xB6 | 0xA4)
-    ts: KeyGenerationTime,
+    let tag = match key_type {
-    fp: Fingerprint,
+        KeyType::Decryption => 0xB8,
-    algo: &Algo,
+        KeyType::Signing => 0xB6,
-    key_cmd: Command,
+        KeyType::Authentication => 0xA4,
-) -> Result<(), OpenpgpCardError> {
+        _ => {
-    // Send all the commands
+            return Err(OpenpgpCardError::InternalError(anyhow!(
-
+                "Unexpected KeyType"
-    // FIXME: Only write algo attributes to the card if "extended
+            )))
-    // capabilities" show that they are changeable!
+        }
-    card_app.set_algorithm_attributes(key_type, algo)?;
+    };
-
+    Ok(Tlv::new([tag], Value::S(vec![])))
-    apdu::send_command(card_app.card(), key_cmd, false)?.check_ok()?;
-
-    card_app.set_fingerprint(fp, key_type)?;
-
-    card_app.set_creation_time(ts, key_type)?;
-
-    Ok(())
 }

openpgp-card/src/tlv/length.rs

@@ -1,6 +1,19 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Length in a TLV data structure
+
+/// Helper fn to encode length fields in TLV structures (see spec 4.4.4)
+pub(crate) fn tlv_encode_length(len: u16) -> Vec<u8> {
+    if len > 255 {
+        vec![0x82, (len >> 8) as u8, (len & 255) as u8]
+    } else if len > 127 {
+        vec![0x81, len as u8]
+    } else {
+        vec![len as u8]
+    }
+}
+
 use nom::{
     branch, bytes::complete as bytes, combinator, number::complete as number,
     sequence,

openpgp-card/src/tlv/mod.rs

@@ -1,17 +1,21 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-pub mod length;
+pub(crate) mod length;
-pub mod tag;
+pub(crate) mod tag;
+pub(crate) mod value;
 
 use anyhow::Result;
 use nom::{bytes::complete as bytes, combinator};
 use std::convert::TryFrom;
 
 use crate::card_do::complete;
-use crate::tlv::tag::Tag;
+use crate::tlv::{length::tlv_encode_length, tag::Tag, value::Value};
 
-/// TLV (Tag-Length-Value)
+/// TLV (Tag-Length-Value) data structure.
+///
+/// Many DOs (data objects) on OpenPGP cards are stored in TLV format.
+/// This struct handles serializing and deserializing TLV.
 #[derive(Debug, Eq, PartialEq)]
 pub struct Tlv {
     tag: Tag,
@@ -42,7 +46,7 @@ impl Tlv {
 
     pub fn serialize(&self) -> Vec<u8> {
         let value = self.value.serialize();
-        let length = crate::tlv::tlv_encode_length(value.len() as u16);
+        let length = tlv_encode_length(value.len() as u16);
 
         let mut ser = Vec::new();
         ser.extend(self.tag.get().iter());
@@ -52,12 +56,16 @@ impl Tlv {
     }
 
     fn parse(input: &[u8]) -> nom::IResult<&[u8], Tlv> {
-        // read tag
+        // Read the tag byte(s)
         let (input, tag) = tag::tag(input)?;
 
+        // Read the length field and get the corresponding number of bytes,
+        // which contain the value of this tlv
         let (input, value) =
             combinator::flat_map(length::length, bytes::take)(input)?;
 
+        // Parse the value bytes, as "simple" or "constructed", depending
+        // on the tag.
         let (_, v) = Value::parse(value, tag.is_constructed())?;
 
         Ok((input, Self::new(tag, v)))
@@ -72,63 +80,6 @@ impl TryFrom<&[u8]> for Tlv {
     }
 }
 
-pub fn tlv_encode_length(len: u16) -> Vec<u8> {
-    if len > 255 {
-        vec![0x82, (len >> 8) as u8, (len & 255) as u8]
-    } else if len > 127 {
-        vec![0x81, len as u8]
-    } else {
-        vec![len as u8]
-    }
-}
-
-/// A TLV "value"
-#[derive(Debug, Eq, PartialEq)]
-pub enum Value {
-    /// A "constructed" value, consisting of a list of Tlv
-    C(Vec<Tlv>),
-
-    /// A "simple" value, consisting of binary data
-    S(Vec<u8>),
-}
-
-impl Value {
-    fn parse(data: &[u8], constructed: bool) -> nom::IResult<&[u8], Self> {
-        match constructed {
-            false => Ok((&[], Value::S(data.to_vec()))),
-            true => {
-                let mut c = vec![];
-                let mut input = data;
-
-                while !input.is_empty() {
-                    let (rest, tlv) = Tlv::parse(input)?;
-                    input = rest;
-                    c.push(tlv);
-                }
-
-                Ok((&[], Value::C(c)))
-            }
-        }
-    }
-
-    pub fn from(data: &[u8], constructed: bool) -> Result<Self> {
-        complete(Self::parse(data, constructed))
-    }
-
-    pub fn serialize(&self) -> Vec<u8> {
-        match self {
-            Value::S(data) => data.clone(),
-            Value::C(data) => {
-                let mut s = vec![];
-                for t in data {
-                    s.extend(&t.serialize());
-                }
-                s
-            }
-        }
-    }
-}
-
 #[cfg(test)]
 mod test {
     use anyhow::Result;
@@ -192,6 +143,10 @@ mod test {
         let data = hex!("6e8201374f10d27600012401030400061601918000005f520800730000e00590007f740381012073820110c00a7d000bfe080000ff0000c106010800001100c206010800001100c306010800001100da06010800001100c407ff7f7f7f030003c5500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000c6500000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000cd1000000000000000000000000000000000de0801000200030081027f660802020bfe02020bfed6020020d7020020d8020020d9020020");
         let tlv = Tlv::try_from(&data[..])?;
 
+        // check that after re-serializing, the data is still the same
+        let serialized = tlv.serialize();
+        assert_eq!(&serialized, &data[..]);
+
         // outermost layer contains all bytes as value
         let value = tlv.find(&[0x6e].into()).unwrap();
         assert_eq!(value.serialize(),

openpgp-card/src/tlv/tag.rs

@@ -1,6 +1,8 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Tag in a TLV data structure
+
 use nom::{
     branch, bytes::complete as bytes, combinator, number::complete as number,
     sequence,

openpgp-card/src/tlv/value.rs

@@ -1,20 +1,59 @@
 // SPDX-FileCopyrightText: 2021 Heiko Schaefer <heiko@schaefer.name>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
+//! Value in a TLV data structure
+
+use anyhow::Result;
+
+use crate::card_do::complete;
+use crate::tlv::Tlv;
+
+/// A TLV "value"
 #[derive(Debug, Eq, PartialEq)]
 pub enum Value {
-    // "Primitive (Simple) DO"
+    /// A "constructed" value, consisting of a list of Tlv
-    S(Vec<u8>),
+    C(Vec<Tlv>),
 
-    // "Constructed DO"
+    /// A "simple" value, consisting of binary data
-    C(Tlv),
+    S(Vec<u8>),
 }
 
 impl Value {
-    pub fn data(&self) -> &[u8] {
+    pub(crate) fn parse(
+        data: &[u8],
+        constructed: bool,
+    ) -> nom::IResult<&[u8], Self> {
+        match constructed {
+            false => Ok((&[], Value::S(data.to_vec()))),
+            true => {
+                let mut c = vec![];
+                let mut input = data;
+
+                while !input.is_empty() {
+                    let (rest, tlv) = Tlv::parse(input)?;
+                    input = rest;
+                    c.push(tlv);
+                }
+
+                Ok((&[], Value::C(c)))
+            }
+        }
+    }
+
+    pub fn from(data: &[u8], constructed: bool) -> Result<Self> {
+        complete(Self::parse(data, constructed))
+    }
+
+    pub fn serialize(&self) -> Vec<u8> {
         match self {
-            Value::S(v) => &v,
+            Value::S(data) => data.clone(),
-            Value::C(c) => &c.0,
+            Value::C(data) => {
+                let mut s = vec![];
+                for t in data {
+                    s.extend(&t.serialize());
+                }
+                s
+            }
         }
     }
 }