tor: Add option to encrypt Tor private key

This commit lays the groundwork for enabling the option of encrypting a Tor private key on disk, and removes the onion type parameters from the OnionStore interface methods, since they are unused.

tor/cmd_onion.go

@@ -1,13 +1,22 @@
 package tor
 
 import (
+	"bytes"
 	"errors"
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 )
 
 var (
+	// ErrEncryptedTorPrivateKey is thrown when a tor private key is
+	// encrypted, but the user requested an unencrypted key.
+	ErrEncryptedTorPrivateKey = errors.New("it appears the Tor private key " +
+		"is encrypted but you didn't pass the --tor.encryptkey flag. " +
+		"Please restart lnd with the --tor.encryptkey flag or delete " +
+		"the Tor key file for regeneration")
+
 	// ErrNoPrivateKey is an error returned by the OnionStore.PrivateKey
 	// method when a private key hasn't yet been stored.
 	ErrNoPrivateKey = errors.New("private key not found")
@@ -24,19 +33,34 @@ const (
 	V3
 )
 
+const (
+	// V2KeyParam is a parameter that Tor accepts for a new V2 service.
+	V2KeyParam = "RSA1024"
+
+	// V3KeyParam is a parameter that Tor accepts for a new V3 service.
+	V3KeyParam = "ED25519-V3"
+)
+
 // OnionStore is a store containing information about a particular onion
 // service.
 type OnionStore interface {
 	// StorePrivateKey stores the private key according to the
 	// implementation of the OnionStore interface.
-	StorePrivateKey(OnionType, []byte) error
+	StorePrivateKey([]byte) error
 
 	// PrivateKey retrieves a stored private key. If it is not found, then
 	// ErrNoPrivateKey should be returned.
-	PrivateKey(OnionType) ([]byte, error)
+	PrivateKey() ([]byte, error)
 
 	// DeletePrivateKey securely removes the private key from the store.
-	DeletePrivateKey(OnionType) error
+	DeletePrivateKey() error
+}
+
+// EncrypterDecrypter is used for encrypting and decrypting the onion service
+// private key.
+type EncrypterDecrypter interface {
+	EncryptPayloadToWriter([]byte, io.Writer) error
+	DecryptPayloadFromReader(io.Reader) ([]byte, error)
 }
 
 // OnionFile is a file-based implementation of the OnionStore interface that
@@ -44,6 +68,8 @@ type OnionStore interface {
 type OnionFile struct {
 	privateKeyPath string
 	privateKeyPerm os.FileMode
+	encryptKey     bool
+	encrypter      EncrypterDecrypter
 }
 
 // A compile-time constraint to ensure OnionFile satisfies the OnionStore
@@ -52,31 +78,85 @@ var _ OnionStore = (*OnionFile)(nil)
 
 // 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) *OnionFile {
+func NewOnionFile(privateKeyPath string, privateKeyPerm os.FileMode,
+	encryptKey bool, encrypter EncrypterDecrypter) *OnionFile {
 
 	return &OnionFile{
 		privateKeyPath: privateKeyPath,
 		privateKeyPerm: privateKeyPerm,
+		encryptKey:     encryptKey,
+		encrypter:      encrypter,
 	}
 }
 
-// StorePrivateKey stores the private key at its expected path.
-func (f *OnionFile) StorePrivateKey(_ OnionType, privateKey []byte) error {
-	return ioutil.WriteFile(f.privateKeyPath, privateKey, f.privateKeyPerm)
+// StorePrivateKey stores the private key at its expected path. It also
+// encrypts the key before storing it if requested.
+func (f *OnionFile) StorePrivateKey(privateKey []byte) error {
+	privateKeyContent := privateKey
+
+	if f.encryptKey {
+		var b bytes.Buffer
+		err := f.encrypter.EncryptPayloadToWriter(
+			privateKey, &b,
+		)
+		if err != nil {
+			return err
+		}
+		privateKeyContent = b.Bytes()
+	}
+
+	err := ioutil.WriteFile(
+		f.privateKeyPath, privateKeyContent, f.privateKeyPerm,
+	)
+	if err != nil {
+		return fmt.Errorf("unable to write private key "+
+			"to file: %v", err)
+	}
+	return nil
 }
 
-// PrivateKey retrieves the private key from its expected path. If the file
-// does not exist, then ErrNoPrivateKey is returned.
-func (f *OnionFile) PrivateKey(_ OnionType) ([]byte, error) {
-	if _, err := os.Stat(f.privateKeyPath); os.IsNotExist(err) {
+// PrivateKey retrieves the private key from its expected path. If the file does
+// not exist, then ErrNoPrivateKey is returned.
+func (f *OnionFile) PrivateKey() ([]byte, error) {
+	_, err := os.Stat(f.privateKeyPath)
+	if err != nil && errors.Is(err, os.ErrNotExist) {
 		return nil, ErrNoPrivateKey
 	}
-	return ioutil.ReadFile(f.privateKeyPath)
+
+	// Try to read the Tor private key to pass into the AddOnion call.
+	privateKeyContent, err := ioutil.ReadFile(f.privateKeyPath)
+	if err != nil {
+		return nil, err
+	}
+
+	// If the privateKey starts with either v2 or v3 key params then
+	// it's likely not encrypted and we can return the data as is.
+	if bytes.HasPrefix(privateKeyContent, []byte(V2KeyParam)) ||
+		bytes.HasPrefix(privateKeyContent, []byte(V3KeyParam)) {
+
+		return privateKeyContent, nil
+	}
+
+	// If the privateKeyContent is encrypted but --tor.encryptkey
+	// wasn't set we return an error.
+	if !f.encryptKey {
+		return nil, ErrEncryptedTorPrivateKey
+	}
+
+	// Attempt to decrypt the key.
+	reader := bytes.NewReader(privateKeyContent)
+	privateKeyContent, err = f.encrypter.DecryptPayloadFromReader(
+		reader,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	return privateKeyContent, nil
 }
 
 // DeletePrivateKey removes the file containing the private key.
-func (f *OnionFile) DeletePrivateKey(_ OnionType) error {
+func (f *OnionFile) DeletePrivateKey() error {
 	return os.Remove(f.privateKeyPath)
 }
 
@@ -117,13 +197,13 @@ func (c *Controller) prepareKeyparam(cfg AddOnionConfig) (string, error) {
 	switch cfg.Type {
 	// TODO(yy): drop support for v2.
 	case V2:
-		keyParam = "NEW:RSA1024"
+		keyParam = "NEW:" + V2KeyParam
 	case V3:
-		keyParam = "NEW:ED25519-V3"
+		keyParam = "NEW:" + V3KeyParam
 	}
 
 	if cfg.Store != nil {
-		privateKey, err := cfg.Store.PrivateKey(cfg.Type)
+		privateKey, err := cfg.Store.PrivateKey()
 		switch err {
 		// Proceed to request a new onion service.
 		case ErrNoPrivateKey:
@@ -142,11 +222,13 @@ func (c *Controller) prepareKeyparam(cfg AddOnionConfig) (string, error) {
 
 // prepareAddOnion constructs a cmd command string based on the specified
 // config.
-func (c *Controller) prepareAddOnion(cfg AddOnionConfig) (string, error) {
+func (c *Controller) prepareAddOnion(cfg AddOnionConfig) (string, string,
+	error) {
+
 	// Create the keyParam.
 	keyParam, err := c.prepareKeyparam(cfg)
 	if err != nil {
-		return "", err
+		return "", "", err
 	}
 
 	// Now, we'll create a mapping from the virtual port to each target
@@ -178,7 +260,7 @@ func (c *Controller) prepareAddOnion(cfg AddOnionConfig) (string, error) {
 	// await its response.
 	cmd := fmt.Sprintf("ADD_ONION %s %s", keyParam, portParam)
 
-	return cmd, nil
+	return cmd, keyParam, nil
 }
 
 // AddOnion creates an ephemeral onion service and returns its onion address.
@@ -202,7 +284,7 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 	}
 
 	// Construct the cmd command.
-	cmd, err := c.prepareAddOnion(cfg)
+	cmd, keyParam, err := c.prepareAddOnion(cfg)
 	if err != nil {
 		return nil, err
 	}
@@ -234,11 +316,19 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 		return nil, errors.New("service id not found in reply")
 	}
 
-	// If a new onion service was created and an onion store was provided,
-	// we'll store its private key to disk in the event that it needs to be
-	// recreated later on.
-	if privateKey, ok := replyParams["PrivateKey"]; cfg.Store != nil && ok {
-		err := cfg.Store.StorePrivateKey(cfg.Type, []byte(privateKey))
+	// If a new onion service was created, use the new private key for
+	// storage.
+	newPrivateKey, ok := replyParams["PrivateKey"]
+	if ok {
+		keyParam = newPrivateKey
+	}
+
+	// If an onion store was provided and a key return wasn't requested,
+	// we'll store its private key to disk in the event that it needs to
+	// be recreated later on. We write the private key to disk every time
+	// in case the user toggles the --tor.encryptkey flag.
+	if cfg.Store != nil {
+		err := cfg.Store.StorePrivateKey([]byte(keyParam))
 		if err != nil {
 			return nil, fmt.Errorf("unable to write private key "+
 				"to file: %v", err)
@@ -254,6 +344,7 @@ func (c *Controller) AddOnion(cfg AddOnionConfig) (*OnionAddr, error) {
 	return &OnionAddr{
 		OnionService: serviceID + ".onion",
 		Port:         cfg.VirtualPort,
+		PrivateKey:   keyParam,
 	}, nil
 }
 

tor/cmd_onion_test.go

@@ -1,8 +1,8 @@
 package tor
 
 import (
-	"bytes"
 	"errors"
+	"io"
 	"path/filepath"
 	"testing"
 
@@ -10,40 +10,61 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-// TestOnionFile tests that the OnionFile implementation of the OnionStore
+var (
+	privateKey = []byte("RSA1024 hide_me_plz")
+	anotherKey = []byte("another_key")
+)
+
+// TestOnionFile tests that the File implementation of the OnionStore
 // interface behaves as expected.
 func TestOnionFile(t *testing.T) {
 	t.Parallel()
 
-	privateKey := []byte("hide_me_plz")
-	privateKeyPath := filepath.Join(t.TempDir(), "secret")
+	tempDir := t.TempDir()
+	privateKeyPath := filepath.Join(tempDir, "secret")
+	mockEncrypter := MockEncrypter{}
 
 	// Create a new file-based onion store. A private key should not exist
 	// yet.
-	onionFile := NewOnionFile(privateKeyPath, 0600)
-	if _, err := onionFile.PrivateKey(V2); err != ErrNoPrivateKey {
-		t.Fatalf("expected ErrNoPrivateKey, got \"%v\"", err)
-	}
+	onionFile := NewOnionFile(
+		privateKeyPath, 0600, false, mockEncrypter,
+	)
+	_, err := onionFile.PrivateKey()
+	require.ErrorIs(t, err, ErrNoPrivateKey)
 
 	// Store the private key and ensure what's stored matches.
-	if err := onionFile.StorePrivateKey(V2, privateKey); err != nil {
-		t.Fatalf("unable to store private key: %v", err)
-	}
-	storePrivateKey, err := onionFile.PrivateKey(V2)
-	require.NoError(t, err, "unable to retrieve private key")
-	if !bytes.Equal(storePrivateKey, privateKey) {
-		t.Fatalf("expected private key \"%v\", got \"%v\"",
-			string(privateKey), string(storePrivateKey))
-	}
+	err = onionFile.StorePrivateKey(privateKey)
+	require.NoError(t, err)
+
+	storePrivateKey, err := onionFile.PrivateKey()
+	require.NoError(t, err)
+	require.Equal(t, storePrivateKey, privateKey)
 
 	// Finally, delete the private key. We should no longer be able to
 	// retrieve it.
-	if err := onionFile.DeletePrivateKey(V2); err != nil {
-		t.Fatalf("unable to delete private key: %v", err)
-	}
-	if _, err := onionFile.PrivateKey(V2); err != ErrNoPrivateKey {
-		t.Fatal("found deleted private key")
-	}
+	err = onionFile.DeletePrivateKey()
+	require.NoError(t, err)
+
+	_, err = onionFile.PrivateKey()
+	require.ErrorIs(t, err, ErrNoPrivateKey)
+
+	// Create a new file-based onion store that encrypts the key this time
+	// to ensure that an encrypted key is properly handled.
+	encryptedOnionFile := NewOnionFile(
+		privateKeyPath, 0600, true, mockEncrypter,
+	)
+
+	err = encryptedOnionFile.StorePrivateKey(privateKey)
+	require.NoError(t, err)
+
+	storedPrivateKey, err := encryptedOnionFile.PrivateKey()
+	require.NoError(t, err, "unable to retrieve encrypted private key")
+	// Check that PrivateKey returns anotherKey, to make sure the mock
+	// decrypter is actually called.
+	require.Equal(t, storedPrivateKey, anotherKey)
+
+	err = encryptedOnionFile.DeletePrivateKey()
+	require.NoError(t, err)
 }
 
 // TestPrepareKeyParam checks that the key param is created as expected.
@@ -63,7 +84,7 @@ func TestPrepareKeyParam(t *testing.T) {
 
 	// Create a mock store which returns the test private key.
 	store := &mockStore{}
-	store.On("PrivateKey", cfg.Type).Return(testKey, nil)
+	store.On("PrivateKey").Return(testKey, nil)
 
 	// Check that the test private is returned.
 	cfg = AddOnionConfig{Type: V3, Store: store}
@@ -75,7 +96,7 @@ func TestPrepareKeyParam(t *testing.T) {
 
 	// Create a mock store which returns ErrNoPrivateKey.
 	store = &mockStore{}
-	store.On("PrivateKey", cfg.Type).Return(nil, ErrNoPrivateKey)
+	store.On("PrivateKey").Return(nil, ErrNoPrivateKey)
 
 	// Check that the V3 keyParam is returned.
 	cfg = AddOnionConfig{Type: V3, Store: store}
@@ -87,7 +108,7 @@ func TestPrepareKeyParam(t *testing.T) {
 
 	// Create a mock store which returns an dummy error.
 	store = &mockStore{}
-	store.On("PrivateKey", cfg.Type).Return(nil, dummyErr)
+	store.On("PrivateKey").Return(nil, dummyErr)
 
 	// Check that an error is returned.
 	cfg = AddOnionConfig{Type: V3, Store: store}
@@ -158,14 +179,14 @@ func TestPrepareAddOnion(t *testing.T) {
 		tc := tc
 
 		if tc.cfg.Store != nil {
-			store.On("PrivateKey", tc.cfg.Type).Return(
+			store.On("PrivateKey").Return(
 				testKey, tc.expectedErr,
 			)
 		}
 
 		controller := NewController("", tc.targetIPAddress, "")
 		t.Run(tc.name, func(t *testing.T) {
-			cmd, err := controller.prepareAddOnion(tc.cfg)
+			cmd, _, err := controller.prepareAddOnion(tc.cfg)
 			require.Equal(t, tc.expectedErr, err)
 			require.Equal(t, tc.expectedCmd, cmd)
 
@@ -184,20 +205,27 @@ type mockStore struct {
 // interface.
 var _ OnionStore = (*mockStore)(nil)
 
-func (m *mockStore) StorePrivateKey(ot OnionType, key []byte) error {
-	args := m.Called(ot, key)
+func (m *mockStore) StorePrivateKey(key []byte) error {
+	args := m.Called(key)
 	return args.Error(0)
 }
 
-func (m *mockStore) PrivateKey(ot OnionType) ([]byte, error) {
-	args := m.Called(ot)
-	if args.Get(0) == nil {
-		return nil, args.Error(1)
-	}
-	return args.Get(0).([]byte), args.Error(1)
+func (m *mockStore) PrivateKey() ([]byte, error) {
+	args := m.Called()
+	return []byte("hide_me_plz"), args.Error(1)
 }
 
-func (m *mockStore) DeletePrivateKey(ot OnionType) error {
-	args := m.Called(ot)
+func (m *mockStore) DeletePrivateKey() error {
+	args := m.Called()
 	return args.Error(0)
 }
+
+type MockEncrypter struct{}
+
+func (m MockEncrypter) EncryptPayloadToWriter(_ []byte, _ io.Writer) error {
+	return nil
+}
+
+func (m MockEncrypter) DecryptPayloadFromReader(_ io.Reader) ([]byte, error) {
+	return anotherKey, nil
+}

tor/onionaddr.go

@@ -45,6 +45,9 @@ type OnionAddr struct {
 
 	// Port is the port of the onion address.
 	Port int
+
+	// PrivateKey is the onion address' private key.
+	PrivateKey string
 }
 
 // A compile-time check to ensure that OnionAddr implements the net.Addr