Support IAM role / default credential chain for S3 auth#111
Conversation
Release: S3 migration, PTC bash, egress proxy, compose consolidation
docs: Add Star History section to README.md
fix: Align API contract with LibreChat client (storage_session_id, kind, resource fields)
Make s3_access_key and s3_secret_key Optional[str] (default None). When unset, boto3 falls through to its standard credential chain: env vars, ~/.aws/credentials, EC2/ECS instance metadata (IAM role). Extract S3Config.make_client() to centralise boto3 client construction. Update all four call sites to use make_client() and remove now-redundant direct boto3 imports. Fix test fixtures to patch make_client() directly. Existing behaviour with explicit keys (Garage, MinIO, etc.) is unchanged.
usnavy13
left a comment
There was a problem hiding this comment.
Thanks for adding support for IAM role / default credential-chain auth. The direction makes sense, but I think there are two issues to address before merging.
- Existing startup/health S3 validation still requires a broad IAM permission
The PR removes explicit static credentials when S3_ACCESS_KEY / S3_SECRET_KEY are unset, which lets boto3 use IAM roles or the default credential chain. However, the app still validates S3 by calling list_buckets() in ConfigValidator._validate_s3_connection(), and the health check also calls list_buckets().
That means an otherwise valid least-privilege IAM role can still fail startup or health checks if it only has access to the configured app bucket. For example, a role with permissions for code-interpreter-files can successfully HeadBucket, PutObject, and GetObject, but may not have s3:ListAllMyBuckets.
This issue already exists in the current code, but this PR makes it more important because IAM role support is the goal. I’d suggest validating the configured bucket directly with head_bucket(Bucket=settings.s3_bucket) instead of listing all buckets. Health checks should probably do the same.
Relevant locations:
src/utils/config_validator.pysrc/services/health.py
- Partial static credential config is silently ignored
S3Config.make_client() only passes credentials to boto3 when both access_key and secret_key are truthy:
if self.access_key and self.secret_key:
kwargs["aws_access_key_id"] = self.access_key
kwargs["aws_secret_access_key"] = self.secret_keyThat means if an operator accidentally sets only S3_ACCESS_KEY or only S3_SECRET_KEY, the app silently ignores the partial static config and falls back to ambient AWS credentials / IAM role credentials. That could make the app run under a different identity than intended.
This behavior is introduced by this PR. I’d suggest making it a config error when exactly one of S3_ACCESS_KEY / S3_SECRET_KEY is set. Either both should be provided for static S3-compatible auth, or both should be omitted to use boto3’s default credential chain.
Thanks for submitting this. If you can address those issues and verify it working on your end with tests, that would be helpful since we don’t yet have AWS S3 deployed for testing.
- Replace list_buckets() with head_bucket() in config_validator and health check; least-privilege IAM roles need only s3:ListBucket on the configured bucket, not s3:ListAllMyBuckets - Raise ValueError in make_client() when exactly one of S3_ACCESS_KEY / S3_SECRET_KEY is set — partial static credentials are a misconfiguration - Remove total_buckets from health check details (no longer available)
|
Thanks @usnavy13 for the quick review! I have implemented the suggested changes
I am planning on using my fork for testing in the wild while time zones are out of sync. I will provide relevant findings here :) |
e3987df to
64474ae
Compare
|
Production test results - IAM role auth working in the wild Deployed All three S3 paths confirmed working:
IAM credential chain: zero S3/credential errors across all operations. boto3 picked up the EC2 IAM role silently with no static keys configured. The Happy to provide logs if useful for the merge decision. |
Brings in 5 commits from origin/main (PR usnavy13#111/usnavy13#112 from usnavy13): - bf97b64 Support IAM role / default credential chain for S3 auth - 64474ae fix: address PR review — head_bucket and partial-creds validation Touches src/config/{__init__.py,s3.py}, src/services/{file,health,state_archival}.py, src/utils/config_validator.py, tests/{conftest.py,unit/test_file_service.py}. No overlap with runtime-only changes — clean merge expected.
What
Make
s3_access_keyands3_secret_keyoptional. When unset, boto3 falls through to its standard credential chain — env vars,~/.aws/credentials, EC2/ECS instance metadata (IAM role).Why
All four boto3 call sites currently pass credentials explicitly, which bypasses boto3's credential chain entirely. There is no way to authenticate via an EC2 instance profile or ECS task role without creating long-lived IAM user keys — against AWS best practice for hosted deployments.
Changes
src/config/s3.py,src/config/__init__.py—strwithmin_length→Optional[str] = NoneS3Config.make_client()to centralise boto3 client construction and credential-injection logicsettings.s3.make_client()— directboto3imports removedmake_client()directlyExisting behaviour with explicit keys (Garage, MinIO, etc.) is fully unchanged.