Implement TLS Channel ID support for SPDY CREDENTIAL frames

Implement TLS Channel ID support for SPDY CREDENTIAL frames

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

[ramant (doing other things), 2012-08-02 00:03:05]

lgtm

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:46: DCHECK_EQ(0, (int)spk[0]);
overly nit: period at the end of comments (here and line# 50) please.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:49: 
nit: should we DCHECK_GE(spk.length(), 2)?

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:66:
credential->proof.assign(proof_data.begin(), proof_data.end());
nit: it was confusing for me to store SubjectPublicKey in certs field. Should we
consider renaming credential->certs? It is your call.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder.h (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.h:11: #include "net/base/net_export.h"
nit: consider deleting string_piece.h

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.h:31: // Returns the secret data to be signed
as part of a credential frame.
nit: consider making it private and adding tests as friend.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:26: class MockBuilder : public
SpdyCredentialBuilder {
overly nit: do we need this class? Can we call
SpdyCredentialBuilder::GetCredentialSecret and add the tests that call
GetCredentialSecret as FRIEND_TEST_ALL_PREFIXES in SpdyCredentialBuilder?

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:28: std::string
GetCredentialSecret() {
nit: why GetCredentialSecret public?

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:36: new
base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");
nit: consider using "CreateCertAndKey" or "SpdyCredentialBuilder", or does this
need to be  "SpdyHttpStreamSpdy3Test" for the code to work. It is your call.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:37:
scoped_ptr<ServerBoundCertService> server_bound_cert_service(
overly nit: It would be really awesome to share code with
ServerBoundCertServiceTest

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:75: std::string cert_;
nit: do we need helper_?

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_session.cc#n...
net/spdy/spdy_session.cc:638: SpdyCredentialBuilder::Build(tls_unique, type,
key, cert, slot, &credential);
nit: should we consider checking errors returned by
SpdyCredentialBuilder::Build? or CHECK_EQ(OK, ...)

[ramant (doing other things), 2012-08-02 00:54:20]

lgtm

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:28: std::string
GetCredentialSecret() {
On 2012/08/02 00:03:05, ramant wrote:
> nit: why GetCredentialSecret public?
> 

Please ignore this.

[jar (doing other things), 2012-08-02 02:23:04]

drive by commentary...

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:40: base::StringPiece spk;
nit: abbreviations are discouraged..

WTC said that that spki is very common <sigh>.... but it gets harder to read
when a second abreviation is only different by one character (the trailing i).

...how about....

spki -->  public_key_info

spk --> public_key

spk_data --> public_key_vector

or even shorter in this context....

key_info
key
key_vector

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:47: DCHECK_EQ(4, (int)spk[1]);
Consider static_cast<int> rather than C style.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder.cc:52: spki.data() + spki.size());
nit: no need to wrap.

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
File net/spdy/spdy_credential_builder_unittest.cc (right):

http://codereview.chromium.org/10807088/diff/10001/net/spdy/spdy_credential_b...
net/spdy/spdy_credential_builder_unittest.cc:74: SpdyTestStateHelper helper_;
nit: all members should be private.

[Ryan Hamilton, 2012-08-02 15:53:49]

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
File net/spdy/spdy_credential_builder.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:40: base::StringPiece spk;
On 2012/08/02 02:23:04, jar wrote:
> nit: abbreviations are discouraged..
> 
> WTC said that that spki is very common <sigh>.... but it gets harder to read
> when a second abreviation is only different by one character (the trailing i).
> 
> ...how about....
> 
> spki -->  public_key_info
> 
> spk --> public_key
> 
> spk_data --> public_key_vector

Done

> 
> or even shorter in this context....
> 
> key_info
> key
> key_vector

I like this, but it's problematic because I already have "key".

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:46: DCHECK_EQ(0, (int)spk[0]);
On 2012/08/02 00:03:05, ramant wrote:
> overly nit: period at the end of comments (here and line# 50) please.

(weird, your comment is off by a line again.  Is this a problem with
codereview?)

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:47: DCHECK_EQ(4, (int)spk[1]);
On 2012/08/02 02:23:04, jar wrote:
> Consider static_cast<int> rather than C style.

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:49: 
On 2012/08/02 00:03:05, ramant wrote:
> nit: should we DCHECK_GE(spk.length(), 2)? 

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:52: spki.data() + spki.size());
On 2012/08/02 02:23:04, jar wrote:
> nit: no need to wrap.

Heh, the longer names complicated this :>  I ended up using a tiny helper
method.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.cc:66:
credential->proof.assign(proof_data.begin(), proof_data.end());
On 2012/08/02 00:03:05, ramant wrote:
> nit: it was confusing for me to store SubjectPublicKey in certs field. Should
we
> consider renaming credential->certs? It is your call.

I hear you.  Until we change the spec, I'd prefer to leave the name of the field
alone.  But we should definitely fix this in the spec.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
File net/spdy/spdy_credential_builder.h (right):

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.h:11: #include "net/base/net_export.h"
On 2012/08/02 00:03:05, ramant wrote:
> nit: consider deleting string_piece.h

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder.h:31: // Returns the secret data to be signed
as part of a credential frame.
On 2012/08/02 00:03:05, ramant wrote:
> nit: consider making it private and adding tests as friend.

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
File net/spdy/spdy_credential_builder_unittest.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:26: class MockBuilder : public
SpdyCredentialBuilder {
On 2012/08/02 00:03:05, ramant wrote:
> overly nit: do we need this class? Can we call
> SpdyCredentialBuilder::GetCredentialSecret and add the tests that call
> GetCredentialSecret as FRIEND_TEST_ALL_PREFIXES in SpdyCredentialBuilder?

As I've moved GetCredentialSecret from protected to private, and simply added a
friend class SpdyCredentialBuilderTest.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:36: new
base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");
On 2012/08/02 00:03:05, ramant wrote:
> nit: consider using "CreateCertAndKey" or "SpdyCredentialBuilder",

I don't know what you mean?  The method is already called "CreateCertAndKey",
and SpdyCredentialBuilder does not know how to create certificates.  I think I'm
not understanding you.

> or does this
> need to be  "SpdyHttpStreamSpdy3Test" for the code to work. It is your call.
>

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:37:
scoped_ptr<ServerBoundCertService> server_bound_cert_service(
On 2012/08/02 00:03:05, ramant wrote:
> overly nit: It would be really awesome to share code with
> ServerBoundCertServiceTest

Done.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:74: SpdyTestStateHelper helper_;
On 2012/08/02 02:23:04, jar wrote:
> nit: all members should be private.

Normally, this is true, but the style guide specifically permits this behavior:

  "... we allow data members of a test fixture class to be protected when using
Google Test)."
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:75: std::string cert_;
On 2012/08/02 00:03:05, ramant wrote:
> nit: do we need helper_?

(you comment seems to be one line below the line you are actually commenting on.
 Is this a bug in codereview?)

Yes definitely.  Among other things, the helper ensures that we will have
deterministic EC signatures.  I've added a comment.

[jar (doing other things), 2012-08-02 16:21:56]

One nit reminder below.

I didn't study the code very much, but my drive by items were corrected and LGTM
now.

I think Raman read the code much more closely.

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_sess...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:638: SpdyCredentialBuilder::Build(tls_unique, type,
key, cert, slot, &credential);
I think you skipped this question somehow.  I think the code can generate an
error (return), so it seems better to check for it.

On 2012/08/02 00:03:05, ramant wrote:
> nit: should we consider checking errors returned by
> SpdyCredentialBuilder::Build? or CHECK_EQ(OK, ...)

[Ryan Hamilton, 2012-08-02 16:42:39]

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_sess...
File net/spdy/spdy_session.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/10001/net/spdy/spdy_sess...
net/spdy/spdy_session.cc:638: SpdyCredentialBuilder::Build(tls_unique, type,
key, cert, slot, &credential);
On 2012/08/02 16:21:56, jar wrote:
> I think you skipped this question somehow.  I think the code can generate an
> error (return), so it seems better to check for it.
> 
> On 2012/08/02 00:03:05, ramant wrote:
> > nit: should we consider checking errors returned by
> > SpdyCredentialBuilder::Build? or CHECK_EQ(OK, ...)
> 

Eeek!  You're exactly right, of course.  Done.

[ramant (doing other things), 2012-08-02 16:54:19]

lgtm

Awesome. Many many thanks for making the changes.

-raman

https://chromiumcodereview.appspot.com/10807088/diff/15002/net/spdy/spdy_cred...
File net/spdy/spdy_credential_builder_unittest.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/15002/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:29: new
base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");
nit: was suggesting 'new base::SequencedWorkerPool(1,
"SpdyHttpStreamSpdy3Test");' --> new base::SequencedWorkerPool(1,
"SpdyCredentialBuilderTest");

or

'new base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");' --> new
base::SequencedWorkerPool(1, "CreateCertAndKey");

[Ryan Hamilton, 2012-08-02 17:00:25]

https://chromiumcodereview.appspot.com/10807088/diff/15002/net/spdy/spdy_cred...
File net/spdy/spdy_credential_builder_unittest.cc (right):

https://chromiumcodereview.appspot.com/10807088/diff/15002/net/spdy/spdy_cred...
net/spdy/spdy_credential_builder_unittest.cc:29: new
base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");
On 2012/08/02 16:54:19, ramant wrote:
> nit: was suggesting 'new base::SequencedWorkerPool(1,
> "SpdyHttpStreamSpdy3Test");' --> new base::SequencedWorkerPool(1,
> "SpdyCredentialBuilderTest");
> 
> or
> 
> 'new base::SequencedWorkerPool(1, "SpdyHttpStreamSpdy3Test");' --> new
> base::SequencedWorkerPool(1, "CreateCertAndKey");

Oh, I see.  I totally missed that.  Done.

[commit-bot: I haz the power, 2012-08-02 17:24:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10807088/15003

[commit-bot: I haz the power, 2012-08-02 18:19:52]

Try job failure for 10807088-15003 (retry) on mac_rel for step "net_unittests".
It's a second try, previously, step "net_unittests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-08-02 19:28:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10807088/10013

[commit-bot: I haz the power, 2012-08-02 20:03:24]

Try job failure for 10807088-10013 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-08-06 16:34:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rch@chromium.org/10807088/10016

[commit-bot: I haz the power, 2012-08-06 18:11:13]

Change committed as 150112