Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpcserver+lncli: add ability to create encrypted debug information package #8188

Merged
merged 7 commits into from
Jan 9, 2024

Conversation

guggero
Copy link
Collaborator

@guggero guggero commented Nov 16, 2023

Change Description

This PR adds a new RPC method GetDebugInfo that returns the full runtime configuration of the lnd node as well as the complete lnd.log file. The RPC can be called by the new lncli getdebuginfo CLI command.

The even more useful part of this PR are the two new CLI commands though: lncli encryptdebugpackage (and its counter part lncli decryptdebugpackage).
The lncli encryptdebugpackage command calls into the GetDebugInfo RPC described above (as well as a couple of other useful RPCs), collects all that info in a single file, uses the efficient Brotli compression algorithm to reduce the size as much as possible, then encrypts everything using a symmetric key created using ECDH.

The idea is that we would publish a public key in the GitHub issue template and would ask users to run the lncli encryptdebugpackage command and upload the encrypted output files to the GitHub issue to provide us with the information we normally require to debug user problems.

Steps to Test

Here are two example files created on my development machine:

package-minimal.json
package-maximal.json

They were created using:

# DO NOT USE THE FOLLOWING PUBLIC KEY TO ENCRYPT ANY SENSITIVE INFORMATION
# the private key to it is posted further below and is therefore known to anyone

lncli encryptdebugpackage 02bf7768327ce9d50e1bbf149bee4365a8ef6a3824405aaf13ba6c9b612f9fc219 > package.json

lncli encryptdebugpackage 02bf7768327ce9d50e1bbf149bee4365a8ef6a3824405aaf13ba6c9b612f9fc219 --peers --onchain --channels > package-maximal.json

To decrypt them, you can use:

lncli decryptdebugpackage af0aba27520dcf88a27c1725d8b1e14657a200711c98371cbb347bf35de1a43b < package-minimal.json > minimal.txt

lncli decryptdebugpackage af0aba27520dcf88a27c1725d8b1e14657a200711c98371cbb347bf35de1a43b < package-maximal.json > maximal.txt

@guggero guggero added enhancement Improvements to existing features / behaviour config Parameters/arguments/config file related issues/PRs privacy General label for issues/PRs related to the privacy implications of using the software cli Related to the command line interface lncli debugging labels Nov 16, 2023
@saubyk saubyk added this to the v0.18.0 milestone Nov 30, 2023
Copy link
Collaborator

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend adding a Config -> Config transformation that redacts sensitive information:

  • Bitcoind.RPCPass
  • Bitcoind.RPCAuth
  • Btcd.RPCPass
  • Tor.Password
  • DB.Etcd.Pass
  • DB.Postgres.DSN

This is the minimum set of information I'd like to see redacted in this transformation. However, I strongly encourage you to do your own review of the config tree to see if I missed anything, as you have significantly more experience with the codebase than I.

cmd/lncli/cmd_debug.go Outdated Show resolved Hide resolved
// configToFlatMap converts the given config struct into a flat map of key/value
// pairs using the dot notation we are used to from the config file or command
// line flags.
func configToFlatMap(cfg Config) (map[string]string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only major concern I have with this is if we can do a Config -> Config transformation that redacts sensitive info that is irrelevant wrt debugging. The one that comes to mind is any bitcoind rpcpass or stuff like that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good point, those values don't really need to be known by a supporting person. And if a special character in those fields cause the config file not to be parsed correctly, it should be noticeable in another way.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a string suffix based redaction, which covers all the keys you mentioned (except for Bitcoind.RPCAuth which doesn't exist on the lnd side, only in bitcoind's config).

rpcserver.go Show resolved Hide resolved
@lightninglabs-deploy
Copy link

@ProofOfKeags: review reminder
@ellemouton: review reminder

Copy link
Collaborator

@ellemouton ellemouton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK, LGTM! 🔥 this is awesome & will be very very useful. The PR is structured beautifully 🤩

The only question I have is: why not have an "--output" flag for the encrypt command instead of piping the output to a file? the reason I ask is cause when I was testing this, it took me a while to realise that the decrypt was not working cause the output of my command was actually printing a docker related string before the json output - but maybe that's not expected to generally be the case for others.

config.go Show resolved Hide resolved
lnencrypt/crypto.go Show resolved Hide resolved
Copy link
Collaborator

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ship it ⛵️

@guggero
Copy link
Collaborator Author

guggero commented Jan 9, 2024

I added an --output_file and --input_file flag do the two commands respectively.

With this commit we add a new helper function that recursively turns the
runtime configuration into a flat key/value map that is human-readable,
using the dot notation for nested values that is also used in the config
file or command line flags.
With this commit we add a new way to encrypt and decrypt a sensitive
payload: By using Diffie-Hellman over a local and remote key to derive
at a shared secret key.
This commit adds three optional command line flags to the
encryptdebugpackage: --peers, --onchain and --channels.
Each of them adds the output of extra commands to the encrypted debug
package.
their node with a single command and securely encrypt it to the public key of
a developer or support person. That way the person supporting the user with
their issue has an eas way to get all the information they usually require
without the user needing to publicly give away a lot of privacy-sensitive
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious about how to obtain the public key - is it the same as the release signing key?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll define one and then update the issue template and FAQ to mention the lncli encryptdebugpackage command and key to use when submitting info to us. And I'll probably create an internal tool that has the corresponding private key to decrypt the user files in a nice UI.

@guggero guggero merged commit 9afe1b7 into lightningnetwork:master Jan 9, 2024
24 of 25 checks passed
@guggero guggero deleted the debug-rpcs branch January 9, 2024 15:28
// the size of the final payload.
var (
compressBuf bytes.Buffer
options = brotli.WriterOptions{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any particular reason we chose this new-ish algo over what's already available in the stdlib? The set of stdlib algos: https://pkg.go.dev/compress

Generally would like to minimize adding in new dependancies (tho the one added itself adds no new deps other than itself, as it's a transpilation of a c algo and uses the stdlib only).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick research on what compression algorithm works best on large-ish text and Brotli seems to be leading by quite a bit https://paulbradley.dev/go-compression-comparisons/.
And I explicitly only used it because we do already transitively depend on it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command line interface config Parameters/arguments/config file related issues/PRs debugging enhancement Improvements to existing features / behaviour lncli privacy General label for issues/PRs related to the privacy implications of using the software
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants