Skip to content

Allow selecting AWS SDK TLS features#197

Open
dertin wants to merge 9 commits into
aws:mainfrom
dertin:fix/caching-client-tls-features
Open

Allow selecting AWS SDK TLS features#197
dertin wants to merge 9 commits into
aws:mainfrom
dertin:fix/caching-client-tls-features

Conversation

@dertin

@dertin dertin commented Apr 30, 2026

Copy link
Copy Markdown

Description

Why is this change being made?

  1. Some applications need to opt out of the AWS SDK service client default TLS feature set and select the SDK HTTP client features explicitly.

What is changing?

  1. Adds crate features that forward to the AWS SDK default-https-client, rt-tokio, credentials-process, sso, and rustls features.
  2. Keeps the current default feature behavior unchanged for existing users.
  3. Documents how to use aws_secretsmanager_caching with default-features = false and the modern HTTPS client feature set.

Related Links

  • Issue #, if available: N/A

Testing

How was this tested?

  1. cargo check -p aws_secretsmanager_caching
  2. cargo check -p aws_secretsmanager_caching --all-features
  3. cargo check -p aws_secretsmanager_caching --no-default-features --features default-https-client,rt-tokio,credentials-process,sso
  4. cargo test -p aws_secretsmanager_caching --no-fail-fast
  5. cargo test -p aws_secretsmanager_caching --no-default-features --features default-https-client,rt-tokio,credentials-process,sso --no-fail-fast

When testing locally, provide testing artifact(s):

  1. All commands above passed locally. Existing unused_parens warnings in tests are still present.

Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why: Not run locally.
  • Integration tests pass locally
    If not, why: Not run locally.
  • I have updated integration tests (if needed)
    If not, why: Not needed; this only changes Cargo feature wiring and README docs.
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why: Not needed; no runtime logic was added.
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why: No breaking changes. Default features are kept unchanged.

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

@dertin dertin requested a review from a team as a code owner April 30, 2026 02:11
@simonmarty

simonmarty commented May 11, 2026

Copy link
Copy Markdown
Contributor

This looks ok to me at first glance. Can you elaborate on your use case?

simonmarty and others added 2 commits May 11, 2026 11:05
Signed-off-by: Simon Marty <martysi@amazon.com>
Signed-off-by: Simon Marty <simon.marty@protonmail.com>
@codecov

codecov Bot commented May 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.27%. Comparing base (fdfc8af) to head (9b40629).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #197   +/-   ##
=======================================
  Coverage   86.27%   86.27%           
=======================================
  Files          34       34           
  Lines        9331     9331           
  Branches     9331     9331           
=======================================
  Hits         8050     8050           
  Misses       1148     1148           
  Partials      133      133           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@dertin

dertin commented May 11, 2026

Copy link
Copy Markdown
Author

Hi @simonmarty, thanks for asking.

My main use case is dependency-tree control for applications that explicitly manage the AWS SDK HTTP/TLS stack.

In our application, we want to use the SDK default-https-client + rt-tokio path consistently, without aws_secretsmanager_caching enabling the service-client rustls feature again through its default features.

This matters because enabling aws-sdk-secretsmanager/rustls still selects the legacy aws-smithy-runtime/tls-rustls path, which brings in the older hyper/rustls stack. What we want instead is to stay on the modern default-https-client path and avoid carrying the legacy rustls 0.21 stack in this part of the dependency graph.

At the moment, we need to keep temporary cargo audit ignores for advisories that are only present because this legacy TLS path is still being resolved: RUSTSEC-2026-0098, RUSTSEC-2026-0099, and RUSTSEC-2026-0104.

With this PR, existing users keep the current default behavior, while downstream applications can opt out explicitly by using aws_secretsmanager_caching with default-features = false and enabling only the SDK features they need.

For this TLS-selection use case, the following example should be enough:

aws_secretsmanager_caching = { version = "2", default-features = false, features = [
  "default-https-client",
  "rt-tokio",
] }

If an application relies on AWS profiles that use SSO or credential_process, it can add sso and/or credentials-process explicitly. They are useful options for compatibility with those credential-provider setups, but they are not required for the TLS-selection use case itself.

@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label May 14, 2026
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label May 14, 2026
@dertin

dertin commented Jun 24, 2026

Copy link
Copy Markdown
Author

Hi @simonmarty, just checking in on this. Have you had a chance to take another look? Let me know if there’s anything else you’d like me to adjust.

@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 4m 36s —— View job


Review: PR #197 — Allow selecting AWS SDK TLS features

  • Read PR diff against origin/main
  • Read CLAUDE.md and full Cargo.toml
  • Analyze feature-flag wiring for correctness
  • Verify default behavior is preserved
  • Post review feedback

Thanks @dertin — the motivation is well-explained and the approach (forwarding crate features to the SDK features, keeping default equivalent to the old behavior) is the right shape. The default-feature set (rustls, default-https-client, rt-tokio, credentials-process, sso) is a faithful reproduction of the SDK defaults, so existing users should be unaffected. I couldn't run cargo check in this environment (build commands aren't permitted here), so the notes below are from static analysis — please confirm the two build scenarios I call out.

Findings

1. fips may no longer be self-sufficient (highest priority)inline
Making both SDK deps default-features = false means fips = ["rustls/fips"] now only enables FIPS on the standalone rustls 0.23 crate — it no longer implies any SDK HTTP client. A default-features = false, features = ["fips"] build would compile but have no transport at runtime. Also worth confirming that rustls/fips (0.23) actually reaches the SDK's TLS path, given your own note that the SDK rustls feature resolves the legacy rustls 0.21 stack.

2. Feature/crate name collisioninline
The new rustls feature shares a name with the rustls crate dependency. Legal in Cargo, but fips = ["rustls/fips"] (crate) vs rustls = ["aws-sdk-secretsmanager/rustls"] (feature) is confusing side by side. A distinct name (sdk-rustls / legacy-tls) would read better. Non-blocking.

3. Version pinning in the README example + missing version bumpinline
The doc snippet uses version = "2" with default-features = false. With a caret "2" requirement, Cargo can resolve an older 2.x that lacks these features, leaving the user with no SDK features enabled. The package version is still 2.1.0; a feature-adding change should bump the minor version, and the README should reference that minimum.

Things that look good

  • Default behavior preserved — default mirrors the previous implicit SDK defaults.
  • dev-dependencies correctly re-add default-https-client + rt-tokio alongside test-util so the test suite keeps a transport. Codecov shows all covered lines still covered.
  • README documentation of the opt-out path is a helpful addition.

Suggested follow-ups

Overall a clean, backward-compatible change; the main item to resolve before merge is the fips interaction (#1).

Comment thread aws_secretsmanager_caching/Cargo.toml Outdated
rt-tokio = ["aws-config/rt-tokio", "aws-sdk-secretsmanager/rt-tokio"]
rustls = ["aws-sdk-secretsmanager/rustls"]
sso = ["aws-config/sso"]
fips = ["rustls/fips"]

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

fips no longer pulls an HTTP/TLS client on its own.

Before this PR, aws-sdk-secretsmanager/aws-config carried their default features (which included a TLS transport), so a build that only enabled fips still had a working HTTP client. Now that both SDK deps are default-features = false, fips = ["rustls/fips"] only turns on the fips feature of the standalone rustls 0.23 crate — it does not enable default-https-client, rt-tokio, or the SDK rustls feature.

So a downstream build like:

aws_secretsmanager_caching = { version = "2", default-features = false, features = ["fips"] }

compiles but has no SDK HTTP client and will fail at runtime when it tries to make a call. Two questions worth confirming:

  1. Does the standalone rustls 0.23 crate's FIPS provider actually get wired into the SDK's TLS path? Per the PR description the SDK rustls feature resolves the legacy aws-smithy-runtime/tls-rustls (rustls 0.21) stack — a different major version — so it's not obvious rustls/fips on 0.23 affects the transport the SDK uses.
  2. If fips is meant to be usable standalone, consider making it self-sufficient, e.g. fips = ["rustls/fips", "rustls"] (and/or "default-https-client"), plus a README note that FIPS requires the legacy rustls feature.

Comment thread aws_secretsmanager_caching/Cargo.toml
Comment thread aws_secretsmanager_caching/README.md
@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 7, 2026
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 7, 2026
@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 8, 2026

Copy link
Copy Markdown

Claude finished @reyhankoyun's task in 3m 10s —— View job


Re-review: PR #197 — Allow selecting AWS SDK TLS features

  • Read PR diff against origin/main
  • Verified default-feature behavior is preserved
  • Analyzed the fips feature wiring against crate source
  • Checked README example + package version
  • Posted review feedback

Thanks @dertin. The overall shape is still good: forwarding crate features to the SDK features and keeping default equivalent to the previous implicit SDK defaults is the right approach, and it's backward-compatible for existing users. Since the last review only Cargo.lock changed (merge from main), so the three points below are still open — I'm re-stating them with the most important first.

Findings

1. fips is now a no-op / footgun (blocking)aws_secretsmanager_caching/Cargo.toml:21

This is the item to resolve before merge. Two compounding problems:

  • The rustls crate is not referenced anywhere in this crate's source (I searched src/ and benches/ — no use rustls, no CryptoProvider::install_default, nothing). It's a direct dependency purely so fips = ["rustls/fips"] can toggle the FIPS provider on the standalone rustls 0.23 crate. But nothing installs that provider as the process default, and — as you noted in the PR description — the SDK rustls feature resolves the legacy aws-smithy-runtime/tls-rustls (rustls 0.21) path. So enabling fips toggles FIPS on a 0.23 crate that never touches the transport the SDK actually uses. It compiles, but it's unlikely to produce a FIPS-validated TLS path at runtime.
  • Now that both SDK deps are default-features = false, a build like default-features = false, features = ["fips"] compiles with no SDK HTTP client at allfips no longer implies default-https-client/rt-tokio/rustls, so it will fail at runtime on the first API call.

At minimum, fips should be made self-sufficient in the transport it needs, e.g.:

fips = ["rustls/fips", "rustls", "default-https-client"]

…but that only helps if rustls/fips genuinely reaches the SDK's TLS path. Given the 0.23-vs-0.21 mismatch, please verify what fips actually does end-to-end (cargo tree -e features -i rustls and confirm which rustls version the SDK transport links). If it can't be wired meaningfully, it may be clearer to document fips as unsupported alongside default-features = false rather than ship a flag that silently does nothing.

2. Package version not bumped + README pins version = "2"aws_secretsmanager_caching/Cargo.toml:3, aws_secretsmanager_caching/README.md:33

version is still 2.1.0. A feature-adding change should bump the minor version so downstreams can require the release that introduces these features. Relatedly, the README opt-out example uses version = "2" with default-features = false — a caret "2" can resolve to an older 2.x that lacks these features, leaving the user with no SDK features enabled and no transport. Pin the example to the minimum version that ships this wiring (e.g. "2.2", matching the bump).

3. Naming collision: rustls feature vs rustls crate (non-blocking)aws_secretsmanager_caching/Cargo.toml:19

The new rustls feature (= ["aws-sdk-secretsmanager/rustls"]) shares its name with the rustls crate dependency (rustls = "0.23"). Cargo allows it, but reading fips = ["rustls/fips"] (crate) next to rustls = [...] (feature) is confusing. A distinct name like sdk-rustls or legacy-tls would read better. Ties into #1 — if fips's relationship to the SDK TLS path is clarified, renaming makes the intent obvious.

Looks good

  • Default behavior preserved — default = ["rustls", "default-https-client", "rt-tokio", "credentials-process", "sso"] mirrors the previous implicit SDK defaults.
  • dev-dependencies correctly re-add default-https-client + rt-tokio alongside test-util, so the test suite keeps a transport.
  • README documentation of the opt-out path is a helpful addition.

Suggested follow-up

  • Add a CI matrix entry that builds --no-default-features --features default-https-client,rt-tokio (and one for fips) so this feature wiring can't regress silently.

Net: clean, backward-compatible change. The fips interaction (#1) is the one thing I'd want resolved or explicitly documented before merge.
· fix/caching-client-tls-features

…t-https

Wire fips through default-https and optional rustls-crypto so consumers with
default-features = false get Smithy's rustls 0.23 transport without enabling
aws-sdk-secretsmanager/rustls. Bump the crate to 2.2.0 and document the TLS
feature paths in the README.
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