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

monoidic · 2023-04-19T13:34:22Z

This PR fixes #2132 by ignoring AS names with invalid characters, as suggested by Sebastian.
It also ensures that Cymru DNS queries that return NXDOMAIN do not cause any extraneous queries due to search domains.

~~(TODO: changelog, add unit test with invalid characters)~~

sebix · 2023-04-19T21:21:36Z

(TODO: changelog, add unit test with invalid characters)

Yes, a unit test for one of these special cases would be very good. For example the AS266522, which you mentioned in the issue

The fix looks good on first glance.

monoidic · 2023-04-20T08:21:10Z

Right. Well. Having the IP in the test event seemed to mess with things.

kamil-certat · 2023-04-20T14:35:14Z

intelmq/bots/experts/cymru_whois/_lib.py

+IP_QUERY = "%s.origin%s.asn.cymru.com."
+ASN_QUERY = "AS%s.asn.cymru.com."


If it's possible, it would be nice to provide a test for this change as well

The tests cover IP and ASN lookups.

It technically doesn't cover the search domain part.

This is what I meant :)

sebix · 2023-04-20T15:30:39Z

intelmq/bots/experts/cymru_whois/_lib.py

@@ -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:


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

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.

monoidic · 2023-04-24T09:06:41Z

The test explicitly sets the search domain to cert.ee., which causes the test to fail if attempts are made to parse the TXT response because the responses for all subdomains of cert.ee. return an SPF record in response to TXT queries.

It is not a perfect solution (it relies on the cert.ee. domain, and the result for the IP in the test being NXDOMAIN, and the parser raising an exception on TXT records with invalid formatting instead of gracefully handling it somehow).

However, I'm wondering, is there any legitimate use case for intelmq.lib.utils.dns.resolve_dns to force the use of search domains in the first place? What about updating the minimum dnspython version to 2.x and removing search=True altogether?

sebix · 2023-05-03T11:14:12Z

However, I'm wondering, is there any legitimate use case for intelmq.lib.utils.dns.resolve_dns to force the use of search domains in the first place? What about updating the minimum dnspython version to 2.x and removing search=True altogether?

yes, I think we can safely require dnspython >= 2.0 now.

I don't have any reason at hand why search is on. If the tests succeed with search disabled, let's go for it :)

sebix · 2023-05-15T09:13:16Z

@monoidic Is this PR ready to merge or do you want to change the DNS-related parts?

monoidic · 2023-05-15T11:12:46Z

I'd been busy with other projects and forgout about this.
The latest commit should have everything necessary to deprecate dnspython <2.0.0 and remove two tests which are now outdated.
If the changes are acceptable, then I will add changelog entries as well.

sebix

If the changes are acceptable, then I will add changelog entries as well.

Yes, please document the changed minimum required version of dnspython, this has impact on the supported OSs (dropping out Ubuntu 20.04, which need to be removed from the docs, from intelmq-vagrant and at the next release from the packages).

sebix · 2023-05-15T17:08:03Z

intelmq/lib/utils.py

-    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)


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

Could you leave a ticket to remove it?

Added to #1444

kamil-certat

Looks good. What I'm not happy about is that we rely on the real answers in tests instead of some mocking solution (and DNS is relatively unstable), but it's a general problem, nothing related to the change :)

kamil-certat · 2023-05-16T12:38:26Z

intelmq/lib/utils.py

-    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)


Could you leave a ticket to remove it?

Support for Debian 10 Buster was dropped in PR#2352 certtools#2352

sebix marked this pull request as draft April 19, 2023 21:19

sebix added bug Indicates an unexpected problem or unintended behavior component: bots labels Apr 19, 2023

monoidic marked this pull request as ready for review April 20, 2023 06:50

monoidic force-pushed the develop branch from 614e4fb to 786979f Compare April 20, 2023 08:04

monoidic marked this pull request as draft April 20, 2023 08:15

monoidic force-pushed the develop branch from 786979f to 08999cd Compare April 20, 2023 08:19

monoidic marked this pull request as ready for review April 20, 2023 08:21

kamil-certat reviewed Apr 20, 2023

View reviewed changes

sebix reviewed Apr 20, 2023

View reviewed changes

sebix requested a review from kamil-certat May 15, 2023 15:39

sebix requested changes May 15, 2023

View reviewed changes

monoidic added 3 commits May 16, 2023 14:05

ENH: cymru expert: handle invalid asnames

dba2a6b

TST: cymru: unexpected unicode in asname

49e232e

UPD: remove dnspython-1 compat

a0d3c14

monoidic force-pushed the develop branch from c20f543 to a0d3c14 Compare May 16, 2023 11:15

monoidic requested a review from sebix May 16, 2023 11:17

kamil-certat approved these changes May 16, 2023

View reviewed changes

sebix mentioned this pull request May 16, 2023

Deprecations and breaking changes in 4.0.0 #1444

Open

1 task

sebix merged commit 512200c into certtools:develop May 16, 2023
24 checks passed

sebix self-assigned this May 16, 2023

wagner-intevation pushed a commit to wagner-intevation/intelmq that referenced this pull request Sep 5, 2023

remove Debian 10 buster from tests and documentation

911598f

Support for Debian 10 Buster was dropped in PR#2352 certtools#2352

wagner-intevation mentioned this pull request Sep 5, 2023

Packaging fixes for Debian, clean deprecated buster from tests #2391

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

monoidic commented Apr 19, 2023 •

edited

Loading

sebix commented Apr 19, 2023 •

edited

Loading

monoidic commented Apr 20, 2023 •

edited

Loading

kamil-certat Apr 20, 2023

sebix Apr 20, 2023

monoidic Apr 20, 2023

kamil-certat Apr 20, 2023

sebix Apr 20, 2023

monoidic Apr 20, 2023

monoidic commented Apr 24, 2023 •

edited

Loading

sebix commented May 3, 2023

sebix commented May 15, 2023

monoidic commented May 15, 2023

sebix left a comment

sebix May 15, 2023

kamil-certat May 16, 2023

sebix May 16, 2023

kamil-certat left a comment

kamil-certat May 16, 2023

		IP_QUERY = "%s.origin%s.asn.cymru.com."
		ASN_QUERY = "AS%s.asn.cymru.com."

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

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

Conversation

monoidic commented Apr 19, 2023 • edited Loading

sebix commented Apr 19, 2023 • edited Loading

monoidic commented Apr 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monoidic commented Apr 24, 2023 • edited Loading

sebix commented May 3, 2023

sebix commented May 15, 2023

monoidic commented May 15, 2023

sebix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamil-certat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monoidic commented Apr 19, 2023 •

edited

Loading

sebix commented Apr 19, 2023 •

edited

Loading

monoidic commented Apr 20, 2023 •

edited

Loading

monoidic commented Apr 24, 2023 •

edited

Loading