From 00ff934169a3b1be19bf0f9ad872d209d97c9ac7 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 17 May 2023 14:02:57 +0200 Subject: [PATCH 1/5] macaroons: demo GenerateNewRootKey bug This commit adds to the existing TestStoreGenerateNewRootKey to show that the method only successfully regenerates the root key in the default root key ID location. This will be fixed in an upcoming commit. --- macaroons/store_test.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/macaroons/store_test.go b/macaroons/store_test.go index a9a49edda..83692b627 100644 --- a/macaroons/store_test.go +++ b/macaroons/store_test.go @@ -16,6 +16,10 @@ var ( defaultRootKeyIDContext = macaroons.ContextWithRootKeyID( context.Background(), macaroons.DefaultRootKeyID, ) + + nonDefaultRootKeyIDContext = macaroons.ContextWithRootKeyID( + context.Background(), []byte{1}, + ) ) // newTestStore creates a new bolt DB in a temporary directory and then @@ -132,7 +136,9 @@ func TestStore(t *testing.T) { } // TestStoreGenerateNewRootKey tests that a root key can be replaced with a new -// one in the store without changing the password. +// one in the store without changing the password. Also demonstrate that at the +// moment, only the default root key will be replaced. This is a bug that will +// be fixed in an upcoming commit. func TestStoreGenerateNewRootKey(t *testing.T) { _, store := newTestStore(t) @@ -140,23 +146,35 @@ func TestStoreGenerateNewRootKey(t *testing.T) { err := store.GenerateNewRootKey() require.Equal(t, macaroons.ErrStoreLocked, err) - // Unlock the store and read the current key. + // Unlock the store. pw := []byte("weks") err = store.CreateUnlock(&pw) require.NoError(t, err) - oldRootKey, _, err := store.RootKey(defaultRootKeyIDContext) + + // Read the default root key. + oldRootKey1, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) - // Replace the root key with a new random key. + // Read the non-default root-key. + oldRootKey2, _, err := store.RootKey(nonDefaultRootKeyIDContext) + require.NoError(t, err) + + // Attempt to replace the root keys with new random keys. err = store.GenerateNewRootKey() require.NoError(t, err) - // Finally, read the root key from the DB and compare it to the one + // Finally, read both root keys from the DB and compare them to the ones // we got returned earlier. This makes sure that the encryption/ // decryption of the key in the DB worked as expected too. - newRootKey, _, err := store.RootKey(defaultRootKeyIDContext) + // Currently, this is only successful for the default root key and not + // for non-default key. This will be fixed in an upcoming commit. + newRootKey1, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) - require.NotEqual(t, oldRootKey, newRootKey) + require.NotEqual(t, oldRootKey1, newRootKey1) + + newRootKey2, _, err := store.RootKey(nonDefaultRootKeyIDContext) + require.NoError(t, err) + require.Equal(t, oldRootKey2, newRootKey2) } // TestStoreSetRootKey tests that a root key can be set to a specified value. From d43ef314d3e5fee8fcdb149e69ce56e5e5ad65f3 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 17 May 2023 11:51:56 +0200 Subject: [PATCH 2/5] macaroons: Let GenerateNewRootKey be applied to all root keys With this commit, GenerateNewRootKey will regenerate the Default root key and will then also check if any other root keys exist and regenerate those as well. --- macaroons/store.go | 26 +++++++++++++++++++++++++- macaroons/store_test.go | 12 ++++-------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/macaroons/store.go b/macaroons/store.go index 09da98885..7b40dff30 100644 --- a/macaroons/store.go +++ b/macaroons/store.go @@ -325,10 +325,34 @@ func (r *RootKeyStorage) GenerateNewRootKey() error { if bucket == nil { return ErrRootKeyBucketNotFound } + + // The default root key should be created even if it does not + // yet exist, so we do this separately from the rest of the + // root keys. _, err := generateAndStoreNewRootKey( bucket, DefaultRootKeyID, r.encKey, ) - return err + if err != nil { + return err + } + + // Now iterate over all the other root keys that may exist + // and re-generate each of them. + return bucket.ForEach(func(k, v []byte) error { + if bytes.Equal(k, encryptionKeyID) { + return nil + } + + if bytes.Equal(k, DefaultRootKeyID) { + return nil + } + + _, err := generateAndStoreNewRootKey( + bucket, k, r.encKey, + ) + + return err + }) }, func() {}) } diff --git a/macaroons/store_test.go b/macaroons/store_test.go index 83692b627..175635836 100644 --- a/macaroons/store_test.go +++ b/macaroons/store_test.go @@ -135,10 +135,8 @@ func TestStore(t *testing.T) { require.Equal(t, rootID, id) } -// TestStoreGenerateNewRootKey tests that a root key can be replaced with a new -// one in the store without changing the password. Also demonstrate that at the -// moment, only the default root key will be replaced. This is a bug that will -// be fixed in an upcoming commit. +// TestStoreGenerateNewRootKey tests that root keys can be replaced with new +// ones in the store without changing the password. func TestStoreGenerateNewRootKey(t *testing.T) { _, store := newTestStore(t) @@ -159,22 +157,20 @@ func TestStoreGenerateNewRootKey(t *testing.T) { oldRootKey2, _, err := store.RootKey(nonDefaultRootKeyIDContext) require.NoError(t, err) - // Attempt to replace the root keys with new random keys. + // Replace the root keys with new random keys. err = store.GenerateNewRootKey() require.NoError(t, err) // Finally, read both root keys from the DB and compare them to the ones // we got returned earlier. This makes sure that the encryption/ // decryption of the key in the DB worked as expected too. - // Currently, this is only successful for the default root key and not - // for non-default key. This will be fixed in an upcoming commit. newRootKey1, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) require.NotEqual(t, oldRootKey1, newRootKey1) newRootKey2, _, err := store.RootKey(nonDefaultRootKeyIDContext) require.NoError(t, err) - require.Equal(t, oldRootKey2, newRootKey2) + require.NotEqual(t, oldRootKey2, newRootKey2) } // TestStoreSetRootKey tests that a root key can be set to a specified value. From 5ffe5552db3b243fd75225963f9841106071549f Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 17 May 2023 14:11:41 +0200 Subject: [PATCH 3/5] macaroons: demo ChangePassword bug This commits uses TestStoreChangePassword to demonstrate that currently the ChangePassword function only changes the password of the default root key and not that of other root keys. This will be fixed in an upcoming commit. --- macaroons/store_test.go | 29 +++++++++++++++++++++-------- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/macaroons/store_test.go b/macaroons/store_test.go index 175635836..39cff7273 100644 --- a/macaroons/store_test.go +++ b/macaroons/store_test.go @@ -209,20 +209,29 @@ func TestStoreSetRootKey(t *testing.T) { } // TestStoreChangePassword tests that the password for the store can be changed -// without changing the root key. +// without changing the root key. The test also demonstrates that currently, +// this change is only applied to the root key at the default root key ID +// location and not to other root keys. This will be fixed in an upcoming +// commit. func TestStoreChangePassword(t *testing.T) { tempDir, store := newTestStore(t) - // The store must be unlocked to replace the root key. + // The store must be unlocked to replace the root keys. err := store.ChangePassword(nil, nil) require.Equal(t, macaroons.ErrStoreLocked, err) - // Unlock the DB and read the current root key. This will need to stay - // the same after changing the password for the test to succeed. + // Unlock the DB and read the current default root key and one other + // non-default root key. Both of these should stay the same after + // changing the password but currently only the default root key is + // re-encrypted correclty. pw := []byte("weks") err = store.CreateUnlock(&pw) require.NoError(t, err) - rootKey, _, err := store.RootKey(defaultRootKeyIDContext) + + rootKey1, _, err := store.RootKey(defaultRootKeyIDContext) + require.NoError(t, err) + + _, _, err = store.RootKey(nonDefaultRootKeyIDContext) require.NoError(t, err) // Both passwords must be set. @@ -256,9 +265,13 @@ func TestStoreChangePassword(t *testing.T) { err = store.CreateUnlock(&newPw) require.NoError(t, err) - // Finally read the root key from the DB using the new password and - // make sure the root key stayed the same. + // Finally, read the root keys from the DB using the new password and + // make sure the default root key stayed the same but that the + // non-default root key could not be decrypted. rootKeyDb, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) - require.Equal(t, rootKey, rootKeyDb) + require.Equal(t, rootKey1, rootKeyDb) + + _, _, err = store.RootKey(nonDefaultRootKeyIDContext) + require.ErrorContains(t, err, "unable to decrypt") } From f9d99153e04cfa2fead593f57f0278fe2cf6f784 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 17 May 2023 11:47:43 +0200 Subject: [PATCH 4/5] macaroons: let ChangePassword re-encrypt all root keys The ChangePasswords method should re-encrypt all the root keys found in the store, not just the default root key. --- macaroons/store.go | 64 ++++++++++++++++++++++++++++------------- macaroons/store_test.go | 22 ++++++-------- 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/macaroons/store.go b/macaroons/store.go index 7b40dff30..cca548d98 100644 --- a/macaroons/store.go +++ b/macaroons/store.go @@ -54,6 +54,10 @@ var ( // ErrEncKeyNotFound specifies that there was no encryption key found // even if one was expected to be generated. ErrEncKeyNotFound = fmt.Errorf("macaroon encryption key not found") + + // ErrDefaultRootKeyNotFound is returned when the default root key is + // not found in the DB when it is expected to be. + ErrDefaultRootKeyNotFound = fmt.Errorf("default root key not found") ) // RootKeyStorage implements the bakery.RootKeyStorage interface. @@ -140,8 +144,8 @@ func (r *RootKeyStorage) CreateUnlock(password *[]byte) error { }, func() {}) } -// ChangePassword decrypts the macaroon root key with the old password and then -// encrypts it again with the new password. +// ChangePassword decrypts all the macaroon root keys with the old password and +// then encrypts them again with the new password. func (r *RootKeyStorage) ChangePassword(oldPw, newPw []byte) error { // We need the store to already be unlocked. With this we can make sure // that there already is a key in the DB. @@ -159,19 +163,18 @@ func (r *RootKeyStorage) ChangePassword(oldPw, newPw []byte) error { if bucket == nil { return ErrRootKeyBucketNotFound } - encKeyDb := bucket.Get(encryptionKeyID) - rootKeyDb := bucket.Get(DefaultRootKeyID) - // Both the encryption key and the root key must be present - // otherwise we are in the wrong state to change the password. - if len(encKeyDb) == 0 || len(rootKeyDb) == 0 { + // The encryption key must be present, otherwise we are in the + // wrong state to change the password. + encKeyDB := bucket.Get(encryptionKeyID) + if len(encKeyDB) == 0 { return ErrEncKeyNotFound } // Unmarshal parameters for old encryption key and derive the // old key with them. encKeyOld := &snacl.SecretKey{} - err := encKeyOld.Unmarshal(encKeyDb) + err := encKeyOld.Unmarshal(encKeyDB) if err != nil { return err } @@ -188,21 +191,42 @@ func (r *RootKeyStorage) ChangePassword(oldPw, newPw []byte) error { return err } - // Now try to decrypt the root key with the old encryption key, - // encrypt it with the new one and then store it in the DB. - decryptedKey, err := encKeyOld.Decrypt(rootKeyDb) + // foundDefaultRootKey is used to keep track of if we have + // found and re-encrypted the default root key so that we can + // return an error if it is not found. + var foundDefaultRootKey bool + err = bucket.ForEach(func(k, v []byte) error { + // Skip the key if it is the encryption key ID since + // we do not want to re-encrypt this. + if bytes.Equal(k, encryptionKeyID) { + return nil + } + + if bytes.Equal(k, DefaultRootKeyID) { + foundDefaultRootKey = true + } + + // Now try to decrypt the root key with the old + // encryption key, encrypt it with the new one and then + // store it in the DB. + decryptedKey, err := encKeyOld.Decrypt(v) + if err != nil { + return err + } + + encryptedKey, err := encKeyNew.Encrypt(decryptedKey) + if err != nil { + return err + } + + return bucket.Put(k, encryptedKey) + }) if err != nil { return err } - rootKey := make([]byte, len(decryptedKey)) - copy(rootKey, decryptedKey) - encryptedKey, err := encKeyNew.Encrypt(rootKey) - if err != nil { - return err - } - err = bucket.Put(DefaultRootKeyID, encryptedKey) - if err != nil { - return err + + if !foundDefaultRootKey { + return ErrDefaultRootKeyNotFound } // Finally, store the new encryption key parameters in the DB diff --git a/macaroons/store_test.go b/macaroons/store_test.go index 39cff7273..ace1e764a 100644 --- a/macaroons/store_test.go +++ b/macaroons/store_test.go @@ -209,10 +209,7 @@ func TestStoreSetRootKey(t *testing.T) { } // TestStoreChangePassword tests that the password for the store can be changed -// without changing the root key. The test also demonstrates that currently, -// this change is only applied to the root key at the default root key ID -// location and not to other root keys. This will be fixed in an upcoming -// commit. +// without changing the root keys. func TestStoreChangePassword(t *testing.T) { tempDir, store := newTestStore(t) @@ -222,8 +219,7 @@ func TestStoreChangePassword(t *testing.T) { // Unlock the DB and read the current default root key and one other // non-default root key. Both of these should stay the same after - // changing the password but currently only the default root key is - // re-encrypted correclty. + // changing the password for the test to succeed. pw := []byte("weks") err = store.CreateUnlock(&pw) require.NoError(t, err) @@ -231,7 +227,7 @@ func TestStoreChangePassword(t *testing.T) { rootKey1, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) - _, _, err = store.RootKey(nonDefaultRootKeyIDContext) + rootKey2, _, err := store.RootKey(nonDefaultRootKeyIDContext) require.NoError(t, err) // Both passwords must be set. @@ -266,12 +262,12 @@ func TestStoreChangePassword(t *testing.T) { require.NoError(t, err) // Finally, read the root keys from the DB using the new password and - // make sure the default root key stayed the same but that the - // non-default root key could not be decrypted. - rootKeyDb, _, err := store.RootKey(defaultRootKeyIDContext) + // make sure that both root keys stayed the same. + rootKeyDB1, _, err := store.RootKey(defaultRootKeyIDContext) require.NoError(t, err) - require.Equal(t, rootKey1, rootKeyDb) + require.Equal(t, rootKey1, rootKeyDB1) - _, _, err = store.RootKey(nonDefaultRootKeyIDContext) - require.ErrorContains(t, err, "unable to decrypt") + rootKeyDB2, _, err := store.RootKey(nonDefaultRootKeyIDContext) + require.NoError(t, err) + require.Equal(t, rootKey2, rootKeyDB2) } From 0787cb11783b43dea4904978b374a87760424ae0 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 17 May 2023 12:11:38 +0200 Subject: [PATCH 5/5] docs: add release note for 7705 --- docs/release-notes/release-notes-0.17.0.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.0.md b/docs/release-notes/release-notes-0.17.0.md index d2b3b3686..00132fdc0 100644 --- a/docs/release-notes/release-notes-0.17.0.md +++ b/docs/release-notes/release-notes-0.17.0.md @@ -49,6 +49,10 @@ unlock or create. * [Restore support](https://github.com/lightningnetwork/lnd/pull/7678) for `PKCS8`-encoded cert private keys. +* [Re-encrypt/regenerate](https://github.com/lightningnetwork/lnd/pull/7705) + all macaroon DB root keys on `ChangePassword`/`GenerateNewRootKey` + respectively. + ## Code Health * Updated [our fork for serializing protobuf as JSON to be based on the