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

Support changing master password #1981

Closed
kilrau opened this issue Nov 5, 2020 · 4 comments · Fixed by #2007
Closed

Support changing master password #1981

kilrau opened this issue Nov 5, 2020 · 4 comments · Fixed by #2007
Assignees
Labels
enhancement New feature or request P1 top priority
Milestone

Comments

@kilrau
Copy link
Contributor

kilrau commented Nov 5, 2020

This is needed for ExchangeUnion/xud-ui#39 - we are setting a default password in XUD EXPLORER the user "creating" the password is actually "changing" the password.

@sangaman
Copy link
Collaborator

sangaman commented Nov 9, 2020

Something I ran into here while implementing this, lnd only allows password changes while it's in a locked state. Changing the password automatically unlocks lnd and for future unlocks you'd only need the new password, but it's not possible to change the password of a running lnd instance without stopping it and starting it back up into its locked state. Is it fine to replicate this behavior for xud? I can't really think of a simple way to stop and start lnd within xud to implement the password change, so that may be our only practical option, but I wanted to make sure it made sense to others.

One option would be to save the old password in the database and attempt to change the password for lnd on the next time xud starts up, but that is certainly more complex.

One other minor question is what we should do if xud isn't able to change the password for all configured lnds - do we just error the request to change the password? That's our approach for wallet creation, so I figure it makes sense to do the same here lest we wind up with differing passwords between lnd and xud.

@kilrau
Copy link
Contributor Author

kilrau commented Nov 9, 2020

One other minor question is what we should do if xud isn't able to change the password for all configured lnds - do we just error the request to change the password? That's our approach for wallet creation, so I figure it makes sense to do the same here lest we wind up with differing passwords between lnd and xud.

Yes, that's how it should be. Rather a failed password change than differing passwords and unlock errors = unusable lnds later.

Something I ran into here while implementing this, lnd only allows password changes while it's in a locked state. Changing the password automatically unlocks lnd and for future unlocks you'd only need the new password, but it's not possible to change the password of a running lnd instance without stopping it and starting it back up into its locked state. Is it fine to replicate this behavior for xud?

Can lnd+xud just continue running without restart? The next start will require the new password, same for xud. Does anything speak against that?

@kilrau
Copy link
Contributor Author

kilrau commented Nov 9, 2020

When discussing this, it became clear that the lnd restart is needed in order to get lnd into locked state due to lack of a lock grpc call. Since this is far from ideal, I would want to try not replicating this behavior, unless there are technical issues with the following:

  • If there are no lnd swap clients connected, xucli changepassword current_password new_password let's one change xud's password at runtime without setting xud to locked state. The new password will only apply on next xud start and xud continues to function as is
  • If there are lnd swap clients connected, xucli changepassword current_password new_password will trigger restarting lnd's by xud calling StopDaemon and supervisord automatically restarting lnd. Outside of xud-docker there is no guarantee that lnd's come back (that's fine), we probably want a [Y/n] confirmation that lnd swap clients will be restarted for the password change on xucli
  • If one or more of connected lnd's are not in locked state (-> continue with changepassword flow) or ready state (-> StopDaemon to trigger restart), xud will error with a message and code signifying that lnd's are not ready for password change and need manual intervention.
  • xud needs to determine when lnd's are back up in locked state and ok to continue with the changepassword flow after triggering restart and only then ask user's input on the cli so
  • even all this should make sure that lnd's are ready for pwd change, we probably want an output if (and only if) password change for a specific lnd was attempted, but lnd didn't unlock successfully after setting its new password

Let me know what I missed @sangaman

@sangaman
Copy link
Collaborator

  • If there are lnd swap clients connected, xucli changepassword current_password new_password will trigger restarting lnd's by xud calling StopDaemon and supervisord automatically restarting lnd. Outside of xud-docker there is no guarantee that lnd's come back (that's fine), we probably want a [Y/n] confirmation that lnd swap clients will be restarted for the password change on xucli

This part seems challenging. For one thing we may be in the middle of a swap such that we can't safely stop lnd on short notice without causing a swap to fail. But assuming we do, should we wait a fixed amount of time for lnds to come online and fail the call if they're not all up in time? At a minimum it would disrupt market making activity which xob

I'm warming up more to the idea of saving the old password in the database (encrypted) along with which lnds still need their password changed, and trying to change them the next time(s) we unlock xud with the new password.

@kilrau kilrau modified the milestones: 1.3.1, 1.2.2, 1.2.3 Nov 19, 2020
sangaman added a commit that referenced this issue Nov 26, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 27, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
sangaman added a commit that referenced this issue Nov 30, 2020
This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
kilrau pushed a commit that referenced this issue Nov 30, 2020
* feat(db): migration framework

This introduces a framework for tracking a database's version and
migrating from one version to the next when the database schema needs
to be modified. The migrations consist of an array of methods that each
are responsible for upgrading from a particular version, and they are
run in sequence when we detect that the current database version is
lower than the latest version.

* feat(db): migration to db v1

* refactor: cryptoUtils

* test: cryptoUtils

* feat: change master password

This adds the ability to change the master password of an existing xud
node. Changing the password re-encrypts the node key on disk and queues
password changes for all lnd wallets.

Lnd does not currently offer the ability to change the password of an
unlocked, running instance. Instead lnd can only change its password
right after being started while it is still locked. Xud therefore
saves the old password for each lnd wallet to the xud database and
encrypts the old password using the new passwords. On subsequent
unlocks of xud, when we go to unlock lnd wallets we first check whether
we have any old passwords in the database corresponding to any lnd
wallets. If we do, we decrypt the old password and change the password
for lnd, which in turn will unlock lnd.

Closes #1981.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request P1 top priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants