Repository: osa1/tiny (language: rust)

Issue:
Implement getting passwords from external commands
Currently users have to store their passwords in the config file as plain text. That makes it difficult to store the config in e.g. git repos, but also storing password in plain text is just not a great idea in general.

In #171 we designed a feature to solve this problem. Basically we allow getting password from external commands. These commands are just strings, except they start with `!` (`!!` is used for escaping the initial `!`). Commands are allowed in the password fields:

- `nickserv_ident` for NickServ identification password
- `pass` for server passwords
- `sasl.password` for SASL authentication password

If I have `! pass my_irc_password` in a `nickserv_ident` field, tiny runs `pass my_irc_password` when the password is needed and uses the output of the command as the password.

(For reading an env variable `! printenv ENV_VAR` can be used)

Parking the TUI while running the command (without blocking the thread) can be implemented as we do it for the editor support.

The main difficulties are:

- What to do in the client while the command is running. The command may ask a master password to the user to unlock the password store, which may take time to enter. Or the user may simply not be in front of the computer when the command is run etc. The client should be handle waiting arbitrarily long while the command is running.

- How to return the command output to the client.

For (1) I think there are two cases that we need to consider:

- When `pass` is a command: to be able to wait arbitrarily long I think we'll have to run the command *before* actually opening a connection to the server, to avoid timeouts while the server is waiting for `PASS`.

