-
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
Consider removal of sodium-native
optional dependency, leaving only tweetnacl
#339
Comments
+1 to get rid of sodium-native (security concerns)
|
FYI, I have incorporated both signing and verification benchmarks into the little test repo with the help of @s-a-y : https://github.com/truestamp/ed25519-bench This includes benchmarks for tweetnacl-js, sodium-native, and Node.js crypto I like the idea of swapping out However, I found a library that makes easy work of the conversion from a raw byte seed to a keypair in the form that Node crypto expects. https://github.com/ipfs-shipyard/js-crypto-key-composer This lib, which is pure JS and has no non-dev dependencies, makes it easy to swap out any use of https://github.com/truestamp/ed25519-bench#compatibility https://github.com/truestamp/ed25519-bench/blob/master/compat.js Replacing As noted, |
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
I have created a pull request for review which removes the optional dependency on |
Please have a look at https://github.com/paulmillr/noble-curves alternative to replace both |
Is your feature request related to a problem? Please describe.
sodium-native
, while significantly faster thantweetnacl
, introduces complexity in the form of:sodium-native
has not been put under a security audit to my knowledge (tweetnacl
has been formally audited by Cure53 and is FAR less complex code-wise).Describe the solution you'd like
No additional install-time or compilation time dependencies or decision making.
I've created and run a simple benchmark suite to satisfy my own curiosity as to the speed differences between these two libraries. There is no contest for simple signing operations.
sodium-native
is far faster. Here is the benchmark repo and the output:https://github.com/truestamp/ed25519-bench
TL;DR :
54,008
vs.286
operations per second. Crushed it!However, I am left wondering if there is ever any use case for using
js-stellar-base
where this difference is meaningful? Would any use case of this library ever require thousands of signing operations per second vs. hundreds of operations/second? Is 3.49ms/signature ever too slow for normal use?If so, and I am just unaware of these high performance use-cases, then by all means
sodium-native
should stay as an optional dependency. If not, then I feel it should be removed as it adds needless complexity and it introduces additional operational and security overhead and code complexity. This may fall under the category of pre-mature optimization.Describe alternatives you've considered
The alternatives are already baked into this library as primary and optional dependencies.
Additional context
cc: @abuiles
cc: @dchest
Related issue:
stellar/js-stellar-sdk#534
The text was updated successfully, but these errors were encountered: