Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow cookie to be set via CLI flag or ENV var #189

Closed
lukechilds opened this issue Nov 9, 2019 · 9 comments
Closed

Allow cookie to be set via CLI flag or ENV var #189

lukechilds opened this issue Nov 9, 2019 · 9 comments

Comments

@lukechilds
Copy link
Contributor

In addition, config files support cookie option to specify cookie - this is not available using command line or environment variables for security reasons (other applications could read it otherwise).

This good practise in general, however there are some scenarios where it may be useful to allow setting cookie via CLI flag or ENV var where it's not a security concern.

For example, in an isolated Docker network I just use the credentials rpcuser:rpcpassword. It's easy set between different containers and the RPC port isn't even accessible by the host, it's only internal to the Docker network.

To get this working with electrs I need to create a config file purely to set the cookie which is awkward. It would be great if I could just add ELECTRS_COOKIE=rpcuser:rpcpassword to my docker-compose.yml.

Are you open to allowing this?

@lukechilds
Copy link
Contributor Author

Opened a PR for your convenience if you're open to this: #190

@romanz
Copy link
Owner

romanz commented Nov 9, 2019

@Kixunil (the author of configure_me)
What do you think?

@Kixunil
Copy link
Contributor

Kixunil commented Nov 9, 2019

The reasoning is good (also possibly useful with btc-rpc-proxy), but I don't think we should just allow it without any kind of warning. I suggest to name it --cokie-i-understand-security-implications, but in that case I will need to add renaming feature to configure_me.

I'd really like to help you without introducing footguns for other people, so I have some other ideas.

Is this very important to you or you can live with the config file, at least for a while? I wonder, do you set everything else on command line or everything is default except for the cookie?

Would #176 solve your issue better? (rpcport and rpcpassword are deprecated in Core, BTW)

BTW, I've heard some negative information about Docker security from several independent sources, so you might want to avoid relying on its isolation.

@lukechilds
Copy link
Contributor Author

lukechilds commented Nov 9, 2019

Is this very important to you or you can live with the config file, at least for a while? I wonder, do you set everything else on command line or everything is default except for the cookie?

This is the only config file that's required for my whole full node + electrum + tor + explorer setup. Everything else is configured purely inside my docker-compose.yml with environment variables.

I feel like that is the preferred Docker approach to configuration, but I understand this could be potentially dangerous to non Docker users so you might not want to accept it.

#176 wouldn't help in this regard as it's still file based.

I like the idea of allowing this behaviour but giving a warning to prevent accidental usage when it may be unsafe.

I think --cookie-i-understand-security-implications <cookie> seems a little verbose. What about just allowing --cookie <cookie>, but when used on it's own electrs just exits with a warning that this is potentially unsafe and to run again with --allow-cookie if you understand the risks.

Then running again with --cookie <cookie> --allow-cookie would run as normal.

And same for the ELECTRS_COOKIE environment variable.

Thoughts?

@Kixunil
Copy link
Contributor

Kixunil commented Nov 9, 2019

OK. The way configure_me works currently is it doesn't distinguish between file and command line arguments and I don't feel like adding such machinery.

When I was writing configure_me, I planned to implement renaming, but decided that it'd be code that allows people making inconsistent (confusing) setups, so decided against it. This is a good case for revisiting it.

One alternative trick would be to require the cookie at the end preceded with a parameter without -- prefix. This can already be implemented without changes to electrs. If that seems verbose, maybe write it like this:

electrs ... unsecure <COOKIE HERE>

But that's quite ugly trick.

OTOH, I think argument renaming might not be too hard. Or maybe I could limit it to env var renaming, which might be even easier.

@lukechilds
Copy link
Contributor Author

Yeah you should definitely do whatever you feel is best in a more general sense for configure_me. If argument renaming makes sense then lets go with that.

@lukechilds
Copy link
Contributor Author

Actually now I think about it, #176 could be a nice solution!

I could have a shared volume for the bitcoind data dir between the bitcoind and electrs containers and then point electrs to the cookie file in the shared volume.

@Kixunil
Copy link
Contributor

Kixunil commented Nov 10, 2019

Awesome! I prefer that solution much more, because it solves the issue for other people too. I feel like doing some Open Source stuff now, so I'm going to look at it. :)

Kixunil added a commit to Kixunil/electrs that referenced this issue Nov 10, 2019
This change allows the user to specify a custom cookie file, which is then
used instead of `~/.bitcoin/.cookie`. This resolves situations when the
user wants to have the cookie file in non-standard path.

Aside from that, the code now pre-computes the default path, improving
the performance by avoiding allocation (and copying). Unfortunately, due
to limitations of Rust, the code doesn't print out cookie configuration
anymore. This however might be safer, since the cookie isn't printed,
and thus doesn't end up in some readable logs by accident.

Closes romanz#176
Closes romanz#189
@Kixunil
Copy link
Contributor

Kixunil commented Nov 10, 2019

See the referenced PR. :)

@romanz romanz closed this as completed in 5549287 Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants