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

Enable malware scanner - Defender for Storage #3784

Merged
merged 10 commits into from
Nov 20, 2023

Conversation

anatbal
Copy link
Contributor

@anatbal anatbal commented Nov 9, 2023

Resolves #3768

What is being addressed

Since Defender for storage has become publicly available, it required many changes to the existing airlock import infra in order to make it work. This PR automates the process, but here is a complete description of the process in order for customers that are already using it can enable it themselves.

  1. First, in the config.yaml, enable_airlock_malware_scanning: true must be set.
  2. The Microsoft Defender for Cloud should be enabled on "in progress" storage account, stalimip.
  3. When enabling it, set the settings as the following:
    image
    it's important to notice the event grid custom topic is publicly accessed since currently the scan results cannot be delivered over private endpoint.
  4. If you decide to create the custom event grid manually, there must be a subscription for a service bus queue that is called "scan-result". This will trigger the azure function to act upon the scanner results.
  • This solution was tested on a clean deployment and verified for both import and export
    @migldasilva, let me know if it's clear

Copy link

github-actions bot commented Nov 9, 2023

Unit Test Results

21 tests   21 ✔️  0s ⏱️
  1 suites    0 💤
  1 files      0

Results for commit 70267e9.

♻️ This comment has been updated with latest results.

@anatbal anatbal force-pushed the anatbal/3768-enable-malware-scanner branch from eef5303 to ef50fbd Compare November 9, 2023 14:01
@anatbal
Copy link
Contributor Author

anatbal commented Nov 10, 2023

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6822525447 (with refid e718abc7)

(in response to this comment from @anatbal)

@migldasilva
Copy link
Contributor

@anatbal I have just reviewed the code and observed that a private endpoint is not being created for Scanresult eventgrid topic. Is there a specific reason for that?

Copy link
Collaborator

@tamirkamara tamirkamara left a comment

Choose a reason for hiding this comment

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

Thanks for adding this. Just a few comments I think we need to sort.

airlock_processor/ScanResultTrigger/__init__.py Outdated Show resolved Hide resolved
airlock_processor/ScanResultTrigger/__init__.py Outdated Show resolved Hide resolved
core/terraform/airlock/storage_accounts.tf Outdated Show resolved Hide resolved
airlock_processor/_version.py Outdated Show resolved Hide resolved
core/version.txt Outdated Show resolved Hide resolved
core/terraform/main.tf Outdated Show resolved Hide resolved
core/terraform/variables.tf Show resolved Hide resolved
airlock_processor/_version.py Outdated Show resolved Hide resolved
airlock_processor/ScanResultTrigger/__init__.py Outdated Show resolved Hide resolved
@anatbal
Copy link
Contributor Author

anatbal commented Nov 13, 2023

@anatbal I have just reviewed the code and observed that a private endpoint is not being created for Scanresult eventgrid topic. Is there a specific reason for that?

Hey @migldasilva. Yes, since the scan result publishing is not supported over private endpoints

@anatbal
Copy link
Contributor Author

anatbal commented Nov 13, 2023

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6848039669 (with refid e718abc7)

(in response to this comment from @anatbal)

@migldasilva
Copy link
Contributor

@anatbal I have just deployed the this code and observed a small change in the overall behavior. After testing the whole import cycle, the blob container created inside stalimex<MY_ENVIRONMENT> is not deleted. Is not such containers expected to be deleted after moving files?

@anatbal
Copy link
Contributor Author

anatbal commented Nov 13, 2023

@anatbal I have just deployed the this code and observed a small change in the overall behavior. After testing the whole import cycle, the blob container created inside stalimex<MY_ENVIRONMENT> is not deleted. Is not such containers expected to be deleted after moving files?

You are right. I will dig into it and provide a code fix for that as well.

Copy link
Member

@marrobi marrobi left a comment

Choose a reason for hiding this comment

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

LGTM in principal, will try find time to test it out.

Can you update the docs to describe the setting and malware scanning setting etc where appropriate? Thanks.

@anatbal
Copy link
Contributor Author

anatbal commented Nov 19, 2023

@migldasilva I have fixed the deletion problem, the final version of this PR is also deleting the container as it should, and was tested.
@marrobi I added additional documentation where it was needed

@anatbal
Copy link
Contributor Author

anatbal commented Nov 19, 2023

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6921189385 (with refid e718abc7)

(in response to this comment from @anatbal)

@microsoft microsoft deleted a comment from github-actions bot Nov 19, 2023
@anatbal
Copy link
Contributor Author

anatbal commented Nov 19, 2023

/test-destroy-env

Copy link

Destroying PR test environment (RG: rg-tree718abc7)... (run: https://github.com/microsoft/AzureTRE/actions/runs/6921229057)

Copy link

PR test environment destroy complete (RG: rg-tree718abc7)

Copy link

Destroying branch test environment (RG: rg-tre015d31d4)... (run: https://github.com/microsoft/AzureTRE/actions/runs/6921229057)

Copy link

Branch test environment destroy complete (RG: rg-tre015d31d4)

@anatbal
Copy link
Contributor Author

anatbal commented Nov 19, 2023

/test-extended

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6921236648 (with refid e718abc7)

(in response to this comment from @anatbal)

3 similar comments
Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6921236648 (with refid e718abc7)

(in response to this comment from @anatbal)

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6921236648 (with refid e718abc7)

(in response to this comment from @anatbal)

Copy link

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/6921236648 (with refid e718abc7)

(in response to this comment from @anatbal)

@anatbal anatbal enabled auto-merge (squash) November 20, 2023 09:50
@anatbal anatbal merged commit d5cd77b into main Nov 20, 2023
10 checks passed
@anatbal anatbal deleted the anatbal/3768-enable-malware-scanner branch November 20, 2023 15:45
# If malware scanning is enabled, the fact that the blob was created can be dismissed.
# It will be consumed by the malware scanning service
logging.info('Malware scanning is enabled. no action to perform.')
send_delete_event(dataDeletionEvent, json_body, request_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

I have tested this version and it's triggering a runtime error. The blob storage inside stalimip<TRE_ID> is being deleted twice. I could stop getting such errors moving this line to a if statement. Something like this:

if event_type is None or event_type != 'Microsoft.Storage.BlobDeleted':
    send_delete_event(dataDeletionEvent, json_body, request_id)
return

A few lines above I checked if eventType is in the json_body, and if so, I assigned it the value store in the json_body.

I believe the deletion occur because, 1) the file is moved from storage account stalimip<TRE_ID> to a storage account inside the workspace, and it's the last blob in the storage account, and 2) the request status changed (I couldn't identify the correct order of execution).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@migldasilva I will test this in a separate PR. Will try to reproduce on my end, since this shouldnt be triggered from a 'Blob.Deleted' event....

@marrobi
Copy link
Member

marrobi commented Dec 12, 2023

/test-destroy-env

Copy link

Destroying branch test environment (RG: rg-tre015d31d4)... (run: https://github.com/microsoft/AzureTRE/actions/runs/7180394756)

Copy link

Branch test environment destroy complete (RG: rg-tre015d31d4)

Copy link

Destroying PR test environment (RG: rg-tree718abc7)... (run: https://github.com/microsoft/AzureTRE/actions/runs/7180394756)

Copy link

PR test environment destroy complete (RG: rg-tree718abc7)

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.

Enabling Malware scanning
4 participants