feat: wire up insecure TLS option for S3 client#17
Conversation
When an alias is configured with `insecure = true` (via `rc alias set --insecure`), the S3 client now uses a reqwest-based HTTP connector that calls `danger_accept_invalid_certs(true)`, allowing connections to servers using self-signed TLS certificates. Also supports `ca_bundle` for custom CA certificate bundles. Implements the custom connector by wrapping reqwest::Client in the aws-smithy-runtime-api HttpConnector and HttpClient traits, and wiring it into the aws-config builder when insecure mode or ca_bundle is set. Co-authored-by: overtrue <1472352+overtrue@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds TLS flexibility to the S3 client layer by introducing a reqwest-based AWS SDK HTTP connector that can (optionally) skip certificate verification and/or trust a custom CA bundle, enabling use against self-signed or privately-issued endpoints.
Changes:
- Implement
ReqwestConnectorimplementing SmithyHttpConnector+HttpClientto back the AWS SDK with reqwest. - Select the custom connector when
alias.insecureis enabled or whenalias.ca_bundleis provided. - Add workspace/crate dependencies for
aws-smithy-runtime-apiandbytes(and update lockfile).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/s3/src/client.rs | Adds reqwest-based Smithy HTTP connector and wires it into S3 client config based on alias TLS options. |
| crates/s3/Cargo.toml | Adds new dependencies required by the custom connector. |
| Cargo.toml | Adds workspace dependency versions for aws-smithy-runtime-api and bytes. |
| Cargo.lock | Lockfile updates for the new dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if let Some(bundle_path) = ca_bundle { | ||
| let pem = std::fs::read(bundle_path).map_err(|e| { | ||
| Error::Network(format!("Failed to read CA bundle '{bundle_path}': {e}")) | ||
| })?; | ||
| let cert = reqwest::Certificate::from_pem(&pem) | ||
| .map_err(|e| Error::Network(format!("Invalid CA bundle '{bundle_path}': {e}")))?; | ||
| builder = builder.add_root_certificate(cert); |
There was a problem hiding this comment.
ReqwestConnector::new reads the CA bundle using std::fs::read, which is a blocking filesystem call. Since this is invoked from async code (S3Client::new), consider switching to tokio::fs::read or spawn_blocking to avoid blocking the async runtime thread, especially if aliases are created on-demand.
There was a problem hiding this comment.
Fixed in ae82923. ReqwestConnector::new is now async fn and uses tokio::fs::read(...).await instead of the blocking std::fs::read. The call site in S3Client::new has been updated to .await? accordingly.
crates/s3/src/client.rs
Outdated
| let body_bytes = request | ||
| .body() | ||
| .bytes() | ||
| .map(Bytes::copy_from_slice) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
request.body().bytes().unwrap_or_default() silently turns any non-buffered/streaming request body into an empty body. That can break operations that use streaming bodies (e.g., future uploads/multipart) and can lead to signature mismatches or server-side errors. Prefer handling the streaming case explicitly (buffer the body, or return a ConnectorError indicating unsupported streaming) rather than defaulting to empty.
There was a problem hiding this comment.
Fixed in ae82923. request.body().bytes().unwrap_or_default() is replaced with a match that returns a clear ConnectorError::user(...) when bytes() is None (streaming body), rather than silently sending an empty body.
There was a problem hiding this comment.
This was already addressed in ae82923. The unwrap_or_default() is replaced with a match that returns a clear ConnectorError::user(...) when bytes() is None (streaming body).
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: 安正超 <anzhengchao@gmail.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: 安正超 <anzhengchao@gmail.com>
- Switch CA bundle file read from std::fs::read (blocking) to tokio::fs::read (async), avoiding blocking the async runtime - Fix streaming body handling: return a clear ConnectorError instead of silently sending an empty body when request.body().bytes() is None - Make ReqwestConnector::new() async and update call site to .await - Convert unit tests to #[tokio::test] async fns to match async new() - Remove the unreliable reqwest_connector_invalid_ca_bundle_contents test: reqwest with rustls does not validate certificate DER content at build time, making this test non-deterministic across versions Co-authored-by: overtrue <1472352+overtrue@users.noreply.github.com>
aws-smithy-runtime-apiandbytesto workspace/crate dependenciesReqwestConnectoras a customHttpConnector+HttpClientfor AWS SDKReqwestConnectorwithdanger_accept_invalid_certs(true)whenalias.insecure = trueca_bundlesupport for custom CA certificates in S3 clientinsecureandca_bundleactivation conditionsHttpConnectorSettingsnot being appliedstd::fs::readto asynctokio::fs::readConnectorErrorinstead of silently sending empty body#[tokio::test]) to match asyncnew()constructorOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.