Conversation
c5f8e58 to
55da8ff
Compare
061f8a0 to
fc69402
Compare
fc69402 to
0afedb7
Compare
0afedb7 to
f8c2538
Compare
| aws-sdk-s3 = { version = "1.106.0", default-features = false, features = ["rt-tokio"] } | ||
| aws-sdk-sts = { version = "1.96.0", default-features = false, features = ["rt-tokio"] } | ||
| aws-config = { version = "1.8.12", default-features = false, features = ["rt-tokio", "credentials-process"] } | ||
| aws-config = { version = "1.8.12", default-features = false, features = ["rt-tokio", "credentials-process", "sso"] } |
There was a problem hiding this comment.
Drop sso. Do not put ring back into the FIPS artifact for IAM Identity Center.
WIF only needs the default chain for IMDS / IRSA / env keys / instance role. Stage transfers never use this chain. Platform detection is opt-in telemetry. UD Gherkins (workload_identity.feature) cover GetCallerIdentity and GetWebIdentityToken against a cloud identity, not aws sso login.
JDBC and .NET do not ship an SSO/Identity Center module. Python/Go/Node only get it as a boto/SDK default-chain side effect, and even Python's copy says "IAM role attached."
#1345 already had default-features = false without sso. Restoring it is a compatibility extra, not a WIF contract, and it is the remaining ring source this PR is supposed to be shrinking. Omit the feature; record Identity Center as unsupported on this core (same as JDBC).
There was a problem hiding this comment.
Done -- sso is off (e8679e288). You were right on every checkable point, and I verified each before reversing the earlier decision rather than taking it on argument:
#1345had nosso.git show upstream/main:sf_core/Cargo.tomlgivesaws-config = { ..., features = ["rt-tokio", "credentials-process"] }. So the previous revision was re-adding it, not preserving it. That reframes it exactly as you say: a compatibility extra, not a WIF contract.- It was the last ring source.
cargo tree -p sf_core -e normal -i ringnow prints nothing, andodbc/jdbc_bridge/python_bridge/nodejs_bridgeall report zero. Ring keeps aCargo.lockentry only viarcgenunder[dev-dependencies]for test certificate generation -- worth knowing before someone greps the lock and concludes the claim is false. - The WIF path does not need it.
workload_identity/aws.rsresolves throughaws_config::defaults(...), and that chain still covers env keys, IMDS, IRSA / web-identity, instance roles andcredentials-processwithoutsso. Your framing is the clarifying one: WIF is discovering a workload identity, not a human's cachedaws sso loginsession.platform_detection/aws.rspasses no provider at all, ands3_transfer.rssupplies explicit Snowflake-issued credentials, so neither is affected.
Identity Center is now recorded as unsupported on this core, matching JDBC and .NET, in the comment beside the dependency. The old "please do not fix this by dropping sso" note is gone so it will not misdirect the next reader.
For the record: this reverses my earlier call to keep sso, and that call was wrong. It rested on the fact that dropping sso removes Identity Center from the default chain -- true on its own, but I had not put it against the fact that main was already shipping without it, which is what makes the trade one-sided. Settled now, not provisional.
| # The name stays because the *claim* is not yet complete: | ||
| # - SigV4 request signing computes HMAC-SHA256 outside the module: | ||
| # `aws-sigv4` uses RustCrypto unconditionally, and `aws-sdk-s3` enables | ||
| # `sigv4a` by default, which links ring. That is every stage transfer, not |
There was a problem hiding this comment.
This commit turns sigv4a off. The fips-tls comment still says aws-sdk-s3 enables sigv4a and that is why ring is linked. After dropping sso, ring from this graph should be gone; the F7 leftover is SigV4 HMAC via RustCrypto on every stage transfer. Update that bullet.
There was a problem hiding this comment.
Fixed in e8679e288. The bullet no longer mentions sigv4a or ring:
# - SigV4 request signing computes HMAC-SHA256 outside the module:
# `aws-sigv4` uses RustCrypto unconditionally. That is every stage
# transfer, not an edge case; see F7. `ring` itself is no longer linked --
# its only two sources, `sigv4a` and `sso`, are both off (see the AWS SDK
# block below) -- so this bullet is now about RustCrypto alone.
Your diagnosis caught two problems in one sentence, not one: it named sigv4a as enabled when this very commit turns it off, and it pointed at ring as the residue when after dropping sso there is none. The RustCrypto HMAC on every stage transfer is the whole of what remains on that line.
f8c2538 to
d456874
Compare
d456874 to
e8679e2
Compare
zeroshade
changed the title
e8679e2 to
2ec7642
Compare
2ec7642 to
1b5cad6
Compare
1b5cad6 to
48c2c06
Compare
| # TLS-only in practice. | ||
| # | ||
| # The name stays because the *claim* is not yet complete: | ||
| # - SigV4 request signing computes HMAC-SHA256 outside the module: |
There was a problem hiding this comment.
This runs on every S3 stage request and blocks the fips claim. Can we move SigV4 HMAC-SHA256 into AWS-LC?
Please investigate the available paths:
- an upstream crypto-provider hook in
aws-sigv4; - a patched or forked
aws-sigv4using AWS-LC; - replacing the SDK signing stage with an AWS-LC-backed signer.
If none belongs in this PR, open an owned follow-up and link it here. Any replacement needs AWS SigV4 test-vector coverage and stage-transfer coverage.
There was a problem hiding this comment.
This is getting pushed upstream via http://github.com/smithy-lang/smithy-rs/pull/4874#discussion_r4107787573 if we can wait for that to get merged (I had commented on there to ensure the dependency floor for aws-lc-fips was lowered to the 3.x module line). If we can't wait for that to get merged and released we can fork and replicate that until it gets merged.
There was a problem hiding this comment.
nice. let's wati for the upstream. maybe leave a TODO here so that we will remember to update the dependency once it's relased
48c2c06 to
528247a
Compare
528247a to
48d7f9a
Compare
48d7f9a to
f8ba28e
Compare
|
Restacked #1348 onto |
Reshaped against main. The snowflakedb#1345 import already set `default-features = false` on the AWS SDK crates, which removed the legacy ring-backed TLS stack (an older rustls 0.21 on hyper 0.14) -- the bulk of what this change originally did. What remains is two feature choices, both of which re-link `ring`. `sigv4a` goes. It pulls `aws-runtime/sigv4a` -> `aws-sigv4/sigv4a` -> `dep:ring`, and it only signs S3 multi-region access point ARNs. Snowflake stages are ordinary region-scoped buckets, so nothing here can reach that signer. This does not disable request signing, and the naming invites exactly that misreading. SigV4 and SigV4a are separate algorithms in separate modules: plain SigV4 is `aws-sigv4::sign::v4`, carries no `#[cfg]` on this feature, and still signs every S3 request. Only the asymmetric variant (`sign::v4a`, and the `SigningParams::V4a` arm) is gated off. Verified: `aws-sigv4` remains in the dependency graph, SigV4a's own deps (`crypto-bigint`, `base16ct`) drop out, and the S3/WIF/transfer tests pass. `sso` goes too, which is the change from the previous revision of this commit. It hard-requires `dep:ring` and cannot be ported -- it is the IAM Identity Center *credential provider*, not a transport, so there is no aws-lc-backed equivalent to swap in. It can only be carried or dropped. Dropping it removes Identity Center from the SDK's default credential chain. That is a support decision, not a regression in what the driver needs: the chain still resolves env keys, IMDS, IRSA / web-identity tokens, instance roles and `credentials-process`, which covers both ambient call sites. `workload_identity/aws.rs` is discovering a *workload* identity rather than a human's cached `aws sso login` session, and `telemetry/platform_detection/aws.rs` passes no provider at all. `file_manager/s3_transfer.rs` supplies explicit Snowflake-issued credentials and is unaffected either way. Identity Center is therefore unsupported on this core, matching JDBC and .NET. With both features off, `ring` is gone from the shipped graph: `cargo tree -p sf_core -e normal -i ring` finds nothing, and `odbc`, `jdbc_bridge`, `python_bridge` and `nodejs_bridge` all report the same. It retains a `Cargo.lock` entry only through `rcgen` under `[dev-dependencies]` for test certificate generation, which no artifact links. Still not fixable here, and now the only item on this line: `aws-sigv4` computes plain SigV4's HMAC-SHA256 through RustCrypto unconditionally, on every stage transfer. The `fips-tls` feature comment is updated to say that rather than pointing at ring, which is no longer linked. 2313 tests pass by default, 2314 with `--features fips-tls`.
f8ba28e to
6628e5f
Compare
|
Restacked #1348 at |
![snowflake-security-bot[bot]](https://avatars.githubusercontent.com/u/140554735?s=60&v=4){kind=small}
…hipped dependency graph Imported from #1348. Original PR body: > **Stacked on #1347** (which sits on #1346, which sits on #1345). The diff > against `main` therefore carries those PRs' 15 commits — **review only the > last commit here**, `NO-SNOW: FIPS F7: drop the AWS SDK's ring-backed TLS > stack`. GitHub cannot use a cross-repo base branch, so this targets `main` > until the PRs below it merge. ## What Partially closes **F7**: ring is linked into the FIPS artifact. Measured on a `--features fips-tls` lib-test binary built on this tree: | | before | after | | --- | --- | --- | | ring symbols | 221 | **175** | | rustls versions | 0.21 **and** 0.23 | **0.23 only** | | hyper 0.14 | linked | **gone** | aws-lc-fips symbols 1574, non-FIPS aws-lc still 0. ## The trap, for anyone auditing this later The **`rustls` feature** on `aws-sdk-s3`/`aws-sdk-sts` is not the rustls the rest of the driver uses. It expands to `aws-smithy-runtime/tls-rustls` → `aws-smithy-http-client/legacy-rustls-ring`, linking hyper 0.14 and a *second*, older rustls 0.21 built on ring. `default-https-client` is the opposite — it maps to `rustls-aws-lc`, is aws-lc-backed, and is **deliberately kept**, because it still supplies a connector to paths that build an `SdkConfig` without an explicit client (including one S3 test that does exactly that). The names invite precisely the wrong conclusion. Nothing ever used the legacy connector: every `aws_config::defaults(...)` call site hands the SDK its own reqwest-backed `HttpClient` via `tls::aws_http_client`. It was linked but dead. `sigv4a` goes with the defaults — it pulls `aws-runtime/sigv4a` → `aws-sigv4/sigv4a` → `dep:ring` and only signs S3 multi-region access point ARNs. Snowflake stages are ordinary region-scoped buckets, so nothing here can reach that signer. ## The residual 175 symbols are a decision, not an oversight Please read the comment on the `aws-config` line before suggesting `default-features = false` there. Ring's last source is `aws-config/sso`, which hard-requires `dep:ring`. Dropping it **does** reach zero — verified, and `aws-sdk-sso`/`ssooidc` fall out too — but it also removes AWS IAM Identity Center credentials from the SDK's default credential chain, and two of the three call sites resolve through that chain on purpose: - `workload_identity/aws.rs` — credentials are `Option`; ambient discovery is the entire point of the WIF path - `telemetry/platform_detection/aws.rs` — passes no `credentials_provider` at all Only `file_manager/s3_transfer.rs` supplies explicit Snowflake-issued credentials and would be unaffected. So the trade is: ring in the FIPS artifact, or no AWS SSO credential support in *any* artifact. Cargo features are additive and cannot be negated, so it cannot be split per-build without the named-profile machinery the design doc rejects. It also lands hardest on WORKLOAD_IDENTITY, the login path most likely to be used in the deployments that want FIPS in the first place. **Decision taken: keep SSO, record ring as a known deviation.** ## Also not fixable from this repo `aws-sigv4` computes plain SigV4's HMAC-SHA256 through RustCrypto **unconditionally** (`aws-sigv4/src/sign/v4.rs`), so the scope is every S3 stage transfer, not just WIF. That needs an upstream ask or a written deviation. ## Verification - 2199 tests pass by default, 2200 with `--features fips-tls` - fmt clean; clippy unchanged from baseline; all four bindings `cargo check` - root `Cargo.lock` and `python/Cargo.lock.sdist` both refreshed and verified under `--locked` --- Internal CI validates this change before merge. On merge to main the outbound mirror will push the commit back to the public repo; close the original mirror PR with a link to the mirrored commit. Co-authored-by: Matt Topol <zotthewizard@gmail.com> GitOrigin-RevId: 8606cf9f3426fa5a0cddc4e3cf13428e4ef1050a
|
Merged in the private repo. |
…the module Imported from #1349. Original PR body: > **Stacked on #1348** (→ #1347 → #1346). The diff against `main` carries those > PRs' 17 commits — **review only the last commit here**, > `NO-SNOW: FIPS F8: draw OAuth randomness from the module`. > GitHub cannot use a cross-repo base branch, so this targets `main` until the > PRs below it merge. ## What Closes **F8**. The PKCE code verifier, the CSRF `state` and the DPoP proof `jti` are the values in the OAuth flows whose security rests on being unpredictable, and none of them came from the module: the first two from `oauth2`'s generators, which draw on `rand`'s thread RNG, and `jti` from `Uuid::new_v4()`, which draws on `getrandom`. That is a fine default for a general-purpose client and the wrong source here: under `fips-tls` the claim is that security-relevant randomness comes from the validated module. All three now come from AWS-LC's DRBG through a new `oauth::random::token_b64url`, which reproduces the shape `oauth2` produced — N random bytes, base64url without padding — so only the *source* changes. Values on the wire stay indistinguishable, which is what keeps RFC 7636 compliance and IdP interoperability from depending on this swap. A DRBG failure fails the login via a new `OAuthError::RandomGeneration` rather than falling back to another source, which would void the claim exactly when it mattered. `jti` fails the proof the same way. ## `jti` was half of a proof that was otherwise on-module Raised in review: `jti` is the DPoP proof's replay identifier, and it sat directly beside a signature the same function already took from `SystemRandom` — so every proof mixed module and non-module randomness. RFC 9449 only requires the value be unique, so the encoding is free; 16 bytes matches a UUIDv4's 122 bits of entropy. The existing test only asserted the claim was non-empty, which would have passed against a hardcoded constant. Since distinctness is the property replay protection actually rests on, a test now asserts two proofs for the same request carry different `jti`. ## The part worth reviewing carefully: `set_pkce_challenge` is bypassed The challenge digest moves to AWS-LC too. `oauth2` will only build a `PkceCodeChallenge` around a digest it computed itself with RustCrypto (`types.rs:486`), so the flow now appends `code_challenge` and `code_challenge_method` itself — the same two parameters, with the same values, that `set_pkce_challenge` emits (`code.rs:163-165`). That bypass is the one part of this that could break silently, so the existing end-to-end authorize-URL test now asserts the binding it never checked: that `code_challenge` is the S256 digest of the `code_verifier` presented at the token endpoint. Its previous assertions only checked the parameter was non-empty, and would still have passed if the two were unrelated, or if the digest were taken over the raw DRBG bytes rather than the encoded verifier — both of which look right and fail at the IdP. **I confirmed the new assertion is not vacuous** by deliberately hashing the wrong input and watching it fail. ## `pkce.rs` stops being dead scaffolding It was `#[allow(dead_code)]`, duplicating the inline implementation. Rather than fix the RNG twice, or leave a `rand`-based generator sitting next to a DRBG-based one for someone to wire up later, it becomes the single implementation and the allow comes off. ## The `fips-tls` comment is now an inventory Also raised in review: it read as a running changelog, and its "no longer for the original reason" opening was only legible next to deleted OpenSSL sentences that nobody has after merge. It now states what the feature selects and the one reason the name is still `fips-tls` — SigV4's HMAC-SHA256 on RustCrypto — and records that fixing that is AWS SDK work rather than a further commit in this stack, so no one assumes another PR is coming. ## What `rand` is still used for It stays in the dependency graph, for retry and refresh jitter (`http/retry.rs`, `crl/cache.rs`, `token_cache/file_cache.rs`) and a non-secret FFI handle tag (`handle_manager.rs`). None of those are security-relevant randomness. Recording that classification at the call sites, so a dependency audit does not read them as a bypass, belongs with the cargo-deny/SBOM work in Phase 5. ## Verification - 2317 tests pass by default, 2318 with `--features fips-tls` - fmt clean; clippy unchanged from baseline; all four bindings `cargo check` - all three lockfiles accept `--locked` --- Internal CI validates this change before merge. On merge to main the outbound mirror will push the commit back to the public repo; close the original mirror PR with a link to the mirrored commit. Co-authored-by: Matt Topol <zotthewizard@gmail.com> GitOrigin-RevId: 72e07f202d04c6113a2a70532ea74ad0ffa7e938
What
Partially closes F7: ring is linked into the FIPS artifact. Measured on a
--features fips-tlslib-test binary built on this tree:aws-lc-fips symbols 1574, non-FIPS aws-lc still 0.
The trap, for anyone auditing this later
The
rustlsfeature onaws-sdk-s3/aws-sdk-stsis not the rustls therest of the driver uses. It expands to
aws-smithy-runtime/tls-rustls→aws-smithy-http-client/legacy-rustls-ring, linking hyper 0.14 and a second,older rustls 0.21 built on ring.
default-https-clientis the opposite — it maps torustls-aws-lc, isaws-lc-backed, and is deliberately kept, because it still supplies a
connector to paths that build an
SdkConfigwithout an explicit client(including one S3 test that does exactly that). The names invite precisely the
wrong conclusion.
Nothing ever used the legacy connector: every
aws_config::defaults(...)callsite hands the SDK its own reqwest-backed
HttpClientviatls::aws_http_client. It was linked but dead.sigv4agoes with the defaults — it pullsaws-runtime/sigv4a→aws-sigv4/sigv4a→dep:ringand only signs S3 multi-region access pointARNs. Snowflake stages are ordinary region-scoped buckets, so nothing here can
reach that signer.
The residual 175 symbols are a decision, not an oversight
Please read the comment on the
aws-configline before suggestingdefault-features = falsethere. Ring's last source isaws-config/sso, whichhard-requires
dep:ring. Dropping it does reach zero — verified, andaws-sdk-sso/ssooidcfall out too — but it also removes AWS IAM IdentityCenter credentials from the SDK's default credential chain, and two of the three
call sites resolve through that chain on purpose:
workload_identity/aws.rs— credentials areOption; ambient discovery isthe entire point of the WIF path
telemetry/platform_detection/aws.rs— passes nocredentials_providerat allOnly
file_manager/s3_transfer.rssupplies explicit Snowflake-issuedcredentials and would be unaffected.
So the trade is: ring in the FIPS artifact, or no AWS SSO credential support in
any artifact. Cargo features are additive and cannot be negated, so it cannot
be split per-build without the named-profile machinery the design doc rejects.
It also lands hardest on WORKLOAD_IDENTITY, the login path most likely to be
used in the deployments that want FIPS in the first place.
Decision taken: keep SSO, record ring as a known deviation.
Also not fixable from this repo
aws-sigv4computes plain SigV4's HMAC-SHA256 through RustCryptounconditionally (
aws-sigv4/src/sign/v4.rs), so the scope is every S3 stagetransfer, not just WIF. That needs an upstream ask or a written deviation.
Verification
--features fips-tlscargo checkCargo.lockandpython/Cargo.lock.sdistboth refreshed and verifiedunder
--locked