Reland "Don't use RSAPrivateKey in NSS integration code."

chrome/browser/chromeos/ownership/owner_settings_service_chromeos.h

 // Copyright 2014 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 CHROME_BROWSER_CHROMEOS_OWNERSHIP_OWNER_SETTINGS_SERVICE_CHROMEOS_H_
 #define CHROME_BROWSER_CHROMEOS_OWNERSHIP_OWNER_SETTINGS_SERVICE_CHROMEOS_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 117 matching lines @@
 
   // Perform fixups required to ensure sensical local-owner device policy:
   //  1) user whitelisting must be explicitly allowed or disallowed, and
   //  2) the owner user must be on the whitelist, if it's enforced.
   static void FixupLocalOwnerPolicy(
       const std::string& user_id,
       enterprise_management::ChromeDeviceSettingsProto* settings);
 
   // OwnerSettingsService protected interface overrides:
 
-  // Reloads private key from profile's NSS slots, responds via |callback|.
+  // Reloads private key from profile's NSS slots, responds via |callback|. On
+  // success, |private_key| is non-null, but if the private key doesn't exist,
+  // |private_key->key()| may be null.

[davidben, 2015/05/06 18:48:39]

This is really weird and, tbh, I don't think it makes much sense. But
OwnerSettingsService::IsOwnerAsync actually assumes this with the private_key_
check. I'm somewhat puzzled by that code but have left it alone for now. It
seems a boolean keypair_loaded_ would make much more sense, but I didn't want to
make this CL grow in scope that much.

Plus there's failure paths where ReloadKeypairImpl gives back a null
scoped_refptr<PrivateKey> so that gets more confusing...

dpolukhin: I imagine this should get a TODO of some sort. What would you like me
to write in there.

[Dmitry Polukhin, 2015/05/07 10:59:16]

NULL private key means that active user is not an owner. But public key should
be available. I'm not an original author of the code.

[davidben, 2015/05/07 20:51:44]

Right. But the question is whether this is signaled by |private_key| being null
or |private_key->key()| being null. Right now it's the latter. I think it's
overly confusing that you can have an existent PrivateKey with non-existent
RSAPrivateKey.

   void ReloadKeypairImpl(const base::Callback<
       void(const scoped_refptr<ownership::PublicKey>& public_key,
            const scoped_refptr<ownership::PrivateKey>& private_key)>& callback)
       override;
 
   // Possibly notifies DeviceSettingsService that owner's keypair is loaded.
   void OnPostKeypairLoadedActions() override;
 
   // Tries to apply recent changes to device settings proto, sign it and store.
   void StorePendingChanges();
@@ skipping 51 matching lines @@
   base::WeakPtrFactory<OwnerSettingsServiceChromeOS> weak_factory_;
 
   base::WeakPtrFactory<OwnerSettingsServiceChromeOS> store_settings_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(OwnerSettingsServiceChromeOS);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROME_BROWSER_CHROMEOS_OWNERSHIP_OWNER_SETTINGS_SERVICE_CHROMEOS_H_