Merge branch 'nora/types' into 'main'

opgpcard: Make argument parsing more type safe

See merge request openpgp-card/openpgp-card!14

.gitlab-ci.yml

@@ -81,8 +81,8 @@ udeps:
     - apt update -y -qq
     - apt install -y -qq --no-install-recommends curl git clang make pkg-config nettle-dev libssl-dev capnproto ca-certificates libpcsclite-dev
     - apt clean
-    - curl --location --output /tmp/cargo-udeps.tar.gz https://github.com/est31/cargo-udeps/releases/download/v0.1.26/cargo-udeps-v0.1.26-x86_64-unknown-linux-gnu.tar.gz
+    - curl --location --output /tmp/cargo-udeps.tar.gz https://github.com/est31/cargo-udeps/releases/download/v0.1.33/cargo-udeps-v0.1.33-x86_64-unknown-linux-gnu.tar.gz
-    - tar --extract --verbose --gzip --file /tmp/cargo-udeps.tar.gz --directory /usr/local/bin/ --strip-components=2 ./cargo-udeps-v0.1.26-x86_64-unknown-linux-gnu/cargo-udeps
+    - tar --extract --verbose --gzip --file /tmp/cargo-udeps.tar.gz --directory /usr/local/bin/ --strip-components=2 ./cargo-udeps-v0.1.33-x86_64-unknown-linux-gnu/cargo-udeps
   script:
     - cargo udeps --workspace --all-features --all-targets
   cache: [ ]

tools/Cargo.toml

@@ -1,4 +1,5 @@
 # SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer <heiko@schaefer.name>
+# SPDX-FileCopyrightText: 2022 Nora Widdecke <mail@nora.pink>
 # SPDX-License-Identifier: MIT OR Apache-2.0
 
 [package]

tools/src/bin/opgpcard/cli.rs

@@ -1,7 +1,8 @@
 // SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer <heiko@schaefer.name>
+// SPDX-FileCopyrightText: 2022 Nora Widdecke <mail@nora.pink>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
-use clap::{AppSettings, Parser};
+use clap::{AppSettings, Parser, ValueEnum};
 use std::path::PathBuf;
 
 use crate::{OutputFormat, OutputVersion};
@@ -145,8 +146,8 @@ pub enum Command {
         #[clap(name = "card ident", short = 'c', long = "card")]
         ident: String,
 
-        #[clap(name = "identity")]
+        #[clap(name = "identity", value_enum)]
-        id: u8,
+        id: SetIdentityId,
     },
 }
 
@@ -187,15 +188,15 @@ pub enum AdminCommand {
         #[clap(name = "output", long = "output", short = 'o')]
         output: Option<PathBuf>,
 
-        #[clap(long = "no-decrypt")]
+        #[clap(long = "no-decrypt", action = clap::ArgAction::SetFalse)]
-        no_decrypt: bool,
+        decrypt: bool,
 
-        #[clap(long = "no-auth")]
+        #[clap(long = "no-auth", action = clap::ArgAction::SetFalse)]
-        no_auth: bool,
+        auth: bool,
 
-        /// Algorithm (rsa2048|rsa3072|rsa4096|nistp256|nistp384|nistp521|25519)
+        /// Algorithm
-        #[clap()]
+        #[clap(value_enum)]
-        algo: Option<String>,
+        algo: Option<AdminGenerateAlgo>,
 
         /// User ID to add to the exported certificate representation
         #[clap(name = "User ID", short = 'u', long = "userid")]
@@ -204,15 +205,11 @@ pub enum AdminCommand {
 
     /// Set touch policy
     Touch {
-        #[clap(name = "Key slot (SIG|DEC|AUT|ATT)", short = 'k', long = "key")]
+        #[clap(name = "Key slot", short = 'k', long = "key", value_enum)]
-        key: String,
+        key: BasePlusAttKeySlot,
 
-        #[clap(
+        #[clap(name = "Policy", short = 'p', long = "policy", value_enum)]
-            name = "Policy (Off|On|Fixed|Cached|Cached-Fixed)",
+        policy: TouchPolicy,
-            short = 'p',
-            long = "policy"
-        )]
-        policy: String,
     },
 }
 
