From 8cf63c6790192c30c81294e7a940d470bf061cbf Mon Sep 17 00:00:00 2001 From: Daniel Mueller Date: Tue, 25 Aug 2020 19:04:50 -0700 Subject: Introduce support for user-provided extensions This change introduces support for discovering and executing user-provided extensions to the program. Extensions are useful for allowing users to provide additional functionality on top of the nitrocli proper. Implementation wise we stick to an approach similar to git or cargo subcommands in nature: we search the directories listed in the PATH environment variable for a file that starts with "nitrocli-", followed by the extension name. This file is then executed. It is assumed that the extension recognizes (or at least not prohibits) the following arguments: --nitrocli (providing the path to the nitrocli binary), --model (with the model passed to the main program), and --verbosity (the verbosity level). --- src/args.rs | 4 ++ src/commands.rs | 103 +++++++++++++++++++++++++++++++++++ src/main.rs | 62 ++++++++++++++++++---- src/redefine.rs | 11 +--- src/tests/extension_var_test.py | 61 +++++++++++++++++++++ src/tests/extensions.rs | 43 +++++++++++++++ src/tests/mod.rs | 10 ++++ src/tests/run.rs | 115 ++++++++++++++++++++++++++++++++++++++++ 8 files changed, 388 insertions(+), 21 deletions(-) create mode 100644 src/tests/extension_var_test.py create mode 100644 src/tests/extensions.rs (limited to 'src') diff --git a/src/args.rs b/src/args.rs index 6cf37c8..a8f43e1 100644 --- a/src/args.rs +++ b/src/args.rs @@ -4,6 +4,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later use std::convert; +use std::ffi; /// Provides access to a Nitrokey device #[derive(Debug, structopt::StructOpt)] @@ -107,6 +108,9 @@ Command! { Status => crate::commands::status, /// Interacts with the device's unencrypted volume Unencrypted(UnencryptedArgs) => |ctx, args: UnencryptedArgs| args.subcmd.execute(ctx), + /// An extension and its arguments. + #[structopt(external_subcommand)] + Extension(Vec) => crate::commands::extension, ] } diff --git a/src/commands.rs b/src/commands.rs index 092b006..549ebec 100644 --- a/src/commands.rs +++ b/src/commands.rs @@ -3,11 +3,17 @@ // Copyright (C) 2018-2020 The Nitrocli Developers // SPDX-License-Identifier: GPL-3.0-or-later +use std::borrow; use std::convert::TryFrom as _; +use std::env; +use std::ffi; use std::fmt; +use std::io; use std::mem; use std::ops; use std::ops::Deref as _; +use std::path; +use std::process; use std::thread; use std::time; use std::u8; @@ -1081,6 +1087,103 @@ pub fn pws_status(ctx: &mut Context<'_>, all: bool) -> anyhow::Result<()> { }) } +/// Resolve an extension provided by name to an actual path. +/// +/// Extensions are (executable) files that have the "nitrocli-" prefix +/// and are discoverable via the `PATH` environment variable. +pub(crate) fn resolve_extension( + path_var: &ffi::OsStr, + ext_name: &ffi::OsStr, +) -> anyhow::Result { + let mut bin_name = ffi::OsString::from("nitrocli-"); + bin_name.push(ext_name); + + for dir in env::split_paths(path_var) { + let mut bin_path = dir.clone(); + bin_path.push(&bin_name); + // Note that we deliberately do not check whether the file we found + // is executable. If it is not we will just fail later on with a + // permission denied error. The reasons for this behavior are two + // fold: + // 1) Checking whether a file is executable in Rust is painful (as + // of 1.37 there exists the PermissionsExt trait but it is + // available only for Unix based systems). + // 2) It is considered a better user experience to resolve to an + // extension even if it later turned out to be not usable over + // not showing it and silently doing nothing -- mostly because + // anything residing in PATH should be executable anyway and + // given that its name also starts with nitrocli- we are pretty + // sure that's a bug on the user's side. + if bin_path.is_file() { + return Ok(bin_path); + } + } + + let err = if let Some(name) = bin_name.to_str() { + format!("Extension {} not found", name).into() + } else { + borrow::Cow::from("Extension not found") + }; + Err(io::Error::new(io::ErrorKind::NotFound, err).into()) +} + +/// Run an extension. +pub fn extension(ctx: &mut Context<'_>, args: Vec) -> anyhow::Result<()> { + // Note that while `Command` would actually honor PATH by itself, we + // do not want that behavior because it would circumvent the execution + // context we use for testing. As such, we need to do our own search. + let mut args = args.into_iter(); + let ext_name = args.next().context("No extension specified")?; + let path_var = ctx.path.as_ref().context("PATH variable not present")?; + let ext_path = resolve_extension(&path_var, &ext_name)?; + + // Note that theoretically we could just exec the extension and be + // done. However, the problem with that approach is that it makes + // testing extension support much more nasty, because the test process + // would be overwritten in the process, requiring us to essentially + // fork & exec nitrocli beforehand -- which is much more involved from + // a cargo test context. + let mut cmd = process::Command::new(&ext_path); + + if let Some(model) = ctx.config.model { + let _ = cmd.env(crate::NITROCLI_MODEL, model.to_string()); + } + + if let Some(usb_path) = &ctx.config.usb_path { + let _ = cmd.env(crate::NITROCLI_USB_PATH, usb_path); + } + + // TODO: We may want to take this path from the command execution + // context. + let binary = env::current_exe().context("Failed to retrieve path to nitrocli binary")?; + let serial_numbers = ctx + .config + .serial_numbers + .iter() + .map(ToString::to_string) + .collect::>() + .join(","); + + let out = cmd + .env(crate::NITROCLI_BINARY, binary) + .env(crate::NITROCLI_VERBOSITY, ctx.config.verbosity.to_string()) + .env(crate::NITROCLI_NO_CACHE, ctx.config.no_cache.to_string()) + .env(crate::NITROCLI_SERIAL_NUMBERS, serial_numbers) + .args(args) + .output() + .with_context(|| format!("Failed to execute extension {}", ext_path.display()))?; + ctx.stdout.write_all(&out.stdout)?; + ctx.stderr.write_all(&out.stderr)?; + + if out.status.success() { + Ok(()) + } else if let Some(rc) = out.status.code() { + Err(anyhow::Error::new(crate::DirectExitError(rc))) + } else { + Err(anyhow::Error::new(crate::DirectExitError(1))) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/src/main.rs b/src/main.rs index c0c7da5..e7a7d2f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -62,9 +62,19 @@ mod pinentry; mod tests; use std::env; +use std::error; use std::ffi; +use std::fmt; use std::io; use std::process; +use std::str; + +const NITROCLI_BINARY: &str = "NITROCLI_BINARY"; +const NITROCLI_MODEL: &str = "NITROCLI_MODEL"; +const NITROCLI_USB_PATH: &str = "NITROCLI_USB_PATH"; +const NITROCLI_VERBOSITY: &str = "NITROCLI_VERBOSITY"; +const NITROCLI_NO_CACHE: &str = "NITROCLI_NO_CACHE"; +const NITROCLI_SERIAL_NUMBERS: &str = "NITROCLI_SERIAL_NUMBERS"; const NITROCLI_ADMIN_PIN: &str = "NITROCLI_ADMIN_PIN"; const NITROCLI_USER_PIN: &str = "NITROCLI_USER_PIN"; @@ -72,6 +82,28 @@ const NITROCLI_NEW_ADMIN_PIN: &str = "NITROCLI_NEW_ADMIN_PIN"; const NITROCLI_NEW_USER_PIN: &str = "NITROCLI_NEW_USER_PIN"; const NITROCLI_PASSWORD: &str = "NITROCLI_PASSWORD"; +/// A special error type that indicates the desire to exit directly, +/// without additional error reporting. +/// +/// This error is mostly used by the extension support code so that we +/// are able to mirror the extension's exit code while preserving our +/// context logic and the fairly isolated testing it enables. +struct DirectExitError(i32); + +impl fmt::Debug for DirectExitError { + fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result { + unreachable!() + } +} + +impl fmt::Display for DirectExitError { + fn fmt(&self, _: &mut fmt::Formatter<'_>) -> fmt::Result { + unreachable!() + } +} + +impl error::Error for DirectExitError {} + /// Parse the command-line arguments and execute the selected command. fn handle_arguments(ctx: &mut Context<'_>, args: Vec) -> anyhow::Result<()> { use structopt::StructOpt; @@ -101,6 +133,8 @@ pub struct Context<'io> { pub stderr: &'io mut dyn io::Write, /// Whether `stdout` is a TTY. pub is_tty: bool, + /// The content of the `PATH` environment variable. + pub path: Option, /// The admin PIN, if provided through an environment variable. pub admin_pin: Option, /// The user PIN, if provided through an environment variable. @@ -135,6 +169,10 @@ impl<'io> Context<'io> { stdout, stderr, is_tty, + // The std::env module has several references to the PATH + // environment variable, indicating that this name is considered + // platform independent from their perspective. We do the same. + path: env::var_os("PATH"), admin_pin: env::var_os(NITROCLI_ADMIN_PIN), user_pin: env::var_os(NITROCLI_USER_PIN), new_admin_pin: env::var_os(NITROCLI_NEW_ADMIN_PIN), @@ -145,16 +183,21 @@ impl<'io> Context<'io> { } } -fn run<'ctx, 'io: 'ctx>(ctx: &'ctx mut Context<'io>, args: Vec) -> i32 { - match handle_arguments(ctx, args) { - Ok(()) => 0, - Err(err) => { - let _ = eprintln!(ctx, "{:?}", err); - 1 - } +fn evaluate_err(err: anyhow::Error, stderr: &mut dyn io::Write) -> i32 { + if let Some(err) = err.root_cause().downcast_ref::() { + err.0 + } else { + let _ = writeln!(stderr, "{:?}", err); + 1 } } +fn run<'ctx, 'io: 'ctx>(ctx: &'ctx mut Context<'io>, args: Vec) -> i32 { + handle_arguments(ctx, args) + .map(|()| 0) + .unwrap_or_else(|err| evaluate_err(err, ctx.stderr)) +} + fn main() { use std::io::Write; @@ -169,10 +212,7 @@ fn main() { run(ctx, args) } - Err(err) => { - let _ = writeln!(stderr, "{:?}", err); - 1 - } + Err(err) => evaluate_err(err, &mut stderr), }; // We exit the process the hard way below. The problem is that because diff --git a/src/redefine.rs b/src/redefine.rs index 10fb631..fe15765 100644 --- a/src/redefine.rs +++ b/src/redefine.rs @@ -1,6 +1,6 @@ // redefine.rs -// Copyright (C) 2019 The Nitrocli Developers +// Copyright (C) 2019-2020 The Nitrocli Developers // SPDX-License-Identifier: GPL-3.0-or-later // A replacement of the standard println!() macro that requires an @@ -19,12 +19,3 @@ macro_rules! print { write!($ctx.stdout, $($arg)*) }; } - -macro_rules! eprintln { - ($ctx:expr) => { - writeln!($ctx.stderr, "") - }; - ($ctx:expr, $($arg:tt)*) => { - writeln!($ctx.stderr, $($arg)*) - }; -} diff --git a/src/tests/extension_var_test.py b/src/tests/extension_var_test.py new file mode 100644 index 0000000..af7ec84 --- /dev/null +++ b/src/tests/extension_var_test.py @@ -0,0 +1,61 @@ +#!/usr/bin/env python3 + +# Copyright (C) 2020 The Nitrocli Developers +# SPDX-License-Identifier: GPL-3.0-or-later + +from argparse import ( + ArgumentParser, +) +from enum import ( + Enum, +) +from os import ( + environ, +) +from sys import ( + argv, + exit, +) + + +class Action(Enum): + """An action to perform.""" + BINARY = "binary" + MODEL = "model" + NO_CACHE = "no-cache" + SERIAL_NUMBERS = "serial-numbers" + USB_PATH = "usb-path" + VERBOSITY = "verbosity" + + @classmethod + def all(cls): + """Return the list of all the enum members' values.""" + return [x.value for x in cls.__members__.values()] + + +def main(args): + """The extension's main function.""" + parser = ArgumentParser() + parser.add_argument(choices=Action.all(), dest="what") + parser.add_argument("--nitrocli", action="store", default=None) + parser.add_argument("--model", action="store", default=None) + # We deliberately store the argument to this option as a string + # because we can differentiate between None and a valid value, in + # order to verify that it really is supplied. + parser.add_argument("--verbosity", action="store", default=None) + + namespace = parser.parse_args(args[1:]) + # We create a "reverse" mapping from string to variant (e.g., model -> + # MODEL). + options = {v.value: k for k, v in Action.__members__.items()} + try: + var = options[namespace.what] + print(environ[f"NITROCLI_{var}"]) + except KeyError: + return 1 + + return 0 + + +if __name__ == "__main__": + exit(main(argv)) diff --git a/src/tests/extensions.rs b/src/tests/extensions.rs new file mode 100644 index 0000000..a295949 --- /dev/null +++ b/src/tests/extensions.rs @@ -0,0 +1,43 @@ +// extensions.rs + +// Copyright (C) 2020 The Nitrocli Developers +// SPDX-License-Identifier: GPL-3.0-or-later + +use std::env; +use std::fs; + +use super::*; + +#[test] +fn resolve_extensions() -> anyhow::Result<()> { + let dir1 = tempfile::tempdir()?; + let dir2 = tempfile::tempdir()?; + + { + let ext1_path = dir1.path().join("nitrocli-ext1"); + let ext2_path = dir1.path().join("nitrocli-ext2"); + let ext3_path = dir2.path().join("nitrocli-super-1337-extensions111one"); + let _ext1 = fs::File::create(&ext1_path)?; + let _ext2 = fs::File::create(&ext2_path)?; + let _ext3 = fs::File::create(&ext3_path)?; + + let path = env::join_paths(&[dir1.path(), dir2.path()])?; + assert_eq!( + crate::commands::resolve_extension(&path, ffi::OsStr::new("ext1"))?, + ext1_path + ); + assert_eq!( + crate::commands::resolve_extension(&path, ffi::OsStr::new("ext2"))?, + ext2_path + ); + assert_eq!( + crate::commands::resolve_extension(&path, ffi::OsStr::new("super-1337-extensions111one"))?, + ext3_path + ); + + let err = crate::commands::resolve_extension(&ffi::OsStr::new(""), ffi::OsStr::new("ext1")) + .unwrap_err(); + assert_eq!(err.to_string(), "Extension nitrocli-ext1 not found"); + } + Ok(()) +} diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 65983bb..5e47520 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -9,6 +9,7 @@ use nitrokey_test::test as test_device; mod config; mod encrypted; +mod extensions; mod fill; mod hidden; mod list; @@ -23,6 +24,7 @@ mod unencrypted; struct Nitrocli { model: Option, + path: Option, admin_pin: Option, user_pin: Option, new_admin_pin: Option, @@ -34,6 +36,7 @@ impl Nitrocli { pub fn new() -> Self { Self { model: None, + path: None, admin_pin: Some(nitrokey::DEFAULT_ADMIN_PIN.into()), user_pin: Some(nitrokey::DEFAULT_USER_PIN.into()), new_admin_pin: None, @@ -54,6 +57,12 @@ impl Nitrocli { self } + /// Set the `PATH` used for looking up extensions. + fn path(mut self, path: impl Into) -> Self { + self.path = Some(path.into()); + self + } + pub fn admin_pin(mut self, pin: impl Into) -> Self { self.admin_pin = Some(pin.into()); self @@ -102,6 +111,7 @@ impl Nitrocli { stdout: &mut stdout, stderr: &mut stderr, is_tty: false, + path: self.path.clone(), admin_pin: self.admin_pin.clone(), user_pin: self.user_pin.clone(), new_admin_pin: self.new_admin_pin.clone(), diff --git a/src/tests/run.rs b/src/tests/run.rs index 33191d3..e4bbb28 100644 --- a/src/tests/run.rs +++ b/src/tests/run.rs @@ -5,8 +5,12 @@ use std::collections; use std::convert; +use std::convert::TryFrom as _; use std::convert::TryInto as _; +use std::fs; +use std::io::Write; use std::ops; +use std::os::unix::fs::OpenOptionsExt; use std::path; use super::*; @@ -277,3 +281,114 @@ fn connect_usb_path_model_wrong_serial(_model: nitrokey::Model) -> anyhow::Resul } Ok(()) } + +#[test] +fn extension() -> anyhow::Result<()> { + let ext_dir = tempfile::tempdir()?; + { + let mut ext = fs::OpenOptions::new() + .create(true) + .mode(0o755) + .write(true) + .open(ext_dir.path().join("nitrocli-ext"))?; + + ext.write_all( + br#"#!/usr/bin/env python +print("success") +"#, + )?; + } + + let path = ext_dir.path().as_os_str().to_os_string(); + let out = Nitrocli::new().path(path).handle(&["ext"])?; + assert_eq!(out, "success\n"); + Ok(()) +} + +#[test] +fn extension_failure() -> anyhow::Result<()> { + let ext_dir = tempfile::tempdir()?; + { + let mut ext = fs::OpenOptions::new() + .create(true) + .mode(0o755) + .write(true) + .open(ext_dir.path().join("nitrocli-ext"))?; + + ext.write_all( + br#"#!/usr/bin/env python +import sys +sys.exit(42); +"#, + )?; + } + + let path = ext_dir.path().as_os_str().to_os_string(); + let mut ncli = Nitrocli::new().path(path); + + let err = ncli.handle(&["ext"]).unwrap_err(); + // The extension is responsible for printing any error messages. + // Nitrocli is expected not to mess with them, including adding + // additional information. + if let Some(crate::DirectExitError(rc)) = err.downcast_ref::() { + assert_eq!(*rc, 42) + } else { + panic!("encountered unexpected error: {:#}", err) + } + + let (rc, out, err) = ncli.run(&["ext"]); + assert_eq!(rc, 42); + assert_eq!(out, b""); + assert_eq!(err, b""); + Ok(()) +} + +#[test_device] +fn extension_arguments(model: nitrokey::Model) -> anyhow::Result<()> { + fn test(model: nitrokey::Model, what: &str, args: &[&str], check: F) -> anyhow::Result<()> + where + F: FnOnce(&str) -> bool, + { + let ext_dir = tempfile::tempdir()?; + { + let mut ext = fs::OpenOptions::new() + .create(true) + .mode(0o755) + .write(true) + .open(ext_dir.path().join("nitrocli-ext"))?; + + ext.write_all(include_bytes!("extension_var_test.py"))?; + } + + let mut args = args.to_vec(); + args.append(&mut vec!["ext", what]); + + let path = ext_dir.path().as_os_str().to_os_string(); + let out = Nitrocli::new().model(model).path(path).handle(&args)?; + + assert!(check(&out), out); + Ok(()) + } + + test(model, "binary", &[], |out| { + path::Path::new(out) + .file_stem() + .unwrap() + .to_str() + .unwrap() + .trim() + .contains("nitrocli") + })?; + test(model, "model", &[], |out| { + out == args::DeviceModel::try_from(model).unwrap().to_string() + "\n" + })?; + test(model, "no-cache", &[], |out| out == "true\n")?; + test(model, "serial-numbers", &[], |out| out == "\n")?; + test(model, "verbosity", &[], |out| out == "0\n")?; + test(model, "verbosity", &["-v"], |out| out == "1\n")?; + test(model, "verbosity", &["-v", "--verbose"], |out| out == "2\n")?; + + // NITROCLI_USB_PATH should not be set, so the program errors out. + let _ = test(model, "usb-path", &[], |out| out == "\n").unwrap_err(); + Ok(()) +} -- cgit v1.2.3