Re: [WIP] DTLS 1.0 preliminary patches

[Jonathan Bastien-Filiatrault]

Simon Josefsson wrote:
> Jonathan Bastien-Filiatrault <address@hidden> writes:
>
> > Hello,
> >
> > Being interested in DTLS and GnuTLS I have decided to try to implement
> > DTLS in the GnuTLS library.
>
> Great!
>
> > I have managed to send a valid DTLS ClientHello using a modified GnuTLS
> > in a relatively non-intrusive way (but which may break the ABI since it
> > messes with existing enum values). The OpenSSL implementation responds
> > to this ClientHello with a HelloVerifyMessage and Wireshark considers
> > the packet valid DTLS.
> >
> > You may find my patches at this URL: http://x2a.org/pub/dtls/
>
> Even better.
>
> If you feel it would help you, you could have git access so you could
> push your work to a special branch.  You need a savannah.gnu.org account
> and sign FSF papers though.

Alright, you can send me the copyright reassignment FSF papers. 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.

> > 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).

> 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...

> 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 ?

> 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.

> 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 ?

> 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.

> /Simon

Thanks for having a look at my patches,
Cheers,
Jonathan