Add ability to read pass from env variables

[meain]

With this you could write something like below and the password will be read from the env variable.

servers:
  - addr: irc.freenode.net
    port: 6667
    nicks: [meain]
    pass: "$FREENODE_PASSWORD"

[osa1]

Thanks @meain . Interesting idea. I don't have an opinion on whether this is a good idea or a bad one. Do you know any other clients that do this? Is this considered a good practice? Why is env considered safer than your config file to store passwords?

[meain]

@osa1
Sorry that I posted it out the blue and without much context. I was in a hurry.

irssi has a similar option. I have also seen a few other programs like mutt do similar stuff.
One other option that I have seen done is use of something like passcmd which get a command to run in a shell to get the password which can be used to maybe call a passwd manager.

I don't think it is necessarily more secure than having the password in the file, but it lets you to have the config file committed into something like a dotfiles repo and tracked.

[osa1]

OK. This is a user-facing change and a new feature so we should be updating README and CHANGELOG. Let me know if you don't have time for that.

[meain]

Sure, I'll add that in.

I was looking into the shellexpand package and I don't think we need the full package. If you this this could be added in I think I will refactor it to use just std::env (Not sure why I did not think of it in the first place).

Once small issue that might come up is that if the user's password is something like $ILOVEDOLLORSIGN it will look for the env variable ILOVEDOLLORSIGN. We could work around this by using the exact string if we cannot find a variable name like that(which will minimize possible issues by a lot).

What do you think?

[osa1]

I was looking into the shellexpand package and I don't think we need the full package. If you this this could be added in I think I will refactor it to use just std::env (Not sure why I did not think of it in the first place).

The library is small, has no dependencies, and the code is not entirely trivial, so it should be fine to use it. If the code was trivial or the library did too much/brought a set of dependencies then I'd be more inclined to implement it in tiny.

Once small issue that might come up is that if the user's password is something like $ILOVEDOLLORSIGN it will look for the env variable ILOVEDOLLORSIGN.

How does shellexpand do this? Surely it must be providing a way of escaping $s, but I can't see it in README. What happens if you do shellexpand("\$FOO")?

We could work around this by using the exact string if we cannot find a variable name like that(which will minimize possible issues by a lot).

I don't like this. If my password was $BLAH it'll work fine until one day for some stupid reason I'll define an env variable with the same name, then it'll possibly leak my variable? Let's do the simple thing and if a user uses the syntax for an env variable we expect that the variable exists.

[meain]

and the code is not entirely trivial

shellexpand actually lets you expand stuff like $ENV_VAR1 some string $ENV_VAR2 which might not be what we need.

shellexpand("\$FOO")

I seems to error out: while parsing a quoted scalar, found unknown escape character

[osa1]

There should be some way of escaping $s otherwise we simply disallow using $s in passwords. I'm surprised that shellescape doesn't provide this already.

[meain]

Looks like you can use a second $ to escape it. As in you can write $$ILOVEDOLLORSIGN.

One other thing I have noticed is that some programs can differentiate between if it is a var or string based on if there are quotes arround it, but don't think we can do that with yaml.

---

This whole thing feels a bit hacky, maybe we shouldn't have this feature.

[osa1]

I think the only concern is UX -- implementation does not look too bad, and escaping with two $s should be fine. But I wonder if it'll break existing flows. So two concerns:

• This may break existing configs
• Not every user reads the entire user manual/README before using a program so there will be issues like a user having $ in their password have things wrong when trying tiny.

If we had a new syntax for escaped strings it'd be perfect, but not sure if that's possible with YAML.

[meain]

My idea was to expand only if pass was:

• one word
• starts with $
• fits regex [A-z][A-z0-9_]+ (I am guessing this is what bash uses)

This may break existing configs

This will be reduced significantly if we use the specific case with the above three rules. Plus we can give a warning (including how to escape) if we cannot resolve env variable.

Not every user reads the entire user manual/README before using a program so there will be issues like a user having $ in their password have things wrong when trying tiny.

A $ anywhere other than the start is interpreted as normal. Plus warning for starting $ but env variable.

One other option would be have a completely different key if the user wants to use passwords, but that seems like a bit too much.

---

I looked a bit more into shellexpand and it seems to do a lot more. It even expands stuff like ~ to /home/<user> which I don't think is a good idea in our case. This will even substitute if it find $ midway.

[osa1]

My idea was to expand only if pass was: ...

That's too much to explain to a user. A rule I follow basically is; if it's hard to explain that it's probably a bad idea. Hard to explain sometimes means hard to implement too. Bad for code, bad for UX.

I looked a bit more into shellexpand and it seems to do a lot more. It even expands stuff like ~ to /home/ which I don't think is a good idea in our case. This will even substitute if it find $ midway.

Yeah this is not great. I guess we'll have to roll our own implementation if we want this feature.

[meain]

That's too much to explain to a user. A rule I follow basically is; if it's hard to explain that it's probably a bad idea. Hard to explain sometimes means hard to implement too. Bad for code, bad for UX.

What I was going for was if it is a shell variable and there is only one of it.

[osa1]

Hmm, right, that doesn't sound too bad. We could probably say something like "If password is one word and starts with $ it's considered an env variable and looked up in the environment for the actual password. To escape $ use \" or something like that in the documentation.

[meain]

Hi, I have an implementation of what we discussed.

We cannot really use \ for escaping as if I were to give my pass as

pass: "\$FEENODE_PASSWORD"

I will get the following error :

Err(Scan(ScanError { mark: Marker { index: 135, line: 8, col: 10 }, info: "while parsing a quoted scalar, found unknown escape character" }))

We could use $$ for escape but if a users password start with $$ then he will have to use $$$ instead, which might not be apparent to the user. Do you think this might get a bit confusing?

In the case of not finding a shell variable for $DOES_NOT_EXIST we can show a warning, but for pass starting with $$, it might be harder for the user to figure it out.

One option would be to show a message in the messages window when the passwords fails to authenticate and starts with $$

---

Also, might have to change the error message that we are giving right now.

[osa1]

Hi @meain. Would you like to revive this? Few other people also asked for using $HOME in log directory path (log_dir in config). Secondly, @Kabouik mentions in #214

reading env variables would allow conveying the password from pass or secret-tool for instance

I don't use any of these so I don't know how this would work, but if splicing env varibles in strings in the config file will allow that then this seems like a good addition to me.

@Kabouik could you tell more about how you'd use pass or secret-tool if we allowed env variables in strings? Isn't having your password in env also an issue?

@meain spec-wise this is what I have in mind:

• I'm not sure whether we should allow env vars in all strings, or only in some select strings (i.e. passwords and log_dir). Allowing in all strings would not require any special casing and would be easier, and I don't see any problems that this would cause. So perhaps allowing $var syntax in all strings is fine.

• Escaped form of $ is $$. \$ won't work in YAML and \\$ looks worse IMO.

• We allow escaping anywhere, not just in word boundaries. For example if in env I have FOO=bar then the string 123$FOO123 should expand to 123bar123

• If a user has $foo in a string but foo is not in env then we print a warning in "mentions" tab.

• We explain this feature at the beginning of default config.yml file.

Does this make sense to all? cc @Kabouik @trevarj

[Kabouik]

@Kabouik could you tell more about how you'd use pass or secret-tool if we allowed env variables in strings? Isn't having your password in env also an issue?

I'm no expert, but here is what calcurse suggests for pass:

CALCURSE_CALDAV_PASSWORD=$(pass show path/to/calcurse/pass)

Meaning I can set alias calcurse='CALCURSE_CALDAV_PASSWORD=$(pass show path/to/calcurse/pass)' calcurse in my .bashrc and have the password sent to it this way without showing it in plain text files.

And here is what matterhorn does with secret-tool (from the config file):

# Password command. Optional. If this and the password option are both
# missing or give the wrong password, you'll be prompted on startup.
#
# You can also just use
#
# pass: password
#
# but this is much less secure than supplying a command or being
# prompted interactively.
#
# For OS X, the built-in security command can be used to get a password
# from the OS X keychain. To make this work you'll need to put your
# Mattermost password into your keychain as follows:
#
# * Open the Keychain application (Applications -> Utilities -> Keychain
#   Access)
# * Click the "login" keychain.
# * If necessary, click the unlock icon at the top-left corner of the
#   window to unlock keychain access.
# * Click the "+" button at the bottom of the window.
# * Enter a keychain item name (e.g. "mattermost"), your Mattermost
#   username, and your password.
# * Click "Add".
#
# Now the keychain item name can be used with the "security" command
# here:
#
# passcmd: security find-generic-password -s <password name> -w
#
# On Linux, there are a number of options. secret-tool is part of
# libsecret (in libsecret-tools on Ubuntu) and acts as a uniform
# interface to all password managers implementing the LibSecret D-Bus
# API (including kwallet and gnome-keyring). Assuming that you have
# stored a password using
#
# $ secret-tool store --label='matterhorn' matterhorn password
#
# You can then set:
#
passcmd: secret-tool lookup matterhorn password

