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

[ASIM: NXLog ASIM DNS PARSER UPDATES] #8516

Conversation

ghost
Copy link

@ghost ghost commented Jul 13, 2023

This PR addresses the feedback received from Microsoft / @devikamehra about contributing my changes to the ASIM DNS NXLog parser direcly, instead of as part of a solution.

CHANGES:

  • Updated schema version
  • Extended the list of known DNS query types
  • Trying to re-introduce the DvcIpAddr field
  • Removed deprecated fields

REASONS FOR CHANGE(S):

  • The previous version of the parser was over a year old
  • New schema versions were released since then
  • Aiming to sync up the two parser versions that are in the repository

VERSION UPDATED:

  • Yes, to version 0.5

TESTING COMPLETED:

  • Need Help / How can I test this unreleased parser in my environment?

RAN VALIDATIONS:

  • Yes, all validations passed successfully

This work is associated with NXLog's Jira Issue IN-366

This PR addresses the feedback received from Microsoft / @devikamehra about contributing my changes to the ASIM DNS NXLog parser direcly, instead of as part of a solution.

CHANGES:

- Updated schema version
- Extended the list of known DNS query types
- Trying to re-introduce the DvcIpAddr field
- Removed deprecated fields

REASONS FOR CHANGE(S):

- The previous version of the parser was over a year old
- New schema versions were released since then
- Aiming to sync up the two parser versions that are in the repository

VERSION UPDATED:

- Yes, to version 0.5

TESTING COMPLETED:

- Need Help / How can I test this unreleased parser in my environment?

RAN VALIDATIONS:

- Yes, all validations passed successfully


This work is associated with NXLog's Jira Issue IN-366

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
@ghost ghost requested review from a team as code owners July 13, 2023 07:56
@ghost
Copy link
Author

ghost commented Jul 13, 2023

Dear @devikamehra , as requested in PR 8154, these are the changes that I would like to contribute to the Microsoft-maintained, NXLog-related ASIM parsers.
Please let me know if there is anything that needs to be changed or updated.

@ghost
Copy link
Author

ghost commented Jul 19, 2023

@vakohl Could you please give me some feedback, to see if there are any necessary changes, that I'd need to make?

@ghost ghost mentioned this pull request Jul 20, 2023
@vakohl
Copy link
Contributor

vakohl commented Jul 20, 2023

@vakohl Could you please give me some feedback, to see if there are any necessary changes, that I'd need to make?

@jszigetvari-nxlog thankyou for your contributing in making Microsoft Sentinel better. I'll try to complete my review this week.

Can you please run the ASIM parser test functions for schema and data and attach both output with this PR? would be 4 file csv files, two for ASIM and two for vim.

Also, if possible please attach the sample logs as well, would help me with the review.

@ghost
Copy link
Author

ghost commented Jul 20, 2023

@vakohl Thanks for your feedback and kind words. Unfortunately I will have very little time in the upcoming few days to work on the tests. Perhaps next Monday, I can allot some time for this. Either way, I will let you know once I'm done with it, or get stuck somewhere.

@ghost
Copy link
Author

ghost commented Jul 21, 2023

@vakohl Hi Varun,

I tried saving the parsers into my workspace manually, by pasting the ParserQuery parts to the Sentinel Logs view/query editor, and saving them under a unique name, as functions. But unfortunately in case of the vim one, I get an error, and the save operation fails with:

Error saving function. Error code: InvalidOperationArgument. Error message: Invalid function parameters string. Encountered possible syntax errors: 'Missing expression (90,90)', 'Expected: ')' (91,93)', 'Expected: '{' (91,93)', 'Expected: '}' (91,93)'. Please see relevant documentation 'https://docs.microsoft.com/en-us/azure/kusto/query/'.

@v-rbajaj
Copy link
Contributor

Hi @jszigetvari-nxlog, thanks for sharing the information, we will get back soon on this.

  * added link pointing to the source of the DNS RR IDs

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
Parsers/ASimDns/Parsers/ASimDnsMicrosoftNXlog.yaml Outdated Show resolved Hide resolved
Parsers/ASimDns/Parsers/vimDnsMicrosoftNXlog.yaml Outdated Show resolved Hide resolved
Parsers/ASimDns/Parsers/vimDnsMicrosoftNXlog.yaml Outdated Show resolved Hide resolved
Parsers/ASimDns/Parsers/ASimDnsMicrosoftNXlog.yaml Outdated Show resolved Hide resolved
@vakohl
Copy link
Contributor

vakohl commented Jul 28, 2023

@vakohl Hi Varun,

I tried saving the parsers into my workspace manually, by pasting the ParserQuery parts to the Sentinel Logs view/query editor, and saving them under a unique name, as functions. But unfortunately in case of the vim one, I get an error, and the save operation fails with:

Error saving function. Error code: InvalidOperationArgument. Error message: Invalid function parameters string. Encountered possible syntax errors: 'Missing expression (90,90)', 'Expected: ')' (91,93)', 'Expected: '{' (91,93)', 'Expected: '}' (91,93)'. Please see relevant documentation 'https://docs.microsoft.com/en-us/azure/kusto/query/'.

Try replacing last line:
ASimDnsMicrosoftNXLog (starttime=datetime(null), endtime=datetime(null), srcipaddr='', domain_has_any=dynamic([]), responsecodename='', response_has_ipv4='*', response_has_any_prefix=dynamic([]), eventtype='Query', disabled=false)

  * addressing review findings by vakohl (Thanks!)

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
@ghost ghost force-pushed the nxlog-asim-dns-parser-update-data-enrichment-schema-update branch from acc0609 to 77d399a Compare July 28, 2023 14:11
@ghost
Copy link
Author

ghost commented Jul 28, 2023

Try replacing last line:
ASimDnsMicrosoftNXLog (starttime=datetime(null), endtime=datetime(null), srcipaddr='', domain_has_any=dynamic([]), responsecodename='', response_has_ipv4='*', response_has_any_prefix=dynamic([]), eventtype='Query', disabled=false)

@vakohl Thank you Varun for your review comments! I will try your suggestion when I will have some time.
In the other cases, I will make the changes, once we agree on what to do exactly.

@ghost
Copy link
Author

ghost commented Aug 4, 2023

Try replacing last line: ASimDnsMicrosoftNXLog (starttime=datetime(null), endtime=datetime(null), srcipaddr='', domain_has_any=dynamic([]), responsecodename='', response_has_ipv4='*', response_has_any_prefix=dynamic([]), eventtype='Query', disabled=false)

@vakohl Hi Varun,

Thank you, your recommendation worked, I was able to save the vim parser with your changes.
I also installed and ran the parser tests, and generated the csv files for you. Please find them attached.
vimDnsMicrosoftNXlog_ASimSchemaTester_query_out.csv
vimDnsMicrosoftNXlog_ASimDataTester_query_out.csv
ASimDnsMicrosoftNXLog_ASimSchemaTester_query_out.csv
ASimDnsMicrosoftNXLog_ASimDataTester_query_out.csv

@ghost
Copy link
Author

ghost commented Aug 9, 2023

@vakohl Hello Varun,
I realized that in my previous comment the outputs for ASimDnsMicrosoftNXLog were wrong, as I used an older version of the function. I corrected that now and the outputs look a lot better. I included the outputs for both functions in this comment. Those can be regarded as the latest ones.

Please let us agree on the still open questions, and I will push the resulting corrections to this branch, and then hopefully we can move towards merging this PR.

ASimDnsMicrosoftNXLog_ASimSchemaTester_query_out.csv
ASimDnsMicrosoftNXLog_ASimDataTester_query_out.csv
vimDnsMicrosoftNXlog_ASimSchemaTester_query_out.csv
vimDnsMicrosoftNXlog_ASimDataTester_query_out.csv

@vakohl
Copy link
Contributor

vakohl commented Aug 9, 2023

@vakohl Hello Varun, I realized that in my previous comment the outputs for ASimDnsMicrosoftNXLog were wrong, as I used an older version of the function. I corrected that now and the outputs look a lot better. I included the outputs for both functions in this comment. Those can be regarded as the latest ones.

Please let us agree on the still open questions, and I will push the resulting corrections to this branch, and then hopefully we can move towards merging this PR.

ASimDnsMicrosoftNXLog_ASimSchemaTester_query_out.csv ASimDnsMicrosoftNXLog_ASimDataTester_query_out.csv vimDnsMicrosoftNXlog_ASimSchemaTester_query_out.csv vimDnsMicrosoftNXlog_ASimDataTester_query_out.csv

I'll take a look and respond by this week.

@ghost ghost mentioned this pull request Aug 12, 2023
@ghost
Copy link
Author

ghost commented Aug 18, 2023

@vakohl do you have any update on this?

@vakohl
Copy link
Contributor

vakohl commented Aug 19, 2023

@vakohl Hello Varun, I realized that in my previous comment the outputs for ASimDnsMicrosoftNXLog were wrong, as I used an older version of the function. I corrected that now and the outputs look a lot better. I included the outputs for both functions in this comment. Those can be regarded as the latest ones.
Please let us agree on the still open questions, and I will push the resulting corrections to this branch, and then hopefully we can move towards merging this PR.
ASimDnsMicrosoftNXLog_ASimSchemaTester_query_out.csv ASimDnsMicrosoftNXLog_ASimDataTester_query_out.csv vimDnsMicrosoftNXlog_ASimSchemaTester_query_out.csv vimDnsMicrosoftNXlog_ASimDataTester_query_out.csv

I'll take a look and respond by this week.

@jszigetvari-nxlog can you upload the result as mentioned in the guidelines here?
https://learn.microsoft.com/en-us/azure/sentinel/normalization-develop-parsers#test-results-submission-guidelines

@jszigetvari
Copy link

@vakohl Thanks for the review, I will start working on it on next Monday.

@ghost
Copy link
Author

ghost commented Sep 4, 2023

@vakohl After the latest corrections I tried to run both of these (separately)

ASimDnsMicrosoftNXLog | limit 10000 | invoke ASimDataTester('Dns')
vimDnsMicrosoftNXlog | limit 10000 | invoke ASimDataTester('Dns')

But they both returned no results. (The time range was set to the last 4 months, and I used this very same setting during the last round of testing.)

  * addressing latest review findings by vakohl
  * cleaned up non-normalized fields a bit further
  * removed fields/aliases that were marked as deprecated in the schema
  * removed obsolete DNS RR types
  * added new fields/aliases: SrcHostname and EventUid
  * added latest test outputs for the parsers
  * had some problems with running the data tests

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
  * hoping to clean up failing test

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
@ghost
Copy link
Author

ghost commented Sep 4, 2023

@vakohl Apparently some of the tests are failing, but to me they don't seem to be tightly related to the changes I've made. Could you please help in finding out what may be wrong?

@vakohl
Copy link
Contributor

vakohl commented Sep 5, 2023

But they both returned no results. (The time range was set to the last 4 months, and I used this very same setting during the last round of testing.)

when running the parser for testing results, you can use the filter/parameters you were using before i.e. giving specific values. But with PR submission, you should declare the filter as I suggested in one of my previous comment

@vakohl
Copy link
Contributor

vakohl commented Sep 6, 2023

@vakohl Apparently some of the tests are failing, but to me they don't seem to be tightly related to the changes I've made. Could you please help in finding out what may be wrong?

