-
Notifications
You must be signed in to change notification settings - Fork 137
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
Remove sodium-native optional dependency #495
base: master
Are you sure you want to change the base?
Conversation
This commit addresses the following issues: stellar#339 stellar#404 Changes: - removal of optional sodium-native native compiled module - promotion of existing version of tweetnacl-js to handle all sign/verify duties Removal of the optional dependency greatly simplifies the code in `src/signing.js` and removes all native compilation issues that have negatively impacted developers for at least two years when using modern versions of NodeJS. This commit does not choose to prefer a new method of signing, it simply delegates that task to the existing primary signature library (tweetnacl-js) in all cases. This also has the pleasant side-effect of greatly simplifying the signature code removing what had been described in the code comments as being "a little strange". The actual signature generate/sign/verify functions remain completely unchanged from prior code and have been refactored only to simplify the code. This also has the pleasant side effect of allowing any security audits of this code, or the associated tweetnacl-js library, to have far less surface area to examine. Cryptographic 'agility', as previously existed here to address theoretical performance issues, is considered a security anti-pattern. All existing gulp test suites pass when tested with Node version 14.18.2
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.
Thanks for taking the initiative on this; I really appreciate it. It resurfaced the pain point and I tried to investigate its impact to our organization a little more. An important factoid came up: we actually do have an important use case involving low-end hardware.
As noted in your own benchmarks, removing sodium-native would cause a significant slowdown. It would balloon signing times on the order of seconds on certain low-end hardware we support.
As such, I can't approve this PR. We need another strategy for working around sodium-native, though I'm still not sure why that workaround can't just involve, well, not installing it 😋
Can you please share with us what the exact low end hardware platforms you support that are of concern? I can find no information that indicates minimum hardware specs for this lib. This is a question that has gone unanswered for two years. Can you please also share a simple example that shows signing operations would take "seconds" to perform on that low spec platform using tweetnacl (the current default) as claimed? That would be literally thousands of times slower than observed performance on other low end platforms including Raspberry Pi. Do you support some hardware of lower spec than RPi? To answer your question about just skipping installation I think there is a lot of history in these discussions that indicate that this is not an easy viable option using the current dependency setup (e.g docker builds where there is no compilation toolchain fail, also many edge compute serverless platforms will also fail). If the removal of this optional dependency via pull request will not be approved, would an alternative build/install script for the optional sodium based installation be a possible route? However it is done, node-gyp it would seem needs to be completely removed from the normal 'npm install' path to solve this issue which is still apparently very much alive (and getting worse as node-gyp and sodium fall further behind). E.g. Would this type of approach be approved? Where you perhaps need to run a different install command if you want native compiled support? We can't currently help out to try and resolve the situation since we can't get anyone to commit to any information on supported hardware minimums or signing use cases that demand the absolute max performance. So there is no target we can hit for a solution to help improve the developer experience. |
This commit addresses the following issues:
#339
#404
Changes:
tweetnacl-js to handle all sign/verify duties
Removal of the optional dependency greatly simplifies the
code in
src/signing.js
and removes all native compilationissues that have negatively impacted developers for at least
two years when using modern versions of NodeJS.
This commit does not choose to prefer a new method of signing,
it simply delegates that task to the existing primary signature
library (tweetnacl-js) in all cases. This also has the pleasant
side-effect of greatly simplifying the signature code removing
what had been described in the code comments as
being "a little strange".
The actual signature generate/sign/verify functions remain
completely unchanged from prior code and have been refactored
only to simplify the code. This also has the pleasant side
effect of allowing any security audits of this code, or the
associated tweetnacl-js library, to have far less surface
area to examine.
Cryptographic 'agility', as previously existed here to address
theoretical performance issues, is considered a security anti-pattern.
All existing gulp test suites pass when tested with Node version 14.18.2