Support DTLS headers when parsing encrypted packet lengths #2#13656
Support DTLS headers when parsing encrypted packet lengths #2#13656normanmaurer merged 4 commits intonetty:4.1from
Conversation
Motivation: Supporting DTLS headers allows users to use a DTLS SSLContext to build a SSLEngine for a SslHandler. There are probably more changes that need to be added to handle edge cases in DTLS, but this appears to work and starts the process of supporting DTLS. Modifications: Checks for the DTLS version after failing to find the TLS version. If DTLS version is found, we read the length of each DTLS record until there are no more records. Result: Users can create a SslHandler with a DTLS SSLContext and connect to a simple server using DTLS.
|
BTW if I backported this to the latest 3.X, would I just open another PR to that branch? |
|
@dmstocking 3.x is EOL for at least 5 years... maybe even more. So there will be no more release of it. |
GMSSL and DTLS both read the whole short as a version code. We can reuse the variable instead of rereading from the buffer.
|
@normanmaurer Thanks for reviewing and answering my questions. I have it backwards. Is there anything I need to do to get this into 5.X or is there an automated process to do that? |
Adds a check to make sure the buffer has the required bytes to read before we try and get the DTLS record's length. This also adds a test to make sure we will return NOT_ENOUGH_DATA in the event we don't have enough bytes to get to the DTLS record's length.
|
@normanmaurer Those changes you made look good. Is there anything left to do on my end to get this merged? |
|
@dmstocking did you sign our ICLA ? https://netty.io/s/icla |
|
@normanmaurer I am really confident sign it once. I think I signed it under |
|
Do we have dedicated test cases for DTLS end-to-end? |
|
@hyperxpro I don't think so. I would guess not because DTLS definitely doesn't work before this review. I think you could make an argument that it isn't a necessity. The SSLEngine is responsible for telling the SslHandler what to do. So as long as the SslHandler works with various SSLEngines, it should work with a DTLS SSLEngine. That is what I did BTW. I used Java's DTLS SSLEngine. I am pretty sure is in Java 11+. So far, I haven't noticed an issue, but it isn't like I have ran it for hundreds of hours. If we want one and someone points in to where I can do that, I can try to make that a thing. IDK what options exist for DTLS SSLEngines in Java 8. |
|
You could add something to SslHandlerTest maybe ? |
|
@normanmaurer Am I allowed to use Java 11 APIs in the tests? My understanding was this was using Java 8. Otherwise, I don't know how to get a Java DTLS SSLEngine. I could try to use bouncy castles DTLS engine. I am assuming they have one. Hoping it works in Java 8. |
|
@dmstocking I guess you could use java8 with reflection to setup DTLS if java version >= 11 |
|
@normanmaurer Going to try and see if doing SSLContext.getInstance("DTLS") return null or throws or does something I can tell if it exists or not. If it can, I can "disable" the test. I will admit, I don't think this will test much. It looks like these SslHandler tests spin up both a server and client. So if there is a bug, you risk not detecting it because both sides are using the same code. |
|
@dmstocking I think it is better then nothing. That said we can do a followup |
|
Thanks a lot! |
Motivation: Supporting DTLS headers allows users to use a DTLS SSLContext to build a SSLEngine for a SslHandler. There are probably more changes that need to be added to handle edge cases in DTLS, but this appears to work and starts the process of supporting DTLS. Modifications: Checks for the DTLS version after failing to find the TLS version. If DTLS version is found, we read the length of each DTLS record until there are no more records. Result: Users can create a SslHandler with a DTLS SSLContext and connect to a simple server using DTLS. --------- Co-authored-by: David Stocking <david.stocking@stryker.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com>
|
@normanmaurer No problem! Going to trying to find time to get that SslHandlerTest up. This ended up saving me so much time and the bugs of writing my own SSLEngine processing. That is what everyone else does to solve this problem. I ended up adding DTLS support in my project a week thanks to Netty. 😄 |
…etty#13656) Motivation: Supporting DTLS headers allows users to use a DTLS SSLContext to build a SSLEngine for a SslHandler. There are probably more changes that need to be added to handle edge cases in DTLS, but this appears to work and starts the process of supporting DTLS. Modifications: Checks for the DTLS version after failing to find the TLS version. If DTLS version is found, we read the length of each DTLS record until there are no more records. Result: Users can create a SslHandler with a DTLS SSLContext and connect to a simple server using DTLS. --------- Co-authored-by: David Stocking <david.stocking@stryker.com> Co-authored-by: Norman Maurer <norman_maurer@apple.com> (cherry picked from commit 32a025b)
Motivation:
Supporting DTLS headers allows users to use a DTLS SSLContext to build a SSLEngine for a SslHandler. There are probably more changes that need to be added to handle edge cases in DTLS, but this appears to work and starts the process of supporting DTLS.
Modifications:
Checks for the DTLS version after failing to find the TLS version. If DTLS version is found, we read the length of each DTLS record until there are no more records.
Result:
Users can create a SslHandler with a DTLS SSLContext and connect to a simple server using DTLS.
While trying to update this PR, I seem to have broken the previous PR. Sorry about that. I hate branches and tried to just amend the commit instead of needing a merge branch. Turns out that was not a good idea with github lol.