docs: correct AWS IAM permissions in README#39
Conversation
📝 WalkthroughWalkthroughREADME.md updates the AWS IAM Permissions section: fixes a heading typo and replaces a single inline policy with a two-statement IAM policy including Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes ✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Pre-merge checks✅ Passed checks (3 passed)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
README.md (1)
186-186: Align the lead-in sentence with actual permissions used.Mention object reads and tagging to prevent confusion.
-`S3AssetDeploy` requires the following AWS IAM permissions to list, put, and delete objects in your S3 Bucket: +`S3AssetDeploy` requires the following AWS IAM permissions to list the bucket, read/write objects, and read/write object tags in your S3 bucket:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
README.md(1 hunks)
🔇 Additional comments (1)
README.md (1)
201-209: Tighten IAM: allow only s3:GetObject and s3:PutObjectCode only calls Aws::S3::Client#get_object and #put_object; no delete, tagging, or multipart S3 calls were found. Update the README policy to remove s3:DeleteObject (and no DeleteObjectTagging) and change s3:PutObject* → s3:PutObject.
- Relevant locations: lib/s3_asset_deploy/removal_manifest.rb (s3.put_object at line 44; s3.get_object at line 91), lib/s3_asset_deploy/manager.rb (put_object usage at ~175; def at 190–192). Aws::S3::Client is constructed in removal_manifest.rb (line 20), remote_asset_collector.rb (line 20), manager.rb (line 181).
| "Sid": "AllowObjectOperations", | ||
| "Effect": "Allow", | ||
| "Action": [ | ||
| "s3:GetObject", | ||
| "s3:PutObject*", | ||
| "s3:DeleteObject" | ||
| ], | ||
| "Resource": "arn:aws:s3:::#{YOUR_BUCKET}/*" | ||
| } |
There was a problem hiding this comment.
Missing s3:GetObjectTagging; wildcard PutObject grants more than needed.
Docs will still break reading removal TTLs without GetObjectTagging. Also prefer least privilege over s3:PutObject*.
Apply this diff to the object-level statement:
"Action": [
- "s3:GetObject",
- "s3:PutObject*",
- "s3:DeleteObject"
+ "s3:GetObject",
+ "s3:GetObjectTagging",
+ "s3:PutObject",
+ "s3:PutObjectTagging",
+ "s3:DeleteObject"
],Optional (if you ever remove all tags in one call), also include:
+ "s3:DeleteObjectTagging",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "Sid": "AllowObjectOperations", | |
| "Effect": "Allow", | |
| "Action": [ | |
| "s3:GetObject", | |
| "s3:PutObject*", | |
| "s3:DeleteObject" | |
| ], | |
| "Resource": "arn:aws:s3:::#{YOUR_BUCKET}/*" | |
| } | |
| "Sid": "AllowObjectOperations", | |
| "Effect": "Allow", | |
| "Action": [ | |
| "s3:GetObject", | |
| "s3:GetObjectTagging", | |
| "s3:PutObject", | |
| "s3:PutObjectTagging", | |
| "s3:DeleteObject" | |
| ], | |
| "Resource": "arn:aws:s3:::#{YOUR_BUCKET}/*" | |
| } |
🤖 Prompt for AI Agents
In README.md around lines 201-209, the object-level IAM statement is missing
s3:GetObjectTagging and uses the overly-broad s3:PutObject* wildcard; update the
Action array to include "s3:GetObjectTagging" and replace "s3:PutObject*" with
explicit actions "s3:PutObject" and "s3:PutObjectTagging" (and optionally add
"s3:DeleteObjectTagging" if you may remove all tags in one call), leaving
Resource as "arn:aws:s3:::#{YOUR_BUCKET}/*".
- Add missing s3:GetObject permission required for removal manifest - Fix typo: S3AsetDeploy -> S3AssetDeploy - Properly separate bucket-level (s3:ListBucket) and object-level permissions
capytan
force-pushed
the
docs/fix-iam-permissions
branch
from
fd61d4e to
2efaa6a
Compare
1 participant
This PR corrects the IAM permissions documentation to accurately reflect the minimum required permissions for S3AssetDeploy.
Changes
s3:GetObjectpermission required for reading removal manifestS3AsetDeploy→S3AssetDeployWhy
The current documentation mentions S3 object tagging is used for tracking removed assets, but from my investigation of the codebase, it appears that since February 2021 (commit f655480), the gem actually uses a JSON manifest file (
s3_asset_deploy/removal_manifest.json) instead. I couldn't find any references to tagging APIs in the current implementation. Users following the current docs would likely encounter permission errors withouts3:GetObject.Impact
Users following the previous documentation would encounter permission errors when the gem attempts to read the removal manifest from S3. This fix ensures users configure the minimum required permissions correctly.
s3_asset_deploy/lib/s3_asset_deploy/removal_manifest.rb
Lines 90 to 94 in 40ca094