-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Large ti db fixes #9369
Large ti db fixes #9369
Conversation
@microsoft-github-policy-service agree |
Sorry for all the commits - it seems all the validation tests are now working. |
Hi @blauwers, please update |
@v-rbajaj Updated - apologies for the oversight. |
I just looked at that PR, and it potentially has some issues in how it runs the summarize after filtering for active. It means that if TI is updated after the initial report, the query may generate a false positive by discounting the newer TI update marking the signal as inactive. Since I also fixed the validation issues, a better strategy may be to reject that PR and apply the changes here that make sense. I initially tried a similar approach as used in the other PR. |
@v-rbajaj I would be happy to port over relevant changes to this PR if that is helpful. |
Hi @blauwers, there are couple of challenges, we can't simply close that PR unless the other author agrees to it. And if you package this solution with 3.0.2 version than the other PR will have merge conflicts. Can we wait for the other PR to close and then work on this ? |
Hello @v-rbajaj, I am used to seeing code that passes the QA test going before code that does not. That said, the code in the other PR puts some things out of order in such a way it will break the accuracy of the TI queries. Specifically, the statement This PR also fixes the quality test that previously made analytic TI queries fail to validate - ergo why the tests are now passing, which is not addressed in the other PR. It would be faster to merge or port the changes from the other PR to this PR and fix the query issues simultaneously. Can we agree that this is an acceptable strategy? If so, I will start working on merging the PRs and making the necessary corrections. |
To illustrate the point:
outputs something else than the following
As you can see the latter causes false positives to show up. |
d4663cc
to
72f9b8b
Compare
Ok, I had to back all changes as merging @kfriede's last branch caused some trouble. I had to revert the commits and will be reprocessing those changes. |
Hi @blauwers, thanks for making these changes, will check and get back to you by 27 Nov 2023. |
Hi @blauwers, thanks for the changes. I was going through the PR and I noticed that you have made changes in Microsoft Defender XDR's Data Connector, so for that we would need you to repackage Microsoft Defender XDR using v3 tool. Apart from that I am reviewing the changes in Threat Intelligence. |
Hello @v-rbajaj, I updated the package as requested. |
Hi @blauwers, please resolve merge conflicts from the PR. |
Hello @v-rbajaj as requested, I have resolved the merge conflicts by merging my branch with an up to date master. |
Hi @blauwers, apart from that please add/update release notes for Microsoft Defender XDR To fix the failing ARM TTK, please update the maintemplate for Microsoft Defender XDR solution with the change mentioned below and after updating the maintemplate, update the 3.0.2 zip |
@v-rbajaj I made the requested changes. While doing so, I noticed I must not have re-merged the version bump for Microsoft Defender XDR, so I updated that to 3.0.2 again. Thank you for pointing out the issue with the empty arrays causing the ARM TTK to fail. How many issues have been in files generated by a packaging tool is odd. |
Required items, please complete
Change(s):
Reason for Change(s):
Version Updated:
Testing Completed:
Checked that the validations are passing and have addressed any issues that are present: