Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(106)

Issue 307060043: Named group preferences

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years ago by franziskus
Modified:
5 years ago
Reviewers:
mt, ekr-rietveld
Visibility:
Public.

Description

Named group preferences

Patch Set 1 #

Patch Set 2 : SSL_NamedGroupConfig #

Total comments: 37

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Total comments: 43

Patch Set 6 : #

Total comments: 48

Patch Set 7 : #

Patch Set 8 : #

Total comments: 47

Patch Set 9 : #

Total comments: 15

Patch Set 10 : #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+486 lines, -224 lines) Patch
M external_tests/ssl_gtest/ssl_dhe_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +22 lines, -47 lines 1 comment Download
M external_tests/ssl_gtest/ssl_ecdh_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +177 lines, -6 lines 0 comments Download
M external_tests/ssl_gtest/ssl_loopback_unittest.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.h View 1 2 3 4 5 1 chunk +1 line, -1 line 1 comment Download
M external_tests/ssl_gtest/tls_agent.cc View 1 2 3 4 5 2 chunks +5 lines, -14 lines 1 comment Download
M external_tests/ssl_gtest/tls_connect.h View 1 2 3 4 5 2 chunks +17 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/tls_connect.cc View 1 2 3 4 5 6 3 chunks +34 lines, -5 lines 0 comments Download
M external_tests/ssl_gtest/tls_parser.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M lib/ssl/derive.c View 1 2 3 4 5 6 1 chunk +1 line, -3 lines 0 comments Download
M lib/ssl/ssl.h View 1 2 3 4 5 6 3 chunks +14 lines, -14 lines 0 comments Download
M lib/ssl/ssl.def View 1 2 1 chunk +1 line, -1 line 0 comments Download
M lib/ssl/ssl3con.c View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -4 lines 0 comments Download
M lib/ssl/ssl3ecc.c View 1 2 3 4 5 6 7 8 9 6 chunks +55 lines, -32 lines 2 comments Download
M lib/ssl/ssl3ext.c View 1 2 3 4 5 6 7 8 9 2 chunks +5 lines, -2 lines 1 comment Download
M lib/ssl/sslcert.c View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 1 comment Download
M lib/ssl/sslimpl.h View 1 2 3 4 5 6 7 8 9 6 chunks +18 lines, -7 lines 2 comments Download
M lib/ssl/sslsnce.c View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -1 line 0 comments Download
M lib/ssl/sslsock.c View 1 2 3 4 5 6 7 8 9 13 chunks +122 lines, -83 lines 1 comment Download
M lib/ssl/tls13con.c View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -3 lines 0 comments Download

Messages

Total messages: 15
mt
https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc#newcode81 external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, P384Priority) { We should test that the server's ...
5 years ago (2016-08-24 00:19:32 UTC) #1
franziskus
https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc#newcode81 external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, P384Priority) { On 2016/08/24 00:19:32, mt wrote: > ...
5 years ago (2016-08-25 13:03:21 UTC) #2
franziskus
https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/20001/lib/ssl/sslsock.c#newcode1 lib/ssl/sslsock.c:1: /* -*- Mode: C; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: ...
5 years ago (2016-08-26 06:40:54 UTC) #3
ekr-rietveld
I think it's a mistake to keep the other (mask-type) supported groups representation as well ...
5 years ago (2016-08-30 21:02:22 UTC) #4
franziskus
> I think it's a mistake to keep the other (mask-type) supported groups > representation ...
5 years ago (2016-08-31 14:16:11 UTC) #5
ekr-rietveld
Per my comments, I think you should consolidate the mask and the list. https://codereview.appspot.com/307060043/diff/80001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc File ...
5 years ago (2016-09-01 00:27:14 UTC) #6
mt
I think that ekr covered most of this. https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/20001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc#newcode81 external_tests/ssl_gtest/ssl_ecdh_unittest.cc:81: TEST_P(TlsConnectGeneric, ...
5 years ago (2016-09-01 05:55:46 UTC) #7
franziskus
The next patch should be only using the socket's namedGroupPreferences for everything, including enabling/disabling groups. ...
5 years ago (2016-09-02 13:23:30 UTC) #8
ekr-rietveld
This needs another round https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/ssl3con.c#newcode7028 lib/ssl/ssl3con.c:7028: IMPORTANT: Why is this safe ...
5 years ago (2016-09-07 23:30:06 UTC) #9
franziskus
https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/307060043/diff/100001/lib/ssl/sslsock.c#newcode348 lib/ssl/sslsock.c:348: } On 2016/09/07 23:30:05, ekr-rietveld wrote: > On 2016/09/02 ...
5 years ago (2016-09-08 17:26:58 UTC) #10
ekr-rietveld
https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc File external_tests/ssl_gtest/ssl_ecdh_unittest.cc (right): https://codereview.appspot.com/307060043/diff/140001/external_tests/ssl_gtest/ssl_ecdh_unittest.cc#newcode100 external_tests/ssl_gtest/ssl_ecdh_unittest.cc:100: new TlsInspectorRecordHandshakeMessage(kTlsHandshakeHelloRetryRequest); On 2016/09/08 17:26:57, franziskus wrote: > On ...
5 years ago (2016-09-10 19:09:51 UTC) #11
franziskus
This got much bigger than I thought it would. If we can land this tomorrow ...
5 years ago (2016-09-12 03:55:16 UTC) #12
ekr-rietveld
On 2016/09/12 03:55:16, franziskus wrote: > This got much bigger than I thought it would. ...
5 years ago (2016-09-12 04:05:57 UTC) #13
ekr-rietveld
This seems pretty close modulo the issues below https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c File lib/ssl/ssl3ecc.c (right): https://codereview.appspot.com/307060043/diff/160001/lib/ssl/ssl3ecc.c#newcode1199 lib/ssl/ssl3ecc.c:1199: for ...
5 years ago (2016-09-12 17:06:17 UTC) #14
mt
5 years ago (2016-09-14 20:22:06 UTC) #15
https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_agent.cc (right):

https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.cc:126: ConfigNamedGroups(groups,
PR_ARRAY_SIZE(groups));
This should probably include the 2048-bit ffdhe group as well.

https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/tls_agent.h (right):

https://codereview.appspot.com/307060043/diff/180001/external_tests/ssl_gtest...
external_tests/ssl_gtest/tls_agent.h:144: void ConfigNamedGroups(const
SSLNamedGroup* groups, uint8_t num);
size_t instead of uint8_t

https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c
File lib/ssl/ssl3ecc.c (right):

https://codereview.appspot.com/307060043/diff/180001/lib/ssl/ssl3ecc.c#newcod...
lib/ssl/ssl3ecc.c:436: groups = ss->namedGroupPreferences;
namedGroupPreferences can be (const namedGroupDef *)

https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h
File lib/ssl/sslimpl.h (right):

https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:615: namedGroupDef
namedGroupPreferences[SSL_NAMED_GROUP_COUNT];
I'd much prefer if this were const namedGroupDef *[].  That complicates the code
that uses this of course.

That probably means initializing a list, probably using PR_CallOnce.

https://codereview.appspot.com/307060043/diff/180001/lib/ssl/sslimpl.h#newcod...
lib/ssl/sslimpl.h:1089: namedGroupDef *group;
const
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b