rpc: Validate -rpcauth arguments #20461

pull promag wants to merge 3 commits into bitcoin:master from promag:2020-11-validate-rpcauth changing 3 files +24 −11
  1. promag commented at 12:07 PM on November 23, 2020: member

    Invalid -rpcauth arguments are currently silently ignored. This make server initialization fail if any -rpcauth is invalid.

  2. fanquake added the label RPC/REST/ZMQ on Nov 23, 2020
  3. in src/httprpc.cpp:258 in 9860537473 outdated 
     252 | @@ -259,6 +253,16 @@ static bool InitRPCAuthentication()
 253 |      if (gArgs.GetArg("-rpcauth","") != "")
 254 |      {
 255 |          LogPrintf("Using rpcauth authentication.\n");
 256 | +        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
 257 | +            std::vector<std::string> fields;
 258 | +            boost::split(fields, rpcauth, boost::is_any_of(":$"));
    laanwj commented at 12:08 PM on November 23, 2020:

    Do you really need boost::split here? Edit: oh, never mind this is only moved code.

    promag commented at 12:09 PM on November 23, 2020:

    This is almost move only.

  4. laanwj commented at 12:08 PM on November 23, 2020: member

    Concept ACK

    As it is a end-user affecting change it needs a brief (1 line) mention in the release notes.

  5. ryanofsky approved
  6. ryanofsky commented at 4:20 PM on November 23, 2020: member

    Code review ACK 9860537473b160dfabb033f598848d8a60a339dc. Nice to get this better error handling and test coverage!

  7. MarcoFalke renamed this:
    qa: Validate -rpcauth arguments
    rpc: Validate -rpcauth arguments
    on Nov 23, 2020
  8. MarcoFalke commented at 4:43 PM on November 23, 2020: member

    cr ACK 9860537473b160dfabb033f598848d8a60a339dc

  9. jonatack commented at 5:09 PM on November 23, 2020: member

    Concept ACK

  10. promag commented at 6:23 PM on November 23, 2020: member

    As it is a end-user affecting change it needs a brief (1 line) mention in the release notes.

    done

  11. MarcoFalke commented at 6:26 PM on November 23, 2020: member

    cr ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

  12. jonatack commented at 7:30 PM on November 23, 2020: member

    ci (unrelated, I suppose, but noting it before it's restarted)

    163/195 - p2p_fingerprint.py failed, Duration: 2 s
stdout:
2020-11-23T19:08:44.357000Z TestFramework (INFO): Initializing test directory /tmp/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20201123_184330/p2p_fingerprint_29
2020-11-23T19:08:45.474000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 126, in main
    self.run_test()
  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/p2p_fingerprint.py", line 110, in run_test
    assert "block" not in node0.last_message
AssertionError
  13. in doc/release-notes.md:83 in 35838a191f outdated 
      79 | @@ -80,6 +80,8 @@ Updated settings
  80 |  
  81 |  Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.
  82 |  
  83 | +- Invalid `-rpcauth` now cause bitcoind to fail to start.  (#20461)
    jonatack commented at 7:36 PM on November 23, 2020: 
    - Passing an invalid `-rpcauth` argument will now cause bitcoind to fail to start.  (#20461)
  14. in src/httprpc.cpp:256 in 35838a191f outdated 
     252 | @@ -259,6 +253,16 @@ static bool InitRPCAuthentication()
 253 |      if (gArgs.GetArg("-rpcauth","") != "")
 254 |      {
 255 |          LogPrintf("Using rpcauth authentication.\n");
 256 | +        for (std::string rpcauth : gArgs.GetArgs("-rpcauth")) {
    jonatack commented at 7:49 PM on November 23, 2020:

    I'm not sure if the change from reference to const to by value was deliberate.

            for (const std::string& rpcauth : gArgs.GetArgs("-rpcauth")) {
  15. MarcoFalke commented at 7:51 PM on November 23, 2020: member

    Please don't restart a build without filing a bug. Either leave the failure, so that it can be found when scraping the logs with automated tools/manually, or file a bug if this is an intermittent failure. See also

    If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. ( https://cirrus-ci.com/task/5542990686978048?command=ci#L6034 )

  16. in src/httprpc.cpp:30 in 35838a191f outdated 
      26 | @@ -27,6 +27,8 @@
  27 |  /** WWW-Authenticate to present with 401 Unauthorized response */
  28 |  static const char* WWW_AUTH_HEADER_DATA = "Basic realm=\"jsonrpc\"";
  29 |  
  30 | +static std::vector<std::vector<std::string>> g_rpcauth;
    jonatack commented at 7:51 PM on November 23, 2020:

    Could add a Doxygen comment.

  17. jonatack commented at 8:04 PM on November 23, 2020: member

    Approach ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

  18. rpc: Refactor to process -rpcauth once d37c813a43
  19. rpc: Validate -rpcauth arguments 46001323b1
  20. doc: Release note regarding -rpcauth validation 053b4fbad8
  21. promag force-pushed on Nov 23, 2020
  22. promag commented at 9:19 PM on November 23, 2020: member

    Addressed @jonatack review.

  23. DrahtBot commented at 9:42 PM on November 23, 2020: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #20407 (rpc: Support -rpcauthfile argument by promag)

    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.

  24. in doc/release-notes.md:83 in 053b4fbad8 
      79 | @@ -80,6 +80,8 @@ Updated settings
  80 |  
  81 |  Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.
  82 |  
  83 | +- Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start.  (#20461)
    jonatack commented at 10:48 AM on November 24, 2020:

    "now cause" should be either "now causes" or "will now cause"

  25. jonatack commented at 10:49 AM on November 24, 2020: member

    ACK 053b4fbad8729308774d5e7b53f53c12627fa88b

  26. luke-jr changes_requested
  27. luke-jr commented at 12:32 AM on November 25, 2020: member

    Concept NACK. Additional fields is not necessarily an error.

    A debug log line sounds fine.

  28. MarcoFalke commented at 6:59 AM on November 25, 2020: member

    review ACK 053b4fbad8729308774d5e7b53f53c12627fa88b

  29. promag commented at 9:59 AM on November 25, 2020: member

    Additional fields is not necessarily an error. @luke-jr what do you suggest? Ignore the whole argument? Just use the the first 3 fields? Not sure about the debug log since it's a sensitive value.

  30. practicalswift commented at 1:25 PM on November 25, 2020: contributor

    Concept ACK: failing swiftly and loudly is better than failing silently

  31. luke-jr commented at 3:25 PM on November 25, 2020: member

    what do you suggest? Ignore the whole argument?

    Yes, at this startup stage. It will still fail loudly when the user attempts to authenticate later. (Maybe it would make sense to have a special error message returned for users with unrecognised configurations?)

    Just use the the first 3 fields?

    No, this would potentially grant permissions not intended.

    Not sure about the debug log since it's a sensitive value.

    The username isn't (maybe the line number if it's a rpcauthfile).

  32. ryanofsky approved
  33. ryanofsky commented at 1:44 AM on December 2, 2020: member

    Code review ACK 053b4fbad8729308774d5e7b53f53c12627fa88b. Only changes since last review are moving a variable declaration and adding a comment, release notes, and a const.

    I think this PR looks good in it's current form. It seems to me the changes Luke is asking for would make this PR slightly worse. They'd make code more complicated and configuration problems harder to detect because errors would happen at authorization time instead of startup time. They also wouldn't seem to enable useful behavior because unrecognized options would still be rejected, just at a later time. Maybe Luke you could give an example of when the behavior you're suggesting might be useful?

    I think if it comes down to a choice between Luke's more limited error reporting or no error reporting at all Luke's approach would be better. But current PR seems best, as far as I can tell

  34. promag commented at 1:47 AM on December 2, 2020: member

    I think Luke case is to support same conf between knots and core?

  35. ryanofsky commented at 2:16 AM on December 2, 2020: member

    I think Luke case is to support same conf between knots and core?

    I don't understand how. What's the example situation where you would want to use the same configuration for knots and core, but have knots authorize RPC requests which core rejects? What would the requests be? Who would the users be? I can't think of a situation connected to reality where this would be useful. Just an opinion, but I think it would be good to think about specific situations where this feature could be useful before implementing it.

  36. MarcoFalke merged this on Dec 2, 2020
  37. MarcoFalke closed this on Dec 2, 2020
  38. promag deleted the branch on Dec 2, 2020
  39. promag commented at 9:26 AM on December 2, 2020: member

    @ryanofsky right, I'm just guessing.

  40. sidhujag referenced this in commit ed39d9ab96 on Dec 2, 2020
  41. luke-jr commented at 3:57 PM on December 2, 2020: member

    So users don't need to swap out their rpcauth config to downgrade. This basically means people using advanced rpcauth features couldn't run Core on their system anymore without annoying additional steps...

  42. ryanofsky commented at 4:22 PM on December 2, 2020: member

    So users don't need to swap out their rpcauth config to downgrade. This basically means people using advanced rpcauth features couldn't run Core on their system anymore without annoying additional steps...

    Ok IIUC there is no specific situation which causes a problem, just general idea to avoid overly strict validation of config options for more flexibility. I don't disagree with that and do think this could be a warning instead of an error. This all just seemed like an unlikely point of friction, so I was confused if there was more to explain the change requests and concept nack. Any followup that keeps an early error or warning seems good to me!

  43. promag commented at 5:05 PM on December 2, 2020: member

    Might make sense to have a -strict option for those that wish to have and keep clean configurations.

  44. DrahtBot locked this on Feb 15, 2022
Contributors
promaglaanwjryanofskyMarcoFalkejonatackDrahtBotluke-jrpracticalswift
Labels
RPC/REST/ZMQ
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-04-22 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me