OSCrypt supports encryption with KWallet

components/os_crypt/key_storage_kwallet.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/key_storage_kwallet.h"
+
+#include <utility>
+
+#include "base/base64.h"
+#include "base/rand_util.h"
+#include "components/os_crypt/kwallet_dbus.h"
+#include "dbus/bus.h"
+
+namespace {
+
+#if defined(OFFICIAL_BUILD)
+const char kFolderName[] = "Chrome Keys";

[Lei Zhang, 2016/07/20 01:08:26]

Any chance we can share these with the libsecret impl?

[cfroussios, 2016/07/20 15:22:46]

I moved them to KeyStorageLinux, mostly because these are also visible to the
user and should be consistent. Conceptually, reusing those doesn't mean much,
since KWallet and Libsecret have diffent structures. Libsecret doesn't even have
folders and doesn't rely on the name for lookup.

+const char kKey[] = "Chrome Safe Storage";
+#else
+const char kFolderName[] = "Chromium Keys";
+const char kKey[] = "Chromium Safe Storage";
+#endif
+
+}  // namespace
+
+KeyStorageKWallet::KeyStorageKWallet(base::nix::DesktopEnvironment desktop_env,
+                                     std::string app_name)
+    : desktop_env_(desktop_env), handle_(0), app_name_(std::move(app_name)) {}

[Lei Zhang, 2016/07/20 01:08:26]

Should |handle_| be initialized to -1? Is 0 a valid value?

[cfroussios, 2016/07/20 15:22:46]

Both are invalid values, but let's just use -1, since that is what we check for
in line 99.

+
+KeyStorageKWallet::~KeyStorageKWallet() {
+  // The handle is shared between programs that are using the same wallet.
+  // Closing the wallet is a nop in the typical case.
+  bool success = true;
+  ignore_result(kwallet_dbus_->Close(handle_, false, app_name_, &success));
+  kwallet_dbus_->GetSessionBus()->ShutdownAndBlock();
+}
+
+bool KeyStorageKWallet::Init() {
+  // Initialize using the production KWalletDBus.
+  return InitWithKWalletDBus(nullptr);
+}
+
+bool KeyStorageKWallet::InitWithKWalletDBus(
+    std::unique_ptr<KWalletDBus> optional_kwallet_dbus_ptr) {
+  if (optional_kwallet_dbus_ptr) {
+    kwallet_dbus_ = std::move(optional_kwallet_dbus_ptr);
+  } else {
+    // Initializing with production KWalletDBus
+    kwallet_dbus_.reset(new KWalletDBus(desktop_env_));
+    dbus::Bus::Options options;
+    options.bus_type = dbus::Bus::SESSION;
+    options.connection_type = dbus::Bus::PRIVATE;
+    kwallet_dbus_->SetSessionBus(new dbus::Bus(options));
+  }
+
+  // If KWallet might not have started, attempt to start it and retry.
+  InitResult result = InitWallet();
+  if (result == InitResult::TEMPORARY_FAIL)

[Lei Zhang, 2016/07/20 01:08:26]

if (result == InitResult::TEMPORARY_FAIL && kwallet_dbus_->StartKWalletd())
  result = InitWallet();

[cfroussios, 2016/07/20 15:22:46]

I am not a big fan of conditions where the order of execution matters. I tend to
not pay attention, and I am not the only one. In this case, StartKWalletd is
costly (response times vary, up to several seconds) and we explicitly avoid
calling it when unnecessary.

[Lei Zhang, 2016/07/20 19:15:07]

But ordering of execution in if statements do matter and the left to right eval
ordering is a fact of life you have to live with. Writing it as a single if
statement doesn't change the behavior.

As an exaggeration, what if we had 4 conditions to check? Which should we use?

if (foo)
  if (bar)
    if (qux)
      if (baz)
        DoIt();

or

if (foo && bar && qux && baz)
  DoIt();

[cfroussios, 2016/07/21 11:49:45]

There might have been a miscommunication. I didn't say the behavior changes. My
objection is about readability.

Logical operators draw attention to the logic of the condition, whereas ifs draw
attention to the flow of execution. Every time I've seen code in Chrome where
right eval matters, they've also added a comment to note that. That's because
one might reorder a condition without thinking much about it. But why do it in a
way that requires a note, when you can have code that speaks for itself? People
don't invert nested blocks expecting things to be the same.

Aesthetically, in your example, too many nested ifs waste screen space. However,
in my specific case, the result doesn't suffer much. I don't think there needs
to be a one-size-fits-all rule here, such that we need to discuss which rule
gets worse in its extreme.

[Lei Zhang, 2016/07/21 21:07:39]

My objection is also about readability, but I just wanted to point out ordering
does matter for completeness. I wasn't suggesting we change the order.
Can you provide some examples to back up your claim that everyone adds comments
when the ordering matters? To counter your claim, look at
https://cs.chromium.org/search/?q=GetAsDictionary%5C(.*%26%26&sq=package:chro...
and see how many places first make sure base::Value::GetAsDictionary() succeeds
before using the pointer passed in to it. Nobody writes a comment to explain the
ordering.

People don't randomly reorder the insides of conditionals either. The only time
I've seen people bother to do so is when you have:

if (ExpensiveConstFunc() || completely_unrelated_bool)

in which case, reordering makes sense because |completely_unrelated_bool| is
much faster to evaluate and it can evaluate to false faster than
ExpensiveConstFunc(), where ExpensiveConstFunc() doesn't have any side effects.
There's no one side fit all rule and my objection is not a strong one. In my
experience, oftentimes nested conditionals that are not strictly required can
make code harder to read, especially when the nesting goes too deep. That is why
I suggested combining the two if statements, especially considering they fit on
one line.

[cfroussios, 2016/07/22 11:25:25]

"Every time I've seen code in Chrome" is not comparable to "everyone" in my
case, I'm afraid.

The easiest example is the code that this came from
https://cs.chromium.org/chromium/src/chrome/browser/password_manager/native_b...

I've seen it once or twice more, but I don't quickly recall how to search for
it.

My experience with readability comes from outside of Chrome, so if you say that
this is normal in Chrome, let's go with your experience.

+    if (kwallet_dbus_->StartKWalletd())
+      result = InitWallet();
+
+  return result == InitResult::SUCCESS;
+}
+
+KeyStorageKWallet::InitResult KeyStorageKWallet::InitWallet() {
+  // Check that KWallet is enabled.
+  bool enabled = false;
+  KWalletDBus::Error error = kwallet_dbus_->IsEnabled(&enabled);
+  switch (error) {
+    case KWalletDBus::Error::CANNOT_CONTACT:
+      return InitResult::TEMPORARY_FAIL;
+    case KWalletDBus::Error::CANNOT_READ:
+      return InitResult::PERMANENT_FAIL;
+    case KWalletDBus::Error::SUCCESS:
+      break;
+  }
+  if (!enabled)
+    return InitResult::PERMANENT_FAIL;
+
+  // Get the wallet name.
+  error = kwallet_dbus_->NetworkWallet(&wallet_name_);
+  switch (error) {
+    case KWalletDBus::Error::CANNOT_CONTACT:
+      return InitResult::TEMPORARY_FAIL;
+    case KWalletDBus::Error::CANNOT_READ:
+      return InitResult::PERMANENT_FAIL;
+    case KWalletDBus::Error::SUCCESS:
+      return InitResult::SUCCESS;
+  }
+
+  NOTREACHED();
+  return InitResult::PERMANENT_FAIL;
+}
+
+std::string KeyStorageKWallet::GetKey() {
+  // Get handle
+  KWalletDBus::Error error =
+      kwallet_dbus_->Open(wallet_name_, app_name_, &handle_);
+  if (error || handle_ == -1)
+    return std::string();
+
+  // Create folder
+  if (!InitFolder())
+    return std::string();
+
+  // Read password
+  std::string password;
+  error = kwallet_dbus_->ReadPassword(handle_, kFolderName, kKey, app_name_,
+                                      &password);
+  if (error)
+    return std::string();
+
+  // If there is no entry, generate and write a new password.
+  if (password.empty()) {
+    base::Base64Encode(base::RandBytesAsString(16), &password);
+    bool success;
+    error = kwallet_dbus_->WritePassword(handle_, kFolderName, kKey, password,
+                                         app_name_, &success);
+    if (error || !success)
+      return std::string();
+  }
+
+  return password;
+}
+
+bool KeyStorageKWallet::InitFolder() {
+  bool has_folder = false;
+  KWalletDBus::Error error =
+      kwallet_dbus_->HasFolder(handle_, kFolderName, app_name_, &has_folder);
+  if (error)
+    return false;
+
+  if (!has_folder) {
+    bool success = false;
+    error =
+        kwallet_dbus_->CreateFolder(handle_, kFolderName, app_name_, &success);
+    if (error || !success)
+      return false;
+  }
+
+  return true;
+}