Add device password change functionality #1692
Conversation
cedwies
force-pushed
the
cedwies/change-password
branch
4 times, most recently
from
1a3e636 to
e119797
Compare
cedwies
requested review from
NickeZ and
benma
and removed request for
NickeZ
benma
left a comment
benma
left a comment
There was a problem hiding this comment.
Very nice job! Haven't tested it yet, just left some initial comments.
| Returns True if the user changes password successfully by | ||
| unlocking with old password and entering and confirming new password. | ||
| Returns False otherwise. |
There was a problem hiding this comment.
Maybe better to simply return nothing on success and let the exception propagate on failure.
As right now, it returns False if they enter the wrong password, and an exception if cancelled or if there is another kind of error. Seems simpler to just do the same in all cases.
There was a problem hiding this comment.
I suggest a similar approach compared to show_mnemonic:
def change_password(self) -> bool:
"""
Returns True if the user changes password successfully by
unlocking with old password and entering and confirming new password.
Raises a Bitbox02Exception on failure.
"""
# pylint: disable=no-member
request = hww.Request()
request.change_password.CopyFrom(bitbox02_system.ChangePasswordRequest())
self._msg_query(request, expected_response="success")| matches!(state, State::Uninitialized | State::Seeded) | ||
| } | ||
| Request::CreateBackup(_) | Request::ShowMnemonic(_) => { | ||
| Request::CreateBackup(_) | Request::ShowMnemonic(_) | Request::ChangePassword(_) => { |
There was a problem hiding this comment.
It should probably be in the arm below where it's only InitializedAndUnlocked. Here you also allow the call if it is not initialized (but seeded).
There was a problem hiding this comment.
As you mentioned already:
In this case, is_seeded() is enough, because then there is an encrypted seed whose password we can change.
So we should be able to call ChangePassword when seeded, no?
There was a problem hiding this comment.
Technically I guess you are right, but practically we will expose the functionality in the BitBoxApp under 'Manage device' after it's been initialized. I don't think we'll add a change password function right after setting the password, before creating a backup, in the setup wizard. If one wants a new password then, just reconnecting the device will do the trick too, as then the setup starts from scratch.
Long story short, imho you can enforce that it is initialized and unlocked here.
There was a problem hiding this comment.
As for
In this case, is_seeded() is enough, be
That's true, in that case you could leave the check that it is initialized. It does not matter that much, as long as it's seeded and unlocked the function works (so is_seeded() is the minimum check). To me, it makes a bit more sense to keep the minimum check (seeded) in the keystore module, while doing the more strict check in the API here to decide when the API call can be made. No strong opinion though.
| // Must be initialized | ||
| if !bitbox02::memory::is_initialized() { | ||
| return Err(Error::Generic); | ||
| } |
There was a problem hiding this comment.
Not needed if in api.rs you move the check in can_call so that it can only be called if initialized and unlocked.
|
|
||
| // Re-encrypt seed with new password | ||
| if keystore::re_encrypt_seed(hal, &seed, &new_password).is_err() { | ||
| hal.ui().status("Could not change\npassword", false).await; |
There was a problem hiding this comment.
Elsewhere we use
hal.ui().status(&format!("Error\n{:?}", err), false).await;
Maybe that's better, it's not expected to ever be shown, but if it is, there will be some more info.
| // 1. The secure chip's internal keys are regenerated with the new password | ||
| // 2. encrypt_and_store_seed_internal calls lock() which clears RAM including BIP39 seed | ||
| // 3. We want to avoid forcing the user to re-enter their BIP39 passphrase | ||
| let bip39_seed = copy_bip39_seed().map_err(|_| Error::CannotUnlockBIP39)?; |
There was a problem hiding this comment.
Wrong error, we are not in the process of unlocking bip39.
There was a problem hiding this comment.
Thanks, I'd suggest Error::Memory (?)
There was a problem hiding this comment.
In other funcs we just use copy_bip39_seed()?, which propagates as Generic error.
| RETAINED_BIP39_SEED.write(Some(RetainedEncryptedBuffer::from_buffer( | ||
| hal.random(), | ||
| bip39_seed.as_slice(), | ||
| "keystore_retained_bip39_seed_access", | ||
| )?)); |
There was a problem hiding this comment.
Best to make a retain_bip39_seed function similar to retain_seed
| } | ||
|
|
||
| /// Re-encrypts the seed with a (new) password | ||
| pub fn re_encrypt_seed( |
There was a problem hiding this comment.
Would be nice to add some unit tests in this file too for this new function.
| if bitbox02::memory::is_initialized() { | ||
| return Err(Error::Memory); | ||
| } | ||
| // We are in setup phase, so we clear memory first to guarantee clean RAM |
There was a problem hiding this comment.
Thanks, was an outdated comment. In an earlier approach I only called lock() in encrypt_and_store_seed(...). But I later moved it it into encrypt_and_store_seed_internal(...) so RAM is cleared both in encrypt_and_store_seed(...) and in re_encrypt_seed(...)
| if !bitbox02::memory::is_initialized() { | ||
| return Err(Error::Unseeded); | ||
| } |
There was a problem hiding this comment.
Fyi, seeded means an encrypted seed is stored, and initialized means it is seeded and a backup has been created.
In this case, is_seeded() is enough, because then there is an encrypted seed whose password we can change. And the Unseeded error will be correct too then 😄
Implements a new API endpoint to allow users to change their device
password without re-entering the BIP39 passphrase.
Changes:
- Add ChangePasswordRequest message to proto definitions
- Implement change_password module in Rust API
- Refactor keystore to support password re-encryption:
- Add re_encrypt_seed() function to change password while preserving
BIP39 seed state
- Extract encrypt_and_store_seed_internal() helper to share logic
between initial setup and password change
- Add Python bindings for change_password workflow
- Add comprehensive tests including password verification
- Regenerated protobuf files
The password change workflow:
1. Unlock device with current password
2. Prompt for new password (entered twice for confirmation)
3. Re-encrypt seed with new password while preserving BIP39 seed
This prevents users from having to re-enter their BIP39 passphrase
after changing their password.
cedwies
force-pushed
the
cedwies/change-password
branch
3 times, most recently
from
4fa4c53 to
256a514
Compare
- Modified change_password in bitbox02.py to raise BitBox02Exception on failure - The api call is now only allowed if the device is initialized and unlocked - Extra check in removed in process(...) in change_password.rs - Changed ui status to show more info when re_encrypt_seed fails - corrected error type in re_encrypt_seed(...) in keystore.rs - Added retain_bip39_seed(...) to re-use code - Added unit tests for re_encrypt_seed(...) and retain_bip39_seed(...) - Removed outdated comment - Changed check in re_encrypt_seed(...) to is_seeded() instead of is_initialzed() - Adapted change_password.rs due to merge conflict - Bumped firmware version to v9.25.0
cedwies
force-pushed
the
cedwies/change-password
branch
from
256a514 to
8e48d8d
Compare
2 participants
Implements a new API endpoint to allow users to change their device password without re-entering the BIP39 passphrase.
Changes:
The password change workflow:
This prevents users from having to re-enter their BIP39 passphrase after changing their password.