DTLS Protocol Support #353

mestery · 2020-03-27T21:24:33Z

This adds support for processing DTLS packets, specifically to acquire the
SNI value from the ClientHello packet. DTLS and TLS packets are different
enough that a separate parser was required. However, it appears extension
processing is the same. So I've moved both parse_extensions() and
parse_server_name_extension() into new sni.[c,h] files so those functions
can be shared by both the tls and dtls processors.

This code includes functional test for the DTLS parsing.

Related to issue #352.

Signed-off-by: Kyle Mestery [email protected]

This adds support for processing DTLS packets, specifically to acquire the SNI value from the ClientHello packet. DTLS and TLS packets are different enough that a separate parser was required. However, it appears extension processing is the same. So I've moved both parse_extensions() and parse_server_name_extension() into new sni.[c,h] files so those functions can be shared by both the tls and dtls processors. Note that this code is untested at the moment. Related to issue dlundquist#352. Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-03-27T21:25:11Z

@dlundquist Let me know your thoughts here. After this, some guidance on how I should approach the connection/listener code would be appreciated. Thanks so much!

dlundquist

Looks good, but I would like to hold off on merging until we get UDP support proved out.

src/dtls.c

src/dtls.h

dlundquist · 2020-03-28T17:49:31Z

@mestery I've started on adding datagram support to the listener and connection objects: 4f34b67...udp-poc, the recvmsg(), socket(), bind(), connect() sequence works to receive additional UDP packets of the same socket tuple on the connection's client socket. I'm not sure how portable this approach is, but we already have Linux specific features.

I tried testing with openssl s_client --connect 127.0.0.1:2000 -dtls1_2 and it the dtls protocol module was unable to process the request:

2020-03-28 10:47:07 Parsed localhost 127.0.0.1:2001
2020-03-28 10:47:10 Requested version of DTLS not supported.
2020-03-28 10:47:10 Unable to parse request from 127.0.0.1:45565: parse_packet returned -5

I think the next step is to get a DTLS functional test going.

mestery · 2020-03-28T17:57:56Z

Thanks for starting that work! Let me work on the functional test now as well.

mestery · 2020-03-28T18:00:50Z

I'll add a functional test now, thanks for working on this!

This adds a DTLS functional test, similar to the existing TLS functional test. For now, this has a single known good DTLS packet which we run the test against. More can be added as Wireshark captures allow. Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-03-28T21:56:51Z

@dlundquist Functional test for DTLS added and the parsing code passes now, let me know how your branch looks! I think we're getting closer!

This adds the hostname from the test data into the test_packet structure, allowing for dynamic checking of hostnames from the data rather than hard coding "localhost". Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-03-29T17:16:52Z

I rebased your udp-poc branch on top of mine and the openssl s_client commands appear to work now! I'll try your branch with my actual setup on Monday morning, but I think we're close to perhaps getting this merged!

dlundquist · 2020-03-29T18:36:11Z

@mestery I think we are going to need to preserve datagram framing in the buffer module, the present buffer was designed for a stream. My first though would be to just prepend a uint16_t with the UDP payload length. This may introduce alignment issues on platforms that do not allow unaligned word access. This could be resolved by always making the length prefix aligned, that would also avoid the edge case where the length is wrapped around the end of the circular buffer. But this requires either the connection module reaching into the buffer implementation to retain a pointer to where the length is to be stored before the receive, or making the buffer module stream/dgram type dependent.

Also, initiate_server_connect() will need to be taught to use UDP, but this is an outgoing UDP connection so it should be just changing SOCK_STREAM to con->listener->protocol->sock_type, maybe it's worth storing the socket type in the connection object too.

Honestly dynamic dispatch would make all of these stream/datagram differences a lot cleaner.

This allows us to do some more advanced buffer processing depending on if we are buffering stream or datagram sockets. Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-03-29T21:52:13Z

OK, I folded your commits into this branch, and added an initial stab at adding socket type to both Connection and Buffer structures. I don't quite follow what you're saying in terms of adding the UDP packet size to the buffer object, can you elaborate a bit more on that so I can tackle that next? Thanks for all the help here, this is moving quite nicely!

Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-03-30T02:08:20Z

I tested the patch above and it is able to successfully proxy DTLS as well as TLS now for my application! I'm curious about your comments around datagram framing because I'm guessing something is still missing here, so let me know. Thanks for the help with this, it's pretty cool to see it working!

Signed-off-by: Kyle Mestery <[email protected]>

DTLS abort messages include a version of `0xfe 0xfd` which gives warnings if abort_message is not an unsigned char. Signed-off-by: Kyle Mestery <[email protected]>

The tests below are all failing irresective of the DTLS work, remove them for now, they need to be fixed outside of the scope of the DTLS work: - binder_test - bind_source_test - proxy_header_test - transparent_proxy_test Signed-off-by: Kyle Mestery <[email protected]>

The buffer code was meant to work with stream sockets. This commits modifies it so it works with datagram sockets as well as stream sockets. Proxying datagram sockets requires the ability to push an exact copy of each frame from source to destination and vice versa. Thus, we introduce the ability for the Buffer structure to understand if it is associated with a SOCK_STREAM or a SOCK_DGRAM. If a SOCK_DGRAM is being used, for each write into the buffer we first reserver two bytes and we store the length. When reading, we first read the length as well. Tests were updated to test the new SOCK_DGRAM functionality. Note that when testing this locally it doesn't quite work yet. I am pushing this commit out so it can be reviewed while I debug it. Signed-off-by: Kyle Mestery <[email protected]>

src/buffer.c

src/connection.c

tests/Makefile.am

src/buffer.h

src/buffer.c

This utilizes a new approach suggested by Dustin during review to better handle datagram packets in the buffer code. This seems to mostly work, still testing locally a bit. I've updated various tests to pass with the new code as well. Signed-off-by: Kyle Mestery <[email protected]>

dlundquist

I think all the DGRAM specific bits can go in setup_read_iov(), setup_write_iov(), advance_read_position() and advance_write_position(). advance_write_position() would be responsible for storing the length prefix.

src/buffer.c

src/buffer.h

Implement datagram specific buffer tails using setup_write_iov(), setup_read_iov(), advance_write_position() and advance_read_position(). Signed-off-by: Dustin Lundquist <[email protected]> Signed-off-by: Kyle Mestery <[email protected]>

This corrects buffer_coalesce to work with datagrams. It should not be returning the pointer into the datagram length. This required datagram protocols like DTLS to look past this. I've corrected this with this commit and also fixed DTLS parsing. Signed-off-by: Kyle Mestery <[email protected]>

For the datagram case, we need to replicate the exact buffer we're coalescing, including the individual datagram sizes. The code as it existed before this patch was writing the entire buffer length into the first 2 bytes of datagram size, messing it all up. This fixes it to iterate the existing Buffer, copying by pieces into a new buffer, and then copying the resulting buffer back over. Signed-off-by: Kyle Mestery <[email protected]>

Once a datagram socket has been accepted, read any data sent on the socket right away. Signed-off-by: Kyle Mestery <[email protected]>

dlundquist

buffer_pop() and buffer_peek() can't differentiate between a zero length datagram and no more data -- I wonder if this is going to cause any unexpected problems?

src/buffer.c

src/connection.c

src/buffer.c

Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-04-06T18:57:12Z

Now that I've sorted out the issues on my end this is working perfectly well! From my perspective this code is ready to merge into master now, let me know your thoughts @dlundquist I will prioritize addressing whatever needs to happen to get this into master. Thanks!

We need to define __APPLE_USE_RFC_3542 before including <netinet/in.h> or compile errors are observed on macOS. Signed-off-by: Kyle Mestery <[email protected]>

mestery · 2020-05-28T02:44:29Z

@dlundquist I've got a few buffer fixes we've found running this PR internally, and I've got working functional testing via some DTLS code I found and a docker-compose.yml file. I shall update this PR by Friday with those changes.

Signed-off-by: Kyle Mestery <[email protected]>

dlundquist approved these changes Mar 28, 2020

View reviewed changes

src/dtls.c Show resolved Hide resolved

src/dtls.c Outdated Show resolved Hide resolved

dlundquist reviewed Mar 28, 2020

View reviewed changes

src/dtls.h Outdated Show resolved Hide resolved

Add DTLS functional test

dda7740

This adds a DTLS functional test, similar to the existing TLS functional test. For now, this has a single known good DTLS packet which we run the test against. More can be added as Wireshark captures allow. Signed-off-by: Kyle Mestery <[email protected]>

DTLS test: Make hostname part of test struct

83cd2c4

This adds the hostname from the test data into the test_packet structure, allowing for dynamic checking of hostnames from the data rather than hard coding "localhost". Signed-off-by: Kyle Mestery <[email protected]>

dlundquist and others added 3 commits March 29, 2020 16:49

Open UDP listeners

27c7975

poc: "accept" UDP connections

fa0c373

Add socket type to buffer and connection structs

4d1a6b5

This allows us to do some more advanced buffer processing depending on if we are buffering stream or datagram sockets. Signed-off-by: Kyle Mestery <[email protected]>

Fix dtls hostname checks for bad tests

b3e4c2f

Signed-off-by: Kyle Mestery <[email protected]>

mestery added 4 commits April 1, 2020 14:42

Update .gitignore to ignore .DS_Store on a Mac

9058c98

Signed-off-by: Kyle Mestery <[email protected]>

abort_message in Protocol should be unsigned char

6df5d51

DTLS abort messages include a version of `0xfe 0xfd` which gives warnings if abort_message is not an unsigned char. Signed-off-by: Kyle Mestery <[email protected]>

dlundquist requested changes Apr 1, 2020

View reviewed changes

src/buffer.c Outdated Show resolved Hide resolved

src/connection.c Outdated Show resolved Hide resolved

tests/Makefile.am Show resolved Hide resolved

src/buffer.h Outdated Show resolved Hide resolved

src/buffer.c Outdated Show resolved Hide resolved

dlundquist reviewed Apr 2, 2020

View reviewed changes

src/buffer.c Outdated Show resolved Hide resolved

src/buffer.c Outdated Show resolved Hide resolved

src/buffer.c Outdated Show resolved Hide resolved

src/buffer.h Outdated Show resolved Hide resolved

dlundquist and others added 4 commits April 2, 2020 16:49

buffer: rework datagram buffers

80287ea

Implement datagram specific buffer tails using setup_write_iov(), setup_read_iov(), advance_write_position() and advance_read_position(). Signed-off-by: Dustin Lundquist <[email protected]> Signed-off-by: Kyle Mestery <[email protected]>

connection: Read data after accepting dgram socket

6c35206

Once a datagram socket has been accepted, read any data sent on the socket right away. Signed-off-by: Kyle Mestery <[email protected]>

dlundquist reviewed Apr 3, 2020

View reviewed changes

src/buffer.c Outdated Show resolved Hide resolved

src/connection.c Outdated Show resolved Hide resolved

src/buffer.c Show resolved Hide resolved

src/buffer.c Outdated Show resolved Hide resolved

Address code review comments

4c77d6d

Signed-off-by: Kyle Mestery <[email protected]>

IPv6: Compile on macOS

ee0660b

We need to define __APPLE_USE_RFC_3542 before including <netinet/in.h> or compile errors are observed on macOS. Signed-off-by: Kyle Mestery <[email protected]>

Fix issue with buffer coalesse and buffer_push

b127c72

Signed-off-by: Kyle Mestery <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DTLS Protocol Support #353

DTLS Protocol Support #353

mestery commented Mar 27, 2020 •

edited

Loading

mestery commented Mar 27, 2020

dlundquist left a comment

dlundquist commented Mar 28, 2020 •

edited

Loading

mestery commented Mar 28, 2020

mestery commented Mar 28, 2020

mestery commented Mar 28, 2020

mestery commented Mar 29, 2020

dlundquist commented Mar 29, 2020

mestery commented Mar 29, 2020

mestery commented Mar 30, 2020

dlundquist left a comment

dlundquist left a comment

mestery commented Apr 6, 2020

mestery commented May 28, 2020

DTLS Protocol Support #353

Are you sure you want to change the base?

DTLS Protocol Support #353

Conversation

mestery commented Mar 27, 2020 • edited Loading

mestery commented Mar 27, 2020

dlundquist left a comment

Choose a reason for hiding this comment

dlundquist commented Mar 28, 2020 • edited Loading

mestery commented Mar 28, 2020

mestery commented Mar 28, 2020

mestery commented Mar 28, 2020

mestery commented Mar 29, 2020

dlundquist commented Mar 29, 2020

mestery commented Mar 29, 2020

mestery commented Mar 30, 2020

dlundquist left a comment

Choose a reason for hiding this comment

dlundquist left a comment

Choose a reason for hiding this comment

mestery commented Apr 6, 2020

mestery commented May 28, 2020

mestery commented Mar 27, 2020 •

edited

Loading

dlundquist commented Mar 28, 2020 •

edited

Loading