For validation errors, Please see https://dev.azure.com/azure/Azure-Sentinel/_build/results?buildId=64957&view=logs&jobId=2bbe2297-1b5b-54f3-6060-67e7b289f6c3&j=2bbe2297-1b5b-54f3-6060-67e7b289f6c3&t=293f4a23-e36d-5efa-3a4e-ea46d4ab6d83

Please add '_ItemId' field of type string in the custom table here: https://github.com/Azure/Azure-Sentinel/blob/d798b63ced54f34a701e4893e0066b857b4ced72/.script/tests/KqlvalidationsTests/CustomTables/NXLog_DNS_Server_CL.json

the other two fields showing in the error i.e Category and Level, we should not see error for them after removing from Project-away

@ghost
Copy link
Author

ghost commented Sep 6, 2023

Please add '_ItemId' field of type string in the custom table here: https://github.com/Azure/Azure-Sentinel/blob/d798b63ced54f34a701e4893e0066b857b4ced72/.script/tests/KqlvalidationsTests/CustomTables/NXLog_DNS_Server_CL.json

Thanks for pointing to the source of the problem. It really helped.
Fixed.

@ghost
Copy link
Author

ghost commented Sep 6, 2023

But they both returned no results. (The time range was set to the last 4 months, and I used this very same setting during the last round of testing.)

when running the parser for testing results, you can use the filter/parameters you were using before i.e. giving specific values. But with PR submission, you should declare the filter as I suggested in one of my previous comment

It turned out that there was a different problem, but I sorted it now, so I am submitting the latest results for the data tests too.

@ghost
Copy link
Author

ghost commented Sep 6, 2023

@vakohl All the necessary changes were carried out, and the tests are passing now.

  * addressing latest review findings by vakohl
  * re-added fields Category and Level
  * synchronized DNS RR types with the IANA list
  * corrected NXLog_DNS_Server_CL table schema for the KQL validation
    tests
  * added latest test outputs for the parsers

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
@ghost ghost force-pushed the nxlog-asim-dns-parser-update-data-enrichment-schema-update branch from 9d4d4e1 to da8b1f8 Compare September 6, 2023 06:32
vakohl
vakohl previously approved these changes Sep 6, 2023
@ghost
Copy link
Author

ghost commented Sep 6, 2023

@vakohl This has been an exhausing process. I assume it was the same for you. Thanks for your efforts!

@ghost ghost mentioned this pull request Sep 6, 2023
v-atulyadav
v-atulyadav previously approved these changes Sep 6, 2023
  * addressing latest review findings by vakohl
  * undid some of the recent changes to fields, like SrcHostname and Hostname
  * updated test results

Signed-off-by: Janos Szigetvari <janos.szigetvari@nxlog.org>
@ghost ghost dismissed stale reviews from v-atulyadav and vakohl via 1d4ad72 September 12, 2023 08:43
@vakohl
Copy link
Contributor

vakohl commented Sep 12, 2023

@jszigetvari-nxlog thankyou for your contribution. I'll approve this PR once the validation are through.

@ghost
Copy link
Author

ghost commented Sep 12, 2023

@jszigetvari-nxlog thankyou for your contribution. I'll approve this PR once the validation are through.

@vakohl The KqlValidations task failed for some reason, and I tried to look at the error(s) but the error log doesn't seem to be too helpful: https://dev.azure.com/azure/43fcaf68-ba3d-474b-97f3-7a1e6fbb5c8d/_apis/build/builds/65361/logs/155

@vakohl
Copy link
Contributor

vakohl commented Sep 12, 2023

@v-atulyadav can you please check the validation error?

@v-atulyadav
Copy link
Contributor

@vakohl, validations are cleared now.

@vakohl
Copy link
Contributor

vakohl commented Sep 12, 2023

@vakohl, validations are cleared now.

Thaknyou @v-atulyadav

@v-atulyadav v-atulyadav merged commit a187c5a into Azure:master Sep 13, 2023
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants