Open electic opened 3 years ago
gre
commented
3 years ago This is indeed a problem that we also felt on our side (Ledger Live) on our path of migrating from Node 12 to Node 14. Is there any progress done on the matter? Is there a possible workaround to disable sodium-native globally? (it seems yarn only allow to globally disable optional, but you can't target a specific one)
gre
commented
3 years ago Please consider upgrading or changing sodium to something else. 🙏
Shaptic
commented
3 years ago This will be a long reply but I think worthwhile both for current and future context, particularly for myself as a newer maintainer.
TL;DR: There are no suitable, secure alternatives to the library from my investigation. However, try the upgrade-nacl branch to see if your build errors are resolved. It bumps sodium-native to 3.2.1 and node-gyp-build to 4.3.0, which is its latest version.
There have been discussions and changesets before on the crypto dependencies:
Historically, it appears we went from: ed25519 to [sodium-native]() after considering elliptic and crypto, opting for the former for a number of reasons.
sodium-nativeIt's first worth driving home the fact that, as the README states, this is an optional dependency used for performance, so nobody should be blocked by not addressing this request. That should address this, from above:
Is there a possible workaround to disable sodium-native globally?
Obviously, performance is important, so we'd prefer a replacement that is performant and also alleviates people's build issues.
@grempe and @s-a-y graciously made some benchmarks comparing sodium-native to other libraries. It still outperforms the alternatives significantly (by ~37%), but some wondered whether or not this was worth its complexity. Obviously, it's hard to get a complete picture on how this library is used to answer this question, but any replacement should nonetheless keep this metric in mind.
I can't speak for the original authors, but I surmise that sodium-native was chosen because its underlying library, libsodium, and its sibling NaCl has been thoroughly audited (e.g. [1], among others), which is huge. The same goes for its backup library, tweetnacl. Any replacement would ideally provide the same security guarantees.
Relying on NodeJS's native crypto module necessitates introducing unreliable dependencies (e.g. crypto-key-composer) in order to get 32-byte arrays (what we use in the SDK today, which is very ergonomic and user-friendly) in the pem format that crypto expects (see @grempe's comment for details). That aside, crypto itself hasn't been audited.
Relying on a different third-party library like noble-ed25519 introduces reliance on an un-audited implementation. Its author claims it should be "easily auditable", and its used by some wallets, but their blog post about the library doesn't instill high confidence.
The lack of audits for replacement libraries makes me hesitate the most.
Without reaching for a replacement, we could see if bumping the version resolves people's issues. I have a hard time determining whether or not yarn upgrade sodium-native will do this because I can't reproduce the build issues locally. However, you can point your library to depend on the upgrade-nacl branch of stellar-base to see if it does and get back to me.
grempe
commented
3 years ago These are good comments @Shaptic. I would make note of a few things:
By all indications these bindings for libsodium are unmaintained at this point. The latest release version is v1.2.0 which was released on 1/24/2017, nearly five years ago. There have been a number of pre-release releases since, but the last one of those is v3.2.1 and that was nearly a year ago. This does not bode well for the long term health of the codebase to depend on this IMHO. The version you bumped to (2.4.9) is nearly 2 years old and not even the latest pre-release version (3.2.1). Is there a reason for that choice? Even the lastest commits in the repo only bring in libsodium @ 4f5e89f which is two years old.
https://www.npmjs.com/package/sodium-native
This wrapper is unaudited to my knowlege. I struggle with the idea of this being considered the most secure alternative available.
You are putting a lot of weight on the audit of libsodium. However that audit was done five years ago on a version of the software that is far out of date to what is actually being used today. Security audits only apply to the version tested and grow stale.
https://github.com/jedisct1/libsodium/blame/master/ChangeLog
Using this necessitates accepting the maintenance/security risks of the sodium-native lib.
It seems that this lib is meeting all the needs and is currently in use. The only issue I'm aware of that stops it from being the only choice is performance. Its Cure53 audits, like the others, are nearly five years old and yet it is still acceptable. Keep in mind that despite the audits, security issues have still been found and fixed since.
https://github.com/dchest/tweetnacl-js/releases
I can tell you from personal experience that security audits are not a magic protective shield. If the Stellar org is making choices based on audits, then perhaps it should sponsor a re-audit of tweetnacl or noble-crypto if they meet the speed/security goals. Due to the small size, and use of Typescript in both of those libs an audit of one or both is a much smaller task than tackling the same on thousands of lines of C/Assembly/JS code & tooling in the sodium duo. Native bindings in libs are not the modern path. They bring so much pain.
I'm not sure why this is characterized as an "unreliable dependency". If you don't want to use it that's fine. But I'm not sure that applying bias without having examined or tested it is called for. Its just a set of key utility functions to improve ergonomics. I think its only potentially needed if the choice to use modern Node APIs is made (for purely performance reasons and as a replacement for sodium wrappers). I found it useful, but maybe smarter minds will find a better way to handle the key impedance mismatch.
https://github.com/truestamp/ed25519-bench/blob/master/compat.js#L40
It seems to me that the most important question to address is regarding the performance requirements for this js-stellar-base lib. Who's using it, and in what context, and what are their performance needs that sacrifice security complexity for speed? There is currently a trade-off being made favoring performance over runtime and developer complexity. Clearly this issue is causing problems for developers using modern runtimes. I think it could also be argued that accepting the massive security surface exposed by using sodium-native + libsodium for functionality that could be entirely replaced by the already in usetweetnacl is perhaps not worth the cost.
I would also suggest that incorporating two libs (primary and fallback) in itself adds to the security costs of that choice.
A lot of weight is being put on stale security audits at the expense of complexity (risk). Although 'unaudited', there are orders of magnitude more eyes on what Node crypto is doing than anything else comparable in the JavaScript space. Again, it would seem to me that node only needs to be considered if the performance gains outweigh the complexity compared to a pure JS solution. Fastest isn't always best.
I hope this contributes to the conversation and drives discussion of some of the tradeoffs. ✌🏻
Shaptic
commented
3 years ago Thank you for taking the time to write such a thorough reply! I'll try to respond to all of your points in turn.
sodium-nativeBy all indications these bindings for libsodium are unmaintained at this point. I struggle with the idea of this being considered the most secure alternative available.
For the former point, I will admit I didn't note the last release date. For the latter, I can't speak for the original authors: I honestly just made some inferences based on my own cursory investigation.
However, some clarification is needed. libsodium's last release was in 2019, but even though 2 years seems like a long time to us in the JS world, I'd actually expect releases of a crypto library to come infrequently as that suggests maturity and stability. In that same vein, with no changes to the underlying library, the latest version of the bindings being ~9 months ago also makes sense.
Is there a reason for that choice?
Hah! I blame yarn (or, rather, my misuse of yarn). Despite being marked as a pre-release on GitHub (as are all of the other versions, it seems like...), it's not tagged in a special way on npm, nor is 2.4.10 (the actual latest 2.x version) even on npm... Anyway, I've bumped the branch to 3.2.1.
This wrapper is unaudited to my knowlege.
True and valid, but I cut it some slack for just being a binding layer.
libsodiumSecurity audits only apply to the version tested and grow stale.
This is true, but shouldn't we give more credence to a popular, stable library that was known to be secure $TIME ago than one with no such claims? Though I believe you already make a counterpoint later, with
Although 'unaudited', there are orders of magnitude more eyes on what Node crypto is doing than anything else comparable in the JavaScript space.
Comparable to the JS space, but can the same be said for the crypto space at large? Nonetheless, you have a point; we can definitely have a reasonable degree of certainty in Node's library. I will say that as someone more familiar with cryptography from a theoretical standpoint than its various implementations across languages, the lack of frequent audits in the space is concerning (despite them not being a silver bullet, as you said).
tweetnaclAs I mentioned, part of my investigation was actually looking at who is using these alternative libraries. Deciphering that isn't trivial, but I'd love to align our dependencies with whatever another big player using ed25519 is doing. I know zcash uses it, but it'll take some more digging to see what others are doing in the JS space.
Due to the small size, and use of Typescript in both of those libs an audit of one or both is a much smaller task than tackling the same on thousands of lines of C/Assembly/JS code & tooling in the sodium duo.
Excellent point!
They bring so much pain.
This is obviously true as evidenced by this thread, but we need to make sure relieving painful builds doesn't cause painful security vulnerabilities.
crypto-key-composerI'm not sure why this is characterized as an "unreliable dependency".
I didn't mean to come off as biased or inflammatory with this, and was referring to introduction of the dependency itself as being unreliable rather than what's in it, if that makes sense.
I called it that because it would be responsible for a piece of functionality that needs to be very safe, i.e. converting between private key formats. A slip-up here (and we all know how vulnerable npm et al. is to those) would be catastrophic, hence my hesistation. I agree that its scope is very small (as evidenced by the code you linked), but its impact could be large.
I do think (and I wrote this in my initial comment but redacted it after seeing that crypto had never been audited) that (1) seems like the best way forward if we were to go for a language-native replacement.
We could also alleviate my dependency concerns by inlining the relevant code.
It seems to me that the most important question to address is regarding the performance requirements for this js-stellar-base lib.
Heavily agreed with this, and this snippet from your older comment:
Is 3.49ms/signature ever too slow for normal use?
I unfortunately don't know the answer to these questions. The discussion here is naturally biased towards its participants (as is the TBD conclusion). It's hard to know what the impact of a change will be, and I'm sure there will always be that one user.
To be abundantly clear, I appreciate the discussion happening here and of course want to make the SDK both accessible and performant. "Don't roll your own crypto" is an adage hammered home again and again in the security community, so I'm just trying to be very careful before introducing changes to any crypto-related code. I think (1) from my earlier comment (using NodeJS's crypto) may be a reasonable path forward.
mahnunchik
commented
3 years ago Please update sodium-native to v3
Shaptic
commented
3 years ago @mahnunchik give it a go in the upgrade-nacl feature branch (see ce3ff0b).
alejomendoza
commented
2 years ago Deployments fail due to this issue, stellar SDK becomes very painful to use. Is there a way we can fix this on the main package?
grempe
commented
2 years ago They've been sitting on this pain point for nearly two years @alejomendoza. See conversation above and here:
grempe
commented
2 years ago I have created a pull request for review which removes the optional dependency on sodium-native (leaving tweetnacl-js in place to handle all sign/verify functions just as it previously did). I would appreciate any comments to see if we can have this merged.
See : https://github.com/stellar/js-stellar-base/pull/495
I have also filed an issue to request a bump of tweetnacl-js to version 1.0.3 to address a potential security issue in older versions.
Shaptic
commented
2 years ago Deployments fail due to this issue, stellar SDK becomes very painful to use. Is there a way we can fix this on the main package?
The library is an optional dependency; you can install this package without it:
npm install --no-optional stellar-sdk
# or
yarn install --ignore-optional stellar-sdkIf this will work as a solution (I have not yet tested in e.g. a Docker container with no toolchain installed) it needs to be documented at the top level readme's of both projects and not buried in this thread.
I'll try to test further tomorrow.
sinanouri
commented
1 year ago Deployments fail due to this issue, stellar SDK becomes very painful to use. Is there a way we can fix this on the main package?
The library is an optional dependency; you can install this package without it:
npm install --no-optional stellar-sdk # or yarn install --ignore-optional stellar-sdk
tested with docker container
FROM node:20-alpine3.18
WORKDIR /app
COPY package.json .
RUN npm --no-optional install
COPY . .
CMD ["node","app.js"]Steallr-sdk started working
https://github.com/stellar/js-stellar-base/blob/c687cf941133814a40d3e49a13794900453bfed8/package.json#L115
Sodium-native's version is causing issues because the node-gyp version is old. That means it does not have support for VS 2019 causing compiler compiler errors because the enum value is not in node-gyp for python look ups. Can we bump the version?
From the code, it seems it is only used for singing. Do we need this complexity?