Implemented profile-aware owner key loading.

Implemented profile-aware owner key loading.

BUG=230018
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271802

[Mattias Nissler (ping if slow), 2014-05-12 08:26:43]

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
profile = content::Source<Profile>(source).ptr();
chrome/browser/chromeos/settings deliberately doesn't know or bother about
Profiles and Users. Note that a lot of code directly or indirectly depends on
DeviceSettingsService, adding a dependency to Profile here essentially creates a
dependency cycle, which I'd like to avoid.

AFAICT, you're adding all this code here in order to retrieve the NSS slot
reference. Can we instead just pass this in, similar as is already done for the
username via SetUsername()?

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/owner_key_util.h (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/owner_key_util.h:42: virtual
crypto::RSAPrivateKey* FindPrivateKey(
I guess this is now deprecated and should be removed? If so, please add a bug +
TODO.

[ygorshenin1, 2014-05-12 08:35:43]

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
profile = content::Source<Profile>(source).ptr();
You mean to directly pass NSS slot instead of username, right?

On 2014/05/12 08:26:43, Mattias Nissler wrote:
> chrome/browser/chromeos/settings deliberately doesn't know or bother about
> Profiles and Users. Note that a lot of code directly or indirectly depends on
> DeviceSettingsService, adding a dependency to Profile here essentially creates
a
> dependency cycle, which I'd like to avoid.
> 
> AFAICT, you're adding all this code here in order to retrieve the NSS slot
> reference. Can we instead just pass this in, similar as is already done for
the
> username via SetUsername()?

[Mattias Nissler (ping if slow), 2014-05-12 08:40:30]

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
profile = content::Source<Profile>(source).ptr();
On 2014/05/12 08:35:43, ygorshenin1 wrote:
> You mean to directly pass NSS slot instead of username, right?
> 
> On 2014/05/12 08:26:43, Mattias Nissler wrote:
> > chrome/browser/chromeos/settings deliberately doesn't know or bother about
> > Profiles and Users. Note that a lot of code directly or indirectly depends
on
> > DeviceSettingsService, adding a dependency to Profile here essentially
creates
> a
> > dependency cycle, which I'd like to avoid.
> > 
> > AFAICT, you're adding all this code here in order to retrieve the NSS slot
> > reference. Can we instead just pass this in, similar as is already done for
> the
> > username via SetUsername()?
> 

Yes, my idea was indeed to pass the NSS slot reference to use. We'll still need
the user name, so we'd probably rename the function to InitOwner (or somesuch)
and then pass both the user name and the NSS slot reference.

An alternative solution could be to break the entire write path out of
DeviceSettingsService, i.e. move out SignAndStore() into a Profile-dependent
service and have that service produce and sign the policy blob itself and then
call Store(). Thinking of it, hadn't we arrived on that solution on some bug
discussion or something?

[ygorshenin1, 2014-05-12 08:56:54]

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
profile = content::Source<Profile>(source).ptr();
On 2014/05/12 08:40:31, Mattias Nissler wrote:
> On 2014/05/12 08:35:43, ygorshenin1 wrote:
> > You mean to directly pass NSS slot instead of username, right?
> > 
> > On 2014/05/12 08:26:43, Mattias Nissler wrote:
> > > chrome/browser/chromeos/settings deliberately doesn't know or bother about
> > > Profiles and Users. Note that a lot of code directly or indirectly depends
> on
> > > DeviceSettingsService, adding a dependency to Profile here essentially
> creates
> > a
> > > dependency cycle, which I'd like to avoid.
> > > 
> > > AFAICT, you're adding all this code here in order to retrieve the NSS slot
> > > reference. Can we instead just pass this in, similar as is already done
for
> > the
> > > username via SetUsername()?
> > 
> 
> Yes, my idea was indeed to pass the NSS slot reference to use. We'll still
need
> the user name, so we'd probably rename the function to InitOwner (or somesuch)
> and then pass both the user name and the NSS slot reference.
> 
> An alternative solution could be to break the entire write path out of
> DeviceSettingsService, i.e. move out SignAndStore() into a Profile-dependent
> service and have that service produce and sign the policy blob itself and then
> call Store(). Thinking of it, hadn't we arrived on that solution on some bug
> discussion or something?

SignAndStore() is implicitly called from DeviceSettingsProvider::DoSet(). So we
need to make all CrosSettingsProviders BCKS. I'd prefer to do that in a separate
CL, as it requires a lot of changes.

[Mattias Nissler (ping if slow), 2014-05-12 09:03:20]

On 2014/05/12 08:56:54, ygorshenin1 wrote:
>
https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
> File chrome/browser/chromeos/settings/device_settings_service.cc (right):
> 
>
https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
> chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
> profile = content::Source<Profile>(source).ptr();
> On 2014/05/12 08:40:31, Mattias Nissler wrote:
> > On 2014/05/12 08:35:43, ygorshenin1 wrote:
> > > You mean to directly pass NSS slot instead of username, right?
> > > 
> > > On 2014/05/12 08:26:43, Mattias Nissler wrote:
> > > > chrome/browser/chromeos/settings deliberately doesn't know or bother
about
> > > > Profiles and Users. Note that a lot of code directly or indirectly
depends
> > on
> > > > DeviceSettingsService, adding a dependency to Profile here essentially
> > creates
> > > a
> > > > dependency cycle, which I'd like to avoid.
> > > > 
> > > > AFAICT, you're adding all this code here in order to retrieve the NSS
slot
> > > > reference. Can we instead just pass this in, similar as is already done
> for
> > > the
> > > > username via SetUsername()?
> > > 
> > 
> > Yes, my idea was indeed to pass the NSS slot reference to use. We'll still
> need
> > the user name, so we'd probably rename the function to InitOwner (or
somesuch)
> > and then pass both the user name and the NSS slot reference.
> > 
> > An alternative solution could be to break the entire write path out of
> > DeviceSettingsService, i.e. move out SignAndStore() into a Profile-dependent
> > service and have that service produce and sign the policy blob itself and
then
> > call Store(). Thinking of it, hadn't we arrived on that solution on some bug
> > discussion or something?
> 
> SignAndStore() is implicitly called from DeviceSettingsProvider::DoSet(). So
we
> need to make all CrosSettingsProviders BCKS. I'd prefer to do that in a
separate
> CL, as it requires a lot of changes.

True. You'll probably be able to make CrosSettings a keyed service entirely
though - lots of code that reads settings is executing in contexts that do not
(and should not) be tied to a Profile. So the write path would ideally have to
move out of CrosSettings/DeviceSettingsProvider as well I guess.

[ygorshenin1, 2014-05-13 09:46:13]

PTAL

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/device_settings_service.cc:296: Profile*
profile = content::Source<Profile>(source).ptr();
All User/Profile-related things are extracted into OwnerKeyReloader.

On 2014/05/12 08:56:54, ygorshenin1 wrote:
> On 2014/05/12 08:40:31, Mattias Nissler wrote:
> > On 2014/05/12 08:35:43, ygorshenin1 wrote:
> > > You mean to directly pass NSS slot instead of username, right?
> > > 
> > > On 2014/05/12 08:26:43, Mattias Nissler wrote:
> > > > chrome/browser/chromeos/settings deliberately doesn't know or bother
about
> > > > Profiles and Users. Note that a lot of code directly or indirectly
depends
> > on
> > > > DeviceSettingsService, adding a dependency to Profile here essentially
> > creates
> > > a
> > > > dependency cycle, which I'd like to avoid.
> > > > 
> > > > AFAICT, you're adding all this code here in order to retrieve the NSS
slot
> > > > reference. Can we instead just pass this in, similar as is already done
> for
> > > the
> > > > username via SetUsername()?
> > > 
> > 
> > Yes, my idea was indeed to pass the NSS slot reference to use. We'll still
> need
> > the user name, so we'd probably rename the function to InitOwner (or
somesuch)
> > and then pass both the user name and the NSS slot reference.
> > 
> > An alternative solution could be to break the entire write path out of
> > DeviceSettingsService, i.e. move out SignAndStore() into a Profile-dependent
> > service and have that service produce and sign the policy blob itself and
then
> > call Store(). Thinking of it, hadn't we arrived on that solution on some bug
> > discussion or something?
> 
> SignAndStore() is implicitly called from DeviceSettingsProvider::DoSet(). So
we
> need to make all CrosSettingsProviders BCKS. I'd prefer to do that in a
separate
> CL, as it requires a lot of changes.

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
File chrome/browser/chromeos/settings/owner_key_util.h (right):

