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

gkrizek · 2020-07-12T04:11:29Z

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

gkrizek · 2020-07-15T05:50:52Z

Discussion question: In this implementation, if LND is started with --tor.encryptkey it encrypts the key on disk. However, if LND was later started without the --tor.encryptkey it would write the key to disk in plaintext (because it wasn't instructed to encrypt it). Is this the best behavior? Should we always save the key in an encrypted format if the flag is passed and never let it be stored in plaintext?

wpaulino

However, if LND was later started without the --tor.encryptkey it would write the key to disk in plaintext (because it wasn't instructed to encrypt it). Is this the best behavior? Should we always save the key in an encrypted format if the flag is passed and never let it be stored in plaintext?

In that case, a user-friendly error can be returned to prevent startup noting that the private key is encrypted and the flag should be toggled to continue. In the future, we may want to consider toggling the flag by default, but that'd be a breaking change.

server.go

tor/add_onion.go

watchtower/standalone.go

gkrizek · 2020-08-11T03:16:15Z

Thanks for the review, @wpaulino! I've addressed your comments, PTAL.

I also added a condition if the Tor private key is encrypted but LND doesn't have --tor.encryptkey enabled. It will now log a detailed error message and exit.

tor/add_onion.go

wpaulino

Some final minor nits and we're good to go!

watchtower/standalone.go

wpaulino · 2020-08-14T20:23:11Z

@gkrizek the linter reported some failures. Could you address those?

gkrizek · 2020-08-15T01:12:31Z

@wpaulino Addressed. make lint and make check pass locally. It was mainly complaining about some of the test code I did which I refactored to please the linter so that may be worth a new check. Thanks!

Crypt-iQ · 2020-09-21T10:46:24Z

Reviewing now

gkrizek · 2021-12-20T18:33:28Z

ping. I think this would still be valuable.

Crypt-iQ · 2022-01-03T17:32:30Z

In some cases I could see this being valuable, like if there happened to be a remote file-system leak vulnerability in lnd. But IMO our code isn't complex enough to have one of those bugs. That leaves us in the realm of access to the machine that lnd is running on. If somebody has access to the machine, it is already game over as they could dump the memory of the running process if they have privileges. If they don't, they could perform any one of the well-documented side-channel attacks that exist. For those reasons I'm not sure it makes sense to give users a false sense of security at the cost of increased code complexity.

guggero

I think this is a nice security feature and makes sense to add.

Did a high-level pass and things look pretty good functionality wise (didn't run any manual tests yet, will do so in the second pass). But there are quite a few style related comments, also from previous reviewers. I think in general the commits should be smaller too.
Will do a second pass once the commit structure is cleaned up a bit.

chanbackup/backupfile_test.go

keychain/derivation.go

lnencrypt/crypto.go

lnencrypt/crypto_test.go

lntest/mock/secretkeyring.go

lnencrypt/tor.go

tor/cmd_onion.go

tor/onionfile/onion_file.go

…its own package called lnencrypt The functions inside of the crypto.go file in chanbackup (like EncryptPayloadToWriter and DecryptPayloadFromReader) can be used by a lot of things outside of just the chanbackup package. We can't just reference them directly from the chanbackup package because it's likely that it would generate circular dependencies. Therefore we need to move these functions into their own package to be referenced by chanbackup and whatever new functionality that needs them

…er role

…ity to encrypt the Tor private key on disk 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 intead of plaintext. This essentially uses the same method to encrypt the Tor private key as is used to encrypt the Static Channel Backup file.

gkrizek · 2022-02-04T05:04:22Z

@guggero, @orbitalturtle has addressed all your comments. Please take another look. Thanks!

guggero

Looks good, thanks for the update. We're definitely getting closer.

guggero · 2022-02-11T11:31:07Z

chanbackup/backupfile_test.go

@@ -188,7 +190,7 @@ func assertMultiEqual(t *testing.T, a, b *Multi) {
 func TestExtractMulti(t *testing.T) {
 	t.Parallel()

-	keyRing := &mockKeyRing{}
+	keyRing := &lnencrypt.MockKeyRing{}


Can't we move to the mock.MockKeyRing here directly? Instead of creating a new one and then moving away from that in a later commit?

Ah yes so I think I was attempting to split out the mock key change into a new PR as requested here #4458 (comment)

But perhaps it'd be better to resquash it into one commit? Thoughts?

guggero · 2022-02-11T11:37:33Z

tor/onionfile/onion_file.go

+
+// NewOnionFile creates a file-based implementation of the OnionStore interface
+// to store an onion service's private key.
+func NewOnionFile(privateKeyPath string, privateKeyPerm os.FileMode,


Now we still have two OnionFile structs. Can't this be integrated into the existing one? It seems we just add two new parameters and some additional logic. Or is the idea to remove the tor.OnionFile since it's not used anymore? I think the diff could be way smaller if we just integrated this new code into the existing struct.

Yes deleted original onionfile in the Tor package. Moved the onionfile to a new package based on the convo in here: #4458 (comment)

Ooooohhhh, I see now. Yeah I never deleted the OnionFile data in cmd_onion. Will fix that !

lightninglabs-deploy · 2022-05-02T21:16:55Z

@Crypt-iQ: review reminder
@gkrizek, remember to re-request review from reviewers when ready

guggero · 2022-05-05T09:28:18Z

Replaced by #6500.

gkrizek requested review from cfromknecht, Roasbeef and wpaulino as code owners July 12, 2020 04:11

wpaulino added enhancement Improvements to existing features / behaviour security General label for issues/PRs related to the security of the software tor v0.12 labels Jul 13, 2020

gkrizek mentioned this pull request Jul 18, 2020

tls: Add ability to encrypt TLS key on disk #4476

Closed

gkrizek force-pushed the encrypt-tor-privatekey branch from 8dc7725 to 7913317 Compare July 21, 2020 14:47

Roasbeef requested review from Crypt-iQ and removed request for Roasbeef and cfromknecht July 22, 2020 02:55

Roasbeef added this to the 0.12.0 milestone Jul 22, 2020

wpaulino reviewed Aug 10, 2020

View reviewed changes

server.go Outdated Show resolved Hide resolved

tor/add_onion.go Outdated Show resolved Hide resolved

tor/add_onion.go Outdated Show resolved Hide resolved

watchtower/standalone.go Outdated Show resolved Hide resolved

gkrizek force-pushed the encrypt-tor-privatekey branch from 7913317 to a17c609 Compare August 11, 2020 03:14

wpaulino reviewed Aug 11, 2020

View reviewed changes

tor/add_onion.go Outdated Show resolved Hide resolved

gkrizek force-pushed the encrypt-tor-privatekey branch from a17c609 to bb3365e Compare August 11, 2020 20:29

wpaulino reviewed Aug 11, 2020

View reviewed changes

watchtower/standalone.go Outdated Show resolved Hide resolved

watchtower/standalone.go Outdated Show resolved Hide resolved

gkrizek force-pushed the encrypt-tor-privatekey branch from bb3365e to dde6f61 Compare August 14, 2020 00:27

gkrizek force-pushed the encrypt-tor-privatekey branch from dde6f61 to 1e239ec Compare August 15, 2020 01:10

gkrizek force-pushed the encrypt-tor-privatekey branch from 1e239ec to 7d3c553 Compare August 15, 2020 01:14

wpaulino approved these changes Aug 17, 2020

View reviewed changes

wpaulino removed their request for review August 16, 2021 17:23

Roasbeef modified the milestones: v0.14.0, v0.15.0 Oct 19, 2021

gkrizek force-pushed the encrypt-tor-privatekey branch from 9f777c3 to 430c1fc Compare November 3, 2021 04:21

guggero reviewed Jan 4, 2022

View reviewed changes

gkrizek and others added 3 commits February 2, 2022 18:12

keychain: change KeyFamilyStaticBackup name to reflect its new, broad…

14fb4c5

…er role

lntest+lnencrypt+chanbackup: move to mock key in lntest for testing

b7aee08

gkrizek force-pushed the encrypt-tor-privatekey branch from 430c1fc to 8129b6a Compare February 3, 2022 03:43

gkrizek and others added 3 commits February 3, 2022 19:59

docs: Add a section in configuring_tor about the --tor.encryptkey flag

5e0d252

docs: update release notes

a9fcaea

gkrizek force-pushed the encrypt-tor-privatekey branch from 8129b6a to a9fcaea Compare February 4, 2022 05:02

gkrizek requested a review from guggero February 4, 2022 05:03

guggero reviewed Feb 11, 2022

View reviewed changes

orbitalturtle mentioned this pull request Feb 13, 2022

Encrypt tls key updates gkrizek/lnd#8

Open

Roasbeef removed this from the v0.15.0 milestone Apr 12, 2022

orbitalturtle mentioned this pull request May 5, 2022

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

Merged

guggero closed this May 5, 2022

orbitalturtle mentioned this pull request May 11, 2022

tls: Add ability to encrypt TLS key on disk #6527

Merged

orbitalturtle mentioned this pull request Aug 31, 2022

Update Tor module to add option to encrypt Tor key #6526

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gkrizek commented Jul 12, 2020

gkrizek commented Jul 15, 2020

wpaulino left a comment

gkrizek commented Aug 11, 2020

wpaulino left a comment

wpaulino commented Aug 14, 2020

gkrizek commented Aug 15, 2020

Crypt-iQ commented Sep 21, 2020

gkrizek commented Dec 20, 2021

Crypt-iQ commented Jan 3, 2022

guggero left a comment

gkrizek commented Feb 4, 2022

guggero left a comment

guggero Feb 11, 2022

orbitalturtle May 5, 2022

guggero Feb 11, 2022

orbitalturtle May 5, 2022

orbitalturtle May 5, 2022

lightninglabs-deploy commented May 2, 2022

guggero commented May 5, 2022

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

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

Conversation

gkrizek commented Jul 12, 2020

gkrizek commented Jul 15, 2020

wpaulino left a comment

Choose a reason for hiding this comment

gkrizek commented Aug 11, 2020

wpaulino left a comment

Choose a reason for hiding this comment

wpaulino commented Aug 14, 2020

gkrizek commented Aug 15, 2020

Crypt-iQ commented Sep 21, 2020

gkrizek commented Dec 20, 2021

Crypt-iQ commented Jan 3, 2022

guggero left a comment

Choose a reason for hiding this comment

gkrizek commented Feb 4, 2022

guggero left a comment

Choose a reason for hiding this comment

guggero Feb 11, 2022

Choose a reason for hiding this comment

orbitalturtle May 5, 2022

Choose a reason for hiding this comment

guggero Feb 11, 2022

Choose a reason for hiding this comment

orbitalturtle May 5, 2022

Choose a reason for hiding this comment

orbitalturtle May 5, 2022

Choose a reason for hiding this comment

lightninglabs-deploy commented May 2, 2022

guggero commented May 5, 2022