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

Make certificates optional in the DtlsTransport constructor #867

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aboba
Copy link
Contributor

@aboba aboba commented Sep 12, 2018

Fix for Issue #866

@fippo
Copy link
Contributor

fippo commented Sep 12, 2018

what is the behaviour when null is passed? I assume "generate a new one"?

@aboba
Copy link
Contributor Author

aboba commented Sep 12, 2018

@fippo Yes. Now that ECDSA is the default ciphersuite, generation should not block the main thread even on modest processors.

Copy link
Contributor

@fippo fippo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused by ciphersuites, which for me is something like TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 in dtls (from jsep, 5.5)
Maybe this is key generation algorithm as used in https://w3c.github.io/webrtc-pc/#dom-rtcpeerconnection-generatecertificate ?

Either way, this makes sense.

@lgrahl
Copy link
Contributor

lgrahl commented Sep 13, 2018

Mh, I don't think I like this change. Blocking the main thread (even slightly) is never a good idea and generating a certificate is just one line.

index.html Outdated
<li><p>If <var>certificates</var> is non-null and is non-empty,
initialize the <a>[[\Certificates]]</a> internal slot to <var>certificates</var>.</p></li>
<li><p>If <var>certificates</var> is <code>null</code> or is empty,
generate a certificate using the default ciphersuite and store this in the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keygenalgorithm. I assume this is already fixed like in w3c/p2p-webtransport@1ace017 but not pushed :-)

@robin-raymond
Copy link
Contributor

@lgrahl I share your concern about blocking the main thread; I suspect on most desktops this will be < 40ms with P-256 default ECDSA but on slower less powerful mobile this could be 100ms+. We could never generate 2048bit RSA with blocking on a slow device (bad idea) so it's only ECDSA by default at best.

@aboba
Copy link
Contributor Author

aboba commented Oct 3, 2018

Looking at the ECDSA generation times, it would seem that generating a more secure ECDSA certificate (equivalent to RSA 2048) could take longer than 100 ms on a low-end processor. So putting this proposal on hold for now.

@aboba aboba added the Icebox label Oct 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants