cli: Detect port errors in rpcconnect and rpcport #29521
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
tdb3
force-pushed
the
20240229_rpcconnectinvalidportdetection
branch
from
6052f49
to
46c91a2
Compare
|
crACK 46c91a2 |
tdb3
force-pushed
the
20240229_rpcconnectinvalidportdetection
branch
from
46c91a2
to
6e779d8
Compare
|
Thanks for the review @davidgumberg! |
There was a problem hiding this comment.
ACK 6e779d8
Testing
Ran ./test/functional/test_runner.py --extended and got two failures feature_index_prune.py and wallet_importdescriptors.py --descriptors. Reran on the base of the PR, 1105aa4 and got both failures, so shouldn't be related. Will investigate further.
Code
I like how you limited the scope of variables introduced in this PR, I wish fewer lines were needed for the added error detection, but I didn't figure out a way to shrink it either.
Some would suggest breaking out to a function, but since it's only used here I agree it's better to inline it.
src/bitcoin-cli.cpp
Outdated
| std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport"); | ||
| if (rpcport_arg.has_value()) { | ||
| std::optional<uint16_t> rpcport_port{ToIntegral<uint16_t>(rpcport_arg.value())}; | ||
| if (!rpcport_port.has_value() || rpcport_port.value() == 0) { | ||
| throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); | ||
| } | ||
|
|
||
| // Use the valid port provided | ||
| port = rpcport_port.value(); |
There was a problem hiding this comment.
Style: narrow scope of rpcport_arg since it is only used inside the if-block. (In this case the outer scope ends it anyway, this is just a general rule of thumb).
Naming: rpcport_port -> rpcport_int since the former is a bit repetitive and the latter at least describes its purpose somewhat, despite being a bit Hungarian.
| std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport"); | |
| if (rpcport_arg.has_value()) { | |
| std::optional<uint16_t> rpcport_port{ToIntegral<uint16_t>(rpcport_arg.value())}; | |
| if (!rpcport_port.has_value() || rpcport_port.value() == 0) { | |
| throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); | |
| } | |
| // Use the valid port provided | |
| port = rpcport_port.value(); | |
| if (std::optional<std::string> rpcport_arg = gArgs.GetArg("-rpcport")) { | |
| std::optional<uint16_t> rpcport_int{ToIntegral<uint16_t>(rpcport_arg.value())}; | |
| if (!rpcport_int.has_value() || rpcport_int.value() == 0) { | |
| throw std::runtime_error(strprintf("Invalid port provided in -rpcport: %s", rpcport_arg.value())); | |
| } | |
| // Use the valid port provided | |
| port = rpcport_int.value(); |
There was a problem hiding this comment.
Good suggestions. I've implemented these changes and added a small comment to increase readability (since operator bool of std::optional is being used).
tdb3
force-pushed
the
20240229_rpcconnectinvalidportdetection
branch
from
6e779d8
to
cb4f9fc
Compare
|
Thank you for the review @cbergqvist! |
There was a problem hiding this comment.
ACK cb4f9fc
Testing Methodolgy:
- Ran make, which was successful.
- Ran functional tests, all passed.
- Tried the following
clicommands in terminal while the daemon was running:
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:notaport help
error: Invalid port provided in -rpcconnect: 127.0.0.1:notaport
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:-1 help
error: Invalid port provided in -rpcconnect: 127.0.0.1:-1
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:65536 help
error: Invalid port provided in -rpcconnect: 127.0.0.1:65536
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=notaport help
error: Invalid port provided in -rpcport: notaport
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcport=-1 help
error: Invalid port provided in -rpcport: -1
➜ bitcoin git:(20240229_rpcconnectinvalidportdetection) ✗ bcli -rpcconnect=127.0.0.1:9000 -rpcport=8332 getblockcount
Warning: Port specified in both -rpcconnect and -rpcport. Using -rpcport 8332
448483
There was a problem hiding this comment.
ACK cb4f9fc
Verified coverage by changing the new C++ code-block to always throw and ran test/functional/interface_bitcoin_cli.py, confirming the exception message, reset to only PR changes and re-ran test/functional/interface_bitcoin_cli.py successfully.
Ran ./test/functional/test_runner.py --extended --jobs=16 --timeout-factor=1 and saw the same unrelated tests error out as on the previous version of this PR. Still investigating.
There was a problem hiding this comment.
Tested ACK. The new error handling code looks good to me upon review. Ran the daemon and gave invalid values for rpcconnect and rpcport and the proper error messages came up. The behavior of port 0 being invalid is consistent with src/init.cpp. Ran interface_bitcoin_cli.py too and there were no issues with the added tests.
Thank you for reviewing, @marcofleon! |
|
ACK cb4f9fc The changes in Tested with I also verified that passing bad ports to |
| # prefer rpcport over default | ||
| assert_equal(BLOCKS, self.nodes[0].cli(f'-rpcport={node_rpc_port}').getblockcount()) | ||
| # use default if nothing else available | ||
| assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:18443", self.nodes[0].cli("-rpcconnect=127.0.0.1").echo) |
There was a problem hiding this comment.
In caa5242 "cli: Sanitize ports in rpcconnect and rpcport"
This will fail if you just have a regtest node running independent of the test:
2024-04-22T22:30:22.751000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 122, in assert_raises_process_error
fun(*args, **kwds)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 821, in __call__
return self.cli.send_cli(self.command, *args, **kwargs)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_node.py", line 886, in send_cli
raise subprocess.CalledProcessError(returncode, self.binary, output=cli_stderr)
subprocess.CalledProcessError: Command '/home/ava/bitcoin/bitcoin/29521/src/bitcoin-cli' returned non-zero exit status 1.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/test_framework.py", line 132, in main
self.run_test()
File "/home/ava/bitcoin/bitcoin/29521/test/functional/interface_bitcoin_cli.py", line 136, in run_test
assert_raises_process_error(1, "Could not connect to the server 127.0.0.1:18443", self.nodes[0].cli("-rpcconnect=127.0.0.1").echo)
File "/home/ava/bitcoin/bitcoin/29521/test/functional/test_framework/util.py", line 127, in assert_raises_process_error
raise AssertionError("Expected substring not found:" + e.output)
AssertionError: Expected substring not found:error: Authorization failed: Incorrect rpcuser or rpcpassword
There was a problem hiding this comment.
Thanks for taking a look and trying it out @achow101. Much appreciated.
Good point. Yes, the test would fail if an independent node is running on 18443.
Although not ideal, do you think this would be reasonable since a warning is provided when bitcoind is running independently? (i.e. WARNING! There is already a bitcoind process running on this system. Tests may fail unexpectedly due to resource contention!)
Alternatively, the assert statement could be removed, but that seems a bit lacking, since it seems good to check for default port usage (even if the current check is limited to the default regtest port).
Could also look into refactoring a bit, but that might lead to touching a bit more code than desirable.
Thoughts?
There was a problem hiding this comment.
I'd say it should be avoided to use test outside of the allowed range assigned to this test case. Each test is assigned 12, or so, p2p and rpc ports. I think this test can be dropped, because if the default behavior changed, pretty much every developer would notice quickly anyway?
There was a problem hiding this comment.
Yes, that sounds reasonable. Overall, port error checking and associated functional test cases introduce an improvement. Removed the "18443" test and pushed.
tdb3
force-pushed
the
20240229_rpcconnectinvalidportdetection
branch
from
cb4f9fc
to
6c17bde
Compare
| assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:notaport", self.nodes[0].cli("-rpcconnect=127.0.0.1:notaport").echo) | ||
| assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:-1", self.nodes[0].cli("-rpcconnect=127.0.0.1:-1").echo) | ||
| assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:0", self.nodes[0].cli("-rpcconnect=127.0.0.1:0").echo) | ||
| assert_raises_process_error(1, "Invalid port provided in -rpcconnect: 127.0.0.1:65536", self.nodes[0].cli("-rpcconnect=127.0.0.1:65536").echo) |
There was a problem hiding this comment.
I added this line and it seemed to pass
assert_raises_process_error(1, 'timeout on transient error: Could not connect to the server 127.0.0.1:::', self.nodes[0].cli("-rpcconnect=127.0.0.1::").echo)
looks like SplitHostPort does not throw an exception if there are two colons and no port provided
I am unsure if that is needed logic in SplitHostPort
I'm seeing the same error when I do
self.nodes[0].cli("-rpcconnect=127.0.0.256")
probably because 255 is the max value
There was a problem hiding this comment.
Thanks @kevkevinpal. Good example of why review is valued. Your findings nudged me to add some assert test statements (which otherwise wouldn't be present) to help cover cases involving IPv6 hosts.
We're experiencing the result of the scope of SplitHostPort(). SplitHostPort() extracts the host and port. Although the port is validated, host is not (and the function declaration comment header mentions validating the port, but doesn't mention validating the host).
bitcoin/src/util/strencodings.h
Lines 97 to 106 in 7fee0ca
SplitHostPort() is treating "127.0.0.1::" as an IPv6 host since "::" is used for IPv6 address shortening. SplitHostPort() detects no port as part of the input string, which would be why Could not connect to the server 127.0.0.1:::18443 is the error provided for src/bitcoin-cli -rpcconnect=127.0.0.1:: getblockcount (the ":18443" is tacked on the end because it's the default port for regtest).
An enhancement would be to perform a validation of the host received from SplitHostPort() before bitcoin-cli attempts to contact the host. I'm thinking we would need to first define what is "valid" and what is "invalid" for use with bitcoin-cli. The gist of this kind of checking seems to be present in CNetAddr. In general, I would think bitcoin-cli shouldn't be overly restrictive in what it lets the user attempt to use as a host.
Lines 157 to 181 in 7fee0ca
For now, it might make the most sense to leave the enhancement of host validation to a new PR since this PR is focused on port error detection.
Adds error handling of invalid ports to rpcconnect and rpcport, with associated functional tests.
Adds a warning when both -rpcconnect and -rpcport define a port to be used.
tdb3
force-pushed
the
20240229_rpcconnectinvalidportdetection
branch
from
6c17bde
to
867a673
Compare
There was a problem hiding this comment.
reACK 867a673
Inspected git range-diff cb4f9fc~2..cb4f9fc 867a673~2..867a673.
Passed --extended functional tests (-feature_dbcrash, excluded, some others skipped due to configuration). Passed make check.
| self.log.info("Check for ipv6") | ||
| have_ipv6 = test_ipv6_local() |
There was a problem hiding this comment.
(IMO it's slightly cleaner to split the test into --ipv4 and --ipv6 variants similar to rpc_bind.py, that way it becomes explicit whether that part of the test coverage is being skipped or not. This being more of a parsing-test, I guess it is not on the critical end of the spectrum. Also it's just a minor part of interface_bitcoin_cli.py so I completely understand the resistance to breaking it out).
Adds invalid port detection to bitcoin-cli for -rpcconnect and -rpcport.
In addition to detecting malformed/invalid ports (e.g. those outside of the 16-bit port range, not numbers, etc.), bitcoin-cli also now considers usage of port 0 to be invalid. bitcoin-cli previously considered port 0 to be valid and attempted to use it to reach bitcoind.
Functional tests were added for invalid port detection as well as port prioritization.
Additionally, a warning is provided when a port is specified in both -rpcconnect and -rpcport.
This PR is an alternate approach to PR #27820 (e.g. SplitHostPort is unmodified).
Considered an alternative to 127.0.0.1 being specified in functional tests, but at first glance, this might need an update to test_framework/util.py (e.g. rpc_url), which might be left to a future PR.