rpc: Validate -rpcauth arguments

promag commented at 12:07 pm on November 23, 2020: member

Invalid -rpcauth arguments are currently silently ignored. This make server initialization fail if any -rpcauth is invalid.

fanquake added the label RPC/REST/ZMQ on Nov 23, 2020

in src/httprpc.cpp:258 in 9860537473 outdated

252@@ -259,6 +253,16 @@ static bool InitRPCAuthentication()
253     if (gArgs.GetArg("-rpcauth","") != "")
254     {
255         LogPrintf("Using rpcauth authentication.\n");
256+        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
257+            std::vector<std::string> fields;
258+            boost::split(fields, rpcauth, boost::is_any_of(":$"));

laanwj commented at 12:08 pm on November 23, 2020:

Do you really need boost::split here? Edit: oh, never mind this is only moved code.

promag commented at 12:09 pm on November 23, 2020:

This is almost move only.

laanwj commented at 12:08 pm on November 23, 2020: member

Concept ACK

As it is a end-user affecting change it needs a brief (1 line) mention in the release notes.

ryanofsky approved

ryanofsky commented at 4:20 pm on November 23, 2020: member

Code review ACK 9860537473b160dfabb033f598848d8a60a339dc. Nice to get this better error handling and test coverage!

MarcoFalke renamed this:
~~qa: Validate -rpcauth arguments~~
rpc: Validate -rpcauth arguments
on Nov 23, 2020

MarcoFalke commented at 4:43 pm on November 23, 2020: member

cr ACK 9860537473b160dfabb033f598848d8a60a339dc

jonatack commented at 5:09 pm on November 23, 2020: member

Concept ACK

promag commented at 6:23 pm on November 23, 2020: member

As it is a end-user affecting change it needs a brief (1 line) mention in the release notes.

done

MarcoFalke commented at 6:26 pm on November 23, 2020: member

cr ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

jonatack commented at 7:30 pm on November 23, 2020: member

ci (unrelated, I suppose, but noting it before it’s restarted)

0163/195 - p2p_fingerprint.py failed, Duration: 2 s
1stdout:
22020-11-23T19:08:44.357000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20201123_184330/p2p_fingerprint_29
32020-11-23T19:08:45.474000Z TestFramework (ERROR): Assertion failed
4Traceback (most recent call last):
5  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 126, in main
6    self.run_test()
7  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_fingerprint.py", line 110, in run_test
8    assert "block" not in node0.last_message
9AssertionError

in doc/release-notes.md:83 in 35838a191f outdated

79@@ -80,6 +80,8 @@ Updated settings
80 
81 Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.
82 
83+- Invalid `-rpcauth` now cause bitcoind to fail to start.  (#20461)

jonatack commented at 7:36 pm on November 23, 2020:

0- Passing an invalid `-rpcauth` argument will now cause bitcoind to fail to start.  (#20461)

in src/httprpc.cpp:256 in 35838a191f outdated

252@@ -259,6 +253,16 @@ static bool InitRPCAuthentication()
253     if (gArgs.GetArg("-rpcauth","") != "")
254     {
255         LogPrintf("Using rpcauth authentication.\n");
256+        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {

jonatack commented at 7:49 pm on November 23, 2020:

I’m not sure if the change from reference to const to by value was deliberate.

0        for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {

MarcoFalke commented at 7:51 pm on November 23, 2020: member

Please don’t restart a build without filing a bug. Either leave the failure, so that it can be found when scraping the logs with automated tools/manually, or file a bug if this is an intermittent failure. See also

If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. ( https://cirrus-ci.com/task/5542990686978048?command=ci#L6034 )

in src/httprpc.cpp:30 in 35838a191f outdated

26@@ -27,6 +27,8 @@
27 /** WWW-Authenticate to present with 401 Unauthorized response */
28 static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
29 
30+static std::vector<std::vector<std::string>> g_rpcauth;

jonatack commented at 7:51 pm on November 23, 2020:

Could add a Doxygen comment.

jonatack commented at 8:04 pm on November 23, 2020: member

Approach ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

rpc: Refactor to process -rpcauth once d37c813a43

rpc: Validate -rpcauth arguments 46001323b1

doc: Release note regarding -rpcauth validation 053b4fbad8

promag force-pushed on Nov 23, 2020

promag commented at 9:19 pm on November 23, 2020: member

Addressed @jonatack review.

DrahtBot commented at 9:42 pm on November 23, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20407 (rpc: Support -rpcauthfile argument by promag)

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.

in doc/release-notes.md:83 in 053b4fbad8

79@@ -80,6 +80,8 @@ Updated settings
80 
81 Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.
82 
83+- Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start.  (#20461)

jonatack commented at 10:48 am on November 24, 2020:

“now cause” should be either “now causes” or “will now cause”

jonatack commented at 10:49 am on November 24, 2020: member

ACK 053b4fbad8729308774d5e7b53f53c12627fa88b

luke-jr changes_requested

luke-jr commented at 0:32 am on November 25, 2020: member

Concept NACK. Additional fields is not necessarily an error.

A debug log line sounds fine.

MarcoFalke commented at 6:59 am on November 25, 2020: member

review ACK 053b4fbad8729308774d5e7b53f53c12627fa88b

promag commented at 9:59 am on November 25, 2020: member

Additional fields is not necessarily an error. @luke-jr what do you suggest? Ignore the whole argument? Just use the the first 3 fields? Not sure about the debug log since it’s a sensitive value.

practicalswift commented at 1:25 pm on November 25, 2020: contributor

Concept ACK: failing swiftly and loudly is better than failing silently

luke-jr commented at 3:25 pm on November 25, 2020: member

what do you suggest? Ignore the whole argument?

Yes, at this startup stage. It will still fail loudly when the user attempts to authenticate later. (Maybe it would make sense to have a special error message returned for users with unrecognised configurations?)

Just use the the first 3 fields?

No, this would potentially grant permissions not intended.

Not sure about the debug log since it’s a sensitive value.

The username isn’t (maybe the line number if it’s a rpcauthfile).

ryanofsky approved

ryanofsky commented at 1:44 am on December 2, 2020: member

Code review ACK 053b4fbad8729308774d5e7b53f53c12627fa88b. Only changes since last review are moving a variable declaration and adding a comment, release notes, and a const.

I think this PR looks good in it’s current form. It seems to me the changes Luke is asking for would make this PR slightly worse. They’d make code more complicated and configuration problems harder to detect because errors would happen at authorization time instead of startup time. They also wouldn’t seem to enable useful behavior because unrecognized options would still be rejected, just at a later time. Maybe Luke you could give an example of when the behavior you’re suggesting might be useful?

I think if it comes down to a choice between Luke’s more limited error reporting or no error reporting at all Luke’s approach would be better. But current PR seems best, as far as I can tell

promag commented at 1:47 am on December 2, 2020: member

I think Luke case is to support same conf between knots and core?

ryanofsky commented at 2:16 am on December 2, 2020: member

I think Luke case is to support same conf between knots and core?

I don’t understand how. What’s the example situation where you would want to use the same configuration for knots and core, but have knots authorize RPC requests which core rejects? What would the requests be? Who would the users be? I can’t think of a situation connected to reality where this would be useful. Just an opinion, but I think it would be good to think about specific situations where this feature could be useful before implementing it.

MarcoFalke merged this on Dec 2, 2020

MarcoFalke closed this on Dec 2, 2020

promag deleted the branch on Dec 2, 2020

promag commented at 9:26 am on December 2, 2020: member

@ryanofsky right, I’m just guessing.

sidhujag referenced this in commit ed39d9ab96 on Dec 2, 2020

luke-jr commented at 3:57 pm on December 2, 2020: member

So users don’t need to swap out their rpcauth config to downgrade. This basically means people using advanced rpcauth features couldn’t run Core on their system anymore without annoying additional steps…

ryanofsky commented at 4:22 pm on December 2, 2020: member

So users don’t need to swap out their rpcauth config to downgrade. This basically means people using advanced rpcauth features couldn’t run Core on their system anymore without annoying additional steps…

Ok IIUC there is no specific situation which causes a problem, just general idea to avoid overly strict validation of config options for more flexibility. I don’t disagree with that and do think this could be a warning instead of an error. This all just seemed like an unlikely point of friction, so I was confused if there was more to explain the change requests and concept nack. Any followup that keeps an early error or warning seems good to me!

promag commented at 5:05 pm on December 2, 2020: member

Might make sense to have a -strict option for those that wish to have and keep clean configurations.

DrahtBot locked this on Feb 15, 2022

rpc: Validate -rpcauth arguments #20461

Conflicts