-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use a better algorithm than UTF-8 to derive keys from string secrets. #620
Comments
@madmox Buffer is already an accepted secret argument type, you're not required to pass a string. const s = crypto.randomBytes(32).toString('hex')
const b = Buffer.from(string, 'hex')
jsonwebtoken.sign({}, b) Should you want to use a KDF to derive the secret binary buffer you're free to do so after your token consumers do the same, we cannot however do this on a package level by default since we have stay interoperable with every other JWT/S module. |
Thanks for the quick reply. I already use a similar setup in my projects ( This library is used by thousands of projects, and 95% of them just use simple string secrets, which are more than often plain alphanumeric strings, thus each key "byte" actually spans 62 values instead of 256, making JWT generated by this library very weak compared to others. Speaking of compatibility with other libraries, this is not entirely true, as I know that at least Java's jjwt and dotnet's Jose.JWT don't behave the same way. They simply don't support string secrets in the The Java lib provides helpers to derive keys from string secrets (e.g. using PBKDF2), but the functionality is decoupled from the signing part - check this message from one of the Java lib authors in one of your closed issues for more infos. I'm not saying you should change the method signatures overnight, but maybe a deprecation, or at least a documentation update, could be considered. |
We could certainly update the examples and documentation, keeping in mind it's not always the developer who's choosing the secret values, such as in cases of OAuth 2.0 client authentication assertions with a shared client_secret or an OIDC AS signing ID Tokens with HMAC based JWAs. In all these instances the one who generates the random secret uses sufficient entropy to generate a hex or base64/url string value which is then by said specifications used, e.g.
Bottom line library can do more to educate in its README but still has to accept a string for its face value as the signing key. I for one would love to see a proposal going to the appropriate IETF WG for extending the JWA alg support with HS based methods that use KDF to get their secrets rather than having each implementer "do its own thing". |
You're right, this is OIDC spec, I'm a bit surprised :/ Perhaps they judged the loss of entropy is not that problematic.
Well it doesn't have to (except for backwards compatibility), but it's a conveniance method to avoid letting developers get the UTF-8 representation of the secret themselves (using Now I understand your choice better - it's more a matter of wanting the developer to better understand what he's doing or being more "plug'n'play". |
Would you like to propose a change to the README.md file? |
Currently, when I use the following code to generate a JWT:
The actual binary key used for signing is derived from the secret using a simple UTF-8 string-to-byte.
I am surprised that the default sign method of the package uses no key derivation mechanism (like PBKDF2) to generate the signing key from given the secret. PBKDF2 does not solve the secret length issue, but it does mitigate the fact that simple UTF-8 string-to-byte is a very poor algorithm for key derivation, given that UTF-8 is not a bijection to binary data, and given that passphrases are generally plain ASCII strings, which has even less entropy than the full UTF-8 character set...
I raised an issue on the jws github project, but it could also be something to consider at the jsonwebtoken package level, e.g. change method signature and prefer buffer input rather than string secrets (buffer input is more likely to be generated from a truly random base64 key using
Buffer.from(keyAsBase64String, "base64")
).The text was updated successfully, but these errors were encountered: