From 9dd4f3ab56548e1005201c8d3d3072389b5e0932 Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Mon, 24 Oct 2022 23:06:16 +0200 Subject: [PATCH 1/7] opgpcard: Make the KeySlots type safe --- tools/src/bin/opgpcard/cli.rs | 54 +++++++++++++++++++++++++++++----- tools/src/bin/opgpcard/main.rs | 37 ++++++++--------------- 2 files changed, 60 insertions(+), 31 deletions(-) diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index cf594ea..fe8d191 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -1,7 +1,7 @@ // SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer // 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}; @@ -204,8 +204,8 @@ pub enum AdminCommand { /// Set touch policy Touch { - #[clap(name = "Key slot (SIG|DEC|AUT|ATT)", short = 'k', long = "key")] - key: String, + #[clap(name = "Key slot", short = 'k', long = "key", value_enum)] + key: BasePlusAttKeySlot, #[clap( name = "Policy (Off|On|Fixed|Cached|Cached-Fixed)", @@ -277,8 +277,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")] - key: String, + #[clap(name = "Key slot", short = 'k', long = "key", value_enum)] + key: BaseKeySlot, #[clap(name = "User PIN file", short = 'p', long = "user-pin")] user_pin: Option, @@ -290,7 +290,47 @@ pub enum AttCommand { #[clap(name = "card ident", short = 'c', long = "card")] ident: Option, - #[clap(name = "Key slot (SIG|DEC|AUT)", short = 'k', long = "key")] - key: String, + #[clap(name = "Key slot", short = 'k', long = "key", value_enum)] + key: BaseKeySlot, }, } + +#[derive(ValueEnum, Debug, Clone)] +#[clap(rename_all = "UPPER")] +pub enum BaseKeySlot { + Sig, + Dec, + Aut, +} + +impl From 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 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, + } + } +} diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index b914697..868fe71 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -3,6 +3,7 @@ use anyhow::{anyhow, Result}; use clap::Parser; +use cli::BaseKeySlot; use std::path::{Path, PathBuf}; use sequoia_openpgp::cert::prelude::ValidErasedKeyAmalgamation; @@ -128,14 +129,7 @@ fn main() -> Result<(), Box> { let mut sign = util::verify_to_sign(&mut open, user_pin.as_deref())?; - let kt = match key.as_str() { - "SIG" => KeyType::Signing, - "DEC" => KeyType::Decryption, - "AUT" => KeyType::Authentication, - _ => { - return Err(anyhow!("Unexpected Key Type {}", key).into()); - } - }; + let kt = KeyType::from(key); sign.generate_attestation(kt, &|| { println!("Touch confirmation needed to generate an attestation") })?; @@ -160,13 +154,15 @@ fn main() -> Result<(), Box> { } // Select cardholder certificate - match key.as_str() { - "AUT" => open.select_data(0, &[0x7F, 0x21], select_data_workaround)?, - "DEC" => open.select_data(1, &[0x7F, 0x21], select_data_workaround)?, - "SIG" => open.select_data(2, &[0x7F, 0x21], select_data_workaround)?, - - _ => { - return Err(anyhow!("Unexpected Key Type {}", key).into()); + match key { + BaseKeySlot::Aut => { + open.select_data(0, &[0x7F, 0x21], select_data_workaround)? + } + BaseKeySlot::Dec => { + open.select_data(1, &[0x7F, 0x21], select_data_workaround)? + } + BaseKeySlot::Sig => { + open.select_data(2, &[0x7F, 0x21], select_data_workaround)? } }; @@ -337,15 +333,8 @@ fn main() -> Result<(), Box> { )?; } cli::AdminCommand::Touch { key, policy } => { - let kt = match key.as_str() { - "SIG" => KeyType::Signing, - "DEC" => KeyType::Decryption, - "AUT" => KeyType::Authentication, - "ATT" => KeyType::Attestation, - _ => { - return Err(anyhow!("Unexpected Key Type {}", key).into()); - } - }; + let kt = KeyType::from(key); + let pol = match policy.as_str() { "Off" => TouchPolicy::Off, "On" => TouchPolicy::On, From e9787dcbd318b4551a1c2b7603912cecf01251d5 Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Tue, 25 Oct 2022 00:06:33 +0200 Subject: [PATCH 2/7] opgpcard: Make TouchPolicy type safe --- tools/src/bin/opgpcard/cli.rs | 35 ++++++++++++++++++++++++++++------ tools/src/bin/opgpcard/main.rs | 11 +---------- 2 files changed, 30 insertions(+), 16 deletions(-) diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index fe8d191..bae7d92 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -207,12 +207,8 @@ pub enum AdminCommand { #[clap(name = "Key slot", short = 'k', long = "key", value_enum)] key: BasePlusAttKeySlot, - #[clap( - name = "Policy (Off|On|Fixed|Cached|Cached-Fixed)", - short = 'p', - long = "policy" - )] - policy: String, + #[clap(name = "Policy", short = 'p', long = "policy", value_enum)] + policy: TouchPolicy, }, } @@ -334,3 +330,30 @@ impl From for openpgp_card_sequoia::types::KeyType { } } } + +#[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 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, + } + } +} diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index 868fe71..815ed07 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -335,16 +335,7 @@ fn main() -> Result<(), Box> { cli::AdminCommand::Touch { key, policy } => { let kt = KeyType::from(key); - 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 pol = TouchPolicy::from(policy); let mut admin = util::verify_to_admin(&mut open, admin_pin.as_deref())?; From a7731ec467597bc1d219fa6a130cd3d086a374ac Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Tue, 25 Oct 2022 13:38:08 +0200 Subject: [PATCH 3/7] opgpcard: Add Nora to license headers --- tools/Cargo.toml | 1 + tools/src/bin/opgpcard/cli.rs | 1 + tools/src/bin/opgpcard/main.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/tools/Cargo.toml b/tools/Cargo.toml index 9cab082..7f8719b 100644 --- a/tools/Cargo.toml +++ b/tools/Cargo.toml @@ -1,4 +1,5 @@ # SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer +# SPDX-FileCopyrightText: 2022 Nora Widdecke # SPDX-License-Identifier: MIT OR Apache-2.0 [package] diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index bae7d92..ea348e7 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -1,4 +1,5 @@ // SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer +// SPDX-FileCopyrightText: 2022 Nora Widdecke // SPDX-License-Identifier: MIT OR Apache-2.0 use clap::{AppSettings, Parser, ValueEnum}; diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index 815ed07..84cd541 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -1,4 +1,5 @@ // SPDX-FileCopyrightText: 2021-2022 Heiko Schaefer +// SPDX-FileCopyrightText: 2022 Nora Widdecke // SPDX-License-Identifier: MIT OR Apache-2.0 use anyhow::{anyhow, Result}; From e81ebd21a0e6851990c5b0b14e24bac31df64042 Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Tue, 25 Oct 2022 00:28:47 +0200 Subject: [PATCH 4/7] opgpcard: Restrict values of id of set-identity --- tools/src/bin/opgpcard/cli.rs | 24 ++++++++++++++++++++++-- tools/src/bin/opgpcard/main.rs | 5 ++--- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index ea348e7..43a3d71 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -146,8 +146,8 @@ pub enum Command { #[clap(name = "card ident", short = 'c', long = "card")] ident: String, - #[clap(name = "identity")] - id: u8, + #[clap(name = "identity", value_enum)] + id: SetIdentityId, }, } @@ -358,3 +358,23 @@ impl From for openpgp_card_sequoia::types::TouchPolicy { } } } + +#[derive(ValueEnum, Debug, Clone)] +pub enum SetIdentityId { + #[clap(name = "0")] + Zero, + #[clap(name = "1")] + One, + #[clap(name = "2")] + Two, +} + +impl From for u8 { + fn from(id: SetIdentityId) -> Self { + match id { + SetIdentityId::Zero => 0, + SetIdentityId::One => 1, + SetIdentityId::Two => 2, + } + } +} diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index 84cd541..09d1809 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -579,13 +579,12 @@ fn list_cards(format: OutputFormat, output_version: OutputVersion) -> Result<()> Ok(()) } -fn set_identity(ident: &str, id: u8) -> Result<(), Box> { +fn set_identity(ident: &str, id: cli::SetIdentityId) -> Result<(), Box> { 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(()) } From 183476287938bc4820ec3ef8ba7f778555a2d466 Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Tue, 25 Oct 2022 00:39:54 +0200 Subject: [PATCH 5/7] opgpcard: Remove negative logic --- tools/src/bin/opgpcard/cli.rs | 8 ++++---- tools/src/bin/opgpcard/main.rs | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index 43a3d71..4ca1048 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -188,11 +188,11 @@ pub enum AdminCommand { #[clap(name = "output", long = "output", short = 'o')] output: Option, - #[clap(long = "no-decrypt")] - no_decrypt: bool, + #[clap(long = "no-decrypt", action = clap::ArgAction::SetFalse)] + decrypt: bool, - #[clap(long = "no-auth")] - no_auth: bool, + #[clap(long = "no-auth", action = clap::ArgAction::SetFalse)] + auth: bool, /// Algorithm (rsa2048|rsa3072|rsa4096|nistp256|nistp384|nistp521|25519) #[clap()] diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index 09d1809..c8beac6 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -313,8 +313,8 @@ fn main() -> Result<(), Box> { cli::AdminCommand::Generate { user_pin, output, - no_decrypt, - no_auth, + decrypt, + auth, algo, user_id, } => { @@ -327,8 +327,8 @@ fn main() -> Result<(), Box> { admin_pin.as_deref(), user_pin.as_deref(), output, - !no_decrypt, - !no_auth, + decrypt, + auth, algo, user_id, )?; From 6101e17979ab26b1fd7b0b29b35e7058babaca57 Mon Sep 17 00:00:00 2001 From: Heiko Schaefer Date: Tue, 25 Oct 2022 01:04:12 +0200 Subject: [PATCH 6/7] ci: Update udeps --- .gitlab-ci.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 513a154..5377f39 100644 --- a/.gitlab-ci.yml +++ b/.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 - - 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 + - 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.33-x86_64-unknown-linux-gnu/cargo-udeps script: - cargo udeps --workspace --all-features --all-targets cache: [ ] From 23b4c05c3d4f80561e1e9dcbbb46078bb71551f1 Mon Sep 17 00:00:00 2001 From: Nora Widdecke Date: Tue, 25 Oct 2022 14:55:39 +0200 Subject: [PATCH 7/7] opgpcard: Make algo selection type safe --- tools/src/bin/opgpcard/cli.rs | 34 +++++++++++++++++++++++++++++++--- tools/src/bin/opgpcard/main.rs | 22 +++++----------------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/tools/src/bin/opgpcard/cli.rs b/tools/src/bin/opgpcard/cli.rs index 4ca1048..d5874ec 100644 --- a/tools/src/bin/opgpcard/cli.rs +++ b/tools/src/bin/opgpcard/cli.rs @@ -194,9 +194,9 @@ pub enum AdminCommand { #[clap(long = "no-auth", action = clap::ArgAction::SetFalse)] auth: bool, - /// Algorithm (rsa2048|rsa3072|rsa4096|nistp256|nistp384|nistp521|25519) - #[clap()] - algo: Option, + /// Algorithm + #[clap(value_enum)] + algo: Option, /// User ID to add to the exported certificate representation #[clap(name = "User ID", short = 'u', long = "userid")] @@ -378,3 +378,31 @@ impl From for u8 { } } } + +#[derive(ValueEnum, Debug, Clone)] +#[clap(rename_all = "lower")] +pub enum AdminGenerateAlgo { + Rsa2048, + Rsa3072, + Rsa4096, + Nistp256, + Nistp384, + Nistp521, + Curve25519, +} + +impl From 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, + } + } +} diff --git a/tools/src/bin/opgpcard/main.rs b/tools/src/bin/opgpcard/main.rs index c8beac6..28e07c8 100644 --- a/tools/src/bin/opgpcard/main.rs +++ b/tools/src/bin/opgpcard/main.rs @@ -329,7 +329,7 @@ fn main() -> Result<(), Box> { output, decrypt, auth, - algo, + algo.map(AlgoSimple::from), user_id, )?; } @@ -1092,7 +1092,7 @@ fn generate_keys( output_file: Option, decrypt: bool, auth: bool, - algo: Option, + algo: Option, user_ids: Vec, ) -> Result<()> { let mut output = output::AdminGenerate::default(); @@ -1111,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() { - None => None, - 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)); + log::info!(" Key generation will be attempted with algo: {:?}", algo); + output.algorithm(format!("{:?}", algo)); // 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.")); }