Add a dummy entry with libsecret when initializing OSCrypt.

components/os_crypt/libsecret_util_linux.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 "components/os_crypt/libsecret_util_linux.h"
 
 #include <dlfcn.h>
 
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 
 //
 // LibsecretLoader
 //
 
+namespace {
+
+// TODO(crbug.com/660005) A message that is attached to useless entries that we
+// create, to explain its existence.
+const char kExplanationMessage[] =
+    "Because of quirks in the gnome libsecret API, Chrome needs to store a "
+    "dummy entry to quarantee that this keyring was properly unlocked. More "
+    "details at http://crbug.com/660005.";
+
+// True if we're already ensured that the default keyring has been unlocked
+// once.
+bool s_ensured_keyring_unlocked = false;

[vasilii, 2016/10/28 12:56:06]

Are you thread-safe?
I'm not a big fan of globals. Is EnsureKeyringUnlocked called very often?
Also I'd like to protect against the cases when Keyring is locked not on
start-up but later.

[cfroussios, 2016/10/28 13:50:12]

EnsureDefaultUnlocked() is not necessary if a keyring is re-locked. The regular
calls will trigger the necessary prompt (I fixed that in
https://codereview.chromium.org/2441653002). EnsureDefaultUnlocked() is needed
specifically to address the problem where secret_service_search will ignore a
keyring if it's never opened since rebooting.

Calling EnsureDefaultUnlocked() more than once is redundant. The reason for the
s_ensured_keyring_unlocked is to reduce unnecessary interactions with libsecret.
We can do the same thing from the side of the Password Manager if you think it's
more appropriate.

[vasilii, 2016/10/28 14:17:48]

Given that you call the method only once from KeyStorageLibsecret::Init() but
many from the password manager, I'd move the flag there.

[cfroussios, 2016/10/28 14:36:39]

Done.

+
+}  // namespace
+
 decltype(
     &::secret_password_store_sync) LibsecretLoader::secret_password_store_sync;
 decltype(
     &::secret_service_search_sync) LibsecretLoader::secret_service_search_sync;
 decltype(
     &::secret_password_clear_sync) LibsecretLoader::secret_password_clear_sync;
 decltype(&::secret_item_get_secret) LibsecretLoader::secret_item_get_secret;
 decltype(&::secret_value_get_text) LibsecretLoader::secret_value_get_text;
 decltype(
     &::secret_item_get_attributes) LibsecretLoader::secret_item_get_attributes;
@@ skipping 80 matching lines @@
       &error);
   bool success = (error == nullptr);
   if (error)
     g_error_free(error);
   if (found)
     g_list_free(found);
 
   return success;
 }
 
+// TODO(crbug.com/660005) This is needed to properly unlock the default keyring.
+// We don't need to ever read it.
+void LibsecretLoader::EnsureKeyringUnlocked() {
+  if (s_ensured_keyring_unlocked)
+    return;
+
+  VLOG(1) << "Adding dummy entry to keyring to ensure that it unlocked "
+             "properly.";

[vasilii, 2016/10/28 12:56:06]

Do you need this print on every startup?

[cfroussios, 2016/10/28 14:36:39]

Done.

+
+  const SecretSchema kDummySchema = {
+      "_chrome_dummy_schema_for_unlocking",
+      SECRET_SCHEMA_NONE,
+      {{"explanation", SECRET_SCHEMA_ATTRIBUTE_STRING},
+       {nullptr, SECRET_SCHEMA_ATTRIBUTE_STRING}}};
+
+  GError* error = nullptr;
+  bool success = LibsecretLoader::secret_password_store_sync(
+      &kDummySchema, nullptr /* default keyring */,
+      "Chrome Safe Storage Control" /* entry title */,
+      "The meaning of life" /* password */, nullptr, &error, "explanation",
+      kExplanationMessage,
+      nullptr /* null-terminated variable argument list */);
+  if (error) {
+    VLOG(1) << "Dummy store to unlock the default keyring failed: "
+            << error->message;
+    g_error_free(error);
+  } else if (!success) {
+    VLOG(1) << "Dummy store to unlock the default keyring failed.";
+  } else {
+    s_ensured_keyring_unlocked = true;
+  }
+}
+
 //
 // LibsecretAttributesBuilder
 //
 
 LibsecretAttributesBuilder::LibsecretAttributesBuilder() {
   attrs_ = g_hash_table_new_full(g_str_hash, g_str_equal,
                                  nullptr,   // no deleter for keys
                                  nullptr);  // no deleter for values
 }
 
 LibsecretAttributesBuilder::~LibsecretAttributesBuilder() {
   g_hash_table_destroy(attrs_);
 }
 
 void LibsecretAttributesBuilder::Append(const std::string& name,
                                         const std::string& value) {
   name_values_.push_back(name);
   gpointer name_str =
       static_cast<gpointer>(const_cast<char*>(name_values_.back().c_str()));
   name_values_.push_back(value);
   gpointer value_str =
       static_cast<gpointer>(const_cast<char*>(name_values_.back().c_str()));
   g_hash_table_insert(attrs_, name_str, value_str);
 }
 
 void LibsecretAttributesBuilder::Append(const std::string& name,
                                         int64_t value) {
   Append(name, base::Int64ToString(value));
 }