Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1973483002:
OSCrypt for POSIX uses libsecret to store a randomised encryption key. (Closed)
|
Created:
4 years, 7 months ago by cfroussios Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement OSCrypt for Linux.
OSCrypt for linux uses libsecret to store a randomised
encryption key.
Mentions of OSCrypt below refer to the linux version.
OSCrypt now supports a new version of encryption. If
libsecret is available, then a randomised password will be
generated, stored in libsecret and used for producing
encryption keys. The old ways of encrypting, i.e. using a
hardcoded password and using no encryption at all, are still
supported.
Data encrypted with the new version are marked with a version
prefix, "v11". That is an increment on the previous prefix,
"v10", which separated data that was encrypted with the
hardcoded password from data that were not encrypted.
To separate this new linux feature from the generic posix
implementation, a new implementation for OSCrypt was created
in os_crypt_linux.cc.
Since there is support on the way for more services in
addition to libsecret (gnome keyring, kwallet), interaction
with such services is abstracted with the KeyStorageLinux API.
The libsecret utilities that KeyStorageLinux uses
(i.e. LibsecretLoader) are also used by the PasswordManager.
Here we only actually need a subset of their features. Once
PasswordManager stops depending on them, they can be
simplified and/or hidden in the KeyStorageLibsecret
implementation.
BUG=602624
Committed: https://crrev.com/3b5a4e4f0f10e73b9ace1e61079b099a9e62b1d8
Cr-Commit-Position: refs/heads/master@{#396814}
Patch Set 1 #Patch Set 2 : Removed unused variable causing compilation errors #Patch Set 3 : Refactored CL #
Total comments: 72
Patch Set 4 : Fix for chromeos build #Patch Set 5 : Fixed dependency from PasswordManager #Patch Set 6 : Recommended refactoring #Patch Set 7 : Fixed memory leak #Patch Set 8 : Fixed lsan failure #
Total comments: 74
Patch Set 9 : Applied recommendations #Patch Set 10 : Fix for non-linux builds #Patch Set 11 : Nit #
Total comments: 16
Patch Set 12 : Recommendations #Patch Set 13 : Reduced mocking boilerplate #Patch Set 14 : Fix for non-linux #Patch Set 15 : More fix for non-linux #
Total comments: 17
Patch Set 16 : Recommendations #
Total comments: 45
Patch Set 17 : Recommendations #Patch Set 18 : Fixed asan #
Total comments: 6
Patch Set 19 : Synced with master (OSCryptMocker) #Patch Set 20 : #
Total comments: 4
Patch Set 21 : Recommendations #Messages
Total messages: 40 (17 generated)
Description was changed from ========== OSCrypt for POSIX uses libsecret to store a randomised encryption key. All following mentions of OSCrypt refer to the POSIX version. TODO(cfroussios) add description BUG=602624 ========== to ========== OSCrypt for POSIX uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the POSIX version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Storing the generated password is handled by the Libsecret class in libsecret_util_posix. Some utilities are are also exposed, because they are used by Password Manager. Once this dependency is removed, they can be hidden and simplified. Utilities for mocking Libsecret have been implemented in libsecret_util_posix_testing. MockLibsecretLoader is based on the mocker used in chrome/browser/password_manager/native_backend_libsecret_unittest. However, the implementation was greatly simplified: it can store and retrieve exactly one password. Mocking is achieved by replacing LibsecretLoader's static methods with methods that perform the mocked functionality. BUG=602624 ==========
Hi vabr. Can you give quick feedback on this before I pass it to the owner?
Description was changed from ========== OSCrypt for POSIX uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the POSIX version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Storing the generated password is handled by the Libsecret class in libsecret_util_posix. Some utilities are are also exposed, because they are used by Password Manager. Once this dependency is removed, they can be hidden and simplified. Utilities for mocking Libsecret have been implemented in libsecret_util_posix_testing. MockLibsecretLoader is based on the mocker used in chrome/browser/password_manager/native_backend_libsecret_unittest. However, the implementation was greatly simplified: it can store and retrieve exactly one password. Mocking is achieved by replacing LibsecretLoader's static methods with methods that perform the mocked functionality. BUG=602624 ========== to ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Since there is support on the way for more services (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (ie LibsecretLoader) are also used by the PasswordManager. Those utilities can be simplified once PasswordManager stops depending on them. BUG=602624 ==========
Description was changed from ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Since there is support on the way for more services (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (ie LibsecretLoader) are also used by the PasswordManager. Those utilities can be simplified once PasswordManager stops depending on them. BUG=602624 ========== to ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Since there is support on the way for more services (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (ie LibsecretLoader) are also used by the PasswordManager. Those utilities can be simplified once PasswordManager stops depending on them. BUG=602624 ==========
cfroussios@chromium.org changed reviewers: + vabr@chromium.org
Thanks, Christos. I'm really glad to see this progress! I left some comments, some of them perhaps a little demanding. I don't want to slow your progress to a halt by requesting improving of existing code, on the other hand we need to be careful about technical debt, and fixing the tests and infrastructure is something we as a team need to focus on immediately. I'm afraid I will only be available for another review after I'm back, the week after the next one. So don't block yourself waiting for me on this CL. Feel free to use my comments as starting hints and then finish the CL under the review of os_crypt OWNERS, dvadym@ or anybody else you want to get involved instead. Cheers, Vaclav https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:12: #ifdef OFFICIAL_BUILD I'm not sure we need to differentiate the name here. It is not particularly user-facing, and there is no problem arising from potential clashes between Chrome and Chromium running on the same computer (it can also be multiple instances of the official build in parallel, anyway). So to keep out unnecessary #ifdefs, I would just go with the "Chromium" version. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:19: std::unique_ptr<KeyStorage> KeyStorage::FindService() { nit: Here "Find" is a bit confusing. It sounds like there is a pool of already created objects, which are searched for the one to return. Instead this just creates a new one or gives up completely. My suggestion: The appropriate name would be Create(). https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:22: key_storage.reset(new KeyStorageLibsecret()); nit: You can merge lines 20 and 22. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:36: std::string AddRandomPasswordInLibsecret() { nit: Please put local (=not exported beyond this .cc file) functions into an anonymous namespace. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:60: return ""; "" -> std::string() https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:75: KeyStorageMock::KeyStorageMock(std::string in_key) : key(in_key) {} Either use std::move(in_key) or change the type of in_key to "const std::string&" to avoid unnecessary copies. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:78: if (key == "") if (key_.empty()) (empty() is more efficient than string comparison.) https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:12: nit: Normally, code in components is in a namespace, here it would be "namespace os_crypt". But the rest of os_crypt does not seem to use that namespace, so please confirm with the os_crypt OWNER whether to use that namespace or not. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:25: // Tries to load available services. Returns the first that succeeds or nit: Please be careful with calling stuff a "service". It mostly has the meaning of a KeyedService in Chromium [1], so let's try to avoid it for things which are not KeyedService. I think calling the object "key storage" or just "storage" is OK given the type's name. [1] components/keyed_service/core/keyed_service.h https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:33: class KeyStorageLibsecret : public KeyStorage { Please separate the implementations of KeyStorage to their own files, one per implementation. They will be tiny, but it will match the fact that the user of KeyStorage should not care about the implementation. It will also allow to separate the mock from the production code (see the "test_support" targets in files like components/password_manager/core/browser/BUILD.gn -- we can mark them with testonly=true and ensure that production is independent of test code). https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:35: std::string GetKey() override; nit: We usually prefix the block with overrides with the name of the base class they override, in this case: // KeyStorage This case is very simple and it is clear from where the overrides come. But the class might grow, and the style should be consistent anyway, so please add the comment. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:41: explicit KeyStorageMock(std::string in_key = ""); Please use 2 constructors instead of the default argument. In particular, the default argument is evaluated at each call, so having a 0-argument constructor will spare a little work. Also, that's how most of the code is written right now (default args have been banned in the past), so it's better for consistency and thus readability. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:41: explicit KeyStorageMock(std::string in_key = ""); Also, whenever you need to initialize an empty string, use std::string(), not the implicit constructor from const char* applied to "". The former is more efficient. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:43: std::string GetKey() override; // KeyStorage (See above why.) https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:46: void ResetTo(std::string); nit: Please name the argument. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:49: std::string key; nit: Trailing underscore: key_ https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... File components/os_crypt/libsecret_util_posix.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... components/os_crypt/libsecret_util_posix.cc:8: #include <utility> What is this used for? https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... components/os_crypt/libsecret_util_posix.cc:10: #include "base/base64.h" And what is this used for? https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt.h:13: #include "components/os_crypt/key_storage_linux.h" You should #include this file unconditionally if you only put it in build dependencies on Linux. You could enclose it in the same #ifdef as UseMockKeyStorage below, but instead I suggest separating these newly added parts in os_crypt_linux.h and then making os_crypt_linux.h include os_crypt.h and only referencing os_crypt_linux.h from os_crypt_linux.cc. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt.h:48: static KeyStorageMock* UseMockKeyStorage(bool use_mock); nit: Please describe what the method does. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. Looking at the code in this file, I see two main issues: (1) The use of static variables with non-trivial destructor. Ideally we should not use them at all. Alternatively, have a look at LazyInstance [1] for how to create objects as static variables. If you use that, it might make sense to restructure the code so that you end up with one LazyInstance grouping all of the static data. (2) The mix of test utilities and production code, in particular the mock key storage. Ideally, all testing support would be in a separate file, part of a separate build target marked as testonly=true in GN. The production code should have a single place where to inject an alternative KeyStorage instance, which would be done in tests (injecting the test version), and the production code should be left unaware of the test stuff. I know that the current code does not satisfy the above, but if we can, let's repair as much as possible during the transition. [1] https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance... https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:46: bool use_mock = false; nit: Please prefix all global variables with g_. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:48: std::unique_ptr<KeyStorageMock> mock; Only plain old data can be a static variable. unique_ptr has a non-trivial destructor and is banned for static variables. https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:56: static std::unique_ptr<KeyStorage> key_storage(KeyStorage::FindService()); Also here, cannot use unique_ptr, because it has a non-trivial destructor. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:62: static std::unique_ptr<std::string> password(new std::string("peanuts")); Also here. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:78: std::function<std::string*()> get_password[] = { std::function is not yet allowed. http://chromium-cpp.appspot.com/ https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:155: // TODO(cfrousios) update with encryption type used nit: Unless this is something planned for the next CL or two, please capture the TODO in a bug or a bug comment and refer to it in the TODO: TODO(crbug.com/XXX). https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:178: if (ciphertext.find(kObfuscationPrefix[Version::V10]) == 0) { Could you use StartsWith from base/strings/string_util.h instead? That would be more efficient than a general find(). Also below. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:22: explicit MockSecretValue(const gchar* password); nit: Please separate the methods from each other and from |password| by blank lines. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:29: // Sets up the minimum mock implementation necessary for |Libsecret| to work. nit: Please separate methods with blank lines. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:39: MockSecretValue::~MockSecretValue() = default; nit: Please add a blank line above this one. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:41: std::unique_ptr<MockSecretValue> stored_password(nullptr); No non-POD data types as static variables (see my comments in the other file). https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:41: std::unique_ptr<MockSecretValue> stored_password(nullptr); nit: Please use g_ as a prefix for global variables. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:41: std::unique_ptr<MockSecretValue> stored_password(nullptr); nit: No need to pass nullptr to a unique_ptr, just use the default constructor. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:111: ASSERT_TRUE(OSCrypt::EncryptString(originaltext, &ciphertext)); The three blocks do not seem to depend on each other. It might be a better idea to split them out into separate tests. The shorter the test, the easier to understand it when it breaks (note that often people who need to deal with broken tests may have no prior knowledge about that area of code). https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:128: OSCrypt::UseMockKeyStorage(false); It might be safer to define a test fixture (based on testing::Test) and put this to its destructor. That way people won't forget to do this if they add a new test. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:135: EXPECT_NE(password, ""); EXPECT_FALSE(password.empty());
https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... File components/os_crypt/key_storage_linux.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:12: #ifdef OFFICIAL_BUILD On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > I'm not sure we need to differentiate the name here. It is not particularly > user-facing, and there is no problem arising from potential clashes between > Chrome and Chromium running on the same computer (it can also be multiple > instances of the official build in parallel, anyway). So to keep out unnecessary > #ifdefs, I would just go with the "Chromium" version. This name is the name Seahorse uses to display the entry. I did it to be consistent with the mac implementation, which also does this. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:19: std::unique_ptr<KeyStorage> KeyStorage::FindService() { On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > nit: Here "Find" is a bit confusing. It sounds like there is a pool of already > created objects, which are searched for the one to return. Instead this just > creates a new one or gives up completely. > > My suggestion: The appropriate name would be Create(). Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:22: key_storage.reset(new KeyStorageLibsecret()); On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > nit: You can merge lines 20 and 22. I added this redundant expression to be symmetrical with other KeyStorages, that are not yet implemented. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:36: std::string AddRandomPasswordInLibsecret() { On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > nit: Please put local (=not exported beyond this .cc file) functions into an > anonymous namespace. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:60: return ""; On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > "" -> std::string() Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:75: KeyStorageMock::KeyStorageMock(std::string in_key) : key(in_key) {} On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > Either use std::move(in_key) or change the type of in_key to "const > std::string&" to avoid unnecessary copies. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.cc:78: if (key == "") On 2016/05/13 15:10:17, vabr (OOO until 22 May) wrote: > if (key_.empty()) > > (empty() is more efficient than string comparison.) Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:12: On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Normally, code in components is in a namespace, here it would be "namespace > os_crypt". But the rest of os_crypt does not seem to use that namespace, so > please confirm with the os_crypt OWNER whether to use that namespace or not. Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:25: // Tries to load available services. Returns the first that succeeds or On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Please be careful with calling stuff a "service". It mostly has the meaning > of a KeyedService in Chromium [1], so let's try to avoid it for things which are > not KeyedService. > > I think calling the object "key storage" or just "storage" is OK given the > type's name. > > > [1] components/keyed_service/core/keyed_service.h Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:33: class KeyStorageLibsecret : public KeyStorage { On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > Please separate the implementations of KeyStorage to their own files, one per > implementation. They will be tiny, but it will match the fact that the user of > KeyStorage should not care about the implementation. It will also allow to > separate the mock from the production code (see the "test_support" targets in > files like components/password_manager/core/browser/BUILD.gn -- we can mark them > with testonly=true and ensure that production is independent of test code). Specifically on the separation of mock from production code, I do not understand what you mean. The existence of a mock (including the type KeyStorageMock) is written into to OSCrypt's main header. os_crypt.h. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:35: std::string GetKey() override; On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: We usually prefix the block with overrides with the name of the base class > they override, in this case: > // KeyStorage > > This case is very simple and it is clear from where the overrides come. But the > class might grow, and the style should be consistent anyway, so please add the > comment. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:41: explicit KeyStorageMock(std::string in_key = ""); On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > Also, whenever you need to initialize an empty string, use std::string(), not > the implicit constructor from const char* applied to "". The former is more > efficient. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:41: explicit KeyStorageMock(std::string in_key = ""); On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > Please use 2 constructors instead of the default argument. In particular, the > default argument is evaluated at each call, so having a 0-argument constructor > will spare a little work. Also, that's how most of the code is written right now > (default args have been banned in the past), so it's better for consistency and > thus readability. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:43: std::string GetKey() override; On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > // KeyStorage > (See above why.) Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:46: void ResetTo(std::string); On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Please name the argument. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/key... components/os_crypt/key_storage_linux.h:49: std::string key; On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Trailing underscore: key_ Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... File components/os_crypt/libsecret_util_posix.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... components/os_crypt/libsecret_util_posix.cc:8: #include <utility> On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > What is this used for? Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/lib... components/os_crypt/libsecret_util_posix.cc:10: #include "base/base64.h" On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > And what is this used for? Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt.h:13: #include "components/os_crypt/key_storage_linux.h" On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > You should #include this file unconditionally if you only put it in build > dependencies on Linux. You could enclose it in the same #ifdef as > UseMockKeyStorage below, but instead I suggest separating these newly added > parts in os_crypt_linux.h and then making os_crypt_linux.h include os_crypt.h > and only referencing os_crypt_linux.h from os_crypt_linux.cc. Right now, the entry-point to OSCrypt is this file for any build. I think that duplicating the entire declaration for just 2 conditional lines is too high a price, especially since we're not solving mac's conditional lines. That said, the declaration itself is also small, so I do not insist. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt.h:48: static KeyStorageMock* UseMockKeyStorage(bool use_mock); On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Please describe what the method does. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > Looking at the code in this file, I see two main issues: > > (1) The use of static variables with non-trivial destructor. Ideally we should > not use them at all. Alternatively, have a look at LazyInstance [1] for how to > create objects as static variables. If you use that, it might make sense to > restructure the code so that you end up with one LazyInstance grouping all of > the static data. > > (2) The mix of test utilities and production code, in particular the mock key > storage. Ideally, all testing support would be in a separate file, part of a > separate build target marked as testonly=true in GN. The production code should > have a single place where to inject an alternative KeyStorage instance, which > would be done in tests (injecting the test version), and the production code > should be left unaware of the test stuff. > > I know that the current code does not satisfy the above, but if we can, let's > repair as much as possible during the transition. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/base/lazy_instance... (1) The source of the problem is that OSCrypt is a static class that ended up needing state. At the very least, it needs a reference to the selected KeyStorage, or else we will have to check for availability every time. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:46: bool use_mock = false; On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > nit: Please prefix all global variables with g_. > > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:48: std::unique_ptr<KeyStorageMock> mock; On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > Only plain old data can be a static variable. unique_ptr has a non-trivial > destructor and is banned for static variables. > > https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:56: static std::unique_ptr<KeyStorage> key_storage(KeyStorage::FindService()); On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > Also here, cannot use unique_ptr, because it has a non-trivial destructor. Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:62: static std::unique_ptr<std::string> password(new std::string("peanuts")); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > Also here. Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:78: std::function<std::string*()> get_password[] = { On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > std::function is not yet allowed. > http://chromium-cpp.appspot.com/ Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:155: // TODO(cfrousios) update with encryption type used On 2016/05/13 15:10:18, vabr (OOO until 22 May) wrote: > nit: Unless this is something planned for the next CL or two, please capture the > TODO in a bug or a bug comment and refer to it in the TODO: TODO(crbug.com/XXX). Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_linux.cc:178: if (ciphertext.find(kObfuscationPrefix[Version::V10]) == 0) { On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > Could you use StartsWith from base/strings/string_util.h instead? That would be > more efficient than a general find(). > > Also below. I'm not sure it is more efficient. find runs on the whole string only for legacy data. StartsWith, on the other hand, has an option for case sensitivity, making it more general in a different way. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:22: explicit MockSecretValue(const gchar* password); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > nit: Please separate the methods from each other and from |password| by blank > lines. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:29: // Sets up the minimum mock implementation necessary for |Libsecret| to work. On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > nit: Please separate methods with blank lines. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:39: MockSecretValue::~MockSecretValue() = default; On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > nit: Please add a blank line above this one. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:41: std::unique_ptr<MockSecretValue> stored_password(nullptr); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > No non-POD data types as static variables (see my comments in the other file). Acknowledged. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:111: ASSERT_TRUE(OSCrypt::EncryptString(originaltext, &ciphertext)); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > The three blocks do not seem to depend on each other. It might be a better idea > to split them out into separate tests. The shorter the test, the easier to > understand it when it breaks (note that often people who need to deal with > broken tests may have no prior knowledge about that area of code). Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:128: OSCrypt::UseMockKeyStorage(false); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > It might be safer to define a test fixture (based on testing::Test) and put this > to its destructor. That way people won't forget to do this if they add a new > test. Done. https://codereview.chromium.org/1973483002/diff/40001/components/os_crypt/os_... components/os_crypt/os_crypt_util_linux_unittest.cc:135: EXPECT_NE(password, ""); On 2016/05/13 15:10:19, vabr (OOO until 22 May) wrote: > EXPECT_FALSE(password.empty()); Done.
Description was changed from ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. Since there is support on the way for more services (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (ie LibsecretLoader) are also used by the PasswordManager. Those utilities can be simplified once PasswordManager stops depending on them. BUG=602624 ========== to ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ==========
cfroussios@chromium.org changed reviewers: + thestig@chromium.org
thestig@chromium.org: Please review changes in os_crypt/ I implemented libsecret support in os_crypt. I'm sorry that the CL grew on its way to you, but hopefully that's not a big problem. Thanks!
Description was changed from ========== OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ========== to ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ==========
https://codereview.chromium.org/1973483002/diff/140001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1973483002/diff/140001/components/components_... components/components_tests.gyp:462: 'os_crypt/os_crypt_util_linux_unittest.cc', Alphabetical order please. Ditto below. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gypi File components/os_crypt.gypi (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:55: 'os_crypt/libsecret_util_linux.cc', Alphabetical order please. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:61: 'os_crypt/key_storage_mock.cc', Is it possible to set up the code so that this is only linked into tests, and not into production code? i.e. os_crypt_linux.cc should not use a KeyStorageMock, but rather refer to it as a KeyStorage. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/BU... File components/os_crypt/BUILD.gn (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/BU... components/os_crypt/BUILD.gn:79: deps += [ "//third_party/libsecret" ] I can't remember if this is strictly necessary with GN since you already depend on the os_crypt target. Care to double check? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:5: #include <components/os_crypt/key_storage_libsecret.h> Use quotes. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:14: #ifdef OFFICIAL_BUILD if defined() https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:29: base::Base64Encode(base::RandBytesAsString(128 / 8), &password); Is there a benefit to writing out "128 / 8" ? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:50: if (error) { Is |password_libsecret| guaranteed to be a nullptr if |error| is not? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (password_libsecret == nullptr) { There's no need for else / else-if after a return. if (cond) { ... return; } // carry on. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (password_libsecret == nullptr) { foo == nullptr -> !foo https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:15: class KeyStorage { Should this be KeyStorageLinux? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:24: // Load the key storage. False is returned if the service is not available. Grammar: - Load -> Loads - False is returned -> Returns false. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:25: virtual bool Init() = 0; Can we reorder the methods? I imagine users will call Init() first, so it should come first. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:5: #include <components/os_crypt/key_storage_mock.h> Yse quotes. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:14: base::Base64Encode(base::RandBytesAsString(128 / 8), &key_); Ditto, is "128 / 8" necessary? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:22: void KeyStorageMock::ResetTo(std::string new_key) { How about making the parameter a base::StringPiece instead? And add a Clear() method instead of calling ResetTo(std::string()))? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:17: KeyStorageMock() = default; Do you need both ctors? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:18: explicit KeyStorageMock(std::string in_key); Parameter should be const-ref. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:28: // Get a pointer to the stored password. Lives as long as this "Lives as long ..." -> KeyStorageMock owns the returned pointer. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:13: #if defined(USE_LIBSECRET) Add a blank line separator https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:51: // For unit testing purposes, use a mocked |KeyStorage| service. Refer to variables as |var_name|, not classes. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:54: static KeyStorageMock* UseMockKeyStorage(bool use_mock); This sounds confusing. What does UseMock...(use_mock = false) mean? UseMockKeychain() above is slightly better but still a bit vague. That and nobody ever calls UseMockKeychain(false). https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:9: #include <functional> What's this used for? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:43: const char* kObfuscationPrefix[] = { const char kObfuscationPrefix[][4] https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:63: KeyStorage::CreateService().release()); foo.reset(bar.release()) -> foo = bar? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:99: if (password == nullptr) !password https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:173: if (ciphertext.find(kObfuscationPrefix[Version::V10]) == 0) { Use base::StartsWith() for readability? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:185: std::string raw_ciphertext = Can you defer this until you use it? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:206: base::LazyInstance<KeyStorageMock>::Leaky key_storage_mock = g_var_name_for_globals https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:5: #include <dlfcn.h> Is this needed? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:10: #include "base/lazy_instance.h" Not needed? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:21: struct MockSecretValue { Structs with a single value seem unnecessary. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:23: std::unique_ptr<std::string> password; Can this just be std::string password? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:96: delete stored_password_mock_ptr_; There's no need to check a value against nullptr before deleting it. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:165: void TearDown() override { OSCrypt::UseMockKeyStorage(false); } Curious, how does OSCryptTest get away without doing this? https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:219: protected: Remove.
Hi. Thanks for the feedback. Can you have a look again? https://codereview.chromium.org/1973483002/diff/140001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1973483002/diff/140001/components/components_... components/components_tests.gyp:462: 'os_crypt/os_crypt_util_linux_unittest.cc', On 2016/05/18 22:38:15, Lei Zhang wrote: > Alphabetical order please. Ditto below. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gypi File components/os_crypt.gypi (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:55: 'os_crypt/libsecret_util_linux.cc', On 2016/05/18 22:38:15, Lei Zhang wrote: > Alphabetical order please. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt.gy... components/os_crypt.gypi:61: 'os_crypt/key_storage_mock.cc', On 2016/05/18 22:38:15, Lei Zhang wrote: > Is it possible to set up the code so that this is only linked into tests, and > not into production code? > > i.e. os_crypt_linux.cc should not use a KeyStorageMock, but rather refer to it > as a KeyStorage. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/BU... File components/os_crypt/BUILD.gn (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/BU... components/os_crypt/BUILD.gn:79: deps += [ "//third_party/libsecret" ] On 2016/05/18 22:38:15, Lei Zhang wrote: > I can't remember if this is strictly necessary with GN since you already depend > on the os_crypt target. Care to double check? We include libsecret_util_linux.h, which brings the <libsecret/secret.h>. The dependency is needed to include the libsecret directory. Alternatively, we can make the //third_party/libsecret configure all dependents. That's an overkill, since os_crypt doesn't need to expose libsecret stuff. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:5: #include <components/os_crypt/key_storage_libsecret.h> On 2016/05/18 22:38:15, Lei Zhang wrote: > Use quotes. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:14: #ifdef OFFICIAL_BUILD On 2016/05/18 22:38:15, Lei Zhang wrote: > if defined() Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:29: base::Base64Encode(base::RandBytesAsString(128 / 8), &password); On 2016/05/18 22:38:15, Lei Zhang wrote: > Is there a benefit to writing out "128 / 8" ? Explicitness about the number of bits used? The mac implementation does it, so I assumed someone cares. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:50: if (error) { On 2016/05/18 22:38:15, Lei Zhang wrote: > Is |password_libsecret| guaranteed to be a nullptr if |error| is not? The examples on the official site make that assumption. The documentation is not explicit. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (password_libsecret == nullptr) { On 2016/05/18 22:38:15, Lei Zhang wrote: > foo == nullptr -> !foo Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (password_libsecret == nullptr) { On 2016/05/18 22:38:15, Lei Zhang wrote: > There's no need for else / else-if after a return. > > if (cond) { > ... > return; > } > > // carry on. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:15: class KeyStorage { On 2016/05/18 22:38:15, Lei Zhang wrote: > Should this be KeyStorageLinux? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:24: // Load the key storage. False is returned if the service is not available. On 2016/05/18 22:38:15, Lei Zhang wrote: > Grammar: > > - Load -> Loads > - False is returned -> Returns false. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:25: virtual bool Init() = 0; On 2016/05/18 22:38:15, Lei Zhang wrote: > Can we reorder the methods? I imagine users will call Init() first, so it should > come first. The code will call indeed call Init() first, but the users shouldn't have to call it at all. I think your point is best addressed by putting CreateService() first and making Init() protected. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:5: #include <components/os_crypt/key_storage_mock.h> On 2016/05/18 22:38:15, Lei Zhang wrote: > Yse quotes. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:14: base::Base64Encode(base::RandBytesAsString(128 / 8), &key_); On 2016/05/18 22:38:15, Lei Zhang wrote: > Ditto, is "128 / 8" necessary? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:22: void KeyStorageMock::ResetTo(std::string new_key) { On 2016/05/18 22:38:15, Lei Zhang wrote: > How about making the parameter a base::StringPiece instead? And add a Clear() > method instead of calling ResetTo(std::string()))? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:17: KeyStorageMock() = default; On 2016/05/18 22:38:16, Lei Zhang wrote: > Do you need both ctors? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:18: explicit KeyStorageMock(std::string in_key); On 2016/05/18 22:38:15, Lei Zhang wrote: > Parameter should be const-ref. This constructor was removed for not being necessary (see comment above). I would note, however, that capturing by value and moving has a better average case, because it allows objects to be moved into the new instance. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.h:28: // Get a pointer to the stored password. Lives as long as this On 2016/05/18 22:38:16, Lei Zhang wrote: > "Lives as long ..." -> KeyStorageMock owns the returned pointer. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:13: #if defined(USE_LIBSECRET) On 2016/05/18 22:38:16, Lei Zhang wrote: > Add a blank line separator Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:51: // For unit testing purposes, use a mocked |KeyStorage| service. On 2016/05/18 22:38:16, Lei Zhang wrote: > Refer to variables as |var_name|, not classes. Acknowledged. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt.h:54: static KeyStorageMock* UseMockKeyStorage(bool use_mock); On 2016/05/18 22:38:16, Lei Zhang wrote: > This sounds confusing. What does UseMock...(use_mock = false) mean? > > UseMockKeychain() above is slightly better but still a bit vague. That and > nobody ever calls UseMockKeychain(false). I changed it so that a pointer is no longer returned. I assume that was what made it worse than UseMockKeychain()? (use_mock = false) means to stop mocking and restore behaviour to whatever is intended for real use. I think such a cleanup is necessary, since the configuration is static and doesn't go away when a test is destroyed. With that argument, UseMockKeychain(false) should also be used, but this CL is focused on Linux stuff https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:9: #include <functional> On 2016/05/18 22:38:16, Lei Zhang wrote: > What's this used for? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:43: const char* kObfuscationPrefix[] = { On 2016/05/18 22:38:16, Lei Zhang wrote: > const char kObfuscationPrefix[][4] Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:63: KeyStorage::CreateService().release()); On 2016/05/18 22:38:16, Lei Zhang wrote: > foo.reset(bar.release()) -> foo = bar? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:99: if (password == nullptr) On 2016/05/18 22:38:16, Lei Zhang wrote: > !password Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:173: if (ciphertext.find(kObfuscationPrefix[Version::V10]) == 0) { On 2016/05/18 22:38:16, Lei Zhang wrote: > Use base::StartsWith() for readability? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:185: std::string raw_ciphertext = On 2016/05/18 22:38:16, Lei Zhang wrote: > Can you defer this until you use it? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:206: base::LazyInstance<KeyStorageMock>::Leaky key_storage_mock = On 2016/05/18 22:38:16, Lei Zhang wrote: > g_var_name_for_globals Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:5: #include <dlfcn.h> On 2016/05/18 22:38:16, Lei Zhang wrote: > Is this needed? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:10: #include "base/lazy_instance.h" On 2016/05/18 22:38:16, Lei Zhang wrote: > Not needed? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:21: struct MockSecretValue { On 2016/05/18 22:38:16, Lei Zhang wrote: > Structs with a single value seem unnecessary. Done. It was struct to keep some symmetry between the declarations of the functions and their mocks. I made it a typedef instead. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:23: std::unique_ptr<std::string> password; On 2016/05/18 22:38:16, Lei Zhang wrote: > Can this just be std::string password? Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:96: delete stored_password_mock_ptr_; On 2016/05/18 22:38:16, Lei Zhang wrote: > There's no need to check a value against nullptr before deleting it. Done. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:165: void TearDown() override { OSCrypt::UseMockKeyStorage(false); } On 2016/05/18 22:38:16, Lei Zhang wrote: > Curious, how does OSCryptTest get away without doing this? This allows calls to OSCrypt functions to reach all the way to (mocked) libsecret/kwallet functions. It is not strictly necessary, since current tests don't attempt to do that. However, keeping it there helps avoid flakiness in future contributions to the tests. I have now added it to OSCryptTest too. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:219: protected: On 2016/05/18 22:38:16, Lei Zhang wrote: > Remove. Done.
Getting closer. https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:14: base::Base64Encode(base::RandBytesAsString(128 / 8), &key_); On 2016/05/19 21:18:18, cfroussios wrote: > On 2016/05/18 22:38:15, Lei Zhang wrote: > > Ditto, is "128 / 8" necessary? > > Done. Wait, you kept it in the other place where I asked the same question. Please be consistent. https://codereview.chromium.org/1973483002/diff/200001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1973483002/diff/200001/components/components_... components/components_tests.gyp:464: 'os_crypt/key_storage_mock.cc', In ASCII order, '_' comes be 'c'. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/BU... File components/os_crypt/BUILD.gn (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/BU... components/os_crypt/BUILD.gn:76: sources += [ Can we have components/components_tests.gyp follow the same logic? As is, the GYP version does not exclude the mock. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (!password_libsecret) { There's still an else-if after a return. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt.h:61: void UseMockKeyStorageForTesting(bool use, You probably don't need |use| - you can indicate use = false with both function pointers being nullptr. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_unittest.cc:24: void SetUp() override { OSCrypt::UseMockKeychain(true); } I'd highly prefer not to have two different implementations of the same function. Can we keep the ifdefs inside the function, like before? https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_unittest.cc:43: static KeyStorageMock* key_storage_static_; Many places use s_foo_ here, like g_foo for globals. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: typedef std::string MockSecretValue; using foo = bar, instead of typedef bar foo. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:29: // Set OSCrypt's password in the libsecret mock to a specific value Sets
Patchset #12 (id:220001) has been deleted
Patchset #12 (id:240001) has been deleted
Hi. Thanks for the feedback again! I changed the way mocking is requested a bit more radically. I realised that a lot of other tests use OSCrypt and end up hitting the real Libsecret. They also need to use a mock, therefore I reduced the boilerplate needed to set this up. I will go over all of those dependents and add the appropriate setup and tear down. I plan this to be a separate CL that will land before this one. Before changing 40+ tests, I would like to agree on how they'll request mocking. The tests here serve as examples. So, can you have a look again? Thanks! https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... File components/os_crypt/key_storage_mock.cc (right): https://codereview.chromium.org/1973483002/diff/140001/components/os_crypt/ke... components/os_crypt/key_storage_mock.cc:14: base::Base64Encode(base::RandBytesAsString(128 / 8), &key_); On 2016/05/19 22:45:38, Lei Zhang wrote: > On 2016/05/19 21:18:18, cfroussios wrote: > > On 2016/05/18 22:38:15, Lei Zhang wrote: > > > Ditto, is "128 / 8" necessary? > > > > Done. > > Wait, you kept it in the other place where I asked the same question. Please be > consistent. Done. https://codereview.chromium.org/1973483002/diff/200001/components/components_... File components/components_tests.gyp (right): https://codereview.chromium.org/1973483002/diff/200001/components/components_... components/components_tests.gyp:464: 'os_crypt/key_storage_mock.cc', On 2016/05/19 22:45:38, Lei Zhang wrote: > In ASCII order, '_' comes be 'c'. Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/BU... File components/os_crypt/BUILD.gn (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/BU... components/os_crypt/BUILD.gn:76: sources += [ On 2016/05/19 22:45:38, Lei Zhang wrote: > Can we have components/components_tests.gyp follow the same logic? As is, the > GYP version does not exclude the mock. In components/components_tests.gyp, the mock is in 'os_crypt_unittest_sources'. Aren't all of those files supposed to be excluded? https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.cc:54: } else if (!password_libsecret) { On 2016/05/19 22:45:38, Lei Zhang wrote: > There's still an else-if after a return. Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt.h (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt.h:61: void UseMockKeyStorageForTesting(bool use, On 2016/05/19 22:45:38, Lei Zhang wrote: > You probably don't need |use| - you can indicate use = false with both function > pointers being nullptr. Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_unittest.cc:24: void SetUp() override { OSCrypt::UseMockKeychain(true); } On 2016/05/19 22:45:38, Lei Zhang wrote: > I'd highly prefer not to have two different implementations of the same > function. Can we keep the ifdefs inside the function, like before? Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_unittest.cc:43: static KeyStorageMock* key_storage_static_; On 2016/05/19 22:45:38, Lei Zhang wrote: > Many places use s_foo_ here, like g_foo for globals. Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: typedef std::string MockSecretValue; On 2016/05/19 22:45:38, Lei Zhang wrote: > using foo = bar, instead of typedef bar foo. Done. https://codereview.chromium.org/1973483002/diff/200001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:29: // Set OSCrypt's password in the libsecret mock to a specific value On 2016/05/19 22:45:38, Lei Zhang wrote: > Sets Done.
Getting closer. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.cc (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:11: namespace { Add blank line after. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:19: } Add blank line before, add // namespace https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:41: return base::Singleton<OSCryptMockerLinux>::get(); Can we use a lazy instance instead? See comment in singleton.h. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.h (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:11: #include "base/memory/singleton.h" Not needed in the header. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:27: // Get a pointer to the stored password. |KeyStorageMock| owns the pointer |KeyStorageMock| -> OSCryptMockerLinux https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:25: // Sets up the minimum mock implementation necessary for |OSCrypt| to work nit: OSCrypt is not a variable. Ditto for line 110 and 119. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:151: void TearDown() override { OSCryptMockerLinux::SetUpWithSingleton(); } Did you mean to call OSCryptMockerLinux::TearDown() here?
sevenfold1999ac@gmail.com changed reviewers: + sevenfold1999AC@gmail.com
lgtm https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.h (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:11: #include "base/memory/singleton.h" On 2016/05/20 21:15:33, Lei Zhang wrote: > Not needed in the header. Acknowledged. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:27: // Get a pointer to the stored password. |KeyStorageMock| owns the pointer On 2016/05/20 21:15:33, Lei Zhang wrote: > |KeyStorageMock| -> OSCryptMockerLinux Acknowledged. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:27: // Get a pointer to the stored password. |KeyStorageMock| owns the pointer On 2016/05/20 21:15:33, Lei Zhang wrote: > |KeyStorageMock| -> OSCryptMockerLinux Done.
sevenfold1999ac@gmail.com changed reviewers: + sevenfold1999ac@gmail.com
lgtm
Description was changed from ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorage API. The libsecret utilities that KeyStorage uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ========== to ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorageLinux API. The libsecret utilities that KeyStorageLinux uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ==========
cfroussios@chromium.org changed reviewers: - sevenfold1999AC@gmail.com, sevenfold1999ac@gmail.com
https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.cc (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:11: namespace { On 2016/05/20 21:15:33, Lei Zhang wrote: > Add blank line after. Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:19: } On 2016/05/20 21:15:33, Lei Zhang wrote: > Add blank line before, add // namespace Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:41: return base::Singleton<OSCryptMockerLinux>::get(); On 2016/05/20 21:15:33, Lei Zhang wrote: > Can we use a lazy instance instead? See comment in singleton.h. Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.h (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:11: #include "base/memory/singleton.h" On 2016/05/20 21:15:33, Lei Zhang wrote: > Not needed in the header. Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:27: // Get a pointer to the stored password. |KeyStorageMock| owns the pointer On 2016/05/20 21:15:33, Lei Zhang wrote: > |KeyStorageMock| -> OSCryptMockerLinux Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:25: // Sets up the minimum mock implementation necessary for |OSCrypt| to work On 2016/05/20 21:15:33, Lei Zhang wrote: > nit: OSCrypt is not a variable. Ditto for line 110 and 119. Done. https://codereview.chromium.org/1973483002/diff/320001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:151: void TearDown() override { OSCryptMockerLinux::SetUpWithSingleton(); } On 2016/05/20 21:15:33, Lei Zhang wrote: > Did you mean to call OSCryptMockerLinux::TearDown() here? Done.
LGTM with some comments. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:13: // Specialisation of |KeyStorage| that uses Libsecret nit: Please end the sentence with a full-stop. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:14: // service nit: Also here, please end the sentence with a full-stop. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:36: // Used for array indexing nit: Full-stop at the end. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:42: // Prefix for cypher text returned by obfuscation version. We prefix the The US spelling of cypher seems to be with -i- only (UK seems to allow both). Please make the whole file consistent and conform to the US spelling. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:69: // Returns a cached string of "peanuts" nit: full-stop at the end https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:97: // Generates a newly allocated SymmetricKey object based a hard-coded password. nit: "based a" -> "based on a" https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:98: // Ownership of the key is passed to the caller. Returns NULL key if a key nit: Please use just one space to separate sentences. (I'm not claiming one is better than two or vice versa, but let's be consistent with the rest of the file.) https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:98: // Ownership of the key is passed to the caller. Returns NULL key if a key optional nit: NULL -> null Because NULL is an obsolete C macro, whereas null is an English adjective describing what makes nullptr a special pointer. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:112: DCHECK(encryption_key.get()); Please drop ".get()", unique_ptr should convert to bool implicitly. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:140: *ciphertext = std::string(); ciphertext->clear() (Let's use the most direct STL method available, both to improve readability, and also for performance.) https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:150: if (!encryption_key.get()) no need for .get() https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:170: *plaintext = std::string(); plaintext->clear() https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:193: if (!encryption_key.get()) no need for .get() https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:220: get_password[Version::V11] = &GetPasswordV11; Please do not make the assumption that the "real implementation" is V11. The person who potentially adds some V12 in the future is likely to miss this line. Instead, could you cache the initial values and restore those? https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:14: static base::LazyInstance<OSCryptMockerLinux>::Leaky instance = nit: Please use the g_ prefix. Also, consider calling out the type instead of "instance". What about g_mocker? https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:23: // Set the password that OSCryptMockerLinux holds nit: Please end sentences with a full-stop. (Also on lines 26 and 29.) https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; Why are we renaming std::string? Unless there is a concrete benefit in doing that, I would suggest keeping std::string is more readable. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:36: static MockSecretValue* stored_password_mock_ptr_; Why does this need to be static? It is only reset in the static method, all other uses are within non-static ones. Could you make this a non-static unique_ptr? https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:138: libsecret_loaded_ = false; // function pointers will be restored when loading nit: Start a sentence with a capital letter and end it with a full-stop. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:141: class OSCryptLinuxTest : public testing::Test { Could all the OSCryptLinuxTests be in a separate file, os_crypt_linux_unittest.cc? It is unexpected to have them here, and may lead people to omit adding more tests in the future, because they would think there are none so far.
Hi. I addressed the comments. I also updated OSCryptMocker with the linux implementation that is being introduced. Can you have another look? Thanks! https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... File components/os_crypt/key_storage_libsecret.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... components/os_crypt/key_storage_libsecret.h:13: // Specialisation of |KeyStorage| that uses Libsecret On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Please end the sentence with a full-stop. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... File components/os_crypt/key_storage_linux.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/ke... components/os_crypt/key_storage_linux.h:14: // service On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Also here, please end the sentence with a full-stop. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:36: // Used for array indexing On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Full-stop at the end. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:42: // Prefix for cypher text returned by obfuscation version. We prefix the On 2016/05/23 12:38:33, vabr (Chromium) wrote: > The US spelling of cypher seems to be with -i- only (UK seems to allow both). > Please make the whole file consistent and conform to the US spelling. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:69: // Returns a cached string of "peanuts" On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: full-stop at the end Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:97: // Generates a newly allocated SymmetricKey object based a hard-coded password. On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: "based a" -> "based on a" Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:98: // Ownership of the key is passed to the caller. Returns NULL key if a key On 2016/05/23 12:38:33, vabr (Chromium) wrote: > optional nit: NULL -> null > Because NULL is an obsolete C macro, whereas null is an English adjective > describing what makes nullptr a special pointer. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:98: // Ownership of the key is passed to the caller. Returns NULL key if a key On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Please use just one space to separate sentences. (I'm not claiming one is > better than two or vice versa, but let's be consistent with the rest of the > file.) Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:112: DCHECK(encryption_key.get()); On 2016/05/23 12:38:33, vabr (Chromium) wrote: > Please drop ".get()", unique_ptr should convert to bool implicitly. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:140: *ciphertext = std::string(); On 2016/05/23 12:38:33, vabr (Chromium) wrote: > ciphertext->clear() > > (Let's use the most direct STL method available, both to improve readability, > and also for performance.) Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:150: if (!encryption_key.get()) On 2016/05/23 12:38:33, vabr (Chromium) wrote: > no need for .get() Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:170: *plaintext = std::string(); On 2016/05/23 12:38:33, vabr (Chromium) wrote: > plaintext->clear() Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:193: if (!encryption_key.get()) On 2016/05/23 12:38:33, vabr (Chromium) wrote: > no need for .get() Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:220: get_password[Version::V11] = &GetPasswordV11; On 2016/05/23 12:38:33, vabr (Chromium) wrote: > Please do not make the assumption that the "real implementation" is V11. The > person who potentially adds some V12 in the future is likely to miss this line. > Instead, could you cache the initial values and restore those? Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.cc:14: static base::LazyInstance<OSCryptMockerLinux>::Leaky instance = On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Please use the g_ prefix. Also, consider calling out the type instead of > "instance". What about g_mocker? Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_mocker_linux.h (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_mocker_linux.h:23: // Set the password that OSCryptMockerLinux holds On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Please end sentences with a full-stop. (Also on lines 26 and 29.) Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; On 2016/05/23 12:38:33, vabr (Chromium) wrote: > Why are we renaming std::string? Unless there is a concrete benefit in doing > that, I would suggest keeping std::string is more readable. The MockSecretValue maintains symmetry between libsecret methods and their mocks. Someone introducing a new libsecret function and its mock will be coming with the original signature in mind. I would argue that it is more readable to acknowledge the type rather than erase it. That said, I don't have a strong preference. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:36: static MockSecretValue* stored_password_mock_ptr_; On 2016/05/23 12:38:33, vabr (Chromium) wrote: > Why does this need to be static? It is only reset in the static method, all > other uses are within non-static ones. Could you make this a non-static > unique_ptr? mock_secret_password_store_sync needs to be able to set it and mock_secret_value_get_text needs to return it. Those need to be static functions, in order to mock libsecret calls. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:138: libsecret_loaded_ = false; // function pointers will be restored when loading On 2016/05/23 12:38:33, vabr (Chromium) wrote: > nit: Start a sentence with a capital letter and end it with a full-stop. Done. https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:141: class OSCryptLinuxTest : public testing::Test { On 2016/05/23 12:38:33, vabr (Chromium) wrote: > Could all the OSCryptLinuxTests be in a separate file, > os_crypt_linux_unittest.cc? It is unexpected to have them here, and may lead > people to omit adding more tests in the future, because they would think there > are none so far. Done.
I still have a question about MockSecretValue, and some more nits, but otherwise LGTM. Cheers, Vaclav https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; On 2016/05/30 11:50:17, cfroussios wrote: > On 2016/05/23 12:38:33, vabr (Chromium) wrote: > > Why are we renaming std::string? Unless there is a concrete benefit in doing > > that, I would suggest keeping std::string is more readable. > > The MockSecretValue maintains symmetry between libsecret methods and their > mocks. Someone introducing a new libsecret function and its mock will be coming > with the original signature in mind. I would argue that it is more readable to > acknowledge the type rather than erase it. That said, I don't have a strong > preference. I'm afraid I don't understand what you mean by "maintains symmetry", could you please try to explain that bit again to me? https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:36: static MockSecretValue* stored_password_mock_ptr_; On 2016/05/30 11:50:17, cfroussios wrote: > On 2016/05/23 12:38:33, vabr (Chromium) wrote: > > Why does this need to be static? It is only reset in the static method, all > > other uses are within non-static ones. Could you make this a non-static > > unique_ptr? > > mock_secret_password_store_sync needs to be able to set it and > mock_secret_value_get_text needs to return it. Those need to be static > functions, in order to mock libsecret calls. Acknowledged. https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:30: static MockSecretValue* stored_password_mock_ptr_; nit: This needs to be declared after all the method declarations, see https://google.github.io/styleguide/cppguide.html#Declaration_Order . https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:30: static MockSecretValue* stored_password_mock_ptr_; nit: Please add a comment saying that this pointer owns the object it points to. https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:67: gboolean MockLibsecretLoader::mock_secret_password_store_sync( nit: // static https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:80: MockSecretValue* MockLibsecretLoader::mock_secret_service_lookup_sync( nit: // static
I addressed your comments and explained what I mean by symmetry. If you're still not convinced, just let me know. I don't think this is important enough to defend further. -Christos https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; On 2016/05/30 15:05:37, vabr (Chromium) wrote: > On 2016/05/30 11:50:17, cfroussios wrote: > > On 2016/05/23 12:38:33, vabr (Chromium) wrote: > > > Why are we renaming std::string? Unless there is a concrete benefit in doing > > > that, I would suggest keeping std::string is more readable. > > > > The MockSecretValue maintains symmetry between libsecret methods and their > > mocks. Someone introducing a new libsecret function and its mock will be > coming > > with the original signature in mind. I would argue that it is more readable to > > acknowledge the type rather than erase it. That said, I don't have a strong > > preference. > > I'm afraid I don't understand what you mean by "maintains symmetry", could you > please try to explain that bit again to me? By symmetry I mean calling out the type being mocked. Where there is SecretValue in libsecret, the mock has MockSecretValue. SecretValue is a complex type in libsecret, separate from text values that appear as char arrays in the signatures. Here, we can reduce it to an std::string because 1) In the current implementation, we only need one of its fields. 3) Production code only accesses its fields through functions. 2) The mock function is cast to the correct signature in when setting it up. Therefore, the pointers to SecretValue can point to anything we find convenient. I think that expecting a future author, who's looking for what to do with a SecretValue, to see std::string and deduce how we cast to something else entirely, is more mental work. Calling out the mocked type gives a trail to follow. It also makes more apparent what needs to change, if we use more fields of SecretValue in the future. https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:30: static MockSecretValue* stored_password_mock_ptr_; On 2016/05/30 15:05:38, vabr (Chromium) wrote: > nit: Please add a comment saying that this pointer owns the object it points to. Done. https://codereview.chromium.org/1973483002/diff/380001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:30: static MockSecretValue* stored_password_mock_ptr_; On 2016/05/30 15:05:37, vabr (Chromium) wrote: > nit: This needs to be declared after all the method declarations, see > https://google.github.io/styleguide/cppguide.html#Declaration_Order . Done.
LGTM, thanks! https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; On 2016/05/30 16:19:41, cfroussios wrote: > On 2016/05/30 15:05:37, vabr (Chromium) wrote: > > On 2016/05/30 11:50:17, cfroussios wrote: > > > On 2016/05/23 12:38:33, vabr (Chromium) wrote: > > > > Why are we renaming std::string? Unless there is a concrete benefit in > doing > > > > that, I would suggest keeping std::string is more readable. > > > > > > The MockSecretValue maintains symmetry between libsecret methods and their > > > mocks. Someone introducing a new libsecret function and its mock will be > > coming > > > with the original signature in mind. I would argue that it is more readable > to > > > acknowledge the type rather than erase it. That said, I don't have a strong > > > preference. > > > > I'm afraid I don't understand what you mean by "maintains symmetry", could you > > please try to explain that bit again to me? > > By symmetry I mean calling out the type being mocked. Where there is SecretValue > in libsecret, the mock has MockSecretValue. > > SecretValue is a complex type in libsecret, separate from text values that > appear as char arrays in the signatures. Here, we can reduce it to an > std::string because > 1) In the current implementation, we only need one of its fields. > 3) Production code only accesses its fields through functions. > 2) The mock function is cast to the correct signature in when setting it up. > Therefore, the pointers to SecretValue can point to anything we find convenient. > > I think that expecting a future author, who's looking for what to do with a > SecretValue, to see std::string and deduce how we cast to something else > entirely, is more mental work. Calling out the mocked type gives a trail to > follow. It also makes more apparent what needs to change, if we use more fields > of SecretValue in the future. Thanks, this makes great sense to me now. I agree with your choice, please keep MockSecretValue. It might be a good idea to summarise your explanation in a form of a code comment, though, in case somebody as slow as me is reading the code in the future and wondering what this is. :)
lgtm https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:95: std::string* (*get_password[])() = { g_get_password? https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:218: *get_password_save[sizeof(get_password) / sizeof(get_password[0])])(); arraysize(g_get_password) ?
Thanks all for the feedback! https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... File components/os_crypt/os_crypt_util_linux_unittest.cc (right): https://codereview.chromium.org/1973483002/diff/340001/components/os_crypt/os... components/os_crypt/os_crypt_util_linux_unittest.cc:20: using MockSecretValue = std::string; On 2016/05/30 18:00:47, vabr (Chromium) wrote: > On 2016/05/30 16:19:41, cfroussios wrote: > > On 2016/05/30 15:05:37, vabr (Chromium) wrote: > > > On 2016/05/30 11:50:17, cfroussios wrote: > > > > On 2016/05/23 12:38:33, vabr (Chromium) wrote: > > > > > Why are we renaming std::string? Unless there is a concrete benefit in > > doing > > > > > that, I would suggest keeping std::string is more readable. > > > > > > > > The MockSecretValue maintains symmetry between libsecret methods and their > > > > mocks. Someone introducing a new libsecret function and its mock will be > > > coming > > > > with the original signature in mind. I would argue that it is more > readable > > to > > > > acknowledge the type rather than erase it. That said, I don't have a > strong > > > > preference. > > > > > > I'm afraid I don't understand what you mean by "maintains symmetry", could > you > > > please try to explain that bit again to me? > > > > By symmetry I mean calling out the type being mocked. Where there is > SecretValue > > in libsecret, the mock has MockSecretValue. > > > > SecretValue is a complex type in libsecret, separate from text values that > > appear as char arrays in the signatures. Here, we can reduce it to an > > std::string because > > 1) In the current implementation, we only need one of its fields. > > 3) Production code only accesses its fields through functions. > > 2) The mock function is cast to the correct signature in when setting it up. > > Therefore, the pointers to SecretValue can point to anything we find > convenient. > > > > I think that expecting a future author, who's looking for what to do with a > > SecretValue, to see std::string and deduce how we cast to something else > > entirely, is more mental work. Calling out the mocked type gives a trail to > > follow. It also makes more apparent what needs to change, if we use more > fields > > of SecretValue in the future. > > Thanks, this makes great sense to me now. I agree with your choice, please keep > MockSecretValue. It might be a good idea to summarise your explanation in a form > of a code comment, though, in case somebody as slow as me is reading the code in > the future and wondering what this is. :) Done. https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... File components/os_crypt/os_crypt_linux.cc (right): https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:95: std::string* (*get_password[])() = { On 2016/05/30 20:57:09, Lei Zhang wrote: > g_get_password? Done. https://codereview.chromium.org/1973483002/diff/420001/components/os_crypt/os... components/os_crypt/os_crypt_linux.cc:218: *get_password_save[sizeof(get_password) / sizeof(get_password[0])])(); On 2016/05/30 20:57:09, Lei Zhang wrote: > arraysize(g_get_password) ? Done.
The CQ bit was checked by cfroussios@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sevenfold1999AC@gmail.com, thestig@chromium.org, vabr@chromium.org Link to the patchset: https://codereview.chromium.org/1973483002/#ps440001 (title: "Recommendations")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973483002/440001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973483002/440001
Message was sent while issue was closed.
Description was changed from ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorageLinux API. The libsecret utilities that KeyStorageLinux uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ========== to ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorageLinux API. The libsecret utilities that KeyStorageLinux uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:440001)
Message was sent while issue was closed.
Description was changed from ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorageLinux API. The libsecret utilities that KeyStorageLinux uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 ========== to ========== Implement OSCrypt for Linux. OSCrypt for linux uses libsecret to store a randomised encryption key. Mentions of OSCrypt below refer to the linux version. OSCrypt now supports a new version of encryption. If libsecret is available, then a randomised password will be generated, stored in libsecret and used for producing encryption keys. The old ways of encrypting, i.e. using a hardcoded password and using no encryption at all, are still supported. Data encrypted with the new version are marked with a version prefix, "v11". That is an increment on the previous prefix, "v10", which separated data that was encrypted with the hardcoded password from data that were not encrypted. To separate this new linux feature from the generic posix implementation, a new implementation for OSCrypt was created in os_crypt_linux.cc. Since there is support on the way for more services in addition to libsecret (gnome keyring, kwallet), interaction with such services is abstracted with the KeyStorageLinux API. The libsecret utilities that KeyStorageLinux uses (i.e. LibsecretLoader) are also used by the PasswordManager. Here we only actually need a subset of their features. Once PasswordManager stops depending on them, they can be simplified and/or hidden in the KeyStorageLibsecret implementation. BUG=602624 Committed: https://crrev.com/3b5a4e4f0f10e73b9ace1e61079b099a9e62b1d8 Cr-Commit-Position: refs/heads/master@{#396814} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/3b5a4e4f0f10e73b9ace1e61079b099a9e62b1d8 Cr-Commit-Position: refs/heads/master@{#396814} |
