Migrate code changes from deps/ncrypto back to ncrypto repo #10
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
npaun
force-pushed
the
npaun/sync-with-node-deps-ncrypto
branch
from
5609086 to
2c79847
Compare
Contributor
Author
|
I don't really get why it cannot find |
npaun
force-pushed
the
npaun/sync-with-node-deps-ncrypto
branch
from
2c79847 to
37a0995
Compare
An eventual goal for ncrypto is to completely abstract away details of working directly with openssl in order to make it easier to work with multiple different openssl/boringssl versions. As part of that we want to move away from direct reliance on specific openssl APIs in the runtime and instead go through the ncrypto abstractions. Not only does this help other runtimes trying to be compatible with Node.js, but it helps Node.js also by reducing the complexity of the crypto code in Node.js itself. PR-URL: nodejs/node#57300 Reviewed-By: Yagiz Nizipli <[email protected]> Note: Merge conflicts were resolved by Claude Code. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
* Use ncrypto APIs where appropriate * Remove obsolete no-longer used functions * Improve error handling a bit * move secure heap handling to ncrypto To simplify handling of boringssl/openssl, move secure heap impl to ncrypto. Overall the reduces the complexity of the code in crypto_util by eliminating additional ifdef branches. * simplify CryptoErrorStore::ToException a bit * simplify error handling in crypto_common * move curve utility methods to ncrypto * verify that released DataPointers aren't on secure heap The ByteSource does not currently know how to free a DataPointer allocated on the secure heap, so just verify. DataPointers on the secure heap are not something that users can allocate on their own. Their use is rare. Eventually ByteSource is going to be refactored around ncrypto APIs so these additional checks should be temporary. * simplify some ifdefs that are covered by ncrypto * cleanup some obsolete includes in crypto_util Merge conflicts were resolved by Claude Code. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
The `CipherBase` class assumes that any authentication tag will fit into `EVP_GCM_TLS_TAG_LEN` bytes, which is true because Node.js only supports GCM with AES as the blocker cipher, and the block size of AES happens to be 16 bytes, which coincidentally is also the output size of the Poly1305 construction used by ChaCha20-Poly1305 as well as the maximum size of authentication tags produced by AES in CCM or OCB mode. This commit adds a new constant `ncrypto::Cipher::MAX_AUTH_TAG_LENGTH` which is the maximum length of authentication tags produced by algorithms that Node.js supports and replaces some constants in `CipherBase` with semantically more meaningful named constants. The OpenSSL team is debating whether a constant like `MAX_AUTH_TAG_LENGTH` (`EVP_MAX_AEAD_TAG_LENGTH`) should exist at all since its value necessarily depends on the set of AEAD algorithms supported, but I do believe that, for Node.js, this is a step in the right direction. It certainly makes more sense than to use the AES-GCM tag size as defined by TLS. PR-URL: nodejs/node#57803 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
An `std::string_view v` is a `const char* v.data()` along with an `std::size_t v.size()` that guarantees that `v.size()` contiguous elements of type `char` can be accessed relative to the pointer `v.data()`. One of the main reasons behind the existence of `std::string_view` is the ability to operate on `char` sequences without requiring null termination, which otherwise often requires expensive copies of strings to be made. As a consequence, it is generally incorrect to assume that `v.data()` points to a null-terminated sequence of `char`, and the only way to obtain a null-terminated string from an `std::string_view` is to make a copy. It is not even possible to check if the sequence pointed to by `v.data()` is null-terminated because the null character would be at position `v.data() + v.size()`, which is outside of the range that `v` guarantees safe access to. (A default-constructed `std::string_view` even sets its own data pointer to a `nullptr`, which is fine because it only needs to guarantee safe access to zero elements, i.e., to no elements). In `deps/ncrypto` and `src/crypto`, there are various APIs that consume `std::string_view v` arguments but then ignore `v.size()` and treat `v.data()` as a C-style string of type `const char*`. However, that is not what call sites would expect from functions that explicitly ask for `std::string_view` arguments, since it makes assumptions beyond the guarantees provided by `std::string_view` and leads to undefined behavior unless the given view either contains an embedded null character or the `char` at address `v.data() + v.size()` is a null character. This is not a reasonable assumption for `std::string_view` in general, and it also defeats the purpose of `std::string_view` for the most part since, when `v.size()` is being ignored, it is essentially just a `const char*`. Constructing an `std::string_view` from a `const char*` is also not "free" but requires computing the length of the C-style string (unless the length can be computed at compile time, e.g., because the value is just a string literal). Repeated conversion between `const char*` as used by OpenSSL and `std::string_view` as used by ncrypto thus incurs the additional overhead of computing the length of the string whenever an `std::string_view` is constructed from a `const char*`. (This seems negligible compared to the safety argument though.) Similarly, returning a `const char*` pointer to a C-style string as an `std::string_view` has two downsides: the function must compute the length of the string in order to construct the view, and the caller can no longer assume that the return value is null-terminated and thus cannot pass the returned view to functions that require their arguments to be null terminated. (And, for the reasons explained above, the caller also cannot check if the value is null-terminated without potentially invoking undefined behavior.) C++20 unfortunately does not have a type similar to Rust's `CStr` or GSL `czstring`. Therefore, this commit changes many occurrences of `std::string_view` back to `const char*`, which is conventional for null-terminated C-style strings and does not require computing the length of strings. There are _a lot_ of occurrences of `std::string_view` in ncrypto and for each one, we need to evaluate if it is safe and a good abstraction. I tried to do so, but I might have changed too few or too many, so please feel free to give feedback on individual occurrences. Merge conflicts were resolved by Claude Code. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
PR-URL: nodejs/node#58103 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
Support `outputLength` option in crypto.hash() for XOF hash functions to align with the behaviour of crypto.createHash() API closes: nodejs/node#57312 Co-authored-by: Filip Skokan <[email protected]> PR-URL: nodejs/node#58121 Fixes: nodejs/node#57312 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs/node#59259 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#59436 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Richard Lau <[email protected]>
PR-URL: nodejs/node#59461 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]>
Commit 206ebeb44764d58c6a505657edab3a7a78a0b977 added an additional call to EVP_PKEY_public_check and an unconditional return from publicCheck(). This prevents the control flow from reaching the original call to either EVP_PKEY_public_check or EVP_PKEY_public_check_quick. This change restores the previous behavior, which calls EVP_PKEY_public_check_quick instead, if possible. Refs: nodejs/node#56812 PR-URL: nodejs/node#59471 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs/node#59365 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
Co-authored-by: Filip Skokan <[email protected]> Co-authored-by: James M Snell <[email protected]> PR-URL: nodejs/node#50353 Reviewed-By: Ethan Arrowood <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Yagiz Nizipli <[email protected]>
PR-URL: nodejs/node#59491 Reviewed-By: Yagiz Nizipli <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]>
PR-URL: nodejs/node#59569 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#59539 Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#59537 Reviewed-By: Tobias Nießen <[email protected]>
PR-URL: nodejs/node#59647 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#59570 Reviewed-By: James M Snell <[email protected]>
Adds the `signatureAlgorithm` property to a X509Certificate allowing users to retrieve a string representing the algorithm used to sign the certificate. This string is defined by the OpenSSL library. Fixes: nodejs/node#59103 PR-URL: nodejs/node#59235 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Filip Skokan <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>
npaun
force-pushed
the
npaun/sync-with-node-deps-ncrypto
branch
from
37a0995 to
8492c04
Compare
Contributor
Author
|
boringssl stuffs it in |
Member
|
Can we make sure we have couple of tests added before merging this? |
anonrig
approved these changes
Sep 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Over time, changes have been made in Node's
deps/ncryptofolder that are not reflected in this copy of the project. We need to bring the two duelling ncryptos back in sync so that future changes need only be made once.Most changes just apply cleanly with
git ambut three had a lot of merge conflicts. I decided to ask Claude Code to resolve them and I think it did a pretty decent job, but please check these commits especially:A couple commits from me:
libdecreptwhen using boringssl, because this is where they putEVP_CIPHER_do_all_sorted.