fix: remove unrestrictive principal * from KMS policy tests. #712
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| "Statement": [ | ||
| {{ | ||
| "Sid": "Enable IAM User Permissions", | ||
| "Effect": "Allow", | ||
| "Principal": {{ | ||
| "AWS": "*" | ||
| "AWS": {principal} |
There was a problem hiding this comment.
why not grant to account root instead of a role? that way any entity in the account should get these permissions.
There was a problem hiding this comment.
Granting to the account was my first attempt, unfortunately KMS permissions don't propagate to the assumed role when we do that.
There was a problem hiding this comment.
Btw, the account root is already one of the principals in the list.
|
|
||
| def _create_kms_key(kms_client, | ||
| account_id, | ||
| role_arn=None, |
There was a problem hiding this comment.
why accept a role name and a role arn?
There was a problem hiding this comment.
There are three principals that need to be created: the account root, the role that is running the tests, and the role that will be used by SageMaker. Example:
"Principal": {
"AWS": [
"arn:aws:iam::142577830533:role/SageMakerRole",
"arn:aws:sts::142577830533:assumed-role/DevBuildStackPDX-PullRequestBuildRole4B40B95C-G4AE4UBWNU2K/AWSCodeBuild-fe90a41f-9006-4ccc-918c-437c0f5b26d2",
"arn:aws:iam::142577830533:root"
]
},
| role_arn=None, | ||
| sagemaker_role='SageMakerRole'): | ||
| if role_arn: | ||
| principal = PRINCIPAL_TEMPLATE.format(account_id=account_id, |
There was a problem hiding this comment.
is the idea here a policy that allows the root account, some role arn, plus another role constructed from the sagemaker_role name? why not simplify that and just require test environments be set up correctly?
There was a problem hiding this comment.
Talked with @jesterhazy offline. We didn't find an easier way to simplify this test without breaking running tests outside the CI.
|
don't forget to change the PR title to include the appropriate prefix with our new release process (#705 was merged) |
mvsusp
changed the title
…n-sdk into mvs-fix-principal
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
tests/integ/kms_utils.py
Outdated
| from botocore import exceptions | ||
|
|
||
| KEY_ALIAS = "SageMakerIntegTestKmsKey" | ||
| PRINCIPAL_TEMPLATE = '["{account_id}", "{role_arn}", "arn:aws:iam::{account_id}:role/{' \ | ||
| 'sagemaker_role}"] ' |
There was a problem hiding this comment.
seems like an awkward way to break the line. maybe instead do it right before the role arn?
tests/integ/test_tf_script_mode.py
Outdated
| def test_server_side_encryption(sagemaker_session): | ||
|
|
||
| bucket_with_kms, kms_key = kms_utils.get_or_create_bucket_with_encryption(sagemaker_session.boto_session) | ||
| bucket_with_kms, kms_key = kms_utils.get_or_create_bucket_with_encryption(sagemaker_session.boto_session, | ||
| 'SageMakerRole') |
There was a problem hiding this comment.
I'm starting to think this role should be a constant somewhere
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Co-authored-by: Zhenshan Jin <[email protected]> Co-authored-by: Judy Heflin <[email protected]> Co-authored-by: Zhenshan Jin <[email protected]> @Zhenshan-Jin feature: add create model card (aws#663) @Zhenshan-Jin feature: add read update delete model card (aws#671) @Zhenshan-Jin feature: model overview and training details autodiscovery (aws#674) @Zhenshan-Jin feature: add metrics for training and evaluation details (aws#680) @Zhenshan-Jin feature: model card export & list model card version history (aws#688) @Zhenshan-Jin fix: batch update model card and bugs fixes (aws#712) @Zhenshan-Jin @judyheflin documentation: Amazon SageMaker Model Cards Python SDK documentation (aws#716)
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.