fix: increase consistency of rpcauth parsing

tdb3 commented at 1:33 am on July 6, 2024: contributor

The current rpcauth parsing behavior is inconsistent and unintuitive (see #29141 (comment) and additional details below). The current behavior inconsistently treats empty rpcauth as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid rpcauth params.

Empty rpcauth is now consistently treated as an error and prevents bitcoind from starting. Continuation of the upforgrabs PR #29141.

Additional details:

Current rpcauth behavior is nonsensical:

If an empty rpcauth argument was specified as the last command line argument, it would cause all other rpcauth arguments to be ignored.
If an empty rpcauth argument was specified on the command line followed by any nonempty rpcauth argument, it would cause an error.
If an empty rpcauth= line was specified after non-empty rpcauth line in the config file it would cause an error.
If an empty rpcauth= line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were -rpcauth command line arguments and the last one was nonempty, in which case it would cause an error.

New behavior is simple:

If an empty rpcauth config line or command line argument is used it will cause an error

DrahtBot commented at 1:33 am on July 6, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, naiyoma, achow101
Concept ACK	pinheadmz

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo over LogPrintf by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

tdb3 commented at 1:38 am on July 6, 2024: contributor

Behavior Comparison (master (2aff9a36c352640a263e8b5de469710f7e80eb54) vs PR):

CLI:

one valid rpcauth

0src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind starts, rpcauth usable new: no change

one empty rpcauth

0src/bitcoind -regtest -rpcauth
1src/bitcoind -regtest -rpcauth=
2src/bitcoind -regtest -rpcauth=""

current: bitcoind starts new: bitcoind fails, Invalid -rpcauth argument

valid rpcauth, then empty rpcauth

0src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth
1src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth=
2src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth=""

current: bitcoind starts, valid rpcauth ignored new: bitcoind fails, Invalid -rpcauth argument

empty rpcauth, then valid rpcauth

0src/bitcoind -regtest -rpcauth -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1src/bitcoind -regtest -rpcauth= -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
2src/bitcoind -regtest -rpcauth="" -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind fails, Invalid -rpcauth argument new: no change

valid rpcauth, empty rpcauth, then valid rpcauth

0src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth -rpcauth=user2:f639b5ec19359c8a7850c504407eae12\$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408
1src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth= -rpcauth=user2:f639b5ec19359c8a7850c504407eae12\$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408
2src/bitcoind -regtest -rpcauth=user1:6dd184e5e69271fdd69103464630014f\$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b -rpcauth="" -rpcauth=user2:f639b5ec19359c8a7850c504407eae12\$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408

current: bitcoind fails, Invalid -rpcauth argument new: no change

bitcoin.conf:

one valid rpcauth

0# user1/bitcoin
1rpcauth=rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind starts, rpcauth usable new: no change

one empty rpcauth

0rpcauth

current: bitcoind fails, parse error on line X: rpcauth new: no change

0rpcauth=

current: bitcoind starts new: bitcoind fails, Invalid -rpcauth argument

0rpcauth=""

current: bitcoind fails, Invalid -rpcauth argument new: no change

valid rpcauth, then empty rpcauth

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth

current: bitcoind fails, parse error on line X: rpcauth new: no change

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth=

current: bitcoind fails, Invalid -rpcauth argument new: no change

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth=""

current: bitcoind fails, Invalid -rpcauth argument new: no change

empty rpcauth, then valid rpcauth

0rpcauth
1rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind fails, parse error on line X: rpcauth new: no change

0rpcauth=
1rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind starts, valid rpcauth ignored new: bitcoind fails, Invalid -rpcauth argument

0rpcauth=""
1rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b

current: bitcoind fails, Invalid -rpcauth argument new: no change

valid rpcauth, empty rpcauth, then valid rpcauth

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth
2# user2/bitcoin2
3rpcauth=user2:f639b5ec19359c8a7850c504407eae12$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408

current: bitcoind fails, parse error on line X: rpcauth new: no change

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth=
2rpcauth=user2:f639b5ec19359c8a7850c504407eae12$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408

current: bitcoind fails, Invalid -rpcauth argument new: no change

0rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b
1rpcauth=""
2rpcauth=user2:f639b5ec19359c8a7850c504407eae12$bfc7d0b7cd22e4bc2aa4654107db377e979aa21a5ba176869956cfebf804c408

current: bitcoind fails, Invalid -rpcauth argument new: no change

tdb3 commented at 2:12 am on July 6, 2024: contributor

Reviewers can reorder the commits locally (e.g. interactive rebase) or cherry pick to master to see test failure before the fix.

If desired, I could also squash the test commits (they were initially kept separate to delineate specifics being tested).

tdb3 marked this as ready for review on Jul 6, 2024

luke-jr commented at 6:44 pm on July 6, 2024: member

Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

tdb3 commented at 7:10 pm on July 6, 2024: contributor

Does this consider the scenario where bitcoin.conf has valid rpcauth params, but the user specifies -norpcauth or such on the command line to disable them?

Thanks for taking a look. Yes. conf_setup() creates rpcauth statements in node 0’s bitcoin.conf (including for user rt). After restarting the node with -norpcauth, the assert_equal with rt tests that the associated rpcauth statement in the conf is disabled.

https://github.com/bitcoin/bitcoin/blob/0534fa2c216dbd9e1ae43d30a8a406a0a39c409d/test/functional/rpc_users.py#L159-L162

tdb3 force-pushed on Jul 11, 2024

pinheadmz commented at 7:56 pm on July 12, 2024: member

concept ACK, this simplifies a confusing behavior with rpcuser misconfiguration. Code changes look simple enough, will test locally and run through your (very exhaustive!) behavior comparison

luke-jr commented at 7:28 pm on August 2, 2024: member

Tested this. Bare -rpcauth cannot be used to reverse -norpcauth. Since -rpcauth=somethingvalid does reverse -norpcauth (including reinstating -rpcauth params prior to the -norpcauth), this seems like it should be supported?

in test/functional/rpc_users.py:155 in 0534fa2c21 outdated

149         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo:bar'])
150         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo:bar:baz'])
151         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar:baz'])
152         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar$baz'])
153 
154+        # pw = bitcoin

naiyoma commented at 10:20 am on August 5, 2024:

nit: Consider adding logs here, makes it easier to understand the test.

tdb3 commented at 9:41 pm on August 5, 2024:

Thanks. Will add after hearing thoughts on treatment of empty rpcauth after norpcauth.

tdb3 commented at 9:59 pm on August 9, 2024:

Added log statements. Thanks.

naiyoma commented at 11:33 am on August 5, 2024: contributor

Concept ACK. I noticed that https://github.com/bitcoin/bitcoin/pull/29141/commits/6b79de4f9aed365adda0a99d6522e83ccb6c593f included checks for Check -norpcauth can be reversed with -rpcauth and Check -norpcauth followed by a specific -rpcauth= restores config file -rpcauth=* values too. Would it be beneficial to add similar assertions here as well?

tdb3 commented at 9:32 pm on August 5, 2024: contributor

Tested this. Bare -rpcauth cannot be used to reverse -norpcauth. Since -rpcauth=somethingvalid does reverse -norpcauth (including reinstating -rpcauth options prior to the -norpcauth), this seems like it should be supported?

I lean toward empty rpcauth always being treated as an error, but I’m open to it being treated differently (at least on CLI) when at least one valid rpcauth already exists (especially if it is more consistent with the way other options are treated). If I’m in the minority, I could experiment with something like the following.

 0diff --git a/src/httprpc.cpp b/src/httprpc.cpp
 1index 9042378a42..6a2245c556 100644
 2--- a/src/httprpc.cpp
 3+++ b/src/httprpc.cpp
 4@@ -317,10 +317,17 @@ static bool InitRPCAuthentication()
 5 
 6     if (!gArgs.GetArgs("-rpcauth").empty()) {
 7         LogInfo("Using rpcauth authentication.\n");
 8-        for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
 9+        size_t num_empty_rpcauths{0};
10+        std::vector<std::string> rpcauth_args{gArgs.GetArgs("-rpcauth")};
11+        for (const std::string& rpcauth : rpcauth_args) {
12             if (rpcauth.empty()) {
13-                LogError("Invalid (empty) -rpcauth argument\n");
14-                return false;
15+                num_empty_rpcauths++;
16+                if (num_empty_rpcauths == rpcauth_args.size()) {
17+                    LogError("Only invalid (empty) -rpcauth arguments received\n");
18+                    return false;
19+                } else {
20+                    continue;
21+                }
22             }
23             std::vector<std::string> fields{SplitString(rpcauth, ':')};
24             const std::vector<std::string> salt_hmac{SplitString(fields.back(), '$')};

Thoughts?

in src/httprpc.cpp:324 in dc38788312 outdated

320         LogPrintf("Using rpcauth authentication.\n");
321         for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
322+            if (rpcauth.empty()) {
323+                LogError("Invalid (empty) -rpcauth argument\n");
324+                return false;
325+            }

ryanofsky commented at 3:05 pm on August 6, 2024:

In commit “fix: increase consistency of rpcauth parsing” (dc38788312e7cdbbc106103e671d548d5c236862)

I think it would be better for code clarity to drop these 4 lines. The only thing they are doing is triggering a special-case “Invalid (empty) -rpcauth argument” error instead of the more generic “Invalid -rpcauth argument.” error that would already happen below. The generic error seems sufficient, and If it is not sufficient it would still seem better to handle the two errors together rather than separately.

tdb3 commented at 4:06 pm on August 6, 2024:

Thanks. Will do (i.e. handle the errors together)

tdb3 commented at 10:01 pm on August 9, 2024:

Thanks. Dropped the four lines so the errors are handled together. Reordered the commits so the tests are first, then the fix. The tests were tweaked to prevent “test each commit” from failing, then adjusted in the fix commit.

ryanofsky commented at 3:22 pm on August 6, 2024: contributor

re: #30401 (comment)

Since -rpcauth=somethingvalid does reverse -norpcauth (including reinstating -rpcauth options prior to the -norpcauth), this seems like it should be supported?

I think this behavior is surprising and we should not make the code more complicated to support it. It is simpler to just reject empty always, or ignore them always like the previous PR, instead of sometimes accepting or rejecting them based on presence of other values.

For background, the reinstated options are also known as “zombie config values” and are documented in settings code here:

https://github.com/bitcoin/bitcoin/blob/397c6d32c8f8a20a3605ef0d51d159adc21fd125/src/util/settings.cpp#L107-L114

This is also an issue to remove them #17508

ryanofsky approved

ryanofsky commented at 3:28 pm on August 6, 2024: contributor

Code review ACK 0534fa2c216dbd9e1ae43d30a8a406a0a39c409d but I would really encourage reordering commits in this PR so the tests are added first and the fix is the last commit, so the fix commit updates the tests and you can see what effects it has.

DrahtBot requested review from pinheadmz on Aug 6, 2024

DrahtBot requested review from naiyoma on Aug 6, 2024

test: add cases for blank rpcauth ecc98ccff2

test: blank rpcauth CLI interaction

Tests interactions between blank and
non-blank rpcauth args.

67df0dec1a

tdb3 force-pushed on Aug 9, 2024

tdb3 commented at 9:53 pm on August 9, 2024: contributor

Addressed comments from @naiyoma and @ryanofsky. Also rebased.

test: add norpcauth test

Adds test for disabling rpcauth args.

Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>

2ad3689512

tdb3 force-pushed on Aug 9, 2024

DrahtBot commented at 9:58 pm on August 9, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/28588638498

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot added the label CI failed on Aug 9, 2024

DrahtBot removed the label CI failed on Aug 9, 2024

in src/httprpc.cpp:1 in f4b0b69a3e

ryanofsky commented at 0:07 am on August 13, 2024:

In commit “fix: increase consistency of rpcauth parsing” (f4b0b69a3e7df58430cb887028b13265834d8c7e)

I think it would be nice if commit message said explicitly that previous behavior was to sometimes ignore empty -rpcauth= settings, and other times treat them as errors, and now they are consistently treated as errors.

Could also mention the details, but not important

Previous behavior was nonsensical:

If an empty -rpcauth="" argument was specified as the last command line argument, it would cause all other -rpcauth arguments to be ignored.

If an empty -rpcauth="" argument was specified on the command line followed by any nonempty -rpcauth argument, it would cause an error.

If an empty “rpcauth=” line was specified after non-empty rpcauth line in the config file it would cause an error.

If an empty “rpcauth=” line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were -rpcauth command line arguments and the last one was nonempty, in which case it would cause an error.

New behavior is simple:

If an empty “rpcauth=” config line or -rpcauth="" command line argument is used it will cause an error

tdb3 commented at 0:37 am on August 13, 2024:

Thanks. Amended the commit message to mention that the previous rpcauth behavior was inconsistent. Updated the PR description to list detailed behavior.

ryanofsky approved

ryanofsky commented at 0:11 am on August 13, 2024: contributor

Code review ACK f4b0b69a3e7df58430cb887028b13265834d8c7e.

This PR is now just a one line fix accompanied by various tests.

fix: increase consistency of rpcauth parsing

Previous rpcauth behavior was to sometimes
ignore empty -rpcauth= settings, and other times
treat them as errors.
Empty rpcauth is now consistently treated
as an error and prevents bitcoind from starting.
Updates associated test cases.
Also updates to non-deprecated logging macro.

Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

27c976d11a

tdb3 force-pushed on Aug 13, 2024

ryanofsky approved

ryanofsky commented at 2:35 pm on August 13, 2024: contributor

Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior.

I hope this will get reviewed and merged. It is now just a one line fix!

naiyoma commented at 4:14 pm on August 21, 2024: contributor

Tested ACK https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586

Thanks for providing the comprehensive list above. I have tested it, and the output for empty -rpcauth is as expected: bitcoind fails to start, and Invalid -rpcauth argument is printed.

achow101 commented at 6:23 pm on September 4, 2024: member

ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586

achow101 merged this on Sep 9, 2024

achow101 closed this on Sep 9, 2024

fix: increase consistency of rpcauth parsing #30401

Additional details:

Code Coverage

Reviews

Conflicts

Behavior Comparison (master (2aff9a36c352640a263e8b5de469710f7e80eb54) vs PR):

CLI:

bitcoin.conf: