pvsecret: Add -i -o option variants

All pvattest subcommands use the command line option -i <input> and -o
<output> to specify file input and output respectively. pvsecret however
uses mostly positional arguments for <input> and <output> exclusively,
e.g. pvattest check input.bin output.yaml

$ pvsecret add secret.bin

This provides an inconsistent user interface within the Secure
Execution tools and may confuse users.

Add the -i and -o option to the subcommands if applicable.
Input/output can then be specified like so:

$ pvsecret add -i secret.bin
$ pvsecret list -o list.yaml
$ pvsecret verify -i FILE -o out.yaml
$ pvsecret retrieve -i ID -o id.yaml

Reviewed-by: Jan Höppner <hoeppner@linux.ibm.com>
Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
Signed-off-by: Jan Höppner <hoeppner@linux.ibm.com>

rust/pvsecret/src/cli.rs

@@ -292,8 +292,12 @@ impl Display for RetrieveableSecretInpKind {
 #[derive(Args, Debug)]
 pub struct AddSecretOpt {
     /// Specify the request to be sent.
-    #[arg(value_name = "FILE", value_hint = ValueHint::FilePath,)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath,)]
-    pub input: String,
+    input: Option<String>,
+
+    /// Specify the request to be sent.
+    #[arg(value_name = "FILE", value_hint = ValueHint::FilePath, required_unless_present("input"), conflicts_with("input"))]
+    input_pos: Option<String>,
 
     /// Force the addition of add-secret requests.
     ///
@@ -303,6 +307,27 @@ pub struct AddSecretOpt {
     pub force: bool,
 }
 
+#[derive(Debug)]
+pub struct AddSecretOptComb<'a> {
+    pub input: &'a str,
+    pub force: bool,
+}
+
+impl<'a> From<&'a AddSecretOpt> for AddSecretOptComb<'a> {
+    fn from(value: &'a AddSecretOpt) -> Self {
+        let input = match (&value.input, &value.input_pos) {
+            (None, Some(i)) => i.as_str(),
+            (Some(i), None) => i.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        Self {
+            input,
+            force: value.force,
+        }
+    }
+}
+
 #[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, ValueEnum, Debug, Default)]
 pub enum ListSecretOutputType {
     /// Human-focused, non-parsable output format
@@ -317,19 +342,48 @@ pub enum ListSecretOutputType {
 #[derive(Args, Debug)]
 pub struct ListSecretOpt {
     /// Store the result in FILE
-    #[arg(value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath,)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath,)]
-    pub output: String,
+    output: Option<String>,
+
+    /// Store the result in FILE
+    #[arg(value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath, required_unless_present("output"), conflicts_with("output"))]
+    output_pos: Option<String>,
 
     /// Define the output format of the list.
     #[arg(long, value_enum, default_value_t)]
     pub format: ListSecretOutputType,
 }
 
+#[derive(Debug)]
+pub struct ListSecretOptComb<'a> {
+    pub output: &'a str,
+    pub format: ListSecretOutputType,
+}
+
+impl<'a> From<&'a ListSecretOpt> for ListSecretOptComb<'a> {
+    fn from(value: &'a ListSecretOpt) -> Self {
+        let output = match (&value.output, &value.output_pos) {
+            (None, Some(o)) => o.as_str(),
+            (Some(o), None) => o.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        Self {
+            output,
+            format: value.format,
+        }
+    }
+}
+
 #[derive(Args, Debug)]
 pub struct VerifyOpt {
     /// Specify the request to be checked.
-    #[arg(value_name = "FILE", value_hint = ValueHint::FilePath,)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath,)]
-    pub input: String,
+    input: Option<String>,
+
+    /// Specify the request to be checked.
+    #[arg(value_name = "FILE", value_hint = ValueHint::FilePath, required_unless_present("input"), conflicts_with("input"))]
+    input_pos: Option<String>,
 
     /// Certificate containing a public key used to verify the user data signature.
     ///
@@ -345,8 +399,44 @@ pub struct VerifyOpt {
     ///
     /// If the request contained abirtary user-data the output contains this user-data with padded
     /// zeros if available.
-    #[arg(short, long, value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath,)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath,)]
-    pub output: String,
+    output: Option<String>,
+
+    /// Store the result in FILE
+    ///
+    /// If the request contained abirtary user-data the output contains this user-data with padded
+    /// zeros if available.
+    #[arg(value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath, required_unless_present("output"), conflicts_with("output"))]
+    output_pos: Option<String>,
+}
+
+#[derive(Debug)]
+pub struct VerifyOptComb<'a> {
+    pub input: &'a str,
+    pub user_cert: Option<&'a str>,
+    pub output: &'a str,
+}
+
+impl<'a> From<&'a VerifyOpt> for VerifyOptComb<'a> {
+    fn from(value: &'a VerifyOpt) -> Self {
+        let input = match (&value.input, &value.input_pos) {
+            (None, Some(i)) => i.as_str(),
+            (Some(i), None) => i.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        let output = match (&value.output, &value.output_pos) {
+            (None, Some(o)) => o.as_str(),
+            (Some(o), None) => o.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        Self {
+            input,
+            user_cert: value.user_cert.as_deref(),
+            output,
+        }
+    }
 }
 
 #[derive(Args, Debug)]
@@ -358,12 +448,26 @@ pub struct RetrSecretOptions {
     /// handle encodes in hexadecimal. Leading zeros are required. If there are multiple secrets in
     /// the store with the same Id there are no guarantees on which specific secret is retrieved.
     /// Use --inform=idx to make sure a specific secret is retrieved.
-    #[arg(value_name = "ID", value_hint = ValueHint::FilePath)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath)]
-    pub input: String,
+    input: Option<String>,
+
+    /// Specify the secret ID to be retrieved.
+    ///
+    /// Input type depends on '--inform'. If `yaml` (default) is specified, it must be a yaml
+    /// created by the create subcommand of this tool. If `hex` is specified, it must be a 32 byte
+    /// handle encodes in hexadecimal. Leading zeros are required. If there are multiple secrets in
+    /// the store with the same Id there are no guarantees on which specific secret is retrieved.
+    /// Use --inform=idx to make sure a specific secret is retrieved.
+    #[arg(value_name = "ID", value_hint = ValueHint::FilePath, required_unless_present("input"), conflicts_with("input"))]
+    input_pos: Option<String>,
 
     /// Specify the output path to place the secret value
-    #[arg(short, long, value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath)]
+    #[arg(short, long, value_name = "FILE", value_hint = ValueHint::FilePath)]
-    pub output: String,
+    output: Option<String>,
+
+    /// Specify the output path to place the secret value
+    #[arg(value_name = "FILE", default_value = STDOUT, value_hint = ValueHint::FilePath, required_unless_present("output"), conflicts_with("output"))]
+    output_pos: Option<String>,
 
     /// Define input type for the Secret ID
     #[arg(long, value_enum, default_value_t)]
@@ -400,6 +504,35 @@ pub enum RetrOutFmt {
     Bin,
 }
 
+#[derive(Debug)]
+pub struct RetrSecretOptionsComb<'a> {
+    pub input: &'a str,
+    pub output: &'a str,
+    pub inform: RetrInpFmt,
+}
+
+impl<'a> From<&'a RetrSecretOptions> for RetrSecretOptionsComb<'a> {
+    fn from(value: &'a RetrSecretOptions) -> Self {
+        let input = match (&value.input, &value.input_pos) {
+            (None, Some(i)) => i.as_str(),
+            (Some(i), None) => i.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        let output = match (&value.output, &value.output_pos) {
+            (None, Some(o)) => o.as_str(),
+            (Some(o), None) => o.as_str(),
+            (Some(_), Some(_)) => unreachable!(),
+            (None, None) => unreachable!(),
+        };
+        Self {
+            input,
+            output,
+            inform: value.inform,
+        }
+    }
+}
+
 #[derive(Subcommand, Debug)]
 pub enum Command {
     /// Create a new add-secret request.

rust/pvsecret/src/cmd/add.rs

@@ -2,7 +2,10 @@
 //
 // Copyright IBM Corp. 2023
 
-use crate::{cli::AddSecretOpt, cmd::list::list_uvc};
+use crate::{
+    cli::{AddSecretOpt, AddSecretOptComb},
+    cmd::list::list_uvc,
+};
 use anyhow::{bail, Context, Result};
 use log::warn;
 use pv::{
@@ -13,15 +16,16 @@ use utils::get_reader_from_cli_file_arg;
 
 /// Do an Add Secret UVC
 pub fn add(opt: &AddSecretOpt) -> Result<()> {
+    let opt_comb = AddSecretOptComb::from(opt);
     let uv = UvDevice::open()?;
-    let mut rd_in = get_reader_from_cli_file_arg(&opt.input)?;
+    let mut rd_in = get_reader_from_cli_file_arg(opt_comb.input)?;
     let mut cmd =
-        AddCmd::new(&mut rd_in).context(format!("Processing input file {}", opt.input))?;
+        AddCmd::new(&mut rd_in).context(format!("Processing input file {}", opt_comb.input))?;
 
     if let Some(id) = AddSecretRequest::bin_id(cmd.data().unwrap())? {
         if list_uvc(&uv)?.iter().any(|e| e.id() == id.as_ref()) {
             warn!("There is already a secret in the secret store with that id.");
-            match opt.force {
+            match opt_comb.force {
                 true => warn!("'--force' specified: Adding the secret anyways."),
                 false => bail!("Unable to add the secret due to duplicated IDs"),
             }

rust/pvsecret/src/cmd/create.rs

@@ -322,7 +322,7 @@ fn write_yaml<P: AsRef<Path>>(
     write_out(&yaml_path, secret_info, "secret information")?;
     warn!(
         "Successfully wrote secret info to '{}'",
-        yaml_path.display().to_string()
+        yaml_path.display()
     );
     Ok(())
 }

rust/pvsecret/src/cmd/list.rs

@@ -4,7 +4,7 @@
 
 use std::io::ErrorKind;
 
-use crate::cli::{ListSecretOpt, ListSecretOutputType};
+use crate::cli::{ListSecretOpt, ListSecretOptComb, ListSecretOutputType};
 use anyhow::{Context, Error, Result};
 use log::{info, warn};
 use pv::uv::{ListCmd, SecretList, UvDevice};
@@ -34,11 +34,12 @@ pub fn list_uvc(uv: &UvDevice) -> Result<SecretList> {
 
 /// Do a List Secrets UVC and output the list in the requested format
 pub fn list(opt: &ListSecretOpt) -> Result<()> {
+    let opt_comb = ListSecretOptComb::from(opt);
     let uv = UvDevice::open()?;
     let secret_list = list_uvc(&uv)?;
-    let mut wr_out = get_writer_from_cli_file_arg(&opt.output)?;
+    let mut wr_out = get_writer_from_cli_file_arg(opt_comb.output)?;
 
-    match &opt.format {
+    match opt_comb.format {
         ListSecretOutputType::Human => {
             write!(wr_out, "{secret_list}").context("Cannot generate output")?
         }
@@ -50,10 +51,10 @@ pub fn list(opt: &ListSecretOpt) -> Result<()> {
     }
     wr_out.flush()?;
 
-    if opt.output != STDOUT {
+    if opt_comb.output != STDOUT {
         warn!(
             "Successfully wrote the list of secrets to '{}'",
-            &opt.output
+            opt_comb.output
         );
     }
     Ok(())

rust/pvsecret/src/cmd/retr.rs

@@ -15,7 +15,7 @@ use pv::{
 use utils::get_writer_from_cli_file_arg;
 
 use super::list::list_uvc;
-use crate::cli::{RetrInpFmt, RetrOutFmt, RetrSecretOptions};
+use crate::cli::{RetrInpFmt, RetrOutFmt, RetrSecretOptions, RetrSecretOptionsComb};
 
 enum Value {
     Id(SecretId),
@@ -31,19 +31,19 @@ impl Display for Value {
     }
 }
 
-impl TryFrom<&RetrSecretOptions> for Value {
+impl TryFrom<&RetrSecretOptionsComb<'_>> for Value {
     type Error = anyhow::Error;
 
-    fn try_from(opt: &RetrSecretOptions) -> Result<Self> {
+    fn try_from(opt: &RetrSecretOptionsComb) -> Result<Self> {
         match opt.inform {
-            RetrInpFmt::Yaml => match serde_yaml::from_reader(&mut open_file(&opt.input)?)? {
+            RetrInpFmt::Yaml => match serde_yaml::from_reader(&mut open_file(opt.input)?)? {
                 GuestSecret::Retrievable { id, .. } => Ok(Self::Id(id)),
                 gs => bail!("The file contains a {gs}-secret, which is not retrievable."),
             },
-            RetrInpFmt::Hex => serde_yaml::from_str(&opt.input)
+            RetrInpFmt::Hex => serde_yaml::from_str(opt.input)
                 .context("Cannot parse SecretId information")
                 .map(Self::Id),
-            RetrInpFmt::Name => Ok(Self::Id(SecretId::from_string(&opt.input))),
+            RetrInpFmt::Name => Ok(Self::Id(SecretId::from_string(opt.input))),
             RetrInpFmt::Idx => opt
                 .input
                 .parse()
@@ -104,8 +104,9 @@ fn retrieve(value: Value) -> Result<RetrievedSecret> {
 }
 
 pub fn retr(opt: &RetrSecretOptions) -> Result<()> {
-    let mut output = get_writer_from_cli_file_arg(&opt.output)?;
+    let opt_comb = RetrSecretOptionsComb::from(opt);
-    let retr_secret = retrieve(opt.try_into()?)
+    let mut output = get_writer_from_cli_file_arg(opt_comb.output)?;
+    let retr_secret = retrieve((&opt_comb).try_into()?)
         .context("Could not retrieve the secret from the UV secret store.")?;
 
     let out_data = match opt.outform {
@@ -115,7 +116,7 @@ pub fn retr(opt: &RetrSecretOptions) -> Result<()> {
     write(
         &mut output,
         out_data.value(),
-        &opt.output,
+        opt_comb.output,
         "IBM Protected Key",
     )?;
     Ok(())

rust/pvsecret/src/cmd/verify.rs

@@ -2,7 +2,7 @@
 //
 // Copyright IBM Corp. 2024
 
-use crate::cli::VerifyOpt;
+use crate::cli::{VerifyOpt, VerifyOptComb};
 use anyhow::{anyhow, Context, Result};
 use log::warn;
 use pv::misc::{read_certs, read_file};
@@ -22,16 +22,16 @@ fn read_sgn_key(path: &str) -> Result<PKey<Public>> {
 }
 
 pub fn verify(opt: &VerifyOpt) -> Result<()> {
-    let mut rd_in = get_reader_from_cli_file_arg(&opt.input)?;
+    let opt_comb = VerifyOptComb::from(opt);
+    let mut rd_in = get_reader_from_cli_file_arg(opt_comb.input)?;
     let mut data_in = Vec::with_capacity(0x1000);
     rd_in
         .read_to_end(&mut data_in)
-        .with_context(|| format!("Cannot read input file {}", opt.input))?;
+        .with_context(|| format!("Cannot read input file {}", opt_comb.input))?;
 
-    let verify_cert = opt
+    let verify_cert = opt_comb
         .user_cert
-        .as_ref()
+        .map(read_sgn_key)
-        .map(|p| read_sgn_key(p))
         .transpose()
         .context("Cannot read user-verification certificate.")?;
 
@@ -39,9 +39,9 @@ pub fn verify(opt: &VerifyOpt) -> Result<()> {
         .context("Could not verify the the Add-secret request")?;
 
     if let Some(user_data) = user_data {
-        get_writer_from_cli_file_arg(&opt.output)?
+        get_writer_from_cli_file_arg(opt_comb.output)?
             .write_all(&user_data)
-            .with_context(|| format!("Cannot write user data to {}", opt.output))?;
+            .with_context(|| format!("Cannot write user data to {}", opt_comb.output))?;
     }
     warn!("Successfully verified the request.");
     Ok(())