fix(iam): managed-policy can again be granted actions on resources #33115
Conversation
github-actions
bot
added
the
beginning-contributor
rix0rrr
added
pr-linter/exempt-codecov
QuantumNeuralCoder
changed the title
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
added
pr/needs-community-review
…mment @stylistic/spaced-comment"
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33115 +/- ##
=======================================
Coverage 82.21% 82.21%
=======================================
Files 119 119
Lines 6876 6876
Branches 1162 1162
=======================================
Hits 5653 5653
Misses 1120 1120
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
aws-cdk-automation
dismissed
their stale review
Dismissing outdated PRLinter review.
aws-cdk-automation
removed
the
pr/needs-community-review
| @@ -336,7 +336,8 @@ class ManagedPolicyGrantPrincipal implements IPrincipal { | |||
| // This property is referenced to add policy statements as a resource-based policy. | |||
| // We should fail because a managed policy cannot be used as a principal of a policy document. | |||
| // cf. https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_principal.html#Principal_specifying | |||
| throw new Error(`Cannot use a ManagedPolicy '${this._managedPolicy.node.path}' as the 'Principal' or 'NotPrincipal' in an IAM Policy`); | |||
| // I32795: Restoring a previous feature where a grant for a managed policy would generate policy document with actions as a CDK feature. This managed policy needs to be attached to a role or principal for it to be used meaningfully. | |||
| return new PrincipalPolicyFragment({}); | |||
There was a problem hiding this comment.
The reason tableV2.grantX(someManagedPolicy) started throwing error is due to this line:
Down the call path of tableV2.grantX(someManagedPolicy), addToPrincipalOrResource gets called at some point and it does the following:
- It adds permission to the principal (in this case, it adds permission to
someManagedPolicy). - It checks if the principal and resource are in different account. If so, it updates the resource based policy to allow the principal to access the resource (in this case, it would try to add the
someManagedPolicyprincipal to thetableV2resource based policy.)
In step 2, when CDK tries to assemble the correct resource based policy, that is where the error is thrown. The intention of the code seems to be that, a Policy or Managed Policy is not a principal, hence throw an error (in other words, it is not possible to allow a Policy to access a resource. You can only allow an identity, aka principal, to access a resource). Logically, this makes sense. It is also explained in the PR that added this line: #22712 (comment)
(The reason this worked for S3 Bucket is because buckets are global resources, therefore, CDK assumes a bucket is in the same account as the principal and skip step 2.)
With the above thinking, I think we should update the code in class Grant where it throws a warning if the code is trying to get the principals out of a not-real-principal (i.e. Managed Policy, Policy, Group), then skip adding resource based policy. The warning should notify customers that resource based policy will not be added (which is what customers would normally expect.)
There was a problem hiding this comment.
Crossposting analysis from the original issue. The problem is with how import resolves differently for Bucket vs TableV2. The new resources creation usecase passes correctly.
I think this will fix the other usecases where the imports are "inconsistent". Also was trying to avoid warnings.
Allowing grants on managed policy doesnt mean anything unless its attached to a principal or user or role.
There was a problem hiding this comment.
Excellent analysis, both of you! One thing that's throwing me off is that the code is literally there to create a Principal for the ManagePolicy. It feels like this comment was not a good idea after all.
There was a problem hiding this comment.
In step 2, when CDK tries to assemble the correct resource based policy, that is where the error is thrown. The intention of the code seems to be that, a Policy or Managed Policy is not a principal, hence throw an error (in other words, it is not possible to allow a Policy to access a resource.
@samson-keung Keep in mind that the Cx report is about a referenced resource (fromXyz) in the same account. It seems to me that addToPrincipalOrResource has trouble figuring out that the imported resources is in the same account and thus attempts to add something to the resource policy, which will fail.
Do you know why the OrResource part cannot figure out if the from-resource is in the same account or not?
There was a problem hiding this comment.
(The reason this worked for S3 Bucket is because buckets are global resources, therefore, CDK assumes a bucket is in the same account as the principal and skip step 2.)
Actually I think this is key. Managed Policies are also global resources. So like with S3 Buckets, this should work. However something in the code for Managed Policies makes them behave different compared to S3 Buckets.
There was a problem hiding this comment.
Keep in mind that the Cx report is about a referenced resource (fromXyz) in the same account.
Cx showed code using TableV2.fromTableArn but did not show what the actual table arn is. When tested, if the arn is from the same account, no error. That is because for same account access (when the principal and the table are in the same account), there is no need to create a resource-based policy, therefore, the code throwing error wasn't executed.
It seems to me that addToPrincipalOrResource has trouble figuring out that the imported resources is in the same account and thus attempts to add something to the resource policy, which will fail.
Do you know why the OrResource part cannot figure out if the from-resource is in the same account or not?
addToPrincipalOrResource is able to figure out the account from a TableV2.fromTableArn resource. It reads the account from the arn correctly. Whenever it cannot figure out the account (e.g. when the account value is a token), it assumes same-account scenario, hence, won't create resource-based policy, hence, no error.
Edit: If the two accounts IDs are exact same strings, or if BOTH of them are tokens, the code assumes same account. If only one of them is token, the code assumes cross account.
Actually I think this is key. Managed Policies are also global resources. So like with S3 Buckets, this should work. However something in the code for Managed Policies makes them behave different compared to S3 Buckets.
- I think there 2 misconceptions here. S3 Buckets are global in the sense that all accounts in the AWS partition shares the same namespace. (That is why a bucket arn does not have account ID.) Managed Policy is global in the sense that it lives in the account but not in particular region. (Managed Policy arn has account ID in it but not region.)
- When we say S3 Bucket works, there are more nuances to it. Because bucket arn has no account ID in it, CDK always assume it is in the same account, hence, skipping creating resource-based policy, hence, skipping the code throwing error. This means that CDK cannot detect cross account access to a S3 bucket and will not create resource-based policy, and the cross account access will not work. (I said "there are more nuances to it" because even though CDK did not throw error, the infra isn't set up correctly for the cross-account access).
I would like to point out that, conceptually, a managed policy or policy is not a "principal" because a policy is simply a set of permissions, not an actual identity like a user or role which can be considered a principal; you attach a managed policy to a principal (like a user or role) to grant them access to specific resources based on the policy's rules.
There was a problem hiding this comment.
@samson-keung and I had a chat. We agree on the general goal of the solution for addToPrincipalOrResource to detect "not-principals" principals like MangagedPolicy or Policy and bypass the Resoure policy part.
We also identified that there was an unclear case as to why the customer's implementation was triggering the "cross-account" scenario despite both Table and ManagePolicy being in the same account. (Answer: see edit in post above.)
| TableName: this.resourceStack.table.tableName, | ||
| }, | ||
| })); | ||
| } |
There was a problem hiding this comment.
The assertions seem to check CFN created the resources correctly. These should be CFN service assertions imo. In CDK integ tests, I think it is more important to check if CDK added the correct permissions that the integration need. In other words, we should invoke the infrastructure somehow to trigger runtime execution that exercises the permission added by CDK.
There was a problem hiding this comment.
Agreed. This particular check is to check for regressions in future where resource creation might be affected through a change. I can add a check with a managed policy attachment. Let me do that.
There was a problem hiding this comment.
I revisited this. The tricky part is making the integ runner assume the role of the newly created user. I have tested this using the following stub code outside of cdk integ.
import boto3
import json
from botocore.exceptions import ClientError
import os
from dotenv import load_dotenv
load_dotenv()
def get_session_for_user(access_key, secret_key):
"""
Create a boto3 session using IAM user credentials
Args:
access_key (str): AWS access key ID
secret_key (str): AWS secret access key
"""
try:
session = boto3.Session(
aws_access_key_id=access_key,
aws_secret_access_key=secret_key
)
return session
except Exception as e:
print(f"❌ Error creating session: {str(e)}")
return None
def print_caller_identity(session):
sts = session.client('sts')
caller = sts.get_caller_identity()
print(f"Currently executing as: {caller['Arn']}")
def put_object(session, bucket_name, object_key, file_path=None, data=None):
"""
Upload an object to S3 bucket
Args:
session: boto3 session
bucket_name (str): Name of the S3 bucket
object_key (str): Key (path) for the object in S3
file_path (str): Path to local file to upload (optional)
data (bytes/str): Data to upload directly (optional)
"""
try:
s3_client = session.client('s3')
if file_path:
response = s3_client.upload_file(file_path, bucket_name, object_key)
print(f"✅ Successfully uploaded file {file_path} to {bucket_name}/{object_key}")
elif data:
response = s3_client.put_object(
Bucket=bucket_name,
Key=object_key,
Body=data
)
print(f"✅ Successfully uploaded data to {bucket_name}/{object_key}")
return True
except ClientError as e:
print(f"❌ Error uploading object: {str(e)}")
return False
def get_object(session, bucket_name, object_key, download_path=None):
"""
Get an object from S3 bucket
Args:
session: boto3 session
bucket_name (str): Name of the S3 bucket
object_key (str): Key of the object to retrieve
download_path (str): Local path to save the file (optional)
"""
try:
s3_client = session.client('s3')
if download_path:
response = s3_client.download_file(bucket_name, object_key, download_path)
print(f"✅ Successfully downloaded {bucket_name}/{object_key} to {download_path}")
else:
response = s3_client.get_object(Bucket=bucket_name, Key=object_key)
data = response['Body'].read()
print(f"✅ Successfully retrieved object {bucket_name}/{object_key}")
return data
except ClientError as e:
print(f"❌ Error retrieving object: {str(e)}")
return None
def list_objects(session, bucket_name, prefix=''):
"""
List objects in an S3 bucket
Args:
session: boto3 session
bucket_name (str): Name of the S3 bucket
prefix (str): Prefix to filter objects (optional)
"""
try:
s3_client = session.client('s3')
response = s3_client.list_objects_v2(
Bucket=bucket_name,
Prefix=prefix
)
if 'Contents' in response:
print(f"\nObjects in {bucket_name}:")
print("-" * 60)
for obj in response['Contents']:
print(f"{obj['Key']:50} Size: {obj['Size']} bytes")
print("-" * 60)
return response['Contents']
else:
print(f"No objects found in {bucket_name} with prefix '{prefix}'")
return []
except ClientError as e:
print(f"❌ Error listing objects: {str(e)}")
return None
def main():
# Get credentials from environment variables
access_key = os.environ.get('AWS_ACCESS_KEY_ID')
secret_key = os.environ.get('AWS_SECRET_ACCESS_KEY')
# Create session
session = get_session_for_user(access_key, secret_key)
if not session:
print("Failed to create session")
return
# Print current identity
print_caller_identity(session)
# Example usage
bucket_name = "my-example-bucket-123456"
# List objects in bucket
print("\n--- Listing Objects ---")
list_objects(session, bucket_name)
# Upload a string
print("\n--- Uploading Text Data ---")
put_object(
session,
bucket_name,
"test/hello.txt",
data="Hello, S3!"
)
# Upload a file
print("\n--- Uploading File ---")
put_object(
session,
bucket_name,
"test/sample.txt",
file_path="./sample.txt"
)
# Get object as data
print("\n--- Getting Object Data ---")
data = get_object(session, bucket_name, "test/hello.txt")
if data:
print(f"Retrieved data: {data.decode('utf-8')}")
# Download object to file
print("\n--- Downloading Object ---")
get_object(
session,
bucket_name,
"test/sample.txt",
download_path="./downloaded_sample.txt"
)
if __name__ == "__main__":
main()and
import boto3
import json
from botocore.exceptions import ClientError
import os
from dotenv import load_dotenv
# Clear any existing AWS credentials
# os.environ.pop('AWS_ACCESS_KEY_ID', None)
# os.environ.pop('AWS_SECRET_ACCESS_KEY', None)
# os.environ.pop('AWS_SESSION_TOKEN', None)
load_dotenv()
# def get_session_for_user(user_arn):
# """
# Create a boto3 session for the specified IAM user
# Args:
# user_arn (str): ARN of the IAM user
# """
# # Extract account ID and username from ARN
# # ARN format: arn:aws:iam::account-id:user/user-name
# try:
# sts = boto3.client('sts')
# # Assume the user's role
# response = sts.assume_role(
# RoleArn=user_arn,
# RoleSessionName='DynamoDBAccess'
# )
# # Create a new session with the temporary credentials
# session = boto3.Session(
# aws_access_key_id=response['Credentials']['AccessKeyId'],
# aws_secret_access_key=response['Credentials']['SecretAccessKey'],
# aws_session_token=response['Credentials']['SessionToken']
# )
# return session
# except Exception as e:
# print(f"❌ Error creating session for user: {str(e)}")
# return None
def get_session_for_user(access_key, secret_key):
"""
Create a boto3 session using IAM user credentials
Args:
access_key (str): AWS access key ID
secret_key (str): AWS secret access key
"""
try:
session = boto3.Session(
aws_access_key_id=access_key,
aws_secret_access_key=secret_key
)
return session
except Exception as e:
print(f"❌ Error creating session: {str(e)}")
return None
def check_dynamodb_permissions(user_arn, table_arn):
# Create IAM client
iam = boto3.client('iam')
# List of actions to check
actions = [
"dynamodb:BatchGetItem",
"dynamodb:BatchWriteItem",
"dynamodb:ConditionCheckItem",
"dynamodb:DeleteItem",
"dynamodb:DescribeTable",
"dynamodb:GetItem",
"dynamodb:GetRecords",
"dynamodb:GetShardIterator",
"dynamodb:PutItem",
"dynamodb:Query",
"dynamodb:Scan",
"dynamodb:UpdateItem"
]
try:
# Simulate the policy
response = iam.simulate_principal_policy(
PolicySourceArn=user_arn,
ActionNames=actions,
ResourceArns=[table_arn]
)
# Process results
results = {}
for result in response['EvaluationResults']:
action = result['EvalActionName']
decision = result['EvalDecision']
results[action] = {
'allowed': decision == 'allowed',
'decision': decision
}
return results
except Exception as e:
print(f"Error checking permissions: {str(e)}")
return None
def print_caller_identity(session):
sts = session.client('sts')
caller = sts.get_caller_identity()
print(f"Currently executing as: {caller['Arn']}")
def print_permission_results(results):
if results:
print("\nPermission Check Results:")
print("-" * 60)
for action, result in results.items():
status = "✅ ALLOWED" if result['allowed'] else "❌ DENIED"
print(f"{action:<40} {status}")
print("-" * 60)
def put_item(session, table_arn, item_data):
"""
Put an item into DynamoDB table
Args:
table_name (str): Name of the DynamoDB table
item_data (dict): Dictionary containing the item data
"""
try:
dynamodb = session.client('dynamodb')
response = dynamodb.put_item(
TableName=table_arn,
Item=item_data
)
print(f"✅ Successfully added item to table {table_arn}")
return True
except ClientError as e:
print(f"❌ Error putting item: {str(e)}")
return False
def get_item(table_arn, key):
"""
Get an item from DynamoDB table
Args:
table_name (str): Name of the DynamoDB table
key (dict): Primary key of the item to retrieve
"""
try:
dynamodb = boto3.client('dynamodb')
response = dynamodb.get_item(
TableName=table_arn,
Key=key
)
# Check if item was found
if 'Item' in response:
print("✅ Item found:")
print(json.dumps(response['Item'], indent=2))
return response['Item']
else:
print("❌ Item not found")
return None
except ClientError as e:
print(f"❌ Error getting item: {str(e)}")
return None
def main():
# Replace these with your actual ARNs
user_arn = "arn:aws:iam::920372995415:user/same_accnt_test_user_2"
###"arn:aws:iam::924310372990:user/same_account_test_user_3"
###"arn:aws:iam::924310372990:user/cross_accnt_test_user_2"
table_arn = "arn:aws:dynamodb:us-east-1:924310372990:table/cdk_test4"
###"arn:aws:dynamodb:us-east-1:924310372990:table/cdk_test4"
###"arn:aws:dynamodb:us-east-1:920372995415:table/cdk_test2"
# Check permissions
results = check_dynamodb_permissions(user_arn, table_arn)
# Print results
if results:
print_permission_results(results)
access_key = os.environ.get('AWS_ACCESS_KEY_ID')
secret_key = os.environ.get('AWS_SECRET_ACCESS_KEY')
# Get session for the user
session = get_session_for_user(access_key, secret_key)
if not session:
print("Failed to create session for user")
return
# Example item to put (modify according to your table schema)
sample_item = {
'/id': {'S': 'user123'},
'name': {'S': 'John Doe'},
'age': {'N': '30'},
'email': {'S': 'john.doe@example.com'}
}
# Example key for getting item (modify according to your primary key)
sample_key = {
'/id': {'S': 'user123'}
}
print_caller_identity(session) # Add this before put_item
# Put item
print("\n--- Putting Item ---")
put_success = put_item(session, table_arn, sample_item)
if put_success:
# Get item
print("\n--- Getting Item ---")
get_item(table_arn, sample_key)
if __name__ == "__main__":
main()
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
…s formerly errored
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
|
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
aws-cdk-automation
added
the
closed-for-staleness
|
Comments on closed issues and PRs are hard for our team to see. |
Closes #32795.
Reason for this change
Before this change, ManagedPolicy explicitly throw and error for statements like following for TableV2. This was supported in v2.131.
Description of changes
What code changes did you make?
policyFragmentnow returns and empty PrincipalPolicyFragment. This enables the user to attach actions to policies. The generated CFN template will now contain the required actions, effects and resources blocks. It will still require the app to attach this policy to a Principal, user or role for it to work.
Have you made any important design decisions?
See above.
What AWS use cases does this change enable? To enable the use cases, which AWS service features are utilized?
Restores parity with a feature until 2.131 which allowed CDK apps to grant actions on resources to generate Policy Fragments for managed resources.
Describe any new or updated permissions being added
None
Description of how you validated changes
Yes to Both.
Integration test has been majorly refactored to do deployment checks as well in addition to the CFN checks.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license