Implement pin storage backend.

chrome/browser/chromeos/login/quick_unlock/pin_storage.cc

+// 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.
+
+#include "chrome/browser/chromeos/login/quick_unlock/pin_storage.h"
+
+#include "base/base64.h"
+#include "base/lazy_instance.h"
+#include "base/strings/string_util.h"
+#include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chrome/common/pref_names.h"
+#include "chromeos/login/auth/key.h"
+#include "components/prefs/pref_service.h"
+#include "crypto/random.h"
+#include "extensions/browser/browser_context_keyed_api_factory.h"
+
+namespace chromeos {
+
+namespace {
+
+const int kSaltByteSize = 16;
+
+class PinStorageImpl : public PinStorage,
+                       public extensions::BrowserContextKeyedAPI {
+ public:
+  explicit PinStorageImpl(content::BrowserContext* context);
+  ~PinStorageImpl() override;

[achuithb, 2016/05/13 23:34:56]

Can this be protected?

[jdufault, 2016/05/16 22:08:47]

Done.

+
+  // PinStorage implementation:

[achuithb, 2016/05/13 23:34:56]

Everything below here can be private, right?

[jdufault, 2016/05/16 22:08:47]

Done.

+  void MarkStrongAuth() override;
+  base::TimeDelta TimeSinceLastStrongAuth() override;
+
+  void AddUnlockAttempt() override;
+  void ResetUnlockAttemptCount() override;
+  int UnlockAttemptCount() override;

[achuithb, 2016/05/13 23:34:56]

const

[jdufault, 2016/05/16 22:08:47]

Done.

+
+  // PIN storage management.
+  bool HasPin() const override;
+  void SetPin(const std::string& raw_pin) override;
+  void RemovePin() override;
+
+  // The salt and hash for the pin. These methods can only be called if HasPin
+  // returns true.
+  std::string pin_salt() const override;
+  std::string pin_secret() const override;
+
+  // Fetches an instance for the given context.
+  static PinStorageImpl* Get(content::BrowserContext* context);
+
+  // BrowserContextKeyedAPI implementation:
+  static extensions::BrowserContextKeyedAPIFactory<PinStorageImpl>*
+  GetFactoryInstance();
+  static const bool kServiceIsCreatedWithBrowserContext = false;
+
+ private:
+  PrefService* pref_service_;
+  int attempt_count_;
+  base::Time last_strong_auth_;

[achuithb, 2016/05/13 23:34:57]

should this be initialized to something?

[jdufault, 2016/05/16 22:08:47]

It's default constructed to the "null" time (IsPinAvailable needs to check for
this - done in the next patch).

+
+  friend class extensions::BrowserContextKeyedAPIFactory<PinStorageImpl>;
+  // BrowserContextKeyedAPI implementation:
+  static const char* service_name() { return "PinStorageImpl"; }
+
+  DISALLOW_COPY_AND_ASSIGN(PinStorageImpl);
+};
+
+static base::LazyInstance<
+    extensions::BrowserContextKeyedAPIFactory<PinStorageImpl>>
+    g_factory = LAZY_INSTANCE_INITIALIZER;
+
+// static
+extensions::BrowserContextKeyedAPIFactory<PinStorageImpl>*
+PinStorageImpl::GetFactoryInstance() {
+  return g_factory.Pointer();
+}
+
+// static
+PinStorageImpl* PinStorageImpl::Get(content::BrowserContext* context) {
+  return extensions::BrowserContextKeyedAPIFactory<PinStorageImpl>::Get(
+      context);
+}
+
+PinStorageImpl::PinStorageImpl(content::BrowserContext* context)
+    : pref_service_(Profile::FromBrowserContext(context)->GetPrefs()),
+      attempt_count_(0) {}

[achuithb, 2016/05/13 23:34:56]

move to class

[jdufault, 2016/05/16 22:08:47]

Done.

+
+PinStorageImpl::~PinStorageImpl() {}
+
+void PinStorageImpl::MarkStrongAuth() {
+  last_strong_auth_ = base::Time::Now();
+}
+
+base::TimeDelta PinStorageImpl::TimeSinceLastStrongAuth() {
+  // TODO: Verify last_strong_auth_ has a good initial value.
+  return base::Time::Now() - last_strong_auth_;
+}
+
+void PinStorageImpl::AddUnlockAttempt() {
+  ++attempt_count_;
+}
+
+void PinStorageImpl::ResetUnlockAttemptCount() {
+  attempt_count_ = 0;
+}
+
+int PinStorageImpl::UnlockAttemptCount() {
+  return attempt_count_;
+}
+
+bool PinStorageImpl::HasPin() const {
+  return !pin_salt().empty() && !pin_secret().empty();
+}
+
+void PinStorageImpl::SetPin(const std::string& raw_pin) {

[achuithb, 2016/05/13 23:34:56]

This method is probably the most significant bit of code in this CL. Is there a
way to unittest this?

[jdufault, 2016/05/16 22:08:48]

I haven't been able to figure out any tests that would add value.

The best I can think of is making the salt/secret generation done via a
delegate, and then overloading that delegate in a test, and then verifying the
set prefs are equal to the values returned by the delegate.

Note that there is a test verifying that the pref service receives some values.

+  // Compute salt. base64 encode it so it is a UTF8 string, which is required by

[achuithb, 2016/05/13 23:34:57]

Generate random salt.

[jdufault, 2016/05/16 22:08:48]

Done.

+  // the pref service.
+  std::string salt;
+  crypto::RandBytes(base::WriteInto(&salt, kSaltByteSize + 1), kSaltByteSize);
+  base::Base64Encode(salt, &salt);
+  DCHECK(!salt.empty());
+
+  // Compute secret.
+  Key key(raw_pin);
+  key.Transform(Key::KEY_TYPE_SALTED_PBKDF2_AES256_1234, salt);
+  std::string secret = key.GetSecret();

[achuithb, 2016/05/13 23:34:57]

const

[jdufault, 2016/05/16 22:08:47]

Done.

+
+  // Store them.
+  pref_service_->SetString(prefs::kQuickUnlockPinSalt, salt);
+  pref_service_->SetString(prefs::kQuickUnlockPinSecret, secret);
+}
+
+void PinStorageImpl::RemovePin() {
+  // Clear prefs to their default values.
+  pref_service_->SetString(prefs::kQuickUnlockPinSalt, "");
+  pref_service_->SetString(prefs::kQuickUnlockPinSecret, "");
+}
+
+std::string PinStorageImpl::pin_salt() const {

[achuithb, 2016/05/13 23:34:56]

PinSalt?

[jdufault, 2016/05/16 22:08:48]

Done.

+  return pref_service_->GetString(prefs::kQuickUnlockPinSalt);
+}
+
+std::string PinStorageImpl::pin_secret() const {

[achuithb, 2016/05/13 23:34:57]

PinSecret?

[jdufault, 2016/05/16 22:08:47]

Done.

+  return pref_service_->GetString(prefs::kQuickUnlockPinSecret);
+}
+
+}  // namespace
+
+PinStorage::PinStorage() {}
+
+PinStorage::~PinStorage() {}
+
+// static
+void PinStorage::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterStringPref(prefs::kQuickUnlockPinSalt, "",
+                               user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);

[achuithb, 2016/05/13 23:34:57]

Do we want this to be syncable?

[jdufault, 2016/05/16 22:08:47]

Security is fine with it, but we're still not sure which way we want to go. I
prefer syncable so that's what I went with for now, but it may change. Either
way, it should be easy to swap out, since both the syncable and non-syncable
approaches use the pref system.

+  registry->RegisterStringPref(prefs::kQuickUnlockPinSecret, "",
+                               user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+}
+
+// static
+PinStorage* PinStorage::GetInstance(Profile* profile) {
+  return PinStorageImpl::Get(profile);
+}
+
+}  // namespace chromeos