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

Revert to old tower keys #50

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mariocynicys
Copy link
Collaborator

Changes:

  • Renames --overwritekey flag to --generatenewkey.
  • Adds a new option --towerkey <keyindex>, which allows the usage of old tower keys based on their indices (first key have index 0, second has 1, and so on).
  • Makes the dbm::DBM::load_tower_key() method accept an optional key index to load.

Closes #49

This will allow the user to use old tower secret keys given their ids
use_new_key -> generate_new_key

use_key -> tower_key
@sr-gi
Copy link
Member

sr-gi commented Apr 25, 2022

@meryacine loved the proactivity regarding this. However, there's a rationale for not having had this feature in the first place:

The tower identity is supposed to be long lived, that's the way of making it reputationally accountable (i.e. the longer a tower has been properly behaving, the better). I don't think we should encourage easily changing ids, since users expect the tower id not to change though-out their subscription (that's the public key they use to verify data coming from the tower).

There's an exception here, which is the tower key being compromised, in which case it should be re-generated (hence the option being offered).

I'm open to discuss this since it is, at the end of the day, a design decision. To me, this could make sense if we approach it from the perspective of being able to recover an old key if --overwritekey was called by mistake (that's mainly why the database stores old keys, mistakes happen), but I'm not too confident about allowing key rotation.

@mariocynicys
Copy link
Collaborator Author

Shouldn't changing the tower keys already disrepute the old keys since the response signature would be invalid when trying to verify it using the old pubkey (the tower failed to respond). This by itself would discourage towers to rotate their keys.

@mariocynicys
Copy link
Collaborator Author

Considering that reverting to old keys should only be used as a backup method for mistakes, one way to restrict using it for key rotation is by deleting all the keys past the key we are reverting to.
This won't allow the tower to store many identities and swap between them freely.

@sr-gi
Copy link
Member

sr-gi commented May 1, 2022

I'm reconsidering whether the key rotation is a good idea in the first place. Had a chat with @cdecker recently and CoreLN does not allow that in the first place. I'm trying to poll what node implementors do in this case and maybe go the same direction (i.e. only store the last key in the db and maybe offer a fresh start option that may wipe the whole thing).

https://twitter.com/sr_gi/status/1520741992975769602

Any thoughts on that?

@sr-gi
Copy link
Member

sr-gi commented Jun 22, 2022

Putting more thought into this, I think key rotation (or at least bootstrapping with a fresh key from cmd) is a good idea, at least for testing purposes.

If we end up only allowing this for testing (and therefore discouraging this for normal use) we can simply remove the old key storage and just keep the latest key.

@sr-gi sr-gi added the discuss Topics were discussion is encouraged label Jun 22, 2022
@mariocynicys
Copy link
Collaborator Author

Reading this again...

Considering that reverting to old keys should only be used as a backup method for mistakes, one way to restrict using it for key rotation is by deleting all the keys past the key we are reverting to.
This won't allow the tower to store many identities and swap between them freely.

Gave me a though: what if we had only 2 slots (maximum of 2 stored keys at any given time). So the tower owner will have only one chance to retrieve the old key if they are overwritten (if you overwrite the key twice = it's gone forever).
This is basically us deleting the third latest key instead of the second (and note that recovering the second key deletes the first, no recovery for the third key).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Topics were discussion is encouraged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow reverting to old tower keys after using --overwritekey
2 participants