lightninglabs / lightning-terminal

Lightning Terminal: Your Home for Lightning Liquidity
MIT License
487 stars 82 forks source link

root_key_ids keep getting added to lnd even when I delete an LND Account #577

Open AndySchroder opened 1 year ago

AndySchroder commented 1 year ago

root_key_ids keep getting added to lnd even when I delete an LND Account

Expected behavior

When deleting an LND Account, lnd should forget about the root_key_id.

Actual behavior

After deleting an account with litcli, lnd stills has a root_key_id for it.

To reproduce

  1. Run lncli listmacaroonids and see the initial list of root_key_ids.
  2. Run litcli accounts create 150 to create a new LND Account.
  3. Run lncli listmacaroonids again to see the new list of root_key_ids.
  4. Run litcli accounts remove id to remove the account that was just created.
  5. Run lncli listmacaroonids again to see the list of root_key_ids has not shrunk.

System information

Standalone install on ubuntu

Remote lnd

v0.10.1-alpha, self compiled

levmi commented 1 year ago

Hey Andy, thanks for the feedback. Chatting with the team on this one a bit. We have some folks out of office this week who have the most context on this feature so it might take til next week to get a substantive response here, but wanted to let you know that we are looking at it :)

AndySchroder commented 1 year ago

There may be some actual purpose to not delete the root_key_ids forever, but I can't currently think of why. Would like to hear what your team says for sure.

ellemouton commented 12 months ago

Yep, I dont see why we cant just delete it 👍 just want to get a second opinion from @guggero here though to make sure there isnt a security issue with re-using root keys for different macaroons.

guggero commented 12 months ago

There is a "one-to-many" relationship between a macaroon root key and an account (and also between account and LNC session, so this is related to https://github.com/lightninglabs/lightning-terminal/issues/578), so root_key_id <--- n : 1 ---> account <--- n : 1 ---> lnc_session. That's why when deleting/expiring a session we don't automatically remove the account and when removing an account we don't remove the root key. I agree that this is probably quite confusing as there is currently no way in the RPC to specify the root_key_id to use when creating an account, so it's only "one-to-many" under the hood.

Given that we can identify the root key IDs created by litd and can find out if there are multiple accounts using the same, we should be able to clean up. Maybe adding the ability to specify the root key ID to use for an account and also showing that info in the RPC would make the relationship more clear (and that would also allow you to revoke access to multiple accounts/sessions with a single command if you group multiple accounts to the same root key ID).

AndySchroder commented 12 months ago

The way it appears to me, you are creating a new root_key_id for each account. In what scenario are you sharing a root_key_id between multiple accounts?

I would think that deleting a session shouldn't necessarily remove the account because you may want to create a new session for the same account, but deleting an account should delete the account and all sessions (if it does not already do so).

guggero commented 12 months ago

In what scenario are you sharing a root_key_id between multiple accounts?

This is not currently done internally and there is no way to do it over the CLI/RPC either (yet), but I can imagine this being useful as a feature in general. You could create a single root key ID for a specific group of users, create an account for each user but still have the ability to revoke all permissions of the whole user group with a single command.

but deleting an account should delete the account and all sessions (if it does not already do so).

I agree, we should do that (I'm pretty sure we currently don't, as accounts and sessions are mostly independent data structures and only linked through a single attribute).

AndySchroder commented 12 months ago

For now, I'd recommend automatically removing the root_key_ids when removing an account if you don't have a way to link them to anything else. If you ever do add functionality to apply multiple users to the same root_key_id, then consider adding the option to remove root_key_ids when removing the last user from the group.