feat(sdk)!: allow setting CA cert #1924
Conversation
lklimek
changed the title
WalkthroughThe changes in this pull request enhance the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
packages/rs-dapi-client/src/transport/grpc.rs (1)
25-31: Review the usage of bothwith_native_roots()andwith_webpki_roots()Using both
with_native_roots()andwith_webpki_roots()may be redundant, as they both provide root certificates for TLS verification. Consider whether both are necessary or if one suffices for your use case to simplify the TLS configuration.packages/rs-sdk/src/sdk.rs (2)
784-784: Typo in comment: 'conttact' should be 'contract'There's a typo in the comment on line 784. Please correct 'conttact' to 'contract' for clarity.
Line range hint
916-916: Typo in error message: 'connot' should be 'cannot'In the error message on line 916, correct 'connot' to 'cannot' for proper spelling.
packages/rs-dapi-client/src/dapi_client.rs (2)
79-80: Consider enhancing field documentation with example usage.The field documentation is clear but could be more helpful with an example showing how to create a Certificate from PEM data.
- /// Certificate Authority certificate to use for verifying the server's certificate. + /// Certificate Authority certificate to use for verifying the server's certificate. + /// + /// # Example + /// ``` + /// use tonic::transport::Certificate; + /// + /// let pem = std::fs::read("ca.pem").unwrap(); + /// let cert = Certificate::from_pem(pem); + /// ```
Line range hint
1-324: Consider documenting the TLS configuration workflow.The implementation successfully adds CA certificate support, but consider adding high-level documentation explaining:
- The TLS configuration workflow
- How CA certificates are used in the DAPI client
- Common use cases (e.g., mitmproxy integration)
This would help users understand when and how to use this feature effectively.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
packages/rs-dapi-client/src/dapi_client.rs(7 hunks)packages/rs-dapi-client/src/request_settings.rs(3 hunks)packages/rs-dapi-client/src/transport/grpc.rs(4 hunks)packages/rs-sdk/src/sdk.rs(5 hunks)packages/rs-sdk/tests/.env.example(1 hunks)packages/rs-sdk/tests/fetch/config.rs(2 hunks)packages/rs-sdk/tests/fetch/data_contract.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/rs-sdk/tests/.env.example
🔇 Additional comments (19)
packages/rs-dapi-client/src/transport/grpc.rs (4)
11-11: Import added correctly
The import statement for Certificate, ClientTlsConfig, and Uri has been correctly added to support TLS configuration enhancements.
38-41: Correctly applied custom CA certificate and domain name
The custom CA certificate and domain name are appropriately set in the TLS configuration, enabling secure connections with specified CA certificates.
269-272: Verify the impact of setting retries to 0 and reducing the timeout
Setting retries to 0 and reducing the timeout to 80 seconds for WaitForStateTransitionResultRequest may affect the robustness of this request. Please verify if this change is intentional and aligns with the desired behavior, ensuring it does not negatively impact request reliability.
502-504: Clarify the reasoning for setting fields to None
Setting ban_failed_address, connect_timeout, and retries to None for TransactionsWithProofsRequest alters the default behavior of these settings. Please confirm that this change is intentional and verify that it doesn't introduce unintended side effects or affect the request's reliability.
packages/rs-sdk/src/sdk.rs (3)
13-13: Import added for certificate handling
The import statement for Certificate has been correctly added to support CA certificate configuration in the SDK.
837-846: CA certificate configuration methods added appropriately
The methods with_ca_certificate and with_ca_certificate_file have been correctly added to SdkBuilder, allowing users to specify a CA certificate directly or via a file.
999-1002: Applied CA certificate to DapiClient correctly
The CA certificate is appropriately applied to the DapiClient when building the SDK, ensuring secure connections with the specified certificate.
packages/rs-dapi-client/src/request_settings.rs (4)
3-3: Import added for Certificate handling
The import of Certificate from tonic::transport has been correctly added to support CA certificate functionality in request settings.
68-68: Initialized ca_certificate field in AppliedRequestSettings
The ca_certificate field is correctly initialized to None in the finalize method, ensuring a default state when no CA certificate is provided.
74-74: Updated AppliedRequestSettings to include ca_certificate
The AppliedRequestSettings struct has been properly updated to include the ca_certificate field. Note that the Copy trait has been removed due to the addition of the Certificate type, which is not Copy.
89-92: Added with_ca_certificate method
The with_ca_certificate method allows setting a custom CA certificate in AppliedRequestSettings, enhancing TLS configuration capabilities.
packages/rs-sdk/tests/fetch/data_contract.rs (1)
11-12: Added logging setup to improve test output
The addition of super::common::setup_logs(); enhances the test by enabling detailed logging, which aids in debugging test failures.
packages/rs-sdk/tests/fetch/config.rs (2)
52-54: Extended Config struct to include platform_ca_cert_path
The new optional platform_ca_cert_path field allows specifying a CA certificate path when platform_ssl is enabled, enhancing SSL configuration flexibility.
191-195: Conditionally included CA certificate in SDK builder
The code correctly integrates the CA certificate into the SDK builder when platform_ca_cert_path is provided, ensuring secure connections when SSL is enabled.
packages/rs-dapi-client/src/dapi_client.rs (5)
6-6: LGTM!
The Certificate import from tonic transport is correctly added and necessary for the new CA certificate functionality.
97-97: LGTM!
The ca_certificate field is correctly initialized to None in the constructor.
135-144: LGTM! Settings modification is properly handled.
The CA certificate is correctly applied to the settings when present.
101-113: Consider validating the CA certificate.
While the builder pattern implementation is correct, consider adding validation of the certificate to ensure it's properly formatted and valid before storing it.
Let's check if certificate validation is handled elsewhere in the codebase:
pub fn with_ca_certificate(mut self, ca_cert: Certificate) -> Self {
+ // Attempt to validate the certificate
+ if let Err(e) = ca_cert.validate() {
+ tracing::warn!("Invalid CA certificate provided: {}", e);
+ }
self.ca_certificate = Some(ca_cert);
self
}Line range hint 214-224: Verify transport compatibility with CA certificates.
Ensure that the transport layer properly handles the CA certificate during TLS handshake.
Let's check the transport implementation:
✅ Verification successful
Let me gather more context about the transport implementation and certificate handling:
Let me check one more aspect of the TLS configuration implementation:
TLS configuration and certificate handling is properly implemented
Based on the code analysis, the transport layer correctly handles TLS certificates through:
- Proper TLS configuration with native and webpki roots
- Explicit CA certificate handling with domain name verification
- Comprehensive error handling for transport and TLS-related issues
- Proper connection pooling with TLS settings
The implementation in packages/rs-dapi-client/src/transport/grpc.rs shows that:
let mut tls_config = ClientTlsConfig::new()
.with_native_roots()
.with_webpki_roots()
.assume_http2(true);And when custom certificates are provided:
tls_config = tls_config.ca_certificate(cert).domain_name(host);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for transport TLS configuration
rg -g '!target' -g '*.rs' "tls.*config|transport.*tls|certificate.*tls" -A 5
Length of output: 1266
Script:
#!/bin/bash
# Search for transport and certificate related code
ast-grep --pattern 'fn $_(transport|certificate|tls|cert)$_($$$) {
$$$
}'
# Also check for any certificate-related structs or types
rg -g '!target' -g '*.rs' "struct.*Transport|struct.*Certificate|type.*Transport|type.*Certificate" -A 3
Length of output: 4304
Script:
#!/bin/bash
# Check for certificate validation and error handling
rg -g '!target' -g '*.rs' "fn.*create_transport|fn.*create_client" -A 10
# Also check for any TLS-related error handling
rg -g '!target' -g '*.rs' "TlsError|CertificateError|TransportError" -A 3
Length of output: 10650
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
packages/rs-dapi-client/src/dapi_client.rs(8 hunks)packages/rs-dapi-client/src/request_settings.rs(3 hunks)packages/rs-sdk/Cargo.toml(1 hunks)packages/rs-sdk/src/sdk.rs(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/rs-sdk/Cargo.toml
🧰 Additional context used
📓 Learnings (1)
packages/rs-sdk/src/sdk.rs (1)
Learnt from: lklimek
PR: dashpay/platform#1924
File: packages/rs-sdk/src/sdk.rs:855-870
Timestamp: 2024-12-03T15:00:50.385Z
Learning: In `packages/rs-sdk/src/sdk.rs`, remember that `rustls_pemfile::certs` returns an iterator over `Result`, not a `Result` directly. Handle it accordingly in future code reviews.
🔇 Additional comments (8)
packages/rs-dapi-client/src/request_settings.rs (1)
88-94: LGTM! Well-documented builder method
The with_ca_certificate method follows the builder pattern correctly and includes clear documentation explaining its purpose and behavior.
packages/rs-dapi-client/src/dapi_client.rs (3)
80-81: LGTM! Well-documented field addition
The ca_certificate field is properly documented and initialized to None by default.
Also applies to: 98-98
102-114: LGTM! Builder method follows best practices
The with_ca_certificate method follows the builder pattern correctly and includes comprehensive documentation.
205-208: LGTM! Proper CA certificate propagation
The CA certificate is correctly propagated to the applied settings only when present, addressing the previous concern about certificate override behavior.
packages/rs-sdk/src/sdk.rs (4)
744-746: LGTM! Well-documented field addition
The ca_certificate field is properly documented and initialized to None by default.
Also applies to: 778-778
829-837: LGTM! Builder method follows best practices
The with_ca_certificate method follows the builder pattern correctly and includes comprehensive documentation.
839-862: LGTM! Proper certificate file validation
The with_ca_certificate_file method correctly validates the certificate file by:
- Reading the file content
- Verifying the PEM format using rustls_pemfile
- Converting to a Certificate instance
The implementation correctly handles the iterator returned by rustls_pemfile::certs, as noted in the previous feedback.
991-994: LGTM! Proper CA certificate propagation
The CA certificate is correctly propagated to the DapiClient during build, maintaining the builder pattern's immutability.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/rs-dapi-client/src/dapi_client.rs (2)
80-81: LGTM with documentation enhancement suggestionThe new
ca_certificatefield is well-defined and properly initialized. Consider enhancing the documentation to include:
- The expected format of the certificate (PEM)
- Any specific requirements or limitations
- Example usage
Also applies to: 98-98
102-114: Documentation could be more comprehensiveWhile the implementation is correct, the documentation could be enhanced with:
- Example usage code
- Notes about the impact on TLS verification
- Whether the certificate needs to be in a specific format
- Error scenarios (if any)
/// Set CA certificate to use when verifying the server's certificate. /// /// # Arguments /// /// * `pem_ca_cert` - CA certificate in PEM format. /// /// # Returns /// [DapiClient] with CA certificate set. + /// + /// # Example + /// ``` + /// # use std::fs; + /// # use dapi_grpc::tonic::transport::Certificate; + /// let ca_cert = Certificate::from_pem(fs::read("ca.pem")?); + /// let client = DapiClient::new(address_list, settings) + /// .with_ca_certificate(ca_cert); + /// ``` + /// + /// # Notes + /// - The certificate must be in PEM format + /// - This certificate will be used to verify the server's certificate during TLS handshake
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/rs-dapi-client/src/dapi_client.rs(9 hunks)
🔇 Additional comments (3)
packages/rs-dapi-client/src/dapi_client.rs (3)
6-6: LGTM: Import for Certificate type
The addition of the Certificate import from tonic transport is appropriate for the new CA certificate functionality.
203-204: LGTM: Proper handling of settings and CA certificate
The changes appropriately:
- Chain the CA certificate configuration after settings finalization
- Address the previous review comment about shadowing by using references
- Maintain consistent usage of the settings reference throughout the execute method
Also applies to: 220-222, 237-237, 267-267, 277-277, 306-306
203-204: Verify error handling for certificate validation failures
Since this involves TLS certificate validation, we should ensure proper error handling when:
- The CA certificate is invalid
- The server's certificate fails validation
- The certificate chain is incomplete
✅ Verification successful
Certificate validation is properly handled with robust error propagation
The code shows proper certificate validation and error handling:
- Certificate validation errors are propagated through the
TransportErrortype - Invalid certificates are caught during channel creation and result in connection errors
- TLS configuration properly validates certificates against the provided CA certificate
- Errors are properly logged and propagated up the call stack
The implementation follows Rust best practices for error handling and TLS certificate validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling related to certificate validation
rg -A 5 "Certificate|TLS|tls|ssl|SSL"
# Look for transport error handling specifically
ast-grep --pattern 'match $_ {
$$$
Transport($_) => $$$
$$$
}'
Length of output: 73661
Script:
#!/bin/bash
# Search for error handling in the dapi client code
rg -A 5 "Error|error|fail|Fail" packages/rs-dapi-client/src/dapi_client.rs
# Look for certificate validation error handling specifically
ast-grep --pattern 'match $_ {
$$$
Err($_) => $$$,
$$$
}' packages/rs-dapi-client/src/dapi_client.rs
Length of output: 6748
Script:
#!/bin/bash
# Look for certificate validation code and error handling
rg -A 5 "ca_certificate|certificate|Certificate" packages/rs-dapi-client/src/transport/
# Look for error handling in transport layer
rg -A 5 "TransportError|transport_error" packages/rs-dapi-client/src/transport/
Length of output: 5206
shumkov
changed the title
Issue being fixed or feature implemented
To intercept traffic, I use mitmproxy.
However, it doesn't support http/3 without tls, and to work with tls I need to set ca certificate inside SDK.
What was done?
Developers can use
SdkBuilder::with_ca_certificateto provide CA certificate to use when verifying DAPI certificates.Added CA cert to DapiClient and AppliedRequestSettings.
This means that AppliedRequestSettings no longer implements Copy trait.
How Has This Been Tested?
DASH_SDK_PLATFORM_SSL=truebut no SSL certcargo test -p dash-sdk --no-default-features -F network-testing test_data_contract_read_not_found; result: errorDASH_SDK_PLATFORM_CA_CERT_PATH=/path/to/mitmproxy/cacrt.pemcargo test -p dash-sdk --no-default-features -F network-testing test_data_contract_read_not_found; result: OKBreaking Changes
AppliedRequestSettings have one additional field that must be filled (can be
None).AppliedRequestSettings no longer implement Copy trait.
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
DapiClientandSdkBuilder.Bug Fixes
Documentation
Tests