rpc: Validate -rpcauth arguments #20461

promag · 2020-11-23T12:07:21Z

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

src/httprpc.cpp

laanwj · 2020-11-23T12:08:56Z

Concept ACK

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

ryanofsky

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

maflcko · 2020-11-23T16:43:45Z

cr ACK 9860537473b160dfabb033f598848d8a60a339dc

jonatack · 2020-11-23T17:09:41Z

Concept ACK

promag · 2020-11-23T18:23:59Z

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

done

maflcko · 2020-11-23T18:26:53Z

cr ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

jonatack · 2020-11-23T19:30:12Z

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

maflcko · 2020-11-23T19:51:48Z

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 )

jonatack

Approach ACK 35838a191f0e7a91aeeb5cf3e95b2fac022fcb04

doc/release-notes.md

src/httprpc.cpp

promag · 2020-11-23T21:19:54Z

Addressed @jonatack review.

DrahtBot · 2020-11-23T21:42:01Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Support -rpcauthfile argument #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.

jonatack

ACK 053b4fb

jonatack · 2020-11-24T10:48:08Z

doc/release-notes.md


 Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.

+- Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start.  (#20461)


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

luke-jr

Concept NACK. Additional fields is not necessarily an error.

A debug log line sounds fine.

maflcko · 2020-11-25T06:59:41Z

review ACK 053b4fb

promag · 2020-11-25T09:59:26Z

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.

practicalswift · 2020-11-25T13:25:48Z

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

luke-jr · 2020-11-25T15:25:12Z

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).

ryanofsky

Code review ACK 053b4fb. 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

promag · 2020-12-02T01:47:14Z

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

ryanofsky · 2020-12-02T02:16:13Z

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.

promag · 2020-12-02T09:26:06Z

@ryanofsky right, I'm just guessing.

053b4fb doc: Release note regarding -rpcauth validation (João Barbosa) 4600132 rpc: Validate -rpcauth arguments (João Barbosa) d37c813 rpc: Refactor to process -rpcauth once (João Barbosa) Pull request description: Invalid `-rpcauth` arguments are currently silently ignored. This make server initialization fail if any `-rpcauth` is invalid. ACKs for top commit: MarcoFalke: review ACK 053b4fb jonatack: ACK 053b4fb ryanofsky: Code review ACK 053b4fb. Only changes since last review are moving a variable declaration and adding a comment, release notes, and a `const`. Tree-SHA512: c99923d4a121f0c9f882b07f5402ea53e9b2d9455ad34468a094ffab1d64df26c82e1279734c0d42bc2e113eae7b581fbc3be52f3ed4a2d7450d11793afcf406

luke-jr · 2020-12-02T15:57:05Z

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...

ryanofsky · 2020-12-02T16:22:59Z

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!

promag · 2020-12-02T17:05:41Z

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

fanquake added the RPC/REST/ZMQ label Nov 23, 2020

laanwj reviewed Nov 23, 2020

View reviewed changes

src/httprpc.cpp Outdated Show resolved Hide resolved

ryanofsky approved these changes Nov 23, 2020

View reviewed changes

maflcko changed the title ~~qa: Validate -rpcauth arguments~~ rpc: Validate -rpcauth arguments Nov 23, 2020

jonatack reviewed Nov 23, 2020

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

src/httprpc.cpp Outdated Show resolved Hide resolved

src/httprpc.cpp Outdated Show resolved Hide resolved

promag added 3 commits November 23, 2020 21:02

rpc: Refactor to process -rpcauth once

d37c813

rpc: Validate -rpcauth arguments

4600132

doc: Release note regarding -rpcauth validation

053b4fb

promag force-pushed the 2020-11-validate-rpcauth branch from 35838a1 to 053b4fb Compare November 23, 2020 21:03

DrahtBot mentioned this pull request Nov 23, 2020

rpc: Support -rpcauthfile argument #20407

Closed

jonatack reviewed Nov 24, 2020

View reviewed changes

luke-jr suggested changes Nov 25, 2020

View reviewed changes

ryanofsky approved these changes Dec 2, 2020

View reviewed changes

maflcko merged commit 283f22c into bitcoin:master Dec 2, 2020

promag deleted the 2020-11-validate-rpcauth branch December 2, 2020 09:24

promag mentioned this pull request Dec 3, 2020

RPC: Tolerate invalid rpcauth options #20550

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022


		Changes to Wallet or GUI related settings can be found in the GUI or Wallet section below.

		- Passing an invalid `-rpcauth` argument now cause bitcoind to fail to start. (#20461)

rpc: Validate -rpcauth arguments #20461

rpc: Validate -rpcauth arguments #20461

Uh oh!

Conversation

promag commented Nov 23, 2020

Uh oh!

Uh oh!

laanwj commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 23, 2020

Uh oh!

jonatack commented Nov 23, 2020

Uh oh!

promag commented Nov 23, 2020

Uh oh!

maflcko commented Nov 23, 2020

Uh oh!

jonatack commented Nov 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Nov 23, 2020

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

promag commented Nov 23, 2020

Uh oh!

DrahtBot commented Nov 23, 2020

Conflicts

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Nov 24, 2020

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Nov 25, 2020

Uh oh!

promag commented Nov 25, 2020

Uh oh!

practicalswift commented Nov 25, 2020

Uh oh!

luke-jr commented Nov 25, 2020

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Dec 2, 2020

Uh oh!

ryanofsky commented Dec 2, 2020

Uh oh!

promag commented Dec 2, 2020

Uh oh!

luke-jr commented Dec 2, 2020

Uh oh!

ryanofsky commented Dec 2, 2020

Uh oh!

promag commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Nov 23, 2020 •

edited

Loading

jonatack commented Nov 23, 2020 •

edited

Loading

promag commented Dec 2, 2020 •

edited

Loading