https://codereview.chromium.org/270663002/diff/70001/chrome/browser/chromeos/...
chrome/browser/chromeos/settings/owner_key_util.h:42: virtual
crypto::RSAPrivateKey* FindPrivateKey(
On 2014/05/12 08:26:43, Mattias Nissler wrote:
> I guess this is now deprecated and should be removed? If so, please add a bug
+
> TODO.

Done.

[ygorshenin1, 2014-05-13 10:51:59]

+ Nikita

[Mattias Nissler (ping if slow), 2014-05-13 11:55:53]

This looks much better already. I'm still a bit unclear on whether we're on the
same page for a long-term solution, see my inline comment.

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader.h (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader.h:17: class OwnerKeyReloader :
public content::NotificationObserver {
Instead of tracking login / profile creation explicitly here, why not just make
this a (Profile) KeyedService that starts automatically when we bring up a new
profile and tries to load the owner key then? That'll also move us closer to the
long-term solution where the entire write path moves over to the profile-keyed
helper.

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:238:
slot_.reset(slot.release());
nit: slot_ = slot.Pass();

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:31: Profile*
GetProfileByUsername(const std::string& username) {
No longer needed.

[Nikita (slow), 2014-05-13 15:33:05]

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader.h (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader.h:17: class OwnerKeyReloader :
public content::NotificationObserver {
On 2014/05/13 11:55:53, Mattias Nissler wrote:
> Instead of tracking login / profile creation explicitly here, why not just
make
> this a (Profile) KeyedService that starts automatically when we bring up a new
> profile and tries to load the owner key then? That'll also move us closer to
the
> long-term solution where the entire write path moves over to the profile-keyed
> helper.

In case of PKS it should ignore sign in profile.

[ygorshenin1, 2014-05-14 09:10:57]

PTAL

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader.h (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader.h:17: class OwnerKeyReloader :
public content::NotificationObserver {
On 2014/05/13 11:55:53, Mattias Nissler wrote:
> Instead of tracking login / profile creation explicitly here, why not just
make
> this a (Profile) KeyedService that starts automatically when we bring up a new
> profile and tries to load the owner key then? That'll also move us closer to
the
> long-term solution where the entire write path moves over to the profile-keyed
> helper.

Done.

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:238:
slot_.reset(slot.release());
On 2014/05/13 11:55:53, Mattias Nissler wrote:
> nit: slot_ = slot.Pass();

Done.

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.cc (right):

https://codereview.chromium.org/270663002/diff/130001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.cc:31: Profile*
GetProfileByUsername(const std::string& username) {
On 2014/05/13 11:55:53, Mattias Nissler wrote:
> No longer needed.

Done.

[Mattias Nissler (ping if slow), 2014-05-14 12:18:09]

I like this version much better, as it's moving us in the right direction. I've
put some comments to make sure we are on the same page regarding where we want
to take this, no need to act on all them in this CL.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:22: std::string
OwnerKeyReloaderService::username_;
Static non-POD members are outlawed (unless something has changed that I have
missed).

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:25: :
profile_(profile) {
I usually advice people against passing a Profile in as a dependency, and
instead just pass the dependencies you actually need. In this case that would be
the resource context (and eventually the username once all signing happens
here).

A couple reasons:
* If you hold a Profile pointer, there's a risk that you're introducing
dependencies that are not declared at the factory dependency level, and that can
lead to cyclic dependencies between Profile-keyed services, which I've seen
causing grief at shutdown time (for example when Observers come into play that
need to be destroyed in the correct order).
* With explicit dependencies it's much easier to refactor the code in the future
work outside of a Profile context  (that's probably not a concern here, but you
never know).
* Writing unit tests is much easier because you don't have to mock the world,
just the dependencies you need. Also, the test is guaranteed to not have any
hidden dependencies, which avoids surprises for people changing Profile stuff.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:32: void
OwnerKeyReloaderService::SetUsername(const std::string& username) {
Let's check whether we're on the same page here: I think eventually, we don't
want to maintain a global variable for the user name, but instead have each
instance of this service know the username it belongs to. Similarly, the service
instance for each Profile will try to load the owner key from their NSS DB. The
one that succeeds will be able to write owner settings, all others won't.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:34: if
(!UserManager::IsInitialized())
Is this just for unit tests?

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service.h (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.h:16: 
nit: Class comment would be nice to explain what this is good for.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.h:17: class
OwnerKeyReloaderService : public KeyedService {
I was assuming that this is going to be the service that will host the write
path for device settings eventually (i.e. signing, then passing the signed blob
on the DeviceSettingsService). In that light, maybe we should name this
OwnerSettingsService already now?

Also, it would be great if there was a reference to the bug that has the
background on where we want to go with this (I think I saw one where we
discussed stuff?).

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service_factory.cc
(right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service_factory.cc:37: Profile*
profile = static_cast<Profile*>(context);
This is the point where you'd grab your Profile-keyed dependencies and pass them
on to the ctor below explicitly.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:984: if (profile &&
!ProfileHelper::IsSigninProfile(profile) &&
Why isn't this just handled by the factory or ctor of the service?

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:155:
chromeos::OwnerKeyReloaderServiceFactory::GetInstance()->GetForProfile(
Wouldn't it be simpler to mark the service as auto-created via a
ServiceIsCreatedWithBrowserContext override? AFAIK, it's fine to bail out in the
factory if we determine that the type of profile shouldn't have the service (for
sign-in profile and such).

[ygorshenin1, 2014-05-14 15:30:07]

PTAL

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:22: std::string
OwnerKeyReloaderService::username_;
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> Static non-POD members are outlawed (unless something has changed that I have
> missed).

Done.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:25: :
profile_(profile) {
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> I usually advice people against passing a Profile in as a dependency, and
> instead just pass the dependencies you actually need. In this case that would
be
> the resource context (and eventually the username once all signing happens
> here).
> 
> A couple reasons:
> * If you hold a Profile pointer, there's a risk that you're introducing
> dependencies that are not declared at the factory dependency level, and that
can
> lead to cyclic dependencies between Profile-keyed services, which I've seen
> causing grief at shutdown time (for example when Observers come into play that
> need to be destroyed in the correct order).
> * With explicit dependencies it's much easier to refactor the code in the
future
> work outside of a Profile context  (that's probably not a concern here, but
you
> never know).
> * Writing unit tests is much easier because you don't have to mock the world,
> just the dependencies you need. Also, the test is guaranteed to not have any
> hidden dependencies, which avoids surprises for people changing Profile stuff.

Unfortunately ResourceContext is not ready at the time of BCKS creation, so I
need to pass a Profile instance here.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:32: void
OwnerKeyReloaderService::SetUsername(const std::string& username) {
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> Let's check whether we're on the same page here: I think eventually, we don't
> want to maintain a global variable for the user name, but instead have each
> instance of this service know the username it belongs to. Similarly, the
service
> instance for each Profile will try to load the owner key from their NSS DB.
The
> one that succeeds will be able to write owner settings, all others won't.

Done.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:34: if
(!UserManager::IsInitialized())
Yes, it's just for unit tests.

On 2014/05/14 12:18:10, Mattias Nissler wrote:
> Is this just for unit tests?

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service.h (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.h:16: 
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> nit: Class comment would be nice to explain what this is good for.

Done.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.h:17: class
OwnerKeyReloaderService : public KeyedService {
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> I was assuming that this is going to be the service that will host the write
> path for device settings eventually (i.e. signing, then passing the signed
blob
> on the DeviceSettingsService). In that light, maybe we should name this
> OwnerSettingsService already now?
> 
> Also, it would be great if there was a reference to the bug that has the
> background on where we want to go with this (I think I saw one where we
> discussed stuff?).

Done.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service_factory.cc
(right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service_factory.cc:37: Profile*
profile = static_cast<Profile*>(context);
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> This is the point where you'd grab your Profile-keyed dependencies and pass
them
> on to the ctor below explicitly.

As I mentioned in the comment to the OwnerSettingsService ctor, ResourceContext
is not ready at the time of BuildInstanceFor() call.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:984: if (profile &&
!ProfileHelper::IsSigninProfile(profile) &&
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> Why isn't this just handled by the factory or ctor of the service?

Done.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/profiles/profile_helper.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/profiles/profile_helper.cc:155:
chromeos::OwnerKeyReloaderServiceFactory::GetInstance()->GetForProfile(
On 2014/05/14 12:18:10, Mattias Nissler wrote:
> Wouldn't it be simpler to mark the service as auto-created via a
> ServiceIsCreatedWithBrowserContext override? AFAIK, it's fine to bail out in
the
> factory if we determine that the type of profile shouldn't have the service
(for
> sign-in profile and such).

Done.

[Mattias Nissler (ping if slow), 2014-05-14 16:00:36]

This looks pretty good now.

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_key_reloader_service.cc (right):

https://codereview.chromium.org/270663002/diff/190001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_key_reloader_service.cc:25: :
profile_(profile) {
On 2014/05/14 15:30:07, ygorshenin1 wrote:
> On 2014/05/14 12:18:10, Mattias Nissler wrote:
> > I usually advice people against passing a Profile in as a dependency, and
> > instead just pass the dependencies you actually need. In this case that
would
> be
> > the resource context (and eventually the username once all signing happens
> > here).
> > 
> > A couple reasons:
> > * If you hold a Profile pointer, there's a risk that you're introducing
> > dependencies that are not declared at the factory dependency level, and that
> can
> > lead to cyclic dependencies between Profile-keyed services, which I've seen
> > causing grief at shutdown time (for example when Observers come into play
that
> > need to be destroyed in the correct order).
> > * With explicit dependencies it's much easier to refactor the code in the
> future
> > work outside of a Profile context  (that's probably not a concern here, but
> you
> > never know).
> > * Writing unit tests is much easier because you don't have to mock the
world,
> > just the dependencies you need. Also, the test is guaranteed to not have any
> > hidden dependencies, which avoids surprises for people changing Profile
stuff.
> 
> Unfortunately ResourceContext is not ready at the time of BCKS creation, so I
> need to pass a Profile instance here.

Hm, too bad :-/ Let's go with a Profile pointer then.

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:31: //
OnwerSettinsService construction ProfileIOData is not initialized.
fix spelling.

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:41: void
OwnerSettingsService::ReloadOwnerKey() {
I don't see how you make sure this is only getting called for the actual device
owner's Profile?

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service_factory.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.cc:40: return new
OwnerSettingsBaseService();
What happens if you just return NULL here?

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:237: if
(owner_key_.get() && owner_key_->public_key() && owner_key_->private_key())
Ah, here you test whether you already have a loaded the key successfully, and
IIUC that's supposed to make sure we only pick up the actual device owner?

If so, you still have a race condition here: If two Profiles call InitOwner() at
the same time, then you'll have the second one reset username_ below even if the
first one is the legit owner because the owner key reload only happens
asynchronously.

[ygorshenin1, 2014-05-15 08:39:55]

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:31: //
OnwerSettinsService construction ProfileIOData is not initialized.
On 2014/05/14 16:00:36, Mattias Nissler wrote:
> fix spelling.

Done.

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:41: void
OwnerSettingsService::ReloadOwnerKey() {
I don't know how to do that without moving OwnerKeyUtil to OwnerSettingsService.
I thought it's ok for now to restore (Set|Get)Username() API.

On 2014/05/14 16:00:36, Mattias Nissler wrote:
> I don't see how you make sure this is only getting called for the actual
device
> owner's Profile?

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service_factory.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.cc:40: return new
OwnerSettingsBaseService();
On 2014/05/14 16:00:36, Mattias Nissler wrote:
> What happens if you just return NULL here?

Done.

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/210001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:237: if
(owner_key_.get() && owner_key_->public_key() && owner_key_->private_key())
I propose to use old SetUsername() approach, but to move it to
OwnerSettingsServiceFactory.

On 2014/05/14 16:00:36, Mattias Nissler wrote:
> Ah, here you test whether you already have a loaded the key successfully, and
> IIUC that's supposed to make sure we only pick up the actual device owner?
> 
> If so, you still have a race condition here: If two Profiles call InitOwner()
at
> the same time, then you'll have the second one reset username_ below even if
the
> first one is the legit owner because the owner key reload only happens
> asynchronously.

[ygorshenin1, 2014-05-15 10:17:04]

PTAL

[Mattias Nissler (ping if slow), 2014-05-15 11:21:28]

LGTM w/ nits.

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1564: // Owner must be first
user in session. DeviceSettingsService can't deal with
Update comment?

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:241: return;
I think we should replaces these two checks with this:

if (!username_.empty())
  return;

This will make sure that the user only gets set once for now.

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.h (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.h:14: #include
"crypto/scoped_nss_types.h"
Shouldn't this also be x509_util_nss.h?

[ygorshenin1, 2014-05-15 11:58:36]

Many thanks, Mattias!

+ mmenke@ for chrome/browser/profiles/*
+ wtc@ for crypto/*

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1564: // Owner must be first
user in session. DeviceSettingsService can't deal with
This is still true --- we can't remove this restriction right now as
SignAndStore() path is not moved to OwnerSettingsService yet.

On 2014/05/15 11:21:29, Mattias Nissler wrote:
> Update comment?

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/device_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/device_settings_service.cc:241: return;
On 2014/05/15 11:21:29, Mattias Nissler wrote:
> I think we should replaces these two checks with this:
> 
> if (!username_.empty())
>   return;
> 
> This will make sure that the user only gets set once for now.

Done.

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
File chrome/browser/chromeos/settings/session_manager_operation.h (right):

https://codereview.chromium.org/270663002/diff/250001/chrome/browser/chromeos...
chrome/browser/chromeos/settings/session_manager_operation.h:14: #include
"crypto/scoped_nss_types.h"
On 2014/05/15 11:21:29, Mattias Nissler wrote:
> Shouldn't this also be x509_util_nss.h?

Done.

[mmenke, 2014-05-15 12:22:07]

I'm only an owner of the profile code that lives on the IO thread.

[ygorshenin1, 2014-05-15 12:30:23]

Sorry, Matt.

+ davemoore@ for
chrome/browser/profiles/chrome_browser_main_extra_parts_profiles.cc

[ygorshenin1, 2014-05-15 12:31:23]

+ wtc@ for crypto/*
- mmenke@

[Nikita (slow), 2014-05-15 12:48:57]

chromeos/login lgtm

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:26: // ReloadOwnerKey()
is called after delay because at the time of
It seems that this is not actually a delay but just placing this task to the end
of message loop.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:43: std::string username
= user->email();
nit: user_id

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:56:
crypto::ScopedPK11Slot slot;
nit: Can you please add comment describing the situation when resource context
is not available.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.h (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.h:16: // This class reloads
owner key from profile slots.
nit: from profile NSS slots?

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.h:18: // TODO
(ygorshenin@): move write path for device settings here
nit: If this is planned as a refactoring, I suggest filing a separate issue.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service_factory.h (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:28: // Sets name
of the user supposed to be owner. If profile for |username|
nit: an owner

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:33: // Returns
the name of the user supposed to be owner.
nit: an owner

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:51: // Name of
the user supposed to be owner.
nit: an owner

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1566: if
(GetLoggedInUsers().size() == 1) {
When do you plan to remove this restriction?
Can it be removed in this CL?

Can the owner be added as a secondary user after this CL?

[Nikita (slow), 2014-05-15 12:50:54]

I'm splitting chromeos/login code right now
(https://codereview.chromium.org/286933002/), can you please move new files to
something like
chromeos/login/auth?

[Nikita (slow), 2014-05-15 13:26:02]

On 2014/05/15 12:50:54, Nikita Kostylev wrote:
> I'm splitting chromeos/login code right now
> (https://codereview.chromium.org/286933002/), can you please move new files to
> something like
> chromeos/login/auth?

As discussed, another suggestion: chrome/browser/chromeos/ownership

[ygorshenin1, 2014-05-16 12:09:36]

PTAL

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.cc (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:26: // ReloadOwnerKey()
is called after delay because at the time of
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> It seems that this is not actually a delay but just placing this task to the
end
> of message loop.

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:43: std::string username
= user->email();
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: user_id

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.cc:56:
crypto::ScopedPK11Slot slot;
There shouldn't be such situations, so I've added NOTREACHED() instead of the
second branch.

On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: Can you please add comment describing the situation when resource context
> is not available.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service.h (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.h:16: // This class reloads
owner key from profile slots.
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: from profile NSS slots?

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service.h:18: // TODO
(ygorshenin@): move write path for device settings here
It's not a refactoring --- it's a main task for crbug.com/230018 to move write
path here.

On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: If this is planned as a refactoring, I suggest filing a separate issue.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/owner_settings_service_factory.h (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:28: // Sets name
of the user supposed to be owner. If profile for |username|
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: an owner

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:33: // Returns
the name of the user supposed to be owner.
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: an owner

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/owner_settings_service_factory.h:51: // Name of
the user supposed to be owner.
On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> nit: an owner

Done.

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/user_manager_impl.cc (right):

https://codereview.chromium.org/270663002/diff/270001/chrome/browser/chromeos...
chrome/browser/chromeos/login/user_manager_impl.cc:1566: if
(GetLoggedInUsers().size() == 1) {
No, owner still can't be added as a secondary user after this CL, because write
path of device settings wasn't made BCKS yet.

On 2014/05/15 12:48:58, Nikita Kostylev wrote:
> When do you plan to remove this restriction?
> Can it be removed in this CL?
> 
> Can the owner be added as a secondary user after this CL?

[wtc, 2014-05-16 15:06:35]

Patch set 15: src/crypto LGTM.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key....
crypto/rsa_private_key.h:211: // half in a |slot|. The format of the public key
blob is is

1. Nit: either just "in |slot|", or "in the slot specified by |slot|".

2. Nit: is is => is. Also fix line 202.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key....
crypto/rsa_private_key.h:215: // created in the key database.

Nit: the key database => |slot|

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (left):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:139: // Make sure the key is an RSA key.  If not,
that's an error

Please copy this comment to the new GetRSAPublicKey function.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:42: SECKEYPublicKey* GetRSAPublicKey(const
std::vector<uint8>& input) {

Nit: please document this function, especially the ownership of the return
value.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:55: if (!result || result->keyType != rsaKey)

BUG: in the result->keyType != rsaKey case, we need to destroy |result| (with
SECKEY_DestroyPublicKey) before returning NULL.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:190: }

Nit: we can work harder and share more code between these two functions. The
code up to this line (except for the null pointer check for |slot|) is the same
between the two functions.

[ygorshenin1, 2014-05-19 09:43:49]

Many thanks, wtc@.

Dave, could you please take a look?

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key....
crypto/rsa_private_key.h:211: // half in a |slot|. The format of the public key
blob is is
On 2014/05/16 15:06:36, wtc wrote:
> 
> 1. Nit: either just "in |slot|", or "in the slot specified by |slot|".
> 
> 2. Nit: is is => is. Also fix line 202.

Done.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key....
crypto/rsa_private_key.h:215: // created in the key database.
On 2014/05/16 15:06:36, wtc wrote:
> 
> Nit: the key database => |slot|

Done.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (left):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:139: // Make sure the key is an RSA key.  If not,
that's an error
On 2014/05/16 15:06:36, wtc wrote:
> 
> Please copy this comment to the new GetRSAPublicKey function.

Done.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:42: SECKEYPublicKey* GetRSAPublicKey(const
std::vector<uint8>& input) {
On 2014/05/16 15:06:36, wtc wrote:
> 
> Nit: please document this function, especially the ownership of the return
> value.

Done.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:55: if (!result || result->keyType != rsaKey)
On 2014/05/16 15:06:36, wtc wrote:
> 
> BUG: in the result->keyType != rsaKey case, we need to destroy |result| (with
> SECKEY_DestroyPublicKey) before returning NULL.

Done.

https://codereview.chromium.org/270663002/diff/290001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:190: }
On 2014/05/16 15:06:36, wtc wrote:
> 
> Nit: we can work harder and share more code between these two functions. The
> code up to this line (except for the null pointer check for |slot|) is the
same
> between the two functions.

Done.

[wtc, 2014-05-19 17:23:53]

Patch set 17: src/crypto LGTM.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:215: // is created in the key slot.

Nit: I think it's better to say
  in |slot|
or
  in the slot

The term "key slot" is not commonly used. (NSS does use the term "the internal
key slot" to refer to the slot that represents the key database.)

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:269: // is is an X509 SubjectPublicKeyInfo block. This
can return NULL if

Nit: is is => is

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:271: // object, but nothing new is created in the key
slot.

Nit: since this function doesn't have a |slot| parameter, I think we should
change "in the key slot" to "in any slot", or just remove ", but nothing new is
created in the key slot".

It is important to note that this method doesn't initialize the |key_| member.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:52: // takes ownership of the returned value.

Nit: I think it is OK to not document the return value now that you switched to
returning a scoped type. Up to you.

[ygorshenin1, 2014-05-20 07:53:49]

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:215: // is created in the key slot.
On 2014/05/19 17:23:54, wtc wrote:
> 
> Nit: I think it's better to say
>   in |slot|
> or
>   in the slot
> 
> The term "key slot" is not commonly used. (NSS does use the term "the internal
> key slot" to refer to the slot that represents the key database.)

Done.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:269: // is is an X509 SubjectPublicKeyInfo block. This
can return NULL if
On 2014/05/19 17:23:54, wtc wrote:
> 
> Nit: is is => is

Done.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key....
crypto/rsa_private_key.h:271: // object, but nothing new is created in the key
slot.
On 2014/05/19 17:23:54, wtc wrote:
> 
> Nit: since this function doesn't have a |slot| parameter, I think we should
> change "in the key slot" to "in any slot", or just remove ", but nothing new
is
> created in the key slot".
> 
> It is important to note that this method doesn't initialize the |key_| member.

Done.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:52: // takes ownership of the returned value.
On 2014/05/19 17:23:54, wtc wrote:
> 
> Nit: I think it is OK to not document the return value now that you switched
to
> returning a scoped type. Up to you.

Done.

[DaveMoore, 2014-05-20 16:25:18]

User lgtm

[wtc, 2014-05-20 16:46:38]

Patch set 18: src/crypto LGTM. Thanks.

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:52: // takes ownership of the returned value.

Nit: I think we can keep the "The function decodes RSA public key from the
|input|." part of the comment.

https://codereview.chromium.org/270663002/diff/350001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/270663002/diff/350001/crypto/rsa_private_key....
crypto/rsa_private_key.h:271: // object. Note, that this method doesn't
initialize the |key_| member.

Nit: the comma after "Note" is not necessary.

[ygorshenin1, 2014-05-20 17:01:33]

Many thanks to everybody!

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
File crypto/rsa_private_key_nss.cc (right):

https://codereview.chromium.org/270663002/diff/330001/crypto/rsa_private_key_...
crypto/rsa_private_key_nss.cc:52: // takes ownership of the returned value.
On 2014/05/20 16:46:39, wtc wrote:
> 
> Nit: I think we can keep the "The function decodes RSA public key from the
> |input|." part of the comment.

Done.

[ygorshenin1, 2014-05-20 17:03:14]

The CQ bit was checked by ygorshenin@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:03:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ygorshenin@chromium.org/270663002/360001

[commit-bot: I haz the power, 2014-05-21 01:50:28]

Change committed as 271802