feat: change master password #2007
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sangaman
added
enhancement
raladev
requested changes
Nov 26, 2020
There was a problem hiding this comment.
-
Enter a password:change toEnter new password: - add sentence after password changing
lnd restart is required to Apply change to lnd. - it would be good to support conception of password changing -> Enter old password -> Enter new password -> Re-enter new password. I dont like the idea that anyone can change password without any additional data if node is unlocked (even if anyone can withdraw all crypto if node is unlocked :) ).
- suggestion: make wrapper on xud-docker side for changepass call that will restart lnds automatically;
- got err when tried to unlock the node after password changing + my lnd were not unlocked on xud unlock (I unlocked it manually using old password)
Steps:
bash xud.sh -b pwd- create wallet, wait for channels (if u have no, i used old env)
changepass->set new pass- down (stop and remove all containers)
bash xud.sh -b pwd- start env again -> create new containers
utils output:
Enter master xud password:
Error: 2 UNKNOWN: error:0606506D:digital envelope routines:EVP_DecryptFinal_ex:wrong final block length
Enter master xud password:
xud is running and unlocked, try checking its status with 'xucli getinfo'
Enter master xud password:
Ctr+C
simnet > status
┌─────────┬──────────────────────────────────────────────────┐
│ SERVICE │ STATUS │
├─────────┼──────────────────────────────────────────────────┤
│ lndbtc │ Wallet locked. Unlock with xucli unlock. │
├─────────┼──────────────────────────────────────────────────┤
│ lndltc │ Wallet locked. Unlock with xucli unlock. │
├─────────┼──────────────────────────────────────────────────┤
│ connext │ Starting... │
├─────────┼──────────────────────────────────────────────────┤
│ xud │ Waiting for lndbtc, lndltc, connext │
└─────────┴──────────────────────────────────────────────────┘
simnet >
xud logs
26/11/2020 12:32:51.422 [RPC] error: call /xudrpc.Xud/GetInfo errored with code 12: xud is locked
26/11/2020 12:32:52.143 [RPC] error: call /xudrpc.Xud/GetInfo errored with code 12: xud is locked
26/11/2020 12:33:10.284 [RPC] error: call /xudrpc.XudInit/UnlockNode errored with code 3: password is incorrect
26/11/2020 12:33:14.296 [GLOBAL] info: Local nodePubKey is 0343722e976e24e2115199822316d99773e2d12b68c2d152e6b298490274fcee9a
26/11/2020 12:33:14.309 [RPC] error: call /xudrpc.XudInit/UnlockNode errored with code 2: error:0606506D:digital envelope routines:EVP_DecryptFinal_ex:wrong final block length
26/11/2020 12:33:14.313 [P2P] info: Connecting to simnet XU network
26/11/2020 12:33:14.317 [P2P] info: p2p server listening on 0.0.0.0:28885
26/11/2020 12:33:14.318 [P2P] debug: Verifying reachability of advertised address: zd7v22z76zqemn3w6krjb6rb34chtrhbvqquabwixkhsubfwutciy4id.onion:28885
26/11/2020 12:33:14.336 [P2P] info: Connecting to known / previously connected peers
26/11/2020 12:33:14.337 [P2P] debug: creating new outbound socket connection to xud1.simnet.exchangeunion.com:28885
26/11/2020 12:33:14.339 [P2P] debug: creating new outbound socket connection to xud.kilrau.com:28885
lndbtc logs:
2020-11-26 12:32:48.111 [DBG] BTCN: Received headers (num 0) from 35.231.222.142:38555 (outbound)
2020-11-26 12:32:48.513 [DBG] BTCN: Received cfheaders (stop_hash=05060bf2ecd36d91fa5c6725083f0e2ebe22185218d5a860fdeac8002fd23373, num_filter_hashes=5) from 35.231.222.142:38555 (outbound)
2020-11-26 12:32:48.513 [DBG] BTCN: Writing filter headers up to height=219523, hash=05060bf2ecd36d91fa5c6725083f0e2ebe22185218d5a860fdeac8002fd23373, new_tip=9780b3ecd08b43c849b988f65a0dbb9ebfcd028a5c6f634a4d4bc20801f8d7eb
2020-11-26 12:33:23.851 [DBG] BTCN: Received inv (block 5168825a08e70a11da7298205c0214f94fb1483c46404d9c97c3860ce7f4e326) from 35.231.222.142:38555 (outbound)
2020-11-26 12:33:23.851 [DBG] BTCN: Sending getheaders (locator 05060bf2ecd36d91fa5c6725083f0e2ebe22185218d5a860fdeac8002fd23373, stop 5168825a08e70a11da7298205c0214f94fb1483c46404d9c97c3860ce7f4e326) to 35.231.222.142:38555 (outbound)
2020-11-26 12:33:24.066 [DBG] BTCN: Received headers (num 1) from 35.231.222.142:38555 (outbound)
2020-11-26 12:33:24.066 [INF] BTCN: Processed 6 blocks in the last 38.09s (height 219524, 2020-11-26 12:33:23 +0000 UTC)
2020-11-26 12:33:24.072 [DBG] BTCN: Sending getheaders (locator 5168825a08e70a11da7298205c0214f94fb1483c46404d9c97c3860ce7f4e326, stop 0000000000000000000000000000000000000000000000000000000000000000) to 35.231.222.142:38555 (outbound)
lnd unlocking:
simnet > lndbtc-lncli unlock
Input wallet password: (new xud password)
[lncli] rpc error: code = Unknown desc = invalid passphrase for master public key
simnet > lndbtc-lncli unlock
Input wallet password: (old xud password)
lnd successfully unlocked!
simnet >
Not needed since lnd is in unlocked state in this moment and continues to be fully functioning. Next time the environment/lnd restarts, the new password is active, I don't see any reason to trigger a restart / ask the user to restart lnd (and this is probably also the reason why the lightning labs team decided not to do that on lnd).
Yep, agree (and lnd requires current password first too, it's convention):
As i said above, don't think that's needed.
Just to be clear what happened @sangaman : Roman used |
This was more or less what I thought, since we're not expecting the user to be unlocking lnd manually I didn't think a message was necessary. I did think about possibly adding a message like "lnd passwords will be changed next time xud is unlocked" maybe, thoughts?
I skipped this step because the original password is already needed to unlock xud before changing the password is allowed, but I can add it in since both you and kilrau thought this. I'm looking into why the lnd passwords weren't changed now. |
👍
Yes, for the sake of being in line with lnd and others |
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.
sangaman
force-pushed
the
change-pass
branch
from
25a2031
to
ed8e6ad
Compare
|
Feedback applied and the bug with changing lnd password was fixed, apparently taking the encrypted bytes and converting them to Changing the password now requires the old password as well and a message of |
sangaman
force-pushed
the
change-pass
branch
2 times, most recently
from
43c8732
to
6ec1818
Compare
xud logs: lndbtc logs (found nothing in it): |
sangaman
force-pushed
the
change-pass
branch
2 times, most recently
from
1e836ef
to
af6b545
Compare
|
Steps:
Actual result:
Note: |
sangaman
force-pushed
the
change-pass
branch
from
af6b545
to
d8de798
Compare
|
OK this should be good to go now, it passes my tests using the
With the above concerns addressed, changing the master password plays nice with xud without needing any extra restarts. |
raladev
reviewed
Nov 29, 2020
There was a problem hiding this comment.
checked, works fine
With the above concerns addressed, changing the master password plays nice with xud without needing any extra restarts.
does it mean that we need to remove Passwords for lnd wallets will be changed the next time xud is restarted and unlocked. row? @sangaman
|
Should we display a different error message when xud and lnd are running in |
|
Good catch - yes we should |
ghost
previously approved these changes
Nov 30, 2020
|
LGTM, just minor nit: #2007 (comment) |
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
force-pushed
the
change-pass
branch
from
d8de798
to
36de8c6
Compare
|
I added a better error message when xud is in |
kilrau
approved these changes
Nov 30, 2020
raladev
approved these changes
Nov 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Note that this also 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.
The xucli command is
changepass.