@@ -277,8 +274,8 @@ pub enum AttCommand {
         #[clap(name = "card ident", short = 'c', long = "card")]
         ident: String,
 
-        #[clap(name = "Key slot (SIG|DEC|AUT)", short = 'k', long = "key")]
+        #[clap(name = "Key slot", short = 'k', long = "key", value_enum)]
-        key: String,
+        key: BaseKeySlot,
 
         #[clap(name = "User PIN file", short = 'p', long = "user-pin")]
         user_pin: Option<PathBuf>,
@@ -290,7 +287,122 @@ pub enum AttCommand {
         #[clap(name = "card ident", short = 'c', long = "card")]
         ident: Option<String>,
 
-        #[clap(name = "Key slot (SIG|DEC|AUT)", short = 'k', long = "key")]
+        #[clap(name = "Key slot", short = 'k', long = "key", value_enum)]
-        key: String,
+        key: BaseKeySlot,
     },
 }
+
+#[derive(ValueEnum, Debug, Clone)]
+#[clap(rename_all = "UPPER")]
+pub enum BaseKeySlot {
+    Sig,
+    Dec,
+    Aut,
+}
+
+impl From<BaseKeySlot> for openpgp_card_sequoia::types::KeyType {
+    fn from(ks: BaseKeySlot) -> Self {
+        use openpgp_card_sequoia::types::KeyType;
+        match ks {
+            BaseKeySlot::Sig => KeyType::Signing,
+            BaseKeySlot::Dec => KeyType::Decryption,
+            BaseKeySlot::Aut => KeyType::Authentication,
+        }
+    }
+}
+
+#[derive(ValueEnum, Debug, Clone)]
+#[clap(rename_all = "UPPER")]
+pub enum BasePlusAttKeySlot {
+    Sig,
+    Dec,
+    Aut,
+    Att,
+}
+
+impl From<BasePlusAttKeySlot> for openpgp_card_sequoia::types::KeyType {
+    fn from(ks: BasePlusAttKeySlot) -> Self {
+        use openpgp_card_sequoia::types::KeyType;
+        match ks {
+            BasePlusAttKeySlot::Sig => KeyType::Signing,
+            BasePlusAttKeySlot::Dec => KeyType::Decryption,
+            BasePlusAttKeySlot::Aut => KeyType::Authentication,
+            BasePlusAttKeySlot::Att => KeyType::Attestation,
+        }
+    }
+}
+
+#[derive(ValueEnum, Debug, Clone)]
+pub enum TouchPolicy {
+    #[clap(name = "Off")]
+    Off,
+    #[clap(name = "On")]
+    On,
+    #[clap(name = "Fixed")]
+    Fixed,
+    #[clap(name = "Cached")]
+    Cached,
+    #[clap(name = "Cached-Fixed")]
+    CachedFixed,
+}
+
+impl From<TouchPolicy> for openpgp_card_sequoia::types::TouchPolicy {
+    fn from(tp: TouchPolicy) -> Self {
+        use openpgp_card_sequoia::types::TouchPolicy as OCTouchPolicy;
+        match tp {
+            TouchPolicy::On => OCTouchPolicy::On,
+            TouchPolicy::Off => OCTouchPolicy::Off,
+            TouchPolicy::Fixed => OCTouchPolicy::Fixed,
+            TouchPolicy::Cached => OCTouchPolicy::Cached,
+            TouchPolicy::CachedFixed => OCTouchPolicy::CachedFixed,
+        }
+    }
+}
+
+#[derive(ValueEnum, Debug, Clone)]
+pub enum SetIdentityId {
+    #[clap(name = "0")]
+    Zero,
+    #[clap(name = "1")]
+    One,
+    #[clap(name = "2")]
+    Two,
+}
+
+impl From<SetIdentityId> for u8 {
+    fn from(id: SetIdentityId) -> Self {
+        match id {
+            SetIdentityId::Zero => 0,
+            SetIdentityId::One => 1,
+            SetIdentityId::Two => 2,
+        }
+    }
+}
+
+#[derive(ValueEnum, Debug, Clone)]
+#[clap(rename_all = "lower")]
+pub enum AdminGenerateAlgo {
+    Rsa2048,
+    Rsa3072,
+    Rsa4096,
+    Nistp256,
+    Nistp384,
+    Nistp521,
+    Curve25519,
+}
+
+impl From<AdminGenerateAlgo> for openpgp_card_sequoia::types::AlgoSimple {
+    fn from(aga: AdminGenerateAlgo) -> Self {
+        use openpgp_card_sequoia::types::AlgoSimple;
+
+        match aga {
+            AdminGenerateAlgo::Rsa2048 => AlgoSimple::RSA2k,
+            AdminGenerateAlgo::Rsa3072 => AlgoSimple::RSA3k,
+            AdminGenerateAlgo::Rsa4096 => AlgoSimple::RSA4k,
+            AdminGenerateAlgo::Nistp256 => AlgoSimple::NIST256,
+            AdminGenerateAlgo::Nistp384 => AlgoSimple::NIST384,
+            AdminGenerateAlgo::Nistp521 => AlgoSimple::NIST521,
+            AdminGenerateAlgo::Curve25519 => AlgoSimple::Curve25519,
+        }
+    }
+}