[osa1]

Hmm, matterhorn seems to support arbitrary commands. If we want to do this with env variables only, would running tiny this way be acceptable?

FREENODE_PASS=`secret-tool lookup ...` tiny

and then the config file would have $FREENODE_PASS in the relevant field.

I wonder why they chose to implement arbitrary command execution rather than just env lookup.

[meain]

Sure, would love to get on this. I am a bit busy these days, so I might not be able to get anything up pretty soon.

I don't know if populating FREENODE_PASS just before would be a good idea as unlike calcure we are dealing with multiple passwords and the alias could end up being really long. However, we could have something like passCmd as a config field which in each server's object which calls pass or any other password manager. If they want to use an env variable, they could always just do echo $FREENODE_PASS. I have seen similar stuff in mutt and mbsync as well.

We have to do the $HOME stuff separately if we are going this route though.

[osa1]

Sure, would love to get on this. I am a bit busy these days, so I might not be able to get anything up pretty soon.

No worries, we're not in a hurry.

Good points regarding allowing commands vs. env variables. It seems like a good idea to allow commands for passwords.

We have more than one way to log in to an IRC server

• Using PASS messages (pass field)
• Via nickserv (nickserv_ident field)
• Via SASL (sasl field)

So we'll have to accept the new command syntax in all of these.

The main difficulty is we need to distinguish a plain text password from a command to run, so that e.g. if a user has

pass: blah

We know whether to run the command blah to get the password, or simply pass blah as the password. Not sure what would be a good syntax for this.

[trevarj]

The main difficulty is we need to distinguish a plain text password from a command to run

Maybe backticks?

pass: `blah`

[osa1]

That looks good but I think it's not valid YAML. Just tried it using http://yaml-online-parser.appspot.com/ , apparently strings can't start with backtick.

[osa1]

Oh, so this isn't valid:

pass: `cmd`

but this is valid:

pass: "`pass`"

Though it also looks worse.

[trevarj]

Oh, yeah, that doesn't look so great. What about making a command anything in a list?

Regular password:
pass: blah

Command returning password:
pass: [blah, arg1]

Or just have the separate field "passCmd" like @meain said.

[osa1]

I'm not a fan of the array syntax for this purpose. I think ideally user would just paste the command that they would use in the shell directly and it would work.

The problem with passCmd is that we have multiple password fields as listed above, we'd need a variant for all of those.

Another idea: pass: $(blah) apparently this is valid YAML.

[meain]

pass: $(...) does not sound like a bad idea. Maybe we could support both $PASS and $(pass cmd).

I did not think about that fact that we have multiple ways to authenticate, it sure might get messy if we have to add cmd versions to each.

[osa1]

I wonder if we could do something simpler though. The problem with $(...) is when parsing we will expect to see ) at the end, and if a user needs parens in the command then we'll have to provide a way to escape characters.

