-
Notifications
You must be signed in to change notification settings - Fork 309
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
SCRAM key signatures are derived incorrectly #68
Comments
Hey @SomeoneToIgnore, thanks for the report. There are two different issues here though. One is that something isn't supported in Now I am sure the team will fix the Now obviously it could be a problem in the implementation of either SubtleCrypto, but before we go that route, could you please verify that the output of the I have spent way too much time in my life wasted trying to find bugs in crypto software that turned out to be text encoding/decoding issues :D |
Hey @evanderkoogh , thank you for the good advice.
That covers all 3 places in the original snippet, where the encoder is used:
Local workerd (same version as in the issue):
Local deno:
I'm not entirely sure what to check on the "decode" side, there's no such thing in the API: https://developer.mozilla.org/en-US/docs/Web/API/TextEncoder and the The decode is done in depth of Please feel free to ask me for more experiments/tests in case needed! |
Hi @SomeoneToIgnore, In your latest comment it looks like you are saying that cloudflareworkers.com, local workerd, and local Deno all produced the same result? Is that right? So what is the issue at this point? |
I've double checked it with my latest comment what @evanderkoogh asked in his comment
The output of these functions is the same, so whatever Still, there are two major problems that exist:
Yet, I have provided you with the way to run the same thing with This is definitely a bug, the same input |
Oh sorry, I misunderstood your second comment. So the code you posted has a subtle bug unrelated to crypto:
Here, you are trying to use an async function as your event handler. Unfortunately, per the Service Workers standard, this doesn't work. What will happen here is the code will execute up to the first When you run this in When you run this in Cloudflare Workers, the request will fall back to going to your origin... do you actually see a useful result in this case? This weirdness is why we are trying to move away from Service Workers syntax and recommend ES modules syntax instead. If you use service workers syntax we highly recommend your event handler is always a single line: Anyway, would you mind fixing this issue and trying again? You should then at least get identical results on workerd and cloudflareworkers.com, as they run the same code. I'm not sure if any of this could explain the difference from Deno, though. |
Thank you @kentonv for a great elaboration, I've managed to fix local workerd and eventually find the culprit: yet another subtle language difference and, indeed, a Deno bug: denoland/deno#16180 The most interesting, is that I rewrite the same snippet for
pointing to this line: const key = await subtle.deriveKey(
{
hash: "SHA-256",
iterations,
name: "PBKDF2",
salt,
},
pbkdf2_password,
{ name: "HMAC", hash: "SHA-256" }, // <---------
false,
["sign"],
); and looks like Deno, indeed, uses wrong block size here and Node forces everybody to fill this in despite the parameter being optional. On the bright side, both of my original issues (wrong cryptographic numbers and example that failed to run in local workerd) seem to be solved. I can only note that the docs everywhere could use some love 🙂 |
Glad we figured it out! |
fixes #16180 `HMAC`'s `get key length` `op` uses the hash function's block size, not output size. refs cloudflare/workerd#68 (comment)
fixes #16180 `HMAC`'s `get key length` `op` uses the hash function's block size, not output size. refs cloudflare/workerd#68 (comment)
I have been trying to connect from a cloudflare worker to postgres, using SCRAM (which is the new default since PG14).
For that, a modified version of https://github.com/cloudflare/worker-template-postgres was used, which showed that PG with SCRAM auth method is never able to get connected to from the worker, while MD5 and passwordless methods worked just fine.
When similar example was run from Deno, it worked for all auth methods, including SCRAM with the same postgres.
I've narrowed down the case to the following snippet:
The snippet reuses slightly modified code from postgres deno driver:
https://github.com/denodrivers/postgres/blob/8a07131efa17f4a6bcab86fd81407f149de93449/connection/scram.ts#L93
and a test for that: https://github.com/denodrivers/postgres/blob/8a07131efa17f4a6bcab86fd81407f149de93449/tests/auth_test.ts#L6
based on https://www.rfc-editor.org/rfc/rfc7677
When launched from
deno
, it outputs the following:When deployed to the cloudflare worker, it outputs the following (sometimes, twice):
When run with workerd (built from f7bd5f4) locally, it outputs the following:
and returns internal server error.
This is a bug, according to https://github.com/cloudflare/workerd/blame/f7bd5f40adb3909cefcc63b7b06a2d6c1baafbd5/README.md#L40
I would expect both locally built workerd and the one from cloudflare to compute the signatures with their values identical to what Deno script produces for the same input.
The text was updated successfully, but these errors were encountered: