Fix SpdySession::WriteCredentialFrame ECPrivateKey creation args.

Fix SpdySession::WriteCredentialFrame ECPrivateKey creation args.

It was passing the DER certificate instead of a SubjectPublicKeyInfo.

Also adds ECSignatureCreator::SetFactoryForTesting method to allow
easier testing of code that uses ECSignatureCreator.

BUG=none
TEST=SpdyHttpStreamTest.SendCredentialsEC


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123940

[mattm, 2012-02-23 03:49:59]

Figured this fix should be split out.

[Ryan Hamilton, 2012-02-23 04:36:12]

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator.cc
File crypto/ec_signature_creator.cc (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator....
crypto/ec_signature_creator.cc:11: ECSignatureCreatorFactory* g_factory_ = NULL;
I was going to recommend that you move this to the main .cc file, and then you
did :>

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator.h
File crypto/ec_signature_creator.h (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:21: virtual ECSignatureCreator*
Create(ECPrivateKey* key) = 0;
Do you need a virtual destructor?

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
File crypto/ec_signature_creator_nss.cc (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
crypto/ec_signature_creator_nss.cc:54: explicit
ECSignatureCreatorNSS(ECPrivateKey* key);
The comment says private, but the access modifier is public:  Is that
intentional?

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
File crypto/ec_signature_creator_openssl.cc (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
crypto/ec_signature_creator_openssl.cc:12: ECSignatureCreator*
ECSignatureCreator::CreatePlatformImpl(ECPrivateKey* key) {
Nice... cleaner not-implemented implementation...

http://codereview.chromium.org/9455006/diff/6004/net/spdy/spdy_http_stream_un...
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/9455006/diff/6004/net/spdy/spdy_http_stream_un...
net/spdy/spdy_http_stream_unittest.cc:503: &cert, &proof);
I think this needs to be indented one more character.

[mattm, 2012-02-23 04:44:57]

thanks, updated.  Also added OVERRIDE in a few places.

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator.h
File crypto/ec_signature_creator.h (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:21: virtual ECSignatureCreator*
Create(ECPrivateKey* key) = 0;
On 2012/02/23 04:36:12, Ryan Hamilton wrote:
> Do you need a virtual destructor?

yep

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
File crypto/ec_signature_creator_nss.cc (right):

http://codereview.chromium.org/9455006/diff/6004/crypto/ec_signature_creator_...
crypto/ec_signature_creator_nss.cc:54: explicit
ECSignatureCreatorNSS(ECPrivateKey* key);
On 2012/02/23 04:36:12, Ryan Hamilton wrote:
> The comment says private, but the access modifier is public:  Is that
> intentional?

Ah, comment was moved over from the now-abstract ECSignatureCreator class, and
it isn't necessary now since the class is hidden by being in an unnamed
namespace.  Removed the comment.

http://codereview.chromium.org/9455006/diff/6004/net/spdy/spdy_http_stream_un...
File net/spdy/spdy_http_stream_unittest.cc (right):

http://codereview.chromium.org/9455006/diff/6004/net/spdy/spdy_http_stream_un...
net/spdy/spdy_http_stream_unittest.cc:503: &cert, &proof);
On 2012/02/23 04:36:12, Ryan Hamilton wrote:
> I think this needs to be indented one more character.

Done.

[Ryan Hamilton, 2012-02-23 19:38:14]

lgtm

[wtc, 2012-02-23 23:39:57]

Patch Set 5 LGTM.  I didn't review net/spdy/spdy_http_stream_unittest.cc.

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator.h
File crypto/ec_signature_creator.h (right):

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:40: // ECSignatureCreator objects.  Because the
elliptic curve signature algorithm

Nit: elliptic curve signature => ECDSA

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:42: // have deterministic output to compare.

Nit: add "mocked" before "output"

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:56: static ECSignatureCreator*
CreatePlatformImpl(ECPrivateKey* key);

Instead of this CreatePlatformImpl, the design I often see
in the Chromium source tree would be to have a
ECSignatureCreatorImpl class, and have the Factory::Create
method call new ECSignatureCreatorImpl if g_factory_ is NULL.

Do you think that design won't work well here because
we'd need to have an NSS-based and OpenSSL-based versions
of the ECSignatureCreatorImpl class, which is more
confusing than having two versions of the CreatePlatformImpl
function?

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator_...
File crypto/ec_signature_creator_nss.cc (right):

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator_...
crypto/ec_signature_creator_nss.cc:71:
ECSignatureCreatorNSS::~ECSignatureCreatorNSS() { }

Nit: remove the space inside {}.

[mattm, 2012-02-24 01:05:28]

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator.h
File crypto/ec_signature_creator.h (right):

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:40: // ECSignatureCreator objects.  Because the
elliptic curve signature algorithm
On 2012/02/23 23:39:57, wtc wrote:
> 
> Nit: elliptic curve signature => ECDSA

Done.

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:42: // have deterministic output to compare.
On 2012/02/23 23:39:57, wtc wrote:
> 
> Nit: add "mocked" before "output"

Done.

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:56: static ECSignatureCreator*
CreatePlatformImpl(ECPrivateKey* key);
On 2012/02/23 23:39:57, wtc wrote:
> 
> Instead of this CreatePlatformImpl, the design I often see
> in the Chromium source tree would be to have a
> ECSignatureCreatorImpl class, and have the Factory::Create
> method call new ECSignatureCreatorImpl if g_factory_ is NULL.
> 
> Do you think that design won't work well here because
> we'd need to have an NSS-based and OpenSSL-based versions
> of the ECSignatureCreatorImpl class, which is more
> confusing than having two versions of the CreatePlatformImpl
> function?

One issue there would be that it makes the openssl non-implemented case a bit
harder, since we'd either have to return a non-functional ECSignatureCreator
instance, or have an ifdef USE_OPENSSL return NULL in the cross-platform file.

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator_...
File crypto/ec_signature_creator_nss.cc (right):

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator_...
crypto/ec_signature_creator_nss.cc:71:
ECSignatureCreatorNSS::~ECSignatureCreatorNSS() { }
On 2012/02/23 23:39:57, wtc wrote:
> 
> Nit: remove the space inside {}.

Done.

[wtc, 2012-02-24 01:12:19]

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator.h
File crypto/ec_signature_creator.h (right):

http://codereview.chromium.org/9455006/diff/4016/crypto/ec_signature_creator....
crypto/ec_signature_creator.h:56: static ECSignatureCreator*
CreatePlatformImpl(ECPrivateKey* key);
On 2012/02/24 01:05:28, mattm wrote:
>
> One issue there would be that it makes the openssl non-implemented case a bit
> harder, since we'd either have to return a non-functional ECSignatureCreator
> instance, or have an ifdef USE_OPENSSL return NULL in the cross-platform file.

We already had a non-functional ECSignatureCreator class
for OpenSSL, so we can simply rename it to ECSignatureCreatorImpl.

Having an ifdef USE_OPENSSL return NULL would be bad...

[commit-bot: I haz the power, 2012-02-28 05:26:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/9455006/8002

[commit-bot: I haz the power, 2012-02-28 08:06:57]

Change committed as 123940