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

Cannot find crypto in Cloudflare Worker #5

Closed
ryan-mars opened this issue Nov 9, 2021 · 17 comments
Closed

Cannot find crypto in Cloudflare Worker #5

ryan-mars opened this issue Nov 9, 2021 · 17 comments
Labels
bug Something isn't working

Comments

@ryan-mars
Copy link

ryan-mars commented Nov 9, 2021

When deploying a Cloudflare worker I get an error.

Error: Something went wrong with the request to Cloudflare...
Uncaught Error: No such module "crypto".
  at line 0
 [API code: 10021]

Somewhere in detectPRNG() it's not finding that crypto is on globalThis in the Worker, so it is falling through to require("crypto") which doesn't work in Cloudflare Workers.

ulidx/source/ulid.ts

Lines 63 to 65 in 578a98b

const webCrypto =
(rootLookup && (rootLookup.crypto || rootLookup.msCrypto)) ||
(typeof crypto !== "undefined" ? crypto : null);

@perry-mitchell perry-mitchell added the bug Something isn't working label Dec 30, 2021
@perry-mitchell
Copy link
Owner

Hi @ryan-mars - Thanks again for looking into the cloudflare worker functionality. At this time I can absolutely understand why you forked the library - I don't agree with Cloudflare's implementation of their basic JS APIs and therefore won't be supporting Cloudflare workers.

I have updated the PRNG detection, which I hope would now work on Cloudflare workers, but due to the time being irrelevant, somewhat, I wouldn't ever advise anyone to use ulidx on them. Better to generate the IDs off-site on a proper server rather than try to generate them in such a broken environment.

I've added a statement to the readme to clarify that they're not supported, at least for now. Perhaps in the future if they change their stance this library might function correctly.

Cheers.

@grempe
Copy link
Contributor

grempe commented Jan 1, 2022

I don't agree with Cloudflare's implementation of their basic JS APIs and therefore won't be supporting Cloudflare workers.

And also your comment in this now closed PR:
#6 (comment)

While the application executes, time is locked in place.
This sounds.. absolutely terrible. I don't understand why they'd choose to do this. All the same, I don't know how this library is meant to work when time can't be relied upon. It's a crucial part of the algorithm.

and

but due to the time being irrelevant, somewhat, I wouldn't ever advise anyone to use ulidx on them. Better to generate the IDs off-site on a proper server rather than try to generate them in such a broken environment.

If your concern was with how Cloudflare Workers handle time (time freezes at the time of the request), I would suggest you need to understand why they made that choice and why it was important (and should perhaps be emulated and not derided). This was done to help prevent side-channel attacks and Spectre style attacks. This page has a lot of additional context on security choices as well. Worker time can certainly be relied upon, it just reflects the start of the current request, not time changes during a request.

https://developers.cloudflare.com/workers/learning/security-model#step-1-disallow-timers-and-multi-threading

As a result they state:

In adversarial testing and with help from leading Spectre experts, Cloudflare has not been able to develop a remote timing attack that works in production.

As a worker is an instance of a V8 isolate, the time is only frozen for the duration of a request, which is typically a small number of ms of server time. New requests get the current time for that request. So not sure how this meaningfully impacts the generation of a ULID in a worker.

Some additional info that might help:

https://developers.cloudflare.com/workers/learning/how-workers-works
https://blog.cloudflare.com/cloud-computing-without-containers/

@perry-mitchell
Copy link
Owner

@grempe I disagree purely because it's such a fundamental attribute of the environment, and one which this (and many other) libraries rely upon. I am not deriding Cloudflare's efforts to improve security, but their chosen mitigation is one I don't see as being overly friendly to time-sensitive libraries such as this.

I'd happily support Cloudflare workers in this fork, if:

  • Time measurement can be ensured to work in a reliable manner, ensuring that ULID generation behaves the same way on a worker that it does in a normal Node environment
  • Tests can be added that run on a worker (I'm sure that this might be possible, but haven't looked into it)

The values of start and end will always be exactly the same. The attacker cannot use Date to measure the execution time of their code,

time is only frozen for the duration of a request, which is typically a small number of ms of server time

Doesn't this mean that, when generating ULIDs during that time period, the date-time portion will remain static? If so, I'm afraid my response is one simply of necessity.. I don't see a way around this limitation. Besides this timing restriction I guess the library should just work after the most recent update (crypto detection)..

@ryan-mars
Copy link
Author

@perry-mitchell

Doesn't this mean that, when generating ULIDs during that time period, the date-time portion will remain static?

Yes that is correct, which is why only monotonic factories should be used for generating date encoding id's on Cloudflare Workers. Still this is not great in a distributed system for obvious reasons.

@grempe
Copy link
Contributor

grempe commented Jan 2, 2022

So that people can have a better idea of how this works in the real environment, and the caveat that applies, please see this repo I created this morning. It allows anyone to generate ULIDs (up to 1000 per request) with a simple HTTP GET request by CLI or in your browser.

It also allows you to choose the monotonic generator (default) or to turn it off as well as allowing you to provide a seed value (UNIX Epoch milliseconds).

https://github.com/truestamp/ulid-generator

By visiting https://ulid.truestamp.com/?q=10 you can see that the timestamp component of the generated ULIDs reflects the request time (new for each request), but this has no effect on the ability to generate 1.21e+24 unique ULIDs per millisecond as called for in the ULID spec.

I wrote up a caveat section in the README that describes when this might be a problem for your use case (in which case, please do generate your ULIDs somewhere else). In most use cases I think this is likely irrelevant. In any case, it is not really a concern of the library code which has no control over the time accuracy of the clock in the runtime environment it is running in. NodeJS does not solve this problem for you if e.g. your Docker container or virtual host is reporting an inaccurate or stale time.

If you have proposed changes to any of that please do let me know.

The code for the whole API server (<50 LOC) can be found here:

https://github.com/truestamp/ulid-generator/blob/main/src/handler.ts

@perry-mitchell : please feel free to use this if you think its a helpful demo of your library. And please turn on issues for your repo. :-)

Cheers.

@perry-mitchell
Copy link
Owner

Thanks @grempe, I appreciate it. I'll keep an eye on this. I suppose then advising people to use the monotonic factory only on workers is probably enough to ensure somewhat normal operation.

And please turn on issues for your repo.

Aren't we discussing in an issue? 😅

@grempe
Copy link
Contributor

grempe commented Jan 3, 2022

I'm not sure you even need the monotonic. But it can't hurt I think.

Aren't we discussing in an issue? 😅

I was referring to @ryan-mars repo issues which is why I tagged him in that line. 😁

@ryan-mars
Copy link
Author

ryan-mars commented Jan 5, 2022

I was referring to @ryan-mars repo issues which is why I tagged him in that line. 😁

@grempe Fixed, please add your issues. https://github.com/ryan-mars/ulid-workers/issues

@bluet
Copy link
Contributor

bluet commented Dec 30, 2022

Thanks @grempe, I appreciate it. I'll keep an eye on this. I suppose then advising people to use the monotonic factory only on workers is probably enough to ensure somewhat normal operation.

@perry-mitchell maybe need to update the README.md ?

@perry-mitchell
Copy link
Owner

@bluet I'd accept a PR for that if you're interested in documenting :)

@ryan-mars
Copy link
Author

I'd be happy for https://github.com/ryan-mars/ulid-workers to no longer be necessary.

@perry-mitchell
Copy link
Owner

@ryan-mars Its been some time since your project was released- was there anything special regarding Cloudflare worker support you had to implement? I took a brief look at the source and saw there was no standard ulid function, just the monotonic factory as was mentioned earlier in this thread.

I'd be willing to have the changes made here to support workers.

@perry-mitchell
Copy link
Owner

It would also be great to have tests for this, but not sure how doable that is without at least paying for cloudflare workers.

Having some kind of safety for not allowing the ulid method on workers would also help. Some detection method.

@perry-mitchell
Copy link
Owner

New feature @ #13 for official support.

@ryan-mars
Copy link
Author

ryan-mars commented Jan 1, 2023

@ryan-mars Its been some time since your project was released- was there anything special regarding Cloudflare worker support you had to implement? I took a brief look at the source and saw there was no standard ulid function, just the monotonic factory as was mentioned earlier in this thread.

@perry-mitchell two major things:

First, obviously, non-monotonic generation may result in unpredictable behavior including the potential that ULIDs will lexically sort in a different order than they were generated.

Second, an issue without an obvious solution, monotonic generation of ULIDs near max entropy can roll over thereby incrementing the timestamp by 1ms

@perry-mitchell
Copy link
Owner

First, obviously, non-monotonic generation may result in unpredictable behavior including the potential that ULIDs will lexically sort in a different order than they were generated.

This makes sense and we could probably throw in this situation, potentially, or just document this well.

monotonic generation of ULIDs ryan-mars/ulid-workers#8 thereby incrementing the timestamp by 1ms

@ryan-mars I'm confused, the last comment by @grempe in that issue seemed to hint that there was no such problem - the time component shouldn't be affected.. Is there something I'm missing here?

it is solving a problem that does not actually exist (the rollover of time) and that complexity is only preventing throwing an error in the infinitesimal chance of getRandomValues() providing a value that would cause a rollover within the 1ms window.

There's a lot on conjecture in that issue and I'm not sure I see the whole picture. I know that this library's code is working nominally in standard expected environments, but is it currently working as such within cloudflare workers using monotonic generation, or not? Will the time component roll over if I generate a ton of ulids using the monotonic factory, or will it throw an exception? I haven't had a recent look at this code even in this repo.. But I suspect that it'll just throw - if that's the case it is, in my opinion, operating as expected.

Summa summarum I think throwing an error is preferrable to inaccurate/invalid values. If the monotonic factory works otherwise perhaps we could move forward with such a solution here.

@ryan-mars
Copy link
Author

There's a lot on conjecture in that issue and I'm not sure I see the whole picture.

@perry-mitchell For the record, I never quite understood if the concerns in that issue were realistic. I think things are fine as you have them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants