Replace DNS0 analyzers with DNS4EU. Closes #3154#3232
Replace DNS0 analyzers with DNS4EU. Closes #3154#3232mlodic merged 2 commits intointelowlproject:developfrom
Conversation
|
if you flag " I have provided the resulting raw JSON of a finished analysis and a screenshot of the results." and you don't do it...you won't get a review |
|
Hi @mlodic, apologies for the confusion. I mistakenly marked the checklist before the proof was ready. During testing, I discovered that the DNS4EU service requires the binary DoH protocol (RFC 8484) rather than a JSON API, which required a significant update to the analyzer logic. I have now pushed the fixes and updated the unit tests. I’ve also updated the PR description with the raw JSON and the screenshot showing successful results for both scanners. Thank you for your patience |
| "module": "dns.dns_resolvers.dns4eu_resolver.DNS4EUResolver", | ||
| "base_path": "api_app.analyzers_manager.observable_analyzers", | ||
| }, | ||
| "description": "Retrieve current domain resolution with DNS4EU DoH (DNS over HTTPS)", |
There was a problem hiding this comment.
please add markdown-based link to the service, they will be rendered in the GUI
There was a problem hiding this comment.
Done. I have updated the analyzer description in the migration file to include a markdown link to DNS4EU.
| "module": "dns.dns_malicious_detectors.dns4eu_malicious_detector.DNS4EUMaliciousDetector", | ||
| "base_path": "api_app.analyzers_manager.observable_analyzers", | ||
| }, | ||
| "description": "Check if a domain or an url is marked as malicious in DNS4EU database", |
There was a problem hiding this comment.
please add markdown-based link to the service, they will be rendered in the GUI
There was a problem hiding this comment.
Updated the description to include the markdown link to DNS4EU as requested
| }, | ||
| "name": "query_type", | ||
| "type": "str", | ||
| "description": "Query type against the chosen DNS resolver.", |
There was a problem hiding this comment.
add that the default is "A" here
There was a problem hiding this comment.
I've added "Default is A" to the parameter description in the migration.
|
|
||
| class Migration(migrations.Migration): | ||
| dependencies = [ | ||
| ("analyzers_manager", "0170_update_yaraify_archive"), |
There was a problem hiding this comment.
we just merged another analyzer in develop, this must be updated. Please pull changes from develop.
There was a problem hiding this comment.
I have rebased onto the latest develop branch and updated the migration dependency to 0172_analyzer_config_hibppasswords. The new migration is now numbered 0173.
| pass | ||
|
|
||
| url = "https://protective.joindns4.eu/dns-query" | ||
| # DNS4EU blocks by returning 0.0.0.0 or specific sinkhole IPs |
There was a problem hiding this comment.
comment is super useful, can you also add a link to the reference where you got these specific IP addresses?
There was a problem hiding this comment.
Added the reference link to the DNS4EU Protective DNS documentation in the source code comments where these sinkhole IPs are defined.
|
|
||
| # Use dnspython's https query which handles the binary protocol correctly | ||
| with httpx.Client(http2=True, headers=headers) as client: | ||
| response = dns.query.https(query, self.url, session=client, timeout=15) |
There was a problem hiding this comment.
the current soft time limit you set in the migration is 30. Align this value to that.
There was a problem hiding this comment.
Aligned the query timeout to 30 seconds to match the soft_time_limit set in the migration.
| break | ||
|
|
||
| except dns.exception.Timeout: | ||
| logger.warning(f"DNS4EUMaliciousDetector timeout for {observable}") |
There was a problem hiding this comment.
this should still make the analysis result to fail, it is not needed to be handled
There was a problem hiding this comment.
Removed the timeout handling here as well; it will now naturally fail the analysis on timeout
| } | ||
|
|
||
| with httpx.Client(http2=True, headers=headers) as client: | ||
| response = dns.query.https(query, self.url, session=client, timeout=15) |
There was a problem hiding this comment.
the current soft time limit you set in the migration is 30. Align this value to that.
There was a problem hiding this comment.
Done. All DNS4EU queries now use the 30s timeout defined in the migration
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class DNS4EUResolver(classes.ObservableAnalyzer): |
There was a problem hiding this comment.
a lot of the code is shared between the 2 classes. Please try to use a base class to avoid code duplication
There was a problem hiding this comment.
I've implemented a DNS4EUBase class to handle common logic for observable domain extraction and DoH query execution. Both analyzers now inherit from this base.
| for analyzer in analyzer_objs: | ||
| pc.analyzers.add(analyzer) | ||
| pc.full_clean() | ||
| pc.save() |
There was a problem hiding this comment.
can you please execute the Dns playbook once and provide the results?
0acc030 to
1e5687c
Compare
|
Hi @mlodic, I have addressed all your feedback and requested changes:
I’ve also attached the proof of work (Dns playbook results) in the comments above. Everything is ready for your review. Thank you! |
mlodic
left a comment
There was a problem hiding this comment.
good job and thanks for answering to all my points, that's very helpful.
Last things and we are gtg
| if is_malicious: | ||
| break | ||
|
|
||
| except AnalyzerRunException: |
There was a problem hiding this comment.
this try/except is not necessary because the framework already handle it under the hood
There was a problem hiding this comment.
You're absolutely right. I have removed the redundant try/except block since the framework handles AnalyzerRunException automatically.
| resolutions.append(element) | ||
|
|
||
| except AnalyzerRunException: | ||
| raise |
There was a problem hiding this comment.
Corrected. I've removed the unnecessary error handling logic here as well.
| self.client.force_authenticate(user=self.user) | ||
| response = self.client.get(f"{self.URL}/Dns/plugin_config") | ||
| self.assertEqual(response.status_code, 404, response.content) | ||
| # This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl |
There was a problem hiding this comment.
Idk what happened but this change should not be here. Can you please remove this?
There was a problem hiding this comment.
Apologies for the confusion! I had accidentally duplicated some headers during the rebase. I have cleaned the file and removed all stray lines. It is now restored to its proper state
tests/intel_owl/test_tasks.py
Outdated
| }, | ||
| ], | ||
| ) | ||
| import datetime |
There was a problem hiding this comment.
Idk what happened but this change should not be here. Can you please remove this?
There was a problem hiding this comment.
Fixed. I've removed the extra imports and stray code at the end of the file. Thank you for catching that!
1e5687c to
39b1b57
Compare
|
Hi @mlodic, I have addressed the final points regarding redundant error handling and cleaned up the test files. I also squashed these fixes into the main commit. Ready for final review |
|
@NikhilRaikwar could you also fix the last Deepsource style issues? |
39b1b57 to
8584477
Compare
|
hey @mlodic, I have fixed the final DeepSource style issues |
|
@NikhilRaikwar it seems there are some few regressions in some tests that still refer to the old analyzers. Could you take those into account? It's normal to have some back and forth for these kind of refactors, thanks for keeping up |
b2e4f61 to
b80b32b
Compare
|
Hi! @mlodic, I have addressed all the regressions and feedback. The PR is now ready for a final review:
All backend tests should be passing now. Thanks for your patience during this refactor! |
| # See the file 'LICENSE' for copying permission. | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING, Type |
There was a problem hiding this comment.
this problem regarding these files that seem to be completely replaced appeared again.
I appreciate your diligence about squashing the commits but that is helpful only before asking for the first review so that everything is in a single place for the reviewer. When some changes are requested, it is way better to just add a single commit for each isolated change so it is possible to actually track what you changed. I understand that you described them but still is very difficult to me to find the changes in the code.
If you can, revert the last change and re-apply the new changes in a new commit
There was a problem hiding this comment.
Hi @mlodic! Apologies for the confusion with the squashed commit. I have followed your advice and restored the previous state, then applied the new fixes as a separate commit:
- Commit "Replace DNS0 with DNS4EU": Restored and rebased the original migration commit to match the previous review state.
- Commit "Fix test regressions...": Contains all the new fixes documented earlier (Capa mocking, view test modernization using dynamic IDs, and UltraDNS error handling).
Regarding the "completely replaced" files: this appears to be caused by the pre-commit hooks (Black/Ruff) automatically reformatting the files I touched. By keeping these in a separate commit now, you should be able to see the specific changes more easily.
This PR is now ready for final review!
b80b32b to
e8f82ee
Compare
…NS error handling This commit addresses the regressions found during review: - Modernized tests for analyzers, connectors, visualizers, and ingestors to use dynamic ID lookups instead of hardcoded primary keys. - Fixed failing test_capa_info.py by mocking GitHub API requests. - Updated test_tasks.py and playbook views to correctly reference DNS4EU analyzers. - Improved error handling in UltraDNSMaliciousDetector to handle NXDOMAIN and NoAnswer cases.
e8f82ee to
9c53972
Compare
|
still the last commit doesn't clearly show the changes but it is fine, thanks |
|
ah another thing, @NikhilRaikwar can you please update the documentation too ? can you raise a PR in the docs repo and change the references there? |
|
Thanks for merging! @mlodic I’ll get started on the documentation PR in the docs repository right away to update all references from DNS0.eu to DNS4EU. |
|
Hi @mlodic! As requested, I have created the documentation Pull Request in the docs repository to update all references from DNS0.eu to DNS4EU: Docs PR: intelowlproject/docs/pull/48 I have updated the usage.md file with the correct analyzer names and linked them to the official DNS4EU website. |
|
thanks! |

Description
This PR replaces the legacy DNS0 resolvers with the new DNS4EU (Whalebone) analyzers. It includes a standard DNS resolver and a malicious domain detector.
Technical Detail: The initial implementation was failing because the DNS4EU service endpoints do not support the DoH JSON API. I have updated the implementation to use the binary DoH protocol (RFC 8484) via the
dnspythonlibrary andhttpx(HTTP/2) to ensure full compatibility.Closes #3154
Type of change
Checklist
developdumpplugincommand and added it in the project as a data migration. ("How to share a plugin with the community")test_files.zipand you added the default tests for that mimetype in test_classes.py.FREE_TO_USE_ANALYZERSplaybook by following this guide.urlthat contains this information. This is required for Health Checks (HEAD HTTP requests).get_mocker_response()method of the unittest class. This serves us to provide a valid sample for testing.DataModelfor the new analyzer following the documentation# This file is a part of IntelOwl https://github.com/intelowlproject/IntelOwl # See the file 'LICENSE' for copying permission.Ruff) gave 0 errors. If you have correctly installed pre-commit, it does these checks and adjustments on your behalf.testsfolder). All the tests (new and old ones) gave 0 errors.DeepSource,Django Doctorsor other third-party linters have triggered any alerts during the CI checks, I have solved those alerts.Important Rules
Proof of Work
Raw JSON (google.com - Job 12 )
{ "id": 12, "status": "reported_without_fails", "observable_name": "google.com", "analyzer_reports": [ { "name": "DNS4EU", "status": "SUCCESS", "report": { "observable": "google.com", "resolutions": [ { "TTL": 18, "data": "142.250.184.238", "name": "google.com.", "type": 1 } ] }, "errors": [] }, { "name": "DNS4EU_Malicious_Detector", "status": "SUCCESS", "report": { "malicious": false, "observable": "google.com" }, "errors": [] } ] }Screenshot