cryptohome: unify keysets workflow |
||||
Issue descriptionCurrently, different parts of cryptohomed use different methods for working with keysets. 1) Mount in some cases directly uses Mount::LoadVaultKeysetForUser(), Crypto::DecryptVaultKeyset, Mount::StoreVaultKeysetForUser(). Mount::Load/StoreVaultKeysetForUser methods directly operate with files on disk. In other cases, it calls HomeDirs to deal with keysets. 2) HomeDirs works through VaultKeyset: VaultKeyset::Load(), VaultKeyset::Decrypt(), VaultKeyset::Save(), ... These VaultKeyset methods call the methods from the 1st group above internally. This leads to code duplication, which becomes worse when special processing needs to be done on the keyset (e.g. save auth_locked=true flag in keyset file in case of too many attempts for LECredential). It also makes the code prone to errors: it is possible to update only one set of methods when making a change. Plus, it also unnecessary spreads knowledge about keyset internals over several classes - Mount, HomeDirs, VaultKeyset, Crypto. Unify the approach (likely make Mount always call HomeDirs to perform operations on Keysets).
,
Jun 29 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/dfd14e0a42eb1c002dc764be6bb1fd74679d6b69 commit dfd14e0a42eb1c002dc764be6bb1fd74679d6b69 Author: Maksim Ivanov <emaxx@chromium.org> Date: Fri Jun 29 21:16:34 2018 cryptohome: Remove migrate_if_needed from DecryptVaultKeyset Remove the |migrate_if_needed| argument of Mount::DecryptVaultKeyset(), and do the migration always whenever it's needed. This argument was already never set to |false| by the production code, so this is just a cleanup with no behavior changes. BUG=chromium:832395 TEST=existing tests Change-Id: I3ec51821df8e371153d68adc3dd2230cbc0b1279 Reviewed-on: https://chromium-review.googlesource.com/1116918 Commit-Ready: Maksim Ivanov <emaxx@chromium.org> Tested-by: Maksim Ivanov <emaxx@chromium.org> Reviewed-by: Maksim Ivanov <emaxx@chromium.org> [modify] https://crrev.com/dfd14e0a42eb1c002dc764be6bb1fd74679d6b69/cryptohome/mount.h [modify] https://crrev.com/dfd14e0a42eb1c002dc764be6bb1fd74679d6b69/cryptohome/mount.cc [modify] https://crrev.com/dfd14e0a42eb1c002dc764be6bb1fd74679d6b69/cryptohome/mount_unittest.cc
,
Jul 19
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/e821e7059249ffa29d6458a5ca7e48d9931816b9 commit e821e7059249ffa29d6458a5ca7e48d9931816b9 Author: Maksim Ivanov <emaxx@chromium.org> Date: Thu Jul 19 13:12:29 2018 cryptohome: Refactor error handling in Mount::DecryptVaultKeyset Preparatory CL for removal of duplicated logic of vault keyset loading from this method. The goal is to separate out the error handling logic (i.e., which kind of MountError should be used when the keyset loading or decryption fails). After doing that, the next step would be the removal of the duplicate low-level loading mechanism (in favor of the already existing means provided by VaultKeyset+HomeDirs). No behavior changes are expected. Only some error messages for error scenarios (when the loading/decryption fails) became changed and/or more granular. BUG=chromium:832395 TEST=existing tests, manual test: log in under a new account, wait until the TPM is initialized, log out, log in again, log out, try to log in again with a wrong password - and verify that the user cryptohome was retained Change-Id: I6b77fb1a3e6d7ba562c3ab8a457cbed28608f683 Reviewed-on: https://chromium-review.googlesource.com/1118553 Commit-Ready: Maksim Ivanov <emaxx@chromium.org> Tested-by: Maksim Ivanov <emaxx@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> [modify] https://crrev.com/e821e7059249ffa29d6458a5ca7e48d9931816b9/cryptohome/mount.cc
,
Jul 31
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/b39ae7f9c6e18e6daf96d17baec810fe956c0670 commit b39ae7f9c6e18e6daf96d17baec810fe956c0670 Author: Maksim Ivanov <emaxx@chromium.org> Date: Tue Jul 31 23:37:23 2018 cryptohome: Use HomeDirs for loading VK in Mount Remove the duplicated logic from Mount, and use the already existing functionality provided by HomeDirs instead. Also fix comments in unit tests for ephemeral mount, in which some cleanup was actually happening despite the comments were claiming the opposite. BUG=chromium:832395, chromium:853012 TEST=existing tests Change-Id: I91845739f1bc80f2f1bcd276159221a8256a9f87 Reviewed-on: https://chromium-review.googlesource.com/1098255 Commit-Ready: Maksim Ivanov <emaxx@chromium.org> Tested-by: Maksim Ivanov <emaxx@chromium.org> Reviewed-by: Ryo Hashimoto <hashimoto@chromium.org> [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mock_homedirs.h [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs.h [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset_unittest.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset.h [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mount.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto_unittest.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/vault_keyset.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mock_vault_keyset.h [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto.h [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/crypto.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs_unittest.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/homedirs.cc [modify] https://crrev.com/b39ae7f9c6e18e6daf96d17baec810fe956c0670/cryptohome/mount_unittest.cc
,
Oct 12
,
Dec 10
Looks like the WIP CL landed - is this fixed Maksim?
,
Dec 10
Looks like the WIP CL landed - is this fixed Maksim?
,
Dec 11
Making this as Available since my CLs in comments ##2..4 removed a few, but not all places where there's an ad-hoc keyset handling code that bypasses HomeDirs.
,
Dec 12
|
||||
►
Sign in to add a comment |
||||
Comment 1 by emaxx@chromium.org
, Jun 12 2018Status: Started (was: Untriaged)