Re: DTLS patch

Hi!

See inlined comments, to both Andreas and Haiyangs mails.

On 05/31/2015 05:22 PM, Andreas Schultz wrote:
> Hi Haiyang,
>
> I'm sure Ingela will have plenty to say, starting with "Unit Tests".

Yes you are absolutely correct we want tests, a good place to start
could be a dtls_to_openssl_SUITE corresponding to ssl_to_openssl_SUITE.

> A few comments from me:
>
> 1.) Have you tested your changes with packet loss and reordering, especially
> reordering around the the Change Cipher Spec message?
>
> Keeping the correct cipher context for re-sending old flights and decoding
> incoming re-sends across CCS messages is not trivial and from reading the
> code I can't convince myself that it would work.
>
> 2.) I'm not happy with the way new clients are handled. You seem to
> insert them into an ETS tables without waiting for the answer to an
> HelloVerifyRequest. This opens a way for resource exhaustion and denial
> of service attacks. The DTLS RFC is quite explicit that the verify
> cookie should be generated stateless and that the server should expend
> any memory resource until the Hello was verified successfully.
>
> 3.) TLS is almost exclusively used in a pure form over TCP, the situation
> is different with DTLS where some interesting deviations exist. The TCP
> like socket handling is not help full with those.
>
> For example CAPWAP (RFC 5415) multiplexes DTLS and non DTLS data over the
> same UDP socket. Some mechanism to hook between the raw socket and the
> DTLS logic is required for that.
>
> 4.) You have certainly added lots of code, but you have still reused some
> ideas and code from my earlier versions. A small attribution wouldn't hurt.

I am sure these are all valid comments to consider. Andreas has made 
many valuable contributions to the ssl application in the past and has a 
good knowledge in the subject.

>> We have a server application needs DTLS protocol support. But the current OTP
>> release still has an incomplete DTLS implementation. So we create this patch to
>> include DTLS implementation based on current well-designed ssl architecture
>> (which we don't consider this as a new feature, just a patch). What we have
>> added are:
>>
>> 1. DTLS transport layer on top of gen_udp
>> 2. DTLS flight retransmission and timeout mechanism
>> 3. DTLS record fragmentation/defragmentation handling
>>
>> It looks good that the patch can work with OpenSSL 1.0.2a release. If no one is
>> working on dtls now, we would like to have this patch to be reviewed.
>>
>> Following is the repository contains the patch:
>>
>> https://github.com/haiyang-yin/otp
>>
>> To fetch the patch, refer to following git commands:
>>
>> git clone https://github.com/haiyang-yin/otp.git
>> git checkout dtls_patch
 >>
>> Here is code review location:
>>
>> https://github.com/haiyang-yin/otp/compare/maint...haiyang-yin:dtls_patch
>>
>> There are two demo programs to show how dtls client/server works in the
>> attachments.
>>
>> Feel free to let me know if further information is needed.

We would prefer if you could make it into a pull-request, or maybe 
several pull-requests. I think the first commit moving functions from 
tls_connection into ssl_connection could be one pull-request. That can 
be considered part of the refactoring work that I was doing in order to 
make our DTLS-implementation (that I alas have not  had time to finish 
due to other things being prioritized). So we could include that as a 
first step.

The other parts I and the OTP team will need some more time to think 
about your implementation suggestion. But what ever the conclusion will
be, it is a way to give this issue some more focus, and hopefully move 
it forward towards a functioning DLTS implementation.

Regards Ingela Erlang/OTP Team Ericssson AB


_______________________________________________
erlang-patches mailing list
[email protected]
http://erlang.org/mailman/listinfo/erlang-patches