- When `nickserv_ident` or `sasl.password` is a command: by the time we need these passwords we already connected to the server. I think we should wait until the command is run and the password is sent to the server before trying to join channels.

  Note that during this wait the client should run as usual and maintain the connection, by responding to PINGs and sending PINGs to check connectivity, as usual.

  (We need to make sure we handle the case where the connection gets lost while waiting for the command output. I think in the design proposed below we don't need to do anything special for this.)

For (2) I propose we add these methods to `Client`:

- `pass_value`: Provide output of the `pass` command to the client.
- `sasl_pass_value`: Same, for SASL password
- `nickserv_pass_value`: Same, for NickServ password

When `pass` field is used, when connecting (after a disconnect, or when connecting for the first time) a client will only try to (re)connect when `pass_value` is called.

When SASL or nickserv_ident is used, after connecting to a server the client will only send `JOIN`s after `sasl_pass_value` or `nickserv_pass_value` is called.

(We should probably have some sanity checks in client that makes sure the `..._value` methods are called at the right times, e.g. it doesn't make sense to call `pass_value` if we're already connected, or `nickserv_pass_value` if we already sent a `JOIN` message. Those cases are probably bugs that are worth reporting.)

cc @trevarj


Discussion / hints:
Here's a much simpler idea:

Run commands of servers that we're connecting to on startup. If we do this then `Client` doesn't need to change at all as it'll have all the password it needs on initialization.

The problem is if a password is not correct then the user has no way of updating it without restarting tiny. Perhaps that's not a big deal. We can always implement the original, complicated idea above if this is a common case (I doubt it).
> Run commands of servers that we're connecting to on startup.

I support this idea just because of the simplicity. As we talked about on IRC, the code for running the password command would have to spiderweb all over the UI instead of just living beside the config file and it gets a little messy.
Makes sense.

There's also the question of when to run the commands, before or after starting the TUI.

Doing it before is simple. We print a line and then run the command, like:

```
Running command for irc.freenode.net nickserv password...
<run the command here>
Running command for irc.oftc.net SASL password ...
<run the command here>
...
```

After all the commands are run we start the TUI.

A command may print stuff for the user so we shouldn't just capture the output. For example it may ask for the master key password or something like that. I guess we should only consider the last line in the process's stdout as the password.

If a command fails we should probably show the error in the server tab for that command.


Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 1d1a4db3..60f3a5ae 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -12,6 +12,8 @@ Thanks to @ALEX11BR for contributing to this release.
   with PART and QUIT messages to the server. (#365, #395)
 - Fixed running `$EDITOR` on macOS 12. Previously, on macOS 12, changes in the
   temporary file would not be read properly when `$EDITOR` is closed.
+- Passwords can now be read from external commands (e.g. a password manager).
+  See README for details. (#246, #315)
 
 # 2021/11/07: 0.10.0
 
diff --git a/Cargo.lock b/Cargo.lock
index cc057d76..f9d2d34d 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1171,6 +1171,12 @@ version = "1.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "ae1a47186c03a32177042e55dbc5fd5aee900b8e0069a8d70fba96a9375cd012"
 
+[[package]]
+name = "shell-words"
+version = "1.1.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "24188a676b6ae68c3b2cb3a01be17fbf7240ce009799bb56d5b1409051e78fde"
+
 [[package]]
 name = "signal-hook-registry"
 version = "1.4.0"
@@ -1368,6 +1374,7 @@ dependencies = [
  "rustc_tools_util",
  "serde",
  "serde_yaml",
+ "shell-words",
  "term_input",
  "termbox_simple",
  "time",
diff --git a/README.md b/README.md
index 28480779..15a8b4ad 100644
--- a/README.md
+++ b/README.md
@@ -106,6 +106,23 @@ like `NickServ`) is better when some of the channels that you automatically
 join require identification. To use this method enter your nick password to the
 `pass` field in servers.
 
+### Using external commands for passwords
+
+When a password field in the config file is a map with a `command` key, the
+value is used as the shell command to run to get the password.
+
+For example, in this config:
+
+```yaml
+sasl:
+  username: osa1
+  password:
+    command: 'pass show "my irc password"'
+```
+
+tiny runs the `pass ...` command and uses last line printed by the command as
+the password.
+
 ## Command line arguments
 
 By default (i.e. when no command line arguments passed) tiny connects to all
diff --git a/crates/tiny/Cargo.toml b/crates/tiny/Cargo.toml
index f8249fae..2c1c8e24 100644
--- a/crates/tiny/Cargo.toml
+++ b/crates/tiny/Cargo.toml
@@ -26,6 +26,7 @@ libtiny_wire = { path = "../libtiny_wire" }
 log = "0.4"
 serde = { version = "1.0", features = ["derive"] }
 serde_yaml = "0.8"
+shell-words = "1.1.0"
 time = "0.1"
 tokio = { version = "1.17", default-features = false, features = [] }
 tokio-stream = { version = "0.1", features = [] }
diff --git a/crates/tiny/src/config.rs b/crates/tiny/src/config.rs
index b536f1f3..2b638e9e 100644
--- a/crates/tiny/src/config.rs
+++ b/crates/tiny/src/config.rs
@@ -3,15 +3,17 @@ use std::fs;
 use std::fs::File;
 use std::io::{Read, Write};
 use std::path::{Path, PathBuf};
+use std::process::Command;
 
 #[derive(Clone, Deserialize, Debug, PartialEq, Eq)]
-pub(crate) struct SASLAuth {
+pub(crate) struct SASLAuth<P> {
     pub(crate) username: String,
-    pub(crate) password: String,
+    pub(crate) password: P,
 }
 
 #[derive(Clone, Deserialize)]
-pub(crate) struct Server {
+#[serde(bound(deserialize = "P: Deserialize<'de>"))]
+pub(crate) struct Server<P> {
     /// Address of the server
     pub(crate) addr: String,
 
@@ -28,7 +30,7 @@ pub(crate) struct Server {
 
     /// Server password (optional)
     #[serde(default)]
-    pub(crate) pass: Option<String>,
+    pub(crate) pass: Option<P>,
 
     /// Real name to be used in connection registration
     #[serde(deserialize_with = "deser_trimmed_str")]
@@ -44,11 +46,11 @@ pub(crate) struct Server {
     pub(crate) join: Vec<String>,
 
     /// NickServ identification password. Used on connecting to the server and nick change.
-    pub(crate) nickserv_ident: Option<String>,
+    pub(crate) nickserv_ident: Option<P>,
 
     /// Authenication method
     #[serde(rename = "sasl")]
-    pub(crate) sasl_auth: Option<SASLAuth>,
+    pub(crate) sasl_auth: Option<SASLAuth<P>>,
 }
 
 /// Similar to `Server`, but used when connecting via the `/connect` command.
@@ -65,8 +67,8 @@ pub(crate) struct Defaults {
 }
 
 #[derive(Deserialize)]
-pub(crate) struct Config {
-    pub(crate) servers: Vec<Server>,
+pub(crate) struct Config<P> {
+    pub(crate) servers: Vec<Server<P>>,
     pub(crate) defaults: Defaults,
     pub(crate) log_dir: Option<PathBuf>,
 }
@@ -87,8 +89,113 @@ where
     Ok(strs.into_iter().map(|s| s.trim().to_owned()).collect())
 }
 
-impl Config {
-    /// Returns error descriptions
+/// A password, or a shell command to run the obtain a password. Used for password (server
+/// password, SASL, NickServ) fields of `Config`.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum PassOrCmd {
+    /// Password is given directly as plain text
+    Pass(String),
+
+    /// A shell command to run to get the password
+    Cmd(Vec<String>),
+}
+
+impl PassOrCmd {
+    fn is_empty_cmd(&self) -> bool {
+        match self {
+            PassOrCmd::Cmd(cmd) => cmd.is_empty(),
+            PassOrCmd::Pass(_) => false,
+        }
+    }
+}
+
+impl<'de> Deserialize<'de> for PassOrCmd {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        use serde::de::Error;
+        use serde_yaml::Value;
+
+        match Value::deserialize(deserializer)? {
+            Value::String(str) => Ok(PassOrCmd::Pass(str)),
+            Value::Mapping(map) => match map.get(&Value::String("command".to_owned())) {
+                Some(Value::String(cmd)) => match shell_words::split(cmd) {
+                    Ok(cmd_parts) => Ok(PassOrCmd::Cmd(cmd_parts)),
+                    Err(err) => Err(D::Error::custom(format!(
+                        "Unable to parse password field: {}",
+                        err
+                    ))),
+                },
+                _ => Err(D::Error::custom(
+                    "Expeted a 'cmd' key in password map with string value",
+                )),
+            },
+            _ => Err(D::Error::custom("Password field must be a string or map")),
+        }
+    }
+}
+
+fn run_command(command_name: &str, server_addr: &str, args: &[String]) -> Option<String> {
+    println!(
+        "Running {} command for {} (`{}`)",
+        command_name,
+        server_addr,
+        shell_words::join(args)
+    );
+
+    assert!(!args.is_empty()); // should be checked in `validate`
+
+    let mut cmd = Command::new(&args[0]);
+    cmd.args(args[1..].iter());
+
+    let output = match cmd.output() {
+        Err(err) => {
+            println!("Command failed: {:?}", err);
+            return None;
+        }
+        Ok(output) => output,
+    };
+
+    if !output.status.success() {
+        print!("Command returned non-zero");
+        if let Some(code) = output.status.code() {
+            println!(": {}", code);
+        } else {
+            println!();
+        }
+        if output.stdout.is_empty() {
+            println!("stdout is empty");
+        } else {
+            println!("stdout:");
+            println!("--------------------------------------");
+            println!("{}", String::from_utf8_lossy(&output.stdout));
+            println!("--------------------------------------");
+        }
+
+        if output.stderr.is_empty() {
+            println!("stderr is empty");
+        } else {
+            println!("stderr:");
+            println!("--------------------------------------");
+            println!("{}", String::from_utf8_lossy(&output.stderr));
+            println!("--------------------------------------");
+        }
+
+        return None;
+    }
+
+    if output.stdout.is_empty() {
+        println!("Command returned zero, but stdout is empty. Aborting.");
+        return None;
+    }
+
+    let stdout = String::from_utf8_lossy(&output.stdout).into_owned();
+    Some(stdout.lines().last().unwrap().to_owned())
+}
+
+impl Config<PassOrCmd> {
+    /// Returns error descriptions.
     pub(crate) fn validate(&self) -> Vec<String> {
         let mut errors = vec![];
 
@@ -133,10 +240,107 @@ impl Config {
                     server.addr
                 ));
             }
+
+            if let Some(ref pass) = server.pass {
+                if pass.is_empty_cmd() {
+                    errors.push(format!("Empty PASS command for '{}'", server.addr));
+                }
+            }
+
+            if let Some(ref nickserv_ident) = server.nickserv_ident {
+                if nickserv_ident.is_empty_cmd() {
+                    errors.push(format!(
+                        "Empty NickServ password command for '{}'",
+                        server.addr
+                    ));
+                }
+            }
+
+            if let Some(ref sasl_auth) = server.sasl_auth {
+                if sasl_auth.password.is_empty_cmd() {
+                    errors.push(format!("Empty SASL password command for '{}'", server.addr));
+                }
+            }
         }
 
         errors
     }
+
+    /// Runs password commands and updates the config with plain passwords obtained from the
+    /// commands.
+    pub(crate) fn read_passwords(self) -> Option<Config<String>> {
+        let Config {
+            servers,
+            defaults,
+            log_dir,
+        } = self;
+
+        let mut servers_: Vec<Server<String>> = Vec::with_capacity(servers.len());
+
+        for server in servers {
+            let Server {
+                addr,
+                alias,
+                port,
+                tls,
+                pass,
+                realname,
+                nicks,
+                join,
+                nickserv_ident,
+                sasl_auth,
+            } = server;
+
+            let pass = match pass {
+                None => None,
+                Some(PassOrCmd::Pass(pass)) => Some(pass),
+                Some(PassOrCmd::Cmd(cmd)) => Some(run_command("server password", &addr, &cmd)?),
+            };
+
+            let nickserv_ident = match nickserv_ident {
+                None => None,
+                Some(PassOrCmd::Pass(pass)) => Some(pass),
+                Some(PassOrCmd::Cmd(cmd)) => Some(run_command("NickServ password", &addr, &cmd)?),
+            };
+
+            let sasl_auth = match sasl_auth {
+                None => None,
+                Some(SASLAuth {
+                    username,
+                    password: PassOrCmd::Pass(pass),
+                }) => Some(SASLAuth {
+                    username,
+                    password: pass,
+                }),
+                Some(SASLAuth {
+                    username,
+                    password: PassOrCmd::Cmd(cmd),
+                }) => {
+                    let password = run_command("SASL password", &addr, &cmd)?;
+                    Some(SASLAuth { username, password })
+                }
+            };
+
+            servers_.push(Server {
+                addr,
+                alias,
+                port,
+                tls,
+                pass,
+                realname,
+                nicks,
+                join,
+                nickserv_ident,
+                sasl_auth,
+            });
+        }
+
+        Some(Config {
+            servers: servers_,
+            defaults,
+            log_dir,
+        })
+    }
 }
 
 /// Returns tiny config file path. File may or may not exist.
@@ -175,7 +379,7 @@ pub(crate) fn get_config_path() -> PathBuf {
     }
 }
 
-pub(crate) fn parse_config(config_path: &Path) -> Result<Config, serde_yaml::Error> {
+pub(crate) fn parse_config(config_path: &Path) -> Result<Config<PassOrCmd>, serde_yaml::Error> {
     let contents = {
         let mut str = String::new();
         let mut file = File::open(config_path).unwrap();
@@ -216,7 +420,7 @@ mod tests {
 
     #[test]
     fn parse_default_config() {
-        match serde_yaml::from_str(&get_default_config_yaml()) {
+        match serde_yaml::from_str::<Config<String>>(&get_default_config_yaml()) {
             Err(yaml_err) => {
                 println!("{}", yaml_err);
                 panic!();
@@ -271,4 +475,29 @@ mod tests {
             "'realname' can't be empty, please update 'realname' field of 'my_server'"
         );
     }
+
+    #[test]
+    fn parse_password_field() {
+        let field = "command: my pass cmd";
+        assert_eq!(
+            serde_yaml::from_str::<PassOrCmd>(&field).unwrap(),
+            PassOrCmd::Cmd(vec!["my".to_owned(), "pass".to_owned(), "cmd".to_owned()])
+        );
+
+        let field = "my password";
+        assert_eq!(
+            serde_yaml::from_str::<PassOrCmd>(&field).unwrap(),
+            PassOrCmd::Pass(field.to_string())
+        );
+
+        let field = "command: \"pass show 'my password'\"";
+        assert_eq!(
+            serde_yaml::from_str::<PassOrCmd>(&field).unwrap(),
+            PassOrCmd::Cmd(vec![
+                "pass".to_string(),
+                "show".to_string(),
+                "my password".to_string()
+            ])
+        );
+    }
 }
diff --git a/crates/tiny/src/main.rs b/crates/tiny/src/main.rs
index 4f08f463..c9567c43 100644
--- a/crates/tiny/src/main.rs
+++ b/crates/tiny/src/main.rs
@@ -54,6 +54,11 @@ fn main() {
                     exit(1);
                 }
 
+                let config = match config.read_passwords() {
+                    None => exit(1),
+                    Some(config) => config,
+                };
+
                 let config::Config {
                     servers,
                     defaults,
@@ -78,7 +83,7 @@ fn main() {
 const DEBUG_LOG_FILE: &str = "tiny_debug_logs.txt";
 
 fn run(
-    servers: Vec<config::Server>,
+    servers: Vec<config::Server<String>>,
     defaults: config::Defaults,
     config_path: PathBuf,
     log_dir: Option<PathBuf>,


Test patch: