681065 - (dtls) Implement DTLS (Datagram TLS) in libssl

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Comment 1

[Eric Rescorla (:ekr)]

Attachment: First cut at a partial DTLS implementation: for early review

Comment 2

[Eric Rescorla (:ekr)]

Attachment: New functions for DTLS: for early review

Comment 3

[Eric Rescorla (:ekr)]

These patches were submitted for review of implementation strategy and
style, not for consideration for addition to NSS as-is.

The attached patches (as of 2008-08-24) implement the following:

As I discussed with each of you privately, I've been working on DTLS
support for NSS. I'm nowhere near done, but I thought it would be good
to have a preliminary chat about the process for integration.  So far,
I've got support for:

(1) The new record format including explicit IV.
(2) Most of the TLS/not-TLS tests are adapted to DTLS appropriately.
(3) The new handshake message structure.
(4) Handshake message reassembly but not fragmentation.

When you put these together, you can interoperate with a lightly
modified (to remove the Hello Verify) OpenSSL as long as the MTU is
nice and large and there is no packet loss, so it's a start. Next
step is the fragmentation and retransmission logic on the sender
side.

There are a lot of known missing pieces even within that boundary.
Many though perhaps not all are marked with TODO: EKR. I plan to
remove all of these or convert to proper TODOs before submitting 
these patches for real approval.

Comment 4

[Eric Rescorla (:ekr)]

Oops. 2011-08-24

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Eric, please resubmit the patches using cvs diff -u10p format. I think this will enable the "Splinter Review" mechanism to work.

The patch is not as big as I was expecting. I think we should build it on top of the TLS 1.1 implementation (bug 565047). Let's discuss the explicit IV handling in that bug (e.g. whether we really need to use PK11_GenerateRecord to generate the IV, or if we can use the sequence number instead.)

Comment 6

[Eric Rescorla (:ekr)]

Attachment: Revised patch with the right diff flags.

Comment 7

[Eric Rescorla (:ekr)]

Yeah, I had thought it was bigger when we first talked--and of course there is a bunch more work to do which
will make it bigger.

I'll take a look at the explicit IV handling in 565047--it's an orthogonal issue, mostly, so we can chase these in parallel just drop that code in here when we're happy with it.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Eric, some quick thoughts:

1. For DTLS, we should make ss->version be the decoded version of the DTLS version number, so that we don't need to add a "|| IS_DTLS(ss)" condition to every version range check. For example, ss->version would be the same for TLS 1.0 as it is for DTLS 1.0, and the same for TLS 1.2 as it is for DTLS 1.2. This will reduce the noise in the patch considerably.

2. We should definitely implement TLS 1.1 first and then merge this patch on top of it, so that the explicit IV is factored out.

3. I do not think we need an ENABLE_DTLS option. Instead, we can just assume that if you are using a TCP socket, you want TLS, and if you are using UDP, you want DTLS. And, when we've decided that, we can disable incompatible cipher suites and incompatible options (e.g. SSL 2 compatible hello) automatically internally when the first handshake gets kicked off.

4. What do you think we should do, long-term, regarding the memory allocation (dtls_AllocHandshakeMessage et al)? How many fragments should we be expecting to buffer, generally? My first reaction is that we should be avoiding these allocations, using an arena or a buffer that is allocated/grown with ssl_BufferGrow--especially since only handshake records can be fragmented (IIRC). But, maybe that won't be necessary. More importantly, we should bound the number of fragments we accept to avoid memory exhaustion. I do not know what is a reasonable limit yet.

Comment 9

[Eric Rescorla (:ekr)]

1. I did consider that avenue and decided against it, so let me explain my reasoning:

(a) This is a version-orthogonal question. I.e., many of the things you do for dtls
you do regardless of the version, so it's not like you could accidentally negotiate DSSL 3.0
and want to do SSL 3.0ish things. And, indeed, there are places in which DTLS 1.0
acts more like TLS 1.1, as you know.

(b) From a technical perspective, DTLS 1.0's version number is actually 1.0 rather than 
3.1 as with TLS 1.0. So, while ss->version actually *is* set to 1.0, this causes the
stack to constantly get confused with each of these tests. So, we'd need to create
a synthetic version number we stuffed into ss->version


(2) Agreed.

(3) My reasoning here was to try to keep the TLS stack stupid about the underlying transport,
rather than having to interrogate it. And there certainly are legitimate reasons why you
might wish to run DTLS over a stream transport, though not vice versa. For instance,
say you have a TCP connection to a relay.

(4) My basic thought was to use the strategy you see here but to perhaps prune the list of
fragments for packets which totally duplicate existing data. In general, then, you can't
be forced to allocate >> 1 handshake record, which is what you could be forced to allocate
directly.  I'd certainly be happy to have a pool of these, but I wonder if it makes that much
of a difference performance-wise.  I generally try to avoid having a big buffer which you
fill in bit-by-bit. My experience with reassembly systems of this kind is that it's rather
harder to get that code right. But if people feel differently, I'm willing to give it a crack.

Comment 10

[Robert Relyea]

Comment on attachment 555585 [details] [diff] [review]
New functions for DTLS: for early review

move feedback to the more 'readable' diff...

Comment 11

[Robert Relyea]

Comment on attachment 555639 [details] [diff] [review]
Revised patch with the right diff flags.

feedback+ rrelyea (NOTE: this is not an r+).

I like both the idea and the general approach in this patch.

I think Brian has a good point about versioning, but I don't think it's versioning so much as we need to split out the meaning of the ubiquitous 'isTLS'. It seems to me we need several new booleans in the ssl structure:

isTLS
isDTLS
isIETF (that is is not SSL, effectively isTLS|isDTLS)

There is probably some other features which need to be put in the ssl structure.... 

keyderive mechanisms (SSL, TLS 1.0, TLS 1.2)
send_random_data_on_first_block (set to 1 for TLS 1.1 or greather, and DTLS).
probably a few others (usually 1.1 and 1.2, particularly if the get picked up in new versions of DTLS).

I think it's easier to set all those features once when we negotiate versions rather then have code that look like:

if (!IS_DTLS(ss) && ss->version <= SSL_LIBRARY_VERSION_3_0) {
    /* do SSL thing */
} else if ((!IS_DTLS(ss) && ss->version <= SSL_LIBRARY_VERSION_3_2) ||
       (IS_DTLS(ss) && ss->version <= DTLS_VERISON_1_0)) {
    /* do TLS 1.0/1.1/ DTLS 1.0 thing */
} else if ((!IS_DTLS(ss) && ss->version < SSL_LIBRARY_VERSION_3_3) ||
       (IS_DTLS(ss) && ss->version <= DTLS_VERISON_1_1)) {
    /* do TLS 1.2/ DTLS 1.0 thing */
} else {
    ....
}

Note: technically we need to qualify the type with the version, even though it doesn't necessarily bite us now, for the day that eventually DTLS and TLS version numbers begin to overlap...

bob

Comment 12

[Robert Relyea]

I'm OK with keeping the ENABLE_DTLS flag, but I agree with brian that it shouldn't be necessary by default. We should set ENABLE_DTLS automatically if the underlying file is UDP (which we can get from NSPR).

Comment 13

[Eric Rescorla (:ekr)]

Trying to think about the best way to handle the versioning issue...

DTLS is defined as a delta off of TLS:

- DTLS 1.0 (wire version 254, 255) is a delta of TLS 1.1 (wire version 3, 2)
- DTLS 1.2 (wire version 254, 253) is a delta of TLS 1.2 (wire version 3, 3)

This is likely to be the plan for the foreseeable future. 

So, one way to conceptualize this would be as:

1. TLS base version
2. DTLS versus TLS

Which gives us the following table:

Base       TLS      DTLS
------------------
2            SSLv2        -
3.0         SSLv3        -
3.1         TLS 1.0     -
3.2         TLS 1.1   DTLS 1.1
3.3         TLS 1.2   DTLS 1.2

Another would be (as you suggest) to have a bunch of individual params
for each point of variation (IV handling, PRF, etc., either as booleans
(simpler) or as function pointers (arguably more flexible)

Anyway, mostly just thinking out loud....

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Here's +1 for the table in comment 13.  I'd make ss->version be that "Base" version.

Bob, instead of isIETF, how about isSSL, which is equivalent to (!isIETF).

Comment 15

[Robert Relyea]

> Bob, instead of isIETF, how about isSSL, which is equivalent to (!isIETF).

Yes, That's probably easier to understand.

Comment 16

[Eric Rescorla (:ekr)]

Attachment: Revised patch. This still has a conflict with the TLS 1.1 and versioning work

Comment 17

[Maire Reavy [:mreavy]]

Brian and Eric -- we want to move this bug fix forward toward landing since this will become a blocker for webrtc work in the near future.  Eric just posted a patch, but there's still work to be done.  Who needs to do this work?  What are the next steps?

Comment 18

[Eric Rescorla (:ekr)]

I believe the status is that this needs review (I've asked Brian and Wan-Teh to take a look) and then I will no doubt have to drop a new patch to fix a bunch of things they find. Then we have to merge in the versioning and TLS 1.1 stuff and rebase this patch on top of it. I'm happy to help with those other patches if that helps.

P.S. I've been porting libjingle to run on NSS and my unit tests exposed a few problems with the logic for handling packet loss and packet truncation. Will submit a new patch soonish, but that shouldn't block initial reviews since there are no doubt bigger problems.

Comment 19

[Maire Reavy [:mreavy]]

Comment on attachment 594603 [details] [diff] [review]
Revised patch. This still has a conflict with the TLS 1.1 and versioning work

Per ekr's last comment, marking for review by bsmith and wtc.

Comment 20

[Wan-Teh Chang]

Comment on attachment 594603 [details] [diff] [review]
Revised patch. This still has a conflict with the TLS 1.1 and versioning work

Status update: I have read everything in this patch except the
dtlsreplay.c test program.  I gave my review comments and suggested
changes to EKR in a series of face-to-face meetings.

Comment 21

[Wan-Teh Chang]

Re: ss->version and isDTLS/isTLS/isSSL

I also recommended to EKR that we make ss->version the decoded TLS
base version.

I am less sure about adding isDTLS/isTLS/isSSL booleans to the ssl
structure.  Many functions do compute isTLS as a local variable,
so adding these booleans to the ssl structure will save work.

Note: in almost all cases the isTLS local variable will also apply to
DTLS.  So that isTLS variable should ideally be replaced by an isSSL
variable, with the opposite condition.

Re: ENABLE_DTLS

This option is necessary because the NSPR I/O layer below the
SSL socket may be a PRFileDesc wrapper around a third-party I/O
library, rather than an NSPR UDP socket.

Comment 22

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 594603 [details] [diff] [review]
Revised patch. This still has a conflict with the TLS 1.1 and versioning work

Clearing review request. I will look at the next version of the patch.

Comment 23

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Some thoughts, not based on looking at the patch, but from doing the work to implement TLS 1.1 and to implement the version API:

1. There are some assumptions made about how servers vs. how clients work. See bug 565047 comment 76 for one example (DTLS makes cipher suite selection even more dynamic, because it now is also a function of the variant of TLS being used). Since we are now in the situation where clients act as servers, all of these assumptions need to be reviewed. This means we need to go over all the cases where isServer is checked, as well as all the code for sending server-specific messages (e.g. ssl3_SendServerHello) and for receiving client-specific messages (e.g. ssl3_HandleClientHello).

2. There may or may not be some assumptions made about how dynamic server configuration is--i.e. assumptions that certain settings won't change because no reasonable product/sysadmin would change them for a (production) server. We should be on the lookout for such assumptions.

3. Does it make sense for a DTLS connection to resume a session that was negotiated over stream TLS, and vice versa? Ideally, we would be able to enable this optimization. But, I am not sure yet what problems it might cause. At a minimum, we have to make sure that a cached session's parameters are appropriate for the new connection (e.g. do not resume a stream TLS session that uses cipher suites disallowed in DTLS).

4. Is the session cache implementation for the server part of libssl acceptable for clients to use. I remember that it requires different configuration and it may be optimized differently. I do not know all the details here. I know the client-side cache has features for partitioning the session cache that I think the server-side cache does not have. For example, an application may partition the session cache into "non-private-browsing-mode sessions" and "private-browsing-mode-sessions". We should make sure this is handled sensibly for the server role of a connection.

Comment 24

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

In particular, we need to make sure the ECC configuration makes sense for the server-specific code--in particular, that the use of NSS_ENABLE_ECC vs. the NSS_ECC_MORE_THAN_SUITE_B makes sense for a server when NSS_ENABLE_ECC is defined without NSS_ECC_MORE_THAN_SUITE_B.

Comment 25

[Eric Rescorla (:ekr)]

Attachment: Revised patch

This patch has been revised to address the extensive issues Wan-Teh found in his thorough review. I just finished the last TODO, so I intend to re-review it with fresh eyes tomorrow morning, but I believe it is again ready for an independent review.

Comment 26

[Eric Rescorla (:ekr)]

Attachment: Slightly revised patch

Comment 27

[Eric Rescorla (:ekr)]

Attachment: Cleanup of some comments, formatting, and added a call to set error

Also. added dtls1con.c, which didn't make it into 606480

Comment 28

[Eric Rescorla (:ekr)]

Attachment: Updated comments in dtls1con.c

Comment 29

[Eric Rescorla (:ekr)]

Attachment 607007 [details] [diff] should not contain any technical changes.

Comment 30

[Wan-Teh Chang]

Comment on attachment 607007 [details] [diff] [review]
Updated comments in dtls1con.c

Review of attachment 607007 [details] [diff] [review]:
-----------------------------------------------------------------

ekr: I am reviewing the changes to mozilla/security/nss/lib/ssl
using http://codereview.chromium.org/9764001/.

Here are my review comments on the changes to mozilla/security/nss/cmd
(dtlsreplay.c and tstclnt.c).

::: cmd/tests/dtlsreplay.c
@@ +33,5 @@
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +

Please add a comment to explain what this test program does.

@@ +58,5 @@
> +	seq = i;
> +	way_left = PR_FALSE;
> +
> +	/* drop */
> +	if(!(seq % 7))

Add a space between "if" and "(".  Fix all occurrences in this file.

@@ +85,5 @@
> +	    }
> +	    dtls_RecordSetRecvd(&records, seq);
> +	    replay_table[seq] = 1;
> +	}
> +	else if (result == 1) {

Put "}" and "else" on the same line.  Fix all occurrences in this file.

::: cmd/tstclnt/tstclnt.c
@@ +227,5 @@
>      fprintf(stderr, "%-20s Renegotiate N times (resuming session if N>1).\n", "-r N");
>      fprintf(stderr, "%-20s Enable the session ticket extension.\n", "-u");
>      fprintf(stderr, "%-20s Enable compression.\n", "-z");
>      fprintf(stderr, "%-20s Enable false start.\n", "-g");
> +    fprintf(stderr, "%-20s DTLS.\n", "-D");

Add "Use" before "DTLS".

@@ +744,5 @@
>  	/* disable all the ciphers, then enable the ones we want. */
>  	disableAllSSLCiphers();
>      }
> +    
> +    memset(&addr, 0, sizeof(addr));

This is a good change.

Just curious: did something fail, or you just thought this is a
good thing to do?

@@ +835,5 @@
> +
> +	for (;;) {
> +	    /* Bind the remote address on first packet. This must happen
> +	       before we SSL-ize the socket because we want 
> +	       the socket RecvFrom */

Please format this comment block as follows:
    /* line 1
     * line 2
     * line 3 */

I don't understand what you meant by "because we want the
socket RecvFrom".  It sounds like an incomplete sentence.
I guess you meant RecvFrom does not work on an SSL socket?

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ssl/sslsock.c&rev=1.86&mark=2442,2446-2447#2441

Perhaps we should fix that.

@@ +845,5 @@
> +			     &remote, PR_INTERVAL_NO_TIMEOUT);
> +	    if (nb != 1)
> +		continue;
> +
> +	    status = PR_Connect(s, &remote, PR_INTERVAL_NO_TIMEOUT);

Why do we need to call PR_RecvFrom in a loop?  Is it
because the socket is non-blocking?  If so, it would
be better to use PR_Poll to wait until the socket is
readable, rather than using a busy loop.

Nit: we can call PR_Connect outside this for loop.

@@ +1023,5 @@
> +				   kt_rsa) != SECSuccess) {
> +	    return 1;
> +	}
> +	SECKEY_DestroyPrivateKey(privKey);
> +	CERT_DestroyCertificate(cert);

Nit: it'd be nice to turn this block of server setup code
into a local function.

@@ +1034,5 @@
>          SSL_SetURL(s, host);
>      }
>  
>      /* Try to connect to the server */
> +    if (!actAsServer) {

Reverse these two blocks because the actAsServer block is
much shorter.

@@ +1094,5 @@
> +    pollset[SSOCK_FD].in_flags  = PR_POLL_EXCEPT;
> +    if (!actAsServer)
> +	pollset[SSOCK_FD].in_flags |= (clientSpeaksFirst ? 0 : PR_POLL_READ);
> +    else
> +	pollset[SSOCK_FD].in_flags |= PR_POLL_READ;

When acting as a server, should it also honor the clientSpeaksFirst
setting?

Comment 31

[Wan-Teh Chang]

Attachment: EKR's patch for lib/ssl

This is EKR's patch for lib/ssl, updated to the current CVS HEAD.
You can compare it with what I checked in.

Comment 32

[Wan-Teh Chang]

Attachment: EKR's patch for cmd

This contains the new unit test dtlsreplay.c and changes
to tstclnt.c.

Comment 33

[Wan-Teh Chang]

Attachment: EKR's patch for lib/ssl (as checked in)

The code review for EKR's patch for lib/ssl was conducted
using the Rietveld code review tool at
http://codereview.chromium.org/9764001/.

This patch is what I checked in.  I excluded two changes
that are independent of DTLS:
- Allow an empty certificate_authorities list in
  ssl3_SendCertificateRequest (bug 742162)
- Fix a crash in ssl3_HandleAlert (bug 540535)

Patch checked in on the NSS trunk (NSS 3.13.5).

Checking in SSLerrs.h;
/cvsroot/mozilla/security/nss/lib/ssl/SSLerrs.h,v  <--  SSLerrs.h
new revision: 1.21; previous revision: 1.20
done
Checking in derive.c;
/cvsroot/mozilla/security/nss/lib/ssl/derive.c,v  <--  derive.c
new revision: 1.14; previous revision: 1.13
done
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/dtls1con.c,v
done
Checking in dtls1con.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtls1con.c,v  <--  dtls1con.c
initial revision: 1.1
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.21; previous revision: 1.20
done
Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.34; previous revision: 1.33
done
Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.55; previous revision: 1.54
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.176; previous revision: 1.175
done
Checking in ssl3gthr.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3gthr.c,v  <--  ssl3gthr.c
new revision: 1.13; previous revision: 1.12
done
Checking in ssl3prot.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3prot.h,v  <--  ssl3prot.h
new revision: 1.21; previous revision: 1.20
done
Checking in sslcon.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslcon.c,v  <--  sslcon.c
new revision: 1.49; previous revision: 1.48
done
Checking in ssldef.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssldef.c,v  <--  ssldef.c
new revision: 1.12; previous revision: 1.11
done
Checking in sslerr.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslerr.h,v  <--  sslerr.h
new revision: 1.22; previous revision: 1.21
done
Checking in sslgathr.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslgathr.c,v  <--  sslgathr.c
new revision: 1.14; previous revision: 1.13
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.101; previous revision: 1.100
done
Checking in sslproto.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslproto.h,v  <--  sslproto.h
new revision: 1.18; previous revision: 1.17
done
Checking in sslsecur.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsecur.c,v  <--  sslsecur.c
new revision: 1.59; previous revision: 1.58
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.87; previous revision: 1.86
done
Checking in sslt.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslt.h,v  <--  sslt.h
new revision: 1.21; previous revision: 1.20
done

Comment 34

[Wan-Teh Chang]

Attachment: Export the new DTLS_GetTimeout function

Patch checked in on the NSS trunk (NSS 3.13.5).

Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.35; previous revision: 1.34
done

Comment 35

[Wan-Teh Chang]

Attachment: Rename dtls1con.c to dtlscon.c

Removing dtls1con.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtls1con.c,v  <--  dtls1con.c
new revision: delete; previous revision: 1.1
done
RCS file: /cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v
done
Checking in dtlscon.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v  <--  dtlscon.c
initial revision: 1.1
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.22; previous revision: 1.21
done

Comment 36

[Eric Rescorla (:ekr)]

(In reply to Wan-Teh Chang from comment #30)
> @@ +744,5 @@
> >  	/* disable all the ciphers, then enable the ones we want. */
> >  	disableAllSSLCiphers();
> >      }
> > +    
> > +    memset(&addr, 0, sizeof(addr));
> 
> This is a good change.
> 
> Just curious: did something fail, or you just thought this is a
> good thing to do?

Something failed, though I don't remember what. Basically, it's
not safe not to do this :)


> @@ +835,5 @@
> > +
> > +	for (;;) {
> > +	    /* Bind the remote address on first packet. This must happen
> > +	       before we SSL-ize the socket because we want 
> > +	       the socket RecvFrom */
> 
> Please format this comment block as follows:
>     /* line 1
>      * line 2
>      * line 3 */
> 
> I don't understand what you meant by "because we want the
> socket RecvFrom".  It sounds like an incomplete sentence.
> I guess you meant RecvFrom does not work on an SSL socket?

No, what I mean is that we want to see the address but not
process the packet. I rewrote the comment.

> @@ +845,5 @@
> > +			     &remote, PR_INTERVAL_NO_TIMEOUT);
> > +	    if (nb != 1)
> > +		continue;
> > +
> > +	    status = PR_Connect(s, &remote, PR_INTERVAL_NO_TIMEOUT);
> 
> Why do we need to call PR_RecvFrom in a loop?  Is it
> because the socket is non-blocking?  If so, it would
> be better to use PR_Poll to wait until the socket is
> readable, rather than using a busy loop.
> 
> Nit: we can call PR_Connect outside this for loop.

No, it's blocking. IIRC I was seeing empty records on the socket
for some reason and I decided this was the easiest approach.
I can remove the loop if you like.


> @@ +1023,5 @@
> > +				   kt_rsa) != SECSuccess) {
> > +	    return 1;
> > +	}
> > +	SECKEY_DestroyPrivateKey(privKey);
> > +	CERT_DestroyCertificate(cert);
> 
> Nit: it'd be nice to turn this block of server setup code
> into a local function.

Willdo.


> @@ +1034,5 @@
> >          SSL_SetURL(s, host);
> >      }
> >  
> >      /* Try to connect to the server */
> > +    if (!actAsServer) {
> 
> Reverse these two blocks because the actAsServer block is
> much shorter.

No longer necessary.


> @@ +1094,5 @@
> > +    pollset[SSOCK_FD].in_flags  = PR_POLL_EXCEPT;
> > +    if (!actAsServer)
> > +	pollset[SSOCK_FD].in_flags |= (clientSpeaksFirst ? 0 : PR_POLL_READ);
> > +    else
> > +	pollset[SSOCK_FD].in_flags |= PR_POLL_READ;
> 
> When acting as a server, should it also honor the clientSpeaksFirst
> setting?

I'm not sure what it's for. Doesn't the client speak first in DTLS all the time?

Comment 37

[Wan-Teh Chang]

Attachment: Rename DTLS_GetTimeout to DTLS_GetHandshakeTimeout

Ekr: as we previously discussed in email.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in dtlscon.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v  <--  dtlscon.c
new revision: 1.2; previous revision: 1.1
done
Checking in ssl.def;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.def,v  <--  ssl.def
new revision: 1.38; previous revision: 1.37
done
Checking in ssl.h;
/cvsroot/mozilla/security/nss/lib/ssl/ssl.h,v  <--  ssl.h
new revision: 1.58; previous revision: 1.57
done

Comment 38

[Wan-Teh Chang]

Attachment: Replaced hardcoded ssl_variant_stream with ss->protocolVariant

In attachment 606418 [details] [diff] [review] for bug 571722, we added
the SSLProtocolVariant enum type with only one
enumerator -- ssl_variant_stream -- but didn't add
a protocolVariant member to sslSocket.  So we had
to pass a hardcoded ssl_variant_stream to functions
that take a SSLProtocolVariant argument in two places.

Now that we have ss->protocolVariant, the hardcoded
ssl_variant_stream arguments should be replaced by
ss->protocolVariant.

Comment 39

[Eric Rescorla (:ekr)]

Comment on attachment 651948 [details] [diff] [review]
Replaced hardcoded ssl_variant_stream with ss->protocolVariant

lgtm

Comment 40

[Eric Rescorla (:ekr)]

Comment on attachment 634605 [details] [diff] [review]
Rename DTLS_GetTimeout to DTLS_GetHandshakeTimeout

this has already landed but it lgtm

Comment 41

[Wan-Teh Chang]

Comment on attachment 651948 [details] [diff] [review]
Replaced hardcoded ssl_variant_stream with ss->protocolVariant

Patch checked in on the NSS trunk (NSS 3.14).

Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.188; previous revision: 1.187
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.94; previous revision: 1.93
done

Comment 42

[Eric Rescorla (:ekr)]

Attachment: CMD Patch

This is a replacement patch for DTLS in cmd

Comment 43

[Eric Rescorla (:ekr)]

Comment on attachment 664254 [details] [diff] [review]
CMD Patch

Wan-Teh,

New patch attached to allow testing DTLS in tstclnt. This should fix the issues you raised above, but it's been a while so I may have lost some context.

Comment 44

[Eric Rescorla (:ekr)]

Attachment: dtlsreplay.c

Not sure why this didn't show up.

Comment 45

[Wan-Teh Chang]

Attachment: ss->ssl3.hs.lastMessageFlight does not need to be allocated from the heap

In bug 793033, I just removed the strange sslSocket copying in ssl_FreeSocket,
which forced us to allocate ss->ssl3.hs.lastMessageFlight from the heap.
Now ss->ssl3.hs.lastMessageFlight can simply be a PRCList member.

Comment 46

[Eric Rescorla (:ekr)]

Comment on attachment 664288 [details] [diff] [review]
ss->ssl3.hs.lastMessageFlight does not need to be allocated from the heap

Review of attachment 664288 [details] [diff] [review]:
-----------------------------------------------------------------

This looks OK to me. Have you had a chance to test it? Or should I?

Comment 47

[Kai Engert (:KaiE:)]

Comment on attachment 664254 [details] [diff] [review]
CMD Patch

This patch has some conflicts with the work done in bug 785169 and will require merging.

However, this patch looks bigger than it is, because of indentation changes.

I will attach a version of this patch that ignores whitespace changes - for illustrative purposes, to assist the future manual merging.

Comment 48

[Kai Engert (:KaiE:)]

Attachment: CMD Patch - whitespace ignoring version

This is a version of attachment 664254 [details] [diff] [review] that was produced using "cvs diff -w", ignoring whitespace changes, thereby making it easier to read the real changes in the sections that chance indentation.

Comment 49

[Eric Rescorla (:ekr)]

Comment on attachment 664288 [details] [diff] [review]
ss->ssl3.hs.lastMessageFlight does not need to be allocated from the heap

Review of attachment 664288 [details] [diff] [review]:
-----------------------------------------------------------------

Original comment by me: This looks OK to me. Have you had a chance to test it? Or should I?

I have now tested this in alder's transportlayer_unittest and it works fine. Accordingly, I am r+ing it.

Comment 50

[Wan-Teh Chang]

Comment on attachment 664288 [details] [diff] [review]
ss->ssl3.hs.lastMessageFlight does not need to be allocated from the heap

Ekr: thanks for reviewing and testing the patch.

Patch checked in on the NSS trunk (NSS 3.14).

Checking in dtlscon.c;
/cvsroot/mozilla/security/nss/lib/ssl/dtlscon.c,v  <--  dtlscon.c
new revision: 1.5; previous revision: 1.4
done
Checking in ssl3con.c;
/cvsroot/mozilla/security/nss/lib/ssl/ssl3con.c,v  <--  ssl3con.c
new revision: 1.190; previous revision: 1.189
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.108; previous revision: 1.107
done

Comment 51

[Yev]

Just wanted to let everyone know that DTLS v 1.2 was just updated to all Windows PCs

Here is a link to the IETF draft for version 2

http://www.rfc-editor.org/rfc/rfc6347.txt

Comment 52

[Kosuke Kaizuka]

DTLS 1.2 has been implemented into NSS by bug 959864.
This bug need to be closed.