Just initialize this new branch for Cohesity internal collaboration/r… by yinghuang123 · Pull Request #2 · cohesity/Azure-Sentinel

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.

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

+         * 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)

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.

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

+                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.

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

+        }
+
+        [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).

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

+        "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

+    "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

+            }
+            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

+            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

+
+                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

+                    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

+            {
+                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

+        {
+            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

+            {
+                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.

eerus

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

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

+            }
+        }
+
+        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

+        {
+            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

+            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);

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

Initial version of the IncidentProducer Function App

./Solutions/CohesitySecurity/Playbooks/Cohesity_Close_Helios_Incident/azuredeploy.json ./Solutions/CohesitySecurity/Playbooks/Cohesity_CreateOrUpdate_ServiceNow_Incident/azuredeploy.json

+++ ./Solutions/CohesitySecurity/Package/createUiDefinition.json +++ ./Solutions/CohesitySecurity/Package/mainTemplate.json

Returned the reference to the Restore playbook

add Anomaly Strength in description re-organize the codes

eerus

Added a few optimization suggestions after getting rid of Redis.

eerus · 2022-12-22T03:52:21Z

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


The values for both of these variables never change after initialization. It's worth putting them outside of the function

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

eerus · 2022-12-22T03:53:30Z

+
+        private static void TextWriter(string incidentID, string param, string value, ILogger log)
+        {
+            WriteData(incidentID + "\\" + param, value, log);


It doesn't make sense to have a one-line function, TextWriter. Call WriteData directly.

eerus · 2022-12-22T03:54:55Z

+        "AzureWebJobsStorage": "UseDevelopmentStorage=true",
+        "apiKey": "11111111-2222-3333-4444-555555555555",
+        "startDaysAgo": "-30",
+        "connectStr": "your azure redis cache connection string",


Remove it as you don't have Redis dependency anymore.

eerus · 2022-12-22T03:56:29Z

+        "workspace": "your instance name",
+        "CohesityQueueName": "your CohesityQueueName",
+        "DefaultEndpointsProtocol": "your DefaultEndpointsProtocol",
+        "AccountKey": "your AccountKey",


Where do you use it? Remove it if you don't.

eerus · 2022-12-22T03:56:55Z

+        "DefaultEndpointsProtocol": "your DefaultEndpointsProtocol",
+        "AccountKey": "your AccountKey",
+        "BlobStorageConnectionString": "your BlobStorageConnectionString",
+        "BlobStorageAccountKeys": "your BlobStorageAccountKeys",


Where do you use it? Remove it if you don't.

eerus

New comments about redundant env variables

eerus · 2022-12-22T19:14:16Z

+    {
+        private static readonly object queueLock = new object();
+        private static Lazy<ConnectionMultiplexer> lazyConnection = CreateConnection();
+        private static string containerName = Environment.GetEnvironmentVariable("containerName");


There's no need to take it from the environment as it's created by deployment wizard and never changed. Set it equal to "cohesity-extra-parameters"

so, you means this change?

- private static string containerName = GetSecret("containerName"); + private static string containerName = "cohesity-extra-parameters";

Yes, but also can you move shared constants and functions to one place, so both producer and consumer can use them. For ex, you can create a shared library for it or reference from both projects.

eerus

Thoughts on queue name to simplify deployment configs

eerus · 2022-12-22T19:37:01Z

+        }
+
+        [FunctionName("IncidentConsumer")]
+        public void Run([QueueTrigger("%CohesityQueueName%", Connection = "AzureWebJobsStorage")]string queueItem, ILogger log)


Why did we decide to put the queue name to the environment variable? If the deployment wizard creates a storage account with a unique name, then we can just have a common constant between producer and consumer for the queue name

Here you can create a shared constant "cohesity-incidents" that both projects have access to.

eerus · 2022-12-22T19:37:14Z

+        }
+
+        public static async Task ParseAlertToQueue(
+            [Queue("%CohesityQueueName%"), StorageAccount("AzureWebJobsStorage")] ICollector<string> outputQueueItem,


Why did we decide to put the queue name to the environment variable? If the deployment wizard creates a storage account with a unique name, then we can just have a common constant between producer and consumer for the queue name

ARM template for deploying both Azure functions.

…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] <support@github.com>

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

This reverts commit 0beb141.

This reverts commit ef382fa.

This reverts commit 3ac2999.

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

eerus reviewed Dec 16, 2022

View reviewed changes

eerus and others added 20 commits December 19, 2022 21:59

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

Create incident_producer_azuredeploy.json

e13cc5a

Initial version of the IncidentProducer Function App

update these 2 Playbooks:

4520fcb

./Solutions/CohesitySecurity/Playbooks/Cohesity_Close_Helios_Incident/azuredeploy.json ./Solutions/CohesitySecurity/Playbooks/Cohesity_CreateOrUpdate_ServiceNow_Incident/azuredeploy.json

update these 2 package files.

2c7eb69

+++ ./Solutions/CohesitySecurity/Package/createUiDefinition.json +++ ./Solutions/CohesitySecurity/Package/mainTemplate.json

update the readme files.

8f0536e

Update readme.md

ac0ae88

Returned the reference to the Restore playbook

retire redis in IncidentProducer

979c7d3

add Anomaly Strength in description re-organize the codes

eerus reviewed Dec 22, 2022

View reviewed changes

Create azuredeploy.json

d3ef3bd

ARM template for deploying both Azure functions.

v-sudkharat and others added 17 commits October 8, 2024 14:16

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 AI found potential problems Oct 10, 2024

View reviewed changes

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

shishirdw and others added 12 commits October 17, 2024 13:27

corrected alert url

53bc1e6

updated function app zips with correct url

4940e55

Dedup Alerts

2ba8c77

zip packages for clientId changes

0beb141

Revert "zip packages for clientId changes"

ef382fa

This reverts commit 0beb141.

Reapply "zip packages for clientId changes"

3ac2999

This reverts commit ef382fa.

Revert "Reapply "zip packages for clientId changes""

c262c86

This reverts commit 3ac2999.

reverting to original zip files

f280b61

saving secret key changes for posterity

a8b3f99

zip package for removing default azure creds

852f5f7

switching from default azure creds to client id based

83d8736

switch from default creds to clied id with correct zip

d796b69

Uh oh!

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:

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerus Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerus Dec 5, 2022 •

edited

Loading

yinghuang123 Dec 23, 2022 •

edited

Loading