add(std/jwt): Implement the new jwt module

[timonson]

This PR contains the new std/jwt module and therefore closes #7771 .

@timreichen and I put equal amounts of work into this. So big Thank you to him!

A few additional things to consider:

• Although not necessary now, verify and create return promises because we
  expect coming encryption functions to return promises in the future. E.g. for
  RSA algorithms.

• This application uses the JWS Compact Serialization according to the
  JWS specification.

[timreichen]

@kitsonk ready for a second review.

[kitsonk]

LGTM, thanks for addressing all the feedback!!!

[panva]

I only stumbled on this issue because i was checking up on webcrypto presence in deno. Having some experience with JOSE as a whole i wanted to check what std jwt module may look like.

With no disrespect towards the authors whatsoever, everyone involved has my upmost respect for contributing to open source, I think maybe this should be reverted - or at the very least further worked on until being released as std.

1. Web Crypto API should be used for crypto operations and its CryptoKey representation should be used for secrets/keys. Not strings for HMAC-based JWT algorithms. Since you're starting with strings where do you move on from there once RSASSA, ECDSA, EdDSA comes in? Developers will start passing PEM keys like in Node.js and you'll be using them as HMAC secrets verbatim. Bad idea.

2. The module claims to be JWT, but seems to be doing tryToParsePayload and falling back onto a string payload. JWT's payload MUST always be a JSON top-level object. So, is it JWT or JWS? Why mix the two if the developer cannot instruct the lib what he expects to receive.

3. nbf validation is missing, same as with exp not allowing the token to be used anymore when expired, nbf doesn't allow it to be used because it's not valid yet.

4. alg=none should really not be supported. It even seems like the API allows not specifying an algorithm in which case - will a "none" token be accepted? Hope not - given that i see HS512 used as the default for the verify algorithm option, but then again - when RSASSA / ECDSA is added, such default is just awful, either use no defaults (and remove "none") or derive the appropriate algorithm from the key material.

5. the numerous occurences of "encrypt" when no encryption is in sight in the module.

I'm rewriting my jose module into a new universal ESM module with TS (wip here), so once Web Crypto API lands in Deno, it'll be long ready and released (with no dependencies this time). This new universal module (and the current node-specific version too) comes with almost all of JOSE (as long as supported by Web Crypto API), not just JWT, but also JWS, JWE, JWK, Flattened and General serializations, etc...

Again thank you all for the time invested here. But as a developer I would be looking for a different kind of module in a std.

These modules do not have external dependencies and they are reviewed by the Deno core team. The intention is to have a standard set of high quality code that all Deno projects can use fearlessly.

[timonson]

1. Deno doesn't have Web Crypto, yet. Only HS256 is required by the spec.
2. We mention that we use The JWS Compact Serialization, both in the readme and the code.
   I would concede that jws would be a better name technically, but the crossover between jwt and jws is complicated for users to understand. We tried to find a good compromise between simplicity and technical correctness here.
3. Nobody claimed this module has been completed yet. But the usage of the nbf claim is optional. Somebody will add it if needed.
   I really don't know what you mean by: ...exp not allowing the token to be used anymore when expired... The token can be used after being declared expired of course. The user can parse the token using the decode function and has access to all the token information and can return/respond whatever he likes.
4. You are wrong about everything in your first two sentences here. Especially the second sentence. Maybe actually take a look at the API before lying about it.
5. Are you arguing about the name of a function here? We obviously picked the name because of considerations about the future. By the way, encrypt doesn't have numerous occurrences and is used only internally.

A lot of words but little substance.

I am sure the deno devs will be happy to accept your PR, if it satisfies their conditions, in the future.

[panva]

I understand your defensive response, being the proud author of the module, I also want to reiterate, because this can never be said enough - thank you for being a fellow open source maintainer and I mean no disrespect or to offend here.

1. i think it's clear that i am aware webcrypto is not supported from my comment. The point that i failed to make clear is that string type for a secret or key argument is insufficient. A key-representing object with a clear type (oct/RSA/EC/OKP) removes any ambiguity about what a provided key argument is for.

2. not giving devs the affordance to say "i expect a JWT" vs. "i expect a JWS" can cause unexpected behaviours in their code. Since we're talking about security tokens here i'd say that it should be clear out of the box

3. standards. Am i right? Well, the nbf claim may be optional to use - meaning it doesn't have to part of the payload, but that doesn't mean the library responsible for verifying these claims is free to ignore it when actually present. Suggesting to use a token after being expired, using "decode" that ignores signatures to do it... i'll pass.

4. i checked again - The docs really suggest passing the algorithm option is optional, and it really does default to HS512. So where did I lie? The point was again - hardcoding algs without considering the key input leads to mistakes and vulnerabilities.

Substance is there, just please consider not taking this as an attack that you need to somehow defend, but rather me trying to help point this module in a better, more secure future proof direction.

[kitsonk]

@panva thank you for your feedback... we will discuss internally and figure out what to do.

[panva]

@kitsonk thank you for considering my feedback valuable. Good luck, stay safe.

[timonson]

i checked again - The docs really suggest passing the algorithm option is optional, and it really does default to HS512. So where did I lie? The point was again - hardcoding algs without considering the key input leads to mistakes and vulnerabilities.

Edit: I think you implied that an algorithm would not have to be specified, meaning even none would be accepted together with other algorithms. This is not the case. All we did is setting a default value what can be redone easily. I take back the lying and apologize.

[panva]

Because the use of algorithm is not optional in this library. Please check the types.

I checked the readme docs. Entirely my fault clearly. Who reads docs anymore.

Anyway, @kitsonk if you wish to discuss around some of the more high level points, please let me know and i can join discord/slack/whatever.

[timreichen]

Hei @panva Thank you for your concern raising. @timonson and I will work on a patch.
Point 2 and 3 will be fixed.

What did you mean exactly by

when RSASSA / ECDSA is added, such default is just awful, either use no defaults (and remove "none") or derive the appropriate algorithm from the key material.

? The spec demands the implementation of none as an *algorithm *, so that cannot be removed without spec deviation.

[panva]

@timreichen

The key material type passed in should be responsible for what algorithm is "allowed". When Uint8Array is passed in as key - only HMAC algs can be used, when ECDSA key object is passed in there's always only one algorithm (mapped per EC key curve). When RSA key object key is passed in - only RSASSA-PKCS1-v1_5 or RSASSA-PSS can be used. The list of algorithms can then be further limited by providing the algorithms option, but the algorithms should never stray from what a given key is capable of verifying/signing. Why HS512 is a bad default and string is a bad idea? Because someone eventually will pass in a PEM public key thinking they're verifying e.g. PS256 but its the octets of the key used for HMAC instead. Hence - key object representation. Uint8Array for secrets, CryptoKey (when available) for everything else.

Since it's currently only in scope to do HMAC - you only have one key type to worry about, but please, don't make string a valid input, have developers pass in a Uint8Array instance. (note: getting Uint8Array out of a string is as simple as new TextEncoder().encode(str)) so this isn't any burden for devs.

Once Web Crypto API is supported by Deno, you'll be have the other public/private key objects native to your runtime.

As far as "none" goes. If you feel like you MUST support it to be spec conform, then make "none" a special kind of key object representation. E.g. a singleton like i've done here. Only when this key is passed in can "none" JWTs be "verified" or "signed".

I've been dealing with JOSE for a very long time so this feedback / these suggestions come from a great deal of experience, especially in the node/browser JS ecosystems. Mixing "none" with the rest of the secure API is inherently a bad idea, going with a special "none" key object is the least amount of work (and goes in line with the general idea of "key type drives the algorithms", alternatively - provide a completely different API just for "none" alg validation.

As an aside: to be 100% conform for HMAC algorithms you also must ensure that the key is at least the bitsize of the HMAC algorithm blocksize - so >=256 bits for HS256, >=384 bits for HS384, >=512 bits for HS512.

[sanderhahn]

There are WebAPIs on which a more complete JWT could be build once available.

https://developer.mozilla.org/en-US/docs/Web/API/Web_Crypto_API
https://developer.mozilla.org/en-US/docs/Web/API/CryptoKey
https://developer.mozilla.org/en-US/docs/Web/API/SubtleCrypto/sign

[timreichen]

@panva
Thank you for your detailed response.

• I think passing Uint8Array instead of a string is reasonable.
• std/jwt should follow the spec and therefore support none. What makes a passed singleton more secure than a passed string?
• shouldn't the HMAC implementation validate the blocksize instead of jwt?

[panva]

std/jwt should follow the spec and therefore support none. What makes a passed singleton more secure than a passed string?

There are two sides to this - one - the key passed in, two - the algorithms option. Unless you want to be dealing with edge cases where Uint8Array key is passed in but algorithms option lists "none", go with a key object instead.

shouldn't the HMAC implementation validate the blocksize instead of jwt?

It doesn't, usually when keys are shorter they get padded to the required size, when they're longer, they're automatically hashed by the HMAC implementation.

[panva]

With the release of Deno v1.18 https://deno.land/x/jose can now be considered stable and safe to use. I am aware it is not a std library but it is maintained, well tested and already put to use.