OSCrypt for POSIX uses libsecret to store a randomised encryption key.

components/os_crypt/key_storage_linux.h

+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef COMPONENTS_OS_CRYPT_KEY_STORAGE_LINUX_H_
+#define COMPONENTS_OS_CRYPT_KEY_STORAGE_LINUX_H_
+
+#include <memory>
+#include <string>
+
+#include "base/macros.h"
+

[vabr (Chromium), 2016/05/13 15:10:18]

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.

[cfroussios, 2016/05/13 17:09:11]

Acknowledged.

+class KeyStorage {
+ public:
+  KeyStorage() = default;
+  virtual ~KeyStorage() = default;
+
+  // Gets the encryption key from the OS password-managing library. If a key is
+  // not found, a new key will be generated, stored and returned.
+  virtual std::string GetKey() = 0;
+
+  // Load the service. False is returned if the service is not available.
+  virtual bool Init() = 0;
+
+  // Tries to load available services. Returns the first that succeeds or

[vabr (Chromium), 2016/05/13 15:10:18]

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

[cfroussios, 2016/05/13 17:09:11]

Done.

+  // null if none succeed.
+  static std::unique_ptr<KeyStorage> FindService();
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(KeyStorage);
+};
+
+class KeyStorageLibsecret : public KeyStorage {

[vabr (Chromium), 2016/05/13 15:10:18]

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).

[cfroussios, 2016/05/13 17:09:11]

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.

+ public:
+  std::string GetKey() override;

[vabr (Chromium), 2016/05/13 15:10:18]

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.

[cfroussios, 2016/05/13 17:09:12]

Done.

+  bool Init() override;
+};
+
+class KeyStorageMock : public KeyStorage {
+ public:
+  explicit KeyStorageMock(std::string in_key = "");

[vabr (Chromium), 2016/05/13 15:10:18]

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.

[vabr (Chromium), 2016/05/13 15:10:18]

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.

[cfroussios, 2016/05/13 17:09:11]

Done.

[cfroussios, 2016/05/13 17:09:11]

Done.

+
+  std::string GetKey() override;

[vabr (Chromium), 2016/05/13 15:10:18]

// KeyStorage
(See above why.)

[cfroussios, 2016/05/13 17:09:11]

Done.

+  bool Init() override;
+
+  void ResetTo(std::string);

[vabr (Chromium), 2016/05/13 15:10:18]

nit: Please name the argument.

[cfroussios, 2016/05/13 17:09:12]

Done.

+
+ private:
+  std::string key;

[vabr (Chromium), 2016/05/13 15:10:18]

nit: Trailing underscore: key_

[cfroussios, 2016/05/13 17:09:11]

Done.

+};
+
+#endif  // COMPONENTS_OS_CRYPT_KEY_STORAGE_LINUX_H_