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.

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

chore: add support for IAM Identity Center in security diff #1052

Merged
merged 4 commits into from
Apr 30, 2024

Conversation

bergjaak
Copy link
Contributor

@bergjaak bergjaak commented Apr 29, 2024

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

@TheRealAmazonKendra
Copy link
Contributor

Are there any tests that can be added here?

@bergjaak
Copy link
Contributor Author

bergjaak commented Apr 29, 2024

@TheRealAmazonKendra

Are there any tests that can be added here?

I believe there are no tests for scrutinies in this package because they're tested in the aws-cdk repo. But I can make new tests if we think that would be beneficial. Though I don't have a problem with the tests being in aws-cdk. (Unless I am just not able to find the tests that are in this package... but it does make sense to me to test them where they're used, namely in aws-cdk repo.)

(19:19:56) bergjak@bcd074b101ed ~/workplace/CDK/awscdk-service-spec/packages/@aws-cdk AwsSsoFix ✔
 ➜ grep -rin scrut | grep -i test

(19:20:01) bergjak@bcd074b101ed ~/workplace/CDK/awscdk-service-spec/packages/@aws-cdk AwsSsoFix ✔
 ➜ find . | grep test.ts
./service-spec-types/test/resource.test.ts
./service-spec-importers/test/sam-spec.test.ts
./service-spec-importers/test/stateful-resources.test.ts
./service-spec-importers/test/property-conversion.test.ts
./service-spec-importers/test/loading/format-patch-report.test.ts
./service-spec-importers/test/sam-resources.test.ts
./service-spec-importers/test/loading-docs.test.ts
./service-spec-importers/test/db-diff.test.ts
./service-spec-importers/test/cloudformation-registry.test.ts
./service-spec-importers/test/patches/registry-patches.test.ts
./service-spec-importers/test/patches/json-schema-patches.test.ts
./service-spec-importers/test/printable-tree.test.ts
./service-spec-importers/test/double-import.test.ts
./service-spec-importers/test/canned-metrics.test.ts
./service-spec-importers/test/resource-spec.test.ts
./service-spec-importers/test/patching/fp.test.ts

Copy link

@aws-cdk/aws-service-spec: Model database diff detected

└[~] service aws-sso
  └ resources
     ├[~] resource AWS::SSO::Assignment
     │ └  - scrutinizable: undefined
     │    + scrutinizable: SsoAssignmentResource
     ├[~] resource AWS::SSO::InstanceAccessControlAttributeConfiguration
     │ └  - scrutinizable: undefined
     │    + scrutinizable: SsoInstanceACAConfigResource
     └[~] resource AWS::SSO::PermissionSet
       └  - scrutinizable: undefined
          + scrutinizable: SsoPermissionSet

@aws-cdk-automation aws-cdk-automation added this pull request to the merge queue Apr 30, 2024
Merged via the queue into main with commit f1e77e8 Apr 30, 2024
8 checks passed
@aws-cdk-automation aws-cdk-automation deleted the AwsSsoFix branch April 30, 2024 21:25
mergify bot pushed a commit to aws/aws-cdk that referenced this pull request May 2, 2024
### 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants