Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2118443002:
Forward password-store switch to OSCrypt component (Closed)
|
Created:
4 years, 5 months ago by cfroussios Modified:
4 years, 5 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. |
DescriptionForward password-store switch to OSCrypt component
Password manager uses a switch to allow the user to override the
auto-detection of the appropriate password store. OSCrypt should
respect this switch as well.
The switch value is read and passed to OSCrypt at a very early
point in Chrome's start, before any of OSCrypt's dependents
use it.
I also reworked OSCrypt's build to make it simpler for chrome to
deduce whether the linux implementation of OSCrypt will be used.
- Previously, os_crypt_linux was used only if we also decided to
link at least one linux backend. Otherwise we used os_crypt_posix.
+ Now, we always use the linux implementation for linux. If no
KeyStorage is linked, the linux implementation defaults to the
same behavior as for posix.
BUG=602624
Committed: https://crrev.com/92699e345c15243c4023a8739101333ed2658e37
Cr-Commit-Position: refs/heads/master@{#405114}
Patch Set 1 #Patch Set 2 : flag #Patch Set 3 : Removed unnecessary condition #Patch Set 4 : fixed build? #Patch Set 5 : try this #Patch Set 6 : Try this #Patch Set 7 : Try this #Patch Set 8 : Try this #
Total comments: 6
Patch Set 9 : nogncheck #Patch Set 10 : include KeyStorageLibsecret conditionally #Patch Set 11 : also gyp #
Total comments: 6
Patch Set 12 : feedback #Patch Set 13 : build cleanup #Patch Set 14 : Verify that target does not change after lazy initialization #
Total comments: 6
Patch Set 15 : feedback #Patch Set 16 : Init in PreProfileInit() #Patch Set 17 : Fixed order of calling superclass method #Messages
Total messages: 35 (21 generated)
Description was changed from ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should this switch as well. BUG=602624 ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should this switch as well. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ==========
Description was changed from ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should this switch as well. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ==========
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Description was changed from ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ==========
cfroussios@chromium.org changed reviewers: + grt@chromium.org, thestig@chromium.org
grt@chromium.org: Please review changes in chrome/app thestig@chromium.org: Please review changes in components/os_crypt
Can we resolve https://codereview.chromium.org/2117323002/ first? https://codereview.chromium.org/2118443002/diff/170001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2118443002/diff/170001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:127: // Linux allows to chose between different password stores. nit: choose https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:36: static const char* s_store_; Can this be a std::string?
The other issue is moving rather slowly. The solution I proposed there is also implemented here, so there is a good chance that this CL fixes both issues. https://codereview.chromium.org/2118443002/diff/170001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2118443002/diff/170001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:127: // Linux allows to chose between different password stores. On 2016/07/06 18:57:28, Lei Zhang wrote: > nit: choose Done. https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:36: static const char* s_store_; On 2016/07/06 18:57:28, Lei Zhang wrote: > Can this be a std::string? The linter does not permit static string variables.
https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:36: static const char* s_store_; On 2016/07/11 08:32:00, cfroussios wrote: > On 2016/07/06 18:57:28, Lei Zhang wrote: > > Can this be a std::string? > > The linter does not permit static string variables. Oh right. Can it be a base::LazyInstance<std::string> in key_storage_linux.cc then? https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:128: #include "components/os_crypt/os_crypt.h" // nogncheck Is the "nogncheck" needed? What are we ignoring? https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:659: OSCrypt::SetStore(password_store); This is very early in the start up process indeed, though it runs for all process types, e.g. the GPU process. That seems unnecessary. Can we do this in chrome_browser_main.cc? Even PostProfileInit() may be early enough, but I'm not 100% sure. https://codereview.chromium.org/2118443002/diff/230001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2118443002/diff/230001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:21: delete s_store_; Do we ever call SetStore() multiple times?
I'm removing grt@ from the reviewers, since we're not editing chrome/apps anymore. https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/170001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:36: static const char* s_store_; On 2016/07/12 02:02:53, Lei Zhang wrote: > On 2016/07/11 08:32:00, cfroussios wrote: > > On 2016/07/06 18:57:28, Lei Zhang wrote: > > > Can this be a std::string? > > > > The linter does not permit static string variables. > > Oh right. Can it be a base::LazyInstance<std::string> in key_storage_linux.cc > then? Done. https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... File chrome/app/chrome_main_delegate.cc (right): https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:128: #include "components/os_crypt/os_crypt.h" // nogncheck On 2016/07/12 02:02:53, Lei Zhang wrote: > Is the "nogncheck" needed? What are we ignoring? This was news to me (crbug/625122), but gn completely ignores preprocessor conditions. Since os_crypt is added as a dependency only on linux, 'gn check' fails on every other platform, since it sees this #include as illegal. This is not needed anymore. https://codereview.chromium.org/2118443002/diff/230001/chrome/app/chrome_main... chrome/app/chrome_main_delegate.cc:659: OSCrypt::SetStore(password_store); On 2016/07/12 02:02:53, Lei Zhang wrote: > This is very early in the start up process indeed, though it runs for all > process types, e.g. the GPU process. That seems unnecessary. Can we do this in > chrome_browser_main.cc? Even PostProfileInit() may be early enough, but I'm not > 100% sure. From what I can tell, PostProfileInit() is early enough for the way that things are now. I added a DCHECK to verify that this won't change. I don't think it is possible to make a proper test about this. https://codereview.chromium.org/2118443002/diff/230001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2118443002/diff/230001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:21: delete s_store_; On 2016/07/12 02:02:53, Lei Zhang wrote: > Do we ever call SetStore() multiple times? Not currently, but I'm still considering the extend to which KeyStorageLinux::CreateService() can be reasonably tested. Anyway, we get this automatically now.
cfroussios@chromium.org changed reviewers: - grt@chromium.org
lgtm, the rest is all nits. https://codereview.chromium.org/2118443002/diff/290001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/2118443002/diff/290001/components/components_... components/components_tests.gyp:1599: 'USE_KWALLET', nit: alphabetical order https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:15: #endif // defined(USE_LIBSECRET) Personally, I'm kind of meh about the match comment when it's only a few lines. Same inchrome/browser/chrome_browser_main_linux.cc. When it gets to 20+ lines and there's risk of not being able to see the matching #if, or if there are nested #ifs, then the comments are very helpful. https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:11: #include "base/lazy_instance.h" Not used here -> goes in the .cc file.
https://codereview.chromium.org/2118443002/diff/290001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/2118443002/diff/290001/components/components_... components/components_tests.gyp:1599: 'USE_KWALLET', On 2016/07/12 19:12:24, Lei Zhang wrote: > nit: alphabetical order Done. https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... components/os_crypt/key_storage_linux.cc:15: #endif // defined(USE_LIBSECRET) On 2016/07/12 19:12:24, Lei Zhang wrote: > Personally, I'm kind of meh about the match comment when it's only a few lines. > Same inchrome/browser/chrome_browser_main_linux.cc. When it gets to 20+ lines > and there's risk of not being able to see the matching #if, or if there are > nested #ifs, then the comments are very helpful. Done. https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/2118443002/diff/290001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:11: #include "base/lazy_instance.h" On 2016/07/12 19:12:24, Lei Zhang wrote: > Not used here -> goes in the .cc file. Done.
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/2118443002/#ps310001 (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 ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ==========
Message was sent while issue was closed.
Committed patchset #15 (id:310001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 Committed: https://crrev.com/92699e345c15243c4023a8739101333ed2658e37 Cr-Commit-Position: refs/heads/master@{#405114} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/92699e345c15243c4023a8739101333ed2658e37 Cr-Commit-Position: refs/heads/master@{#405114}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:310001) has been created in https://codereview.chromium.org/2152493002/ by cfroussios@chromium.org. The reason for reverting is: mahmadi@ reported reaching the DCHECK in components/os_crypt/os_crypt_linux.cc.
Message was sent while issue was closed.
Patchset #16 (id:330001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 Committed: https://crrev.com/92699e345c15243c4023a8739101333ed2658e37 Cr-Commit-Position: refs/heads/master@{#405114} ========== to ========== Forward password-store switch to OSCrypt component Password manager uses a switch to allow the user to override the auto-detection of the appropriate password store. OSCrypt should respect this switch as well. The switch value is read and passed to OSCrypt at a very early point in Chrome's start, before any of OSCrypt's dependents use it. I also reworked OSCrypt's build to make it simpler for chrome to deduce whether the linux implementation of OSCrypt will be used. - Previously, os_crypt_linux was used only if we also decided to link at least one linux backend. Otherwise we used os_crypt_posix. + Now, we always use the linux implementation for linux. If no KeyStorage is linked, the linux implementation defaults to the same behavior as for posix. BUG=602624 Committed: https://crrev.com/92699e345c15243c4023a8739101333ed2658e37 Cr-Commit-Position: refs/heads/master@{#405114} ==========
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.
I did a closer inspection, and the creation of the profile does indeed use decryption. I have moved the initialization to happen before the creation of the profile.
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: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Depends on Patchset: Issue 2118443002 Patch 310001 Uh, this is getting weird. Patch set 17 depends on patch set 15? This is why I like to use a new CL for a fresh start. (Though not everyone agrees) |
