Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

macaroons: ensure all root keys are re-encrypted or regenerated #7705

Merged

Conversation

ellemouton
Copy link
Collaborator

Currently, the macaroon store ChangePassword method will only re-encrypt the root key stored at the
default root key ID location (0) and GenerateNewRootKey will only re-generate the root key stored at the default location. This PR updates these methods so that all root keys stored in the db are re-encrypted/re-generated respectively.

This will fix an issue noticed in Litd: lightninglabs/lightning-terminal#549

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find and nice fix! Main missing thing is a small unit test, other than that looks good to me!

macaroons/store.go Show resolved Hide resolved
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.
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.
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice, LGTM 🎉

Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥛

macaroons/store_test.go Outdated Show resolved Hide resolved
macaroons/store.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@bhandras bhandras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥛

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.
The ChangePasswords method should re-encrypt all the root keys found
in the store, not just the default root key.
@guggero guggero merged commit 2fd40d1 into lightningnetwork:master May 17, 2023
22 of 24 checks passed
@ellemouton ellemouton deleted the macStoreRootKeyReEncyption branch May 22, 2023 07:28
@saubyk saubyk added this to the v0.16.3 milestone May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants