Don't use RSAPrivateKey in NSS integration code.

Don't use RSAPrivateKey in NSS integration code.

Currently some NSS platform integration logic transits private keys through
RSAPrivateKey on CrOS. This prevents incrementally switching RSAPrivateKey to
BoringSSL while keeping platform integrations on NSS.

The intent of this change is to clarify RSAPrivateKey as a BoringSSL vs NSS
internal crypto library (use_openssl=0 vs use_openssl=1) abstraction. It's
primarily to be used with SignatureCreator. Code which uses NSS based on
use_nss_certs rather than use_openssl because the underlying platform is NSS
should call NSS routines directly, or introduce different abstractions.

Remove the problematic RSAPrivateKey methods and instead add
crypto/nss_key_util.h which contains some helper functions for manipulating NSS
keys. This is sufficient to allow consumers of the removed methods to use NSS
directly with about as much code. (This should not set back migrating that
logic to NSS as that code was already very NSS-specific; those APIs assumed
PK11SlotInfo.)

nss_key_util.h, like nss_util.h, is built whenever NSS is used either
internally or for platform integrations. This is so rsa_private_key_nss.cc can
continue to use the helper functions to implement the NSS-agnostic interface.

With this, the chimera CrOS configuration should build. The RSAPrivateKey logic
is functional with the exception of some logic in components/ownership. That
will be resolved in a future CL.

BUG=478777
Committed: https://crrev.com/a46a990b2ccae2b66e87b5f76d2866044dc3182e
Cr-Commit-Position: refs/heads/master@{#327909}

[davidben, 2015-04-27 18:07:22]

davidben@chromium.org changed reviewers:
+ pneubeck@chromium.org, rsleevi@chromium.org

[davidben, 2015-04-27 18:07:22]

rsleevi and pneubeck for primary reviewers, since you're the people I associate
with crypto and CrOS crypto, respectively.

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
File components/ownership/owner_key_util_impl.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
components/ownership/owner_key_util_impl.cc:70: return nullptr;
Leaving this for a follow-up since I'm not sure yet what components/ownership's
NSS dependency is intended to be. Given that this function is USE_NSS_CERTS and
depends on PK11SlotInfo, naively I would think we should replace
crypto::RSAPrivateKey with crypto::ScopedSECKEYPrivateKey. But, unlike the other
places, this key is used with SignatureCreator rather than raw NSS APIs. And the
tests pull a key out of corp policy. (Perhaps that should just transit through a
PrivateKeyInfo and we replace the SignatureCreator call with NSS APIs directly?)

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util.h
File net/test/cert_test_util.h (right):

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util...
net/test/cert_test_util.h:35: bool ImportSensitiveKeyFromFile(const
base::FilePath& dir,
Nothing ever uses this output except as a boolean. I could make it return a
crypto::ScopedSECKEYPrivateKey, but then net needs to add NSS to
export_dependent_settings/public_deps which seemed unnecessary here.

[Ryan Sleevi, 2015-04-27 19:11:27]

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:127:
crypto::ScopedSECKEYPrivateKey private_key;
Why the different naming? public_key_obj vs private_key (no _obj); seems like
there should be consistency (and yes, I'm aware it's because public_key is used
on 117)

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:137:
crypto::ScopedSECItem created_public_key(
seems like this should be named like public_key_der ?

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:414:
crypto::ScopedSECItem spki_item(
public_key_der?

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
File components/ownership/owner_key_util_impl.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
components/ownership/owner_key_util_impl.cc:70: return nullptr;
On 2015/04/27 18:07:22, David Benjamin wrote:
> Leaving this for a follow-up since I'm not sure yet what
components/ownership's
> NSS dependency is intended to be. Given that this function is USE_NSS_CERTS
and
> depends on PK11SlotInfo, naively I would think we should replace
> crypto::RSAPrivateKey with crypto::ScopedSECKEYPrivateKey. But, unlike the
other
> places, this key is used with SignatureCreator rather than raw NSS APIs. And
the
> tests pull a key out of corp policy. (Perhaps that should just transit through
a
> PrivateKeyInfo and we replace the SignatureCreator call with NSS APIs
directly?)

That seems less-desirable; I mean, the team is using SignatureCreator to avoid
needing to reimplement NSS-based signing, whether you're dealing with software
keys (e.g. Extensions) or hardware keys (such as here).

In a BoringSSL world, this would need to coordinate with chapsd and friends,
which further suggests we should try to figure a decent abstraction here so that
//components/ownership isn't reimplementing crypto primitives.

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:35: // the CKA_ID of that public key or null on error.
s/null/nullptr/ (or NULL)

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:158: if (!cka_id.get()) {
s/.get()//

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h
File crypto/nss_key_util.h (right):

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h#n...
crypto/nss_key_util.h:43: // or null on error.
s/null/nullptr/ (or NULL)

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h#n...
crypto/nss_key_util.h:49: // on success or null on error.
ditto

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util.h
File net/test/cert_test_util.h (right):

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util...
net/test/cert_test_util.h:35: bool ImportSensitiveKeyFromFile(const
base::FilePath& dir,
On 2015/04/27 18:07:22, David Benjamin wrote:
> Nothing ever uses this output except as a boolean. I could make it return a
> crypto::ScopedSECKEYPrivateKey, but then net needs to add NSS to
> export_dependent_settings/public_deps which seemed unnecessary here.

I don't follow this.

[davidben, 2015-04-27 19:17:10]

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
File components/ownership/owner_key_util_impl.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/components/ownership/ow...
components/ownership/owner_key_util_impl.cc:70: return nullptr;
On 2015/04/27 19:11:27, Ryan Sleevi wrote:
> On 2015/04/27 18:07:22, David Benjamin wrote:
> > Leaving this for a follow-up since I'm not sure yet what
> components/ownership's
> > NSS dependency is intended to be. Given that this function is USE_NSS_CERTS
> and
> > depends on PK11SlotInfo, naively I would think we should replace
> > crypto::RSAPrivateKey with crypto::ScopedSECKEYPrivateKey. But, unlike the
> other
> > places, this key is used with SignatureCreator rather than raw NSS APIs. And
> the
> > tests pull a key out of corp policy. (Perhaps that should just transit
through
> a
> > PrivateKeyInfo and we replace the SignatureCreator call with NSS APIs
> directly?)
> 
> That seems less-desirable; I mean, the team is using SignatureCreator to avoid
> needing to reimplement NSS-based signing, whether you're dealing with software
> keys (e.g. Extensions) or hardware keys (such as here).
> 
> In a BoringSSL world, this would need to coordinate with chapsd and friends,
> which further suggests we should try to figure a decent abstraction here so
that
> //components/ownership isn't reimplementing crypto primitives.

Yeah, that's also an option, which is why I set this aside for now. One
possibility is reusing SECKEYPrivateKey -> EVP_PKEY bridge that client auth will
need (async vs sync aside) and route through there.

Though note that this API is already not suitable for a NSS/CHAPS abstraction.
It depends on PK11SlotInfo.

[davidben, 2015-04-27 19:53:44]

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:127:
crypto::ScopedSECKEYPrivateKey private_key;
On 2015/04/27 19:11:26, Ryan Sleevi wrote:
> Why the different naming? public_key_obj vs private_key (no _obj); seems like
> there should be consistency (and yes, I'm aware it's because public_key is
used
> on 117)

Done.

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:137:
crypto::ScopedSECItem created_public_key(
On 2015/04/27 19:11:26, Ryan Sleevi wrote:
> seems like this should be named like public_key_der ?

Done.

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/chrome/browser/chromeos...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:414:
crypto::ScopedSECItem spki_item(
On 2015/04/27 19:11:26, Ryan Sleevi wrote:
> public_key_der?

Done.

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:35: // the CKA_ID of that public key or null on error.
On 2015/04/27 19:11:27, Ryan Sleevi wrote:
> s/null/nullptr/ (or NULL)

Done.

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:158: if (!cka_id.get()) {
On 2015/04/27 19:11:27, Ryan Sleevi wrote:
> s/.get()//

Done.

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h
File crypto/nss_key_util.h (right):

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h#n...
crypto/nss_key_util.h:43: // or null on error.
On 2015/04/27 19:11:27, Ryan Sleevi wrote:
> s/null/nullptr/ (or NULL)

Done.

https://codereview.chromium.org/1106103003/diff/60001/crypto/nss_key_util.h#n...
crypto/nss_key_util.h:49: // on success or null on error.
On 2015/04/27 19:11:27, Ryan Sleevi wrote:
> ditto

Done.

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util.h
File net/test/cert_test_util.h (right):

https://codereview.chromium.org/1106103003/diff/60001/net/test/cert_test_util...
net/test/cert_test_util.h:35: bool ImportSensitiveKeyFromFile(const
base::FilePath& dir,
The only caller of this function just casts the output to a boolean and cares
about the side effect on |slot|. So bool is sufficient for that caller.

If this were to return a crypto::ScopedSECKEYPrivateKey, that means that a
public header file in //net depends on header files from NSS. That means that
targets which depend on //net must also pull in NSS headers. The way one
expresses this is for NSS to be in net's export_dependent_settings (gyp) or
public_deps (gn).

Given that we have no need for the return value anyway, it seemed better to just
not expose that dependency. (Though, honestly, we're pretty bad at exporting
dependencies when we should. If our buildsystem had better control over our
includes, we'd probably have to declare a lot more stuff.)

[pneubeck (no reviews), 2015-04-28 09:56:27]

several comments apply to the original code already, so feel free to punt these
parts. (I can also do them myself in a follow up)

https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:122:
crypto::ScopedSECKEYPrivateKey CreateOwnerKeyInSlot(PK11SlotInfo* slot) {
just drop the return type, it's anyways not used.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:40: key_der.data = const_cast<unsigned char*>(&input[0]);
note that this fails if size is null.
probably (depending on what the policy for crypto/ is) replace by
vector_as_array from base/stl_util.h .

or handle empty input explicitly.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:62: return
ScopedSECItem(PK11_MakeIDFromPubKey(pub_key_index));
you could move this to line 55 and remove the temporary pub_key_index .

(if not, initialize pub_key_index to nullptr pls)

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:69: bool GenerateRSAKeyPairNSS(PK11SlotInfo* slot,
it's unclear (or at least subtle) how PK11_GenerateKeyPair handles a
slot==nulltptr.
you could add a CHECK for that.

same for all other functions that take a slot argument.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:77: SECKEYPublicKey* public_key_raw;
should be intialized to nulltpr.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:78: ScopedSECKEYPrivateKey
private_key(PK11_GenerateKeyPair(
could directly assign to out_private_key
and rename out_private_key to private_key.
and similarly, out_public_key to public_key

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:89: ScopedSECKEYPrivateKey
ImportNSSKeyFromPrivateKeyInfo(
i think this function did an EnsureNSSInit before and doesn't do anymore.

I do not know whether it's actually required.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:90: PK11SlotInfo* slot,
ditto, add check for slot

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:95: NOTREACHED();
these NOTREACHED (which equals a DCHECK) + return are not following the chrome
policy:

A DCHECK() means "this condition must always be true", ...
A consequence of this is that you should not handle DCHECK() failures, even if
failure would result in a crash.

If these are indeed invariants, then change to DCHECK(arena).
If these are not invariants, these should be handled with the return and
probably a LOG(ERROR) or a CHECK(arena).

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:102: input_item.data = const_cast<unsigned
char*>(&input.front());
ditto, fails if input is empty.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:115: SECKEYPrivateKey* key_raw;
should be initialized to nullptr.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:132: NOTREACHED();
ditto, wrong NOTREACHED

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:138: SECMODModuleList* head =
SECMOD_GetDefaultModuleList();
+ const

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
+ const

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:154: ScopedSECKEYPrivateKey
FindNSSKeyFromPublicKeyInfoInSlot(
i think this function did an EnsureNSSInit before and doesn't do anymore.

I do not know whether it's actually required.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:156: PK11SlotInfo* slot) {
ditto, add check for slot

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:159: NOTREACHED();
ditto, wrong NOTREACHED

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util_uni...
File crypto/nss_key_util_unittest.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util_uni...
crypto/nss_key_util_unittest.cc:31: PK11SlotInfo* internal_slot() { return
internal_slot_.get(); }
can't you just return test_nssdb_.slot() ?

If not, you can probably remove test_nssdb_, as it's not used.

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
File net/test/cert_test_util_nss.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
net/test/cert_test_util_nss.cc:34: crypto::ScopedSECKEYPrivateKey private_key(
nit:
missing
#include "crypto/scoped_nss_types.h" ?

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
net/test/cert_test_util_nss.cc:39: return !!private_key;
nit: 
s/!!//

[davidben, 2015-04-28 16:27:46]

https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc
(right):

https://codereview.chromium.org/1106103003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/auth/cryptohome_authenticator_unittest.cc:122:
crypto::ScopedSECKEYPrivateKey CreateOwnerKeyInSlot(PK11SlotInfo* slot) {
On 2015/04/28 09:56:26, pneubeck wrote:
> just drop the return type, it's anyways not used.

Switched it to a bool + ASSERT_TRUE.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:40: key_der.data = const_cast<unsigned char*>(&input[0]);
On 2015/04/28 09:56:26, pneubeck wrote:
> note that this fails if size is null.
> probably (depending on what the policy for crypto/ is) replace by
> vector_as_array from base/stl_util.h .
> 
> or handle empty input explicitly.

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:62: return
ScopedSECItem(PK11_MakeIDFromPubKey(pub_key_index));
On 2015/04/28 09:56:26, pneubeck wrote:
> you could move this to line 55 and remove the temporary pub_key_index .
> 
> (if not, initialize pub_key_index to nullptr pls)

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:69: bool GenerateRSAKeyPairNSS(PK11SlotInfo* slot,
On 2015/04/28 09:56:26, pneubeck wrote:
> it's unclear (or at least subtle) how PK11_GenerateKeyPair handles a
> slot==nulltptr.
> you could add a CHECK for that.
> 
> same for all other functions that take a slot argument.

NSS generally checks for NULL inputs (and does here). I've added DCHECKs, but
the intent was that these functions can only called on non-NULL slots. It's sad
that the typesystem doesn't check this, but I don't think our code usually
allows for any random T* parameter to be NULL.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:77: SECKEYPublicKey* public_key_raw;
On 2015/04/28 09:56:27, pneubeck wrote:
> should be intialized to nulltpr.

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:78: ScopedSECKEYPrivateKey
private_key(PK11_GenerateKeyPair(
On 2015/04/28 09:56:26, pneubeck wrote:
> could directly assign to out_private_key
> and rename out_private_key to private_key.
> and similarly, out_public_key to public_key

I prefer explicitly annotating output parameters in functions (the out_ prefix
is required in BoringSSL style), but done since it's not Chromium style.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:89: ScopedSECKEYPrivateKey
ImportNSSKeyFromPrivateKeyInfo(
On 2015/04/28 09:56:26, pneubeck wrote:
> i think this function did an EnsureNSSInit before and doesn't do anymore.
> 
> I do not know whether it's actually required.

I didn't put in EnsureNSSInit for functions that take a PK11SlotInfo because, if
you already have a slot, you must have initialized NSS already.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:90: PK11SlotInfo* slot,
On 2015/04/28 09:56:26, pneubeck wrote:
> ditto, add check for slot

Added DCHECK.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:95: NOTREACHED();
On 2015/04/28 09:56:26, pneubeck wrote:
> these NOTREACHED (which equals a DCHECK) + return are not following the chrome
> policy:
> 
> A DCHECK() means "this condition must always be true", ...
> A consequence of this is that you should not handle DCHECK() failures, even if
> failure would result in a crash.
> 
> If these are indeed invariants, then change to DCHECK(arena).
> If these are not invariants, these should be handled with the return and
> probably a LOG(ERROR) or a CHECK(arena).

This came from the old code. Invariant is really not a well-defined word here.
This is a function that normally can fail due to malloc failure, and NSS tries
to be malloc-failure-safe. However our malloc cannot fail. But, being a library
that handles malloc failures (and an external one at that!), it's strictly
speaking not promised anywhere that only malloc failures are the only ways it
will ever fail. I think NOTREACHED() + return is a fine way to handle such a
thing.

Regardless, switched to a DCHECK. We vary elsewhere between never checking and
DCHECK, so DCHECK it is.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:102: input_item.data = const_cast<unsigned
char*>(&input.front());
On 2015/04/28 09:56:26, pneubeck wrote:
> ditto, fails if input is empty.

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:115: SECKEYPrivateKey* key_raw;
On 2015/04/28 09:56:26, pneubeck wrote:
> should be initialized to nullptr.

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:132: NOTREACHED();
On 2015/04/28 09:56:26, pneubeck wrote:
> ditto, wrong NOTREACHED

Removed the NOTREACHED. I probably got it from the old code. It's wrong.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:138: SECMODModuleList* head =
SECMOD_GetDefaultModuleList();
On 2015/04/28 09:56:26, pneubeck wrote:
> + const

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
On 2015/04/28 09:56:26, pneubeck wrote:
> + const

I'm assume this meant to be the previous line? Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:154: ScopedSECKEYPrivateKey
FindNSSKeyFromPublicKeyInfoInSlot(
On 2015/04/28 09:56:26, pneubeck wrote:
> i think this function did an EnsureNSSInit before and doesn't do anymore.
> 
> I do not know whether it's actually required.

Ditto about slot parameter.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:156: PK11SlotInfo* slot) {
On 2015/04/28 09:56:26, pneubeck wrote:
> ditto, add check for slot

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:159: NOTREACHED();
On 2015/04/28 09:56:26, pneubeck wrote:
> ditto, wrong NOTREACHED

Done.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util_uni...
File crypto/nss_key_util_unittest.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util_uni...
crypto/nss_key_util_unittest.cc:31: PK11SlotInfo* internal_slot() { return
internal_slot_.get(); }
On 2015/04/28 09:56:27, pneubeck wrote:
> can't you just return test_nssdb_.slot() ?

No, because that doesn't make sense in non-USE_NSS_CERTS ports.

> If not, you can probably remove test_nssdb_, as it's not used.

I don't really know what that code is doing. This is mimicking the behavior of
rsa_priviate_key_nss_unittest.cc. If this doesn't have some obnoxious hidden
global behavior, I'll get rid of it.

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
File net/test/cert_test_util_nss.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
net/test/cert_test_util_nss.cc:34: crypto::ScopedSECKEYPrivateKey private_key(
On 2015/04/28 09:56:27, pneubeck wrote:
> nit:
> missing
> #include "crypto/scoped_nss_types.h" ?

Done.

https://codereview.chromium.org/1106103003/diff/80001/net/test/cert_test_util...
net/test/cert_test_util_nss.cc:39: return !!private_key;
On 2015/04/28 09:56:27, pneubeck wrote:
> nit: 
> s/!!//

Done. I believe there are cases where MSVC wants an explicit cast to bool and I
somewhat prefer it, but this seems to compile.

[pneubeck (no reviews), 2015-04-28 16:40:28]

I don't know about the components/ownership/ change, but the rest lgtm.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:69: bool GenerateRSAKeyPairNSS(PK11SlotInfo* slot,
On 2015/04/28 16:27:46, David Benjamin wrote:
> On 2015/04/28 09:56:26, pneubeck wrote:
> > it's unclear (or at least subtle) how PK11_GenerateKeyPair handles a
> > slot==nulltptr.
> > you could add a CHECK for that.
> > 
> > same for all other functions that take a slot argument.
> 
> NSS generally checks for NULL inputs (and does here). I've added DCHECKs, but
> the intent was that these functions can only called on non-NULL slots. It's
sad
> that the typesystem doesn't check this, but I don't think our code usually
> allows for any random T* parameter to be NULL.

That's right. Usually we drop a DCHECK if we are confident that it crashes in a
reproducible / debugging-friendly way.
I was just not sure whether that is the case here.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:89: ScopedSECKEYPrivateKey
ImportNSSKeyFromPrivateKeyInfo(
On 2015/04/28 16:27:46, David Benjamin wrote:
> On 2015/04/28 09:56:26, pneubeck wrote:
> > i think this function did an EnsureNSSInit before and doesn't do anymore.
> > 
> > I do not know whether it's actually required.
> 
> I didn't put in EnsureNSSInit for functions that take a PK11SlotInfo because,
if
> you already have a slot, you must have initialized NSS already.

Acknowledged.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
On 2015/04/28 16:27:46, David Benjamin wrote:
> On 2015/04/28 09:56:26, pneubeck wrote:
> > + const
> 
> I'm assume this meant to be the previous line? Done.

then both lines ;-)

[davidben, 2015-04-29 21:51:31]

davidben@chromium.org changed reviewers:
+ dpolukhin@chromium.org

[davidben, 2015-04-29 21:51:32]

+dpolukhin for chrome/browser/chromeos/ and components/ownership/ OWNERS.

Also for questions re what do to here about ownership's use of RSAPrivateKey.
One thought is to make sure the SECKEYPrivateKey -> EVP_PKEY bridge (it doesn't
exist right now, but we'll need one for client auth anyway) works with
RSAPrivateKey and then components/ownership can continue to be slightly more
insulated from the future NSS to CHAPS thing. (The others I just switched to NSS
because they were using NSS directly anyway.) Downside is that there'll be less
control over the NSS/CHAPS abstraction since everything has to route through
EVP_PKEY.

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
On 2015/04/28 16:40:27, pneubeck wrote:
> On 2015/04/28 16:27:46, David Benjamin wrote:
> > On 2015/04/28 09:56:26, pneubeck wrote:
> > > + const
> > 
> > I'm assume this meant to be the previous line? Done.
> 
> then both lines ;-)

Do you mean the local variable? I didn't think we usually consted those. It's
not a pointer.

[pneubeck (no reviews), 2015-04-30 07:45:56]

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc
File crypto/nss_key_util.cc (right):

https://codereview.chromium.org/1106103003/diff/80001/crypto/nss_key_util.cc#...
crypto/nss_key_util.cc:140: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
On 2015/04/29 21:51:32, David Benjamin wrote:
> On 2015/04/28 16:40:27, pneubeck wrote:
> > On 2015/04/28 16:27:46, David Benjamin wrote:
> > > On 2015/04/28 09:56:26, pneubeck wrote:
> > > > + const
> > > 
> > > I'm assume this meant to be the previous line? Done.
> > 
> > then both lines ;-)
> 
> Do you mean the local variable?
Yes.

> I didn't think we usually consted those. It's
> not a pointer.

as you prefer. I don't think there is a clear rule for that. I tend to const
such as well.

[Dmitry Polukhin, 2015-04-30 08:44:45]

In general things looks good but I don't know how we build real Chrome for
Chrome OS. If USE_OPENSSL is set by default, current CL will break ownership
checking as far as I understand. Could you fix ownership in this CL instated of
resolving it in future CL?

[davidben, 2015-04-30 13:35:38]

On 2015/04/30 08:44:45, Dmitry Polukhin wrote:
> In general things looks good but I don't know how we build real Chrome for
> Chrome OS. If USE_OPENSSL is set by default, current CL will break ownership
> checking as far as I understand. Could you fix ownership in this CL instated
of
> resolving it in future CL?

USE_OPENSSL + CrOS doesn't even build right now. :-) I'm hoping to ship it in a
release or two, so this is part of trying to get it working. See
https://docs.google.com/document/d/1x4DOCKwFkAxl9MGfd6snIzFigO4ku6Shuci0r5Bza...

[Dmitry Polukhin, 2015-04-30 13:38:17]

lgtm

[Ryan Sleevi, 2015-04-30 18:34:23]

lgtm

[davidben, 2015-04-30 19:40:47]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-04-30 19:41:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1106103003/100001

[commit-bot: I haz the power, 2015-04-30 19:52:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-30 19:52:10]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[davidben, 2015-04-30 22:37:29]

davidben@chromium.org changed reviewers:
+ caitkp@chromium.org

[davidben, 2015-04-30 22:37:30]

+caitkp for components/ownership.gypi (hrm, perhaps that should be OWNERSd to
the components/ownership folks)

[Cait (Slow), 2015-05-01 14:04:02]

I'm not too familiar with the ownership/ component. Please get a
components/ownership OWNER (alemate@ or dpolukhin@) to give it a once over, then
I can give it a rubber stamp l-g-t-m as a components/ owner.

https://codereview.chromium.org/1106103003/diff/100001/components/ownership.gypi
File components/ownership.gypi (right):

https://codereview.chromium.org/1106103003/diff/100001/components/ownership.g...
components/ownership.gypi:40: '../build/linux/system.gyp:ssl',
Why are we including this here, but not in the corresponding BUILD.gn file?

[Ryan Sleevi, 2015-05-01 14:05:21]

On 2015/05/01 14:04:02, Cait wrote:
> I'm not too familiar with the ownership/ component. Please get a
> components/ownership OWNER (alemate@ or dpolukhin@) to give it a once over,
then
> I can give it a rubber stamp l-g-t-m as a components/ owner.
> 

See above, dpolukhin@ already did :)

[Ryan Sleevi, 2015-05-01 14:07:41]

https://codereview.chromium.org/1106103003/diff/100001/components/ownership.gypi
File components/ownership.gypi (right):

https://codereview.chromium.org/1106103003/diff/100001/components/ownership.g...
components/ownership.gypi:40: '../build/linux/system.gyp:ssl',
On 2015/05/01 14:04:01, Cait wrote:
> Why are we including this here, but not in the corresponding BUILD.gn file?

The target is different in BUILD.gn because we could do the layering right in
GN.

That is, this is what //crypto:platform resolves to

[Cait (Slow), 2015-05-01 14:09:37]

Ah -- I see a components/ownership OWNER has already approved this CL, so please
disregard that part of my last comment.

I'm still wondering about the .gypi vs .gn files though.

[Cait (Slow), 2015-05-01 14:13:05]

codereview needs a "new message" notification :\
sorry for the noise and thanks for clarification.
lgtm

[davidben, 2015-05-01 15:31:15]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-05-01 15:31:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1106103003/100001

[commit-bot: I haz the power, 2015-05-01 15:36:06]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-01 15:36:59]

Patchset 6 (id:??) landed as
https://crrev.com/a46a990b2ccae2b66e87b5f76d2866044dc3182e
Cr-Commit-Position: refs/heads/master@{#327909}

[spang, 2015-05-01 21:00:23]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1118263003/ by spang@chromium.org.

The reason for reverting is: Causes SEGV during login on Chrome OS

BUG=483606.