doc: Add release note for RPC Whitelist #17743
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7ebd664 to
58c0417
Compare
Member
|
it should probably at least mention one or all of the config options, otherwise users will have to just grep and guess |
Contributor
Author
|
@instagibbs Updated, however it isn't updated on the PR page for some weird reason. If you look into my branch you will see the update o.O |
Contributor
Author
|
See https://www.githubstatus.com/, they have issues at the moment |
0816d40 to
cb66408
Compare
maflcko
reviewed
Dec 13, 2019
Contributor
|
You could just copy the help-text if you want it to be more thorough. gArgs.AddArg("-rpcwhitelist=<whitelist>", "Set a whitelist to filter incoming RPC calls for a specific user. The field <whitelist> comes in the format: <USERNAME>:<rpc 1>,<rpc 2>,...,<rpc n>. If multiple whitelists are set for a given user, they are set-intersected. See -rpcwhitelistdefault documentation for information on default whitelist behavior.", ArgsManager::ALLOW_ANY, OptionsCategory::RPC);
gArgs.AddArg("-rpcwhitelistdefault", "Sets default behavior for rpc whitelisting. Unless rpcwhitelistdefault is set to 0, if any -rpcwhitelist is set, the rpc server acts as if all rpc users are subject to empty-unless-otherwise-specified whitelists. If rpcwhitelistdefault is set to 1 and no -rpcwhitelist is set, rpc server acts as if all rpc users are subject to empty whitelists.", ArgsManager::ALLOW_BOOL, OptionsCategory::RPC); |
Member
|
The release notes are meant as a brief pointer, not to duplicate the rpc and/or command line help. I think the current patch is ok |
cb66408 to
2b705b0
Compare
Member
|
thanks for the cleanups |
2b705b0 to
3ae22b0
Compare
Contributor
Author
|
@instagibbs Sorry but could you re-ACK? The commit hash just changed as I included the PR reference :P |
Member
|
argh, needed to refresh one more time :) ACK 3ae22b0 |
Contributor
|
ACK 3ae22b0c8fb9a59ab95cbf6c970166b01274f846. |
Contributor
|
Oh, before merging this I think we should either bump UniValue or add a temporary warning about this RPC DoS gotcha (#17742 (comment)): Temporary NACK until this is addressed. Sorry about that, but end-user security is first, second and third priority :) |
Contributor
|
This is just release notes so I don't think a nack here makes sense, but I
would get the univalve bump added to high priority.
While this is certainly a bad bug, if it's limited to segfaulting the node
it's at least "Fail-Safe" insofar as stopping a node is generally safe and
doesn't lead to an un-permitted rpc being called.
Now, a segfault may be able to be pivoted to something more extreme but
I've not looked at the particulars of the univalue issue.
…On Sat, Dec 14, 2019, 9:34 AM practicalswift ***@***.***> wrote:
Oh, before merging this I think we should either bump UniValue or add a
temporary warning about this RPC DoS gotcha: #17742 (comment)
<#17742 (comment)>.
$ share/rpcauth/rpcauth.py u p
String to be appended to bitcoin.conf:
rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5
Your password:
p
$ src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5' &
$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p help | head -1
== Blockchain ==
$ src/bitcoin-cli -regtest -rpcuser=u -rpcpassword=p getnetworkinfo
2019-12-13T02:01:37Z RPC User u not allowed to call method getnetworkinfo
error: server returned HTTP error 403
$ curl -u u:p --request POST \
--data $(python -c 'print(50000 * "[");') http://127.0.0.1:18443
curl: (52) Empty reply from server
[1]+ Segmentation fault (core dumped) src/bitcoind -regtest -rpcwhitelist=u:help '-rpcauth=u:d7ca300b5aeceb73e9825068fc4496ef$166cbdedfe11c30a95c28d13b3383682825308d1069d347f9705b8cf922637c5'
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#17743?email_source=notifications&email_token=AAGYN65EZC5DA4FXM2KDFMLQYUKLXA5CNFSM4J2M7TC2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4HMDI#issuecomment-565736973>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGYN647K22T3GN3HWASFBLQYUKLXANCNFSM4J2M7TCQ>
.
|
Contributor
|
@JeremyRubin I'm just NACK:ing the change as-is (per 3ae22b0c8fb9a59ab95cbf6c970166b01274f846): if @emilengler adds a (temporary) warning about this gotcha that would be perfectly fine and I'll ACK immediately :) We should do what we can to help our users stay safe: not adding a warning about a known gotcha when we have the opportunity would feel bad :) |
3ae22b0 to
f1cf747
Compare
Contributor
Author
|
@practicalswift Adjusted, could you check it? |
laanwj
reviewed
Dec 15, 2019
f1cf747 to
7965e0b
Compare
Member
|
ACK 7965e0b |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A release note for #12763