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

CDK Diff and Deploy do not include AWS::SSO::Assignment in security related changes #29835

Closed
2 of 3 tasks
TheRealAmazonKendra opened this issue Apr 15, 2024 · 2 comments · Fixed by #30009
Closed
2 of 3 tasks

CDK Diff and Deploy do not include AWS::SSO::Assignment in security related changes #29835

TheRealAmazonKendra opened this issue Apr 15, 2024 · 2 comments · Fixed by #30009
Assignees
@bergjaak
Labels
in-progress This issue is being actively worked on. p1

Comments

@TheRealAmazonKendra
Copy link
Contributor

TheRealAmazonKendra commented Apr 15, 2024 •
edited by bergjaak

Context

We received a report regarding diff not handling AWS::SSO::Assignment resource changes as we security related change. We currently handle two packages as security related in cloudformation-diff: iam and network. We should add similar logic for SSO.

I'm not actually positive whether we should call this a bug or a feature request because on closer inspection of our documentation for ConfirmPermissionsBroadening specifies Pause the pipeline if a deployment would add IAM permissions or Security Group rules. but the difference here is not super important. We should add this.

Task

Update diff to include these resource changes. As part of this task we should also check to see if any other security related libraries have been added since this was last updated (Organizations comes to mind) and check to see if any updates have been added within the known libraries that we may not be checking for already. If those are found, new tasks should be added for each of those.

Acceptance Criteria

  • AWS::SSO::Assignment resources are added to the list of security changes when cdk diff is run
  • A integ test is added/updated to include checking for these resources
  • New tasks are created for each other resource/library type that we find are not currently covered

Original Report

Summary

The documentation states: "To protect you against unintended changes that affect your security posture, the AWS CDK Toolkit prompts you to approve security-related changes before deploying them."

However, I noticed that new AWS::SSO::Assignment resources do not trigger the prompt thus allowing bypass of ConfirmPermissionsBroadening.

The changes are visible, as expected, when doing cdk diff
image

Details

AWS::SSO::Assignment is used with AWS IAM Identity Center which can be used to assign very wide range of permissions. It's important that this is also gated as documented.

PoC

// For each permission set, account and user/group combination, create a new CfnAssignment as defined in the configuration
        const permissionSetArn= getPermissionSetArn(props, permissionSetName);

        const groups = config[permissionSetName].Groups ?? [];
        const users = config[permissionSetName].Users ?? [];

        groups.forEach((group) => {
          new CfnAssignment(this, `Assignment-${account}-${permissionSetName}-${group}`, {
            instanceArn: props.ssoInstanceArn,
            permissionSetArn,
            principalId: groupIds[group].getResponseField('GroupId'),
            principalType: 'GROUP',
            targetId: account,
            targetType: 'AWS_ACCOUNT',
          });
        });
        
        users.forEach((user) => {
          new CfnAssignment(this, `Assignment-${account}-${permissionSetName}-${user}`, {
            instanceArn: props.ssoInstanceArn,
            permissionSetArn,
            principalId: userIds[user].getResponseField('UserId'),
            principalType: 'USER',
            targetId: account,
            targetType: 'AWS_ACCOUNT',
          });
        });
@TheRealAmazonKendra TheRealAmazonKendra changed the title Big: CDK Diff does not include AWS::SSO::Assignment in security related changes Bug: CDK Diff does not include AWS::SSO::Assignment in security related changes Apr 15, 2024
@TheRealAmazonKendra
Copy link
Contributor Author

TheRealAmazonKendra commented Apr 15, 2024 •
edited

Another resource like this: #29731

@comcalvi comcalvi added the p1 label Apr 15, 2024
@TheRealAmazonKendra TheRealAmazonKendra changed the title Bug: CDK Diff does not include AWS::SSO::Assignment in security related changes CDK Diff does not include AWS::SSO::Assignment in security related changes Apr 18, 2024
@TheRealAmazonKendra TheRealAmazonKendra changed the title CDK Diff does not include AWS::SSO::Assignment in security related changes CDK Diff and Deploy do not include AWS::SSO::Assignment in security related changes Apr 18, 2024
This was referenced Apr 22, 2024
Closed
Closed
@TheRealAmazonKendra TheRealAmazonKendra added the in-progress This issue is being actively worked on. label Apr 22, 2024
This was referenced Apr 29, 2024
Merged
Merged
github-merge-queue bot pushed a commit to cdklabs/awscdk-service-spec that referenced this issue Apr 30, 2024
@bergjaak
chore: add support for IAM Identity Center in security diff (#1052)
f1e77e8 
For issue aws/aws-cdk#29835

This is the first of 2 PRs. The other PR will be to the main aws-cdk
repository.

Notice that AWS::SSO::PermissionSet has a property called
`ManagedPolicies`. That's why I add that property check. And judging by
the db.json that we create in this package (the service spec),
AWS::SSO::PermissionSet is the only resource with that property name:

```
(18:36:39) bergjak@bcd074b101ed ~/workplace/CDK/awscdk-service-spec AwsSsoFix ✔
 ➜ cat ~/db.json4 | jq '.schema.resource.entities.[]' | jq '.properties' | grep ManagedPolicies
    "scrutinizable": "ManagedPolicies"
    "scrutinizable": "ManagedPolicies"
    "scrutinizable": "ManagedPolicies"
    "scrutinizable": "ManagedPolicies"
    "scrutinizable": "CustomerManagedPolicies"
  "ManagedPolicies": {
    "scrutinizable": "ManagedPolicies"
```

AWS::SSO is the IAM Identity Center, and therefore changes to AWS SSO
resources are security sensitive. Hence the issue.

### Testing
As you'll see in the next pull request, I have integration tests for
this change
* Here is the PR with all the testing
aws/aws-cdk#30009
@github-actions github-actions bot mentioned this issue May 1, 2024
Closed
@mergify mergify bot closed this as completed in #30009 May 2, 2024
mergify bot pushed a commit that referenced this issue May 2, 2024
@bergjaak
feat: support for IAM Identity Center in security diff (#30009)
0a3cb94 
### Issue # (if applicable)

Closes #29835 

### Reason for this change

IAM Identity Center resources were ignored in the security diff

### Description of changes

* Adds the  IAM Identity Center resources to CDK diff
* fixes not presenting property changes when a resource is removed from the template

### Description of how you validated changes

* Added unit tests and integration tests.
* Ran the integration tests that mention cdk diff (`bin/run-suite -a cli-integ-tests -t 'cdk diff'`):
```
Test Suites: 2 skipped, 1 passed, 1 of 3 total
Tests:       90 skipped, 13 passed, 103 total
Snapshots:   0 total
Time:        312.397 s
Ran all test suites with tests matching "cdk diff": 
```

### Dependent PRs

* Before this change can be merged, this change cdklabs/awscdk-service-spec#1052 must be merged.

### Checklist
- [Y] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions GitHub Actions
Copy link

github-actions bot commented May 2, 2024

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@bergjaak bergjaak

Labels
in-progress This issue is being actively worked on. p1
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: support for IAM Identity Center in security diff aws/aws-cdk
4 participants
@TheRealAmazonKendra @comcalvi @vinayak-kukreja @bergjaak