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

Issue 277090043: Bug 1227905 - Support ChaCha20+Poly1305 cipher suites (Closed)

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

Description

Bug 1227905 - Support ChaCha20+Poly1305 cipher suites

Patch Set 1 #

Patch Set 2 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Total comments: 6

Patch Set 3 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Patch Set 4 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Total comments: 1

Patch Set 5 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Total comments: 1

Patch Set 6 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Patch Set 7 : Rebased and pdated to draft-ietf-tls-chacha20-poly1305-04 #

Total comments: 14

Patch Set 8 : Addressed WTC's comments for patch set #7 #

Total comments: 5

Patch Set 9 : Bug 1227905 - Support ChaCha20+Poly1305 cipher suites #

Patch Set 10 : Restoring a cipher suite accidentally removed from sslcov.txt #

Patch Set 11 : Remove invalid OID #

Patch Set 12 : Rebased on top of the patch from bug 917571 #

Total comments: 1

Patch Set 13 : ssl.sh cleanup wrt CLONG/CSHORT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+152 lines, -22 lines) Patch
M cmd/ssltap/ssltap.c View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M external_tests/ssl_gtest/ssl_loopback_unittest.cc View 1 2 3 4 5 6 2 chunks +36 lines, -0 lines 0 comments Download
M lib/pk11wrap/pk11pars.c View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M lib/ssl/ssl3con.c View 1 2 3 4 5 6 7 8 9 10 11 10 chunks +77 lines, -6 lines 0 comments Download
M lib/ssl/ssl3ecc.c View 1 2 3 4 5 6 4 chunks +4 lines, -0 lines 0 comments Download
M lib/ssl/sslenum.c View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download
M lib/ssl/sslimpl.h View 1 2 3 4 5 6 3 chunks +4 lines, -2 lines 0 comments Download
M lib/ssl/sslinfo.c View 1 2 3 4 5 6 4 chunks +4 lines, -0 lines 0 comments Download
M lib/ssl/sslproto.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M lib/ssl/sslt.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M lib/util/secoid.c View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +2 lines, -1 line 0 comments Download
M lib/util/secoidt.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M tests/ssl/ssl.sh View 1 2 3 4 5 6 7 8 9 10 11 12 3 chunks +6 lines, -12 lines 0 comments Download
M tests/ssl/sslcov.txt View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -0 lines 0 comments Download

Messages

