ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 49 forks source link

Support changing master password #1981

Closed kilrau closed 3 years ago

kilrau commented 3 years ago

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

sangaman commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

Let me know what I missed @sangaman

sangaman commented 3 years ago
  • 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.