QUIC Crypto - return the reasons for reject message. Reject reason

QUIC Crypto - return the reasons for reject message. Reject reason
contains failures due to invalid source address token client nonce and
server nonce. Will add UMA histogram in chrome when this CL is merged.

Send reject reason to client for debugging purposes. Protected behind
new flags: FLAGS_use_early_return_when_verifying_chlo and
FLAGS_send_quic_crypto_reject_reason

Merge internal change: 69227401

R=jar@chromium.org, wtc@chromium.org, avd@google.com

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

[ramant (doing other things), 2014-06-16 17:35:55]

Hi Jim and Wan-Teh,
  This CL is merge of the internal server changes for reject reason (it is same
as what was checked into our internal source tree).

  Will submit a separate CL for UMA histograms (to implement chrome specific
changes).

PTAL.

thanks
raman

[wtc, 2014-06-19 00:13:25]

Patch set 2 LGTM.

I didn't review the tests, but I reviewed everything else carefully.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/crypto_p...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/crypto_p...
net/quic/crypto/crypto_protocol.h:135: const QuicTag kRejectReason = TAG('R',
'R', 'E', 'J');

This QUIC tag should be named kRREJ and moved up.

The three QUIC tags defined here use an alternative naming convention because
their values contain a non-printing character such as 0 or 255.

I guess you can add a new section "Rejection tags" at line 98 and move this tag
there.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:590: const QuicTag*
reject_reason_tags;

Nit: name this variable |reject_reasons| because strictly speaking they are not
QuicTags.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:594: #ifndef NDEBUG

Nit: use
  #if defined(DEBUG)
or
  #if !defined(NDEBUG)

Chromium's local style recommends against #ifdef and #ifndef. If QUIC's local
style prefers #ifdef and #ifndef, then this is fine.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:596: DVLOG(1) << "Reason for
rejection: " << reject_reason_tags[i];

Nit: Reason => Reasons

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<HandshakeFailureReason>
reject_reasons;

Since reject_reasons is passed to SetVector(), we need to ensure that
sizeof(HandshakeFailureReason) is the same on all platforms.

I recommend declare |reject_reasons| as a vector<QuicTag> (or QuicTagVector?).
QuicTag has a fixed width and it is the type assumed on the client side.

This may require static_cast<QuicTag> when adding a HandshakeFailureReason value
to this vector.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:907: }

Nit: require lines 902-907 as follows:

  if (client_hello.GetStringPiece(kSCID, &requested_scid)) {
    info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
  } else {
    info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
  }

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:908: // No server config with the
requested ID.

It is possible to keep going in this case, although the
ValidateSourceAddressToken test, which requirea a non-null requested_config,
must be skipped.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:915:
DCHECK(requested_config.get());

This DCHECK's condition is ensured by the test on line 900 and the return on
line 910. So I would omit this DCHECK. Up to you.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:954: info->unique = true;

IMPORTANT: change this line to:

  if (!found_error) {
    info->unique = true;
  }

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:976: found_error = true;

IMPORTANT: replace lines 972-976 with:

    helper.ValidationComplete(QUIC_NO_ERROR, "");
    return;

Your new code ends up with the same outcome but is confusing because it sets
|found_error| to true.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:979: if (found_error) {

Please add a comment to explain why you do not keep going at this point. The
reason is not obvious.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.h (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:45: // TODO(rtenneti): Implement
capturing of error from strike register.

I think line 44 should be deleted (seems very similar to line 48), and line 45
should be moved right after line 48.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:50: CLIENT_NONCE_INVALID_FAILURE,

It seems that we should have a CLIENT_NONCE_NOT_UNIQUE_FAILURE

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:67:
SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE,

Nit: these enum values should be documented with one-sentence comments.

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_flags.cc
File net/quic/quic_flags.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_flags.cc#n...
net/quic/quic_flags.cc:45: // client nonce or server nonce or source address
token or server config.

Nit: this comment should mention CHLO. I think you can just say:

// If true, it will return as soon as an error is detected while validating
// CHLO.

If you prefer the current comment, add some commas:

// If true, it will return as soon as an error is detected while validating
// client nonce, server nonce, source address token, or server config.

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_session.cc...
net/quic/quic_session.cc:491:
flow_controller_->UpdateSendWindowOffset(new_window);

This change seems unrelated.

[ramant (doing other things), 2014-06-19 01:57:35]

PTAL. Thanks.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/crypto_p...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/crypto_p...
net/quic/crypto/crypto_protocol.h:135: const QuicTag kRejectReason = TAG('R',
'R', 'E', 'J');
On 2014/06/19 00:13:24, wtc wrote:
> 
> This QUIC tag should be named kRREJ and moved up.
> 
> The three QUIC tags defined here use an alternative naming convention because
> their values contain a non-printing character such as 0 or 255.
> 
> I guess you can add a new section "Rejection tags" at line 98 and move this
tag
> there.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_client_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:590: const QuicTag*
reject_reason_tags;
On 2014/06/19 00:13:24, wtc wrote:
> 
> Nit: name this variable |reject_reasons| because strictly speaking they are
not
> QuicTags.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:594: #ifndef NDEBUG
On 2014/06/19 00:13:24, wtc wrote:
> 
> Nit: use
>   #if defined(DEBUG)
> or
>   #if !defined(NDEBUG)
> 
> Chromium's local style recommends against #ifdef and #ifndef. If QUIC's local
> style prefers #ifdef and #ifndef, then this is fine.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_client_config.cc:596: DVLOG(1) << "Reason for
rejection: " << reject_reason_tags[i];
On 2014/06/19 00:13:24, wtc wrote:
> 
> Nit: Reason => Reasons

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<HandshakeFailureReason>
reject_reasons;
On 2014/06/19 00:13:24, wtc wrote:
> 
> Since reject_reasons is passed to SetVector(), we need to ensure that
> sizeof(HandshakeFailureReason) is the same on all platforms.
> 
> I recommend declare |reject_reasons| as a vector<QuicTag> (or QuicTagVector?).
> QuicTag has a fixed width and it is the type assumed on the client side.
> 
> This may require static_cast<QuicTag> when adding a HandshakeFailureReason
value
> to this vector.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:907: }
On 2014/06/19 00:13:24, wtc wrote:
> 
> Nit: require lines 902-907 as follows:
> 
>   if (client_hello.GetStringPiece(kSCID, &requested_scid)) {
>     info->reject_reasons.push_back(SERVER_CONFIG_UNKNOWN_CONFIG_FAILURE);
>   } else {
>     info->reject_reasons.push_back(SERVER_CONFIG_INCHOATE_HELLO_FAILURE);
>   }

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:908: // No server config with the
requested ID.
On 2014/06/19 00:13:24, wtc wrote:
> 
> It is possible to keep going in this case, although the
> ValidateSourceAddressToken test, which requirea a non-null requested_config,
> must be skipped.

avd suggested we do an early return. Will submit a separate CL with that change
so that we can discuss in it. Hope you don't mind.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:915:
DCHECK(requested_config.get());
On 2014/06/19 00:13:24, wtc wrote:
> 
> This DCHECK's condition is ensured by the test on line 900 and the return on
> line 910. So I would omit this DCHECK. Up to you.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:954: info->unique = true;
On 2014/06/19 00:13:24, wtc wrote:
> 
> IMPORTANT: change this line to:
> 
>   if (!found_error) {
>     info->unique = true;
>   }

Thanks for catching this.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:976: found_error = true;
On 2014/06/19 00:13:24, wtc wrote:
> 
> IMPORTANT: replace lines 972-976 with:
> 
>     helper.ValidationComplete(QUIC_NO_ERROR, "");
>     return;
> 
> Your new code ends up with the same outcome but is confusing because it sets
> |found_error| to true.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:979: if (found_error) {
On 2014/06/19 00:13:24, wtc wrote:
> 
> Please add a comment to explain why you do not keep going at this point. The
> reason is not obvious.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.h (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:45: // TODO(rtenneti): Implement
capturing of error from strike register.
On 2014/06/19 00:13:25, wtc wrote:
> 
> I think line 44 should be deleted (seems very similar to line 48), and line 45
> should be moved right after line 48.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:50: CLIENT_NONCE_INVALID_FAILURE,
On 2014/06/19 00:13:25, wtc wrote:
> 
> It seems that we should have a CLIENT_NONCE_NOT_UNIQUE_FAILURE

Will add it in the next CL when we verify client_nonce via strike register
(after making changes in the internal source
tree).

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.h:67:
SOURCE_ADDRESS_TOKEN_EXPIRED_FAILURE,
On 2014/06/19 00:13:25, wtc wrote:
> 
> Nit: these enum values should be documented with one-sentence comments.

Will do in the next CL (will add the comments in the internal source tree).

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_flags.cc
File net/quic/quic_flags.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_flags.cc#n...
net/quic/quic_flags.cc:45: // client nonce or server nonce or source address
token or server config.
On 2014/06/19 00:13:25, wtc wrote:
> 
> Nit: this comment should mention CHLO. I think you can just say:
> 
> // If true, it will return as soon as an error is detected while validating
> // CHLO.
> 
> If you prefer the current comment, add some commas:
> 
> // If true, it will return as soon as an error is detected while validating
> // client nonce, server nonce, source address token, or server config.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_session.cc
File net/quic/quic_session.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/quic_session.cc...
net/quic/quic_session.cc:491:
flow_controller_->UpdateSendWindowOffset(new_window);
On 2014/06/19 00:13:25, wtc wrote:
> 
> This change seems unrelated.

Rebase with TOT mixed it up. Rebasing again fixed this.

[avd, 2014-06-19 02:27:54]

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<HandshakeFailureReason>
reject_reasons;
On 2014/06/19 01:57:35, ramant wrote:
> On 2014/06/19 00:13:24, wtc wrote:
> > 
> > Since reject_reasons is passed to SetVector(), we need to ensure that
> > sizeof(HandshakeFailureReason) is the same on all platforms.
> > 
> > I recommend declare |reject_reasons| as a vector<QuicTag> (or
QuicTagVector?).
> > QuicTag has a fixed width and it is the type assumed on the client side.
> > 
> > This may require static_cast<QuicTag> when adding a HandshakeFailureReason
> value
> > to this vector.
> 
> Done.

QuicTag has a very specific meaning, and this is not it.  I think you should be
doing static_cast<uint32> if anything.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:954: info->unique = true;
On 2014/06/19 01:57:34, ramant wrote:
> On 2014/06/19 00:13:24, wtc wrote:
> > 
> > IMPORTANT: change this line to:
> > 
> >   if (!found_error) {
> >     info->unique = true;
> >   }
> 
> Thanks for catching this.
> 
> Done.

In practice it doesn't matter since other failure conditions will be set in the
info object.  I still think it is pretty silly to attempt to continue processing
after some of these failure conditions - I even think there is mutual desire to
restore the early return version once we're done collecting data.

[ramant (doing other things), 2014-06-19 17:25:48]

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<HandshakeFailureReason>
reject_reasons;
On 2014/06/19 02:27:54, avd wrote:
> On 2014/06/19 01:57:35, ramant wrote:
> > On 2014/06/19 00:13:24, wtc wrote:
> > > 
> > > Since reject_reasons is passed to SetVector(), we need to ensure that
> > > sizeof(HandshakeFailureReason) is the same on all platforms.
> > > 
> > > I recommend declare |reject_reasons| as a vector<QuicTag> (or
> QuicTagVector?).
> > > QuicTag has a fixed width and it is the type assumed on the client side.
> > > 
> > > This may require static_cast<QuicTag> when adding a HandshakeFailureReason
> > value
> > > to this vector.
> > 
> > Done.
> 
> QuicTag has a very specific meaning, and this is not it.  I think you should
be
> doing static_cast<uint32> if anything.

Changed it vector<uint32> and added static_cast<uint32>. A compromise.

Done.

https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:954: info->unique = true;
On 2014/06/19 02:27:53, avd wrote:
> On 2014/06/19 01:57:34, ramant wrote:
> > On 2014/06/19 00:13:24, wtc wrote:
> > > 
> > > IMPORTANT: change this line to:
> > > 
> > >   if (!found_error) {
> > >     info->unique = true;
> > >   }
> > 
> > Thanks for catching this.
> > 
> > Done.
> 
> In practice it doesn't matter since other failure conditions will be set in
the
> info object.  I still think it is pretty silly to attempt to continue
processing
> after some of these failure conditions - I even think there is mutual desire
to
> restore the early return version once we're done collecting data.

Acknowledged.

[ramant (doing other things), 2014-06-19 18:35:48]

The CQ bit was checked by rtenneti@chromium.org

[ramant (doing other things), 2014-06-19 18:36:35]

Will submit a new CL for newer comments.

thanks
raman

On 2014/06/19 17:25:48, ramant wrote:
>
https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
> File net/quic/crypto/quic_crypto_server_config.cc (right):
> 
>
https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
> net/quic/crypto/quic_crypto_server_config.cc:87:
vector<HandshakeFailureReason>
> reject_reasons;
> On 2014/06/19 02:27:54, avd wrote:
> > On 2014/06/19 01:57:35, ramant wrote:
> > > On 2014/06/19 00:13:24, wtc wrote:
> > > > 
> > > > Since reject_reasons is passed to SetVector(), we need to ensure that
> > > > sizeof(HandshakeFailureReason) is the same on all platforms.
> > > > 
> > > > I recommend declare |reject_reasons| as a vector<QuicTag> (or
> > QuicTagVector?).
> > > > QuicTag has a fixed width and it is the type assumed on the client side.
> > > > 
> > > > This may require static_cast<QuicTag> when adding a
HandshakeFailureReason
> > > value
> > > > to this vector.
> > > 
> > > Done.
> > 
> > QuicTag has a very specific meaning, and this is not it.  I think you should
> be
> > doing static_cast<uint32> if anything.
> 
> Changed it vector<uint32> and added static_cast<uint32>. A compromise.
> 
> Done.
> 
>
https://codereview.chromium.org/331143006/diff/40001/net/quic/crypto/quic_cry...
> net/quic/crypto/quic_crypto_server_config.cc:954: info->unique = true;
> On 2014/06/19 02:27:53, avd wrote:
> > On 2014/06/19 01:57:34, ramant wrote:
> > > On 2014/06/19 00:13:24, wtc wrote:
> > > > 
> > > > IMPORTANT: change this line to:
> > > > 
> > > >   if (!found_error) {
> > > >     info->unique = true;
> > > >   }
> > > 
> > > Thanks for catching this.
> > > 
> > > Done.
> > 
> > In practice it doesn't matter since other failure conditions will be set in
> the
> > info object.  I still think it is pretty silly to attempt to continue
> processing
> > after some of these failure conditions - I even think there is mutual desire
> to
> > restore the early return version once we're done collecting data.
> 
> Acknowledged.

[commit-bot: I haz the power, 2014-06-19 18:37:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/rtenneti@chromium.org/331143006/80001

[wtc, 2014-06-19 19:58:43]

Patch set 4 LGTM. You can let the commit queue job finish and make the changes I
suggested in your new CL. Thanks.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_p...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_p...
net/quic/crypto/crypto_protocol.h:114: const QuicTag kRREJ = TAG('R', 'R', 'E',
'J');

Nit: please make this change in your new CL.

Move this definition to line 98, before the "// Server hello tags" line, and
change this to:

// Rejection tags
const QuicTag kRREJ = TAG('R', 'R', 'E', 'J');   // Reasons for server sending
                                                 // rejection message tag.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_s...
File net/quic/crypto/crypto_server_test.cc (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_s...
net/quic/crypto/crypto_server_test.cc:273: const QuicTag* reject_reason_tags;

Nit: also rename this variable "reject_reasons".

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<uint32> reject_reasons;

Nit: you may want to add a COMPILE_ASSERT that sizeof(QuicTag) == sizeof(uint32)
because you are using GetTagList() to decode the kRREJ field of a CHLO.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:981: // We want to contact strike
register if there are no errors because it is

Nit: add "only" before "if".

[wtc, 2014-06-19 20:04:09]

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:157:
static_cast<uint32>(CLIENT_NONCE_UNKNOWN_FAILURE));

It would be nice to omit the static_cast if the compiler doesn't complain.
Please do this experiment in your new CL.

[commit-bot: I haz the power, 2014-06-19 23:03:36]

Change committed as 278523

[ramant (doing other things), 2014-06-20 23:20:47]

Fixed these comments in internal source tree first and then uploaded the merged
changes in  https://codereview.chromium.org/336273006/

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_p...
File net/quic/crypto/crypto_protocol.h (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_p...
net/quic/crypto/crypto_protocol.h:114: const QuicTag kRREJ = TAG('R', 'R', 'E',
'J');
On 2014/06/19 19:58:42, wtc wrote:
> 
> Nit: please make this change in your new CL.
> 
> Move this definition to line 98, before the "// Server hello tags" line, and
> change this to:
> 
> // Rejection tags
> const QuicTag kRREJ = TAG('R', 'R', 'E', 'J');   // Reasons for server sending
>                                                  // rejection message tag.

Fixed in CL: https://codereview.chromium.org/336273006/

Done.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_s...
File net/quic/crypto/crypto_server_test.cc (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/crypto_s...
net/quic/crypto/crypto_server_test.cc:273: const QuicTag* reject_reason_tags;
On 2014/06/19 19:58:42, wtc wrote:
> 
> Nit: also rename this variable "reject_reasons".

Fixed in CL: https://codereview.chromium.org/336273006/

Done.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
File net/quic/crypto/quic_crypto_server_config.cc (right):

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:87: vector<uint32> reject_reasons;
On 2014/06/19 19:58:42, wtc wrote:
> 
> Nit: you may want to add a COMPILE_ASSERT that sizeof(QuicTag) ==
sizeof(uint32)
> because you are using GetTagList() to decode the kRREJ field of a CHLO.

Fixed in CL: https://codereview.chromium.org/336273006/

Done.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:157:
static_cast<uint32>(CLIENT_NONCE_UNKNOWN_FAILURE));
On 2014/06/19 20:04:09, wtc wrote:
> 
> It would be nice to omit the static_cast if the compiler doesn't complain.
> Please do this experiment in your new CL.

Fixed in CL: https://codereview.chromium.org/336273006/

Done.

https://codereview.chromium.org/331143006/diff/80001/net/quic/crypto/quic_cry...
net/quic/crypto/quic_crypto_server_config.cc:981: // We want to contact strike
register if there are no errors because it is
On 2014/06/19 19:58:42, wtc wrote:
> 
> Nit: add "only" before "if".

Done.