Skip to content

RIP Esplora, configure bitcoin-d as chain source. #21

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 8, 2024
Merged

RIP Esplora, configure bitcoin-d as chain source. #21

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions server/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ use std::sync::Arc;
fn main() {
let args: Vec<String> = std::env::args().collect();

if args.len() < 6 {
if args.len() < 8 {
eprintln!(
"Usage: {} storage_path listening_addr rest_svc_addr network esplora_server_url",
"Usage: {} storage_path listening_addr rest_svc_addr network bitcoind_rpc_addr bitcoind_rpc_user bitcoind_rpc_password",
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 not take a plaintext password from command line here, please. Let's parse username password from the default .cookie file and let the user supply an override path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That does sound like a good idea, but this needs more thought.

iiuc, the .cookie is ephemeral and is meant for local-only access; it is read-only if we use the same user for bitcoin-d as the LDK daemon.

For remote access, I think the RPC user and pass in bitcoin.conf need to be set.

Similarly, it is equivalent to have an rpc-user and rpc-pass in ldk-server.conf.

Generally, just relying on RPC (pass) isn't a sufficient security measure; most people need to configure bitcoind with secure network access in addition to RPC authentication.

I checked this for both LND and c-lightning; they do it the same way i.e., credentials in a file: https://docs.corelightning.org/reference/lightningd-config.

We should probably switch to rpc-auth instead of rpc-user-password for the bitcoind connection.

Currently, given that the cookie method restricts bitcoin-d usage to local-only, I don't think we should restrict ourselves to that method.

Copy link
Collaborator

@tnull tnull Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Similarly, it is equivalent to have an rpc-user and rpc-pass in ldk-server.conf.

Mhh, I tend to disagree. Supplying these parameters via shell input means that they might get inadvertently distributed across different hosts/history files. IMO, there is a notable difference between reading RPC credentials from (likely hopefully protected by 700 permissions) config file and reading them as arguments, which might leak to other users on the same host through ps or similar.

We should probably switch to rpc-auth instead of rpc-user-password for the bitcoind connection.

Not sure how rpc-auth is different here? It's just the username and the hashed password in a single string, which is exactly the .cookie format?

Copy link
Contributor Author

@G8XSU G8XSU Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Mhh, I tend to disagree. Supplying these parameters via shell input means that they might get inadvertently distributed across different hosts/history files. IMO, there is a notable difference between reading RPC credentials from (likely hopefully protected by 700 permissions) config file

As noted in main PR description, CLI reading is a stop-gap solution until the next PR which introduces read all args from file.
All the CLI options are moving to a config file. Hence it is is equivalent.

Not sure how rpc-auth is different here? It's just the username and the hashed password in a single string, which is exactly the .cookie format?

RPCAuth refers https://github.com/bitcoin/bitcoin/blob/master/share/rpcauth/rpcauth.py , where we configure
`rpcauth=$username:$SALT$hash

It is different from .cookie since .cookie is ephemeral and is meant for local access only.

Afaik, rpc-auth is the preffered method for accessing bitcoind remotely: https://bitcoin.stackexchange.com/a/90420

Copy link

Choose a reason for hiding this comment

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

As noted in main PR description, CLI reading is a stop-gap solution until the next PR which introduces read all args from file. All the CLI options are moving to a config file. Hence it is is equivalent.

Is there a pressing need for this PR? Why not just do the next one instead?

Copy link
Contributor Author

@G8XSU G8XSU Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a pressing need for this PR? Why not just do the next one instead?

I don't see a reason to block this on next one, given they are independent from functionality perspective.
Reading from config is in milestone-3 (#11), and i will have to prioritize it. It also helps me if all the possible config options are checked-in more or less, before i think about config-file schema. Also hoping that it is easier to test with bitcoind than setup local esplora.

Given I have already said that we will move all args to config file, and only take config-file path as input, i assume we can move forward with that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

RPCAuth refers https://github.com/bitcoin/bitcoin/blob/master/share/rpcauth/rpcauth.py , where we configure `rpcauth=$username:$SALT$hash

It is different from .cookie since .cookie is ephemeral and is meant for local access only.

Afaik, rpc-auth is the preffered method for accessing bitcoind remotely: https://bitcoin.stackexchange.com/a/90420

It's literally the same scheme. The .cookie just houses rpcauth token with user __cookie__ and a random password. If we implement reading .cookie files first, we'd get a way to supply rpcauth params from file for free.

In any case, I don't want to hold up this PR over it if we agree that this is a stop-gap solution that will be dropped in the future anyways.

Copy link

Choose a reason for hiding this comment

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

Will defer to y'all, but note we already have code supporting cookie-based auth in LDK sample.

Copy link
Contributor Author

@G8XSU G8XSU Nov 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's literally the same scheme.

The reason the cookie doesn't work for us is that it is ephemeral.
Not really an option for us as default unless we support other methods.

args[0]
);
std::process::exit(-1);
Expand Down Expand Up @@ -59,7 +59,21 @@ fn main() {
};

let mut builder = Builder::from_config(config);
builder.set_chain_source_esplora(args[5].clone(), None);

let bitcoind_rpc_addr = match SocketAddr::from_str(&args[5]) {
Ok(addr) => addr,
Err(_) => {
eprintln!("Failed to parse bitcoind_rpc_addr: {}", args[3]);
std::process::exit(-1);
},
};

builder.set_chain_source_bitcoind_rpc(
bitcoind_rpc_addr.ip().to_string(),
bitcoind_rpc_addr.port(),
args[6].clone(),
args[7].clone(),
);

let runtime = match tokio::runtime::Builder::new_multi_thread().enable_all().build() {
Ok(runtime) => Arc::new(runtime),
Expand Down