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

Add Locations to SuccessThenFail_DiffIP_SameUserandApp.yaml #8907

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

tduarte14
Copy link
Contributor

Added Success and Failed Locations
Fixed SuccessIPBlock to also consider IPV6

Required items, please complete

Change(s):

  • Added Success and Failed Locations
  • Fixed SuccessIPBlock to consider IPv6

Reason for Change(s):

  • Locations are very useful for whitelisting known activity
  • IPv6 are now more used than before, so it makes sense to have that into account

Version Updated:

  • Yes

Testing Completed:

  • Yes

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

  • Yes

Added Success and Failed Locations
Fixed SuccessIPBlock to also consider IPV6
@tduarte14 tduarte14 requested review from a team as code owners August 30, 2023 10:59
@v-atulyadav v-atulyadav added Solution Solution specialty review needed Analytic Rules labels Aug 30, 2023
@v-prasadboke
Copy link
Contributor

Hello @tduarte14, Thank you for raising this PR. We will examine this PR and update you about the same before 05 September, 2023.

@v-prasadboke
Copy link
Contributor

Hello @tduarte14, The description is crossing 255 character. Try to reduce it below the count.

"Detects a user’s successful and failed logins to an Azure App from different IPs within 10 mins. This could mean password guessing by an attacker. The query also uses UEBA logs for more context."

Please check if the above descriptions suits.

And also can you please share the results/ running screenshots of the detection, as we dont have the sample data to test it.

Thanks.

@tduarte14
Copy link
Contributor Author

Hello @tduarte14, The description is crossing 255 character. Try to reduce it below the count.

"Detects a user’s successful and failed logins to an Azure App from different IPs within 10 mins. This could mean password guessing by an attacker. The query also uses UEBA logs for more context."

Please check if the above descriptions suits.

And also can you please share the results/ running screenshots of the detection, as we dont have the sample data to test it.

Thanks.

Hi,

I haven't changed the description, just added the location fields and fixed the IPBlock to consider IPv6.

As for the results, this was never requested and I tested in many different environments, however cannot share them as they are in customer environments, so due to privacy, I cannot share it.

@v-prasadboke
Copy link
Contributor

Hello @tduarte14, As per new guidelines. Description of Hunting query and Analytic rule should be below 255 characters.
That's the reason I asked you to shorten the description.

So for Detection can you share sample data to test the content,
Thanks.

@v-prasadboke
Copy link
Contributor

Hello @tduarte14, Hope you are doing well. Can you please respond to the above requested changes.

@v-prasadboke
Copy link
Contributor

Hello @tduarte14, It's been a while we haven't heard from you. Please respond to the above mentioned changes & data.
Thanks.

@v-prasadboke
Copy link
Contributor

Hello @tduarte14, Please respond to the above comments.

Got Name and UPNSuffix back as someone removed it in last commit, although they were still expected for Entities match.
Cleaned the description to be until 250 characters.
@tduarte14
Copy link
Contributor Author

Apologies, but had no time these days to get back here.
Changes were now made.

I continue saying this, it's not my responsibility to change the description, it wasn't I that did that, it was done on the previous commit by someone when they added the UEBA sources, where description was "increased" in size.

I also fixed a problem of that commit where the person removed the line that contained the declaration of Name and UPNSuffix which are necessary for Entities match.

I cannot provide data for testing as it's customer data and I cannot share it as may understand for privacy concerns. Unless you want a big problem for Microsoft by requesting that.

The changes I made were heavily tested (more than the last commit that broke the query and nobody noticed that).

Thank you

Fix to version 2.1.5 previously commited to restore Name and UPNSuffix field declarations.
@v-prasadboke
Copy link
Contributor

Hello @tduarte14, Thank you for responding. That should be enough as you heavily affirm on testing and working on the analytic rule. As to maintain integrity, you are unable to share sample data then its ok. I'll just check for what the KQL validation is failing

@tduarte14
Copy link
Contributor Author

Hi Prasad, I noticed a bad copy paste on the last commit, so it was fixed and it's running checks again.
I enabled Azure DevOps to allow some checks that weren't going through (that's probably what you were talking about).

@v-atulyadav v-atulyadav merged commit 4aa1f0d into Azure:master Sep 14, 2023
31 checks passed
@tduarte14
Copy link
Contributor Author

Thanks everyone!

@tduarte14 tduarte14 deleted the patch-2 branch September 14, 2023 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Analytic Rules Solution Solution specialty review needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants