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(sdk): fix client tls connections #2223

Merged
merged 6 commits into from
Oct 8, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
added root certificates
  • Loading branch information
QuantumExplorer committed Oct 8, 2024
commit 2f60720fc4abec478fdb8b3881fc3d433b86f0f5
6 changes: 5 additions & 1 deletion packages/rs-dapi-client/src/transport/grpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@
use super::{CanRetry, TransportClient, TransportRequest};
use crate::connection_pool::{ConnectionPool, PoolPrefix};
use crate::{request_settings::AppliedRequestSettings, RequestSettings};
use dapi_grpc::core::v0::core_client::CoreClient;

Check failure on line 8 in packages/rs-dapi-client/src/transport/grpc.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

failed to resolve: could not find `core` in `dapi_grpc`

error[E0433]: failed to resolve: could not find `core` in `dapi_grpc` --> packages/rs-dapi-client/src/transport/grpc.rs:8:16 | 8 | use dapi_grpc::core::v0::core_client::CoreClient; | ^^^^ could not find `core` in `dapi_grpc` | note: found an item that was configured out --> /home/ubuntu/actions-runner/_work/platform/platform/packages/dapi-grpc/src/lib.rs:4:9 | 4 | pub mod core { | ^^^^ = note: the item is gated behind the `core` feature
use dapi_grpc::core::v0::{self as core_proto};

Check failure on line 9 in packages/rs-dapi-client/src/transport/grpc.rs

View workflow job for this annotation

GitHub Actions / Rust packages (rs-dapi-client) / Linting

unresolved import `dapi_grpc::core`

error[E0432]: unresolved import `dapi_grpc::core` --> packages/rs-dapi-client/src/transport/grpc.rs:9:16 | 9 | use dapi_grpc::core::v0::{self as core_proto}; | ^^^^ could not find `core` in `dapi_grpc` | note: found an item that was configured out --> /home/ubuntu/actions-runner/_work/platform/platform/packages/dapi-grpc/src/lib.rs:4:9 | 4 | pub mod core { | ^^^^ = note: the item is gated behind the `core` feature
use dapi_grpc::platform::v0::{self as platform_proto, platform_client::PlatformClient};
use dapi_grpc::tonic::transport::{ClientTlsConfig, Uri};
use dapi_grpc::tonic::Streaming;
Expand All @@ -22,7 +22,11 @@
uri: Uri,
settings: Option<&AppliedRequestSettings>,
) -> Result<Channel, dapi_grpc::tonic::transport::Error> {
let mut builder = Channel::builder(uri).tls_config(ClientTlsConfig::new())?;
let mut builder = Channel::builder(uri).tls_config(
ClientTlsConfig::new()
.with_native_roots()
.with_webpki_roots(),
)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Redundant TLS root certificates configuration

In the create_channel function, both with_native_roots() and with_webpki_roots() are called on the TLS configuration. These methods set the root certificate store, and calling both may lead to unintended behavior as they might overwrite each other. To ensure the TLS configuration is set correctly, consider using only one of these methods based on the desired root certificates source.

Apply this diff to fix the redundancy:

         ClientTlsConfig::new()
-            .with_native_roots()
             .with_webpki_roots(),
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let mut builder = Channel::builder(uri).tls_config(
ClientTlsConfig::new()
.with_native_roots()
.with_webpki_roots(),
)?;
let mut builder = Channel::builder(uri).tls_config(
ClientTlsConfig::new()
.with_webpki_roots(),
)?;


if let Some(settings) = settings {
if let Some(timeout) = settings.connect_timeout {
Expand Down
Loading