Merge bitcoin/bitcoin#30401: fix: increase consistency of rpcauth parsing

27c976d11a68d66db97d9a7a30c6a6a71c6ab586 fix: increase consistency of rpcauth parsing (tdb3)
2ad3689512a36eaff957df9ac28e65b2fedbc36c test: add norpcauth test (tdb3)
67df0dec1abe547773e532aa60aff0317e018123 test: blank rpcauth CLI interaction (tdb3)
ecc98ccff25b7e758337e764e59d764076772fec test: add cases for blank rpcauth (tdb3)

Pull request description:

  The current `rpcauth` parsing behavior is inconsistent and unintuitive (see https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1972085251 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

ACKs for top commit:
  naiyoma:
    Tested ACK  [https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586](https://github.com/bitcoin/bitcoin/commit/27c976d11a68d66db97d9a7a30c6a6a71c6ab586)
  achow101:
    ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586
  ryanofsky:
    Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior.

Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b

2 files changed, 22 insertions, 4 deletions

diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 05767a253f..69dd821dc0 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -314,8 +314,9 @@ static bool InitRPCAuthentication()
         LogPrintf("Config options rpcuser and rpcpassword will soon be deprecated. Locally-run instances may remove rpcuser to use cookie-based auth, or may be replaced with rpcauth. Please see share/rpcauth for rpcauth auth generation.\n");
         strRPCUserColonPass = gArgs.GetArg("-rpcuser", "") + ":" + gArgs.GetArg("-rpcpassword", "");
     }
-    if (gArgs.GetArg("-rpcauth", "") != "") {
-        LogPrintf("Using rpcauth authentication.\n");
+
+    if (!gArgs.GetArgs("-rpcauth").empty()) {
+        LogInfo("Using rpcauth authentication.\n");
         for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
             std::vector<std::string> fields{SplitString(rpcauth, ':')};
             const std::vector<std::string> salt_hmac{SplitString(fields.back(), '$')};
diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py
index 44187ce790..49eb64abad 100755
--- a/test/functional/rpc_users.py
+++ b/test/functional/rpc_users.py
@@ -139,15 +139,32 @@ class HTTPBasicsTest(BitcoinTestFramework):
         init_error = 'Error: Unable to start HTTP server. See debug log for details.'
 
         self.log.info('Check -rpcauth are validated')
-        # Empty -rpcauth= are ignored
-        self.restart_node(0, extra_args=['-rpcauth='])
+        self.log.info('Empty -rpcauth are treated as error')
         self.stop_node(0)
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth'])
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth='])
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=""'])
+        self.log.info('Check malformed -rpcauth')
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo'])
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo:bar'])
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo:bar:baz'])
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar:baz'])
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=foo$bar$baz'])
 
+        self.log.info('Check interactions between blank and non-blank rpcauth')
+        # pw = bitcoin
+        rpcauth_user1 = '-rpcauth=user1:6dd184e5e69271fdd69103464630014f$eb3d7ce67c4d1ff3564270519b03b636c0291012692a5fa3dd1d2075daedd07b'
+        rpcauth_user2 = '-rpcauth=user2:57b2f77c919eece63cfa46c2f06e46ae$266b63902f99f97eeaab882d4a87f8667ab84435c3799f2ce042ef5a994d620b'
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=[rpcauth_user1, rpcauth_user2, '-rpcauth='])
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=[rpcauth_user1, '-rpcauth=', rpcauth_user2])
+        self.nodes[0].assert_start_raises_init_error(expected_msg=init_error, extra_args=['-rpcauth=', rpcauth_user1, rpcauth_user2])
+
+        self.log.info('Check -norpcauth disables previous -rpcauth params')
+        self.restart_node(0, extra_args=[rpcauth_user1, rpcauth_user2, '-norpcauth'])
+        assert_equal(401, call_with_auth(self.nodes[0], 'user1', 'bitcoin').status)
+        assert_equal(401, call_with_auth(self.nodes[0], 'rt', self.rtpassword).status)
+        self.stop_node(0)
+
         self.log.info('Check that failure to write cookie file will abort the node gracefully')
         (self.nodes[0].chain_path / ".cookie.tmp").mkdir()
         self.nodes[0].assert_start_raises_init_error(expected_msg=init_error)