Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Restrict kms keys #524

Merged
merged 81 commits into from
Jul 6, 2023
Merged

feat: Restrict kms keys #524

merged 81 commits into from
Jul 6, 2023

Conversation

noah-paige
Copy link
Contributor

@noah-paige noah-paige commented Jun 16, 2023 •
edited
Loading

Feature or Bugfix

  • Feature

Detail

  • Restricting KMS Key Policies

This PR restricts the KMS Keys created in Data.all Environments (i.e. linked AWS Accounts), including

  • Environment KMS Keys (Used for subscriptions and Custom Resources)
  • Dataset KMS Keys (To Encrypt Data in S3)
  • Notebook KMS Keys (To Encrypt SageMaker Notebook Instance Storage Volumes)
  • SageMaker Studio KMS Keys (To Encrypt SageMaker Studio Storage Volumes)

Additional as part of this PR:

  • Consolidate Environment Keys

    • Reduce 4 KMS Keys to 1 Key to Encrypt all SQS DLQs used for Lambda Custom Resource Failed Invocations
    • Reduce 3 KMS Keys to 1 Key to Encrypt SNS and SQS Resources related to Environment Subscriptions

  • Remove Unused Custom Resources

    • Remove Glue DB No Delete Custom Resource (this is no longer used and is replaced by Glue DB Custom Resource which additionally handles deletes of Glue DBs)
    • Remove Data Location CustomResource (this is no longer used and is replaced by CfnResource used to register S3 location in LakeFormation)

  • Remove KMS Key Permissions in Environment Roles where applicable

    • Since we are restricting KMS Key Permissions we do not need to explicitly give KMS Permissions to IAM Roles for Env Group Roles where the dataset is created by data.all (if the dataset is imported we still add KMS Permissions to the IAM Role)

Relates

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

dlpzx added 30 commits June 1, 2023 14:40
@dlpzx
Treat environment_admin_group role as any other team role
eb8e56e
@dlpzx
restrict data policies for env roles
208763e
@dlpzx
Remove redshift policies of env role
ac475b9
@dlpzx
Restricted SSM and secrets policies in env team roles
9130c88
@dlpzx
Added id of CloudFormation role to avoid deletion and ensure replacin…
7d06290 
…g of the default role
@dlpzx
Finalize KMS and data permissions for env roles. Key ids instead of a…
07666ec 
…lias. athena bucket
@dlpzx
Initial permission restriction in lakeformation, service policy and d…
bb1443e 
…atabrew
@dlpzx
Divided athena, ssm and secrets in service policies, reviewed glue pe…
951d061 
…rmissions
@dlpzx
Reviewed cicd, cloudformation and glue permissions for env team role
002b00c
@dlpzx
Reviewed quicksight, sagemaker and step functions permissions for env…
1851126 
… team role
@dlpzx
Changes after testing athena, glue, cw
406b4c9
@dlpzx
Logging Lambda, Glue, create service roles, restricted step functions
2e6a968
@dlpzx
Hardened secrets and ssm parameters
45147a7
@dlpzx
Changes after testing cloudformation
607b05d
@dlpzx
Changes after testing sagemaker - hardened to resource prefix
9f0e386
@dlpzx
Linting
467f396
@dlpzx
Linting-makefile
5f8c5a0
@dlpzx
Merge remote-tracking branch 'origin/v1m6m0' into feature/limit-envir…
c7e502e 
…onment-roles-permissions
@dlpzx
adding kms key in importing datasets first draft
0e18308
@dlpzx
Documentation on dataset and environment roles
c73d9b9
@dlpzx
Documentation on dataset and environment roles + modified frontend fo…
afcde7d 
…r imported dataset view
@dlpzx
Self review PR
044bfac
@dlpzx
Replace KMS key for Alias - better security
1a9ca87
@dlpzx
Added tag-based limitations to Lambda, update environment output, upd…
dd9be23 
…ate docs for KMSAlias
@dlpzx
Added missing glue crawler permissions in dataset, removed envAdminRo…
207475d 
…le from table syncer, fixed tests for kmsAlias, docs
@dlpzx
Review Lambda permissions
38bc0ce
@dlpzx
Review athena permissions - added listTags
873f6bb
@dlpzx
Review SSM and secrets permissions
685d85f
@dlpzx
Review CloudFormation permissions
944bcfa
@dlpzx
Review Step Functions permissions
311a2d7
dlpzx
dlpzx reviewed Jun 28, 2023
View reviewed changes
backend/dataall/cdkproxy/stacks/notebook.py
@@ -62,26 +63,47 @@ def __init__(self, scope, id: str, target_uri: str = None, **kwargs) -> None:

