Bug 1084669

It looks like I'm going to need this to do multiple certificates on WebRTC properly, so I rebased it this morning.

[ekr-rietveld, 2015-07-08 21:42:54]

LGTM

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:161: {
{ goes on the opening line.

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:237: // A version of 0 is invalid and
indicates no expectation.
The idea here is that you don't always know the version so you're storing it for
later? Can you make these comments clearer.

Minor note:
You could remove this else y doing

if (!expected_version_) { expected_version_ = info.protocolVersion;}
EXPECT_EQ(expected_version_, info.protocolVersion);

Finally, why isn't this ASSERT_EQ()

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:267: ASSERT_EQ(expect_falsestart_hook_ &&
!expect_resumption_,
It seems like expect_falsestart_hook_ doesn't actually mean that. Perhaps rename
it "enable_falsestart_"

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.h:62: void PrepareRenegotiate();
Wouldn't this be better called like "PrepareForRenegotiate"?

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_connect.h (right):

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_connect.h:80: void
CheckResumption(SessionResumptionMode expected);
This doesn't need an argument since it uses expected_resumption_mode_

[ekr-rietveld, 2015-07-08 21:48:17]

IMPORTANT: Per MT's comments, I did not check the code, just the tests

[mt, 2015-07-09 19:27:57]

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:237: // A version of 0 is invalid and
indicates no expectation.
On 2015/07/08 21:42:54, ekr-webrtc wrote:
> The idea here is that you don't always know the version so you're storing it
for
> later? Can you make these comments clearer.

Yep.
 
> Minor note:
> You could remove this else y doing
> 
> if (!expected_version_) { expected_version_ = info.protocolVersion;}
> EXPECT_EQ(expected_version_, info.protocolVersion);

Done.

> Finally, why isn't this ASSERT_EQ()

EXPECT_EQ causes the test to fail.  ASSERT_EQ causes the test to fail AND it
stops.  Wan-Teh suggested that EXPECT_EQ be used unless we have good cause to
stop going.  I wrote this patch before that recommendation and forgot to update
some of the ASSERT_* instances.  We have little cause to use ASSERT_* here. 
There are few places where failure needs to be immediately fatal.

https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
external_tests/ssl_gtest/tls_agent.cc:267: ASSERT_EQ(expect_falsestart_hook_ &&
!expect_resumption_,
On 2015/07/08 21:42:54, ekr-webrtc wrote:
> It seems like expect_falsestart_hook_ doesn't actually mean that. Perhaps
rename
> it "enable_falsestart_"

falsestart_enabled_ I think works.

[mt, 2015-07-09 19:39:58]

Just ignore that please review thing.  I'm going to land with the
changes you suggested.

On 9 July 2015 at 12:27,  <martin.thomson@gmail.com> wrote:
> Reviewers: ekr, ekr-webrtc,
>
>
>
https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
> File external_tests/ssl_gtest/tls_agent.cc (right):
>
>
https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
> external_tests/ssl_gtest/tls_agent.cc:237: // A version of 0 is invalid
> and indicates no expectation.
> On 2015/07/08 21:42:54, ekr-webrtc wrote:
>>
>> The idea here is that you don't always know the version so you're
>
> storing it for
>>
>> later? Can you make these comments clearer.
>
>
> Yep.
>
>> Minor note:
>> You could remove this else y doing
>
>
>> if (!expected_version_) { expected_version_ = info.protocolVersion;}
>> EXPECT_EQ(expected_version_, info.protocolVersion);
>
>
> Done.
>
>> Finally, why isn't this ASSERT_EQ()
>
>
> EXPECT_EQ causes the test to fail.  ASSERT_EQ causes the test to fail
> AND it stops.  Wan-Teh suggested that EXPECT_EQ be used unless we have
> good cause to stop going.  I wrote this patch before that recommendation
> and forgot to update some of the ASSERT_* instances.  We have little
> cause to use ASSERT_* here.  There are few places where failure needs to
> be immediately fatal.
>
>
https://codereview.appspot.com/250320043/diff/1/external_tests/ssl_gtest/tls_...
> external_tests/ssl_gtest/tls_agent.cc:267:
> ASSERT_EQ(expect_falsestart_hook_ && !expect_resumption_,
> On 2015/07/08 21:42:54, ekr-webrtc wrote:
>>
>> It seems like expect_falsestart_hook_ doesn't actually mean that.
>
> Perhaps rename
>>
>> it "enable_falsestart_"
>
>
> falsestart_enabled_ I think works.
>
> Description:
> It looks like I'm going to need this to do multiple certificates on
> WebRTC properly, so I rebased it this morning.
>
> Please review this at https://codereview.appspot.com/250320043/
>
> Affected files (+370, -90 lines):
>   M external_tests/ssl_gtest/ssl_loopback_unittest.cc
>   M external_tests/ssl_gtest/tls_agent.h
>   M external_tests/ssl_gtest/tls_agent.cc
>   M external_tests/ssl_gtest/tls_connect.h
>   M external_tests/ssl_gtest/tls_connect.cc
>   M lib/ssl/ssl.h
>   M lib/ssl/ssl.def
>   M lib/ssl/ssl3con.c
>   M lib/ssl/ssl3ext.c
>   M lib/ssl/sslimpl.h
>   M lib/ssl/sslinfo.c
>   M lib/ssl/sslsecur.c
>   M lib/ssl/sslt.h
>
>