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

ENH: cymru expert: prevent extra lookups, handle invalid asnames #2352

Merged
merged 3 commits into from
May 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ CHANGELOG
------------------

### Core
- `intelmq.lib.utils`:
- `resolve_dns`: Deprecate dnspython versions pre-2.0.0 and disable search domains (PR#2352)
- Fixed not resetting destination path statistics in the stats cache after restarting bot (Fixes [#2331](https://github.com/certtools/intelmq/issues/2331))
- Force flushing statistics if bot will sleep longer than flushing delay (Fixes [#2336](https://github.com/certtools/intelmq/issues/2336))

Expand All @@ -30,6 +32,9 @@ CHANGELOG
#### Experts
- `intelmq.bots.experts.sieve`:
- Allow empty lists in sieve rule files (PR#2341 by Mikk Margus Möll).
- `intelmq.bots.experts.cymru_whois`:
- Ignore AS names with unexpected unicode characters (PR#2352, fixes #2132)
- Avoid extraneous search domain-based queries on NXDOMAIN result (PR#2352)

#### Outputs

Expand Down
2 changes: 1 addition & 1 deletion debian/control
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ Depends: bash-completion,
cron,
jq,
python3-dateutil (>= 2.5),
python3-dnspython (>= 1.11.1),
python3-dnspython (>= 2.0.0),
python3-openssl,
python3-psutil (>= 1.2.1),
python3-redis (>= 2.10),
Expand Down
2 changes: 1 addition & 1 deletion docs/requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# SPDX-License-Identifier: AGPL-3.0-or-later

Sphinx>=3.2.1
dnspython>=1.11.1
dnspython>=2.0.0
psutil>=1.2.1
python-dateutil>=2.5
python-termstyle>=0.1.10
Expand Down
4 changes: 3 additions & 1 deletion intelmq/bots/experts/cymru_whois/_lib.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ def __asn_query_parse(text):
if items[4]:
# unicode characters need to be decoded explicitly
# with the help of https://stackoverflow.com/questions/60890590/
result['as_name'] = items[4].encode('latin1').decode('utf8')
as_name = items[4].encode('latin1', errors='ignore').decode('utf8')
if as_name:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this the "Avoid extraneous search domain-based queries on NXDOMAIN result"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the added periods in IP_QUERY and ASN_QUERY, actually.
Example: if you have a search domain of "lan" and resolving homeserver fails with NXDOMAIN, then the search domain will cause homeserver.lan. to be queried.
However, if, for instance, 1.2.3.4.origin4.cymru.com returns NXDOMAIN, then 1.2.3.4.origin4.cymru.com.lan. would be queried.
But, the added period at the end signifies that this is a fully qualified domain name and such queries should not be made.

result['as_name'] = as_name

return result
5 changes: 1 addition & 4 deletions intelmq/lib/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -928,9 +928,6 @@ def resolve_dns(*args, **kwargs) -> dns.resolver.Answer:

Parameters:
see: https://dnspython.readthedocs.io/en/stable/resolver-class.html#dns.resolver.Resolver.resolve
The `search` parameter is always set to True for compatibility with dnspython version 1.

"""
if dns.version.MAJOR < 2:
return dns.resolver.query(*args, **kwargs)
return dns.resolver.resolve(*args, **kwargs, search=True)
return dns.resolver.resolve(*args, **kwargs)
Comment on lines -934 to +933
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With that change the function is obsolete now. But that's something for a major version as it is a change in the interface.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you leave a ticket to remove it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to #1444

9 changes: 9 additions & 0 deletions intelmq/tests/bots/experts/cymru_whois/test_expert.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@
"source.as_name": "ACONET ACOnet Backbone, AT",
"time.observation": "2015-01-01T00:00:00+00:00",
}
UNEXPECTED_UNICODE = {"__type": "Event",
"source.asn": 266522}
UNKNOWN_IP = {"__type": "Event",
"source.ip": "255.255.255.210",
}


@test.skip_redis()
Expand Down Expand Up @@ -106,6 +111,10 @@ def test_overwrite(self):
self.run_bot(parameters={'overwrite': False})
self.assertMessageEqual(0, OVERWRITE_OUT)

def test_unexpected_unicode(self):
self.input_message = UNEXPECTED_UNICODE.copy()
self.run_bot()
self.assertMessageEqual(0, UNEXPECTED_UNICODE)

if __name__ == '__main__': # pragma: no cover
unittest.main()
14 changes: 0 additions & 14 deletions intelmq/tests/lib/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -368,20 +368,6 @@ def test_load_configuration_yaml_invalid(self):
with self.assertRaises(ScannerError):
utils.load_configuration(filename)

@unittest.mock.patch.object(utils.dns.version, "MAJOR", 1)
def test_resolve_dns_supports_dnspython_v1(self):
with unittest.mock.patch.object(utils.dns.resolver, "query") as query_mocks:
utils.resolve_dns("example.com", "any", other="parameter")

query_mocks.assert_called_once_with("example.com", "any", other="parameter")

@unittest.mock.patch.object(utils.dns.version, "MAJOR", 2)
def test_resolve_dns_supports_dnspython_v2(self):
with unittest.mock.patch.object(utils.dns.resolver, "resolve", create=True) as resolve_mocks:
utils.resolve_dns("example.com", "any", other="parameter")

resolve_mocks.assert_called_once_with("example.com", "any", other="parameter", search=True)

@skip_internet()
def test_resolve_dns_returns_answer(self):
answer = utils.resolve_dns("example.com")
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from setuptools import find_packages, setup

REQUIRES = [
'dnspython>=1.11.1',
'dnspython>=2.0.0',
'psutil>=1.2.1',
'python-dateutil>=2.5',
'python-termstyle>=0.1.10',
Expand Down