Total messages: 27
mt
This looks fine to me overall. I'm not sure that this is enough test coverage. ...
4 years ago (2015-11-25 22:23:19 UTC) #1
ekr
On Wed, Nov 25, 2015 at 2:23 PM, <martin.thomson@gmail.com> wrote: > This looks fine to ...
4 years ago (2015-11-25 22:27:27 UTC) #2
ttaubert
On 2015/11/25 22:23:19, mt wrote: > I'm not sure that this is enough test coverage. ...
4 years ago (2015-11-30 17:48:07 UTC) #3
ttaubert
Updated cipher suite numbers according to the latest draft at https://tools.ietf.org/html/draft-ietf-tls-chacha20-poly1305-03
4 years ago (2015-12-01 10:16:20 UTC) #4
mt
That looks fine to me. https://codereview.appspot.com/277090043/diff/60001/external_tests/ssl_gtest/ssl_loopback_unittest.cc File external_tests/ssl_gtest/ssl_loopback_unittest.cc (right): https://codereview.appspot.com/277090043/diff/60001/external_tests/ssl_gtest/ssl_loopback_unittest.cc#newcode91 external_tests/ssl_gtest/ssl_loopback_unittest.cc:91: void ConnectSendReceive(PRInt32 cipher_suite) I ...
4 years ago (2015-12-02 04:25:11 UTC) #5
ttaubert
On 2015/12/02 04:25:11, mt wrote: > https://codereview.appspot.com/277090043/diff/60001/external_tests/ssl_gtest/ssl_loopback_unittest.cc#newcode91 > external_tests/ssl_gtest/ssl_loopback_unittest.cc:91: void > ConnectSendReceive(PRInt32 cipher_suite) > I ...
4 years ago (2015-12-02 07:36:32 UTC) #6
ttaubert
On 2015/12/02 07:36:32, ttaubert wrote: > BTW, please ignore the ssl3con.c changes in patch set ...
4 years ago (2015-12-02 07:44:41 UTC) #7
mt
LGTM Lost the last one apparently. https://codereview.appspot.com/277090043/diff/80001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/277090043/diff/80001/lib/ssl/ssl3con.c#newcode2123 lib/ssl/ssl3con.c:2123: } Might be ...
4 years ago (2015-12-08 20:55:50 UTC) #8
ttaubert
On 2015/12/08 20:55:50, mt wrote: > https://codereview.appspot.com/277090043/diff/80001/lib/ssl/ssl3con.c#newcode2123 > lib/ssl/ssl3con.c:2123: } > Might be worth a ...
4 years ago (2015-12-08 21:08:08 UTC) #9
ttaubert
Updated to draft-ietf-tls-chacha20-poly1305-04 and had to rebase due to Bob's changes. It would be good ...
3 years, 11 months ago (2016-01-12 15:57:27 UTC) #10
wtc1
On 2016/01/12 15:57:27, ttaubert wrote: > Updated to draft-ietf-tls-chacha20-poly1305-04 and had to rebase due to ...
3 years, 11 months ago (2016-01-12 18:33:16 UTC) #11
wtc1
Review comments on patch set 7: This isn't a real review. I was just skimming ...
3 years, 11 months ago (2016-01-12 18:34:42 UTC) #12
ttaubert
Thanks for your comments Wan-Teh! I addressed them all in the latest patch set.
3 years, 11 months ago (2016-01-12 19:00:41 UTC) #13
wtc1
Patch set 7 LGTM. I completed a careful review. Although the CL needs work, all ...
3 years, 11 months ago (2016-01-12 19:26:40 UTC) #14
wtc1
Review comments on patch set 8: https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode99 lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, SSL_ALLOWED, ...
3 years, 11 months ago (2016-01-12 19:35:22 UTC) #15
mt
https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c File lib/ssl/ssl3con.c (right): https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode2123 lib/ssl/ssl3con.c:2123: switch (calg) { Tabs => spaces
3 years, 11 months ago (2016-01-13 03:08:44 UTC) #16
ttaubert
On 2016/01/13 03:08:44, mt wrote: > https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode2123 > lib/ssl/ssl3con.c:2123: switch (calg) { > Tabs => ...
3 years, 11 months ago (2016-01-14 14:19:50 UTC) #17
ttaubert
On 2016/01/12 19:35:22, wtc1 wrote: > https://codereview.appspot.com/277090043/diff/140001/lib/ssl/ssl3con.c#newcode99 > lib/ssl/ssl3con.c:99: { TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, > SSL_ALLOWED, PR_TRUE, PR_FALSE}, ...
3 years, 11 months ago (2016-01-14 14:27:33 UTC) #18
ttaubert
On 2016/01/12 19:26:40, wtc1 wrote: > https://codereview.appspot.com/277090043/diff/120001/cmd/ssltap/ssltap.c#newcode448 > cmd/ssltap/ssltap.c:448: case 0x00CCA9: cs_str = > "TLS/DHE-RSA/CHACHA20-POLY1305/SHA256"; ...
3 years, 11 months ago (2016-01-15 14:03:04 UTC) #19
ttaubert
> https://codereview.appspot.com/277090043/diff/120001/lib/util/secoid.c#newcode203 > lib/util/secoid.c:203: CONST_OID chacha20_poly1305[] = { > ALGORITHM, 0x1e }; > > IMPORTANT: ...
3 years, 11 months ago (2016-01-15 14:04:32 UTC) #20
ttaubert
Would it be better to use CONST_OID chacha20_poly1305[] = { NETSCAPE_ALGS, 0x02 }; as long ...
3 years, 11 months ago (2016-01-15 14:39:48 UTC) #21
wtc1
Tim: could you please upload a new patch set, so I can review how you ...
3 years, 11 months ago (2016-01-15 18:19:56 UTC) #22
wtc1
On 2016/01/15 14:04:32, ttaubert wrote: > > So the problem is that it seems as ...
3 years, 11 months ago (2016-01-15 18:27:12 UTC) #23
ttaubert
On 2016/01/15 18:27:12, wtc1 wrote: > On 2016/01/15 14:04:32, ttaubert wrote: > > > > ...
3 years, 11 months ago (2016-01-16 12:35:03 UTC) #24
wtc1
Patch set 12 LGTM! https://codereview.appspot.com/277090043/diff/220001/tests/ssl/ssl.sh File tests/ssl/ssl.sh (right): https://codereview.appspot.com/277090043/diff/220001/tests/ssl/ssl.sh#newcode93 tests/ssl/ssl.sh:93: CLONG="-c ABCDEF:C001:C002:C003:C004:C005:C006:C007:C008:C009:C00A:C00B:C00C:C00D:C00E:C00F:C010:C011:C012:C013:C014:C023:C027:C02B:C02F:CCA8:CCA9:CCAA:0016:0032:0033:0038:0039:003B:003C:003D:0040:0041:0067:006A:006B:0084:009C:009E:00A2cdefgijklmnvyz" Perhaps the definition ...
3 years, 10 months ago (2016-01-20 22:04:20 UTC) #25
ttaubert
On 2016/01/20 22:04:20, wtc1 wrote: > Perhaps the definition of CSHORT and CLONG should be ...
3 years, 10 months ago (2016-01-21 13:42:34 UTC) #26
wtc1
3 years, 10 months ago (2016-01-21 18:45:48 UTC) #27
Patch set 13 LGTM. I like your solution very much!
Sign in to reply to this message.

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