Skip to content

feat(cubestore): support AWS Web Identity Token File in S3RemoteFs#10687

Open
bsod90 wants to merge 1 commit intomasterfrom
support_oidc_federation_for_s3
Open

feat(cubestore): support AWS Web Identity Token File in S3RemoteFs#10687
bsod90 wants to merge 1 commit intomasterfrom
support_oidc_federation_for_s3

Conversation

@bsod90
Copy link
Copy Markdown
Member

@bsod90 bsod90 commented Apr 14, 2026

When CUBESTORE_AWS_ACCESS_KEY_ID is not set and AWS_WEB_IDENTITY_TOKEN_FILE is present, the credential provider chain falls through to STS AssumeRoleWithWebIdentity — reading the JWT from the token file and exchanging it for temporary session credentials.

The refresh loop now polls the token file mtime every 30 seconds in web identity mode (vs 3-hour default for static credentials). Credentials are only re-exchanged when the file actually changes, keeping STS calls minimal.

Check List

  • Tests have been run in packages where changes have been made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

@bsod90 bsod90 requested a review from a team as a code owner April 14, 2026 19:18
@github-actions github-actions bot added cube store Issues relating to Cube Store rust Pull requests that update Rust code labels Apr 14, 2026
@claude
Copy link
Copy Markdown

claude bot commented Apr 14, 2026

Claude finished @bsod90's task in 1m 46s —— View job


Code Review: PR #10687

  • Read the PR diff and changed file
  • Analyze code quality, security, and correctness
  • Review error handling and edge cases
  • Check for test coverage
  • Post comprehensive review

Summary

This PR adds support for AWS Web Identity Token File (OIDC federation) in the CubeStore S3 remote filesystem. This is useful for Kubernetes/EKS environments where pods use IRSA (IAM Roles for Service Accounts) instead of static credentials. The approach is sound — it detects the web identity mode, polls the token file for changes, and only re-exchanges credentials when the file is updated.

Issues Found

1. Bug: token_file gate can break static credential refresh (Medium)

File: rust/cubestore/cubestore/src/remotefs/s3.rs:136-145

The mtime-check block at line 136 is gated on token_file.is_some(), not is_web_identity. If someone has both CUBESTORE_AWS_ACCESS_KEY_ID and AWS_WEB_IDENTITY_TOKEN_FILE set in the environment (e.g., during a migration), is_web_identity is false but token_file is Some(...). This means the mtime check activates in the static credentials path and silently skips refreshes unless the token file happens to change.

See inline comment with suggested fix.

2. Hardcoded 30s interval ignores CUBESTORE_AWS_CREDS_REFRESH_EVERY_MINS (Minor)

File: rust/cubestore/cubestore/src/remotefs/s3.rs:105-108

In web identity mode, the existing CUBESTORE_AWS_CREDS_REFRESH_EVERY_MINS env var is silently ignored. Operators who set this expecting it to control all S3 credential refresh behavior will be surprised.

See inline comment.

What Looks Good

  • Design approach: Detecting web identity mode via access_key.is_none() && token_file.is_some() is a clean heuristic that avoids adding new configuration knobs.
  • Mtime-based polling: Only re-exchanging credentials when the token file actually changes keeps STS API calls minimal — good for rate limits and latency.
  • Error handling: The existing Credentials::new / Bucket::new error paths (log + continue) handle transient STS failures gracefully.
  • Logging: The enhanced debug log with web_identity={} and the info! on token file change are helpful for production debugging.
  • Code style: Clean, well-commented, and follows existing patterns in the file.

Testing

There are no unit tests for spawn_creds_refresh_loop — this is pre-existing (the file has no #[cfg(test)] module at all), not introduced by this PR. Integration testing of this feature would require mocking STS or running against a real OIDC provider, which may be impractical in CI. At minimum, consider a unit test that verifies the is_web_identity flag is correctly derived from the combination of access_key and token_file values.

Security

No concerns. The PR correctly avoids logging token file contents or credentials. The web identity token is read by the rust-s3 credential chain, not by this code directly.

Comment on lines +136 to +145
if let Some(ref file) = token_file {
let current_modified = std::fs::metadata(file)
.ok()
.and_then(|m| m.modified().ok());
if current_modified == last_modified {
continue;
}
last_modified = current_modified;
info!("Web identity token file changed, refreshing S3 credentials");
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: This block gates on token_file.is_some(), but token_file can be Some even when is_web_identity is false — specifically when a user sets both CUBESTORE_AWS_ACCESS_KEY_ID and AWS_WEB_IDENTITY_TOKEN_FILE in the environment (e.g., during a migration or in a misconfigured pod).

In that scenario, is_web_identity = false, so refresh_every uses the 3-hour default, but this block still activates and skips credential refreshes unless the token file mtime changes. That effectively breaks the static-credential refresh path.

The fix is to gate on is_web_identity instead:

Suggested change
if let Some(ref file) = token_file {
let current_modified = std::fs::metadata(file)
.ok()
.and_then(|m| m.modified().ok());
if current_modified == last_modified {
continue;
}
last_modified = current_modified;
info!("Web identity token file changed, refreshing S3 credentials");
}
// In web identity mode, only refresh when the token file changed.
if is_web_identity {
if let Some(ref file) = token_file {
let current_modified = std::fs::metadata(file)
.ok()
.and_then(|m| m.modified().ok());
if current_modified == last_modified {
continue;
}
last_modified = current_modified;
info!("Web identity token file changed, refreshing S3 credentials");
}
}

let is_web_identity = access_key.is_none() && token_file.is_some();

let refresh_every = if is_web_identity {
Duration::from_secs(30)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: The 30-second poll interval is reasonable for web identity, but it's hardcoded and ignores CUBESTORE_AWS_CREDS_REFRESH_EVERY_MINS entirely. Consider either:

  1. Using CUBESTORE_AWS_CREDS_REFRESH_EVERY_MINS as an override when set (falling back to 30s), or
  2. Adding a brief comment noting that the env var is intentionally ignored in web identity mode.

This prevents confusion when an operator sets the env var expecting it to control all S3 credential refresh behavior.

@codecov
Copy link
Copy Markdown

codecov bot commented Apr 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 58.01%. Comparing base (43b3fd5) to head (635d2ed).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #10687   +/-   ##
=======================================
  Coverage   58.01%   58.01%           
=======================================
  Files         215      215           
  Lines       16648    16648           
  Branches     3347     3347           
=======================================
  Hits         9659     9659           
  Misses       6499     6499           
  Partials      490      490           
Flag Coverage Δ
cube-backend 58.01% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 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.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

When CUBESTORE_AWS_ACCESS_KEY_ID is not set and AWS_WEB_IDENTITY_TOKEN_FILE
is present, the credential provider chain falls through to STS
AssumeRoleWithWebIdentity — reading the JWT from the token file and
exchanging it for temporary session credentials.

The refresh loop now polls the token file mtime every 30 seconds in web
identity mode (vs 3-hour default for static credentials). Credentials are
only re-exchanged when the file actually changes, keeping STS calls minimal.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cube store Issues relating to Cube Store rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant