second-state / wasmedge-quickjs

A high-performance, secure, extensible, and OCI-complaint JavaScript runtime for WasmEdge.
Apache License 2.0
477 stars 59 forks source link

feat: Add support for custom SSL certificates #140

Closed subbu963 closed 3 months ago

subbu963 commented 3 months ago

wasmedge-quickjs doesnt support custom SSL support. Currently, wasmedge-quickjs uses webpki_roots to inject commonly used ssl certs without a way to customize. Without this PR, you cant call any HTTPS endpoints which has a cert that isnt supported by webpki_roots crate. Particulary, its important for companies with custom SSL certs.

juntao commented 3 months ago

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


Overall Summary:

The Pull Request introduces support for custom SSL certificates through various key changes, such as adding scripts and modules, updating dependencies, and creating new files. Potential issues include the lack of error handling in certain parts of the code, the impact of dependency updates on compatibility, and assumptions made about the success of certificate operations. Additionally, the removal of test files raises concerns about test coverage and the need to ensure thorough testing of custom SSL certificate functionality post-removal.

Key Findings:

  1. Error handling needs improvement in the script get_cert.sh, certs.rs, and mod.rs.
  2. Compatibility testing is essential due to updated dependency versions, especially with the introduction of rustls-pemfile.
  3. Careful consideration should be given to the success assumption in certificate operations to enhance robustness.
  4. The impact of removing test files on test coverage and the adequacy of testing for custom SSL certificate functionality should be addressed.
  5. Offering more flexibility in specifying custom certificate locations and enhancing error-handling and troubleshooting information in README instructions can improve user experience and tool usability.

Details

Commit 1b3016c0126a848e4da06665b0cc4cd8eff7d17e

Key Changes:

  1. Added support for custom SSL certificates by introducing a new script get_cert.sh to retrieve combined TLS certificates for a given domain.
  2. Added a new module certs.rs for loading certificates from environment variable SSL_CERT_FILE.
  3. Updated the mod.rs file to load custom certificates if available, or fallback to default webpki certificates.
  4. Updated dependencies (rustls-pemfile version) in Cargo.lock and Cargo.toml.
  5. Created new files (get_cert.sh, certs.rs, custom-ssl.js, test-cert.rs).

Potential Problems:

  1. The script get_cert.sh might need error handling for cases where the domain argument is missing or invalid.
  2. In the certs.rs file, handling Err with io::Result::Err could be improved for better error reporting.
  3. The dependency versions have been updated, which may introduce compatibility issues with existing code or other dependencies.
  4. Since rustls-pemfile is a new dependency, its usage and compatibility with existing code should be thoroughly tested.
  5. The root_store.add(&cert).unwrap() call in mod.rs assumes that adding a certificate will always succeed, which may not always be the case. Error handling should be improved here.

Overall, the addition of custom SSL certificate support is a significant enhancement, but the error handling, compatibility testing, and robustness of the new implementation should be carefully reviewed before merging.

Commit 6ba840efde52a5e028ce32977b30c6fc92a716b6

Key Changes:

Potential Problems:

Commit 3f8a4d41b9d96eb76c597bd4e8d2a9b1bc6e2d32

Key changes in the patch:

  1. Added instructions in the README for using custom SSL certificates with the tool.
  2. Provided an example command for using custom SSL certificates when running the tool.

Potential problems:

  1. Using hardcoded paths (/etc/ssl/cert.pem in this case) may not be suitable for all users or systems. It might be better to provide a more flexible way for users to specify custom certificate locations.
  2. It's unclear how the tool handles errors or missing SSL certificates. It might be beneficial to include information on how to troubleshoot common SSL certificate-related issues.
subbu963 commented 3 months ago

@juntao @L-jasmine please check this

L-jasmine commented 3 months ago

LGTM. @subbu963 Thank you so much for your pr.