Closed ThaUnknown closed 2 years ago
Wow, thanks! Will review this asap, see reply Re: #2
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.
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
Here’s the RFC https://datatracker.ietf.org/doc/html/rfc5245#section-15.4
Here’s the RFC https://datatracker.ietf.org/doc/html/rfc5245#section-15.4
-
is rejected, my bad
this is broken, dont merge!
Noticed the package has process
again - I think that is an unintentional revert?
Noticed the package has
process
again - I think that is an unintentional revert?
yes, which is why the > dont merge sorry
this now works x)
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.
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.
Oh, one thing that may have been missed - I was using
random-string
notrandomstring
for the reasons you mentioned (therandombytes
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
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!)
Any idea when simple-peer calls randombytes? I can dig into it later, just wondering if you knew already.
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)
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 :)
Ah cool, yeah it would be ideal to drop them and if not avoid calling into them. We can pass those into simple peer.
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.
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)
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
Ah I see that the _id can’t be avoided - too bad
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
Thank you! Headed to airport but will test this tonight and merge if it looks good. Appreciate the PR!
Pulled into https://github.com/gfodor/p2pcf/pull/3, thank you for the PR!
note: #2 needs to be merged first!
randomstring
with the crypto API [which was already used byrandombytes
], 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 implementationrequire
making the module itself ESM compliant