[openssl-dev] [openssl/openssl] Dtls listen refactor (#5024)

[Michael Richardson]

please see https://github.com/openssl/openssl/pull/5024

mattcaswell asks on github:
    mattcaswell> I am unclear about the underlying premise of this PR:

    mcr>     This patch refactors the DTLSv1_listen() to create an
    mcr> alternative API that is called DTLSv1_accept().
    mcr> DTLSv1_accept() is useable by programs that need to treat
    mcr> DTLS interfaces in a way similar to TCP: new connections
    mcr> must be accepted, and new sockets created.

    mattcaswell> Your going to give more justification than this. Why is it
    mattcaswell> that DTLSv1_listen() is not appropriate for your use case?

As I understand using DTLSv1_listen(), one does the following:
  1) open a UDP socket, bind(2) it appropriately.
     {in an RTP context, one might already know the remote port numbers, and
     one could connect(2) it already. In the CoAP case, that certainly is not
     the case}
     Put the socket into an SSL, do appropriate blocking or non-blocking
     event handling in application.

  2) call DTLSv1_listen() when there is traffic.
     DTLSv1_listen() will process (via peek) the first packet in the socket.
     If it's a Client Hello without a cookie, then a Hello Verify is sent
     back (%).  If it ate the packet, then it loops until it find something
     it can't handle or runs out of packets.

  3) DTLSv1_listen(), when it finds a Client Hello with a cookie, marks the
     provided SSL as having transitioned to a state where things can start,
     and it returns 1, along with the BIO_ADDR of the newly Verify Hello'ed client.

  4) the application is now expected to connect() the FD to the BIO_ADDR,
     and call SSL_accept(), and then to proceed with SSL_read()/SSL_write(), etc.

Perhaps I've gotten something wrong with this process.

This flow is entirely appropriate for a RTP user, but for a CoAP server there
are a number of problems:

a) when the existing FD is connect(2) any future traffic to the bound port
   will get rejected with no port.  So the application really has to open a
   new socket first.
   The application can do this two ways: it can open a new socket on which to receive
   new connections, or it can open a new socket on which to communicate with
   the new client.    The second method is better for reason (b) below.
   Either way, it socket to communicate with the client needs to be bind(2)
   to the address that the client used to communicate with the server, and
   DTLSv1_listen() didn't collect or return that information.

b) the existing FD might have additional packets from other clients. This
   argues for opening a new socket for the new client, and leaving the queue
   on the existing FD.

c) the DTLSv1_listen() uses the SSL* (and associated CTX) that is provided to
   it for callbacks, and cookie verification.  It modifies the state of that
   SSL* to continue the transaction.
   I think that the roles should be split up.

also, from point (2) above:
(%) - the send that DTLSv1_listen() depends upon the socket having been
    bind(2) with a non-INADDR_ANY/IN6ADDR_ANY_INIT IP address, or that the
    system in question has only a single IP address.  This is because the
    write that is done relies upon the kernel to pick the right source
    address,  which appears to be easy for IPv4 with a single interface, but
    trivially can fail for IPv6 even with a single interface due to
    temporary, stable-private, and link-local addresses.


DTLSv1_accept() takes two SSL*.  The first is used for cookie verification,
while the second is filled in with a new FD that has been bind/connect to the
client and state advanced to be ready for SSL_accept().  It also returns the
same BIO_ADDR for the client, but that could be removed as it can trivially
be retrieved from the new SSL*.

    mattcaswell> In any case the PR as it currently stands is a very long way
    mattcaswell> off being acceptable:

I totally agree, but I had to post something to start the conversation.

    mattcaswell> * As you point out the use of the POSIX socket APIs is
    mattcaswell> unacceptable and is at the wrong level of abstraction. I
    mattcaswell> might perhaps expect to see this sort of thing in the BIO
    mattcaswell> layer.

a) I could move the socket creation code into BIO layer, a new BIO_ctrl method
   could be created to "duplicate" the BIO.  This would probably eliminate
   having to expose BIO_ADDR_sockaddr{,_size} from libcrypto->libssl.

b) creation of a new socket could be a new callback.

c) DTLSv1_accept() could return at:
   "now set up a socket based upon the original rbio's peer/addr"
   as all of the subsequent operations could be done by the application given
     BIO_dgram_get_peer(rbio, client) and BIO_dgram_get_addr(rbio, ouraddr)

d) a combination of (a) and (c), where the duplication code is provided by
   the BIO layer, but the application could do something different if it
   needed to.

My preference is for (d), because I think that it's common code and the
application writer will get it wrong.  In particular, you need to open the
socket with SO_REUSEPORT in order to be allowed to bind() the new socket
before connect(2)ing it.  If there were a system call to do both at the same
time it would be better.  There is a race condition by calling bind() first,
because the kernel is likely to put new traffic from new sources into the
new socket.  They will be dropped as having the wrong cookies.

    mattcaswell> * The code does not seem to be portable - it needs
    mattcaswell> to work on all our platforms

    mattcaswell> * There is no documentation

I will write more documentation when I am sure what the structure is going to be.

    mattcaswell> I noticed a number of other things at a more specific level,
    mattcaswell> just scanning through the code, but at this point I have not
    mattcaswell> reviewed it in detail. I am not yet convinced there is a
    mattcaswell> need for this.

I absolutely need to have recvmsg()/sendmsg() in the bss_dgram.c in order to
get the destination address used.   This IPv6 code is portable, since the RFC
API says how to do it.

The IPv4 code varies by OS; I can probably write the correct thing and get
tested it on FreeBSD, but I have no idea about Windows.  We used to solve
this by opening a socket for each interface and listening to the routing
socket, or having a human configure an explicit list of interfaces, or just
failing on multi-homed hosts.

I propose to split this pull request up into one that deals with the changes
to the BIO layer only.
A second pull request will include the "duplicate" BIO functionality.

A third then deals with the d1_lib.c layer.



--
]               Never tell me the odds!                 | ipv6 mesh networks [
]   Michael Richardson, Sandelman Software Works        | network architect  [
]     mcr at sandelman.ca  http://www.sandelman.ca/        |   ruby on rails    [