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

Support specifying rpcpassword by file #10071

Closed
basvandijk opened this issue Mar 24, 2017 · 6 comments · Fixed by #10267
Closed

Support specifying rpcpassword by file #10071

basvandijk opened this issue Mar 24, 2017 · 6 comments · Fixed by #10267

Comments

@basvandijk
Copy link

Original issue: namecoin/namecoin-core#148.

Currently the rpcpassword must be specified in the configuration file. On some systems (for example in NixOS) configuration files are stored in world-readable locations. It's a bad idea to store secrets in world-readable locations.

To better support these systems it would be nice if there was a rpcpasswordFile configuration parameter. Users can then keep the configuration file in a world-readable location but set rpcpasswordFile to a file with restricted ownership and permissions.

@laanwj
Copy link
Member

laanwj commented Mar 25, 2017

Another (more general) approach, also used in many other daemons, would be to support include statements in configuration files, this would allow including any options from other places besides the main configuration file not just the password.

BTW rpcpassword is deprecated. If you use the newer rpcauth mechanism (introduced in #7044, first in 0.12.0) you can use hashed passwords in the configuration file. This is not a workaround for storing secrets in world-readable locations but makes it non-trivial to recover the cleartext password given access to the configuration file.

@kallewoof
Copy link
Member

Sorry for the spam. Every time I rebase that commit it pings this thread. Wish Github was more intelligent in that regard.

@jnewbery
Copy link
Contributor

jnewbery commented May 2, 2017

@kallewoof - reword the commit message to remove the issue #.

Arguably, commit messages shouldn't contain references to issues, which are a github concept - the git repository should stand alone.

@laanwj
Copy link
Member

laanwj commented May 3, 2017

Arguably, commit messages shouldn't contain references to issues, which are a github concept - the git repository should stand alone.

In a purely ontological sense you are right, but in practice I disagree: I really like to see issue IDs while looking back (though leaving out the # is of course fine) because they allow me to look up the context of something and the discussion around it.

Sorry for the spam. Every time I rebase that commit it pings this thread. Wish Github was more intelligent in that regard.

Yes would be useful if it garbage-collected notifications from no-longer-referenced commits.

@kallewoof
Copy link
Member

In a purely ontological sense you are right, but in practice I disagree: I really like to see issue IDs while looking back (though leaving out the # is of course fine) because they allow me to look up the context of something and the discussion around it.

If I'm not mistaken, referencing an issue in a commit as "fixes <issue#>" will actually auto-close the issue when the PR is merged. That's also handy, I guess.

kallewoof added a commit to kallewoof/bitcoin that referenced this issue Jul 14, 2017
jnewbery pushed a commit to jnewbery/bitcoin that referenced this issue Aug 14, 2017
jnewbery pushed a commit to jnewbery/bitcoin that referenced this issue Aug 14, 2017
@meshcollider
Copy link
Contributor

@kallewoof note that you can include it in the PR OP and not in the commit message and it will work fine without this spam ^ :)

jnewbery pushed a commit to jnewbery/bitcoin that referenced this issue Mar 20, 2018
laanwj added a commit that referenced this issue May 9, 2018
…uration files

25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm)
0f0badd test: Test includeconf parameter. (Karl-Johan Alm)
629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm)

Pull request description:

  Fixes: #10071.

  Done:
  - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file
  - protects against circular includes
  - updates help docs

  ~~~Thoughts:~~~
  - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553
UdjinM6 pushed a commit to UdjinM6/dash that referenced this issue May 21, 2021
… configuration files

25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm)
0f0badd test: Test includeconf parameter. (Karl-Johan Alm)
629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm)

Pull request description:

  Fixes: bitcoin#10071.

  Done:
  - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file
  - protects against circular includes
  - updates help docs

  ~~~Thoughts:~~~
  - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553
UdjinM6 pushed a commit to UdjinM6/dash that referenced this issue May 25, 2021
… configuration files

25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm)
0f0badd test: Test includeconf parameter. (Karl-Johan Alm)
629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm)

Pull request description:

  Fixes: bitcoin#10071.

  Done:
  - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file
  - protects against circular includes
  - updates help docs

  ~~~Thoughts:~~~
  - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553
TheArbitrator pushed a commit to TheArbitrator/dash that referenced this issue Jun 7, 2021
… configuration files

25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm)
0f0badd test: Test includeconf parameter. (Karl-Johan Alm)
629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm)

Pull request description:

  Fixes: bitcoin#10071.

  Done:
  - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file
  - protects against circular includes
  - updates help docs

  ~~~Thoughts:~~~
  - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this issue Apr 30, 2022
… configuration files

25b7ab9 doc: Add release notes for -includeconf (Karl-Johan Alm)
0f0badd test: Test includeconf parameter. (Karl-Johan Alm)
629ff8c -includeconf=<path> support in config handler, for including external configuration files (Karl-Johan Alm)

Pull request description:

  Fixes: bitcoin#10071.

  Done:
  - adds `-includeconf=<path>`, where `<path>` is relative to `datadir` or to the path of the file being read, if in a file
  - protects against circular includes
  - updates help docs

  ~~~Thoughts:~~~
  - ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~

Tree-SHA512: cb31f1b2f69fbc0890d264948eb2e501ac05cf12f5e06a5942f9c1539eb15ea8dc3cae817f4073aecb2fcc21d0386747f14f89d990772003a76e2a6d25642553
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants