http: check rpcallowip immediately after accepting connection #35592

pull pinheadmz wants to merge 2 commits into bitcoin:master from pinheadmz:http-early-allowip changing 8 files +81 −39
  1. pinheadmz commented at 6:07 PM on June 23, 2026: member

    This is a follow-up to #35182 addressing a review comment from that PR: #35182#pullrequestreview-4322490068

    This update to HTTPServer checks the IP subnet allowlist as soon as possible (immediately after receiving a connection from a client) before any data is received. This does not entirely protect the server from the "slow loris" attack or CWE-400 but does restrict the attack surface to localhost and clients explicitly allowed by the user.

    If a client is not allowed by the list, we disconnect as soon as possible. This is a behavior change from master branch (and previous release with libevent) where 403 Forbidden was returned (after a potentially large amount request data was written to memory by the server).

    To facilitate existing unit tests, this commit includes a refactor that moves the subnet allow list and relevant methods into the HTTPServer class instead of static file scope. This is needed because otherwise the allow list would be empty when the unit tests run.

    There is still plenty of refactoring to do in order to modernize HTTPServer and de-globalize it, but since this specific issue has a resource allocation guard, I wanted to open it quickly on its own.

  2. DrahtBot added the label RPC/REST/ZMQ on Jun 23, 2026
  3. DrahtBot commented at 6:07 PM on June 23, 2026: contributor

    <!--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/35592.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    ACK janb84, winterrdog, w0xlt, fjahr
    Stale ACK ViniciusCestarii

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35730 (http: limit connected HTTPRemoteClients by pinheadmz)

    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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. fanquake added this to the milestone 32.0 on Jun 23, 2026
  5. ViniciusCestarii commented at 2:26 PM on June 24, 2026: contributor

    tACK f21b2064c95a5dc5253dc7495101081a590c693f

    Tested this locally and it works correctly. I liked that this also discloses less information to a potential attacker.

    I noticed 2 non-blocking things:

    1. The rejected client loses its diagnostic signal at the bitcoin-cli. Before this change, a blocked IP got a readable error:
    error: server returned HTTP error 403

    After this change, the same case surfaces as a generic connection failure:

    error: Error while attempting to communicate with server 192.168.2.24:18443 (Read error: Connection reset by peer (104))

Make sure the bitcoind server is running and that you are connecting to the correct RPC port.

    I'm fine with the trade. But it might be worth listing -rpcallowip as a possible cause in the bitcoin-cli connection-failure hint. Could be a follow-up.

    1. This is a user-visible behavior change so I believe this should have a release note.
  6. pinheadmz commented at 3:08 PM on June 24, 2026: member

    Thanks @ViniciusCestarii this was mentioned by @b-l-u-e as well. I pushed 429ad3bd29fef33ec5f1b70f386e3846bb18c9a3 which adds a release note.

  7. in src/httpserver.h:392 in 429ad3bd29 
     387 | +    std::vector<CSubNet> m_allow_subnets;
 388 | +
 389 | +    /**
 390 | +     * Check an incoming connection's source IP against the allow list
 391 | +     */
 392 | +    bool ClientAllowed(const CNetAddr& netaddr);
    b-l-u-e commented at 8:29 PM on June 24, 2026:

    nit i think maybe should be const since it only reads m_allow_subnets

    -    bool ClientAllowed(const CNetAddr& netaddr);
+    bool ClientAllowed(const CNetAddr& netaddr) const;
    pinheadmz commented at 2:37 PM on June 30, 2026:

    Good catch! adding const

  8. in src/httpserver.cpp:80 in 429ad3bd29 
      74 | @@ -77,23 +75,24 @@ static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_m
  75 |  static ThreadPool g_threadpool_http("http");
  76 |  static int g_max_queue_depth{100};
  77 |  
  78 | +namespace http_bitcoin {
  79 |  /** Check if a network address is allowed to access the HTTP server */
  80 | -static bool ClientAllowed(const CNetAddr& netaddr)
  81 | +bool HTTPServer::ClientAllowed(const CNetAddr& netaddr)
    b-l-u-e commented at 8:30 PM on June 24, 2026: 
     namespace http_bitcoin {
 /** Check if a network address is allowed to access the HTTP server */
-bool HTTPServer::ClientAllowed(const CNetAddr& netaddr)
+bool HTTPServer::ClientAllowed(const CNetAddr& netaddr) const
 {
     if (!netaddr.IsValid())
         return false;
  9. in src/httpserver.cpp:78 in 429ad3bd29 outdated 
      74 | @@ -77,23 +75,24 @@ static std::vector<HTTPPathHandler> pathHandlers GUARDED_BY(g_httppathhandlers_m
  75 |  static ThreadPool g_threadpool_http("http");
  76 |  static int g_max_queue_depth{100};
  77 |  
  78 | +namespace http_bitcoin {
    b-l-u-e commented at 9:04 PM on June 24, 2026:

    i see duplicate namespace http_bitcoin introduced here maybe move both definitions to existing block namespace http_bitcoin

    pinheadmz commented at 2:38 PM on June 30, 2026:

    I'm going to leave this alone for now because it keeps the diff in this PR smaller. There are more followups to #35182 coming that move more of these static methods inside the HTTPServer class and I think that will be a better time for moving functions around.

    fjahr commented at 10:20 PM on July 30, 2026:

    Hm, if I remember correctly the namespace was only supposed to exist temporarily during the transition. Did you change your mind on that @pinheadmz ?

  10. pinheadmz force-pushed on Jun 30, 2026
  11. pinheadmz commented at 2:39 PM on June 30, 2026: member

    Push to 69a28fe60772f938ab94daff27980aa1b33858f5

    Address feedback from @b-l-u-e adding const to one of the moved methods.

  12. in test/functional/rpc_bind.py:7 in 69a28fe607 
       6 | @@ -7,7 +7,8 @@
   7 |  from test_framework.netutil import all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local
    janb84 commented at 8:32 AM on July 1, 2026: 
    from test_framework.netutil import NETWORK_ERRORS, all_interfaces, addr_to_hex, get_bind_addrs, test_ipv6_local

    NIT combine separate imports from netutil (remove line 10)

    pinheadmz commented at 2:36 PM on July 8, 2026:

    👍

  13. in src/httpserver.cpp:782 in 69a28fe607
    janb84 commented at 9:12 AM on July 1, 2026: 
        // by the ClientAllowed() check.

    NIT: Because of the change the comment "downstream" is not fitting anymore.

    janb84 commented at 12:31 PM on July 1, 2026: 
        // The socket handler reads m_allow_subnets in ClientAllowed(). InitHTTPAllowList()
    // must have populated it first; localhost entries are always added, so an empty
    // list means it was never called and every connection is rejected.
    Assume(!m_allow_subnets.empty());
    m_thread_socket_handler = std::thread(&util::TraceThread,

    NIT: make sure InitHTTPAllowlist is called before starting the socket handler. (This can be a bit to defensive programming.)

    pinheadmz commented at 2:32 PM on July 8, 2026:

    Sure, added this defense the I/O thread initialization.

    pinheadmz commented at 2:35 PM on July 8, 2026:

    right! nice! removing the comment.

  14. in src/httpserver.cpp:93 in 69a28fe607 outdated 
      94 | +bool HTTPServer::InitHTTPAllowList()
  95 |  {
  96 | -    rpc_allow_subnets.clear();
  97 | -    rpc_allow_subnets.emplace_back(LookupHost("127.0.0.1", false).value(), 8);  // always allow IPv4 local subnet
  98 | -    rpc_allow_subnets.emplace_back(LookupHost("::1", false).value());  // always allow IPv6 localhost
  99 | +    m_allow_subnets.clear();
    janb84 commented at 12:31 PM on July 1, 2026: 
        Assume(!m_thread_socket_handler.joinable());
    m_allow_subnets.clear();

    NIT: Make sure InitHTTPAllowList() can only be ran before StartSocketsThreads(). StartSocketsThreads calls ClientAllowed() and calling InitHttpAllowlist afterwards can cause a race condition.

    pinheadmz commented at 2:26 PM on July 8, 2026:

    Good call, adding the assumption with a comment here.

  15. janb84 commented at 12:32 PM on July 1, 2026: contributor

    Concept ACT 69a28fe60772f938ab94daff27980aa1b33858f5

    Thanks for addressing this so fast ! The code changes in this PR will solve my review findings.

    Did code review & security review. Some minor nit-picks and one possible race condition in the comments.

  16. pinheadmz force-pushed on Jul 8, 2026
  17. pinheadmz commented at 2:53 PM on July 8, 2026: member

    push to a89fa837ad0985afd7f2382719c2e08ac393fa8d

    Address feedback from @janb84, some nits, and extra Assume to prevent future regressions

  18. http: check rpcallowip immediately after accepting connection
    Instead of sending 403 Forbidden, disconnect as soon as possible.

To facilitate unit testing, this commit includes a refactor
that moves the subnet allow list and relevant methods
into the HTTPServer class instead of file-scope static scope.
    d1ed2a6e25
  19. doc: add release note describing change for forbidden clients 55d3cd51a4
  20. pinheadmz force-pushed on Jul 8, 2026
  21. DrahtBot added the label CI failed on Jul 8, 2026
  22. DrahtBot commented at 3:30 PM on July 8, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/28952193672/job/85901606969</sub> <sub>LLM reason (✨ experimental): CI failed because ctest aborted on httpserver_tests due to an assertion failure in HTTPServer::StartSocketsThreads() (!m_allow_subnets.empty()).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

    </details>

  23. pinheadmz commented at 3:30 PM on July 8, 2026: member

    push to 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

    • rebase on master and fix silent conflict with #35614 which added another HTTPServer unit test, and therefore also needs a call to InitHTTPAllowList()
  24. DrahtBot removed the label CI failed on Jul 8, 2026
  25. janb84 commented at 5:51 PM on July 8, 2026: contributor

    ACK 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

    Thanks for incorporating my nit suggestions.

  26. winterrdog commented at 9:56 AM on July 30, 2026: contributor

    ACK 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

    the main behavioral change is that clients are now checked against the allowlist immediately after accept(), before any request data is read or parsed. nice low-cost improvement, as disallowed clients can no longer make the server buffer/parse requests before rejection

    the rest of the diff is mostly refactoring: moving the allowlist and helpers (ClientAllowed, InitHTTPAllowList) from static file scope into HTTPServer so state is managed per-server-instance rather than globally

  27. w0xlt commented at 9:13 PM on July 30, 2026: contributor

    ACK 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

    Suggestion for a follow-up PR: a test verifying that disallowed clients are rejected immediately after accept(), before any request bytes are read

    <details> <summary>diff</summary>

    diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
index cd020fc60c..bce800f205 100644
--- a/src/test/httpserver_tests.cpp
+++ b/src/test/httpserver_tests.cpp
@@ -631,6 +631,59 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests)
     server.StopListening();
 }
 
+BOOST_AUTO_TEST_CASE(http_server_rejects_disallowed_client_before_read)
+{
+    // DynSock reports accepted connections as coming from 5.5.5.5.
+    gArgs.ForceSetArg("-rpcallowip", "1.1.1.1");
+
+    std::atomic_bool request_dispatched{false};
+    HTTPServer server{[&request_dispatched](std::unique_ptr<HTTPRequest>&&) {
+        request_dispatched = true;
+    }};
+    BOOST_REQUIRE(server.InitHTTPAllowList());
+
+    CService addr_bind{Lookup("0.0.0.0", /*portDefault=*/0, /*fAllowLookup=*/false).value()};
+    BOOST_REQUIRE(server.BindAndStartListening(addr_bind));
+    server.StartSocketsThreads();
+
+    // Queue a complete request. The server should close this socket immediately after
+    // accept(), without creating an HTTPRemoteClient or reading any request bytes.
+    std::shared_ptr<DynSock::Pipes> mock_client_socket_pipes{
+        ConnectClient(std::as_bytes(std::span(full_request)))};
+
+    bool disconnected{false};
+    for (int attempts{1000}; attempts > 0; --attempts) {
+        char byte;
+        const ssize_t bytes_read{mock_client_socket_pipes->send.GetBytes(&byte, sizeof(byte), MSG_PEEK)};
+        if (bytes_read == 0) {
+            disconnected = true;
+            break;
+        }
+        if (bytes_read > 0) {
+            break;
+        }
+        std::this_thread::sleep_for(10ms);
+    }
+
+    const size_t connections{server.GetConnectionsCount()};
+
+    server.InterruptNet();
+    server.JoinSocketsThreads();
+    const bool dispatched{request_dispatched.load()};
+    server.ClearConnectedClients();
+    server.StopListening();
+
+    BOOST_CHECK(disconnected);
+    BOOST_CHECK_EQUAL(connections, 0);
+    BOOST_CHECK(!dispatched);
+
+    std::string unread_request(full_request.size(), '\0');
+    BOOST_REQUIRE_EQUAL(
+        mock_client_socket_pipes->recv.GetBytes(unread_request.data(), unread_request.size()),
+        static_cast<ssize_t>(full_request.size()));
+    BOOST_CHECK_EQUAL(unread_request, std::string{full_request});
+}
+
 BOOST_AUTO_TEST_CASE(http_socket_error_tests)
 {
     // Create a tiny threadpool for the HTTPRequest handler

    </details>

  28. fjahr commented at 10:23 PM on July 30, 2026: contributor

    Code review ACK 55d3cd51a48d1a8979b2fb542a0e8dc5fe793398

    Change looks good to me, particularly in relation to #35730 .

  29. pinheadmz commented at 7:50 PM on July 31, 2026: member

    @fjahr

    Hm, if I remember correctly the namespace was only supposed to exist temporarily during the transition. Did you change your mind on that @pinheadmz ?

    see #35829

    I think there's a good amount of general cleanup to do still, including creating an HTTPServer::Options and moving more values out of the file-level scope and into the class...

  30. fanquake merged this on Aug 1, 2026
  31. fanquake closed this on Aug 1, 2026
  32. in test/functional/rpc_bind.py:176 in 55d3cd51a4 
     174 | +        # Otherwise we are denied
 175 |          if self.options.usecli:
 176 |              self.log.info("Skip negative IP test with CLI, because the CLI can not throw the tested exception type")
 177 |              return
 178 | -        assert_raises_rpc_error(-342, "non-JSON HTTP response with '403 Forbidden' from server", self.run_allowip_test, ['1.1.1.1'], self.non_loopback_ip, self.defaultport)
 179 | +        assert not self.run_allowip_test(['1.1.1.1'], self.non_loopback_ip, self.defaultport)
    maflcko commented at 6:17 AM on August 2, 2026:

    Hmm, on a macOS-cross build, I still get:

    test  2026-08-01T16:41:53.495475Z TestFramework.node0 (DEBUG): RPC successfully started 
 node0 2026-08-01T16:41:53.495915Z [http] [httpserver.cpp:834] [NewSockAccepted] [http] HTTP Connection accepted from 192.168.64.6:50331 (id=1) 
 node0 2026-08-01T16:41:53.495932Z [http] [httpserver.cpp:1025] [MaybeDispatchRequestsFromClient] [http] Received a POST request for / from 192.168.64.6:50331 (id=1) 
 node0 2026-08-01T16:41:53.495939Z [http] [httpserver.cpp:133] [MaybeDispatchRequestToWorker] [http] HTTP request from 192.168.64.6:50331 rejected: Client network is not allowed RPC access 
 node0 2026-08-01T16:41:53.495948Z [http] [httpserver.cpp:616] [WriteReply] [http] HTTPResponse (status code: 403 size: 127) added to send buffer for client 192.168.64.6:50331 (id=1) 
 node0 2026-08-01T16:41:53.495964Z [http] [httpserver.cpp:1185] [MaybeSendBytesFromBuffer] [http] Sent 127 bytes to client 192.168.64.6:50331 (id=1) 
 test  2026-08-01T16:41:53.496091Z TestFramework (ERROR): Unexpected exception: 
                                   Traceback (most recent call last):
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/test_framework/test_framework.py", line 145, in main
                                       self.run_test()
                                       ~~~~~~~~~~~~~^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/rpc_bind.py", line 134, in run_test
                                       self._run_nonloopback_tests()
                                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/rpc_bind.py", line 176, in _run_nonloopback_tests
                                       assert not self.run_allowip_test(['1.1.1.1'], self.non_loopback_ip, self.defaultport)
                                                  ~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/rpc_bind.py", line 77, in run_allowip_test
                                       node.getnetworkinfo()
                                       ~~~~~~~~~~~~~~~~~~~^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/test_framework/coverage.py", line 50, in __call__
                                       return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/test_framework/authproxy.py", line 128, in __call__
                                       response, status = self._request('POST', self.__url.path, postdata.encode('utf-8'))
                                                          ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/test_framework/authproxy.py", line 102, in _request
                                       return self._get_response()
                                              ~~~~~~~~~~~~~~~~~~^^
                                     File "/Users/runner/work/bitcoin-core-nightly/bitcoin-core-nightly/test/functional/test_framework/authproxy.py", line 188, in _get_response
                                       raise JSONRPCException(
                                           {'code': -342, 'message': f"non-JSON HTTP response with \'{http_response.status} {http_response.reason}\' from server: {http_response.read().decode()}"},
                                           http_response.status)
                                   test_framework.util.JSONRPCException: non-JSON HTTP response with '403 Forbidden' from server:  (-342)  [http_status=403]
    winterrdog commented at 9:28 AM on August 2, 2026:

    i do not think the rpc_bind.py failure is actually a bug in the PR. it looks more like your test and binary are not in sync

    your logs show the request making it to NewSockAccepted, getting parsed as a POST, and then being rejected in MaybeDispatchRequestToWorker with a 403 Forbidden. that is the old behavior, where the permission check happens in MaybeDispatchRequestToWorker and the server replies with a 403

    this PR strips out that path entirely. with the changes applied, a client that is not allowed should be rejected much earlier in AcceptConnection, so it MUST never reach NewSockAccepted in any scenario, otherwise that would be a bug requiring a fix

    so, it looks like your test files are newer than the bitcoind binary you are actually running

    <details> <summary>what i got locally </summary>

    $ ./build/test/functional/test_runner.py rpc_bind.py

Temporary test directory at /tmp/test_runner_₿_🏃_20260802_120943
Remaining jobs: [rpc_bind.py --ipv4, rpc_bind.py --ipv6, rpc_bind.py --nonloopback]
1/3 - rpc_bind.py --nonloopback passed, Duration: 5 s
Remaining jobs: [rpc_bind.py --ipv4, rpc_bind.py --ipv6]
2/3 - rpc_bind.py --ipv4 passed, Duration: 6 s
Remaining jobs: [rpc_bind.py --ipv6]
3/3 - rpc_bind.py --ipv6 passed, Duration: 10 s

TEST                      | STATUS    | DURATION

rpc_bind.py --ipv4        | ✓ Passed  | 6 s
rpc_bind.py --ipv6        | ✓ Passed  | 10 s
rpc_bind.py --nonloopback | ✓ Passed  | 5 s

ALL                       | ✓ Passed  | 21 s (accumulated)
Runtime: 10 s

    </details>

    maflcko commented at 9:50 AM on August 2, 2026:

    Ah, sorry. Yeah, I forgot to freeze the commit.

  33. Kino1994 referenced this in commit 37c6a556c6 on Aug 2, 2026
Contributors
pinheadmzDrahtBotViniciusCestariib-l-u-efjahrjanb84winterrdogw0xltmaflcko
Labels
RPC/REST/ZMQ

Milestone
32.0

Linked (view graph)
#35182 Replace libevent with our own HTTP and socket-handling implementation#35866 test: Verify unwelcome RPC clients are rejected before reading their requests
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-08-14 17:51 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me