Database reads on key-related fields should be atomic #5177
Comments
This was referenced Feb 13, 2023
stringhandler
pushed a commit
that referenced
this issue
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
It was pointed out that database reads on multiple fields have no inherent guarantee of consistency, and may return unexpected results due to write operations. However, SQLite apparently does guarantee read consistency within a read-only transaction.
While the use case in question will fail safe in the event this edge case happens, it's something of a footgun to rely on failed key derivation to catch this.
A more comprehensive and general approach to read transactions may be warranted, but for now, the read operations used for key-related database fields (version identifier, secondary key salt, and encrypted main key) should be done atomically. Even if these reads produce outdated but consistent data due to a write transaction, they will still produce a valid database cipher with the correct main key.
This is also a good opportunity to refactor the atomic write operations for these same fields to reduce code duplication and make intent more clear.
The text was updated successfully, but these errors were encountered: