Add support for AWS::IoT::MitigationAction #15

saurabh14rajput · 2020-11-05T00:04:08Z

Notes
Mitigation actions allow you to take actions for mitigating issues that were found during an audit. AWS IoT Device Defender provides predefined actions for the different audit checks. You configure those actions for your AWS account and then apply them to a set of findings.
Documentation for Mitigation actions can be found at https://docs.aws.amazon.com/iot/latest/developerguide/device-defender-mitigation-actions.html
Most of the complications for this resources are around tagging (in handlers, we have to call separate tag management APIs).

This change includes: model, handlers, unit tests, inputs for contract tests.

Testing
Unit tests - 80%+ coverage.
All contract tests passed - except for the ones that failed due to known issues in the contract tests library.
Ran pre-commit run --all-files, mvn verify successfully.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

anton-aws

Let's add a description to this PR, can be something similar to what we have in the one for Dimensions.

anton-aws · 2020-11-06T22:03:23Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+            ]
+        },
+        "EnableIoTLoggingParams": {
+            "description": "Allows logging",


Let's use the descriptions from https://docs.aws.amazon.com/iot/latest/apireference/API_MitigationActionParams.html#iot-Type-MitigationActionParams and make sure to have one for every property.

anton-aws · 2020-11-06T22:05:22Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+            }
+        },
+        "AddThingsToThingGroupParams": {
+            "description": "Allows adding things that triggered the mitigation action to thing groups",


Minor: let's stay consistent and add periods at the end of all descriptions.

anton-aws · 2020-11-06T22:09:36Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/Translator.java

+ *  - object translation to/from aws sdk
+ *  - resource model construction for read/list handlers
+ */
+


Minor: There seem to be some extra new lines and inconsistent indentation depths. Let's reformat all the files that need it.

anton-aws · 2020-11-06T22:11:48Z

aws-iot-mitigationaction/inputs/inputs_1_create.json

+    "ActionName": "CfnContractTest",
+    "RoleArn": "arn:aws:iam::072938559174:role/Admin",
+    "ActionParams":
+        {


Let's run pre-commit run --all-files, that should reformat all the json files to be consistent.

anton-aws · 2020-11-06T22:12:33Z

aws-iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/ListHandlerTest.java

+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.when;
+
+import java.sql.Date;


Looks like an accidental import. Let's run "organize imports" for all files that need it.

anton-aws · 2020-11-09T21:56:02Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/ReadHandler.java

+// TODO: replace all usage of SdkClient with your service client type, e.g; YourServiceAsyncClient
+// import software.amazon.awssdk.services.yourservice.YourServiceAsyncClient;


Accidental leftover

anton-aws · 2020-11-09T22:03:05Z

aws-iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/TestConstants.java

+            software.amazon.awssdk.services.iot.model.PublishFindingToSnsParams.builder()
+                    .topicArn("arn:aws:sns:us-east-1:072938559174:myTopic.fifo")
+                    .build();
+    protected static final software.amazon.awssdk.services.iot.model.MitigationActionParams SDK_ACTION_PARAMS2 =


Minor: IntelliJ pointed out to me that this one is unused. If there's no need for it, let's remove it.

anton-aws · 2020-11-09T22:08:32Z

aws-iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/TranslatorTest.java

+    }
+
+    @Test
+    void translateActionParamsToSdkTest() {


Let's verify conversion for all the mitigation actions, not just ReplaceDefaultPolicyVersion. Same below.

Minor: it would be nice to keep the test naming convention as above.

anton-aws · 2020-11-09T22:14:19Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/Translator.java

+            .collect(Collectors.toSet());
+  }
+
+  static MitigationActionParams translateActionParamsToSdk(ActionParams actionParams) {


Consider a case where customer fills out all the actions (which is incorrect). Looks like this method will just fill out AddThingsToThingGroup and return. How about we pass the customer's template the way it is and let the service throw the right exception? If so, maybe drop the else in else ifs. And let's have a unit test to cover that.

Makes sense. Done

anton-aws · 2020-11-09T22:20:15Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/Translator.java

+            .addThingsToThingGroupParams(addThingsToThingGroupParams)
+            .addThingsToThingGroupParams(addThingsToThingGroupParams)


Looks like an accidental copy.

anton-aws · 2020-11-09T22:21:24Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/Translator.java

+    }
+    else if (actionParams.enableIoTLoggingParams() != null) {
+      enableIoTLoggingParams = EnableIoTLoggingParams.builder()
+              .logLevel(actionParams.enableIoTLoggingParams().logLevel().toString())


Minor: the SDK provides a method logLevelAsString, it's nice for this use case.

Nice. Changed.

anton-aws · 2020-11-09T23:09:53Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+            ]
+        },
+        "update": {
+            "permissions": [


Let's add iam:PassRole here as well

anton-aws · 2020-11-16T22:09:29Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+    },
+    "additionalProperties": false,
+    "required": [
+        "ActionName",


Shouldn't the ActionName be optional for this schema? CreateHandler generates an ActionName if it's not provided.

Good catch! Changed.

saurabh14rajput · 2020-12-01T02:57:02Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

@@ -1,239 +1,248 @@
 {


JSON files are showing big delta because of fixing indentation using pre-commit hook.

anton-aws · 2020-12-01T22:46:57Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/Translator.java

+    } else if (e instanceof InvalidRequestException) {
+      return new CfnInvalidRequestException(e);
+    } else if (e instanceof LimitExceededException) {
+      return new CfnServiceLimitExceededException(e);


Minor: somehow the indentations in this file are size 2 instead of 4 like everywhere else

Yeah. It is weird. Auto indentation did not work either.
Fixed it with this(In case we face again): "Settings | Editor | Code Style" -- try disabling "Detect and use existing file indents for editing" in case if you have it enabled (it is by default). NOTE: re-opening file in editor may be required.

anton-aws · 2020-12-01T22:49:03Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/CreateHandler.java

+        // Like most services, we don't require an explicit resource name in the template,
+        // and, if it's not provided, generate one based on the stack ID and logical ID.
+        if (StringUtils.isBlank(model.getActionName())) {
+            // TODO: include stack-id in the generated name. Will do it once the new release of the java plugin


Let's do this as part of this PR

I did already made that change. Missed taking out the TODO. Removed now.

anton-aws · 2020-12-01T22:50:28Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+    "MitigationActionId": {
+      "type": "string"
+    },
+    "CreationDate": {


As discussed, let's remove the CreationDate

anton-aws · 2020-12-01T22:51:15Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/UpdateHandler.java

+        // Yet we should, otherwise the resource wouldn't equate the template.
+        Set<Tag> currentTags = listTags(proxy, resourceArn, logger);
+
+        // getDesiredResourceTags includes model+stack-level tags, reference: https://tinyurl.com/y2p8medk


Here's a better link https://tinyurl.com/y55mqrnc

anton-aws · 2020-12-02T18:32:08Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+  "typeName": "AWS::IoT::MitigationAction",
+  "description": "Mitigation actions can be used to take actions to mitigate issues that were found during an audit.",
+  "sourceUrl": "https://github.com/aws-cloudformation/aws-cloudformation-resource-providers-iot.git",
+  "definitions": {


Let's add "additionalProperties": false for all entries with type=object

anton-aws · 2020-12-02T18:33:22Z

aws-iot-mitigationaction/inputs/inputs_1_create.json

+  "RoleArn": "{{RoleForDeviceDefenderAuditArn}}",
+  "ActionParams": {
+    "PublishFindingToSnsParams": {
+      "TopicArn": "{{TopicForDeviceDefenderAuditCreateArn}}"


vabhishe93 · 2020-12-04T01:41:04Z

aws-iot-mitigationaction/inputs/inputs_1_create.json

@@ -0,0 +1,15 @@
+{
+  "ActionName": "CfnContractTest",


It will be nice to have a test for every single action that we support, else how do you know that the definition of one of the actions isn't broken?

Adding an integ test for them in the integ test package.

vabhishe93 · 2020-12-04T01:43:52Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/HandlerUtils.java

+public class HandlerUtils {
+
+    public static List<software.amazon.awssdk.services.iot.model.Tag> listTags(


It might be better to pull the common code to a common package, it would be painful for now but reduces the effort for future.

Opened an issue about this with the cfn team: aws-cloudformation/cloudformation-cli-java-plugin#335

vabhishe93 · 2020-12-04T01:46:38Z

...iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/CreateHandlerTest.java

+@ExtendWith(MockitoExtension.class)
+public class CreateHandlerTest {


Let's add tests for every single action that we support in both CREATE and UPDATE paths

Nevermind, saw that you added tests for translator

vabhishe93 · 2020-12-17T02:18:53Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

@@ -0,0 +1,253 @@
+{
+  "typeName": "AWS::IoT::MitigationAction",
+  "description": "Mitigation actions can be used to take actions to mitigate issues that were found during an audit.",


Mitigation Action isn't specific to Audit, now even ML Detect uses it. So lets paraphrase

vabhishe93 · 2020-12-17T02:23:07Z

aws-iot-mitigationaction/aws-iot-mitigationaction.json

+    },
+    "UpdateCACertificateParams": {
+      "type": "object",
+      "description": "Parameters to define a mitigation action that changes the state of the CA certificate to inactive.s",


Typo s/inactive.s/inactive./g

vabhishe93 · 2020-12-17T02:27:04Z

aws-iot-mitigationaction/inputs/inputs_1_create.json

@@ -0,0 +1,15 @@
+{
+  "ActionName": "CfnContractTest",
+  "RoleArn": "{{RoleForDeviceDefenderAuditArn}}",


Let's not use "Audit" in names of even the test resource. Reason being MitigationActions isn't specific to Audit anymore (it's a common resource to both Audit/Detect)

vabhishe93 · 2020-12-17T02:27:26Z

aws-iot-mitigationaction/inputs/inputs_1_invalid.json

+  "RoleArn": "{{RoleForDeviceDefenderAuditArn}}",
+  "ActionParams": {
+    "PublishFindingToSnsParams": {
+      "TopicArn": "{{TopicForDeviceDefenderAuditCreateArn}}"
+    }


Same as above, do not use Audit.

vabhishe93 · 2020-12-17T02:28:02Z

...iot-mitigationaction/packaging_additional_published_artifacts/contract_test_dependencies.yml

@@ -0,0 +1,46 @@
+AWSTemplateFormatVersion: 2010-09-09
+Description: 'Resources for running the AccountAuditConfiguration contract tests'


This Description seems wrong. Should say "Mitigation Actions contract tests"

vabhishe93 · 2020-12-17T02:29:10Z

...iot-mitigationaction/packaging_additional_published_artifacts/contract_test_dependencies.yml

+Outputs:
+  RoleForDeviceDefenderAuditOutput:
+    Value:
+      Fn::GetAtt: [RoleForDeviceDefenderAudit, Arn]
+    Export:
+      Name: RoleForDeviceDefenderAuditArn
+  TopicForDeviceDefenderAuditCreateOutput:
+    Value:
+      Ref: TopicForDeviceDefenderAuditCreate
+    Export:
+      Name: TopicForDeviceDefenderAuditCreateArn
+  TopicForDeviceDefenderAuditUpdateOutput:
+    Value:
+      Ref: TopicForDeviceDefenderAuditUpdate
+    Export:
+      Name: TopicForDeviceDefenderAuditUpdateArn
+  AccountIdOutput:
+    Value:
+      Fn::Sub: "${AWS::AccountId}"
+    Export:


Same as above. It doesn't matter that much, but why not use a generic name that doesn't contain Audit.

vabhishe93 · 2020-12-17T02:33:10Z

aws-iot-mitigationaction/src/main/java/com/amazonaws/iot/mitigationaction/CreateHandler.java

+
+        if (!StringUtils.isEmpty(model.getMitigationActionId())) {
+            logger.log(String.format("MitigationActionId is read-only, but the caller passed %s.",
+                    model.getMitigationActionArn()));


Incorrect field name, should be MitigationActionId.

vabhishe93 · 2020-12-17T02:41:51Z

...iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/CreateHandlerTest.java

+    @Test
+    public void handleRequest_HappyCase_VerifyRequestResponse() {


In all tests lets verify that no more interactions were made with the proxy mock.

vabhishe93 · 2020-12-17T02:42:51Z

...iot-mitigationaction/src/test/java/com/amazonaws/iot/mitigationaction/CreateHandlerTest.java

+        assertThat(submittedRequest.actionName().length() > 20).isTrue();
+    }
+
+    // TODO: test system tags when the src code is ready


Remove this line I'd say. When we add support then it's possible we don't even look at this line and just forget about it. Hardly people check for TODOs in code.

…resource name case

Add support for AWS::IoT::MitigationAction

8fc28dc

anton-aws suggested changes Nov 6, 2020

View reviewed changes

anton-aws suggested changes Nov 9, 2020

View reviewed changes

anton-aws reviewed Nov 16, 2020

View reviewed changes

Add improvements for the second revision for MitigationAction PR

d5cf3ff

saurabh14rajput commented Dec 1, 2020

View reviewed changes

anton-aws suggested changes Dec 1, 2020

View reviewed changes

Add improvements for the third revision for MitigationAction PR

90588d5

anton-aws suggested changes Dec 2, 2020

View reviewed changes

Add improvements for the fourth revision for MitigationAction PR

8ba164e

anton-aws approved these changes Dec 3, 2020

View reviewed changes

vabhishe93 reviewed Dec 4, 2020

View reviewed changes

Use ProgressEvents instead of Cfn exceptions

2244540

vabhishe93 reviewed Dec 17, 2020

View reviewed changes

Address PR comments and add workaround in delete handler for invalid …

2b99a7c

…resource name case

vabhishe93 approved these changes Dec 18, 2020

View reviewed changes

anton-aws merged commit d5a82f9 into aws-cloudformation:master Dec 19, 2020

		// TODO: replace all usage of SdkClient with your service client type, e.g; YourServiceAsyncClient
		// import software.amazon.awssdk.services.yourservice.YourServiceAsyncClient;

		.addThingsToThingGroupParams(addThingsToThingGroupParams)
		.addThingsToThingGroupParams(addThingsToThingGroupParams)

		public class HandlerUtils {

		public static List<software.amazon.awssdk.services.iot.model.Tag> listTags(

		@ExtendWith(MockitoExtension.class)
		public class CreateHandlerTest {

		@@ -0,0 +1,46 @@
		AWSTemplateFormatVersion: 2010-09-09
		Description: 'Resources for running the AccountAuditConfiguration contract tests'

		@Test
		public void handleRequest_HappyCase_VerifyRequestResponse() {

Add support for AWS::IoT::MitigationAction #15

Add support for AWS::IoT::MitigationAction #15

Conversation

saurabh14rajput commented Nov 5, 2020 • edited Loading

anton-aws left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saurabh14rajput commented Nov 5, 2020 •

edited

Loading