Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2273723002:
Migrate Libsecret for OSCrypt to a new schema (Closed)
|
Created:
4 years, 4 months ago by cfroussios Modified:
4 years, 3 months ago Reviewers:
Lei Zhang CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMigrate Libsecret for OSCrypt to a new schema
Schemas for gnome-keyring and libsecret are not 100% interchangeable. To
support gnome-keyring, we need to use equivalent schemas in both
libraries. This will allow a user to upgrade their machine from gnome-keyring
to libsecret, without chrome losing access to OSCrypt's encryption key.
We correct the unfortunate initial choice for a schema by copying entries
to the new schema.
As a bonus, this will correct the mislabeling of some entries (see crbug/640603)
BUG=639298
Committed: https://crrev.com/02da78d1d5cb36b3e40afd72e40c0c0cc6883c41
Cr-Commit-Position: refs/heads/master@{#415254}
Patch Set 1 #Patch Set 2 : Clear deletes passwords #Patch Set 3 : Migrate only if password is not found #Patch Set 4 : nit #Patch Set 5 : search by attrs #Patch Set 6 : fixed generated password's attributes #
Total comments: 12
Patch Set 7 : feedback #Patch Set 8 : Branded application attribute #
Total comments: 2
Patch Set 9 : feedback #
Messages
Total messages: 48 (39 generated)
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. We need to use equivalent schemas in both libraries. This will allow a user to upgrade from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. BUG=639298 ========== to ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. We need to use equivalent schemas in both libraries. This will allow a user to upgrade from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ==========
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cfroussios@chromium.org changed reviewers: + thestig@chromium.org
Description was changed from ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. We need to use equivalent schemas in both libraries. This will allow a user to upgrade from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ========== to ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. To support gnome-keyring, we need to use equivalent schemas in both libraries. This will allow a user to upgrade their machine from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ==========
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Comments are all nits. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:14: const SecretSchema kDeprecatedSchema = { Let's call this kKeystoreSchemaV1. (and add a comment to say deprecated in M55) Name the new schema kKeystoreSchemaV2, because if we ever deprecate V2, we don't want to have to name a schema kTheOtherDeprecatedSchema. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:98: // Even if deletion failed, we have to use the password that we created. Move this to between lines 95-96. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.h (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:29: // TODO(crbug/639298) We begun storing passwords with a problematic schema. Can you put crbug.com so one can more easily paste it into the omnibox? https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:29: // TODO(crbug/639298) We begun storing passwords with a problematic schema. How about something like: Older Chromium releases stored passwords with a problematic schema. Detect password entries with the old schema and migrate them to the new schema. Returns the migrated password or an empty string if none were migrated. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:19: const SecretSchema kDeprecatedSchema = { Ditto re: naming. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:240: EXPECT_EQ(password, "swallow"); expected value on the left
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:14: const SecretSchema kDeprecatedSchema = { On 2016/08/26 21:33:56, Lei Zhang wrote: > Let's call this kKeystoreSchemaV1. (and add a comment to say deprecated in M55) > Name the new schema kKeystoreSchemaV2, because if we ever deprecate V2, we don't > want to have to name a schema kTheOtherDeprecatedSchema. Done. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:98: // Even if deletion failed, we have to use the password that we created. On 2016/08/26 21:33:56, Lei Zhang wrote: > Move this to between lines 95-96. Done. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.h (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:29: // TODO(crbug/639298) We begun storing passwords with a problematic schema. On 2016/08/26 21:33:56, Lei Zhang wrote: > Can you put http://crbug.com so one can more easily paste it into the omnibox? Done. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:29: // TODO(crbug/639298) We begun storing passwords with a problematic schema. On 2016/08/26 21:33:56, Lei Zhang wrote: > How about something like: > > Older Chromium releases stored passwords with a problematic schema. Detect > password entries with the old schema and migrate them to the new schema. Returns > the migrated password or an empty string if none were migrated. Done. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:19: const SecretSchema kDeprecatedSchema = { On 2016/08/26 21:33:57, Lei Zhang wrote: > Ditto re: naming. Done. https://codereview.chromium.org/2273723002/diff/100001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:240: EXPECT_EQ(password, "swallow"); On 2016/08/26 21:33:57, Lei Zhang wrote: > expected value on the left Done.
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I made one additional change. The attribute now is "chrome" or "chromium" depending on build. This means that each version has its own key. Otherwise, the two versions would use the same key and the first one gets to name it. This also makes the behaviour consistent with what happens with KWallet.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm, with one more nit: https://codereview.chromium.org/2273723002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2273723002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:15: const char* kApplicationName = "chrome"; const char kApplicationName[]
The CQ bit was checked by cfroussios@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2273723002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/2273723002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:15: const char* kApplicationName = "chrome"; On 2016/08/29 17:42:35, Lei Zhang wrote: > const char kApplicationName[] Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cfroussios@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2273723002/#ps160001 (title: "feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. To support gnome-keyring, we need to use equivalent schemas in both libraries. This will allow a user to upgrade their machine from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ========== to ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. To support gnome-keyring, we need to use equivalent schemas in both libraries. This will allow a user to upgrade their machine from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. To support gnome-keyring, we need to use equivalent schemas in both libraries. This will allow a user to upgrade their machine from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 ========== to ========== Migrate Libsecret for OSCrypt to a new schema Schemas for gnome-keyring and libsecret are not 100% interchangeable. To support gnome-keyring, we need to use equivalent schemas in both libraries. This will allow a user to upgrade their machine from gnome-keyring to libsecret, without chrome losing access to OSCrypt's encryption key. We correct the unfortunate initial choice for a schema by copying entries to the new schema. As a bonus, this will correct the mislabeling of some entries (see crbug/640603) BUG=639298 Committed: https://crrev.com/02da78d1d5cb36b3e40afd72e40c0c0cc6883c41 Cr-Commit-Position: refs/heads/master@{#415254} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/02da78d1d5cb36b3e40afd72e40c0c0cc6883c41 Cr-Commit-Position: refs/heads/master@{#415254} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
