Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(44)

Issue 9764001: Add DTLS support to NSS, contributed by Eric Rescorla. (Closed)

Created:
8 years, 9 months ago by wtc
Modified:
8 years, 8 months ago
Reviewers:
ekr, Ryan Sleevi
CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org, agl
Visibility:
Public.

Description

Add DTLS support to NSS, contributed by Eric Rescorla. R=rsleevi@chromium.org BUG=120938 TEST=none Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=129778

Patch Set 1 #

Total comments: 34

Patch Set 2 : Made one pass, dtls1con.c not reviewed yet #

Total comments: 18

Patch Set 3 : Made a second pass, reviewed dtls1con.c only for coding style #

Total comments: 48

Patch Set 4 : Made changes based on comments from EKR #

Total comments: 8

Patch Set 5 : Made changes suggested by rsleevi #

Total comments: 6

Patch Set 6 : Made the second round of changes suggested by rsleevi #

Total comments: 2

Patch Set 7 : Third round of changes suggested by ekr and rsleevi #

Patch Set 8 : Remove an extra 'the' #

Total comments: 15

Patch Set 9 : Add the dtls.patch file #

Patch Set 10 : Update AUTHORS #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5436 lines, -127 lines) Patch
M AUTHORS View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M net/third_party/nss/README.chromium View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M net/third_party/nss/patches/applypatches.sh View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
A net/third_party/nss/patches/dtls.patch View 1 2 3 4 5 6 7 8 1 chunk +3321 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl.gyp View 1 chunk +1 line, -0 lines 0 comments Download
M net/third_party/nss/ssl/SSLerrs.h View 1 1 chunk +6 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/derive.c View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
A net/third_party/nss/ssl/dtls1con.c View 1 2 3 4 5 6 7 8 1 chunk +1163 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/manifest.mn View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/third_party/nss/ssl/ssl.h View 1 2 chunks +14 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/ssl3con.c View 1 2 3 4 5 6 7 71 chunks +487 lines, -97 lines 0 comments Download
M net/third_party/nss/ssl/ssl3gthr.c View 1 2 3 4 5 6 7 8 6 chunks +156 lines, -3 lines 0 comments Download
M net/third_party/nss/ssl/ssl3prot.h View 1 2 chunks +4 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslcon.c View 1 2 chunks +7 lines, -2 lines 0 comments Download
M net/third_party/nss/ssl/ssldef.c View 1 1 chunk +5 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslerr.h View 1 1 chunk +3 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslgathr.c View 1 2 3 4 5 2 chunks +3 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslimpl.h View 1 2 3 4 17 chunks +149 lines, -2 lines 0 comments Download
M net/third_party/nss/ssl/sslproto.h View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslsecur.c View 1 1 chunk +1 line, -0 lines 0 comments Download
M net/third_party/nss/ssl/sslsock.c View 1 2 3 4 19 chunks +99 lines, -22 lines 0 comments Download
M net/third_party/nss/ssl/sslt.h View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 35 (0 generated)
wtc
ekr: Patch set 1 is your original NSS patch. I have reviewed everything except dtls1con.c ...
8 years, 9 months ago (2012-03-21 01:22:07 UTC) #1
ekr
Thanks for the careful review and catching this stuff. My comments below. What's the best ...
8 years, 9 months ago (2012-03-21 01:36:40 UTC) #2
wtc
On 2012/03/21 01:36:40, ekr wrote: > What's the best way to proceed? Should I generate ...
8 years, 9 months ago (2012-03-21 01:46:43 UTC) #3
wtc
https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c#newcode2889 net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); On 2012/03/21 01:36:40, ekr wrote: > > IIRC ...
8 years, 9 months ago (2012-03-21 02:12:01 UTC) #4
wtc
https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): https://chromiumcodereview.appspot.com/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c#newcode2889 net/third_party/nss/ssl/ssl3con.c:2889: ss->sec.uncache(ss->sec.ci.sid); I found that this crash was reported in ...
8 years, 9 months ago (2012-03-21 02:15:22 UTC) #5
wtc
ekr: I uploaded patch set 2. You can click the underlined "1" links to review ...
8 years, 9 months ago (2012-03-21 02:40:46 UTC) #6
ekr
OK. I'm in the air most of today but I'll try to find some wireless ...
8 years, 9 months ago (2012-03-21 13:55:53 UTC) #7
wtc
On 2012/03/21 13:55:53, ekr wrote: > OK. I'm in the air most of today but ...
8 years, 9 months ago (2012-03-21 16:38:06 UTC) #8
wtc
ekr: I have some questions for you. If you are busy, just answer the first ...
8 years, 9 months ago (2012-03-21 23:12:15 UTC) #9
ekr
Just have a few minutes on the ground in Newark. Will get this either on ...
8 years, 9 months ago (2012-03-21 23:56:24 UTC) #10
wtc
rsleevi: please review this CL. agl: just FYI. This is a big patch. I have ...
8 years, 9 months ago (2012-03-22 01:11:42 UTC) #11
ekr
http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.h File net/third_party/nss/ssl/ssl.h (right): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl.h#newcode955 net/third_party/nss/ssl/ssl.h:955: SSL_IMPORT SECStatus DTLS_GetTimeout(PRFileDesc *socket, On 2012/03/21 23:12:16, wtc wrote: ...
8 years, 9 months ago (2012-03-22 15:07:17 UTC) #12
wtc
ekr: thank you for answering my questions! http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (left): http://codereview.chromium.org/9764001/diff/9003/net/third_party/nss/ssl/ssl3con.c#oldcode2819 net/third_party/nss/ssl/ssl3con.c:2819: pwSpec->write_seq_num.low = ...
8 years, 9 months ago (2012-03-22 20:09:20 UTC) #13
wtc
rsleevi: I have done enough checking to convince myself that this patch is safe if ...
8 years, 9 months ago (2012-03-22 21:59:53 UTC) #14
Ryan Sleevi
http://codereview.chromium.org/9764001/diff/19001/AUTHORS File AUTHORS (right): http://codereview.chromium.org/9764001/diff/19001/AUTHORS#newcode174 AUTHORS:174: Eric Rescorla <ekr@rtfm.com> Verified the CLA. Looks good. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/derive.c ...
8 years, 9 months ago (2012-03-22 22:26:36 UTC) #15
wtc
rsleevi: thank you for the review! Please review patch set 5. I made all the ...
8 years, 9 months ago (2012-03-23 00:21:18 UTC) #16
Ryan Sleevi
Thanks for the changes, wtc. I'm still not entirely convinced about the dtlsPacket.len, but it's ...
8 years, 9 months ago (2012-03-23 00:38:28 UTC) #17
wtc
ekr: one more question for you. http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl3con.c#newcode10080 net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0; ...
8 years, 9 months ago (2012-03-23 02:10:43 UTC) #18
ekr
Thanks so much for the thorough review. It's really great to have people do such ...
8 years, 9 months ago (2012-03-23 12:46:40 UTC) #19
wtc
http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl3con.c#newcode1223 net/third_party/nss/ssl/ssl3con.c:1223: (suite_def->bulk_cipher_alg != cipher_rc4_56)); I think the use of PORT_Assert ...
8 years, 9 months ago (2012-03-23 13:48:49 UTC) #20
wtc
rsleevi: please review patch set 6. I made all the changes you suggested except the ...
8 years, 9 months ago (2012-03-23 22:01:04 UTC) #21
ekr
Had no network today and too tired to do anything useful now. Will check out ...
8 years, 9 months ago (2012-03-23 22:04:52 UTC) #22
ekr
On Fri, Mar 23, 2012 at 2:48 PM, <wtc@chromium.org> wrote: > http://codereview.chromium.org/9764001/diff/28001/net/third_party/nss/ssl/ssl3con.c > File net/third_party/nss/ssl/ssl3con.c ...
8 years, 9 months ago (2012-03-24 09:10:14 UTC) #23
Ryan Sleevi
Changes LGTM. Per your request, I have not reviewed the DLTS implementation. http://codereview.chromium.org/9764001/diff/19001/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c ...
8 years, 9 months ago (2012-03-26 19:11:55 UTC) #24
wtc
rsleevi: thanks for the review. Patch set 8 has all the changes you suggested. I ...
8 years, 9 months ago (2012-03-26 19:52:00 UTC) #25
ekr
Willdo. Right now I'm trying to track down some memory complaints Valgrind is giving me. ...
8 years, 9 months ago (2012-03-26 19:54:20 UTC) #26
Ryan Sleevi
On 2012/03/26 19:54:20, ekr wrote: > Willdo. Right now I'm trying to track down some ...
8 years, 9 months ago (2012-03-26 20:02:32 UTC) #27
ekr
Thanks for the heads-up... Turns out that building valgrind from SVN head removes the error. ...
8 years, 9 months ago (2012-03-26 20:33:04 UTC) #28
ekr
These generally look right. I'm still diagnosing why I'm seeing sporadic errors in DTLS in ...
8 years, 9 months ago (2012-03-26 21:49:18 UTC) #29
wtc
Ekr: thanks for your comments. I responded to them below. http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c File net/third_party/nss/ssl/ssl3con.c (right): http://codereview.chromium.org/9764001/diff/1/net/third_party/nss/ssl/ssl3con.c#newcode230 ...
8 years, 9 months ago (2012-03-27 00:28:25 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9764001/49003
8 years, 8 months ago (2012-03-29 22:11:13 UTC) #31
commit-bot: I haz the power
Try job failure for 9764001-49003 (retry) on mac_rel for step "browser_tests". It's a second try, ...
8 years, 8 months ago (2012-03-30 00:05:49 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/wtc@chromium.org/9764001/49003
8 years, 8 months ago (2012-03-30 01:55:04 UTC) #33
commit-bot: I haz the power
Change committed as 129778
8 years, 8 months ago (2012-03-30 03:50:59 UTC) #34
ekr
8 years, 8 months ago (2012-03-30 04:28:43 UTC) #35
http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl...
File net/third_party/nss/ssl/ssl3con.c (right):

http://codereview.chromium.org/9764001/diff/31004/net/third_party/nss/ssl/ssl...
net/third_party/nss/ssl/ssl3con.c:10080: ss->ssl3.hs.sendMessageSeq = 0;
On 2012/03/27 00:28:25, wtc wrote:
> 
> On 2012/03/26 21:49:18, ekr wrote:
> > Conveniently, I already solved this problem and have a DTLS function for it,
> so
> > we can extend it. Patch below.
> > 
> > As you can imagine, I basically haven't tested rehandshake for DTLS yet.
> > 
> > 
> > --- a/mozilla/security/nss/lib/ssl/dtls1con.c
> > +++ b/mozilla/security/nss/lib/ssl/dtls1con.c
> > @@ -935,6 +935,7 @@ dtls_RehandshakeCleanup(sslSocket *ss)
> >  {
> >         dtls_CancelTimer(ss);
> >         ssl3_DestroyCipherSpec(ss->ssl3.pwSpec, PR_FALSE);
> > +       ss->ssl3.hs.sendMessageSeq = 0;
> >  }
> >  
> >  /* Set the MTU to the next step less than or equal to the 
> 
> Should dtls_RehandshakeCleanup also set
> ss->ssl3.hs.recvMessageSeq to 0?

Yes, I think it should.

Powered by Google App Engine
This is Rietveld 408576698