Skip to content
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 Bio read buffer is to small and not compatible with ecdsa-with-SHA256 key/cert with CA #828

Open
zucher opened this issue Feb 10, 2023 · 18 comments
Labels
question Further information is requested stale There has been no recent activity

Comments

@zucher
Copy link

zucher commented Feb 10, 2023

With a Janus DTLS certificate with 4096 size, DTLS handshake goes in timeout, the transsaction never succeed.

a related issue was solved few year ago on janus side: meetecho/janus-gateway#252

after investiguation it appears that the ssl bio_read buffer is too small to read the full certificate.

see

data = self.ssl.bio_read(1500)

I do some test ad I suggest to increase the bio_read buffer size
from:

try:
            data = self.ssl.bio_read(1500)
        except SSL.Error:
            data = b""
...

to a bigger value

try:
            data = self.ssl.bio_read(8192)
        except SSL.Error:
            data = b""
...

It will solves also this closed issue: #346

an alternative but not tested, could be to read in a loop all the bio_read data, and send the full buffer after at

await self.transport._send(data)

@zucher zucher changed the title DTLS Bio buffer is to small and not compatible with 4096 key/cert DTLS Bio read buffer is to small and not compatible with 4096 key/cert Feb 10, 2023
@dfybc
Copy link

dfybc commented Feb 15, 2023

hi,
I increase the bio_read buffer size from 1500 to 8192, but error still occured.
i send 16kb data every 0.1 second through the data channel。
error happened as following:
2023-02-15 13:59:06.453708 - aiortc.rtcdtlstransport - WARNING - RTCDtlsTransport(client) Traceback (most recent call last):
File "/usr/local/lib/python3.7/dist-packages/aiortc/rtcdtlstransport.py", line 460, in __run
await self._recv_next()
File "/usr/local/lib/python3.7/dist-packages/aiortc/rtcdtlstransport.py", line 545, in _recv_next
await self._write_ssl()
File "/usr/local/lib/python3.7/dist-packages/aiortc/rtcdtlstransport.py", line 630, in _write_ssl
data = self.ssl.bio_read(8192)
File "/usr/local/lib/python3.7/dist-packages/OpenSSL/SSL.py", line 2029, in bio_read
self._handle_bio_errors(self._from_ssl, result)
File "/usr/local/lib/python3.7/dist-packages/OpenSSL/SSL.py", line 2004, in _handle_bio_errors
raise ValueError("unknown bio failure")
ValueError: unknown bio failure

@zucher
Copy link
Author

zucher commented Feb 15, 2023

What is the size of your Janus DTLS certificate ?

@dfybc
Copy link

dfybc commented Feb 18, 2023

What is the size of your Janus DTLS certificate ?

I don't know how do i found this size ? could you tell me how to find it?

@zucher
Copy link
Author

zucher commented Feb 18, 2023

If you don't have access to Janus server, you can use Wireshark to analyse the exchange
image (1)

@zucher
Copy link
Author

zucher commented Mar 8, 2023

How to progress on this topic ?

@jlaine
Copy link
Collaborator

jlaine commented Apr 1, 2023

A PR with a failing test case would help, or at least a full wireshark capture.

What I don't understand is how you expect to send out a frame that large, how will it fit in a UDP datagram?

@jlaine jlaine added the question Further information is requested label Apr 1, 2023
@zucher
Copy link
Author

zucher commented May 17, 2023

Hi, sorry for this long delay
The issue is not around UDP datagram but around the bio_read DTLS exchange, in the current case the buffer is to small to achieve the complete DTLS transaction and the exchange certificate is bigger than the read buffer, the certificate won't never be fully readed.

Currently we have the following error: https://www.pyopenssl.org/en/latest/api/ssl.html#OpenSSL.SSL.WantReadError on bio_read at 630.

Doing the loop over bio_read doesn't help, the WantRearError is treated after BIO_Read call in pyopenssl lib

See wireshark traces.
dtls_bio_read.zip

Thanks

@zucher
Copy link
Author

zucher commented Aug 2, 2023

Hi @dfybc and @jlaine , did you take a look into the traces ?

Regards

@zucher
Copy link
Author

zucher commented Aug 28, 2023

Any status ?

@lgrahl
Copy link

lgrahl commented Aug 28, 2023

What I don't understand is how you expect to send out a frame that large, how will it fit in a UDP datagram?

It will be framed across multiple UDP datagrams. DTLS explicitly frames for that purpose.

8192 bytes seems sensible for large x509 certificates using RSA 4096. If it resolves the problem for you, I'd suggest to make a PR for it.

Edit: This probably won't resolve your issue. You'll have to tell the DTLS stack the maximum MTU so that it fragments accordingly. Similar to versatica/mediasoup#1143

@jlaine
Copy link
Collaborator

jlaine commented Oct 31, 2023

I'm revisiting this issue and I have to admit I'm a bit confused.

The call to self.ssl.bio_read(1500) takes data from the TLS stack and sends them out to the network.

If the remote (non-aiortc) party is sending us a certificate, I would expect the call to self.ssl.recv(1500) to be the problematic one?

@zucher
Copy link
Author

zucher commented Oct 31, 2023

When the remote party is sending multiple fragment of incoming certificate, th gol of bio_read parameter is to give the size of the buffer to allocate to receive the concatenated fragments. As where as self.ssl.recv( MTU ) define the maximum buffer for on ip frame, so the MTU. the bio contains uncrypted data, so to be able to encrypt or decrypt a frame you have to receive the full buffer, of define a buffer for encryption before sending.

so sefl.ssl.recv(1500) is for me ok, and bios read depends of the max amount of data you expect to receive. In the case of certificate you have to receive or send the full content to be able to start decryption or encryption.

take a look into https://www.roxlu.com/2014/042/using-openssl-with-memory-bios , there is an exemple of usage of bios_read in line with this MR.

After today we have 4096 bytes certificates , by tomorrow could be more. Should we put this variable more flexible ?

@jlaine
Copy link
Collaborator

jlaine commented Dec 2, 2023

I think the world is moving away from RSA certificates, so I'm not sure whether this is worth the trouble..

@zucher
Copy link
Author

zucher commented Dec 2, 2023

Thank you @jlaine , in the meantime of the RSA replacemnt, I will patch during deployment of aiortc to be in line with our constraint.

@zucher
Copy link
Author

zucher commented Dec 4, 2023

Hi just saw our certificate is not RSA but ecdsa-with-SHA256 compliant, so update :p . And in that case the certificate returned by our Janus server is larger than the MTU. See attached Wireshark traces here ( #828 (comment) )

@zucher zucher changed the title DTLS Bio read buffer is to small and not compatible with 4096 key/cert DTLS Bio read buffer is to small and not compatible with ecdsa-with-SHA256 key/cert with CA Jan 19, 2024
@jlaine
Copy link
Collaborator

jlaine commented Jan 25, 2024

I am not able to read the .pcap file you provided, as your screenshot illustrates, Wireshark is unable to parse the DTLS messages so I have nothing to go on here. Unless you can provide me with a better way of reproducing this issue I'm going to have to close it.

@zucher
Copy link
Author

zucher commented Jan 25, 2024

@jlaine I will try to generate similar certificate for the validation.

Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale There has been no recent activity label May 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested stale There has been no recent activity
Projects
None yet
Development

No branches or pull requests

4 participants