QUIC: retransmit packets with the correct encryption.

QUIC: retransmit packets with the correct encryption.

This change does four things:
  * Splits the concept of a completed handshake in two: when encryption is
    established and when the server has confirmed the handshake. In order to do
    0-RTT, we have to start sending after the first of those events.
  * Retransmits packets using the same encryption level as they were sent with.
    Without this, the loss of a client hello message is fatal to the connection
    because it will be retransmitted under encryption and the server will never
    be able to process it.
  * Makes decryption failures an ignored error. This is needed because, if a
    client hello message is lost, the subsequent packets will be encrypted and
the
    server won't have the decrypter to process them.
  * Changes how decrypters are handled by the framer. A server now replaces its
    decrypter completely - thus removing the NullDecrypter. The client now has
    latching alternative decrypters which replace the primary decrypter when
    used. This doesn't completely close the hole: the connection still needs to
    worry about plaintext packets injected into the client.

This change does not implement the correct fallback for the server rejecting a
full client hello. It also doesn't implement a limit for the number of packets
that we'll send without the server confirming the handshake. I'm hoping that
rch can do that much more easily than I can!

Merge internal change: 44690884

R=rch@chromium.org
BUG=

[ramant (doing other things), 2013-04-25 00:11:58]

Hi Ryan,
  Ported all the changes from agl except for the following:

QuicClientSessionTest's CompleteCryptoHandshake passed callback to
CryptoConnect. 

QuicClientSession::OnCryptoHandshakeEvent calls the callback's RUN method if
either handshake is established or confirmed.

Because we haven't enabled ProofVerifier, checked for 2 client hellos instead of
3.

thanks
raman

[wtc, 2013-04-25 00:39:18]

Patch set 1 LGTM.

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session....
net/quic/quic_client_session.cc:114: // TODO(rtenneti): how should we handle
ENCRYPTION_FIRST_ESTABLISHED event?

I don't understand this TODO comment. Could you elaborate?

The internal version of this file doesn't have the
QuicClientSession::OnCryptoHandshakeEvent() method. So I
don't know what this method should look like.

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_crypto_server_s...
File net/quic/quic_crypto_server_stream_test.cc (right):

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_crypto_server_s...
net/quic/quic_crypto_server_stream_test.cc:134: // CompleteCryptoHandshake()
from 2 to 3.

Could you explain (not in this TODO comment) why the handshake
needs one more client hello if there is a ProofVerifier? Thanks.

[Ryan Hamilton, 2013-04-25 17:59:45]

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session....
net/quic/quic_client_session.cc:114: // TODO(rtenneti): how should we handle
ENCRYPTION_FIRST_ESTABLISHED event?
On 2013/04/25 00:39:19, wtc wrote:
> 
> I don't understand this TODO comment. Could you elaborate?
> 
> The internal version of this file doesn't have the
> QuicClientSession::OnCryptoHandshakeEvent() method. So I
> don't know what this method should look like.

I believe we should handle this even by executing the callback.  When encryption
is first established, we "pretend" that we are connected and allow for data to
be sent.  This is the 0RTT case, right?

Sounds like we perhaps need a unit test of QuicClientSession to demonstrate the
0RTT handshake, what do you think?

[agl, 2013-04-25 18:04:58]

On 2013/04/25 17:59:45, Ryan Hamilton wrote:
> I believe we should handle this even by executing the callback.  When
encryption
> is first established, we "pretend" that we are connected and allow for data to
> be sent.  This is the 0RTT case, right?

That's correct. Although there are also a couple of other things that these
events should tie into that I had mentally pegged as "rch tasks", although I can
give them a shot if you wish :)

FIRST_ESTABLISHED should limit the amount of outstanding data so that we don't
go crazy sending 0RTT packets until the server replies. (Maybe a limit of 10
packets?)

REESTABLISHED should be a signal that all the encrypted packets so far are
toast. Congestion control shouldn't see them as drops if possible, but they all
need to be retransmitted.

[ramant (doing other things), 2013-04-26 19:29:54]

PTAL.

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session.cc
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_client_session....
net/quic/quic_client_session.cc:114: // TODO(rtenneti): how should we handle
ENCRYPTION_FIRST_ESTABLISHED event?
On 2013/04/25 17:59:45, Ryan Hamilton wrote:
> On 2013/04/25 00:39:19, wtc wrote:
> > 
> > I don't understand this TODO comment. Could you elaborate?
> > 
> > The internal version of this file doesn't have the
> > QuicClientSession::OnCryptoHandshakeEvent() method. So I
> > don't know what this method should look like.
> 
> I believe we should handle this even by executing the callback.  When
encryption
> is first established, we "pretend" that we are connected and allow for data to
> be sent.  This is the 0RTT case, right?
> 
> Sounds like we perhaps need a unit test of QuicClientSession to demonstrate
the
> 0RTT handshake, what do you think?

Will upload a separate CL for unit test of 0RTT.
Done.

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_crypto_server_s...
File net/quic/quic_crypto_server_stream_test.cc (right):

https://codereview.chromium.org/14083012/diff/1/net/quic/quic_crypto_server_s...
net/quic/quic_crypto_server_stream_test.cc:134: // CompleteCryptoHandshake()
from 2 to 3.
On 2013/04/25 00:39:19, wtc wrote:
> 
> Could you explain (not in this TODO comment) why the handshake
> needs one more client hello if there is a ProofVerifier? Thanks.

Done.

[Ryan Hamilton, 2013-04-26 19:52:17]

https://codereview.chromium.org/14083012/diff/9001/net/quic/quic_client_sessi...
File net/quic/quic_client_session.cc (right):

https://codereview.chromium.org/14083012/diff/9001/net/quic/quic_client_sessi...
net/quic/quic_client_session.cc:54: callback_.Reset();
I though we were going to run the callback here with some sort of error?

Oh, you did it in CloseSession.  Fair enough.  In that case, I don't think you
need to do this since it'll be cleared when the session is deleted.

That being said, you could add a DCHECK(callback_.is_null());

https://codereview.chromium.org/14083012/diff/9001/net/quic/quic_client_sessi...
net/quic/quic_client_session.cc:143: void
QuicClientSession::CloseSessionOnError(int error) {
I think you also want to override QuicSession::ConnectionClose() to invoke the
callback.  What do you think?

[ramant (doing other things), 2013-04-30 02:17:03]

Hi Ryan,
  Please take another look.

thanks
raman

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
File net/quic/quic_client_session.cc (right):

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
net/quic/quic_client_session.cc:54: callback_.Reset();
On 2013/04/26 19:52:17, Ryan Hamilton wrote:
> I though we were going to run the callback here with some sort of error?
> 
> Oh, you did it in CloseSession.  Fair enough.  In that case, I don't think you
> need to do this since it'll be cleared when the session is deleted.
> 
> That being said, you could add a DCHECK(callback_.is_null());

Ran the callback_ if it is not NULL with error.

Done.

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
net/quic/quic_client_session.cc:143: void
QuicClientSession::CloseSessionOnError(int error) {
On 2013/04/26 19:52:17, Ryan Hamilton wrote:
> I think you also want to override QuicSession::ConnectionClose() to invoke the
> callback.  What do you think?

Done.

[Ryan Hamilton, 2013-04-30 02:56:14]

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
File net/quic/quic_client_session.cc (right):

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
net/quic/quic_client_session.cc:54: callback_.Reset();
On 2013/04/30 02:17:03, ramant wrote:
> On 2013/04/26 19:52:17, Ryan Hamilton wrote:
> > I though we were going to run the callback here with some sort of error?
> > 
> > Oh, you did it in CloseSession.  Fair enough.  In that case, I don't think
you
> > need to do this since it'll be cleared when the session is deleted.
> > 
> > That being said, you could add a DCHECK(callback_.is_null());
> 
> Ran the callback_ if it is not NULL with error.
> 
> Done.

I'm not sure I follow.  Is the callback suposed to be set here?  if not, let's
DCHECK().  if so, under what circumstances would it be set?

[wtc, 2013-04-30 15:54:02]

Patch set 3 LGTM. Please wait for rch's approval.

[ramant (doing other things), 2013-04-30 19:00:32]

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
File net/quic/quic_client_session.cc (right):

https://chromiumcodereview.appspot.com/14083012/diff/9001/net/quic/quic_clien...
net/quic/quic_client_session.cc:54: callback_.Reset();
On 2013/04/30 02:56:14, Ryan Hamilton wrote:
> On 2013/04/30 02:17:03, ramant wrote:
> > On 2013/04/26 19:52:17, Ryan Hamilton wrote:
> > > I though we were going to run the callback here with some sort of error?
> > > 
> > > Oh, you did it in CloseSession.  Fair enough.  In that case, I don't think
> you
> > > need to do this since it'll be cleared when the session is deleted.
> > > 
> > > That being said, you could add a DCHECK(callback_.is_null());
> > 
> > Ran the callback_ if it is not NULL with error.
> > 
> > Done.
> 
> I'm not sure I follow.  Is the callback suposed to be set here?  if not, let's
> DCHECK().  if so, under what circumstances would it be set?

Changed the QuicClientSessionTest's GoAwayReceived to CompleteCryptoHandshake
before calling OnGoAway.

Added DCHECK.
Done.

[ramant (doing other things), 2013-05-01 23:39:03]

Hi Ryan,
  Added the zero RTT unit test. MockCryptoClientStream::CryptoConnect doesn't
set  handshake_confirmed_  to true. ZeroRTT test sends the data and verifies
that it got the response.

  Will add other unit tests we had discussed in another CL.

  Would appreciate if you could take another look (not happy with my comments
for HandshakeMode enum).

thanks
raman

[Ryan Hamilton, 2013-05-01 23:47:31]

couple small nits and you're good to go.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
File net/quic/test_tools/mock_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream.h:13: #include
"net/quic/test_tools/mock_crypto_client_stream_factory.h"
I think you can remove this include once you move the enum.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
File net/quic/test_tools/mock_crypto_client_stream_factory.h (right):

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:18: // HandshakeMode
enumerates the handshake mode MockCryptoClientStream should
nit: please move this enum to the mock stream (out of the factory)  (This will
let you remove the new include in the mock_stream.h

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:22: //
encryption_established_ to true but handshake_confirmed_ to true.
// CONFIRM_HANDSHAKE indicates that CryptoConnect will immediately confirm the
handshake and establish encryption.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:26: //
encryption_established_ to true but handshake_confirmed_ to false.
// ZERO_RTT indicates that CryptoConnect will establish encryption but will not
confirm the handshake.

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

PTAL. Thanks for your comments and quick review.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
File net/quic/test_tools/mock_crypto_client_stream.h (right):

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream.h:13: #include
"net/quic/test_tools/mock_crypto_client_stream_factory.h"
On 2013/05/01 23:47:32, Ryan Hamilton wrote:
> I think you can remove this include once you move the enum.

Done.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
File net/quic/test_tools/mock_crypto_client_stream_factory.h (right):

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:18: // HandshakeMode
enumerates the handshake mode MockCryptoClientStream should
On 2013/05/01 23:47:32, Ryan Hamilton wrote:
> nit: please move this enum to the mock stream (out of the factory)  (This will
> let you remove the new include in the mock_stream.h

Done.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:22: //
encryption_established_ to true but handshake_confirmed_ to true.
On 2013/05/01 23:47:32, Ryan Hamilton wrote:
> // CONFIRM_HANDSHAKE indicates that CryptoConnect will immediately confirm the
> handshake and establish encryption.

Done.

https://chromiumcodereview.appspot.com/14083012/diff/77002/net/quic/test_tool...
net/quic/test_tools/mock_crypto_client_stream_factory.h:26: //
encryption_established_ to true but handshake_confirmed_ to false.
On 2013/05/01 23:47:32, Ryan Hamilton wrote:
> // ZERO_RTT indicates that CryptoConnect will establish encryption but will
not
> confirm the handshake.

Done.

[Ryan Hamilton, 2013-05-02 01:18:46]

lgtm

[Ryan Hamilton, 2013-05-02 17:37:36]

Sorry, one minor comment.

https://chromiumcodereview.appspot.com/14083012/diff/78001/net/quic/quic_clie...
File net/quic/quic_client_session.cc (right):

https://chromiumcodereview.appspot.com/14083012/diff/78001/net/quic/quic_clie...
net/quic/quic_client_session.cc:117: // could send the data. Change the
following code if that changes.
I don't understand what this comment means.

[ramant (doing other things), 2013-05-02 18:31:51]

https://chromiumcodereview.appspot.com/14083012/diff/78001/net/quic/quic_clie...
File net/quic/quic_client_session.cc (right):

https://chromiumcodereview.appspot.com/14083012/diff/78001/net/quic/quic_clie...
net/quic/quic_client_session.cc:117: // could send the data. Change the
following code if that changes.
On 2013/05/02 17:37:36, Ryan Hamilton wrote:
> I don't understand what this comment means.

Updated the comment in the CL https://chromiumcodereview.appspot.com/14718011/

Done.