Log RPC parameters (arguments) if -debug=rpcparams #16365

When a developer is examining debug.log (or client terminal output), it's often useful to know which RPCs have been submitted to the client; this can be enabled with the -debug=rpc configuration option. But this prints only the method name. This PR adds -debug=rpcparams to enable the logging of each RPC's parameters (arguments). The parameters of certain RPCs are keys or passwords; these should not be logged.

<!--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:

• #22736 (log, sync: change lock contention from preprocessor directive to log category by jonatack)
• #20487 (Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)

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.

How about private keys?

Rebased, addressed review comments, force-pushed.

IMO the blacklist doesn't look a great option.

How about disallow -debug=rpcparams in mainnet and drop the blacklist?

concept NACK, someone is going to forget when adding a new RPC and start logging secrets in production.

I think @promag suggestion is better.

Another approach is to add a sensitive options/flag (false by default) to RPCHelpMan RPCArg.

@promag, interesting idea, I like that it would allow specific arguments to be designated as sensitive, rather than the entire method. Also, it would be nice not to have to touch so many lines of code (as my latest commit does). But @instagibbs's concern, #16365 (comment), which I was attempted to overcome with my latest commit, becomes live again.

Rebased, replaced functional (python) test with a proper unit test.

I had an idea for what may be an improvement. It's in a separate commit, "replace sensitive bool with more general flags". Please let me know what you think; this commit is optional.

Modifying all of the lines of the static const CRPCCommand commands[] tables is disruptive, and all that's being done is to add a boolean value (to indicate whether to log the RPC's params or not). As long as we're touching all these lines anyway, why not implement a more general flags bitfield, of which DONTLOG is just the first bit. There may be other future attributes that would be useful to add to RPC methods. With this commit, adding them would touch only the lines corresponding to the affected RPCs, rather than all of them.

Rebased to resolve conflicts.

I don't know the cause of the Travis CI failure; feature_pruning.py succeeds on my machine. I would not think this PR has any particular connection to that test. Maybe it's a transient failure?

Sorry, I accidentally closed this PR, reopening now...

Still not buying the approach.

How about disallow -debug=rpcparams in mainnet and drop the blacklist?

Couldn't we use this approach for now?

Couldn't we use this approach for now?

Just so I'm sure I understand, you're suggesting not adding the flags member to CRPCCommand (here, which causes many lines to be touched, and that's clearly undesirable), and instead just don't censor anything (no black or white list), but restrict this to testnet only?

I like that idea, would make this PR much simpler. If it proves useful on testnet, then we can try to find an acceptable way to bring it to mainnet -- maybe there's a less intrusive way that's still very safe. I always appreciate your comments.

So it seems everything depends on whether this is considered useful for mainnet. I can imagine that for production logging it is. If not, @promag's approach is fine. If it is, the safest approach seems something like the current approach.

There is perhaps a less disruptive approach:

• Make it possible to have an optional argument to the CRPCCommand constructors, which defaults to DONT_LOG, and not explicitly change it initially for any commands.
• Then incrementally introduce DO_LOG flags, which can be done separately without needing to keep the whole PR up to date here.

One unrelated nit: I think it would be a bit cleaner to log the RPC command and arguments in the same log line (the logic would be something like if (debug_rpcparams && command.do_log) { log_rpc_with_params(); } else if (debug_rpc || debug_rpcparams) { log_rpc_without_params(); }

Thanks, @sipa and @promag, I took both of your advice. The default now is to not log arguments on mainnet, and always log arguments on testnet. This change is now much less invasive.

Force-pushed (diff) a different approach that does not require changing each of the 149 per-rpc CRPCCommand initializers. This new way is much less invasive and more manageable. I finally decided it's best to go with an "allow" list (hard to beat simplicity). If someone adds a new RPC later without being aware of this form of logging, its arguments will not be logged. That makes this approach very safe against accidentally leaking secret information to the log file. I incorporated some of the review comments as well.

Here are some examples (without showing the RPC output):

$ bitcoin-cli getnetworkinfo
$ bitcoin-cli getblockhash 0
$ bitcoin-cli logging '["rpcparams"]' '["rpc","mempool"]'
$ bitcoin-cli encryptwallet WALLET-PASSWORD-SECRET

Here's what appears in debug.log:

2020-09-29T05:58:50Z rpc=getnetworkinfo()
2020-09-29T05:59:00Z rpc=getblockhash(0)
2020-09-29T06:02:51Z rpc=logging([rpcparams],[rpc,mempool])
2020-09-29T06:02:55Z rpc=encryptwallet(****)

Tested almost-ACK 7d22a6fad9d855e9dbb0bca70857c9aadc55f188 rebased to current master, modulo the comments below

Maintenance of this list seems like a drawback

I agree, I'm going to convert this PR to draft because I think there's a better way to do this that doesn't require a separate list. I'll also pick up your other suggestions. Thanks for taking the time to look it over!

<!--cf906140f33d8803c4a75a2196329ecb-->

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

<!--13523179cfe9479db18ec6c5d236f789-->There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

I'm going to convert this PR to draft because I think there's a better way to do this that doesn't require a separate list. I'll also pick up your other suggestions.

Given it's been 16 months this statement, I'm going to close this for now. Feel free to comment / ping if you're going to work on this again and need the PR reopened.