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 rpc_user and rpc_pass in config file #1527

Merged
merged 48 commits into from
May 1, 2023

Conversation

raphjaph
Copy link
Collaborator

@raphjaph raphjaph commented Feb 6, 2023

#1399

Probably closes:
#1785
#1562

@raphjaph raphjaph requested a review from casey February 6, 2023 18:07
@raphjaph
Copy link
Collaborator Author

raphjaph commented Feb 6, 2023

Let's not support this as flags but as a option in the config file. https://docs.rs/config/latest/config/

@raphjaph raphjaph marked this pull request as draft February 6, 2023 22:36
@bliotti
Copy link

bliotti commented Feb 7, 2023

PR title says "instead of" we are just adding to.

@bliotti
Copy link

bliotti commented Feb 7, 2023

Tested 37d36ea 👍🏼

rpcuser and rpcpassword will soon be deprecated. ref: JSON-RPC Interface:Security

@raphjaph raphjaph changed the title Allow rpc_user and rpc_pass instead of cookie file Allow rpc_user and rpc_pass in config file Feb 7, 2023
@raphjaph
Copy link
Collaborator Author

raphjaph commented Feb 7, 2023

Yea, using rpc_user and rpc_pass should not be encouraged

@andrewtoth
Copy link
Contributor

andrewtoth commented Feb 10, 2023

rpcuser and rpcpassword will soon be deprecated.

They will not be deprecated from use in clients. The deprecation refers to using bare rpcuser/rpcpassword in favor of generating an rpcauth with the user and password in Bitcoin Core's bitcoin.conf. When configuring bitcoind you should not use rpcuser/rpcpassword but rpcauth, which does not expose the password in plaintext in the config file. For clients rpcuser/rpcpassword will not be deprecated.

@raphjaph
Copy link
Collaborator Author

They will not be deprecated from use in clients. The deprecation refers to using bare rpcuser/rpcpassword in favor of generating an rpcauth with the user and password in Bitcoin Core's bitcoin.conf. When configuring bitcoind you should not use rpcuser/rpcpassword but rpcauth, which does not expose the password in plaintext in the config file. For clients rpcuser/rpcpassword will not be deprecated.

Yes, true. Passing credentials as command line flags is still not ideal because those will be stored in plaintext in your bash history. I've add it as flags anyway because that fear is probably overblown.

@raphjaph
Copy link
Collaborator Author

raphjaph commented Feb 13, 2023

rpc_user and rpc_pass can now be passed in to ord in a plethora of ways. In order of precedence:

  • in the ord.yaml config file
  • as environmental variables
  • as command line options
  • through the cookie file

Generally I think we have to overhaul or configuration setup. I think there should be global Config or Options struct maybe initialized with lazy_static! à la this method. We were creating rpc clients in different places in index.rs that didn't go through our Options methods. These were verifying that the client is and Bitcoin Core are the correct version, etc. This PR also contains some .clone()s in places that might be inefficient.

@raphjaph raphjaph marked this pull request as ready for review February 13, 2023 14:37
@Sjors
Copy link

Sjors commented Feb 18, 2023

In addition to these methods, you could also read rpcuser and rpcpassword from bitcoin.conf, like bitcoin-cli does. For example a user might want to run their bitcoind under a different user than ord, in which case it won't have access to the cookie file, but you can create a bitcoin.conf file with the RPC credentials. (this isn't super useful in practice because Bitcoin Core doesn't have a way yet to limit RPC permissions)

@raphjaph
Copy link
Collaborator Author

In addition to these methods, you could also read rpcuser and rpcpassword from bitcoin.conf, like bitcoin-cli does. For example a user might want to run their bitcoind under a different user than ord, in which case it won't have access to the cookie file, but you can create a bitcoin.conf file with the RPC credentials. (this isn't super useful in practice because Bitcoin Core doesn't have a way yet to limit RPC permissions)

I think rpcuser/rpcpass from bitcoin.conf is being deprecated in favor of rpcauth. This will only store the username, password hash and salt.

@Sjors
Copy link

Sjors commented Feb 23, 2023

Assuming a system with two users: bitcoin and ord. It makes sense for the bitcoin user to use rpcauth so only the hash is stored, which all bitcoind needs. But the ord user would use rpcpass, which is what bitcoin-cli or another client needs. It's a bit confusing that both bitcoind and bitcoin-cli use the same config file. I don't think this will stop working anytime soon.

@AngusP AngusP mentioned this pull request Mar 3, 2023
src/options.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@casey casey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO CLOSE

src/options.rs Show resolved Hide resolved
tests/index.rs Show resolved Hide resolved
@@ -35,3 +35,32 @@ fn re_opening_database_does_not_trigger_schema_check() {
.rpc_server(&rpc_server)
.run();
}

#[test]
fn index_runs_with_rpc_user_and_pass_as_env_vars() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's modify this test to make sure we're using the correct values. Let's record the user and pass which are passed each time a client connects, and then do something like assert_eq!(rpc_server.client_credentials, &[("foo", "bar")]).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the our mock bitcoin core instance is written I do not have access to the Basic Authentication http header. So I can't know what credentials are passed in.

@@ -298,3 +298,50 @@ fn expected_sat_time_is_rounded() {
r".*<dt>timestamp</dt><dd><time>.* \d+:\d+:\d+ UTC</time> \(expected\)</dd>.*",
);
}

#[test]
fn server_runs_with_rpc_user_and_pass_as_env_vars() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do the same style of test in index_runs_with_rpc_user_and_pass_as_env_vars

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

@casey casey merged commit 3204d83 into ordinals:master May 1, 2023
@raphjaph raphjaph deleted the allow-rpc-pass branch May 1, 2023 18:36
popcnt1 pushed a commit to popcnt1/ord that referenced this pull request Jan 11, 2025
Add the ability to authenticate bitcoind RPC calls using a username
and password, as an alternative to cookie file authentication.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants