Skip to content
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

feat: Add support for custom SSL certificates #140

Merged
merged 3 commits into from
May 31, 2024

Conversation

subbu963
Copy link
Contributor

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.

Copy link
Member

juntao commented May 30, 2024

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:

  • Two test files, custom-ssl.js and test-cert.rs, have been removed from the codebase.

Potential Problems:

  • Removing test files may result in a decrease in test coverage, potentially leading to undiscovered bugs.
  • It's important to ensure that the functionality covered by the removed test files is still adequately tested elsewhere in the codebase.
  • If these tests were related to custom SSL certificates, it's crucial to verify that the support for custom SSL certificates is still adequately tested after the removal of these files.

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
Copy link
Contributor Author

@juntao @L-jasmine please check this

@L-jasmine
Copy link
Collaborator

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

@L-jasmine L-jasmine merged commit ab18110 into second-state:main May 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants