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 configuration via a config file #3332

Closed
pipermerriam opened this Issue Nov 23, 2016 · 16 comments

Comments

Projects
None yet
6 participants
@pipermerriam
Member

pipermerriam commented Nov 23, 2016

What is wrong?

Parity allows configuration via a config file. This is very convenient for a number of reasons.

  • If I consistently run two clients I can set one of them to use a different port by default so that they do not conflict.
  • Certain command line flags are inherently long, such as --bootnodes. Being able to hide these within a config file makes command line interaction easier.
  • Being able to specify which config file to use with something like --config allows for pre-saved configurations.

How could it be implemented

  • Have geth load defaults for cli options from a config file from somewhere like ./data-dir/<config-file>.
  • Anything provided via command line would override values from a config file.
  • Allow specifying a specific config file with something like --config

Cute animal picture

baby-hippo-hippos-24490777-500-667

@pipermerriam

This comment has been minimized.

Member

pipermerriam commented Nov 23, 2016

I'm guessing that there is already an issue for this but I could not find it so... maybe I'm the first to request this?

@fjl

This comment has been minimized.

Contributor

fjl commented Nov 23, 2016

See #2067 for reference.

@fjl

This comment has been minimized.

Contributor

fjl commented Nov 24, 2016

I'm not opposed to a config file btw. The request has come up multiple times in the past and I see how it can be useful. We didn't do it so far because startup scripts are a good enough workaround.

The same goes for the "daemon mode" (#2608) and "log file" (#2646) requests. We have not implemented these because they're not useful with most process managers (launchd, systemd, supervisord, docker, ...) and you can always do nohup geth 2>>geth.log & in the shell
if you don't want to use init system features.

@fjl fjl reopened this Nov 24, 2016

@fjl fjl added the feature label Nov 24, 2016

@fjl

This comment has been minimized.

Contributor

fjl commented Nov 24, 2016

@pipermerriam comment from #2067, to keep the discussion here:

I would like to petition that this be reconsidered as a feature.

@obscuren you say that they tend to bite people in the ass. I don't see why the implementation of a config file cannot be done so that it protects people from the common pitfalls.

If a config value is present in the file that doesn't map to one of the allowed options display a warning or throw an error.
At startup, if a config file was found or used, print out a message indicating as much: "Loaded configuration options /path/to/config.something"
As for maintenance overhead, it has been almost a year since this was first proposed. I understand there is overhead here, but configuration via config file is extremely common and geth has matured a lot in the last 10 months.

@pipermerriam

This comment has been minimized.

Member

pipermerriam commented Nov 24, 2016

Huzzah! Thanks for clarifying @fjl

@obscuren

This comment has been minimized.

Member

obscuren commented Nov 24, 2016

Hippos are fucking dangerous and evil, they are far from cute.

@obscuren

This comment has been minimized.

Member

obscuren commented Nov 24, 2016

Seriously, I still think this is a terrible idea for the same reasons I mentioned then. What's wrong with creating an alias?

Anyways, I'll give it some more thought. But FTR, default configs are out of the question.

EDIT: Hippos are still evil as fuck

@fjl

This comment has been minimized.

Contributor

fjl commented Nov 24, 2016

Would be nice to know some use cases. The format and how geth loads the file is not important for this.

If geth supported a config file:

  • What kinds of options would you put into the file?
  • How would you create the file?
  • Where would you put it on disk?

You already have two examples in the description:

  • --bootnodes are long and you don't want to specify them every time
  • You don't want to assign the listening port via the command line.
@obscuren

This comment has been minimized.

Member

obscuren commented Nov 24, 2016

My main concern with configuration files is that they are error prone and generally dangerous in critical systems and maybe even more so in programs where there's actual value involved.

Having an additional file means that you need to make sure that an additional file is protected (e.g. write protected and maybe even read only by you). There are certainly ways that we can accommodate but we can't help everyone from making silly mistakes.

Here's one such an example that might seem OK at first. Let's assume the following configuration file:

rpcaddr: 127.0.0.1
rpcport: 8080

This is neat, we bind the rpcaddr to localhost on port 8080. This configuration option is extremely dangerous, especially if someone gains access to this "silly" configuration file. rpcaddr 0.0.0.0 is extremely dangerous as it will give anyone on the internet remote access to the RPC and might (if you recall past incidents) swipe your accounts if left open.

I'm sure there are other such scenarios where attacks become plausible, and for what, because we are lazy? Because it's trivial to have configuration files?

Security is way more important.

@pipermerriam

This comment has been minimized.

Member

pipermerriam commented Nov 24, 2016

I'm sorry @obscuren but I don't feel like your argument against config files holds up.

You say that config files are dangerous because an attacker with access can modify something like the rpchost. That same argument applies to your proposed alternative to config files which I believe is either a bash alias (which would likely be in $HOME/.profile), or a shell script. An attacker who can access these files can produce the same harmful results.

There are plenty of critical systems that use config files. I don't know if this helps or hurts my case, but I recall the bitcoin client allowing configuration via config file. Parity has config file support. Every database that I know of has a config file. Most software that I've used supports configuration via a config file and while I understand that you feel it's a security hole or foot gun or something, I think the prevalence of configuration via config file across the software world is evidence to the contrary.

I've made my case for this and said what I have to say. I understand that this may not be a feature that the go team supports and I'll respect that decision. Thank you very much for creating the go ethereum client. It's awesome and I appreciate all the hard work you and the rest of the team have done in creating it.

How about this cute animal????!??!

73406284

@pipermerriam

This comment has been minimized.

Member

pipermerriam commented Nov 24, 2016

@fjl

Where would you put it on disk?

Probably in <default-data-dir>/config.something

@obscuren

This comment has been minimized.

Member

obscuren commented Nov 24, 2016

Hah, much better.

We had some form of priliminary discussion about this. @holiman had a few suggestions and I'll let him elaborate.

@holiman

This comment has been minimized.

Contributor

holiman commented Nov 24, 2016

I usually end up writing a bash-file which contains the flags I want, e.g. start_geth_testnet.sh. But it's a bit pita. I'd prefer to have orderly configuration files.

If we're concerned about protection and erroneous configs or whatever, we could do something like this:

  • Geth is started with a configfile.
  • All configfile options are displayed:
    You're about to launch geth with the following config
    networkId: 3
    bootnodes: [...]

For any security related options, we could print a warning and prompt:

WARNING: This configuration enables RPC over external address
enable? (y/N)

WARNING: This configuration enables admin API over RPC
enable? (y/N)

People may configure strange defaults, but anyone running a 'complex' setup is likely to just shove it in a script anyway. So I think providing warnings and user-prompts may enhance security from where we are now. A bit like providing password managers; the alternatives (easy passwords, password reuse, txt-files) that people will use otherwise are worse.

We could also do it so that security defaults are not even, e.g. setting the rpc to non-localhost isn't even possible, whereas setting chaindata folder maybe should.

@karalabe

This comment has been minimized.

Member

karalabe commented Dec 11, 2016

A proposed implementation #3424

@pipermerriam

This comment has been minimized.

Member

pipermerriam commented Dec 11, 2016

@karalabe this is great and very much inline with what I was looking for.

@fjl

This comment has been minimized.

Contributor

fjl commented Sep 25, 2017

Added in #13875, released in 1.6.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment