Use aws-lc-rs where possible for TLS crypto #164
Conversation
Summary of ChangesHello @jadamcrain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of TLS cryptographic providers by integrating AWS-LC as an alternative to the existing Ring implementation. It allows users to choose the optimal crypto backend based on their platform and performance requirements, leveraging AWS-LC for modern 64-bit systems and retaining Ring for broader compatibility with musl and 32-bit ARM. The changes are implemented through new and updated feature flags, ensuring that the default behavior remains consistent while offering powerful new options for advanced configurations. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Summary of ChangesHello @jadamcrain, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the flexibility of the system's TLS capabilities by integrating AWS-LC as an alternative cryptographic backend. This strategic addition provides users with a choice of crypto providers, enabling them to optimize for either superior long-term support and performance on modern 64-bit systems with AWS-LC, or broader compatibility with musl libc and 32-bit ARM architectures offered by the existing Ring implementation. The changes are implemented through new feature flags and conditional compilation, ensuring that the default behavior remains unchanged while offering powerful new options for advanced configurations. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for AWS-LC as an alternative cryptographic provider for TLS, which is a valuable addition for users seeking better performance and long-term support on modern platforms. The implementation is well-structured, particularly the use of the enable-tls feature flag to gate all TLS-related code, which simplifies conditional compilation across different crypto providers. The changes to Cargo.toml files are consistent and correctly define the new feature flags. The codebase is updated cleanly to use the new enable-tls feature. Making the crc dependency optional and part of the serial feature is also a good improvement. Overall, this is a high-quality pull request with well-thought-out changes.
There was a problem hiding this comment.
Code Review
This pull request introduces support for AWS-LC as an alternative cryptographic provider for TLS, which is a great addition for performance and long-term support. The changes are well-structured, particularly the introduction of the enable-tls feature flag to gate all TLS-related code, and the clear separation of ring and aws-lc providers into distinct features. I also appreciate the improvement of making the crc dependency optional and part of the serial feature, which slims down the dependency tree for users not requiring serial communication.
I have one suggestion to further improve the maintainability of the feature flags in rodbus/Cargo.toml by reducing some duplication. Overall, this is a high-quality contribution.
rodbus/Cargo.toml
Outdated
| enable-tls = [] | ||
| tls = ["enable-tls", "rx509", "sfio-rustls-config/crypto-ring", "tokio-rustls"] | ||
| tls-aws-lc = ["enable-tls", "rx509", "sfio-rustls-config/crypto-aws-lc", "tokio-rustls"] |
There was a problem hiding this comment.
To improve maintainability and reduce duplication, you could move the common TLS dependencies into the enable-tls feature itself. This makes the definitions for tls and tls-aws-lc cleaner and ensures that all necessary TLS dependencies are enabled whenever any TLS feature is active.
| enable-tls = [] | |
| tls = ["enable-tls", "rx509", "sfio-rustls-config/crypto-ring", "tokio-rustls"] | |
| tls-aws-lc = ["enable-tls", "rx509", "sfio-rustls-config/crypto-aws-lc", "tokio-rustls"] | |
| enable-tls = ["rx509", "tokio-rustls"] | |
| tls = ["enable-tls", "sfio-rustls-config/crypto-ring"] | |
| tls-aws-lc = ["enable-tls", "sfio-rustls-config/crypto-aws-lc"] |
Adds support for AWS-LC as an alternative cryptographic provider for TLS, alongside the existing Ring implementation. This allows users to choose the crypto provider that best suits their platform and performance requirements.
New Features
tls-aws-lcfeature: Enables TLS with AWS-LC crypto providerenable-tlsfeature: Internal feature flag that gates all TLS code, enabled by bothtlsandtls-aws-lcUpdated Features
tlsfeature: Now explicitly uses Ring crypto provider viasfio-rustls-config/crypto-ringCI Updates
FFI binaries now use the optimal crypto provider for each platform:
tls-aws-lc): Windows, macOS, x86_64-linux-gnu, aarch64-linux-gnu)tls): musl targets and 32-bit ARM Linux platformsMotivation
AWS-LC is maintained by a (MUCH) larger team and the long term prospects for its support are superior. Secondarily
it provides better performance on modern 64-bit platforms (x86_64 and ARM64), while Ring offers superior compatibility with musl libc and 32-bit ARM architectures.
Breaking Changes
None. The default feature set remains unchanged (
tlsandserial), continuing to use Ring as the crypto provider.Usage
Users can now choose their crypto provider when building:
# Use Ring (default) - `tls,serial` features enabled by default cargo build# Use AWS-LC cargo build --no-default-features --features tls-aws-lc,serial