feat: node crypto module

[L-jasmine]

please sync #95

[juntao]

flows summarize

[juntao]

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.

---

This extensive patch covers various components within the crypto modules and related files. It adds new tests, modifies several crypto class implementations, and addresses potential issues related to the update, final, and transform methods. The patch primarily focuses on introducing new features and functionalities, such as cryptography algorithms, FIPS support, PBKDF2, HKDF, scrypt hashing, and cipher/decipher support.

Several potential problems across these patches include:

1. Large numbers of new test files might make the project harder to maintain and increase test run time.
2. Various imports and export modifications in the files, causing errors or unexpected behavior.
3. Multiple instances of commented or uncommented code that may cause confusion or unexpected behavior.
4. Missing tests and validations for new functionalities in some cases, along with unclear reasons for ignoring tests.
5. The patch only considers subsets of algorithms supported by the Web Crypto API, and other algorithms may not work well with this setup.

The most important findings include:

1. Implementations of various cryptographic algorithms, such as AES, RSA, DH, DSA, ECDSA, and more, with new test cases coverage.
2. FIPS handling implementation, allowing FIPS-compliant crypto operations.
3. Implementations of PBKDF2, HKDF, and scrypt hashing algorithms to enhance the crypto module.
4. Support for cipher/decipher operations with the introduction of cipheriv and decipheriv functionality.

Recommendations on mitigating potential issues include reviewing test files individually, keeping track of dependencies, separating tests into smaller groups, and implementing additional test cases.

Details

Commit 1

The given GitHub patch contains a large number of new test cases related to cryptography functionality in the codebase. The patch adds 62 new files with 12,504 insertions to the project, primarily focused on testing various cryptographic algorithms and features, such as AES, DH, ECDH, ECDSA, RSA, and more.

Key changes:

1. Adds tests for various cryptographic algorithms, such as AES, RSA, DH, DSA, ECDSA, and more.
2. Includes tests for multiple supported padding schemes, signature encodings, and salt lengths.
3. Adds a new dependency wasi-crypto-guest to the Cargo.toml file.

Potential problems:

1. A large number of new test files might make the project more difficult to maintain and might slow down the test run time.
2. Without perusing each test file individually, it's difficult to ascertain if there is any duplication, incorrect test cases, or missed edge cases in the tests.
3. The added dependency wasi-crypto-guest might introduce the risk of breaking changes or security vulnerabilities in the future.

It is recommended to:

1. Review the test files individually to make sure there is no test case duplication, all edge cases are covered, and the tests are written in a maintainable way.
2. Keep an eye on the added dependency wasi-crypto-guest to track any possible breaking changes or security vulnerabilities.
3. Consider separating tests into smaller groups if necessary, to keep them maintainable and keep the test run time reasonable.

Commit 2

This patch adds a new implementation for the FIPS (Federal Information Processing Standards) in the crypto module. The following changes were made:

• Created a new file modules/crypto.js with implementation for FIPS handling and various commented-out imports.
• Added a new error class named ERR_CRYPTO_FIPS_FORCED in modules/internal/errors.js.
• Created a new file src/internal_module/crypto.rs to handle crypto in Rust, but with empty implementations.
• Included the newly created crypto module in src/internal_module/mod.rs.
• Created a new file tests/test-crypto.rs with various tests for crypto functionality.

Key changes:

1. Implementation for FIPS handling is added in the modules/crypto.js file.
2. An error class for FIPS is created in modules/internal/errors.js.

Potential problems:

1. Various imports in the modules/crypto.js file are commented out, which might cause errors or unexpected behavior when using the crypto functionality.
2. The src/internal_module/crypto.rs file contains empty implementations, which might lead to unexpected behavior when using the crypto functionality in Rust.

Some of the tests have been marked with ignore for certain reasons, such as "unsupported", "working", "unsupported, domain", "unsupported, worker thread", etc. These ignored tests may need to be reviewed and fixed before merging the patch.

Commit 3

The patch contains changes to the crypto module, introducing new features and fixing existing issues. Here is the summary of key changes:

1. Added new files:

   • modules/internal/crypto/random.js: New random functions implementation.
   • test/crypto/test-crypto-timing-safe-equal.js: New test for timing-safe-equal functions.

2. Modifications in the following files:

   • modules/assert.js: Added "assert.getErrMessage unsupported" return statement in the getErrMessage function.
   • modules/buffer.js: Introduced FastBuffer class and added it to the exports alongside existing exports.
   • modules/crypto.js: Modified imports, added timingSafeEqual function, and updated exports.
   • modules/internal/errors.js: Included new error codes ERR_CRYPTO_TIMING_SAFE_EQUAL_LENGTH and ERR_INVALID_ARG_TYPE.

3. Other minor changes and fixes in several files, including tests.

Potential problems:

1. In modules/crypto.js, some of the imports, functions, and exports are commented out. If this is unintentional, it could lead to issues in the module's functionality.

2. In modules/internal/crypto/random.js, the generatePrime and generatePrimeSync functions throw an "unimplemented" error. This needs proper implementation to be functional.

3. Some functions might not have been thoroughly tested due to the changes in the code. It's essential to check for any possible issues in edge cases or with different inputs.

Commit 4

Summary of key changes:

1. In random.js, the import process statement is added.
2. In the randomFillSync and randomFill functions, buf.buffer ?? buf is used instead of buf.buffer.
3. In test-crypto.rs, various test attributes and ignore statuses are modified.

Potential problems:

1. The newly added import process statement in random.js is not used in the code, leading to an unused import.
2. The change from buf.buffer to buf.buffer ?? buf in randomFillSync and randomFill might cause unexpected behavior if buf is not an ArrayBuffer-like object or if its structure is not compatible with the expected input.
3. In test-crypto.rs, a few tests had their #[ignore = "working"] attributes removed, causing them not to be ignored anymore. If these tests are not stable, this change could lead to instability in the test suite.
4. The change of ignore reasons in test-crypto.rs could cause confusion about the reasons for ignoring those tests, impacting the clarity of the code.

Commit 5

This patch introduces a new feature, the implementation of PBKDF2 functionality, as well as several other miscellaneous updates. Key changes include:

1. Five new files were created: hashnames.js, pbkdf2.js, random.js, util.js, and crypto.rs. These files work together to implement PBKDF2 functionality.
2. In the existing crypto.js file, several import lines and export default lines were uncommented to include the new PBKDF2 functionality.
3. The hashnames.js file implements a utility for mapping between WebCrypto standard SHA-* digest algorithm names, which is useful to handle different naming conventions in different contexts.
4. The pbkdf2.js file implements the main PBKDF2 functionality, including the functions pbkdf2(), pbkdf2Sync(), and pbkdf2DeriveBits().
5. The random.js file contains an updated version of the lazyDOMException() function.

Please note that there might be potential issues:

1. There are no tests included in the patch, so it's unknown whether the functionality works as expected.
2. The patch only considers a subset of algorithms supported by the Web Crypto API. Other algorithms might not work well with this setup.
3. Code readability can be improved, as there are long inline comments starting at line 82 in hashnames.js and line 87 in random.js.

Overall, the patch provides support for PBKDF2 functionality in the crypto module.

Commit 6

This patch primarily focuses on the PBKDF2 functionality in the crypto module.

Key changes:

1. Updated pbkdf2 function in pbkdf2.js to use pbkdf2_sync instead of the previous PBKDF2Job.
2. Added an ERR_CRYPTO_INVALID_DIGEST error type to errors.js.
3. Commented out some crypto bindings in modules/internal/crypto/util.js.
4. Added the filterDuplicateStrings and cachedResult functions to modules/internal/util.js.
5. Exported the validateArray function in modules/internal/validators.js.
6. Added the pbkdf2_sync function in src/internal_module/crypto.rs.
7. Modified the test file test/crypto/test-crypto-pbkdf2.js to use the new crypto module.

Potential problems:

1. The newly added pbkdf2_sync function may not be fully compatible with the previous PBKDF2Job implementation.
2. Several crypto bindings have been commented out in the modules/internal/crypto/util.js file, which might cause issues with other parts of the crypto module that rely on these bindings.
3. The error handling in the new pbkdf2_sync function may not be sufficient, as it only returns JsValue::UnDefined in case of an error.
4. The updated PBKDF2 test file might have compatibility issues if it relies on the older PBKDF2Job implementation.

Commit 7

This patch introduces the implementation of the scrypt hashing algorithm. The following are the key changes:

1. Adds a new file scrypt.js to the modules/internal/crypto/ directory, which includes the implementation of scrypt and scryptSync functions.

2. Modifies the errors.js file under the modules/internal/ directory to add two new error classes, ERR_CRYPTO_SCRYPT_INVALID_PARAMETER and ERR_CRYPTO_SCRYPT_NOT_SUPPORTED.

3. Adds a new ScryptRom struct and associated methods in the crypto.rs file under the src/internal_module/ directory.

Potential problems:

1. Missing or incorrect validation for input parameters in the scrypt and scryptSync functions could lead to unexpected behavior.

2. In the implementation of the scrypt function in the crypto.rs file, there should be error handling to ensure that the various steps (pbkdf2 and scrypt_rom) are executed successfully before generating the final hash.

Commit 8

Summary of key changes:

1. The scrypt and scryptSync functions have been modified to use the new scrypt_sync function.
2. The setTimeout function has been added to ensure stack fairness with asynchronous scrypt function.
3. The input parameters validation errors handling for the scrypt function have been added.
4. The scrypt_sync function has been added to the Rust code.
5. The unsafely defined R function was made safe with the help of macro_rules.
6. Now, the invalid random_fill behavior would be handled.
7. The scrypt_sync handling was added for test case in the file.

Potential problems:

1. It seems that there is a typo in the import statement import assert from'assert';. There should be a space between from and 'assert'. It should be import assert from 'assert';.
2. In modules/crypto.js, comment block boundaries (/ ... /) are misplaced, which can cause issues with the code. You should check and correct the comment block boundaries.
3. Double-check if the removal of common.skip when ScryptJob is not available is alright or if it will cause any test case issues.

Commit 9

This patch implements internal support for the HKDF (HMAC-based Key Derivation Function) algorithm. The developer has added new files for the hkdf implementation and the key object exporting/importing system.

Key changes:

1. Imported and exported new functionality from different files like util, validators, crypto, and errors.
2. Added new file hkdf.js to implement the HKDF logic, including validation of input hash, key, salt, info, and length.
3. Created a new file keys.js which contains the key object classes SecretKeyObject, PublicKeyObject, and PrivateKeyObject.
4. Modified existing file, crypto.js, to import new functionality.

Here are some potential issues:

1. The diff contains extra, commented-out lines with create mode and permissions which might not be required.
2. Some of the commenting out and importing may not be correct or required, such as in the crypto.js file.
3. The patch does not include any new tests for the implemented logic or modified code, so it might be a good idea to implement unit tests to ensure the proper functioning of this patch.

Commit 10

This patch includes a set of changes in 11 files with a total of 4488 insertions and 435 deletions. The main purpose of this patch is to remove the dependency on the wasi-crypto-guest library from the project.

Key changes:

1. Removal of dependency: The wasi-crypto-guest dependency is removed from Cargo.toml.
2. Modifications in the crypto code: Changes are made in the internal/crypto/hkdf.js and internal/crypto/keys.js files related to the handling of cryptographic functions.
3. Addition of new files: Four new files src/internal_module/crypto/lib.rs, src/internal_module/crypto/mod.rs, src/internal_module/crypto/raw.rs, and src/internal_module/crypto.rs are added to implement the handling of cryptography in Rust.
4. Test adjustments: Changes are made in test/crypto/test-crypto-hkdf.js to adjust the crypto tests according to the new implementation.

Potential issues:

1. Performance differences: Since the wasi-crypto-guest library has been removed, the performance might be different from the previous implementation as the new implementation is written in Rust.
2. Code maintainability: With the new implementation in Rust, the maintainability of the code might be slightly affected if the previous implementation was more suited to the project.

Overall, this patch removes an external dependency and re-implements cryptographic functions within the project. However, potential concerns should be addressed, such as the difference in performance and maintainability of the new implementation.

Commit 11

This GitHub patch introduces changes and additions to the internal crypto hashing functionality.

Key changes:

1. Added a new hash.js file, which exports the Hash, Hmac, and asyncDigest classes.
2. Created a new lazy_transform.js file for the LazyTransform class that is lazily loaded.
3. Updated lib.rs to implement the Hmac and Hash structs and related functions.
4. Minor changes to the test-crypto.rs file.

Potential problems:

1. The hash.js file could have potential problems such as memory leaks, race conditions, or incorrect error handling.
2. The added LazyTransform functionality might create issues related to performance or compatibility with other related code.
3. Possible issues regarding the use of unsafe Rust code and error handling in lib.rs.
4. The ignored test in test-crypto.rs might need further investigation.

Overall, the patch adds new functionality for hashing and stream transformations, which might have some potential issues in error handling, performance, or compatibility.

Commit 12

The patch makes changes to the files related to the crypto module:

1. It modifies the .github/workflows/examples.yml file to include Wasi-crypto while installing WasmEdge. This will make sure the Wasi-crypto plugin is installed and available for these examples' CI pipeline.

2. It updates the createHash and createHmac functions in modules/crypto.js by importing them for Node.js crypto module. Moreover, it also updates their method implementations according to the Node.js environment.

3. It modifies the Hash and Hmac classes in modules/internal/crypto/hash.js, with updates like checking if the given algorithm is supported or not before initializing the objects.

4. It adds new error codes in modules/internal/errors.js related to hash update failures and finalized hash.

5. It updates the LazyTransform module in modules/internal/streams/lazy_transform.js, replacing stream.Transform with Transform.

6. It adds validateEncoding function in modules/internal/validators.js, which is used to check the validity of a given encoding according to the data length.

7. It introduces new Cipher class in src/internal_module/crypto/lib.rs to support encryption functionalities.

Key Findings:

• It modifies and improves the crypto module's functionalities and compatibility with the crypto module of Node.js.
• It adds new errors for better error handling and validation.

Potential Problems:

• Some commented lines and uncommented lines with '/' or '/' may cause confusion or errors in the future. It is advised to remove such lines.
• Although it implements the Cipher class in lib.rs, it isn't utilized in the rest of the codebase.

Commit 13

This patch implements cipheriv and decipheriv for the crypto module. Key changes include:

1. Adding the use of public and private encryption/decryption methods (publicEncrypt, publicDecrypt, privateEncrypt, and privateDecrypt) and a getCipherInfo function.
2. Creating new cipher and decipher classes (Cipher, Cipheriv, Decipher, Decipheriv) with methods such as init, update, final, setAutoPadding, getAuthTag, setAuthTag, and setAAD.
3. Updating the version of WasmEdge being used in the workflows file.
4. Various refactoring and error handling enhancements.

Potential problems:

1. Mix of commented and uncommented code in modules/crypto.js might lead to confusion and unexpected behavior.
2. The use of the ERR_CRYPTO_INVALID_STATE, ERR_INVALID_ARG_TYPE, and ERR_INVALID_ARG_VALUE error classes have not been defined in the provided code. This may cause issues if the corresponding error classes are not imported or defined elsewhere.
3. The Cipher, Decipher, Cipheriv, and Decipheriv classes utilize a mix of inheritance using Object.setPrototypeOf and function-based inheritance. This may lead to confusion and potential issues with class instances if not done correctly.
4. The publicEncrypt, publicDecrypt, privateEncrypt, privateDecrypt, and _getCipherInfo functions are defined with empty bodies at the beginning of the cipher.js file, with the actual implementation missing. This may lead to errors or incomplete functionality.

It is important to ensure the error handling and functionality work as expected, especially with the mix of inheritance and function-based classes. Given the empty function bodies for key encryption/decryption methods, it is necessary to verify the correct behavior of the code prior to merging this pull request.

Commit 14

This patch includes changes related to the crypto-cipheriv-decipheriv test module. Key changes include:

1. Modification in the .github/workflows/examples.yml file, the line is changed to use sudo for moving the libwasmedgePluginWasiCrypto.so file.
2. Updates in the modules/internal/crypto/cipher.js file:
   • Importing JsCipher from the _node:crypto module.
   • Adding a check for supported ciphers using getCiphers() and throwing ERR_CRYPTO_UNKNOWN_CIPHER if the cipher is not supported.
   • Modification in the createCipherWithIV function to handle GCM mode and check for zero-sized IVs.
   • Changing _transform, update, and final methods in the Cipher class to provide the correct input and output values.
3. Adding ERR_CRYPTO_INVALID_STATE and ERR_CRYPTO_UNKNOWN_CIPHER classes to the modules/internal/errors.js file.
4. Modifications in the test/crypto/test-crypto-cipheriv-decipheriv.js file:
   • Updating test cases with new key and IV sizes.
   • Adding a new test function testCipher4 that uses 'aes-128-gcm' instead of the previously hardcoded cipher name.
   • Adjusting tests to work with the updated createCipherWithIV function.

Potential issues that may arise:

1. It is essential to ensure that the getCiphers() function correctly filters supported ciphers to avoid potential problems in the application.
2. The output of the update method has changed, so it returns an empty string by default. This could potentially affect the current implementation in other modules relying on this method.
3. Test cases cover only 'aes-128-gcm' cipher. It could be necessary to include more test cases considering an extensive range of supported ciphers to ensure the correct functioning of the code.

Commit 15

This patch updates the GitHub Actions workflow file (examples.yml) to use the Ubuntu 20.04 version of the WasmEdge-plugin-wasi_crypto plugin instead of the previous manylinux2014 version.

Key changes:

1. The URL for downloading the WasmEdge-plugin-wasi_crypto plugin has been updated to fetch the Ubuntu 20.04 version instead of the manylinux2014 version.
2. The file name used for the extracted plugin has been updated to reflect the switch to the Ubuntu 20.04 version.

Potential problems or concerns:

1. Ensure that the Ubuntu 20.04 version of the plugin is compatible with the current environment and dependencies.

Commit 16

Summary of key changes:

• In the .github/workflows/examples.yml file, the long command to install WasmEdge has been modified, specifically removing the --image-deps-version=$VERSION argument.

Potential problems:

• By removing --image-deps-version=$VERSION, this patch assumes that the default version of the image dependencies will be compatible with the specified VERSION. If the versions are not compatible, this can lead to issues in the workflow, such as incomplete or incorrect installation.

Commit 17

This patch adds a new feature to enable the Node.js crypto module for the project. It introduces changes in 4 files:

1. .github/workflows/examples.yml: The patch replaces the commented-out "Node fs module test" section with a new "Node crypto module test" section. The test runs cargo test test_crypto with the release configuration and the nodejs_crypto feature enabled.

2. Cargo.toml: A new feature named nodejs_crypto has been added.

3. src/internal_module/mod.rs: The crypto module is now only imported when the nodejs_crypto feature is enabled using the #[cfg(feature = "nodejs_crypto")] attribute.

4. src/quickjs_sys/mod.rs: The init_module function for the crypto module is now only called when the nodejs_crypto feature is enabled (also using the #[cfg(feature = "nodejs_crypto")] attribute).

Possible issues:

1. There is no listed timeout for the "Node crypto module test" in the .github/workflows/examples.yml file. Depending on the duration of the test suite, it could stall the CI pipeline if it takes too long.

2. As the patch modifies the .github/workflows/examples.yml file, it's important to ensure that the changes are not breaking the GitHub Actions workflow execution.

3. If there is any code depending on the crypto module elsewhere in the project that hasn't been updated, it could potentially cause compilation errors when the nodejs_crypto feature is not enabled.

Other than these potential issues, the patch seems straightforward and enables a new feature to include the crypto module in the build process.

Commit 18

Summary of Key Changes:

1. Added new modules for crypto library with multiple functionalities:

   • certificate.js
   • diffiehellman.js
   • keygen.js
   • sig.js
   • x509.js

2. Modified Cargo.lock and src/internal_module/crypto/lib.rs

3. Added numerous new test fixtures for keys along with Makefile and .gitattributes file.

Potential Problems:

It is difficult to identify any potential problems without looking at the actual implementation of the new modules. However, here are some points of consideration:

1. Ensure that the added crypto modules have appropriate error handling and follow coding standards.

2. Verify that the added test fixtures and keys cover the required functionalities and edge cases.

3. Check if any dependencies in Cargo.lock are updated or changed, ensure that they are compatible with the existing codebase.

4. Review the changes made in src/internal_module/crypto/lib.rs to ensure that it is correctly utilizing the added modules.

Commit 19

Summary: The patch removes the finalizers from the implementation of three structs: JsHash, JsHmac, and JsCipher. These finalizers were calling std::mem::drop(data) function, which is unnecessary because Rust will automatically call the drop function on the data if necessary.

Key Changes:

1. Removal of the finalizer method for JsHash in the impl JsClassDef block.
2. Removal of the finalizer method for JsHmac in the impl JsClassDef block.
3. Removal of the finalizer method for JsCipher in the impl JsClassDef block.

Potential Problems:

• The patch appears reasonably safe since the finalizers were just calling the drop function, which will automatically be called by Rust when needed.
• However, if there are any custom memory management settings or dependencies on these specific finalizers, removing them may cause unintended side effects or memory leaks. It would be important to ensure that these finalizers are truly unnecessary before approving the changes.

Commit 20

Key changes:

1. Implemented SecretKeyHandle class in modules/internal/crypto/keys.js that wraps a key buffer and provides methods for symmetric key management.
2. Modified various existing functions to use SecretKeyHandle for key management.
3. Updated test cases to use createSecretKey() method.

Potential problems:

1. Commented-out code blocks in modules/crypto.js might lead to confusion and can be removed if not necessary.
2. Duplicated code blocks in modules/crypto.js showing both commented and uncommented code without any explanation may lead to confusion and should be cleaned up.
3. A direct replacement of the prepareKey function in modules/internal/crypto/hkdf.js is performed, which changes the return type from ArrayBuffer to SecretKeyObject. This might cause issues in the existing code that expects an ArrayBuffer in return.

Aside from the mentioned problems, the implementation seems satisfactory, and the test cases have been updated appropriately.

[lastmjs]

What's it going to take to get this PR merged?

[L-jasmine]

@Puellaquae https://github.com/second-state/wasmedge-quickjs/actions/runs/7643352809/job/20825160000?pr=92#step:26:81

Actually, the tests for crypto probably did not run because an error was encountered, indicating that the plugin is not installed. Please install the WASI-Crypto plugin in the CI environment. You should be able to install it using a script.