env_group = self.get_env_group(notebook)

cdk_exec_role = SessionHelper.get_cdk_exec_role_arn(notebook.AWSAccountId, notebook.region)

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we use environment.CDKRoleArn and avoid this function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes we could also do it that way - currently notebooks stack has no helper function to get the environment, only to get the env group. So either it would be adding that helper function in the Notebook Stack to get the environment and the environment.CDKRoleArn property or using the get_cdk_exec_role_arn() defined above

This is the only use of get_cdk_exec_role_arn() but in future it could be used more if needed - similar to how we have get_cdk_look_up_role_arn(). The value is rather static where the IAM role ARN would only change if we had custom synthesizers in CDK changing the qualifier - which I see little risk for. What are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

2 things.

  • Independently of how we get the role arn, we need to be consistent across modules. Either we use the environment property or the method but I would not have a mix of both.
  • I am more inclined to use a method exactly because of what you pointed out, custom qualifiers in CDK. Let's say that one customer wants to implement something like this. It would be relatively easy to add an SSM parameter with the qualifier and then get it in the get_cdk_exec_role_arn() method. Plus, we can get rid of an unnecessary column in RDS

dlpzx
dlpzx reviewed Jun 28, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am testing it locally and when I try to update an existing environment I get the following in the cdkproxy logs
image

@noah-paige noah-paige changed the base branch from feature/limit-environment-roles-permissions to v1m6m0 June 29, 2023 13:50
dlpzx
dlpzx approved these changes Jul 3, 2023
View reviewed changes
Copy link
Contributor

@dlpzx dlpzx left a comment

Choose a reason for hiding this comment

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

I will do a final testing as well, but it looks good

@noah-paige noah-paige merged commit 150fdc4 into v1m6m0 Jul 6, 2023
This was referenced Jul 11, 2023
Closed
Merged
@dlpzx dlpzx deleted the 512-restrict-kms-keys branch July 14, 2023 10:20
@dlpzx dlpzx restored the 512-restrict-kms-keys branch July 14, 2023 10:20
dlpzx added a commit that referenced this pull request Jul 19, 2023
@dlpzx @gmuslia
@noah-paige @nikpodsh @chamcca @dbalintx @mourya-33
V1.6.0 features (#565)
84c555e 
### Feature or Bugfix
Release PR with the following list of features. Refer to each PR for the
details

### Detail
- #498 
- #482 
- #543
- #524 (which also solves #531)
- #532 
- #535 
- #497 
- #515
- #529 
- #562 
- #455 
- #572 
- #567 
- #573 
- #579 
- #578 
- #582 

### Breaking changes - release notes
- ⚠️ IMPORTANT: upgrade to a version >V1.5.0 before upgrading to V1.6 to
avoid deletion of resources in custom resource deletion
- ⚠️ IMPORTANT: requires an update of environments and then datasets
after upgrading. Either using cdk.json parameter
`enable_update_dataall_stacks_in_cicd_pipeline`, waiting for overnight
update stack task, or manually updating first environments and then
datasets
- CloudFront distribution replace for #529 
- Additional EC2 permissions in CDK Synth CodeBuild stage for #543 -->
this can be avoided by upgrading to v1.5.6 before upgrading to v1.6.0
- local development affected by more restrictive pivotRole trust policy


### Relates
V1.6.0 release

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license.

---------

Co-authored-by: Gezim Musliaj <102723839+gmuslia@users.noreply.github.com>
Co-authored-by: Noah Paige <69586985+noah-paige@users.noreply.github.com>
Co-authored-by: nikpodsh <124577300+nikpodsh@users.noreply.github.com>
Co-authored-by: chamcca <40579012+chamcca@users.noreply.github.com>
Co-authored-by: Nikita Podshivalov <nikpodsh@amazon.com>
Co-authored-by: dbalintx <132444646+dbalintx@users.noreply.github.com>
Co-authored-by: mourya-33 <134511711+mourya-33@users.noreply.github.com>
@dlpzx dlpzx deleted the 512-restrict-kms-keys branch July 20, 2023 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dlpzx dlpzx dlpzx approved these changes

@nikpodsh nikpodsh Awaiting requested review from nikpodsh

@dbalintx dbalintx Awaiting requested review from dbalintx

Assignees

@noah-paige noah-paige

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Limit KMS keys policies
2 participants
@noah-paige @dlpzx