test: Verify unwelcome RPC clients are rejected before reading their requests #35866

this is a follow-up PR from a suggestion in this comment

it adds a unit test that confirms that clients not permitted by -rpcallowip are rejected immediately after accept(), before any request bytes are read from the socket

specifically, the test checks that: no request is ever dispatched to the server's request handler, the connection is closed without any response to the client, no HTTPRemoteClient is ever registered for it, and the client's request bytes are left completely unread in the socket's receive buffer

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35866.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline and AI policy for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

concept ACK, more tests are always good ;-) I'm curious to see the corecheck report to see if any new lines are covered

ACK 89353cda701d7704751b922ab545b86865736b16

utACK 89353cda701d7704751b922ab545b86865736b16

tACK 89353cda701d7704751b922ab545b86865736b16

Tested locally on WSL/Ubuntu. Re-built branch using CMake and verified that the new unit test passes successfully (./bin/test_bitcoin -t "httpserver_tests").

This fails on the line BOOST_REQUIRE(server.InitHTTPAllowList()); on corecheck.dev but I'm not really sure why.

Error is: /tmp/bitcoin/src/test/httpserver_tests.cpp:798:26: error: no member named 'InitHTTPAllowList' in 'http_bitcoin::HTTPServer'

@m3dwards corecheck uses git rebase --onto "$BASE_COMMIT" "$PR_MERGE_BASE". I presume to "backport" a pull request to the latest master coverage baseline? If yes, this has several issues:

• It doesn't work with subtree bumps, see also https://corecheck.dev/bitcoin/bitcoin/pulls/35781
• It doesn't generally work when commits are removed from a pull request that are required (as is the case here)

I think the correct approach would be to merge the pull request on top of the latest master coverage.

Of course this could mean that additional coverage is included in the diff, but then one could instead do: When a new push to master happens, schedule a new master coverage task before any pull coverage tasks.

If you want to avoid running a new coverage task on every master commit to reduce load, you could check and optionally schedule a new master coverage only when a pull request is pushed to.

Error is: /tmp/bitcoin/src/test/httpserver_tests.cpp:798:26: error: no member named 'InitHTTPAllowList' in 'http_bitcoin::HTTPServer'

the coverage report on corecheck is using an older master commit as its base: https://github.com/bitcoin/bitcoin/commit/9611a356035be531d62bfc40879f388d5dc359c4. that commit was merged before the changes that introduced HTTPServer::InitHTTPAllowList(). the method was added later in https://github.com/bitcoin/bitcoin/commit/556988790a7f961693a8fd93f73725baea66476a, so when the test is compiled against the older base, HTTPServer does not have that member, which explains the error

about why corecheck is using an older master commit here, i cannot tell why so. other later PRs seem to be using a newer base, though - i know why, see the next comment below

• It doesn't generally work when commits are removed from a pull request that are required (as is the case here)

correct!

i forgot to keep up with the upstream changes from master. i made this PR's branch off of an older base master (https://github.com/bitcoin/bitcoin/commit/556988790a7f961693a8fd93f73725baea66476a) commit on a local fork. the issue is squarely on me. i think a rebase on my local branch would fix this

no, it is not an issue with this pull. The prior commit compiled fine, you can reset to it. the issue is inside corecheck itself.

@maflcko

i had started resetting the branch, but i ran into an unexpected power outage midway and was offline for a while. i noticed the new corecheck runs are almost done now. since resetting the branch would likely trigger another run, do you still think it is worth rolling back to the old commit (89353cda701d7704751b922ab545b86865736b16) ?

Should be fine to reset to avoid re-review. The next corecheck run should be passing as well.

While reviewing I refactored the test to make it simpler, help me understand why it worked. Do you think this is any more clear?

diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index 109446f984..ca4a6df53f 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -807,44 +807,30 @@ BOOST_AUTO_TEST_CASE(http_server_rejects_disallowed_client_before_read)
     std::shared_ptr<DynSock::Pipes> client_pipes{
         ConnectClient(std::as_bytes(std::span(full_request)))};
 
-    // Wait for the socket to close
-    bool disconnected{false};
+    // Wait for the socket to close with EOF (bytes_read == 0)
+    // bytes_read > 0 means the server replied to the prohibited client
+    // bytes_read < 0 is an error which is expected until the socket is served by the I/O loop
+    ssize_t bytes_read;
+    char buf[0x10000] = {};
     for (int attempts{0}; attempts != 1'000; ++attempts) {
-        char byte;
-        const ssize_t bytes_read{client_pipes->send.GetBytes(&byte, sizeof(byte), MSG_PEEK)};
-        if (bytes_read == 0) {
-            disconnected = true;
-            break;
-        }
-        if (bytes_read > 0) {
-            break;
-        }
+        bytes_read = client_pipes->send.GetBytes(&buf, sizeof(buf), MSG_PEEK);
+        if (bytes_read >= 0) break;
         std::this_thread::sleep_for(10ms);
     }
+    BOOST_CHECK_EQUAL(bytes_read, 0);
+    BOOST_CHECK(!request_dispatched);
 
-    const size_t connections{server.GetConnectionsCount()};
+    BOOST_CHECK_EQUAL(server.GetConnectionsCount(), 0);
 
     server.InterruptNet();
     server.JoinSocketsThreads();
-    const bool dispatched{request_dispatched.load()};
-    server.ClearConnectedClients();
     server.StopListening();
 
-    BOOST_CHECK(!dispatched);
-    BOOST_CHECK(disconnected);
-    BOOST_CHECK_EQUAL(connections, 0);
-
     // 'recv' buffer still holds the client's request untouched which
     // proves the server never called Recv()
-    std::string unread_request(full_request.size(), '\0');
     BOOST_REQUIRE_EQUAL(
-        client_pipes->recv.GetBytes(unread_request.data(), unread_request.size()),
+        client_pipes->recv.GetBytes(&buf, sizeof(buf)),
         static_cast<ssize_t>(full_request.size()));
-    BOOST_CHECK_EQUAL(unread_request, std::string{full_request});
-
-    // No extra bytes expected after the recv buffer is drained
-    char extra_byte;
-    BOOST_CHECK_EQUAL(client_pipes->recv.GetBytes(&extra_byte, sizeof(extra_byte)), -1);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

pushed a779bd861a995dc2f3138f080a4d367e7de8795b

1. simplified the test as suggested by @pinheadmz. took the full diff, but retained the request data comparison (now done via std::string_view) because comparing lengths alone confirms that the right amount of data survived unread, while comparing the request data also confirms that it is the same data

rebased to fix this CI issue

ACK d055a3ab1005119befca290eaf7350ed0b42b49d

Built locally, tested and reviewed. Confirmed the test fails if regression introduced back into rpcallowip logic.

<details><summary>Show Signature</summary>

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK d055a3ab1005119befca290eaf7350ed0b42b49d
-----BEGIN PGP SIGNATURE-----

iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmp8jMcbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick6b+AP+gKk11ELLk9TgWeQ36Xw
FJ4+PzDxt2e7gsGXtSAG0R+ygS4GnIO+hyMMNJqPtUJmIrRcTVxi19KKG9mfR7J7
ZumMYr9akIGzSvKgdFkrh/elYFKDDmca/gp+kouY2P/xsewKPoEi6jk7MG1rXX4R
rEN5LK/B6Xw/n1l3c0i/yAuDSRizOLgAv02+Kly6QgdI0KOPzCF5HUaQiXITD1LZ
/zrn7mQOSc3JbwABIsK9Q9zxTXk5z5ERVawmHcSVF/7hEWcWfkZSCKPAUZ2rRdrA
Xy66in+R8bPkHHUqrjqxFoh91Zdziqi4GmX+xS5EDCTjV0/lCjZilhj2S4Xp2wVD
hi9C6dE/SJLVDgtsL2+ho7pyUx/HKSAPkH2A6K76uFh8Gc1QviakjlqBJwihID0X
VJe2gphdMnFYlgsz6DixIfL3X8WJVl7P8MKA+7UqhFy52oUCR21+XTVKcPQsntrW
aY6byrT+CA3/wTrlzgCWbEuZDSHBFmGmTtlkomNF1dTOZRbfZt1h1bR4WVB37wPL
5kCiaWfUF0lmvNkx9YQ8v6vO1sVDveotWFsQzpkIxPPZyPVw5wMhp4IBQE0xbQ+I
//vAB+iVlBPatrElaeimKyR2njijF1OjS6tf3jlHpHejIZgTeUhvms+uyPVEGtr6
Hc+cKJX52ieKFCUhmO2re8rZ
=nUsC
-----END PGP SIGNATURE-----

pinheadmz's public key is on openpgp.org

</details>

reACK d055a3ab1005119befca290eaf7350ed0b42b49d

ACK d055a3ab1005119befca290eaf7350ed0b42b49d