tools/src/bin/opgpcard/main.rs

@@ -1,8 +1,10 @@
 // SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer <heiko@schaefer.name>
+// SPDX-FileCopyrightText: 2022 Nora Widdecke <mail@nora.pink>
 // SPDX-License-Identifier: MIT OR Apache-2.0
 
 use anyhow::{anyhow, Result};
 use clap::Parser;
+use cli::BaseKeySlot;
 use std::path::{Path, PathBuf};
 
 use sequoia_openpgp::cert::prelude::ValidErasedKeyAmalgamation;
@@ -128,14 +130,7 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
 
                 let mut sign = util::verify_to_sign(&mut open, user_pin.as_deref())?;
 
-                let kt = match key.as_str() {
+                let kt = KeyType::from(key);
-                    "SIG" => KeyType::Signing,
-                    "DEC" => KeyType::Decryption,
-                    "AUT" => KeyType::Authentication,
-                    _ => {
-                        return Err(anyhow!("Unexpected Key Type {}", key).into());
-                    }
-                };
                 sign.generate_attestation(kt, &|| {
                     println!("Touch confirmation needed to generate an attestation")
                 })?;
@@ -160,13 +155,15 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
                 }
 
                 // Select cardholder certificate
-                match key.as_str() {
+                match key {
-                    "AUT" => open.select_data(0, &[0x7F, 0x21], select_data_workaround)?,
+                    BaseKeySlot::Aut => {
-                    "DEC" => open.select_data(1, &[0x7F, 0x21], select_data_workaround)?,
+                        open.select_data(0, &[0x7F, 0x21], select_data_workaround)?
-                    "SIG" => open.select_data(2, &[0x7F, 0x21], select_data_workaround)?,
+                    }
-
+                    BaseKeySlot::Dec => {
-                    _ => {
+                        open.select_data(1, &[0x7F, 0x21], select_data_workaround)?
-                        return Err(anyhow!("Unexpected Key Type {}", key).into());
+                    }
+                    BaseKeySlot::Sig => {
+                        open.select_data(2, &[0x7F, 0x21], select_data_workaround)?
                     }
                 };
 
@@ -316,8 +313,8 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
                 cli::AdminCommand::Generate {
                     user_pin,
                     output,
-                    no_decrypt,
+                    decrypt,
-                    no_auth,
+                    auth,
                     algo,
                     user_id,
                 } => {
@@ -330,32 +327,16 @@ fn main() -> Result<(), Box<dyn std::error::Error>> {
                         admin_pin.as_deref(),
                         user_pin.as_deref(),
                         output,
-                        !no_decrypt,
+                        decrypt,
-                        !no_auth,
+                        auth,
-                        algo,
+                        algo.map(AlgoSimple::from),
                         user_id,
                     )?;
                 }
                 cli::AdminCommand::Touch { key, policy } => {
-                    let kt = match key.as_str() {
+                    let kt = KeyType::from(key);
-                        "SIG" => KeyType::Signing,
+
-                        "DEC" => KeyType::Decryption,
+                    let pol = TouchPolicy::from(policy);
-                        "AUT" => KeyType::Authentication,
-                        "ATT" => KeyType::Attestation,
-                        _ => {
-                            return Err(anyhow!("Unexpected Key Type {}", key).into());
-                        }
-                    };
-                    let pol = match policy.as_str() {
-                        "Off" => TouchPolicy::Off,
-                        "On" => TouchPolicy::On,
-                        "Fixed" => TouchPolicy::Fixed,
-                        "Cached" => TouchPolicy::Cached,
-                        "Cached-Fixed" => TouchPolicy::CachedFixed,
-                        _ => {
-                            return Err(anyhow!("Unexpected Policy {}", policy).into());
-                        }
-                    };
 
                     let mut admin = util::verify_to_admin(&mut open, admin_pin.as_deref())?;
 
@@ -598,13 +579,12 @@ fn list_cards(format: OutputFormat, output_version: OutputVersion) -> Result<()>
     Ok(())
 }
 
-fn set_identity(ident: &str, id: u8) -> Result<(), Box<dyn std::error::Error>> {
+fn set_identity(ident: &str, id: cli::SetIdentityId) -> Result<(), Box<dyn std::error::Error>> {
     let backend = util::open_card(ident)?;
     let mut card = Card::new(backend);
     let mut open = card.transaction()?;
 
-    open.set_identity(id)?;
+    open.set_identity(u8::from(id))?;
-
     Ok(())
 }
 
@@ -1112,7 +1092,7 @@ fn generate_keys(
     output_file: Option<PathBuf>,
     decrypt: bool,
     auth: bool,
-    algo: Option<String>,
+    algo: Option<AlgoSimple>,
     user_ids: Vec<String>,
 ) -> Result<()> {
     let mut output = output::AdminGenerate::default();
@@ -1131,26 +1111,14 @@ fn generate_keys(
     // Because of this, for generation of RSA keys, here we take the approach
     // of first trying one variant, and then if that fails, try the other.
 
-    let a = match algo.as_deref() {
+    log::info!(" Key generation will be attempted with algo: {:?}", algo);
-        None => None,
+    output.algorithm(format!("{:?}", algo));
-        Some("rsa2048") => Some(AlgoSimple::RSA2k),
-        Some("rsa3072") => Some(AlgoSimple::RSA3k),
-        Some("rsa4096") => Some(AlgoSimple::RSA4k),
-        Some("nistp256") => Some(AlgoSimple::NIST256),
-        Some("nistp384") => Some(AlgoSimple::NIST384),
-        Some("nistp521") => Some(AlgoSimple::NIST521),
-        Some("25519") => Some(AlgoSimple::Curve25519),
-        _ => return Err(anyhow!("Unexpected algorithm")),
-    };
-
-    log::info!(" Key generation will be attempted with algo: {:?}", a);
-    output.algorithm(format!("{:?}", a));
 
     // 2) Then, generate keys on the card.
     // We need "admin" access to the card for this).
     let (key_sig, key_dec, key_aut) = {
         if let Ok(mut admin) = util::verify_to_admin(&mut open, admin_pin) {
-            gen_subkeys(&mut admin, decrypt, auth, a)?
+            gen_subkeys(&mut admin, decrypt, auth, algo)?
         } else {
             return Err(anyhow!("Failed to open card in admin mode."));
         }