Parse .cookie, .env, or environment variables for RPC auth details #87

alecchendev · 2022-12-20T06:01:10Z

Implements #49

Summary of changes

Modified control flow for getting the RPC auth info to allow for parsing .cookie, .env or environment variables
Added functions get_cookie_path, get_rpc_auth_from_cookie, get_rpc_auth_from_env_vars, get_rpc_auth_from_env_file and parse_env_file
Added helper function parse_rpc_auth
Added unit tests for the added functions and some test data for these tests
Added print_rpc_auth_help to communicate to the user how they can specify credentials in these new ways

Tests: Wasn't sure what the best way to test was; mainly added unit tests for new functions.

Output

These are some of my results from running the sample command for different scenarios:

Explicitly specifying credentials in command (current functionality):

$ cargo run testuser:testpassword@127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
LDK startup successful. Enter "help" to view available commands. Press Ctrl-D to quit.
LDK logs are available at <your-supplied-ldk-data-dir-path>/.ldk/logs
Local Node ID is ****.
> 

# Using .cookie explicitly
$ cargo run $(cat /path/to/bitcoin/folder/regtest/.cookie)@127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
LDK startup successful. Enter "help" to view available commands. Press Ctrl-D to quit.
LDK logs are available at <your-supplied-ldk-data-dir-path>/.ldk/logs
Local Node ID is ****.
>

Without .cookie, .env file or environment variables (should error):

$ cargo run 127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
ERROR: unable to get bitcoind RPC username and password
To provide the bitcoind RPC username and password, you can either:
1. Provide the username and password as the first argument to this program in the format:
   <bitcoind-rpc-username>:<bitcoind-rpc-password>@<bitcoind-rpc-host>:<bitcoind-rpc-port>
2. Provide <bitcoind-rpc-username>:<bitcoind-rpc-password> in a .cookie file in the default bitcoind data directory (automatically created by bitcoind on startup): `/default/path/to/bitcoin/folder`
3. Set the RPC_USER and RPC_PASSWORD environment variables
4. Provide RPC_USER and RPC_PASSWORD fields in a .env file in the current directory

With .cookie in default datadir location for bitcoind:

$ cargo run 127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
LDK startup successful. Enter "help" to view available commands. Press Ctrl-D to quit.
LDK logs are available at <your-supplied-ldk-data-dir-path>/.ldk/logs
Local Node ID is ****.
>

With .env file:

# Sample .env file
RPC_USER=testuser
RPC_PASSWORD=testpassword

$ cargo run 127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
LDK startup successful. Enter "help" to view available commands. Press Ctrl-D to quit.
LDK logs are available at <your-supplied-ldk-data-dir-path>/.ldk/logs
Local Node ID is ****.
>

With environment variables

$ export RPC_USER=testuser RPC_PASSWORD=testpassword
$ cargo run 127.0.0.1:18443 ./ 9732 regtest hellolightning 0.0.0.0
[compilation output...]
LDK startup successful. Enter "help" to view available commands. Press Ctrl-D to quit.
LDK logs are available at <your-supplied-ldk-data-dir-path>/.ldk/logs
Local Node ID is ****.
>

Possible next steps

I'd have to refactor a bit but I don't think it would be too hard to extend this to be able to include other arguments in some sort of general config file as well.

I mentioned it in the issue but I'm fairly new to both Rust and contributing to open-source so please let me know if I can do anything better!

tnull · 2022-12-20T09:01:19Z

Thanks for looking into this!

I think having the option to read the credentials from environment variables is nice, but we still should be able to give them explicitly, if they are not set via the environment variables. Moreover, probably even more useful that allowing to read a .env file would be to be able to read the auth cookie directly coming from bitcoind, as it usually is already available to users. E.g., I can currently supply the credentials pretty easily by just running via

cargo run $(cat /path/to/bitcoin/folder/regtest/.cookie)@127.0.0.1:18443 ldk 9501 regtest

alecchendev · 2022-12-20T19:00:51Z

My bad, forgot to add that explicitly specifying the credentials still works, just added it to the PR description. And that makes sense about using the .cookie being more useful.

For getting the .cookie, I'll try to implement a version that just looks at the default path for the current OS, but I'm wondering is there a way to dynamically find the location of the cookie file or .bitcoin directory? The closest thing I could find is bitcoin-cli -getinfo which returns the location of the debug.log file which would be in wherever they set -datadir to be, but they can also set the location of the log file specifically. Maybe that's getting too into the details.

alecchendev · 2022-12-20T23:59:48Z

