DTLS "accept" functionality

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

DTLS "accept" functionality

Michael Richardson

I have been working since mid-November in my "copious spare time" to bring
DTLS support into ruby-openssl in order to bring DTLS into the Rails "David"
CoAP server.

DTLS_listen() seems entirely focused on single-use situations like in RTP,
where only a single connection (single DTLS session) is expected. If used
in CoAP situation, there are a number of race conditions that make it
hard to use correctly.   I wrote another email about that, which I think did
not get to the list, which I can resend.

I have preserved DLTSv1_listen() functionality, refactoring most of it
into DTLSv1_anwerHello(), and then adding DTLSv1_accept() on top of it.

Unfortunately I was also forced to delve into dgram_bss.c in order to bring
some additional information out; stuff which is rather OS dependant for IPv4,
and which perhaps would be better done in the application somehow.

I'm looking for advice.

Also, I found it difficult to find the right incantation to get a static
build; I noticed finally that my test/* programs were linking against the
libssl that was in my /lib, rather than the code I was testing.  I suggest
that this be captured into a ./Configure option.  I used:

     ./Configure no-shared --debug linux-x86_64

"no-dso" seemed like it ought to help, but it made things worse...

I hope that I added the new exposed symbols correctly.
I have yet to validate the complete david/coap/ruby-openssl/dtls
functionality, but I wanted to post this sooner for review.


https://github.com/openssl/openssl/pull/5024 says:

This patch refactors the DTLSv1_listen() to create an alternative API that is
called DTLSv1_accept().

DTLSv1_accept() is useable by programs that need to treat DTLS interfaces in
a way similar to TCP: new connections must be accepted, and new sockets
created.

There are a number of changes included here:

1. dgram_write() and dgram_read() now use sendmsg(2) and recvmsg(2) in order to set and collect the local address used in the datagram.  This allows a socket to bound to ::, while still sending traffic correctly from the address that the other peer used. The IPv6 version of this code is according to rfc3542 API, but the IPv4 of the code is Linux specific and needs to be either abstracted and translated for *BSD and Windows, or IPv6 mapped IPv4 sockets could be used.
2. a new BIO control SET_ADDR and GET_ADDR are added, and if the address is not set then it pulls it out of the socket using getsockname(2).
3. DTLSv1_accept() accepts a second SSL* on which new connections are setup. A new socket is set up using the addresses from the received message and it is inserted into the BIO. There is a race condition during setup which turned out to be unavoidable: between the bind(2) and the connect(2), the new socket could have the same address as the "listen" socket, and additional hello packets could arrive on the wrong socket.  Such packets will hopefully be dropped as garbage. It was thought that connect(2) could be called before bind(2), but that seems to cause the kernel to allocate a local address for the new socket (a random port), which the subsequent bind(2) can not seem to change.
4. the use of POSIX socket APIs inside this code is likely inappropriate and this routine should be split up in some better way so that socket activities can be done by the application, using the correct abstractions for the OS.
5. a new test case dtlsaccepttest was created.  It uses fork() and also calls system() on "openssl s_client". Probably it should just call code internally, but apps/* is not in a library form that can be used. Perhaps this use is acceptable.  Other test functions just use canned packets in/out, but since this is testing the creation/adaption of socket code, real sockets would seem to be necessary.  Probably this test case should be disabled on non-Unix platforms.
6. routines BIO_ADDR_sockaddr and BIO_ADDR_sockaddr_size exposed from libcrypto to libssl.  There might be a better way to do this, perhaps a BIO_CTRL would be better?



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


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [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  [
]     [hidden email]  http://www.sandelman.ca/        |   ruby on rails    [

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Matt Caswell-2


On 16/01/18 15:32, Michael Richardson wrote:

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

The second way is what is intended. Maybe I misunderstand your point -
but the client address *is* returned? Admittedly its wrapped in a
BIO_ADDR, but its easy to get the underlying "raw" address using
BIO_ADDR_rawaddress():

https://www.openssl.org/docs/man1.1.0/crypto/BIO_ADDR_rawaddress.html

i.e. call BIO_ADDR_rawaddress() on the BIO_ADDR returned by
DTLSv1_listen() and then do something like this:

 /* Handle client connection */
 int client_fd = socket(AF_INET6, SOCK_DGRAM, 0);
 bind(client_fd, &server_addr, sizeof(struct sockaddr_in6));
 connect(client_fd, &client_addr, sizeof(struct sockaddr_in6));
 /* Set new fd and set BIO to connected */
 BIO *cbio = SSL_get_rbio(ssl);
 BIO_set_fd(cbio, client_fd, BIO_NOCLOSE);
 BIO_ctrl(cbio, BIO_CTRL_DGRAM_SET_CONNECTED, 0, &client_addr);
 /* Finish handshake */
 SSL_accept(ssl);

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

Which is what I recommend.


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

Why isn't recvfrom() suitable (which is what the code currently uses to
get the address)?

Matt
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Michael Richardson

Matt Caswell <[hidden email]> wrote:
    >> 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.

    > The second way is what is intended.

Unfortunately, there remains a race condition because we have to call bind()
before connect() on the new socket.  Under load, if a packet is received
between the bind() and the connect(), it might go onto the wrong socket
queue. So some packets that could have been processed will get dropped and
have to be retransmitted by the client.

It could be solved if there was a way to punt packets received on the wrong
socket to the correct BIO on the input side.  I looked into this, but decided
it was too difficult...

That would also let one operate a multitude of DTLS connections using single
socket which might be a boon to some users.  Mis-designed it would scale
badly on multi-threaded machines and involve lots of ugly locks.
I don't want to consider the impacts if one had to pass packets between processes...
I don't advocate a solution like this (I'll live with the dropped packets),
but I think it's worth making people aware that they might see mis-directed
packets get dropped.

    > Maybe I misunderstand your point -
    > but the client address *is* returned? Admittedly its wrapped in a
    > BIO_ADDR, but its easy to get the underlying "raw" address using
    > BIO_ADDR_rawaddress():

    > Why isn't recvfrom() suitable (which is what the code currently uses to
    > get the address)?

The address of the remote client is returned ("getpeername()") by DTLSv1_listen().
That's all that recvfrom() gives you.

recvfrom() was a reasonable API for SunOS 3.x machines with a single 10Mb/s
interface with a single IPv4 address.  I loved all that at the time...
But it doesn't work that well when we might have a dozen different kind of
IPv6 addresses on each virtual interface.

The address that I'm talking about needing is the one the remote client used
to reach us.  That's the destination IP of the incoming packet ("getsockname()" in TCP speak).

With TCP this is never an issue because the kernel creates the new socket and
copies the right stuff in for us when it creates the socket.

With UDP, the source address for outgoing packets needs to match or the
client may get a response from an address that it didn't connect to.  Worse,
there might be firewalls or policy routing that would cause the packet to
disappear or get routed differently.  In my application, I definitely dealing
with connections over IPv6 Link-Local addresses with a multitude of virtual
links.

In your code example:
    bind(client_fd, &server_addr, sizeof(struct sockaddr_in6));

server_addr has to be set from the destination address of the incoming
packet, it's not a global that the admin set, or the SIP negotiated.

In the bad old days, this meant opening a socket for every interface on the
machine, and re-reading the list of interfaces based upon some heuristic.
(routing socket, SIGHUP, ...)

Even getting a list of interfaces (and their addresses) is itself a
OS-dependant activity!  And, if you use the old BSD interface on Linux,
you'll miss a bunch of interfaces, because the Linux kernel people thought
that it would confuse BSD APIs if interfaces were returned that the BSD
interface didn't create.  So you can't even win there.

The IPv6 API gives us recvmsg() and ipi6_pktinfo, which makes it all sane.
But, we never got a standard interface for IPv4: Linux uses something that
looks identical to IPv6, and BSD has something with slightly different names.

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


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Matt Caswell-2


On 16/01/18 19:44, Michael Richardson wrote:

>
> Matt Caswell <[hidden email]> wrote:
>     >> 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.
>
>     > The second way is what is intended.
>
> Unfortunately, there remains a race condition because we have to call bind()
> before connect() on the new socket.  Under load, if a packet is received
> between the bind() and the connect(), it might go onto the wrong socket
> queue. So some packets that could have been processed will get dropped and
> have to be retransmitted by the client.

This seems like a non-issue to me. At this point in the handshake the
client will have sent its ClientHello and won't progress until it gets
the server's flight back (ServerHello etc), i.e. in the vast majority of
cases it won't be sending anything.

It is possible that the client may retransmit the ClientHello if the
server hasn't responded within a timely manner. Retransmit times are
usually quite a while (relatively speaking) after the original
transmission, so the chances of this happening immediately after we've
processed the original ClientHello and before we've called connect()
seem quite small - although possible. If a retransmitted ClientHello
arrives *after* the connect() it will be dropped anyway by OpenSSL so we
really don't care about these messages going missing.

