feat: wallet password change #5175

AaronFeickert · 2023-02-09T22:55:33Z

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:

Attempt to use the existing passphrase to authenticate and decrypt the encrypted database main key. If this fails, the passphrase is incorrect, so we abort.
Get the latest encryption version parameters from the hardcoded list.
Generate a fresh secondary key salt. Use it and the new passphrase to derive a new secondary key, using the fetched encryption version parameters.
Encrypt the database main key using the new secondary key.
Store the new version identifier, secondary key salt, and encrypted main key in the database, overwriting the previous values.

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.

base_layer/wallet/src/storage/sqlite_db/wallet.rs

hansieodendaal

Nice, I tested the password change functionality from the command line, and it works as expected.

I have some comments regarding the atomicity below.

base_layer/wallet/src/storage/sqlite_db/wallet.rs

SWvheerden

utACK

hansieodendaal

ACK

stringhandler · 2023-02-13T07:26:32Z

applications/tari_console_wallet/src/init/mod.rs


-    let passphrase = prompt_password("New wallet password: ")?;
+    let new = prompt_password("New wallet password: ")?;


nit: while not a reserved word in rust, it does seem strange to see new as a variable

stringhandler · 2023-02-13T07:32:47Z

base_layer/wallet/src/storage/sqlite_db/wallet.rs

+    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)?;


Should technically also be in a transaction since it could change half way through reading

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.

Will address in this issue.

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.

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.

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.

SWvheerden reviewed Feb 10, 2023

View reviewed changes

base_layer/wallet/src/storage/sqlite_db/wallet.rs Outdated Show resolved Hide resolved

AaronFeickert force-pushed the change-passphrase branch 2 times, most recently from af6509e to c34af83 Compare February 10, 2023 18:10

AaronFeickert marked this pull request as ready for review February 10, 2023 18:10

Wallet password change

57ce3c0

AaronFeickert force-pushed the change-passphrase branch from c34af83 to 57ce3c0 Compare February 10, 2023 18:18

Update CLI help

f45a443

hansieodendaal reviewed Feb 11, 2023

View reviewed changes

base_layer/wallet/src/storage/sqlite_db/wallet.rs Outdated Show resolved Hide resolved

base_layer/wallet/src/storage/sqlite_db/wallet.rs Show resolved Hide resolved

Make initial encryption setup atomic

cab86e4

AaronFeickert force-pushed the change-passphrase branch from 5fd8c25 to cab86e4 Compare February 11, 2023 19:36

AaronFeickert requested review from SWvheerden and hansieodendaal and removed request for SWvheerden and hansieodendaal February 11, 2023 20:08

SWvheerden approved these changes Feb 13, 2023

View reviewed changes

hansieodendaal approved these changes Feb 13, 2023

View reviewed changes

stringhandler approved these changes Feb 13, 2023

View reviewed changes

stringhandler added this pull request to the merge queue Feb 13, 2023

stringhandler removed this pull request from the merge queue due to a manual request Feb 13, 2023

stringhandler added this pull request to the merge queue Feb 13, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 13, 2023

stringhandler merged commit 7f13fa5 into tari-project:development Feb 13, 2023

AaronFeickert mentioned this pull request Feb 13, 2023

Database reads on key-related fields should be atomic #5177

Closed

AaronFeickert deleted the change-passphrase branch February 13, 2023 15:46

AaronFeickert mentioned this pull request Feb 13, 2023

feat: refactor key-related field operations to be atomic #5178

Merged

This was referenced Mar 28, 2023

Wallet bad password just continues on #5127

Closed

feat: improved passphrase flow #5279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: wallet password change #5175

feat: wallet password change #5175

AaronFeickert commented Feb 9, 2023 •

edited

Loading

hansieodendaal left a comment

SWvheerden left a comment

hansieodendaal left a comment

stringhandler Feb 13, 2023

stringhandler Feb 13, 2023

AaronFeickert Feb 13, 2023

AaronFeickert Feb 13, 2023


		let passphrase = prompt_password("New wallet password: ")?;
		let new = prompt_password("New wallet password: ")?;

feat: wallet password change #5175

feat: wallet password change #5175

Conversation

AaronFeickert commented Feb 9, 2023 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

hansieodendaal left a comment

Choose a reason for hiding this comment

SWvheerden left a comment

Choose a reason for hiding this comment

hansieodendaal left a comment

Choose a reason for hiding this comment

stringhandler Feb 13, 2023

Choose a reason for hiding this comment

stringhandler Feb 13, 2023

Choose a reason for hiding this comment

AaronFeickert Feb 13, 2023

Choose a reason for hiding this comment

AaronFeickert Feb 13, 2023

Choose a reason for hiding this comment

AaronFeickert commented Feb 9, 2023 •

edited

Loading