I think a simpler approach would be to add a marker as prefix so that when the string starts with the the rest (regardless of what's in the there) will be the command.

For example

pass: $ cmd

This is valid YAML according to the YAML parser linked above. Parsing this is much simpler: if the first character is $ drop it and the whitespace that follows. The remaining string is the command.

We won't have the ability to splice command results in the middle of a string, e.g.

pass: blah$(...)blah

will no longer run ... and splice the result in the string, but I doubt that's a useful feature. No one will need it probably.

[meain]

What if we add another field called authKind? Then we can only have pass and passCmd? We can support the current ones and maybe even slowly deprecate them as well. By default authKind can be set to password.

[osa1]

I'm not sure if I understand the suggestion correctly. Could you give a concrete
example? Do you mean something like

servers:
    - addr: ...
      auth_kind: pass_cmd
      pass: <the command>

or when using NickServ:

servers:
    - addr: ...
      auth_kind: nickserv_cmd
      auth_pass: <the command>

[meain]

Parsing wise it should be pretty easy, I was just concerned if prefixing with $ might seem unnatural.

[osa1]

$ looks like shell prompt so that's why I suggested that. Any other ideas for the prefix?

[meain]

If we are going with a prefix, $ would be the best option.
One issue could be the that $var in shell and $ command looks kinda similar. I might be overthinking this though.

---

Maybe !command could be an option.

[meain]

Another issue keeping it same is that we will have to provide them a way to escape if they use the same thing for pass.

[osa1]

You mean if the password starts with $? Let's use $$ for that. It's enough to only allow escaping the first $ as the rest never need escaping.

Tbh ! also looks good. What do others think? I'd be happy with either. (for escaping ! we can use !!?) cc @trevarj @Kabouik

[trevarj]

If the command prefix is "$ " (dollar sign + space) do we even have to escape?

[osa1]

So if your password if $blah then you need to escape that $ so that tiny won't run blah as command. I was thinking that the space after $ is optional, but even if it's not then I guess your password could be $ blah and in that case you'll have to escape $ again.

[trevarj]

Yeah, forget what I said. I completely forgot that spaces are valid in passwords

[Kabouik]

My main concern with the $ prefix is actually that it looks like an environment variable, or a shell prompt, people may misinterpret it and remove the space, see it as an environment variable and mess with their .bashrc, or just delete it thinking it's a shell prompt example. So maybe ! or !! is safer. But in any case, nothing a good and clear comment (with an example maybe) in the corresponding section in the tiny config file wouldn't solve.

Note that I am not a programmer, I have no knowledge whatsoever of what @meain's suggestion would imply under the hood, but from a simple user's perspective, it looks cool. Just select the authentication type in a single config section and always format it the same way, instead of having different section names (sasl:, pass:, nickserv_ident:) that may or may not be relevant for different networks. But maybe it's overcomplicated to implement in the code, or not feasible at all. Or maybe in some situations we need to have multiple authentication methods set to have a fallback?

Speaking of sanitizing the config if his suggestion is doable, would it be possible to parse pass: either as plain text or as a command depending on what authentication type is set in auth_kind:? This would allow scrapping pass_cmd and just use pass everywhere. Or, alternatively, pass could be a command in all situations, and we just require the form pass: echo <password> when we just want a plain password. Maybe that's too unusual though.

[osa1]

Came across this blog post about storing secrets in env variables: https://diogomonica.com/2017/03/27/why-you-shouldnt-use-env-variables-for-secret-data/

Seems like supporting arbitrary commands (rather than only env variables) is a good idea.

[meain]

Yeah, I have some not so sensitive passwords in env vars. But most of them are stored in OS keychain or using pass.
Do we have any decision of how the interface should be like in tiny btw?

[osa1]

Are you asking about the config file syntax? I think we can use the ! prefix as discussed above.

[osa1]

@meain let us know if you won't be able to finish this so that we can pick it up. Thanks!

[meain]

Will submit a patch by today or tomorrow. Was a bit busy before.
About the escape, if they have !<space> as the first two chars I am guessing they will have to write the password as !!<space>?

[osa1]

@meain I'm a bit confused. !blah means we will run the command blah and the output will be the password, right? If that's the case we can ignore any whitespace after !. Am I missing something?

[meain]

Oh, OK. I was thinking it was ! blah. Makes sense now.

[meain]

Are we adding an option to use an env variable?

[osa1]

I think we don't need anything special for env variables as you can get value of env variables by running e.g. printenv MY_IRC_PASSWORD.

[meain]

@osa1 Should be good to go now.

[osa1]

Thanks for the update @meain ! I added some inline comments. Again, let us know if you don't have the time or don't want to work on this anymore.

[osa1]

Let's avoid regexes unless absolutely necessary. They're hard to read, understand, maintain. Here's a simpler implementation:

fn is_command(val: &str) -> bool {
    let val = val.trim_start();
    if val.starts_with("!!") {
        false
    } else if val.starts_with('!') {
        true
    } else {
        false
    }
}

Or slightly more efficient:

fn is_command(val: &str) -> bool {
    if val.starts_with('!') {
        // Value starts with '!', it's a command unless '!' is escaped
        if val[1..].starts_with('!') {
            // Escaped '!', not a command
            false
        } else {
            true
        }
    } else {
        // Value doesn't start with '!', not a command
        false
    }
}

Secondly, instead of checking whether the string is a command here and then
doing the same !! check again later, you can do it once. Just introduce a new
type: (feel free to pick another name)

enum CmdOrText<'a> {
    Cmd(&'a str), // NOTE: ! prefix removed
    Text(&'a str),
}

then the check function becomes

fn is_command(val: &str) -> CmdOrText {
    ...
}

[osa1]

I wonder if we run the commands later, when we actually need them, and every time we need them. Reasons:

• If we do this in parse time we'll running the commands for servers we won't be connecting to. E.g. if I run tiny freenode it shouldn't need the command for my OFTC password.
• If I update a password in my password manager currently /connect won't pick it up.

What do you think?

[meain]

@osa1 The second change seems to be a bit more involved and the codebase has changed from the last time I went thought it. If someone else wants to take over, feel free. I might get a bit busy.

[osa1]

We should open an issue for this feature and close the PR.