Another possibility is that the client transmits an alert of some form.
This also seems quite unlikely (i.e. send a ClientHello and then
immediately send an alert before the server has had a chance to respond)
- but again, its possible. It would be bad luck indeed for this unlikely
scenario to happen and then for the alert to not make it onto the right
fd due to the race: but if it did its not a big deal. This connection is
doomed in any case (an alert was sent) and we have to be prepared to
deal with packets getting dropped anyway (this is DTLS!). It is very
common for clients to just abort without sending an alert anyway - so
this will just appear like that.

>
> It could be solved if there was a way to punt packets received on the wrong
> socket to the correct BIO on the input side.  I looked into this, but decided
> it was too difficult...
>
> That would also let one operate a multitude of DTLS connections using single
> socket which might be a boon to some users.  Mis-designed it would scale
> badly on multi-threaded machines and involve lots of ugly locks.
> I don't want to consider the impacts if one had to pass packets between processes...
> I don't advocate a solution like this (I'll live with the dropped packets),
> but I think it's worth making people aware that they might see mis-directed
> packets get dropped.
>
>     > Maybe I misunderstand your point -
>     > but the client address *is* returned? Admittedly its wrapped in a
>     > BIO_ADDR, but its easy to get the underlying "raw" address using
>     > BIO_ADDR_rawaddress():
>
>     > Why isn't recvfrom() suitable (which is what the code currently uses to
>     > get the address)?
>
> The address of the remote client is returned ("getpeername()") by DTLSv1_listen().
> That's all that recvfrom() gives you.
>
> recvfrom() was a reasonable API for SunOS 3.x machines with a single 10Mb/s
> interface with a single IPv4 address.  I loved all that at the time...
> But it doesn't work that well when we might have a dozen different kind of
> IPv6 addresses on each virtual interface.
>
> The address that I'm talking about needing is the one the remote client used
> to reach us.  That's the destination IP of the incoming packet ("getsockname()" in TCP speak).

Ahhh....its the *server's* address you are after. This requirement seems
more reasonable. I think the API is designed to expect you to only bind
to a single IP. I'd be interested in Richard Levitte's opinion on this.

It seems like a fairly simple solution could solve this. Currently we
have BIO_dgram_get_peer() which returns the peer's address for the last
message read from a BIO. You could imagine a new call being introduced
to get our own address. You could then call that immediately after a
successful DTLSv1_listen() call. Obviously we'd have to change the dgram
BIO to use recvmsg for this to work.

Matt

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Michael Richardson

Matt Caswell <[hidden email]> wrote:
    >> Matt Caswell <[hidden email]> wrote: >> 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.
    >>
    >> > The second way is what is intended.
    >>
    >> Unfortunately, there remains a race condition because we have to call
    >> bind() before connect() on the new socket.  Under load, if a packet is
    >> received between the bind() and the connect(), it might go onto the
    >> wrong socket queue. So some packets that could have been processed
    >> will get dropped and have to be retransmitted by the client.

    > This seems like a non-issue to me. At this point in the handshake the
    > client will have sent its ClientHello and won't progress until it gets
    > the server's flight back (ServerHello etc), i.e. in the vast majority
    > of cases it won't be sending anything.

*That* client will be waiting, but other clients may be sending new ClientHello
messages (with or without cookies).

    >> The address of the remote client is returned ("getpeername()") by
    >> DTLSv1_listen().  That's all that recvfrom() gives you.
    >>
    >> recvfrom() was a reasonable API for SunOS 3.x machines with a single
    >> 10Mb/s interface with a single IPv4 address.  I loved all that at the
    >> time...  But it doesn't work that well when we might have a dozen
    >> different kind of IPv6 addresses on each virtual interface.
    >>
    >> The address that I'm talking about needing is the one the remote
    >> client used to reach us.  That's the destination IP of the incoming
    >> packet ("getsockname()" in TCP speak).

    > Ahhh....its the *server's* address you are after. This requirement
    > seems more reasonable. I think the API is designed to expect you to
    > only bind to a single IP. I'd be interested in Richard Levitte's
    > opinion on this.

okay.
binding to a single IP is not scalable in many applications.

    > It seems like a fairly simple solution could solve this. Currently we
    > have BIO_dgram_get_peer() which returns the peer's address for the last
    > message read from a BIO. You could imagine a new call being introduced
    > to get our own address. You could then call that immediately after a
    > successful DTLSv1_listen() call. Obviously we'd have to change the
    > dgram BIO to use recvmsg for this to work.

