feat: wallet password change #5175
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
SWvheerden
reviewed
Feb 10, 2023
AaronFeickert
force-pushed
the
change-passphrase
branch
2 times, most recently
from
af6509e to
c34af83
Compare
AaronFeickert
force-pushed
the
change-passphrase
branch
from
c34af83 to
57ce3c0
Compare
AaronFeickert
force-pushed
the
change-passphrase
branch
from
5fd8c25 to
cab86e4
Compare
AaronFeickert
requested review from
SWvheerden and
hansieodendaal
and removed request for
SWvheerden and
hansieodendaal
stringhandler
approved these changes
Feb 13, 2023
|
|
||
| let passphrase = prompt_password("New wallet password: ")?; | ||
| let new = prompt_password("New wallet password: ")?; |
There was a problem hiding this comment.
nit: while not a reserved word in rust, it does seem strange to see new as a variable
| let conn = database_connection.get_pooled_connection()?; | ||
|
|
||
| // Fetch the database fields used for encryption, if they exist | ||
| let secondary_key_version = WalletSettingSql::get(&DbKey::SecondaryKeyVersion, &conn)?; |
There was a problem hiding this comment.
Should technically also be in a transaction since it could change half way through reading
There was a problem hiding this comment.
Would using a transaction catch this edge case? Even if not, I don't think it's a practical problem here. In the event that the fields were to be only partially updated when the reads happen, the subsequent key derivation will fail and return an error. The user can then simply try again to load the wallet.
There was a problem hiding this comment.
Will address in this issue.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
stringhandler
pushed a commit
that referenced
this pull request
Feb 14, 2023
Description --- Refactors key-related database field operations to be atomic. Closes [issue 5177](#5177). Motivation and Context --- Key-related database fields always travel together. We need a consistent set of secondary key version identifier, secondary key salt, and encrypted main key in order to set up the `XChaCha20-Poly1305` cipher used for database encryption operations and passphrase changes. While a [recent PR](#5175) ensures that write operations for these fields are done atomically via a write transaction, there is no corresponding read transaction. It's therefore possible that those fields are inconsistent. While this should only result in an error and require the user to load their wallet again, it seemed like a smart idea to ensure that reads are consistent for any future use cases. This PR refactors the handling of those fields to reduce redundancy and ensure atomicity for reads and writes. It introduces a new `DatabaseKeyFields` struct that handles reads and writes, and additionally takes care of encoding and decoding of the underlying data. It also makes the handling of the three fields more consistent. Previously, individual reads and writes required the use of a complex `match` to handle different states. This functionality has been mostly moved into `DatabaseKeyFields` to make these states more apparent. How Has This Been Tested? --- Existing unit tests pass. Manually tested the following operations: - setting up a new wallet and successfully loading it with the correct passphrase - setting up a new wallet and unsuccessfully loading it with an incorrect passphrase - setting up a new wallet and unsuccessfully loading it due to a simulated read transaction failure - failing to set up a new wallet due to a simulated write transaction failure - failing to set up a new wallet due to a simulated read transaction failure - a successful passphrase change via CLI - an unsuccessful passphrase change via CLI due to an incorrect existing passphrase - an unsuccessful passphrase change via CLI due to a mismatched new passphrase - an unsuccessful passphrase change via CLI due to a simulated write transaction failure - an unsuccessful passphrase change via CLI due to a simulated read transaction failure It does not seem possible to directly test read operation inconsistency caused by a simultaneous write operation.
This was referenced Mar 28, 2023
SWvheerden
pushed a commit
that referenced
this pull request
Mar 31, 2023
Description --- Improves the flow for setting or changing a passphrase. Closes [issue 5127](#5127). Motivation and Context --- When setting or [changing](#5175) a wallet passphrase, the console wallet provides [feedback](#5111) on the strength of the provided passphrase. In the case of a weak passphrase, it does not prompt the user to choose a better one. This PR implements a better flow for this process, as shown in [this flowchart](#5127 (comment)). How Has This Been Tested? --- Tested manually. What process can a PR reviewer use to test or verify this change? --- Testing needs to be done manually to assert that the process represented by the linked flowchart is implemented. Manual testing should cover the entire flow for these two operations: - Setting the passphrase for a new wallet - Changing the passphrase for an existing wallet Breaking Changes --- None.
agubarev
pushed a commit
to agubarev/tari
that referenced
this pull request
Mar 31, 2023
Description --- Improves the flow for setting or changing a passphrase. Closes [issue 5127](tari-project#5127). Motivation and Context --- When setting or [changing](tari-project#5175) a wallet passphrase, the console wallet provides [feedback](tari-project#5111) on the strength of the provided passphrase. In the case of a weak passphrase, it does not prompt the user to choose a better one. This PR implements a better flow for this process, as shown in [this flowchart](tari-project#5127 (comment)). How Has This Been Tested? --- Tested manually. What process can a PR reviewer use to test or verify this change? --- Testing needs to be done manually to assert that the process represented by the linked flowchart is implemented. Manual testing should cover the entire flow for these two operations: - Setting the passphrase for a new wallet - Changing the passphrase for an existing wallet Breaking Changes --- None.
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.
Description
Adds functionality to change wallet passphrase, updating to the latest encryption version parameters at the same time. Minor refactoring of database main and secondary key operations.
Removes unneeded functionality that tried to assert any cipher seed was encrypted (and associated tests).
Closes issue 5003.
Motivation and Context
Currently, wallet passphrases cannot be changed; there is a CLI flow, but it is non-functional.
This PR continues the work started in PR 5154, which refactors wallet database encryption. When the user wishes to change their passphrase, we do the following:
The update operations are done in a single transaction to mitigate the (unlikely) risk of database corruption. There isn't an included test for this, but you can simulate a failure by having the transaction closure return an error, after which the existing wallet passphrase should continue to work.
How Has This Been Tested?
Existing unit tests pass. A new unit test passes. Manually tested a successful passphrase change. Manually tested a failed passphrase change. Manually tested a simulated database transaction failure.