Remove platform-specific implementations of RSAPrivateKey and SignatureCreator

Remove platform-specific implementations of RSAPrivateKey and SignatureCreator

Use NSS/OpenSSL on all platforms, rather than deferring to the underlying OS routines.

Because X509Certificate::CreateSelfSigned no longer relies on platform-native types for RSA keys or certificates, it has been moved to x509_util and simply returns a DER-encoded certificate as a string.

BUG=none
R=wtc

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

[Ryan Sleevi, 2013-06-20 00:48:35]

wtc: PTAL.

The X509Certificate -> x509_util shuffling is to reflect the fact that there are
no platform-specific operations going on. It also reflects the fact that there's
only one consumer of CreateSelfSignedCert(), which is Chromoting, and Chromoting
only cares about the DER-encoded certificate.

[dkrahn, 2013-06-20 16:37:24]

https://codereview.chromium.org/17265013/diff/1/chrome/browser/chromeos/attes...
File chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc
(right):

https://codereview.chromium.org/17265013/diff/1/chrome/browser/chromeos/attes...
chrome/browser/chromeos/attestation/attestation_policy_observer_unittest.cc:22:
#include "net/cert/x509_util_nss.h"
Should this now be x509_util.h?

[wtc, 2013-06-20 21:35:56]

Patch set 2 LGTM.

Note the IMPORTANT commet below about |permanent| not
implemented for OS_WIN and OS_MACOSX.

https://codereview.chromium.org/17265013/diff/5001/crypto/rsa_private_key.h
File crypto/rsa_private_key.h (right):

https://codereview.chromium.org/17265013/diff/5001/crypto/rsa_private_key.h#n...
crypto/rsa_private_key.h:244: bool permanent,

IMPORTANT: The |permanent| flag can't be supported when NSS is initialized with
NSS_NoDB_Init (for OS_WIN and OS_MACOSX). Since the function does not return
an error code, the reason for failure may be confusing. So we need to at
least document this limitation for OS_WIN and OS_MACOSX in the comment.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_certificate.h
File net/cert/x509_certificate.h (left):

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_certificate....
net/cert/x509_certificate.h:216: static X509Certificate*
CreateSelfSigned(crypto::RSAPrivateKey* key,

The removal of the X509Certificate::CreateSelfSigned() should be mentioned
in the CL's description.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_certificate_...
File net/cert/x509_certificate_unittest.cc (right):

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_certificate_...
net/cert/x509_certificate_unittest.cc:805: EXPECT_FALSE(der_cert.empty());

This is a very weak test of GetDEREncoded. Ideally we should verify
der_cert has the expected contents. This would require having a DER
version of the ok_cert in the test certs directory.

Or we can just delete this unit test.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util.h
File net/cert/x509_util.h (right):

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util.h#newco...
net/cert/x509_util.h:66: NET_EXPORT bool
CreateSelfSignedCert(crypto::RSAPrivateKey* key,

The change to x509_util::CreateSelfSignedCert should be mentioned in the
CL's description.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util.h#newco...
net/cert/x509_util.h:71: std::string* der_encoded);

Nit: this output argument should ideally have the same name as the
output argument of CreateDomainBoundCertEC.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util_nss.cc
File net/cert/x509_util_nss.cc (right):

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util_nss.cc#...
net/cert/x509_util_nss.cc:345: CERTCertificate* cert =
CreateSelfSignedCertInternal(

I think we can just inline CreateSelfSignedCertInternal here.

https://codereview.chromium.org/17265013/diff/5001/net/cert/x509_util_nss.cc#...
net/cert/x509_util_nss.cc:382: return
CreateDomainBoundCertInternal(key->public_key(),

CreateDomainBoundCertInternal is only used here. It seems that we should
just inline it here.

https://codereview.chromium.org/17265013/diff/5001/remoting/base/rsa_key_pair.cc
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/17265013/diff/5001/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.cc:95: std::string encoded;

Nit: not we can name this variable |cert| or |der_cert|, which would
be more descriptive.

[commit-bot: I haz the power, 2013-06-20 22:37:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/11001

[Ryan Sleevi, 2013-06-20 22:38:41]

wez: remoting/ stamp
mnissler: chrome/browser/chromeos/attestation/ stamp

[Wez, 2013-06-20 23:30:47]

lgtm

https://codereview.chromium.org/17265013/diff/11001/remoting/base/rsa_key_pai...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/17265013/diff/11001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:103: CHECK(result);
This should probably be if (!result) return std::string() - the CHECK was
previously only on the DER encoding, not the cert generation.

[Mattias Nissler (ping if slow), 2013-06-21 00:32:38]

LGTM

[commit-bot: I haz the power, 2013-06-21 00:41:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/19001

[commit-bot: I haz the power, 2013-06-21 01:07:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 01:14:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/15002

[commit-bot: I haz the power, 2013-06-21 01:33:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-21 01:45:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/28003

[wtc, 2013-06-21 02:01:23]

https://codereview.chromium.org/17265013/diff/28003/remoting/base/rsa_key_pai...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/17265013/diff/28003/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:102: &der_cert);

This requires the function to either leave der_cert unchanged
or erase der_cert on failure. I think this is very likely to be
the case.

[commit-bot: I haz the power, 2013-06-21 02:05:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[wtc, 2013-06-21 02:50:30]

https://chromiumcodereview.appspot.com/17265013/diff/28003/net/cert/x509_util...
File net/cert/x509_util_openssl.cc (right):

https://chromiumcodereview.appspot.com/17265013/diff/28003/net/cert/x509_util...
net/cert/x509_util_openssl.cc:67: std::string* der_encoded {

This line is missing a closing parenthesis ')'.

[commit-bot: I haz the power, 2013-06-24 17:00:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/46001

[commit-bot: I haz the power, 2013-06-24 17:53:16]

Retried try job too often on linux_rel for step(s) net_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-06-26 23:27:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/17265013/71001

[commit-bot: I haz the power, 2013-06-27 09:18:44]

Change committed as 208870