That's here:
       https://github.com/mcr/openssl/commit/f764151782b4b32a752b4016336c0ceafa98ed5c
       https://github.com/mcr/openssl/commit/50692219afe92762e85338b8d947e7ac732d2cde
and:   https://github.com/mcr/openssl/commit/bb6f6b2cc860f25eb2b08aa109d1c7dc9ea94323


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


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Matt Caswell-2


On 17/01/18 16:34, Michael Richardson wrote:

>
> Matt Caswell <[hidden email]> wrote:
>     >> Matt Caswell <[hidden email]> wrote: >> 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.
>     >>
>     >> > The second way is what is intended.
>     >>
>     >> Unfortunately, there remains a race condition because we have to call
>     >> bind() before connect() on the new socket.  Under load, if a packet is
>     >> received between the bind() and the connect(), it might go onto the
>     >> wrong socket queue. So some packets that could have been processed
>     >> will get dropped and have to be retransmitted by the client.
>
>     > This seems like a non-issue to me. At this point in the handshake the
>     > client will have sent its ClientHello and won't progress until it gets
>     > the server's flight back (ServerHello etc), i.e. in the vast majority
>     > of cases it won't be sending anything.
>
> *That* client will be waiting, but other clients may be sending new ClientHello
> messages (with or without cookies).

So how does your refactor solve this issue? AFAICT this also just does a
bind then connect:

+
if(bind(rfd,BIO_ADDR_sockaddr(ouraddr),BIO_ADDR_sockaddr_size(ouraddr))
!= 0){
 +      goto end;
 +    }
 +
if(connect(rfd,BIO_ADDR_sockaddr(client),BIO_ADDR_sockaddr_size(client))
!= 0) {
 +      goto end;
 +    }

Doesn't this suffer from the same problem? i.e. packets could arrive
from other clients between the bind and connect.

Matt
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Michael Richardson

Matt Caswell <[hidden email]> wrote:
    >> Matt Caswell <[hidden email]> wrote:
    >> >> Matt Caswell <[hidden email]> wrote: >> 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.
    >> >>
    >> >> > The second way is what is intended.
    >> >>
    >> >> Unfortunately, there remains a race condition because we have to call
    >> >> bind() before connect() on the new socket.  Under load, if a packet is
    >> >> received between the bind() and the connect(), it might go onto the
    >> >> wrong socket queue. So some packets that could have been processed
    >> >> will get dropped and have to be retransmitted by the client.
    >>
    >> > This seems like a non-issue to me. At this point in the handshake the
    >> > client will have sent its ClientHello and won't progress until it gets
    >> > the server's flight back (ServerHello etc), i.e. in the vast majority
    >> > of cases it won't be sending anything.
    >>
    >> *That* client will be waiting, but other clients may be sending new ClientHello
    >> messages (with or without cookies).

    > So how does your refactor solve this issue? AFAICT this also just does a
    > bind then connect:

My refactor does not solve it. I'm noting that this is still an issue.

It's not solvable unless the kernel can do both operations at the same time,
for which there isn't a standard call (nor a non-standard one).
If we could punt segments between BIOs easily, then we could deal with the
problem, but I don't think it's worth solving.


    > if(bind(rfd,BIO_ADDR_sockaddr(ouraddr),BIO_ADDR_sockaddr_size(ouraddr))
    > != 0){
    > +      goto end;
    > +    }
    > +
    > if(connect(rfd,BIO_ADDR_sockaddr(client),BIO_ADDR_sockaddr_size(client))
    > != 0) {
    > +      goto end;
    > +    }

    > Doesn't this suffer from the same problem? i.e. packets could arrive
    > from other clients between the bind and connect.

Yes.

This is in contrast this situation to the original problem with connect()'ing
the socket which is given to DTLSv1_listen(). That socket has lots of time
between the two points in which to accumulate new connection requests.

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


--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev

signature.asc (497 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Matt Caswell-2
In reply to this post by Michael Richardson


On 17/01/18 16:34, Michael Richardson wrote:

>
>     > It seems like a fairly simple solution could solve this. Currently we
>     > have BIO_dgram_get_peer() which returns the peer's address for the last
>     > message read from a BIO. You could imagine a new call being introduced
>     > to get our own address. You could then call that immediately after a
>     > successful DTLSv1_listen() call. Obviously we'd have to change the
>     > dgram BIO to use recvmsg for this to work.
>
> That's here:
>        https://github.com/mcr/openssl/commit/f764151782b4b32a752b4016336c0ceafa98ed5c
>        https://github.com/mcr/openssl/commit/50692219afe92762e85338b8d947e7ac732d2cde
> and:   https://github.com/mcr/openssl/commit/bb6f6b2cc860f25eb2b08aa109d1c7dc9ea94323

Please raise a separate PR for this work. It *must* be portable though
and work across all our platforms (e.g. including VisualC etc). My
suggestion is that your BIO_CTRL_DGRAM_GET_ADDR/BIO_CTRL_DGRAM_SET_ADDR
ctrls should return an error on platforms that we don't know we can
support, i.e. attempt to detect (at compile time) whether we are on a
platform that we know has the required system calls - if we are then use
them, otherwise we do things the old way.

Note that stuff like this is problematic:

    char __attribute__((aligned(8))) chdr[CMSG_SPACE(sizeof(struct
in_pktinfo))];

The "attribute" is compiler specific and not something we can rely on to
be available. Additionally "CMSG_SPACE" is probably not portable, and in
any case may not evaluate to a compile time constant (according to the
man page), so this is not C90 (which is a requirement for OpenSSL
submissions).

I suggest you Configure with the "--strict-warnings" option which will
probably complain about some of this stuff.

Please also make sure we have suitable documentation and ideally tests too.

Once this PR is in (assuming it gets accepted), then you can look at
what remains of your original PR and see if it makes sense to raise new
PRs to bring the rest of it in.

Matt

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Michael Richardson
Matt Caswell <[hidden email]> wrote:
    > Please raise a separate PR for this work. It *must* be portable though
    > and work across all our platforms (e.g. including VisualC etc). My
    > suggestion is that your BIO_CTRL_DGRAM_GET_ADDR/BIO_CTRL_DGRAM_SET_ADDR
    > ctrls should return an error on platforms that we don't know we can
    > support, i.e. attempt to detect (at compile time) whether we are on a
    > platform that we know has the required system calls - if we are then use
    > them, otherwise we do things the old way.

    > Note that stuff like this is problematic:

    > char __attribute__((aligned(8))) chdr[CMSG_SPACE(sizeof(struct
    > in_pktinfo))];

    > The "attribute" is compiler specific and not something we can rely on to
    > be available. Additionally "CMSG_SPACE" is probably not portable, and in
    > any case may not evaluate to a compile time constant (according to the
    > man page), so this is not C90 (which is a requirement for OpenSSL
    > submissions).

Understood.
I think that CMSG_SPACE might be in the POSIX spec, but I'll double check.

What do you suggest I do for the IPv4 stuff, which has no POSIX standard?
A bunch of #ifdef on OS definitions?

    > Once this PR is in (assuming it gets accepted), then you can look at
    > what remains of your original PR and see if it makes sense to raise new
    > PRs to bring the rest of it in.

Roger. Wilco.

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

--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev
Reply | Threaded
Open this post in threaded view
|

Re: [openssl/openssl] Dtls listen refactor (#5024)

Matt Caswell-2


On 19/01/18 16:32, Michael Richardson wrote:

> Matt Caswell <[hidden email]> wrote:
>     > Please raise a separate PR for this work. It *must* be portable though
>     > and work across all our platforms (e.g. including VisualC etc). My
>     > suggestion is that your BIO_CTRL_DGRAM_GET_ADDR/BIO_CTRL_DGRAM_SET_ADDR
>     > ctrls should return an error on platforms that we don't know we can
>     > support, i.e. attempt to detect (at compile time) whether we are on a
>     > platform that we know has the required system calls - if we are then use
>     > them, otherwise we do things the old way.
>
>     > Note that stuff like this is problematic:
>
>     > char __attribute__((aligned(8))) chdr[CMSG_SPACE(sizeof(struct
>     > in_pktinfo))];
>
>     > The "attribute" is compiler specific and not something we can rely on to
>     > be available. Additionally "CMSG_SPACE" is probably not portable, and in
>     > any case may not evaluate to a compile time constant (according to the
>     > man page), so this is not C90 (which is a requirement for OpenSSL
>     > submissions).
>
> Understood.
> I think that CMSG_SPACE might be in the POSIX spec, but I'll double check.
>
> What do you suggest I do for the IPv4 stuff, which has no POSIX standard?
> A bunch of #ifdef on OS definitions?

If its non portable and we're not already using it then that's the
probably the best we can do. We should try and provide some sensible
fallback wherever possible. Or if not possible it shouldn't break
anything that already works.

Matt
--
openssl-dev mailing list
To unsubscribe: https://mta.openssl.org/mailman/listinfo/openssl-dev