Had a bunch of small things to fix and didn't realize force push would make a record of each update and clutter up the log heh... sorry to anyone if you got a bunch of unnecessary emails/notifications because of this.

Anyway, I've now added functionality to parse the .cookie from the default location. It seems Rust doesn't allow me to specify file paths with ~ or %APPDATA% which were present in the default paths I got from the config generator, so I used std::env::home_dir and default paths relative to home. I noticed it's deprecated and people recommend using a separate crate for this functionality, but that seemed like something that would be unnecessary to add a dependency for, and std::env::home_dir seemed to work. Is there a better way to do this?

I've tested this on MacOS and Linux. I'm not sure how it holds up on Windows, and I don't have a Windows machine on hand. If anyone is able to test on Windows that'd be greatly appreciated, otherwise in the meantime I may investigate ways to do it remotely or maybe using docker.

src/cli.rs

dunxen · 2022-12-23T08:33:28Z

Nice one, @alecchendev! Reading the .cookie has been in the back of my mind for a while. Was also considering the best way to find the directory.

Had a bunch of small things to fix and didn't realize force push would make a record of each update and clutter up the log heh... sorry to anyone if you got a bunch of unnecessary emails/notifications because of this.

That's not a problem! Usually we will just add fixup commits during review to address comments and then squash them later before merge.

I've tested this on MacOS and Linux. I'm not sure how it holds up on Windows, and I don't have a Windows machine on hand. If anyone is able to test on Windows that'd be greatly appreciated, otherwise in the meantime I may investigate ways to do it remotely or maybe using docker.

Yeah, Windows is definitely the odd one out here so testing would be nice! I'm in the same boat with no access to Windows.

src/cli.rs

alecchendev · 2022-12-23T18:08:20Z

Thanks for the feedback @dunxen!

That's not a problem! Usually we will just add fixup commits during review to address comments and then squash them later before merge.

Ah, makes sense!

Yeah, Windows is definitely the odd one out here so testing would be nice! I'm in the same boat with no access to Windows.

I'll try and see what I can do about this today and try to leave instructions for any reviewers to reproduce if I figure it out.

dunxen · 2022-12-23T18:38:22Z

I'll try and see what I can do about this today and try to leave instructions for any reviewers to reproduce if I figure it out.

Great stuff! I'll take another look at this PR for anything else. This will probably only merge after the holidays I imagine.

alecchendev · 2022-12-23T21:49:16Z

Okay I tested on Windows and made some changes so it works now. Not really sure how else to show some sort of proof so here are some screenshots:

Testing on Windows

Results

Without any rpc auth provided (error message, note the correct windows filepath):

With bitcoind running (works as it should):

How to reproduce

I was hoping my testing would be more easily reproducible maybe using docker but didn't get that working, and seemed to be a lot more work than I thought. Instead I spun up a windows EC2 instance on AWS and connected to it using remote desktop:

Installed git, rustup (+ visual studio c++ toolchain which rustup does automatically), and bitcoin core
Cloned this repository, ran tests and sample commands

TheBlueMatt

Wow, this is really awesome work! I think it looks basically good to go as-is, with one note, Given we're adding so much contents here IMO it'd be nice to move the existing parse_startup_args and the new code to a new file.

src/cli.rs

TheBlueMatt · 2023-01-03T19:47:22Z

Also sorry for the delay in getting back over the holidays here.

alecchendev · 2023-01-03T22:31:11Z

Wow, this is really awesome work! I think it looks basically good to go as-is, with one note, Given we're adding so much contents here IMO it'd be nice to move the existing parse_startup_args and the new code to a new file.

Thanks! I was thinking cli.rs was getting pretty big, so will do.

TheBlueMatt · 2023-01-03T23:04:54Z

Awesome, thanks. Can you squash this down into one or five logically-distinct commits that don't undo or fixup changes in previous commits? https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits is a good reference if you need it.

alecchendev · 2023-01-03T23:09:49Z

Yes!

TheBlueMatt

This is awesome work, thanks. I'm gonna go ahead and merge with the two trivial nits here, but would appreciate a followup to fix the testnet path, at least.

TheBlueMatt · 2023-01-05T00:32:02Z

src/args.rs

