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

Server vulnerability assessment resource #10030

Merged
merged 28 commits into from
Mar 9, 2021
Merged

Server vulnerability assessment resource #10030

merged 28 commits into from
Mar 9, 2021

Conversation

martenbohlin
Copy link
Contributor

Adds a new resource azurerm_server_vulnerability_assessment that installes the vulnerability assessment on a VM

@martenbohlin martenbohlin force-pushed the server_vulnerability_assessment_resource branch from 119d9ce to 3dc89c6 Compare January 1, 2021 20:27
@sharebear
Copy link
Contributor

Fun, I was just looking at the API for this yesterday to see how much work it would be to implement :)

This is getting towards a functional question rather than a PR comment, but have I understood correctly that you need to have activated Azure Defender for Servers enabled on the Subscription in order to use Qualys;

  1. Should there be a note about this in the documentation?
  2. How does the resource behave if the Subscription does not have Azure Defender enabled?
  3. Do the acceptance tests run in a Subscription that has Azure Defender enabled?

@martenbohlin
Copy link
Contributor Author

This is getting towards a functional question rather than a PR comment, but have I understood correctly that you need to have activated Azure Defender for Servers enabled on the Subscription in order to use Qualys;

  1. Should there be a note about this in the documentation?
  2. How does the resource behave if the Subscription does not have Azure Defender enabled?
  3. Do the acceptance tests run in a Subscription that has Azure Defender enabled?

Good questions. It would be interesting to know what would happen if Azure Defender is disabled. If I could get help to test that it would be great since the company I work for require defender to be enabled on all subscriptions. (Should be enough to run the test TestAccServerVulnerabilityAssessment_basic)

@martenbohlin
Copy link
Contributor Author

This is getting towards a functional question rather than a PR comment, but have I understood correctly that you need to have activated Azure Defender for Servers enabled on the Subscription in order to use Qualys;

  1. Should there be a note about this in the documentation?
  2. How does the resource behave if the Subscription does not have Azure Defender enabled?
  3. Do the acceptance tests run in a Subscription that has Azure Defender enabled?

@sharebear
I was able to test this my self. So here are the answers:

  1. I added a note in the documentation that Azure Defender must be enabled.
  2. You get the following error message back from the API that is show:Subscription is not in the Standard or Standard Trial subscription plan. Please upgrade to use this feature.
  3. I am guessing here, but there is tests for the resource azurerm_mssql_server_vulnerability_assessment so I belive Azure Defender is enabled where the integration tests run.

@sharebear
Copy link
Contributor

@sharebear
I was able to test this my self. So here are the answers:

  1. I added a note in the documentation that Azure Defender must be enabled.
  2. You get the following error message back from the API that is show:Subscription is not in the Standard or Standard Trial subscription plan. Please upgrade to use this feature.
  3. I am guessing here, but there is tests for the resource azurerm_mssql_server_vulnerability_assessment so I belive Azure Defender is enabled where the integration tests run.

Awesome, I think that's the best possible answer for these questions.

@martenbohlin martenbohlin mentioned this pull request Jan 10, 2021
Closed
@martenbohlin
Fixed feedback from neil-yechenwei on pull request hashicorp#10030.
a341ee1
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
85288dd 
…y_assessment_resource
@martenbohlin
Fixed error in documentation meta data.
29d2fc3
@martenbohlin
The refrenced bug was closed as duplicate, so update the reference to…
2bc6837 
… point to the correct one.
@martenbohlin
Added test asserting that server vulnerability assessment resource gi…
ed9f9c8 
…ves an error when a import is required.
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
f81be24 
…y_assessment_resource
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
828015d 
…y_assessment_resource
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
eb51b02 
…y_assessment_resource
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
9e233b7 
…y_assessment_resource
@martenbohlin
Revert changes to resourceid since it caused problems with other reso…
b19e769 
…urces.

This reverts commit 8e59bda
@martenbohlin
Changed location in documentation.
54b7f7d
@martenbohlin
Merge remote-tracking branch 'origin/master' into server_vulnerabilit…
3b7f573 
…y_assessment_resource
@martenbohlin
Fixed formating after merge.
412a716
katbyte
katbyte requested changes Mar 5, 2021
View reviewed changes
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @martenbohlin - i've given this a review and overall while its good i've let some comments inline to address before merge

azurerm/internal/services/securitycenter/server_vulnerability_assessment_resource.go Outdated Show resolved Hide resolved
website/docs/r/azurerm_server_vulnerability_assessment.html.markdown Outdated

```shell
terraform import azurerm_server_vulnerability_assessment.example /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/resource-group-name/providers/Microsoft.Compute/virtualMachines/vm-name/providers/Microsoft.Security/serverVulnerabilityAssessments/Default
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd like us to show both please

website/docs/r/azurerm_server_vulnerability_assessment.html.markdown Outdated
---
subcategory: "Security Center"
layout: "azurerm"
page_title: "Azure Resource Manager: azurerm_server_vulnerability_assessment"
Copy link
Collaborator

Choose a reason for hiding this comment

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

as this is in the security center service could we rename this to

Suggested change
page_title: "Azure Resource Manager: azurerm_server_vulnerability_assessment"
page_title: "Azure Resource Manager: azurerm_security_centre_server_vulnerability_assessment"

website/docs/r/azurerm_server_vulnerability_assessment.html.markdown Outdated

Manages an Azure Server Vulnerability Assessment (Qualys) to a VM.

-> **NOTE** Azure Defender has to be enabled on the subscription in order for this resource to work.
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we include a link here to the docs on how to get started with this?

@martenbohlin martenbohlin requested a review from katbyte March 6, 2021 21:11
@martenbohlin
Copy link
Contributor Author

Thanks for the feedback @katbyte ! I think all of your suggestions were great and have implemented them.

@manicminer
Copy link
Contributor

@martenbohlin I've tested this locally and it looks good. Could you resolve the merge conflict and I will retest? Thanks!

@manicminer
Copy link
Contributor

My bad, there wasn't a conflict after all, but thanks for merging in master.

manicminer
manicminer approved these changes Mar 9, 2021
View reviewed changes
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

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

Tests are passing, this looks good to merge

Screenshot 2021-03-09 at 20 13 54

@manicminer manicminer merged commit 7a273ca into hashicorp:master Mar 9, 2021
manicminer added a commit that referenced this pull request Mar 9, 2021
@manicminer
Update changelog for #10030
a81ccf5
@ghost
Copy link

ghost commented Apr 9, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@neil-yechenwei neil-yechenwei neil-yechenwei left review comments

@sharebear sharebear sharebear left review comments

@manicminer manicminer manicminer approved these changes

@katbyte katbyte Awaiting requested review from katbyte

Assignees
No one assigned
Labels
documentation service/security-center size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@martenbohlin @sharebear @neil-yechenwei @manicminer @katbyte