Just initialize this new branch for Cohesity internal collaboration/r… #2

yinghuang123 · 2022-12-02T23:57:01Z

…eview.

Required items, please complete

Change(s):

See guidance below

Reason for Change(s):

See guidance below

Version Updated:

Required only for Detections/Analytic Rule templates
See guidance below

Testing Completed:

See guidance below

Checked that the validations are passing and have addressed any issues that are present:

See guidance below

Guidance <- remove section before submitting

Before submitting this PR please ensure that you have read the following sections and filled out the changes, reason for change and testing complete sections:

Thank you for your contribution to the Microsoft Sentinel Github repo.

Details of the code changes in your submitted PR. Providing descriptions for pull requests ensures there is context to changes being made and greatly enhances the code review process. Providing associated Issues that this resolves also easily connects the reason.

Change(s):

Updated syntax for XYZ.yaml

Reason for Change(s):

New schema used for XYZ.yaml
Resolves ISSUE Add Logstash required version to Readme file Azure/Azure-Sentinel#1234

Version updated:

Yes
Detections/Analytic Rule templates are required to have the version updated

The code should have been tested in a Microsoft Sentinel environment that does not have any custom parsers, functions or tables, so that you validate no incorrect syntax and execution functions properly. If your submission requires a custom parser or function, it must be submitted with the PR.

Testing Completed:

Yes/No/Need Help

Note: If updating a detection, you must update the version field.

Before the submission has been made, please look at running the KQL and Yaml Validation Checks locally.
https://github.com/Azure/Azure-Sentinel#run-kql-validation-locally

Checked that the validations are passing and have addressed any issues that are present:

Yes/No/Need Help

Note: Let us know if you have tried fixing the validation error and need help.

References:

Guidance for Detection checks

General contribution guidance

PR validation troubleshooting

eerus

@yinghuang123 , I finished my review. Please submit changes before you submit another round of reviews to Microsoft.

@ishitagupta25 , please chime in, too. In a few comments I specifically tagged you if I think that you should do the change.

Solutions/CohesitySecurity/Analytic Rules/RansomwareAlert.yaml

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/local.settings.json

eerus · 2022-12-05T21:07:50Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

+         * need to rewrite GetAccessTokenAsync function
+         * as it uses obsolete technology to get the bearer token.
+         */
+        internal async Task<string> GetAccessTokenAsync(string uri)


Rewrite GetAccessTokenAsync function as it uses obsolete technology to get the bearer token. Here's the migration guide to new authentication functions. Please remember that the new authentication mechanism may nit need TenantID, ClientID and and ClientKey, so you'll also have to apply changes in local.settings.json and the corresponding ARM templates.

put this one to lower priority.

You can also check how other pieces of code authenticate, e.g. https://github.com/cohesity/Azure-Sentinel/blob/CohesitySecurity.internal/Tools/Sample%20Code/AzureSentinel-ManagementAPICsharp/AzureSentinel_ManagementAPI/Incidents/IncidentsController.cs.
Also, check how we can put all secrets to the KeyVault to enable key rotation (see example at https://github.com/cohesity/Azure-Sentinel/tree/CohesitySecurity.internal/DataConnectors/AzureStorage#readme)

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

Solutions/CohesitySecurity/readme.md

Tools/Create-Azure-Sentinel-Solution/input/Solution_CohesitySecurity.json

eerus

Deleted comments about pre-built binaries

eerus

@yinghuang123 , I walked through your changes. The code definitely looks better in terms of handling exceptions and parallelism. I left only a few comments.

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

eerus · 2022-12-12T15:26:48Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

+                log.LogError("URI --> " + URI);
+                log.LogError("body --> " + body);
+                log.LogError("ex --> " + ex.Message);
+                throw new Exception();


Move it outside of the catch block, i.e. outside of the exception handler. Place it after the line 71. This way, it'll be clearer that you're intentionally re-throwing the exception if the code didn't reach ln 63: return result. Also, it'd be great to explain in the comments why you're doing it and how to customize your queue wait and retry times.

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/host.json

eerus · 2022-12-12T15:38:18Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

+        }
+
+        [FunctionName("IncidentConsumer")]
+        [FixedDelayRetry(5, "00:05:00")]


I'd move these parameters to host.json file together with other settings (see my another comment in host.json).

.script/tests/KqlvalidationsTests/CustomTables/HeliosAuditNativePoller_CL.json

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentConsumer/IncidentConsumer.cs

eerus

Please address the comment about exposing extra-parameters and then please switch to updating the Restore From Last Snapshot playbook.
Other comments can be addressed later.

eerus · 2022-12-16T15:55:17Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/local.settings.json

+        "CohesityQueueName": "your CohesityQueueName",
+        "DefaultEndpointsProtocol": "your DefaultEndpointsProtocol",
+        "AccountKey": "your AccountKey",
+        "BlobEndpoint": "https://127.0.0.1:10000/devstoreaccount1",


I don't think that you use the following parameters anywhere.

"BlobEndpoint": "https://127.0.0.1:10000/devstoreaccount1"

"QueueEndpoint": "https://127.0.0.1:10001/devstoreaccount1"

"TableEndpoint": "https://127.0.0.1:10002/devstoreaccount1"

Please remove them.

eerus · 2022-12-16T15:58:03Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/local.settings.json

+    "IsEncrypted": false,
+    "Values": {
+        "AzureWebJobsStorage": "UseDevelopmentStorage=true",
+        "apiKey": "11111111-2222-3333-4444-555555555555",


Come up with a single standard for naming variables and stick to it. For ex, apiKey is written in camel case starting from the lowercase letter, while CohesityQueueName starts from the uppercase letter.

eerus · 2022-12-16T16:11:27Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            }
+            else
+            {
+                throw new InvalidProgramException("Invalid format string");


Make the error message clearer and more specific, e.g. "Invalid storage connection string."

eerus · 2022-12-16T16:15:13Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            try
+            {
+                const string container = "extra-parameters";
+                string Blob = incidentID + "\\" + param; // ID unique to the incident


Nit: Come up with a single way for variable naming and stick to it. For ex, Blob starts from the uppercase letter and blobStorageKeys from the lowercase one.

These 2 variables can be made class variables and can be initialized only once in RunAsync. There's no need to re-initialize them every time in every function call as they never change.

var blobStorageConnectionString = Environment.GetEnvironmentVariable("BlobStorageConnectionString"); var blobStorageKeys = Environment.GetEnvironmentVariable("BlobStorageAccountKeys");

Also, after your remove the variables above to a different place, will you still need TextWriter function for just 2 lines of code? Probably, it'd be better to remove the function and just directly call
WriteData(incidentID + "\\" + param, value);

eerus · 2022-12-16T16:18:47Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+
+                foreach (var alert in alerts)
+                {
+                    await ParseAlertToQueueAsync(outputQueueItem, alert, log);


Why are you doing it sequentially now? There was a code here that did all iterations in parallel.

eerus · 2022-12-16T16:21:45Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+                    else
+                        output.properties.severity = "Medium";
+                    break;
+                case 0:


Nit: Sort the case statements in ascending order for a better readability, like this
case 0:
case 4:
case 7:
case 10:
case 11:

eerus · 2022-12-16T16:24:13Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            {
+                switch (i)
+                {
+                case 12:


Nit: For a better readability, put case 12 statement to the end of switch as it's easier to read when the values go in ascending order, especially if we plan to extend the amount of extra info.

eerus · 2022-12-16T16:39:51Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+        {
+            try
+            {
+                const string container = "extra-parameters";


The container name can't be hardcoded as users are supposed to create it prior deployment.

Create an environment variable with the container name. Make the default value equal to "cohesity-extra-parameters".

Make sure that the corresponding playbooks are getting it as a parameter as well.

The corresponding readme-files should be updated with the instructions about creating containers and entering container names during deployment.

I also think that this constant can be initialized only once, e.g. in RunAsync or when the object is created. You can make it a class variable as there's no need to re-initialize it multiple times with every function call.

eerus · 2022-12-16T16:55:29Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            {
+                var db = Connection.GetDatabase();
+                string apiKey = Environment.GetEnvironmentVariable("apiKey");
+                string redisKey = Environment.GetEnvironmentVariable("workspace") + apiKey;


With introducing the storage blob we don't really need storing timestamps in Redis anymore. We can re-use the same container and create there a separate blob with the name equal to Environment.GetEnvironmentVariable("workspace") + apiKey.

It'll reduce monthly bill for the customer.

take back the multi-thread implementation of ParseAlertToQueue

eerus

Everything looks good. I left a few more nit comments.

eerus · 2022-12-16T18:49:37Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            }
+        }
+
+        private static void WriteData(string storageConnectionString, string path, string data)


storageConnectionString doesn't change. There's no need to pass it all the time. You can get the value only once at the object creation time or in RunAsync.
private static void WriteData(string path, string data)

eerus · 2022-12-16T18:51:11Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+        {
+            if (CloudStorageAccount.TryParse(storageConnectionString, out var storageAccount))
+            {
+                var cloudBlobClient = storageAccount.CreateCloudBlobClient();


The values of these variables don't change much. You can easily set them up during object initialization or in RunAsync.

var cloudBlobClient = storageAccount.CreateCloudBlobClient(); var container = cloudBlobClient.GetContainerReference(containerName);

eerus · 2022-12-16T18:55:52Z

DataConnectors/CohesitySecurity/Helios2Sentinel/IncidentProducer/IncidentProducer.cs

+            try
+            {
+                const string container = "extra-parameters";
+                string Blob = incidentID + "\\" + param; // ID unique to the incident


Also, after your remove the variables above to a different place, will you still need TextWriter function for just 2 lines of code? Probably, it'd be better to remove the function and just directly call
WriteData(incidentID + "\\" + param, value);

…st_Snapshot update to read the details from blob storage.

Updated instructions for SNOW playbook to create automation rule for closing SNOW tickets.

Updated info related to using the KeyVault to store the API Key.

rename to "Solutions/CohesitySecurity/Data\\ Connectors/Helios2Sentinel update to use keyvault instead of apiKey from env. rename some playbooks.

…o CohesitySecurity.internal

Typo

rename to Solutions/CohesitySecurity/Data Connectors/

…dent/azuredeploy.json +++ b/Solutions/CohesitySecurity/Playbooks/Cohesity_Close_Helios_Incident/readme.md

Added specific name for the vault

Added specific name for the playbook

- Fixed broken links - Added info about the Cohesity Close Helios Incident playbook - Did some formatting

- Replaced Helios with DataHawk + a few other formatting changes.

- Added info about creating an automation trigger for closing SNOW tickets

Add info about skipping steps if the API key is already stored in the vault.

Typo.

Fixed broken links

Added connection authorization instructions

Added instructions about connection authorization

Test Azure func deploy

Update 3.0.0.zip of Infoblox Solution

Corrected URL

Adding queries to look for abnormal sch task creation and launch

…-files adding gcp firewall terraform file for quick setup

…torUpdate Updated azuredeploy template of Rubrik Data Connector to prevent public access and cross tenant replicaion in storage account

Update ingestASimSampleData.py

Bumps [System.Text.Json](https://github.com/dotnet/runtime) from 8.0.4 to 8.0.5. - [Release notes](https://github.com/dotnet/runtime/releases) - [Commits](dotnet/runtime@v8.0.4...v8.0.5) --- updated-dependencies: - dependency-name: System.Text.Json dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]>

Fixing "Invalid Login Endpoint" issue

Added TeamCymruScout Solution

…ure-function-v4 Cybersixgill Actionable Alerts Python Version Upgrade

…tors/O365-DataCSharp/Teams.CustomConnector.StorageHandler/System.Text.Json-8.0.5 Bump System.Text.Json from 8.0.4 to 8.0.5 in /DataConnectors/O365 DataCSharp/Teams.CustomConnector.StorageHandler

Updated code to fix the bug in CrowdStrike Replicator V2

Solutions/Team Cymru Scout/Data Connectors/TeamCymruScout/SharedCode/utils.py

+                    '{}(method={}) : "{}" field is not set in the environment please set '
+                    "the environment variable and run the app.".format(
+                        self.logs_starts_with,
+                        __method_name,
+                        label,
+                    )


To fix the problem, we should avoid logging the names of sensitive environment variables. Instead, we can log a generic message indicating that some required environment variables are missing without specifying which ones. This approach maintains the utility of the log message for debugging purposes while avoiding the exposure of potentially sensitive information.

Modify the validate_params method in Solutions/Team Cymru Scout/Data Connectors/TeamCymruScout/SharedCode/utils.py to log a generic error message when required environment variables are missing.

Ensure that the log message does not include the names of the missing environment variables.

eerus suggested changes Dec 5, 2022

View reviewed changes

eerus reviewed Dec 7, 2022

View reviewed changes

yinghuang123 force-pushed the CohesitySecurity.internal branch from 8463ba0 to 2bb8bdb Compare December 10, 2022 00:03

eerus reviewed Dec 12, 2022

View reviewed changes

eerus suggested changes Dec 16, 2022

View reviewed changes

parameterize the containerName

1a2df6f

take back the multi-thread implementation of ParseAlertToQueue

eerus reviewed Dec 16, 2022

View reviewed changes

yinghuang123 and others added 23 commits December 19, 2022 12:13

rename playbook from Incident_VM_Playbook to Cohesity_Restore_From_La…

8295c49

…st_Snapshot update to read the details from blob storage.

Update readme.md

2eb0306

Updated instructions for SNOW playbook to create automation rule for closing SNOW tickets.

Update readme.md

6d788fa

Updated info related to using the KeyVault to store the API Key.

rename from DataConnectors/CohesitySecurity/Helios2Sentinel

2e21ed5

rename to "Solutions/CohesitySecurity/Data\\ Connectors/Helios2Sentinel update to use keyvault instead of apiKey from env. rename some playbooks.

Merge remote-tracking branch 'cohesity/CohesitySecurity.internal' int…

94e4bdf

…o CohesitySecurity.internal

Update readme.md

ef5279c

Typo

rename from "Solutions/CohesitySecurity/Data\\ Connectors/

4923932

rename to Solutions/CohesitySecurity/Data Connectors/

fix the extra "\" in readme files.

fffc00b

+++ b/Solutions/CohesitySecurity/Playbooks/Cohesity_Close_Helios_Inci…

864b714

…dent/azuredeploy.json +++ b/Solutions/CohesitySecurity/Playbooks/Cohesity_Close_Helios_Incident/readme.md

Update readme.md

d48f6f2

Added specific name for the vault

Update readme.md

5b8a57d

Added specific name for the playbook

+ "Playbooks/Cohesity_Close_Helios_Incident/azuredeploy.json",

c371a46

Update readme.md

617395e

- Fixed broken links - Added info about the Cohesity Close Helios Incident playbook - Did some formatting

Update readme.md

0249a43

- Replaced Helios with DataHawk + a few other formatting changes.

Update readme.md

829395b

- Added info about creating an automation trigger for closing SNOW tickets

Update readme.md

6b73ea2

Add info about skipping steps if the API key is already stored in the vault.

Update readme.md

d9a069e

Typo.

Update readme.md

138ce81

Fixed broken links

Update readme.md

c454187

Fixed broken links

Update readme.md

4e38c1c

Fixed broken links

Update readme.md

7b950bc

Added connection authorization instructions

Update readme.md

db241de

Added instructions about connection authorization

Create func_azuredeploy.json

0d2c1d1

Test Azure func deploy

v-prasadboke and others added 27 commits October 7, 2024 18:48

Update 3.0.0.zip

494966a

Update ReleaseNotes.md

71cefa3

Merge pull request Azure#11228 from Azure/v-prasadboke-infoblox

b3d32db

Update 3.0.0.zip of Infoblox Solution

Merge pull request Azure#11226 from dridderhof/patch-2

7d62f5e

Corrected URL

Merge pull request Azure#11224 from JouniMi/Scheduled_tasks

6adb982

Adding queries to look for abnormal sch task creation and launch

Update ingestASimSampleData.py

c65a692

Updated code to fix the bug in CrowdStrike Replicator V2

389bbe4

to run the stuck validations

e885036

Updated_URL

d48632f

Merge pull request Azure#11223 from idoshabi07/gcp-firewall-terraform…

da1102f

…-files adding gcp firewall terraform file for quick setup

Update O365APItoAS-Template.zip

e5623cf

Merge pull request Azure#11209 from niralishah-crest/RubrikDataConnec…

e7380c9

…torUpdate Updated azuredeploy template of Rubrik Data Connector to prevent public access and cross tenant replicaion in storage account

Update ingestASimSampleData.py

180ea6a

Merge pull request Azure#11230 from Azure/updatescriptingestion01

3bc4d79

Update ingestASimSampleData.py

Merge branch 'master' into v-sudkharat/FixScriptError

767c46e

Merge pull request Azure#11227 from Azure/v-sudkharat/FixScriptError

bb1458d

Fixing "Invalid Login Endpoint" issue

Merge pull request Azure#10939 from niralishah-crest/TeamCymruScout

84238cb

Added TeamCymruScout Solution

Merge pull request Azure#11173 from syed-loginsoft/cybersixgill-aa-az…

a7648a9

…ure-function-v4 Cybersixgill Actionable Alerts Python Version Upgrade

Merge pull request Azure#11237 from Azure/dependabot/nuget/DataConnec…

01e25df

…tors/O365-DataCSharp/Teams.CustomConnector.StorageHandler/System.Text.Json-8.0.5 Bump System.Text.Json from 8.0.4 to 8.0.5 in /DataConnectors/O365 DataCSharp/Teams.CustomConnector.StorageHandler

Merge branch 'master' into v-sabiraj-Crowdstrikebugfix

767cfe7

Merge pull request Azure#11231 from Azure/v-sabiraj-Crowdstrikebugfix

e1f4921

Updated code to fix the bug in CrowdStrike Replicator V2

corrected the request params for recovery

b43e1c8

corrected the request params for recovery

e53545b

updated package version

edda311

Merge branch 'CohesitySecurity' into CohesitySecurity.internal

92b5b01

Merge branch 'master' into CohesitySecurity.internal

a417c64

github-advanced-security bot found potential problems Oct 10, 2024

View reviewed changes

shishirdw added 2 commits October 17, 2024 13:27

corrected alert url

53bc1e6

updated function app zips with correct url

4940e55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Just initialize this new branch for Cohesity internal collaboration/r… #2

Just initialize this new branch for Cohesity internal collaboration/r… #2

yinghuang123 commented Dec 2, 2022

eerus left a comment

eerus Dec 5, 2022 •

edited

Loading

yinghuang123 Dec 8, 2022

eerus Dec 16, 2022

eerus left a comment

eerus left a comment

eerus Dec 12, 2022

eerus Dec 12, 2022

eerus left a comment

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus left a comment

eerus Dec 16, 2022

eerus Dec 16, 2022

eerus Dec 16, 2022

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Just initialize this new branch for Cohesity internal collaboration/r… #2

Are you sure you want to change the base?

Just initialize this new branch for Cohesity internal collaboration/r… #2

Conversation

yinghuang123 commented Dec 2, 2022

Guidance <- remove section before submitting

Before submitting this PR please ensure that you have read the following sections and filled out the changes, reason for change and testing complete sections:

eerus left a comment

Choose a reason for hiding this comment

eerus Dec 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerus left a comment

Choose a reason for hiding this comment

eerus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerus 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

eerus 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

eerus Dec 5, 2022 •

edited

Loading