[WIP] DTLS 1.0 preliminary patches

[Simon Josefsson]

Jonathan Bastien-Filiatrault <joe at x2a.org> writes:

> Alright, you can send me the copyright reassignment FSF papers.

I sent them separately.

> I will ensure that I have savannah account. I don't mind working on my
> local git tree for now. A dtls branch would be nice when the code is
> almost merge-ready or at least close to functionnal.

Ok, that's fine too.

>>> Unfortunately the lower end of the record layer and buffer/transport
>>> layer seems rather messy to my untrained eye. I am having trouble
>>> imagining implementing UDP buffering easely. I would need to buffer the
>>> whole packet then iterate over the records contained within the packet.
>>>
>>> The main problem seems to be layering violations between the handshake,
>>> record and buffer layers. Would it be better if _gnutls_{recv,send}_int
>>> dealt with whole records (and possibly return prematurely if more data
>>> or buffer space is required) ? _gnutls_{recv,send}_int could also deal
>>> with the SSLv2.0 record encapsulation. The handhake layer would
>>> therefore deal with those two functions for sending/receiving from the
>>> lower layer. The handshake layer buffering would also be moved to
>>> gnutls_handshake.c.
>>>
>>> Am I making any sense ?
>>
>> I agree that code is hairy, and I'm hoping Nikos can give you some
>> advice.  I don't feel strongly about changing this code, but it will
>> need significant testing before we can feel comfortable with the
>> changes.  Maybe as a bonus side-effect, the code will become more
>> readable... ;)
>
> Actually, I started to read the code yesterday and it seems to make
> more sense than I originally thought. I would be more inclined to
> short-circuit a lot of the buffer layer to a UDP buffering
> layer. Datagram transport requires its own buffers for retransmission
> and reassembly. I think it would be possible to do this without
> disturbing much of the record and handshake layer and without code
> duplication (I would still use functions such as _gnutls_read).

That's better.

>> Re 0002-Add-DTLS1.0-protocol-entry.patch: This breaks the API.  Can you
>> re-order the DTLS addition so it is after GNUTLS_TLS1_2 and add a '=
>> 100' after it so there is room for TLS 1.3 etc?  Also, please drop the
>> GNUTLS_DTLS1 mapping, I think it helps to be specific about version
>> numbers at all places.  I think this patch could be added quickly
>> without problem.
>
> Alright, but DTLS1.0 needs to be sandwiched between TLS1.1 and TLS1.2,
> mostly for ver < GNUTLS_TLS1_2 checks. Since TLS1.2 is still
> experimental, could this breakage be tolerated ? I am wide open for a
> suggestions in this case...

Where are these "ver <" checks?  I recall only a few.  I think those
checks are broken, and should be replace with a small inline function
that maps TLS version to requested behaviour (I'm thinking a
switch..case mapping).

>> Re 0003-Add-DTLS-state.patch: do you think there will be more DTLS
>> additions?  I suggest putting the DTLS stuff into a separate struct, if
>> you think it is OK.
>
> Yes, there needs to be more additions for datagram buffering and
> replay protection. Putting it in a sub-struct is not a problem. Would
> I keep it in the internals struct or at a higher level in the session
> struct ?

I prefer the internals struct.

>> Re 0004-Add-gnutls_session_datagram-function.patch: this just toggles
>> one way.  DTLS is really a completely new protocol, not just a different
>> transport method for TLS.  So maybe there should really be a new
>> function that replaces gnutls_init?  How about gnutls_init_dtls?  It
>> would return a gnutls_session_t for DTLS.
>
> I was thinking more of a set_transport in the long run with a DGRAM or
> STREAM argument and with a transport protocol selector for UDP, SCTP,
> DCCP and UDP-Lite. I guess gnutls_init_dtls could take those arguments
> instead.

A set_transport function would work if the protocols are the same, but
it seems several parts of GnuTLS will behave quite different depending
on whether TLS or DTLS is used (e.g., the set_priority functions) so it
is important to use a gnutls_init_dtls approach.

>> Re last part of 0005-Make-version-lookup-transport-dependent.patch: Is
>> this a good idea?  Running DTLS over TCP won't work, will it?  But I
>> understand why you need this.  Hm.  Maybe the default priorities needs
>> to be DTLS-specific -- this also argues for having gnutls_init_dtls
>> function so that the priority setting functions know for certain that
>> the session is a DTLS or non-DTLS session, and that won't change later.
>
> DTLS over TCP would probably work as long as the code handles the
> "partial read" problem, but that is rather far in Bad Idea (TM)
> territory. I have been hesitant to do priority fiddling since I have
> not had a good look at that code. Is there a easy way I could put all
> DTLS versions in a separate priority list and set the priority list in
> gnutls_init_dtls ?

That's how it should work, I think.  You could make the set_priority
functions check early on whether it is for DTLS or TLS.  Keep the
TLS-case as is today, and write new code for the DTLS code as
appropriate.  You may want to forbid some old ciphers or SSL stuff for
DTLS.

>> Generally, I like your incremental approach since it helps to review and
>> merge the patches.  I definitely think we should create a branch for
>> your DTLS work.
>
> Thanks, that is what git rebase and git commit --amend is for
> :-)... Until the code is alpha quality I will probably do a lot of
> rebasing and cleaning of previous commits, I would need to be allowed
> to force updates on the dtls branch, at least for a while.

Right.

/Simon