test: Add coverage for Tor control `HASHEDPASSWORD` authentication #35292

winterrdog commented at 6:28 PM on May 14, 2026: contributor

this is a tests-only PR aimed at adding functional test coverage for Tor control HASHEDPASSWORD authentication.

currently, the functional test suite does not explicitly explore this authentication path, which means regressions in tor authentication handling could go unnoticed. for instance incorrectly formatted AUTHENTICATE commands, broken fallback behavior, or authentication attempts being made when no password is configured.

The 4 tests herein extend the existing mock tor control server to simulate METHODS=HASHEDPASSWORD responses and cover successful authentication with the correct password, failing with an incorrect password, behavior when -torpassword is not set, and cases where the server does not choose to advertise HASHEDPASSWORD as a way of authenticating.

tested with the test harness:

./build/test/functional/test_runner.py feature_torcontrol \
  --loglevel=debug --failfast

DrahtBot added the label Tests on May 14, 2026

DrahtBot commented at 6:28 PM on May 14, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/35292
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/35292.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/35292#author-ralph

Reviews

See the guideline and AI policy for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36374 (torcontrol: escape backslashes in -torpassword by 0xShadowX)
#36142 (net: validate Tor onion service replies and cached keys by l0rinc)
#34486 (net: Reduce local network activity when networkactive=0 by willcl-ark)

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.

sedited commented at 6:42 PM on May 14, 2026: contributor

@winterrdog can you re-write the description in your own words without following this LLM-formulaic format? This would give reviewers more confidence that you understand the change.

winterrdog commented at 6:59 PM on May 14, 2026: contributor

re-write the description in your own words without following this LLM-formulaic format?

thanks! done edition (seems like i followed the wrong examples online).

DrahtBot added the label CI failed on May 14, 2026

DrahtBot commented at 7:44 PM on May 14, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/25877900203/job/76052468485</sub> <sub>LLM reason (✨ experimental): CI failed due to Python lint errors from ruff (F541: extraneous f-string prefixes) in test/functional/feature_torcontrol.py.</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>

winterrdog force-pushed on May 14, 2026

DrahtBot removed the label CI failed on May 14, 2026

in test/functional/feature_torcontrol.py:303 in f886aa2434

 298 | +        self.wait_until(lambda: len(mock_tor.received_commands) >= 2, timeout=10)
 299 | +        assert_equal(mock_tor.received_commands[0], "PROTOCOLINFO 1")
 300 | +        assert_equal(mock_tor.received_commands[1], 'AUTHENTICATE "wrong_password"')
 301 | +        # After auth failure, no further commands should be sent and authentication failure should be logged
 302 | +        ensure_for(duration=2, f=lambda: len(mock_tor.received_commands) == 2)
 303 | +        self.wait_until(lambda: any("Authentication failed" in line for line in self.nodes[0].debug_log_path.read_text().splitlines()))

davidgumberg commented at 11:23 PM on May 14, 2026:

What is this testing separately from the correct password test?

winterrdog commented at 8:28 PM on May 15, 2026:

this log check was an extra step i added to fully confirm that the error was actually noticed and reported instead of being silently swallowed. it was not needed in the correct password test since there we already verify that Bitcoin Core continues past authentication by checking that more than 2 commands (AUTHENTICATE and PROTOCOLINFO - the initial commands) are sent.

here comes the long story. let's assume the correct password is the happy path and the wrong password is not.

so in the happy path (correct password), this is what happens in Bitcoin Core:

correct password sent
    -> tor daemon returns `250 OK`
        -> `TorController::auth_cb` hits the `if` branch & continues on to `GETINFO` and `ADD_ONION`
            -> connection stays active

now for the sad path (wrong password):

incorrect password sent
    -> tor daemon returns `515 Bad authentication`
        -> `TorController::auth_cb` hits the `else` branch
            -> `"tor: Authentication failed"` is logged
                -> no further commands are sent
                    -> connection effectively stops

the key difference here is that the wrong password test verifies Bitcoin Core logs and stops trying to do anything else when authentication fails, which is not the case with a correct password as you saw above.

did you have something different in mind ?

winterrdog requested review from davidgumberg on May 19, 2026

winterrdog force-pushed on Jun 4, 2026

sedited requested review from vasild on Jun 8, 2026

winterrdog force-pushed on Jun 9, 2026

winterrdog force-pushed on Aug 3, 2026

winterrdog force-pushed on Aug 4, 2026

DrahtBot added the label CI failed on Aug 4, 2026

DrahtBot removed the label CI failed on Aug 4, 2026

DrahtBot added the label Needs rebase on Sep 16, 2026

winterrdog force-pushed on Sep 18, 2026

DrahtBot removed the label Needs rebase on Sep 18, 2026

0xShadowX commented at 8:16 PM on September 28, 2026: none

#36374 fixes -torpassword values containing a backslash, which aren't escaped in AUTHENTICATE. It adds a small HASHEDPASSWORD mock to feature_torcontrol.py that overlaps with yours. Would you like to add an escaping case (e.g. pa\ss"word\) here, and I'll drop the test from #36374 and keep just the fix? Or I can rebase onto this once it's merged.

winterrdog commented at 1:05 PM on October 3, 2026: contributor

@0xShadowX

that is a valid catch you got going on in #36374. backslashes are not escaped on present master. i verified that this is also required by the tor control spec:

In QuotedStrings, backslashes and quotes must be escaped; other characters need not be escaped.

from https://spec.torproject.org/control-spec/message-format.html#description-format.

anyway, i see 2 ways we can handle this:

wait for this PR to be merged into mainline, then rebase #36374 on top of it
combine both PRs into one, with three commits:
- all tests that came before #36374
- a test verifying the backslash fix
- the backslash fix in src/torcontrol.cpp

for the second & third commit, i can add you as a co-author. so #36374 effectively becomes part of this PR's shadow. essentially, it comes down to whether we want to keep the PRs separate and rebase later, or combine them now

my take: option 1

what do you propose ?

winterrdog force-pushed on Oct 3, 2026

winterrdog commented at 2:42 PM on October 3, 2026: contributor

pushed changes:

took Ralph's suggestion here: https://git.fish.foo/bitcoin/bitcoin/pulls/35292#issuecomment-607942 -- now, we wait for the mock's PROTOCOLINFO/AUTHENTICATE exchange inside the assert_debug_log context and give it a 10s timeout so that the "Authentication failed" check no longer races the 515 response (less likely to trigger in practice, but cheap to make robust)

P.S: looks like the Ralph bot edited the original message. so, you might have to look for the edit just below the latest edition it made on the comment

0xShadowX commented at 2:56 PM on October 3, 2026: none

Thanks for checking it against the spec. Option 1 sounds good to me, keeping them separate makes each one easier to review.

One small adjustment: since #36374 carries its own minimal mock test, it doesn't strictly need to wait for this one. Whichever gets merged first, I'll rebase the other on top and fold the overlapping mock code into yours, so the escaping case ends up as a test in your HASHEDPASSWORD coverage. Does that work for you?

winterrdog commented at 3:50 PM on October 3, 2026: contributor

I'll rebase the other on top and fold the overlapping mock code into yours, so the escaping case ends up as a test in your HASHEDPASSWORD coverage. Does that work for you?

alright!

then, your PR becomes the C++ fix plus one subcase in test_hashedpassword_auth. my mock currently checks if command != f'AUTHENTICATE "{test_password}"', so i would have to make that configurable (by adding a member like self.expected_auth in HashedPasswordServer, defaulting to the test_password) and your subcase can just set it to the escaped command in test_hashedpassword_auth:

diff --git a/test/functional/feature_torcontrol.py b/test/functional/feature_torcontrol.py
index da16f42065..1faff71f4b 100755
--- a/test/functional/feature_torcontrol.py
+++ b/test/functional/feature_torcontrol.py
@@ -306,6 +306,19 @@ class TorControlTest(BitcoinTestFramework):
         assert_equal(mock_tor.received_commands[1], f'AUTHENTICATE "{test_password}"')
         mock_tor.stop()

+        self.log.info("Test that -torpassword is sent as a correctly escaped quoted string")
+        escaped_auth = 'AUTHENTICATE "pa\\\\ss\\"word\\\\"'
+        mock_tor = HashedPasswordServer(self.next_port(), expected_auth=escaped_auth)
+        mock_tor.start()
+        self.restart_node(0, extra_args=[
+            f"-torcontrol=127.0.0.1:{mock_tor.port}",
+            '-torpassword=pa\\ss"word\\',
+        ] + base_args)
+        mock_tor.conn_ready.wait(timeout=10)
+        self.wait_until(lambda: len(mock_tor.received_commands) >= 2, timeout=10)
+        assert_equal(mock_tor.received_commands[1], escaped_auth)
+        mock_tor.stop()
+
         self.log.info("Test that an incorrect password produces authentication failure with HASHEDPASSWORD authentication")
         mock_tor = HashedPasswordServer(self.next_port())
         mock_tor.start()

but, in the event that yours goes first, i will rebase, fold your mock into mine, and move your escaping test into test_hashedpassword_auth as a subcase. whichever way it goes, i am fine with it

winterrdog force-pushed on Oct 3, 2026

winterrdog commented at 5:12 PM on October 3, 2026: contributor

so i would have to make that configurable (by adding a member like self.expected_auth in HashedPasswordServer, defaulting to the test_password)

done in the latest push

winterrdog commented at 5:51 PM on October 3, 2026: contributor

@0xShadowX

after some thought, i think your PR can go in first. it is lighter and straightforward to review than this one

in #35292 (comment)

but, in the event that yours goes first, i will rebase, fold your mock into mine, and move your escaping test into test_hashedpassword_auth as a subcase.

with the PR's current state, i think making this change in order to incorporate your new test case into this PR is trivial to do

test: Add TorControl `HASHEDPASSWORD` authentication coverage

add functional test coverage for `HASHEDPASSWORD` authentication,
including a correctly quoted password, an incorrect password, an unset
`-torpassword`, and a TOR server that does not advertise
`HASHEDPASSWORD`

153baf0b36

winterrdog force-pushed on Oct 4, 2026