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

New resource: aws_securityhub_standards_control #14714

Merged
merged 12 commits into from
Jul 13, 2021

Conversation

Tensho
Copy link
Contributor

@Tensho Tensho commented Aug 18, 2020

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #11624

Release note for CHANGELOG:

New resource: aws_securityhub_standards_control

Example Usage

resource "aws_securityhub_account" "example" {}

resource "aws_securityhub_standards_subscription" "cis_aws_foundations_benchmark" {
  standards_arn = "arn:aws:securityhub:::ruleset/cis-aws-foundations-benchmark/v/1.2.0"
  depends_on    = [aws_securityhub_account.example]
}

resource "aws_securityhub_standards_control" "ensure_iam_password_policy_prevents_password_reuse" {
  arn             = "arn:aws:securityhub:us-east-1:078884324539:control/cis-aws-foundations-benchmark/v/1.2.0/1.10"
  control_status  = "DISABLED"
  disabled_reason = "We handle password policies within Okta" 
  depends_on      = [aws_securityhub_standards_subscription.cis_aws_foundations_benchmark]
}

Acceptance Tests

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHubStandardsControl'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHubStandardsControl -timeout 120m
=== RUN   TestAccAWSSecurityHubStandardsControl_basic
--- PASS: TestAccAWSSecurityHubStandardsControl_basic (19.31s)
=== RUN   TestAccAWSSecurityHubStandardsControl_disabledControlStatus
--- PASS: TestAccAWSSecurityHubStandardsControl_disabledControlStatus (18.76s)
=== RUN   TestAccAWSSecurityHubStandardsControl_enabledControlStatusAndDisabledReason
--- PASS: TestAccAWSSecurityHubStandardsControl_enabledControlStatusAndDisabledReason (6.82s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	47.379s

Notes

  1. This is my first attempt to make terraform-plugin-sdk/v2 compatible resource. Please don't hesitate to refer me to the best practices according to this shift.
  2. I don't' know what is a standard approach for time.Time attribute type in this provider and chose a simple String() conversion.
  3. Standard control resource has an "adoption" lifecycle – resource already exists in the cloud and Terraform provider calls Create() to accept it into the management and Delete() just removes from the state.
  4. I noticed some resource have Read() func call at the end of Update() func and some not. I do believe if a resource has computed attributes any update (for synchronous API) should re-read them to the state. Please correct me if I'm wrong.

@Tensho Tensho requested a review from a team August 18, 2020 10:14
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. service/securityhub Issues and PRs that pertain to the securityhub service. needs-triage Waiting for first response or review from a maintainer. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. documentation Introduces or discusses updates to documentation. labels Aug 18, 2020
@Tensho Tensho force-pushed the securityhub-standard-controls branch 4 times, most recently from ff595c7 to 96cad0c Compare August 18, 2020 10:31
@Tensho
Copy link
Contributor Author

Tensho commented Aug 18, 2020

@DrFaust92 Why does terrafmt argue here? Local run doesn't bring more context... 🤔

Screenshot 2020-08-18 at 13 37 59

@Tensho Tensho force-pushed the securityhub-standard-controls branch from aaf0e84 to 6fe8166 Compare August 18, 2020 10:59
@DrFaust92
Copy link
Collaborator

I think that the issue lies with the name of website/docs/r/securityhub_standards_control.markdown
rename to website/docs/r/securityhub_standards_control.html.markdown

@Tensho
Copy link
Contributor Author

Tensho commented Aug 19, 2020

@DrFaust92 Thank you for the suggestion 🙇 The problem was in trailing space 😅

@Tensho Tensho force-pushed the securityhub-standard-controls branch 2 times, most recently from 404e956 to 3605e16 Compare August 25, 2020 09:56
@Tensho
Copy link
Contributor Author

Tensho commented Sep 7, 2020

Just to not forget, I've submitted AWS feature request support ticket to provide any of these options in SecurityHub API and AWS Go DSK to get the specific standard control:

  1. Add GetStandardsControls API action accepting standard control ARN parameter.
  2. Extend DescribeStandardsControls API action with StandardControlArn parameter.
  3. Return StandardSubscriptionArn in DescribeStandardsControls API action response (StandardsControl data type).

This should allow to rid of the local filtering.

@ewbankkit
Copy link
Contributor

Relates: #15090.

@ewbankkit
Copy link
Contributor

  • If Create and Update perform the same underlying action the pattern is to name the method with Put and use set it in both schema.Resource.Create and schema.Resource.Update. Take a look at aws/resource_aws_cloudwatch_dashboard.go for an example
  • For timestamps we prefer RFC 3339: Add awsproviderlint check to prefer RFC 3339 dates #13708

@Tensho Tensho force-pushed the securityhub-standard-controls branch 2 times, most recently from c4a960f to cba70bf Compare September 29, 2020 14:30
@lanzrein
Copy link

Hello,
Is there any timeline on when this PR will be merged?
I would like very much to use this feature.
Many thanks !

@MRinalducci
Copy link

MRinalducci commented Apr 14, 2021

Hi @ewbankkit any news regarding the merge of this PR?
We're currently in a uncomfortable situation without this feature.
Maybe @bill-rich can help us out?
Thanks for your time!

@jgeurts
Copy link
Contributor

jgeurts commented May 19, 2021

@Tensho Could you please merge latest with your changes?

@navjotkaur10
Copy link

Hello,
Any news on getting this PR merged? We would like to use this feature.

@robertstettner
Copy link
Contributor

Hi @ewbankkit, could we please merge this?

@ewbankkit ewbankkit force-pushed the securityhub-standard-controls branch from a2a4c14 to c578a79 Compare July 13, 2021 13:28
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

Commercial
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHub_serial/StandardsSubscription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHub_serial/StandardsSubscription -timeout 180m
=== RUN   TestAccAWSSecurityHub_serial
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/basic
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/disappears
--- PASS: TestAccAWSSecurityHub_serial (35.15s)
    --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription (35.15s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/basic (18.83s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/disappears (16.32s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	38.304s
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHub_serial/StandardsControl'     
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHub_serial/StandardsControl -timeout 180m
=== RUN   TestAccAWSSecurityHub_serial
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/basic
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/DisabledControlStatus
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/EnabledControlStatusAndDisabledReason
--- PASS: TestAccAWSSecurityHub_serial (49.49s)
    --- PASS: TestAccAWSSecurityHub_serial/StandardsControl (49.49s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/basic (20.87s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/DisabledControlStatus (17.66s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/EnabledControlStatusAndDisabledReason (10.95s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	53.082s
GovCloud
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHub_serial/StandardsSubscription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHub_serial/StandardsSubscription -timeout 180m
=== RUN   TestAccAWSSecurityHub_serial
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/basic
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/disappears
--- PASS: TestAccAWSSecurityHub_serial (46.33s)
    --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription (46.33s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/basic (25.05s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/disappears (21.29s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	49.527s
% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHub_serial/StandardsControl'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHub_serial/StandardsControl -timeout 180m
=== RUN   TestAccAWSSecurityHub_serial
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/basic
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/DisabledControlStatus
=== RUN   TestAccAWSSecurityHub_serial/StandardsControl/EnabledControlStatusAndDisabledReason
--- PASS: TestAccAWSSecurityHub_serial (60.13s)
    --- PASS: TestAccAWSSecurityHub_serial/StandardsControl (60.13s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/basic (23.57s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/DisabledControlStatus (22.34s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsControl/EnabledControlStatusAndDisabledReason (14.22s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	63.236s

@ewbankkit
Copy link
Contributor

@Tensho Thanks for the contribution 🎉 👏.
I made a couple of minor tweaks to get this merged quicker:

  • Documentation changes
  • Serialized acceptance tests

… status in 'testAccCheckAWSSecurityHubStandardsSubscriptionDestroy'.

Acceptance test output:

% make testacc TEST=./aws TESTARGS='-run=TestAccAWSSecurityHub_serial/StandardsSubscription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSSecurityHub_serial/StandardsSubscription -timeout 180m
=== RUN   TestAccAWSSecurityHub_serial
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/basic
=== RUN   TestAccAWSSecurityHub_serial/StandardsSubscription/disappears
--- PASS: TestAccAWSSecurityHub_serial (39.10s)
    --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription (39.10s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/basic (22.59s)
        --- PASS: TestAccAWSSecurityHub_serial/StandardsSubscription/disappears (16.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	42.508s
@ewbankkit ewbankkit merged commit 1d391eb into hashicorp:main Jul 13, 2021
@github-actions github-actions bot added this to the v3.50.0 milestone Jul 13, 2021
@Tensho Tensho deleted the securityhub-standard-controls branch July 13, 2021 20:51
@github-actions
Copy link

This functionality has been released in v3.50.0 of the Terraform AWS Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. provider Pertains to the provider itself, rather than any interaction with AWS. service/securityhub Issues and PRs that pertain to the securityhub service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS SecurityHub: the ability to disable specific compliance controls