lnd
lnd copied to clipboard
[tor] Add ability to encrypt the Tor private key on disk
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
@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
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 Ah got it, thank you for the explanation!
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
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
EncryptPayloadtoWriterandDecryptPayloadFromReaderto be an interface that is independent of thekeychainpackage (requires thegenEncryptionKeypart to be pulled out and the encryption key be a[]bytedirectly). - copy that new interface to the
torpackage and accept it as the input for theNewOnionFileinstead of thekeyRing(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/mockmock one
Got it, thank you @guggero . Will let you know when I have the latest PRs up.
Sounds good! Will review once you give me the heads up.
@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
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).
Rebased this just fyi. It's ready for a look!
@Crypt-iQ and @guggero, thanks again for the reviews! I made those updates.
@guggero: review reminder @orbitalturtle, remember to re-request review from reviewers when ready
Can be rebased now and use the tor/v1.1.0 dependency in go.mod.
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!
@crypt-iq: review reminder
Needs a rebase.
@guggero rebased