-
Notifications
You must be signed in to change notification settings - Fork 40
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
Drop support for old nodejs versions (req v14) #49
base: master
Are you sure you want to change the base?
Conversation
@@ -1,18 +1,23 @@ | |||
var bufferEqual = require('buffer-equal-constant-time'); | |||
var Buffer = require('safe-buffer').Buffer; | |||
var Buffer = require('buffer').Buffer; | |||
var crypto = require('crypto'); | |||
var formatEcdsa = require('ecdsa-sig-formatter'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Node crypto's own dsaEncoding
option valued ieee-p1363
can be used for ECDSA to completely remove the ecdsa-sig-formatter
dependency. This was added in nodejs/node#29292 and is available in all nodejs versions above v13.2.0 and v12.16.0.
At the same time the node crypto now offers one-shot sign and verify methods which can be used both in both a blocking and non-blocking manner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, can you show a code example of how to solve it...
maybe just do a little diff suggestion and then i can just apply the changes ;)
```diff
+
-
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would need a separate signer and verifier functions (like it is done for RSA-PSS) and instead of padding
and saltLength
options use the dsaEncoding
option. Nothing special really.
- https://github.com/nodejs/node/blob/6a489df73b0ab64f8fab016a7a71e209066fdb82/test/parallel/test-crypto-sign-verify.js#L554-L557
- https://github.com/nodejs/node/blob/6a489df73b0ab64f8fab016a7a71e209066fdb82/test/parallel/test-crypto-sign-verify.js#L516C38-L516C38
The one-shot APIs are self-explanatory and documented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sry, i'm too lost right now. don't work much with cryptografy. will leave this fully up to you guys to solve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really just a lift and shift from the old api to the new and for ecdsa having a dedicated signer/verifier with the dsaEncoding option.
Any update on this PR? |
By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.
Description
Removed some dependencies that are no longer needed.
This will require new nodejs versions.
the functionality stays the same.
References
Testing
Checklist
I have added documentation for new/changed functionality in this PR or in auth0.com/docs
All active GitHub checks for tests, formatting, and security are passing
The correct base branch is being used, if not the default branch
fix move from
buffer-equal-constant-time
totimingSafeEqual
#46fix Enhancement/ tech debt: use Node's built-in toString(base64url) #48
fix Signature is generated differently on Node v4 and Node v6 #19
fix refactor: remove return value from key check helpers #45
fix Deprecating jwa and jws packages #40
fix Update error message typo #43