-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DTLS: Use bio callback to get fragment packet. v5.0.156, v6.0.47 #3565
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
winlinvip
force-pushed
the
bugfix/dtls-fragment
branch
2 times, most recently
from
June 2, 2023 11:34
e7317c0
to
cbb4584
Compare
winlinvip
force-pushed
the
bugfix/dtls-fragment
branch
3 times, most recently
from
June 3, 2023 01:54
92a5f9a
to
6becadd
Compare
winlinvip
force-pushed
the
bugfix/dtls-fragment
branch
from
June 3, 2023 12:07
6becadd
to
2c96b32
Compare
winlinvip
force-pushed
the
bugfix/dtls-fragment
branch
2 times, most recently
from
June 4, 2023 10:00
d9e8256
to
2d1db8a
Compare
winlinvip
force-pushed
the
bugfix/dtls-fragment
branch
from
June 4, 2023 10:07
2d1db8a
to
bcb62ee
Compare
winlinvip
changed the title
DTLS: Use bio callback to get fragment packet.
DTLS: Use bio callback to get fragment packet. v5.0.156, v6.0.47
Jun 5, 2023
chundonglinlin
approved these changes
Jun 5, 2023
winlinvip
added a commit
that referenced
this pull request
Jun 5, 2023
1. The MTU is effective, with the certificate being split into two DTLS records to comply with the limit. 1. The issue occurs when using BIO_get_mem_data, which retrieves all DTLS packets in a single call, even though each is smaller than the MTU. 1. An alternative callback is available for using BIO_new with BIO_s_mem. 1. Improvements to the MTU setting were made, including adding the DTLS_set_link_mtu function and removing the SSL_set_max_send_fragment function. 1. The handshake process was refined, calling SSL_do_handshake only after ICE completion, and using SSL_read to handle handshake messages. 1. The session close code was improved to enable immediate closure upon receiving an SSL CloseNotify or fatal message. ------ Co-authored-by: chundonglinlin <[email protected]>
winlinvip
added a commit
that referenced
this pull request
Jun 5, 2023
1. The MTU is effective, with the certificate being split into two DTLS records to comply with the limit. 2. The issue occurs when using BIO_get_mem_data, which retrieves all DTLS packets in a single call, even though each is smaller than the MTU. 3. An alternative callback is available for using BIO_new with BIO_s_mem. 4. Improvements to the MTU setting were made, including adding the DTLS_set_link_mtu function and removing the SSL_set_max_send_fragment function. 5. The handshake process was refined, calling SSL_do_handshake only after ICE completion, and using SSL_read to handle handshake messages. 6. The session close code was improved to enable immediate closure upon receiving an SSL CloseNotify or fatal message. ------ Co-authored-by: chundonglinlin <[email protected]>
winlinvip
added a commit
that referenced
this pull request
Jun 5, 2023
1. The MTU is effective, with the certificate being split into two DTLS records to comply with the limit. 2. The issue occurs when using BIO_get_mem_data, which retrieves all DTLS packets in a single call, even though each is smaller than the MTU. 3. An alternative callback is available for using BIO_new with BIO_s_mem. 4. Improvements to the MTU setting were made, including adding the DTLS_set_link_mtu function and removing the SSL_set_max_send_fragment function. 5. The handshake process was refined, calling SSL_do_handshake only after ICE completion, and using SSL_read to handle handshake messages. 6. The session close code was improved to enable immediate closure upon receiving an SSL CloseNotify or fatal message. ------ Co-authored-by: chundonglinlin <[email protected]>
johzzy
pushed a commit
to johzzy/srs
that referenced
this pull request
Jun 26, 2023
…rs#3565) 1. The MTU is effective, with the certificate being split into two DTLS records to comply with the limit. 2. The issue occurs when using BIO_get_mem_data, which retrieves all DTLS packets in a single call, even though each is smaller than the MTU. 3. An alternative callback is available for using BIO_new with BIO_s_mem. 4. Improvements to the MTU setting were made, including adding the DTLS_set_link_mtu function and removing the SSL_set_max_send_fragment function. 5. The handshake process was refined, calling SSL_do_handshake only after ICE completion, and using SSL_read to handle handshake messages. 6. The session close code was improved to enable immediate closure upon receiving an SSL CloseNotify or fatal message. ------ Co-authored-by: chundonglinlin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the event that the certificate is large, such as a 4096-bit certificate or contains a significant amount of information, the DTLS packet may exceed the MTU of 1200 bytes, even if the MTU has been set using SSL_set_mtu.
To reproduce this issue, you can change the SRS certificate to 4096 bits:
Then, please configure SRS to use the DTLS client role:
To clarify further, the MTU is indeed effective. As an example, the fourth packet, which is the certificate, is split into two DTLS records. The first record is 1092 bytes, while the second record is 130 bytes.
Therefore, it is important to remember that this bug is not related to the MTU setting of DTLS, which may have caused confusion among myself and others.
The DTLS record has already been divided to comply with the MTU limit, ensuring that each record is smaller than 1200 bytes!!!
The issue arises when we use
BIO_get_mem_data
to retrieve the DTLS packet response to the peer. In this case, we receive all the packets, which total 2056 bytes. These packets include the ServerHello (82B), CertificateFragment (1092B), CertificateFragment (130B), ServerKeyExchange (564B), CertificateRequest (66B), and ServerHelloDone (12B). Although each of these packets is smaller than the MTU, we receive all of them in a single call.It is possible to parse the DTLS record individually and send them as separate UDP packets, with each DTLS record contained within a single UDP packet.
However, this approach is not ideal, and we would prefer not to use it. This is because SSL should have already handled this process, and we should be able to use the appropriate SSL API to obtain the correct packet for transmission.
For instance, Janus does not encounter this issue because it does not use
BIO_new
. Instead, it usesBIO_janus_dtls_agent_new
, which returns a BIO withBIO_METHOD
.If you wish to use
BIO_new
withBIO_s_mem
, there is an alternative callback available.This is the intended outcome of this patch, and as a result, we can observe that we receive fragment UDP packets, each of which is smaller than the MTU.
By the way, we have made some improvements to the MTU setting. We referred to mediasoup's implementation (versatica/mediasoup#217) when adding the
DTLS_set_link_mtu
function, and we have also removed theSSL_set_max_send_fragment
function as it was deemed unnecessary. See bellow code:As part of our refinement process, we have made changes to the handshake process. Specifically, we now call SSL_do_handshake only after ICE has completed, since SSL_read can handle the handshake messages in case the handshake process is not yet complete. See bellow code:
Upon receiving a DTLS packet, regardless of whether it's a handshake message or an application message, we can easily process it by feeding it to bio_in and then consuming it using SSL_read. It's worth noting that there's no need to use SSL_do_handshake to consume the handshake message since SSL_read can handle it on its own. See bellow code:
It has been observed that SSL automatically resets the previous timeout to zero upon receiving DTLS packets, eliminating the need for manual resetting. Therefore, all that is required is to set the timeout as demonstrated in the following code:
Additionally, within the ARQ loop, it is important to obtain the timeout using DTLSv1_get_timeout and sleep for a specific duration to ensure precise retransmission timeout when calling DTLSv1_handle_timeout. Using a constant timeout for sleeping and calling DTLSv1_handle_timeout may result in a slightly imprecise and inaccurate timeout, but the deviation should not be significant. See bellow code:
The session close code has been refined to enable immediate closure upon receiving an SSL CloseNotify or fatal message. For instance, if a DTLS handshake fails, the session is closed due to the fatal IP (Illegal Parameter) message.