security topic sns subscription #41

64ne · 2020-12-17T09:03:37Z

This is PR to support subscribing to SNS topic, aws-controltower-AggregateSecurityNotifications

github-actions · 2020-12-17T09:04:26Z

`terraform validate` Failed


Error: Missing required argument

  on audit.tf line 156, in module "security_hub_audit":
 156: module "security_hub_audit" {

The argument "account_id" is required, but no definition was found.


Error: Missing required argument

  on audit.tf line 156, in module "security_hub_audit":
 156: module "security_hub_audit" {

The argument "sns_endpoint" is required, but no definition was found.


Error: Missing required argument

  on audit.tf line 156, in module "security_hub_audit":
 156: module "security_hub_audit" {

The argument "sns_endpoint_protocol" is required, but no definition was found.

Workflow: Terraform, Action: hashicorpterraform-github-actions2, Working Directory: ., Workspace: default

shoekstra · 2020-12-17T09:27:23Z

variables.tf

+variable "sns_endpoint" {
+  type        = string
+  description = "Endpoint for SNS topic subscription"
+}
+
+variable "sns_endpoint_protocol" {
+  type        = string
+  description = "Endpoint protocol for SNS topic subscription"
+}
+
+variable "sns_security_topic_subscription" {
+  type        = bool
+  description = "Enable SNS aggregated security topic subscription"
+}


Could these values be specified in a single sns_security_subscription object?

shoekstra · 2020-12-17T09:27:34Z

modules/security_hub/variables.tf

+variable "sns_endpoint" {
+  type        = string
+  description = "Endpoint for SNS topic subscription"
+}
+
+variable "sns_endpoint_protocol" {
+  type        = string
+  description = "Endpoint protocol for SNS topic subscription"
+}
+
+variable "sns_security_topic_subscription" {
+  type        = bool
+  default     = false
+  description = "Enable SNS aggregated security topic subscription"
+}


Could these values be specified in a single sns_security_subscription object?

shoekstra · 2020-12-17T09:28:41Z

modules/security_hub/variables.tf

+variable "account_id" {
+  type        = string
+  description = "AWS Account ID"
+}


What is this for? There is no reference to this variable anywhere else.

it is referenced in topic_arn = "arn:aws:sns:${var.region}:${var.account_id}:aws-controltower-AggregateSecurityNotifications", line 34 in modules/security_hub/main.tf

shoekstra · 2020-12-17T09:30:08Z

README.md

+If you would like to subscribe to aggregated security SNS topic created by Control Tower, set `sns_security_topic_subscription` variable to `true`.
+And provide values for your endpoint to receive notifications, variable `sns_endpoint` and protocol to be used, variable `sns_endpoint_protocol`.


This needs to be reworded as it's hard to follow in the current form. Can you please also add an example like other opt-in settings.

github-actions · 2020-12-29T10:12:42Z

`terraform fmt` Failed

./audit.tf

 }
 
 module "security_hub_audit" {
-  source                          = "./modules/security_hub"
-  providers                       = { aws = aws.audit }
-  account_id                      = data.aws_caller_identity.current.account_id
-  sns_security_subscription       = var.sns_security_subscription
+  source                    = "./modules/security_hub"
+  providers                 = { aws = aws.audit }
+  account_id                = data.aws_caller_identity.current.account_id
+  sns_security_subscription = var.sns_security_subscription
 
   member_accounts = {
     for id, email in local.aws_account_emails : id => email if id != var.control_tower_account_ids.audit

./modules/security_hub/variables.tf

 
 variable "sns_security_subscription" {
   type = list(object({
-    sns_endpoint                    = string
-    sns_endpoint_protocol           = string
+    sns_endpoint          = string
+    sns_endpoint_protocol = string
   }))
   default     = null
   description = "Aggregated security SNS topic subscription options"

./variables.tf

 
 variable "sns_security_subscription" {
   type = list(object({
-    sns_endpoint                    = string
-    sns_endpoint_protocol           = string
+    sns_endpoint          = string
+    sns_endpoint_protocol = string
   }))
   default     = null
   description = "Aggregated security SNS topic subscription options"

Workflow: Terraform, Action: hashicorpterraform-github-actions, Working Directory: ., Workspace: default

github-actions · 2020-12-29T10:13:09Z

`terraform validate` Failed


Error: Missing required argument

  on main.tf line 129, in module "security_hub_master":
 129: module "security_hub_master" {

The argument "account_id" is required, but no definition was found.

Workflow: Terraform, Action: hashicorpterraform-github-actions2, Working Directory: ., Workspace: default

github-actions · 2020-12-29T10:16:44Z

`terraform validate` Failed


Error: Missing required argument

  on logging.tf line 70, in module "security_hub_logging":
  70: module "security_hub_logging" {

The argument "account_id" is required, but no definition was found.

Workflow: Terraform, Action: hashicorpterraform-github-actions2, Working Directory: ., Workspace: default

shoekstra · 2020-12-29T11:39:56Z

CHANGELOG.md

@@ -4,7 +4,11 @@ All notable changes to this project will be documented in this file.

 The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/).

-## Unreleased
+## Unreleased (2020-12-29)


Suggested change

## Unreleased (2020-12-29)

## Unreleased

No need to append a date to the unreleased header.

shoekstra · 2020-12-29T11:41:08Z

modules/security_hub/main.tf

@@ -26,3 +26,11 @@ resource "aws_securityhub_standards_subscription" "default" {
  standards_arn = each.value
  depends_on    = [aws_securityhub_account.default]
 }
+
+resource "aws_sns_topic_subscription" "datadog-security" {


Suggested change

resource "aws_sns_topic_subscription" "datadog-security" {

resource "aws_sns_topic_subscription" "datadog_security" {

We should use _ in resource names.

shoekstra · 2020-12-29T11:41:46Z

modules/security_hub/variables.tf

+variable "account_id" {
+  type        = string
+  default     = null
+  description = "AWS Account ID"


What AWS Account ID should be entered here? The description could do with some updating 🙂

shoekstra · 2020-12-29T11:42:49Z

modules/security_hub/variables.tf

+    sns_endpoint          = string
+    sns_endpoint_protocol = string


Suggested change

sns_endpoint = string

sns_endpoint_protocol = string

endpoint = string

protocol = string

No need to include an sns prefix if the variable is about SNS already.

Does it make sense to trim endpoint_protocol down to just protocol too?

shoekstra · 2020-12-29T11:43:24Z

variables.tf

+    sns_endpoint          = string
+    sns_endpoint_protocol = string


Suggested change

sns_endpoint = string

sns_endpoint_protocol = string

endpoint = string

protocol = string

No need to include an sns prefix if the variable is about SNS already.

Does it make sense to trim endpoint_protocol down to just protocol too?

shoekstra · 2020-12-29T11:48:49Z

README.md

+  sns_endpoint          = "https://app.datadoghq.com/intake/webhook/sns?api_key=qwerty0123456789"
+  sns_endpoint_protocol = "https"


Suggested change

sns_endpoint = "https://app.datadoghq.com/intake/webhook/sns?api_key=qwerty0123456789"

sns_endpoint_protocol = "https"

sns_security_subscription = {

endpoint = "https://app.datadoghq.com/intake/webhook/sns?api_key=qwerty0123456789"

protocol = "https"

}

github-actions · 2020-12-29T14:12:05Z

`terraform validate` Failed


Error: Error in function call

  on audit.tf line 2, in locals:
   2:   sns_security_subscription = merge(
   3:     var.sns_security_subscription,
   4:     { account_id = var.control_tower_account_ids.audit }
   5:   )

Call to function "merge" failed: arguments must be maps or objects, got "list
of object".

Workflow: Terraform, Action: hashicorpterraform-github-actions2, Working Directory: ., Workspace: default

shoekstra · 2020-12-29T13:43:11Z

README.md

+Example for https protocol and specified webhook endpoint:
+
+```hcl
+module "landing_zone"{


Suggested change

module "landing_zone"{

module "landing_zone" {

shoekstra · 2020-12-29T13:47:58Z

README.md

@@ -148,6 +148,24 @@ module "landing_zone" {
  ]
 ```

+### Enable SNS topic subscription
+
+If you need to subscribe to AggregatedSecurityNotifications topic in order to receive security findings, please set values for `sns_endpoint` and `sns_endpoint_protocol` variables.


Suggested change

If you need to subscribe to AggregatedSecurityNotifications topic in order to receive security findings, please set values for `sns_endpoint` and `sns_endpoint_protocol` variables.

To subscribe to the `AggregatedSecurityNotifications` topic to receive security findings, set the `sns_security_subscription` variable as shown below.

github-actions · 2020-12-29T14:17:43Z

`terraform validate` Failed


Error: Invalid value for module argument

  on audit.tf line 166, in module "security_hub_audit":
 166:   sns_subscription = local.sns_security_subscription

The given value is not suitable for child module variable "sns_subscription"
defined at modules/security_hub/variables.tf:19,1-28: list of object required.

Workflow: Terraform, Action: hashicorpterraform-github-actions2, Working Directory: ., Workspace: default

github-actions · 2020-12-29T14:30:58Z

`terraform fmt` Failed

./audit.tf

 locals {
   sns_security_subscription = [
     for sub in var.sns_security_subscription :
-      merge(sub, { account_id = var.control_tower_account_ids.audit }
+    merge(sub, { account_id = var.control_tower_account_ids.audit }
     )
   ]
 }

Workflow: Terraform, Action: hashicorpterraform-github-actions, Working Directory: ., Workspace: default

shoekstra · 2020-12-29T14:34:57Z

audit.tf

+    merge(sub, { account_id = var.control_tower_account_ids.audit }
+    )


Suggested change

merge(sub, { account_id = var.control_tower_account_ids.audit }

)

merge(sub, { account_id = var.control_tower_account_ids.audit })

64ne requested review from shoekstra, fernandogoncalves-me and marwinbaumannsbp December 17, 2020 09:03

shoekstra requested changes Dec 17, 2020

View reviewed changes

64ne added 4 commits December 29, 2020 11:09

support for aggregated security sns topic subscription

6d92c4d

update README.md and CHANGELOG.md

2a91094

changed subscription to support multiple subscriptions

21a0ebe

update README.md

cb768ab

64ne force-pushed the banovicn-security-topic-sns-subscription branch from 2b039c2 to cb768ab Compare December 29, 2020 10:12

fmt fix

45bc98e

64ne force-pushed the banovicn-security-topic-sns-subscription branch from a496048 to 45bc98e Compare December 29, 2020 10:16

added default value for variable account_id

42a0f04

64ne force-pushed the banovicn-security-topic-sns-subscription branch from b3afc70 to 42a0f04 Compare December 29, 2020 10:48

64ne requested a review from shoekstra December 29, 2020 10:52

shoekstra requested changes Dec 29, 2020

View reviewed changes

changes

8c89755

64ne force-pushed the banovicn-security-topic-sns-subscription branch from 223e21f to 8c89755 Compare December 29, 2020 13:42

64ne requested a review from shoekstra December 29, 2020 13:42

change of variables

2ab0cc2

64ne force-pushed the banovicn-security-topic-sns-subscription branch from 1639d16 to 2ab0cc2 Compare December 29, 2020 14:11

shoekstra requested changes Dec 29, 2020

View reviewed changes

change of README.md

1146d3c

64ne force-pushed the banovicn-security-topic-sns-subscription branch from 0ceaf1f to 1146d3c Compare December 29, 2020 14:16

change of locals

2dd3ad8

64ne force-pushed the banovicn-security-topic-sns-subscription branch from d1b869a to 2dd3ad8 Compare December 29, 2020 14:30

fmt

4cd4a9d

64ne force-pushed the banovicn-security-topic-sns-subscription branch from 66e8b0d to 4cd4a9d Compare December 29, 2020 14:32

64ne requested a review from shoekstra December 29, 2020 14:33

shoekstra requested changes Dec 29, 2020

View reviewed changes

fix

2f075b0

64ne force-pushed the banovicn-security-topic-sns-subscription branch from cc9c8b0 to 2f075b0 Compare December 29, 2020 14:36

64ne requested a review from shoekstra December 29, 2020 14:36

terraform-docs: Update module usage

1db85eb

shoekstra approved these changes Dec 29, 2020

View reviewed changes

64ne merged commit 55a55be into master Dec 29, 2020

64ne deleted the banovicn-security-topic-sns-subscription branch December 29, 2020 14:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security topic sns subscription #41

security topic sns subscription #41

64ne commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

shoekstra Dec 17, 2020

64ne Dec 29, 2020

shoekstra Dec 17, 2020

64ne Dec 29, 2020

shoekstra Dec 17, 2020

64ne Dec 21, 2020

shoekstra Dec 17, 2020

64ne Dec 29, 2020

github-actions bot commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

github-actions bot commented Dec 29, 2020

shoekstra Dec 29, 2020

shoekstra Dec 29, 2020

github-actions bot commented Dec 29, 2020

github-actions bot commented Dec 29, 2020

shoekstra Dec 29, 2020

		If you would like to subscribe to aggregated security SNS topic created by Control Tower, set `sns_security_topic_subscription` variable to `true`.
		And provide values for your endpoint to receive notifications, variable `sns_endpoint` and protocol to be used, variable `sns_endpoint_protocol`.

	resource "aws_sns_topic_subscription" "datadog-security" {
	resource "aws_sns_topic_subscription" "datadog_security" {

		sns_endpoint = "https://app.datadoghq.com/intake/webhook/sns?api_key=qwerty0123456789"
		sns_endpoint_protocol = "https"

	If you need to subscribe to AggregatedSecurityNotifications topic in order to receive security findings, please set values for `sns_endpoint` and `sns_endpoint_protocol` variables.
	To subscribe to the `AggregatedSecurityNotifications` topic to receive security findings, set the `sns_security_subscription` variable as shown below.

		merge(sub, { account_id = var.control_tower_account_ids.audit }
		)

security topic sns subscription #41

security topic sns subscription #41

Conversation

64ne commented Dec 17, 2020

github-actions bot commented Dec 17, 2020

terraform validate Failed

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

github-actions bot commented Dec 29, 2020

terraform fmt Failed

github-actions bot commented Dec 29, 2020

terraform validate Failed

github-actions bot commented Dec 29, 2020

terraform validate Failed

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

github-actions bot commented Dec 29, 2020

terraform validate Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 29, 2020

terraform validate Failed

github-actions bot commented Dec 29, 2020

terraform fmt Failed

Choose a reason for hiding this comment

`terraform validate` Failed

`terraform fmt` Failed

`terraform validate` Failed

`terraform validate` Failed

`terraform validate` Failed

`terraform validate` Failed

`terraform fmt` Failed