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

Issue 297180043: Bug 1266237 - Enable FFDHE and DHE for TLS 1.3 (Closed)

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

Description

Bug 1266237 - Enable FFDHE and DHE for TLS 1.3

Patch Set 1 #

Total comments: 1

Patch Set 2 : Added resumption #

Total comments: 100

Patch Set 3 : First review round, less gratuity #

Total comments: 155

Patch Set 4 : Updated with comments, new tests #

Total comments: 46

Patch Set 5 : Latest #

Total comments: 2

Patch Set 6 : Correcting omissions #

Total comments: 27

Patch Set 7 : Moar tests #

Total comments: 20

Patch Set 8 : More tests, fix for overrun #

Total comments: 16

Patch Set 9 : Fixes, test restructure #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+2515 lines, -1568 lines) Patch
M cmd/selfserv/selfserv.c View 1 2 3 3 chunks +8 lines, -2 lines 0 comments Download
M cmd/tstclnt/tstclnt.c View 1 2 3 5 chunks +17 lines, -1 line 0 comments Download
M external_tests/ssl_gtest/manifest.mn View 1 chunk +7 lines, -6 lines 2 comments Download
M external_tests/ssl_gtest/ssl_ciphersuite_unittest.cc View 1 2 3 4 1 chunk +5 lines, -6 lines 0 comments Download
A external_tests/ssl_gtest/ssl_dhe_unittest.cc View 1 2 3 4 5 6 7 8 1 chunk +463 lines, -0 lines 1 comment Download
M external_tests/ssl_gtest/ssl_extension_unittest.cc View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M external_tests/ssl_gtest/ssl_loopback_unittest.cc View 1 2 3 4 5 6 7 8 7 chunks +22 lines, -25 lines 0 comments Download
M external_tests/ssl_gtest/ssl_skip_unittest.cc View 1 1 chunk +1 line, -1 line 0 comments Download
M external_tests/ssl_gtest/tls_agent.h View 1 2 3 4 5 6 7 4 chunks +8 lines, -5 lines 0 comments Download
M external_tests/ssl_gtest/tls_agent.cc View 1 2 3 4 5 6 7 8 2 chunks +18 lines, -13 lines 0 comments Download
M external_tests/ssl_gtest/tls_connect.h View 1 2 3 4 2 chunks +10 lines, -3 lines 0 comments Download
M external_tests/ssl_gtest/tls_connect.cc View 1 2 chunks +22 lines, -9 lines 0 comments Download
M lib/ssl/derive.c View 1 2 3 4 5 6 2 chunks +5 lines, -6 lines 0 comments Download
M lib/ssl/dhe-param.c View 5 chunks +10 lines, -5 lines 0 comments Download
M lib/ssl/ssl.h View 1 2 2 chunks +17 lines, -1 line 0 comments Download
M lib/ssl/ssl3con.c View 1 2 3 4 5 6 7 8 40 chunks +644 lines, -549 lines 1 comment Download
M lib/ssl/ssl3ecc.c View 1 2 3 4 5 6 7 8 11 chunks +359 lines, -599 lines 0 comments Download
M lib/ssl/ssl3ext.c View 1 2 3 4 5 7 chunks +111 lines, -60 lines 0 comments Download
M lib/ssl/ssl3prot.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M lib/ssl/sslcert.h View 1 2 3 4 1 chunk +6 lines, -8 lines 0 comments Download
M lib/ssl/sslcert.c View 1 2 3 4 17 chunks +25 lines, -35 lines 0 comments Download
M lib/ssl/sslenum.c View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M lib/ssl/sslimpl.h View 1 2 3 4 5 6 7 8 18 chunks +124 lines, -67 lines 0 comments Download
M lib/ssl/sslinfo.c View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M lib/ssl/sslproto.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M lib/ssl/sslsnce.c View 1 2 3 4 5 2 chunks +5 lines, -3 lines 0 comments Download
M lib/ssl/sslsock.c View 1 2 3 4 5 6 7 8 12 chunks +423 lines, -93 lines 0 comments Download
M lib/ssl/sslt.h View 1 2 3 4 2 chunks +5 lines, -1 line 0 comments Download
M lib/ssl/tls13con.c View 1 2 3 4 5 6 7 8 5 chunks +170 lines, -69 lines 1 comment Download
M lib/util/secoid.c View 1 2 3 1 chunk +15 lines, -0 lines 0 comments Download
M lib/util/secoidt.h View 1 2 3 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 30
ekr-rietveld
I got partway through this, but there seem to be a very large number of ...
7 years, 10 months ago (2016-05-10 09:33:24 UTC) #1
ekr-rietveld
More comments. https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c File lib/ssl/sslenum.c (right): https://codereview.appspot.com/297180043/diff/20001/lib/ssl/sslenum.c#newcode55 lib/ssl/sslenum.c:55: TLS_DHE_PSK_WITH_AES_128_GCM_SHA256, Why does this depend on ECC? ...
7 years, 10 months ago (2016-05-10 09:42:47 UTC) #2
mt
I have a patch ready, but I haven't split out the removal of dead options. ...
7 years, 10 months ago (2016-05-11 12:31:54 UTC) #3
ekr-rietveld
I got partway through sslsock.c. Going to need to pick up the rest tomorrow. https://codereview.appspot.com/297180043/diff/40001/cmd/selfserv/selfserv.c ...
7 years, 10 months ago (2016-05-18 00:44:17 UTC) #4
ekr-rietveld
I got partway through sslsock.c. Going to need to pick up the rest tomorrow.
7 years, 10 months ago (2016-05-18 17:29:10 UTC) #5
ekr-rietveld
NOTE: This patch does not apply to master. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c File lib/ssl/sslsock.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/sslsock.c#newcode1538 lib/ssl/sslsock.c:1538: break; ...
7 years, 10 months ago (2016-05-18 22:57:27 UTC) #6
ekr-rietveld
I continue to believe that we should use ssl3_ uniformly. if you don't lets discuss ...
7 years, 10 months ago (2016-05-18 23:14:38 UTC) #7
mt
I've updated this. I might add some tests later, but it's in a reasonable shape ...
7 years, 10 months ago (2016-05-21 00:38:50 UTC) #8
mt
Shouldn't have rebased, but here the new stuff is here: https://codereview.appspot.com/295300043/
7 years, 10 months ago (2016-05-21 03:30:43 UTC) #9
ekr-rietveld
Dude, what is going on? There's all sorts of totally irrelevant stuff here. Is this ...
7 years, 10 months ago (2016-05-28 18:26:08 UTC) #10
mt
It is a rebase. See my last comment. On 29 May 2016 4:26 AM, <ekr-webrtc@rtfm.com> ...
7 years, 10 months ago (2016-05-28 23:53:42 UTC) #11
ekr-rietveld
OK, I guess I misread the comment. This is going to be a real pain ...
7 years, 10 months ago (2016-05-28 23:56:21 UTC) #12
ekr-rietveld
It looks like something went wrong with the rebase. https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode7308 lib/ssl/ssl3con.c:7308: ...
7 years, 10 months ago (2016-05-29 22:08:26 UTC) #13
mt
https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/40001/lib/ssl/ssl3con.c#newcode9047 lib/ssl/ssl3con.c:9047: #endif On 2016/05/29 22:08:25, ekr-webrtc wrote: > IMPORTANT: Where ...
7 years, 10 months ago (2016-05-30 00:26:43 UTC) #14
mt
https://bugzilla.mozilla.org/show_bug.cgi?id=1276546 https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/297180043/diff/60001/lib/ssl/ssl3con.c#newcode1070 lib/ssl/ssl3con.c:1070: return PR_FALSE; On 2016/05/29 22:08:26, ekr-webrtc wrote: > ...
7 years, 10 months ago (2016-05-30 02:29:37 UTC) #15
ekr-rietveld
This does not apply against origin/master.
7 years, 10 months ago (2016-05-30 15:49:21 UTC) #16
ekr-rietveld
Thanks for rebasing, but this seems to be missing the rest of my review comments. ...
7 years, 10 months ago (2016-05-31 12:22:29 UTC) #17
mt
On 2016/05/31 12:22:29, ekr-webrtc wrote: > Thanks for rebasing, but this seems to be missing ...
7 years, 10 months ago (2016-06-01 01:27:09 UTC) #18
ekr-rietveld
https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/ssl_dhe_unittest.cc#newcode94 external_tests/ssl_gtest/ssl_dhe_unittest.cc:94: client_->CheckErrorCode(SSL_ERROR_WEAK_SERVER_EPHEMERAL_DH_KEY); On 2016/05/29 22:08:25, ekr-webrtc wrote: > Why does ...
7 years, 10 months ago (2016-06-01 03:23:37 UTC) #19
mt
Dammit, I had a long explanation of what changed and lost it... https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File external_tests/ssl_gtest/ssl_dhe_unittest.cc ...
7 years, 10 months ago (2016-06-01 13:02:57 UTC) #20
ekr-rietveld
Do we have agreement on removing the ECC ifdef? This seems pretty close, but I'd ...
7 years, 10 months ago (2016-06-01 18:08:50 UTC) #21
mt
Partial response, I have spent most of my time writing up new tests. https://codereview.appspot.com/297180043/diff/60001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File ...
7 years, 10 months ago (2016-06-02 04:23:57 UTC) #22
mt
I added some more tests. I also discovered that I didn't properly pad all the ...
7 years, 10 months ago (2016-06-02 10:02:18 UTC) #23
ekr-rietveld
LGTM with comments below. https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/120001/external_tests/ssl_gtest/ssl_dhe_unittest.cc#newcode161 external_tests/ssl_gtest/ssl_dhe_unittest.cc:161: // At that point, Y ...
7 years, 10 months ago (2016-06-02 18:50:05 UTC) #24
ekr-rietveld
Unfortunately, this crashes when I run it. Stack trace below. IMPORTANT 1: I see that ...
7 years, 10 months ago (2016-06-02 19:41:16 UTC) #25
mt
In doing this, I discovered that we had bugs in a few of the key ...
7 years, 10 months ago (2016-06-03 02:09:07 UTC) #26
ekr-rietveld
I think this needs another spin https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest/ssl_dhe_unittest.cc#newcode123 external_tests/ssl_gtest/ssl_dhe_unittest.cc:123: return KEEP; You ...
7 years, 10 months ago (2016-06-03 15:44:33 UTC) #27
mt
I restructured the tests so that they are more straightforward. Still not perfect, but much ...
7 years, 10 months ago (2016-06-04 00:19:47 UTC) #28
ekr-rietveld
Partial comments. Will review more tomorrow https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest/ssl_dhe_unittest.cc File external_tests/ssl_gtest/ssl_dhe_unittest.cc (right): https://codereview.appspot.com/297180043/diff/140001/external_tests/ssl_gtest/ssl_dhe_unittest.cc#newcode153 external_tests/ssl_gtest/ssl_dhe_unittest.cc:153: // improbably large, ...
7 years, 10 months ago (2016-06-04 01:06:57 UTC) #29
ekr-rietveld
7 years, 9 months ago (2016-06-04 16:59:56 UTC) #30
MT, I have only reviewed the diffs that correspond to my comments, under the
assumption that everything else is due to the rebase from Kai. Please inform me
if there are any other sections that need review.

https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest...
File external_tests/ssl_gtest/manifest.mn (right):

https://codereview.appspot.com/297180043/diff/160001/external_tests/ssl_gtest...
external_tests/ssl_gtest/manifest.mn:38:
$(DIST)/lib/$(LIB_PREFIX)softokn.$(LIB_SUFFIX)
On 2016/06/04 01:06:57, ekr-webrtc wrote:
> Why was this needed?

Assuming it's Kai?

https://codereview.appspot.com/297180043/diff/160001/lib/ssl/ssl3con.c
File lib/ssl/ssl3con.c (right):

https://codereview.appspot.com/297180043/diff/160001/lib/ssl/ssl3con.c#newcod...
lib/ssl/ssl3con.c:6962: }
Perfect.

https://codereview.appspot.com/297180043/diff/160001/lib/ssl/tls13con.c
File lib/ssl/tls13con.c (right):

https://codereview.appspot.com/297180043/diff/160001/lib/ssl/tls13con.c#newco...
lib/ssl/tls13con.c:529: CKA_DERIVE, tls13_GetHashSize(ss),
Good catch.
Sign in to reply to this message.

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