perf: use crypto and ESM

[ThaUnknown]

note: #2 needs to be merged first!

• replaces the not random randomstring with the crypto API [which was already used by randombytes], a drawback of this is that it doesn't specify the string length, [is this needed?]
  - replaces some deps with the built-in atob/btoa which is deprecated in node, but is supported in browsers and is consistent with CF's implementation
• drops require making the module itself ESM compliant

[gfodor]

Wow, thanks! Will review this asap, see reply Re: #2

[gfodor]

The string length doesn’t matter for the session id but we’ll need to confirm the UUIDs comply with the spec requirements for the username frag and password for ICE.

[ThaUnknown]

The string length doesn’t matter for the session id but we’ll need to confirm the UUIDs comply with the spec requirements for the username frag and password for ICE.

I didn't test that, didn't think much of it, but that should be looked at considering i havent

[gfodor]

Here’s the RFC https://datatracker.ietf.org/doc/html/rfc5245#section-15.4

[ThaUnknown]

Here’s the RFC https://datatracker.ietf.org/doc/html/rfc5245#section-15.4

- is rejected, my bad

[ThaUnknown]

this is broken, dont merge!

[gfodor]

Noticed the package has process again - I think that is an unintentional revert?

[ThaUnknown]

Noticed the package has process again - I think that is an unintentional revert?

yes, which is why the > dont merge
sorry

[gfodor]

DTLS fingerprints are hex encoded, so that conversion has to be maintained

[ThaUnknown]

this now works x)

[gfodor]

Oh, one thing that may have been missed - I was using random-string not randomstring for the reasons you mentioned (the randombytes dep in particular seemed bad.)

https://github.com/valiton/node-random-string/blob/master/lib/random-string.js

In light of that does this PR still make sense? The actual routine has no dependencies afaik.

[gfodor]

Btw that change was made recently so might mean we could drop some of the esbuild browserify bits. I think those get tree shaken out but def worth seeing if that helps to remove the ones not needed by simple-peer.

[ThaUnknown]

Oh, one thing that may have been missed - I was using random-string not randomstring for the reasons you mentioned (the randombytes dep in particular seemed bad.)

https://github.com/valiton/node-random-string/blob/master/lib/random-string.js

In light of that does this PR still make sense? The actual routine has no dependencies afaik.

random-string isnt random, simple-peer uses randombytes anyways

[gfodor]

Ah ok - so rn this PR drops that dep and improves the security of the ICE creds?

I’m a little worried about randombytes since it drops into an infinite loop if Buffer is not in the global namespace :/ I figured people will have browserify or esbuild packing it but it is quite a gnarly failure mode. (There is a while(true) with a try/catch!)

[gfodor]

Any idea when simple-peer calls randombytes? I can dig into it later, just wondering if you knew already.

[evan-brass]

Not sure if this is helpful, but this is what I used to generate the ufrag and password in my tests:

function gen_random_ice_str(byte_len) {
	const bytes = crypto.getRandomValues(new Uint8Array(byte_len));
	const byte_str = bytes.reduce((accum, v) => accum+String.fromCharCode(v), '');
	return btoa(byte_str).replaceAll('=', '');
}

ICE ufrag is a minimum of 3 bytes of entropy and the ICE password is a minimum of 16 bytes of entropy. So

const ufrag = gen_random_ice_str(3);
const password = gen_random_ice_str(16)

[ThaUnknown]

Any idea when simple-peer calls randombytes? I can dig into it later, just wondering if you knew already.

when generating random id/channel name

’m a little worried about randombytes since it drops into an infinite loop if Buffer is not in the global namespace :/ I figured people will have browserify or esbuild packing it but it is quite a gnarly failure mode. (There is a while(true) with a try/catch!)

yeah this is where I was heading with all my changes, to drop the need to polyfill anything at all :)

[gfodor]

Ah cool, yeah it would be ideal to drop them and if not avoid calling into them. We can pass those into simple peer.

[gfodor]

And yep that is a good catch @evan-brass - the per character entropy of the current code isn’t the entire byte so my lengths are too conservative.

[gfodor]

I think these changes would be good:

• fix the entropy issue as per @evan-brass ’s approach

• avoid call paths to randombytes by passing in safely generated ids to simple-peer

• top comment mentioned something about a require making it a non-esm, didn’t see that here, just checking

• remove any unnecessary browserify deps for local esbuild (these still may be necessary due to simple-peer)

[ThaUnknown]

avoid call paths to randombytes by passing in safely generated ids to simple-peer

not possible, you'd need to drop simple-peer to do that

remove any unnecessary browserify deps for local esbuild (these still may be necessary due to simple-peer)

they are all necessary for simple-peer as it uses node:event node:buffer node:process and node:stream

this is why i suggested polite-peer instead as it's dependency free, just a little bit more bare bones

[gfodor]

Ah I see that the _id can’t be avoided - too bad

[ThaUnknown]

I think these changes would be good:

• fix the entropy issue as per @evan-brass ’s approach
• avoid call paths to randombytes by passing in safely generated ids to simple-peer
• top comment mentioned something about a require making it a non-esm, didn’t see that here, just checking
• remove any unnecessary browserify deps for local esbuild (these still may be necessary due to simple-peer)

should be done

[gfodor]

Thank you! Headed to airport but will test this tonight and merge if it looks good. Appreciate the PR!

[gfodor]

Pulled into #3, thank you for the PR!