lnd icon indicating copy to clipboard operation
lnd copied to clipboard

[tor] Add ability to encrypt the Tor private key on disk

Open orbitalturtle opened this issue 3 years ago • 14 comments

This PR replaces #4458. Since I've been making some of the changes for @gkrizek and it's been a bit tricky to manage making PRs into his PRs... we thought it would be nice to have a fresh start. :)

Depends on #6526

Graham's original PR description:

It's possible that a user might not want the Tor private key to sit on the disk in plaintext (it is a private key after all). So this commit adds a new flag to encrypt the Tor private key on disk using the wallet's seed. When the --tor.encryptkey flag is used, LND will still write the Tor key to the same file, however it will now be encrypted instead of plaintext. This essentially uses the same method to encrypt the Tor private key as is used to encrypt the Static Channel Backup file.

Summary of Changes:

  • Moves the crypto.go file from chanbackup into its own package called lnencrypt. This was needed because we can't directly reference the encryptPayloadToWriter and decryptPayloadFromReader functions from the chanbackup package due to circular dependency issues. Also, I think it's very likely that future enhancements will also want to use these same functions. Thus I've moved them to their own package.

  • Creates a flag called --tor.encryptkey which encrypts the Tor private key.

When this is set it encrypts the private key and writes the encrypted blob to the same file on disk. This allows users to still keep the same functionality and flows as before (like refreshing a hidden service), but adds protection when running in untrusted environments.

  • Add necessary tests and documentation

orbitalturtle avatar May 05 '22 05:05 orbitalturtle

@guggero Oops, sorry, looks like I broke something when trying to fix the onion file stuff, as discussed here: https://github.com/lightningnetwork/lnd/pull/4458#discussion_r804571800 But I'm having trouble fixing the issue...

First question, if you get a chance: Does moving the onion file stuff to a new package within Tor make sense? Re: this convo https://github.com/lightningnetwork/lnd/pull/4458#discussion_r620729177?

If it does make sense to move it... I'm getting the following error that I am struggling to fix

no required module provides package github.com/lightningnetwork/lnd/tor/onionfile; to add it: go get github.com/lightningnetwork/lnd/tor/onionfile

and if I try to do that...

go get: module github.com/lightningnetwork/lnd/tor@upgrade found (v1.0.0), but does not contain package github.com/lightningnetwork/lnd/tor/onionfile

orbitalturtle avatar May 05 '22 23:05 orbitalturtle

Since you last rebased we made the tor package its own submodule. That fixed a dependency issue but makes it a bit harder to create PRs that change the functionality, unfortunately.

You need to do the following now (that goes for any change to a package that is a submodule, meaning it has its own go.mod file):

  • create PR 1 with the changes to the submodule only (in this case al the changes in tor).
  • after PR 1 is merged, a new tag for the affected submodule is pushed
  • create PR 2 that uses the updated submodule in the rest of the code base

Of course for development it makes sense to create PR 1 and PR2 at the same time and just stack the commits of both in the second PR and use a replace directive until PR 1 is merged and tagged.

guggero avatar May 06 '22 11:05 guggero

@guggero Ah got it, thank you for the explanation!

orbitalturtle avatar May 06 '22 20:05 orbitalturtle

Hey @guggero one more question for you!

Now I've split the Tor code out into a separate PR. But the Tor changes depend on changes in the keychain and lnencrypt packages. So now I'm getting this response when I try to run "go test" in the Tor module

onionfile/onion_file.go:10:2: no required module provides package github.com/lightningnetwork/lnd/keychain; to add it:
	go get github.com/lightningnetwork/lnd/keychain
onionfile/onion_file.go:11:2: no required module provides package github.com/lightningnetwork/lnd/lnencrypt; to add it:
	go get github.com/lightningnetwork/lnd/lnencrypt

And when I do go get github.com/lightningnetwork/lnd/keychain I get:

go get: module github.com/lightningnetwork/lnd@upgrade found (v0.0.2), but does not contain package github.com/lightningnetwork/lnd/keychain

Wondering if I need to split out the keychain & lnencrypt changes into a third PR? Or if something else is going on

orbitalturtle avatar May 09 '22 18:05 orbitalturtle

Hmm, this is a tough one. I guess what I forgot to mention is that the tor submodule, to be independent and to work standalone and to avoid circular dependencies, shouldn't have any references to the parent lnd packages.

So this requires some additional refactoring...

My suggestion:

  • refactor the EncryptPayloadtoWriter and DecryptPayloadFromReader to be an interface that is independent of the keychain package (requires the genEncryptionKey part to be pulled out and the encryption key be a []byte directly).
  • copy that new interface to the tor package and accept it as the input for the NewOnionFile instead of the keyRing (that's the way to decouple packages: if an interface has exactly the same methods with the same signatures they are compatible, even if the interface is declared under different names in different packages)
  • Create a simple mock for that new interface instead of using the lntest/mock mock one

guggero avatar May 09 '22 20:05 guggero

Got it, thank you @guggero . Will let you know when I have the latest PRs up.

orbitalturtle avatar May 10 '22 17:05 orbitalturtle

Sounds good! Will review once you give me the heads up.

guggero avatar May 10 '22 18:05 guggero

@guggero One more question... so because we're getting rid of all the lnd dependencies in the Tor module, we don't need to split the onionfile code into a new subpackage of Tor, correct? Just want to make sure I before I move that back

orbitalturtle avatar May 11 '22 17:05 orbitalturtle

I think the OnionFile struct (and its file) can be moved directly to the main tor package/submodule. Or it can stay in the tor/onionfile package (but shouldn't be a submodule, so shouldn't have its own go.mod file) and be renamed to just File (since it will be used as onionfile.File externally, with the current name onionfile.OnionFile it's a bit too verbose IMO).

guggero avatar May 11 '22 17:05 guggero

Rebased this just fyi. It's ready for a look!

orbitalturtle avatar Aug 02 '22 19:08 orbitalturtle

@Crypt-iQ and @guggero, thanks again for the reviews! I made those updates.

orbitalturtle avatar Aug 31 '22 19:08 orbitalturtle

@guggero: review reminder @orbitalturtle, remember to re-request review from reviewers when ready

lightninglabs-deploy avatar Sep 07 '22 19:09 lightninglabs-deploy

Can be rebased now and use the tor/v1.1.0 dependency in go.mod.

guggero avatar Sep 09 '22 14:09 guggero

Thanks @guggero! I made those changes. And sure, I'll definitely keep a closer eye out for those nits & code style guidelines in the future!

orbitalturtle avatar Sep 14 '22 19:09 orbitalturtle

@crypt-iq: review reminder

lightninglabs-deploy avatar Sep 28 '22 20:09 lightninglabs-deploy

Needs a rebase.

guggero avatar Sep 29 '22 07:09 guggero

@guggero rebased

orbitalturtle avatar Sep 30 '22 07:09 orbitalturtle