+	};
+
+	let data_dir_path_with_net = match network {
+		Some(Network::Testnet) => data_dir_path.join("testnet"),


IIRC this should be testnet3

TheBlueMatt · 2023-01-05T00:38:23Z

src/args.rs

+	let (bitcoind_rpc_username, bitcoind_rpc_password) = if bitcoind_rpc_info_parts.len() == 1 {
+		get_rpc_auth_from_cookie(None, Some(network), None)
+			.or(get_rpc_auth_from_env_file(None))
+			.or(get_rpc_auth_from_env_vars())


nit: generally env vars should override cookie auth, though I'm not opinionated about env file preference.

alecchendev · 2023-01-05T01:10:33Z

Will do! Will probably get to it sometime in the next couple of days.

alecchendev force-pushed the parse-env branch 5 times, most recently from c467680 to 7c3cb6c Compare December 20, 2022 23:36

alecchendev force-pushed the parse-env branch 2 times, most recently from 10564e3 to 834c790 Compare December 22, 2022 01:45

alecchendev commented Dec 22, 2022

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

alecchendev changed the title ~~Parse .env or environment variables for RPC auth details~~ Parse .cookie, .env, or environment variables for RPC auth details Dec 22, 2022

alecchendev force-pushed the parse-env branch from f60cc4f to 8604cb0 Compare December 22, 2022 22:37

dunxen reviewed Dec 23, 2022

View reviewed changes

src/cli.rs Outdated Show resolved Hide resolved

src/cli.rs Outdated Show resolved Hide resolved

alecchendev force-pushed the parse-env branch from b4a71aa to d840d19 Compare December 23, 2022 18:11

TheBlueMatt reviewed Jan 3, 2023

View reviewed changes

src/cli.rs Show resolved Hide resolved

alecchendev force-pushed the parse-env branch from ca95993 to c905b9a Compare January 3, 2023 23:29

Parse .cookie, .env, or environment variables for RPC auth details

e352827

alecchendev force-pushed the parse-env branch from c905b9a to e352827 Compare January 4, 2023 15:59

TheBlueMatt approved these changes Jan 5, 2023

View reviewed changes

TheBlueMatt merged commit d072085 into lightningdevkit:main Jan 5, 2023

alecchendev mentioned this pull request Jan 5, 2023

Followup: Parse .cookie, .env, or environment variables for RPC auth details #89

Merged

TheBlueMatt mentioned this pull request Aug 13, 2023

Parse .env or environment variables for RPC auth details #49

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parse .cookie, .env, or environment variables for RPC auth details #87

Parse .cookie, .env, or environment variables for RPC auth details #87

alecchendev commented Dec 20, 2022 •

edited

Loading

tnull commented Dec 20, 2022 •

edited

Loading

alecchendev commented Dec 20, 2022 •

edited

Loading

alecchendev commented Dec 20, 2022 •

edited

Loading

dunxen commented Dec 23, 2022

alecchendev commented Dec 23, 2022

dunxen commented Dec 23, 2022

alecchendev commented Dec 23, 2022

TheBlueMatt left a comment

TheBlueMatt commented Jan 3, 2023

alecchendev commented Jan 3, 2023

TheBlueMatt commented Jan 3, 2023

alecchendev commented Jan 3, 2023

TheBlueMatt left a comment

TheBlueMatt Jan 5, 2023

TheBlueMatt Jan 5, 2023

alecchendev commented Jan 5, 2023 •

edited

Loading

Parse .cookie, .env, or environment variables for RPC auth details #87

Parse .cookie, .env, or environment variables for RPC auth details #87

Conversation

alecchendev commented Dec 20, 2022 • edited Loading

Summary of changes

Output

Possible next steps

tnull commented Dec 20, 2022 • edited Loading

alecchendev commented Dec 20, 2022 • edited Loading

alecchendev commented Dec 20, 2022 • edited Loading

dunxen commented Dec 23, 2022

alecchendev commented Dec 23, 2022

dunxen commented Dec 23, 2022

alecchendev commented Dec 23, 2022

Testing on Windows

Results

How to reproduce

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt commented Jan 3, 2023

alecchendev commented Jan 3, 2023

TheBlueMatt commented Jan 3, 2023

alecchendev commented Jan 3, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

TheBlueMatt Jan 5, 2023

Choose a reason for hiding this comment

TheBlueMatt Jan 5, 2023

Choose a reason for hiding this comment

alecchendev commented Jan 5, 2023 • edited Loading

alecchendev commented Dec 20, 2022 •

edited

Loading

tnull commented Dec 20, 2022 •

edited

Loading

alecchendev commented Dec 20, 2022 •

edited

Loading

alecchendev commented Dec 20, 2022 •

edited

Loading

alecchendev commented Jan 5, 2023 •

edited

Loading