More tests for session resumption

More tests for session resumption

[mt, 2015-01-15 23:08:09]

I think that you should take a little harder at the session ticket stuff, but
it's pretty good.

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:466: 
What about info_, as requested last time?

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:613: CheckTicketUsed(true);
This looks like it works, but where are the session tickets persisted if you
create a new agent on every Reset() ?  Also, don't you want to disable the
session cache here explicitly?

Also, I assume that this test is also asserting that tickets are favored over
session IDs.  That seems like its own test.

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:621:
server_->SetSessionTicketsEnabled(true);
Looks like a prime example of some lines that can be factored out.  Especially
if you want to ensure that the session cache is enabled (I assume that 'true' is
the initial state).

[ekr-rietveld, 2015-01-15 23:28:09]

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:466: 
On 2015/01/15 23:08:09, mt wrote:
> What about info_, as requested last time?

That's not a global but rather on the agent. See line 217.

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:613: CheckTicketUsed(true);
On 2015/01/15 23:08:09, mt wrote:
> This looks like it works, but where are the session tickets persisted if you
> create a new agent on every Reset() ? 

On the client they are in the global session cache, which supersedes agents. On
the server you just store the cache key.


> Also, don't you want to disable the
> session cache here explicitly?

You can't. Check the tests below to see what happens.


> Also, I assume that this test is also asserting that tickets are favored over
> session IDs.

Yes.


>  That seems like its own test.

Can you say more about this

https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:621:
server_->SetSessionTicketsEnabled(true);
On 2015/01/15 23:08:09, mt wrote:
> Looks like a prime example of some lines that can be factored out.  Especially
> if you want to ensure that the session cache is enabled (I assume that 'true'
is
> the initial state).

Can you provide some suggestions of what you mean here.

[mt, 2015-01-15 23:46:11]

On 2015/01/15 23:28:09, ekr-webrtc wrote:
> That's not a global but rather on the agent. See line 217.

Sorry, I missed that.
 
>
https://codereview.appspot.com/193230044/diff/1/external_tests/ssl_gtest/ssl_...
> external_tests/ssl_gtest/ssl_loopback_unittest.cc:613: CheckTicketUsed(true);
> On 2015/01/15 23:08:09, mt wrote:
> > This looks like it works, but where are the session tickets persisted if you
> > create a new agent on every Reset() ? 
> 
> On the client they are in the global session cache, which supersedes agents.
On
> the server you just store the cache key.

So obvious.

> > Also, don't you want to disable the
> > session cache here explicitly?
> 
> You can't. Check the tests below to see what happens.

Sorry, I should have said enable.  I assume that it's effectively a noop.

> > Also, I assume that this test is also asserting that tickets are favored
over
> > session IDs.
> 
> Yes.
> 
> 
> >  That seems like its own test.
> 
> Can you say more about this

So I'm not sure about the granularity of control here.  It seems like you could:
a. disable tickets, enabled IDs -> resume on ID
b. enable tickets, disabled IDs -> resume on ticket
c. enable both -> resume on ticket
d. disable both -> don't resume

Noting that if the disable happens on either side you get the same result, so
that all the tests involving disabling need two variations.

> > Looks like a prime example of some lines that can be factored out. 
Especially
> > if you want to ensure that the session cache is enabled (I assume that
'true'
> is
> > the initial state).
> 
> Can you provide some suggestions of what you mean here.

see above...
TlsAgent::ConfigureResumption(bool tickets, bool sessionIDs)

...Test::ConfigureResumption(bool tickets, bool sessionIDs) {
  client_->ConfigureResumption(tickets, sessionIDs);
  server_->ConfigureResumption(tickets, sessionIDs);
}

[ekr-rietveld, 2015-01-19 00:29:55]

PTAL

[mt, 2015-01-20 04:52:28]

Looks pretty thorough.  Not a complete matrix, but one could be inferred from
the results.

https://codereview.appspot.com/193230044/diff/20001/external_tests/ssl_gtest/...
File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right):

https://codereview.appspot.com/193230044/diff/20001/external_tests/ssl_gtest/...
external_tests/ssl_gtest/ssl_loopback_unittest.cc:27: RESUME_BOTH = 3 };
Isn't it possible to define:
RESUME_BOTH = RESUME_SESSIONID | RESUME_TICKET
since you are using this as a bitfield.

Also, odd brace placement here.

[ekr-rietveld, 2015-01-21 19:43:20]

MT, are you happy with landing this?

[ekr-rietveld, 2015-01-21 19:49:08]

BTW, sorry that Rietveld got confused about the state of the tree.

[ekr-rietveld, 2015-01-21 20:48:30]

OK, I think this is ready now

[ekr-rietveld, 2015-01-21 20:51:04]

OK, I think this is ready now

[ekr-rietveld, 2015-02-24 21:51:01]

Martin, can you please review this final patch? Would like to land it.

[mt, 2015-02-24 22:19:13]

Ship it.

https://codereview.appspot.com/193230044/diff/100001/coreconf/command.mk
File coreconf/command.mk (left):

https://codereview.appspot.com/193230044/diff/100001/coreconf/command.mk#oldc...
coreconf/command.mk:15: LINK_EXE      = $(LINK) $(OS_LFLAGS) $(LFLAGS)
I don't understand these changes.  I'll assume that you've somehow managed to
test on the different platforms.

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/gtest_utils.h (right):

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/gtest_utils.h:11: 
This needs some forward decls or includes.

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/ssl_gtest.cc (right):

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_gtest.cc:5: #include <stdlib.h>
cstdlib

[ekr-rietveld, 2015-02-24 22:26:11]

https://codereview.appspot.com/193230044/diff/100001/coreconf/command.mk
File coreconf/command.mk (left):

https://codereview.appspot.com/193230044/diff/100001/coreconf/command.mk#oldc...
coreconf/command.mk:15: LINK_EXE      = $(LINK) $(OS_LFLAGS) $(LFLAGS)
On 2015/02/24 22:19:13, mt wrote:
> I don't understand these changes.  I'll assume that you've somehow managed to
> test on the different platforms.

These are a present from some previous patch and rietveld.

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/gtest_utils.h (right):

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/gtest_utils.h:11: 
On 2015/02/24 22:19:13, mt wrote:
> This needs some forward decls or includes.

Sorry, I'm not seeing why. Can you explain?

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/ssl_gtest.cc (right):

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/ssl_gtest.cc:5: #include <stdlib.h>
On 2015/02/24 22:19:13, mt wrote:
> cstdlib

Acknowledged.

[mt, 2015-02-25 03:17:30]

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/gtest_utils.h (right):

https://codereview.appspot.com/193230044/diff/100001/external_tests/ssl_gtest...
external_tests/ssl_gtest/gtest_utils.h:11: 
On 2015/02/24 22:26:10, ekr-webrtc wrote:
> On 2015/02/24 22:19:13, mt wrote:
> > This needs some forward decls or includes.
> 
> Sorry, I'm not seeing why. Can you explain?

I think that the following would work.

#include "gtest.h"
#include "test_io.h"

Most files that include this already include those headers, but if they don't,
the only way to find out is to start grepping around for Poller and ASSERT_EQ