init: Remove unnecessary sensitive flag from rpcbind #26829

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:dont-hide-rpcbind-log changing 2 files +5 −3

achow101 commented at 3:39 AM on January 6, 2023: member

-rpcbind is currently flagged as a sensitive option which means that its value will be masked when the command line args are written to the debug.log file. However this is not useful as if rpcbind is actually activated, the bound IP addresses will be written to the log anyways. The test feature_config_args.py did not catch this contradiction as the test node was not started with rpcallowip and so rpcbind was not acted upon.

This also brings rpcbind inline with bind as that is not flagged as sensitive either.
DrahtBot commented at 3:39 AM on January 6, 2023: contributor



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



Reviews

See the guideline for information on the review process.

Type Reviewers

ACK theStack, Sjors, willcl-ark

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.



Conflicts

No conflicts as of last run.
achow101 force-pushed on Jan 7, 2023
Sjors commented at 1:32 PM on January 19, 2023: member

utACK c5fbea97c020df43907ca648c050215c9b20dfc9

That said, for a followup it could make sense to go the other direction and make sure we don't log the user IP address anywhere. For IPv4 -(rpc)bind is typically just a 192.168.x.x address, but for IPv6 it's a global address.
in test/functional/feature_config_args.py:147 in c5fbea97c0 outdated
```
 141 |              self.start_node(0, extra_args=[
 142 |                  '-addnode=some.node',
 143 |                  '-rpcauth=alice:f7efda5c189b999524f151318c0c86$d5b51b3beffbc0',
 144 |                  '-rpcbind=127.1.1.1',
 145 | +                '-rpcbind=127.0.0.1',
 146 | +                "-rpcallowip=127.0.0.1",
```
theStack commented at 12:29 AM on January 23, 2023:

Are those additions left-overs from local testing? I think keeping -rpcbind/-rpcallowip would only make sense if they would somehow appear in expected_msgs/unexpected_msgs (could add e.g. rpcbind=**** on unexpected_msgs to check that it is not sensitive anymore), otherwise the test coverage is not increased in any way.

achow101 commented at 10:25 PM on January 23, 2023:

Added them to unexpected_msgs.
theStack commented at 12:30 AM on January 23, 2023: contributor

Concept ACK
init: Remove sensitive flag from rpcbind b9d5674541
achow101 force-pushed on Jan 23, 2023
theStack approved
theStack commented at 10:55 PM on January 23, 2023: contributor

ACK b9d567454159f062ce84353f5821d6e6daf433bd
Sjors commented at 9:27 AM on January 24, 2023: member

re-utACK b9d567454159f062ce84353f5821d6e6daf433bd

Running the old code against the new test triggers the unexpected argument error, as expected.
fanquake requested review from willcl-ark on Jan 25, 2023
willcl-ark commented at 2:29 PM on January 25, 2023: contributor

ACK b9d5674

Looks like it was added in b951b0973cfd4e0db4607a00d434a04afb0d6199 as part of writing config options to debug.log.

FWIW I was just working on #26964 which ensures that we don't create the RPC cookie before we've bound to the server. As long as we don't do that, there seems little danger in having the bound port and address exposed in the log messages. We should not rely on port obscurity for security.
willcl-ark approved
maflcko merged this on Jan 25, 2023
maflcko closed this on Jan 25, 2023
sidhujag referenced this in commit e0a3d1bd6a on Jan 25, 2023
bitcoin locked this on Jan 25, 2024

Type	Reviewers
ACK	theStack, Sjors, willcl-ark

Contributors

init: Remove unnecessary sensitive flag from rpcbind #26829

Reviews

Conflicts