Opened 3 years ago

Closed 5 months ago

#18342 closed defect (fixed)

Provide more accurate reverse DNS results

Reported by: cypherpunks Owned by: irl
Priority: Medium Milestone: Onionoo 1.15.0
Component: Metrics/Onionoo Version:
Severity: Normal Keywords: metrics-2018
Cc: tyseom, metrics-team Actual Points:
Parent ID: Points:
Reviewer: karsten Sponsor:

Description

What DNS server does onionoo use for reverse DNS lookups to generate the host_name entries?

https://onionoo.torproject.org/details?search=SGGS

example, of onionoo's reverse lookup results for SG.GS relays, all IPs:

+----------+--------------+
| nickname | host_name    |
+----------+--------------+
| SGGSUK4  | 124.6.36.196 |
| SGGSHK0  | 124.6.32.230 |
| SGGSUK6  | 124.6.36.198 |
| SGGSUK0  | 124.6.36.230 |
| SGGSUK3  | 124.6.36.195 |
| SGGSUK7  | 124.6.36.199 |
| SGGSUK1  | 124.6.36.193 |
| SGGSUK8  | 124.6.36.200 |
| SGGSUK5  | 124.6.36.197 |
| SGGSUK2  | 124.6.36.194 |
| SGGSLAX0 | 124.6.40.230 |
| SGGSNYC0 | 124.6.44.230 |
| SGGSUK9  | 124.6.36.201 |
+----------+--------------+

compare that with
http://torstatus.blutmagie.de/index.php

Child Tickets

Change History (37)

comment:1 Changed 3 years ago by tyseom

Cc: tyseom added

comment:2 Changed 2 years ago by karsten

Here's how we're currently resolving addresses to domain names:

      String result = InetAddress.getByName(this.address).getHostName();

Are there better ways to do this?

comment:3 in reply to:  description Changed 2 years ago by cypherpunks

As of 2016-09-05 09:00 there are still about 37% (3233 out of 8592) of onionoo records without proper reverse DNS entries.

I assumed that the problem resides on the used upstream DNS server (more than the method to query it, since I thought that is a pretty standard procedure).

That is why I asked which DNS server you (onionoo.tpo) use:

Replying to cypherpunks:

What DNS server does onionoo use for reverse DNS lookups to generate the host_name entries?

comment:4 Changed 2 years ago by Sebastian

When discussing this bug, it was pointed out on IRC that these examples that were given in the original report are misconfigured. While the reverse dns for these IPs is set, forward entries don't exist. Perhaps this is an additional check that onionoo does (and I think it probably should do). The host uses hetzner's nameservers, but they can resolve the reverse dns just fine.

The TTL of the entries of those hosts is set to something very unreasonably low. If you are in any way responsible for the relays mentioned above, please fix their DNS config. This is not to say that there isn't a bug still somewhere in onionoo, but afaict, not for the hosts mentioned above.

comment:5 in reply to:  4 Changed 2 years ago by cypherpunks

Replying to Sebastian:

The TTL of the entries of those hosts is set to something very unreasonably low. If you are in any way responsible for the relays mentioned above, please fix their DNS config.

This ticket is not about rDNS entries of specific relays, these were merely examples, AFAIK these relays left the tor network a while ago:
https://lists.torproject.org/pipermail/tor-relays/2016-February/008807.html

While the reverse dns for these IPs is set, forward entries don't exist. Perhaps this is an additional check that onionoo does (and I think it probably should do).

If onionoo requires PTR and A DNS records to match please add that information to the description, currently it does not mention that in any way:

https://onionoo.torproject.org/protocol.html#details :

 host_name string optional

Host name as found in a reverse DNS lookup of the relay IP address. This field is updated at most once in 12 hours, unless the relay IP address changes. Omitted if the relay IP address was not looked up or if no lookup request was successful yet.

I care about rDNS records (even if PTR and A records don't match) because I'd like to use them for group detection for ornetradar and it is apparently very common that providers do not have A records for PTR records.
https://lists.riseup.net/www/info/ornetradar

comment:6 Changed 2 years ago by iwakeh

Milestone: Onionoo 3.1-1.0.0

Planned to be part of the first Onionoo release.

comment:7 Changed 2 years ago by iwakeh

Milestone: Onionoo 3.1-1.0.0Onionoo 3.1-1.1.0

The first Onionoo release should be kept small and as this seems still an open question I move this to the next milestone.

comment:8 Changed 2 years ago by iwakeh

Milestone: Onionoo 3.1-1.1.0

comment:9 Changed 15 months ago by karsten

Summary: onionoo has poor reverse DNS resultsProvide more accurate reverse DNS results

Make an summary more accurate (at least).

comment:10 Changed 15 months ago by karsten

Keywords: metrics-2018 added

comment:11 Changed 15 months ago by karsten

Owner: set to metrics-team
Status: newassigned

comment:12 Changed 13 months ago by irl

Java does indeed check to see that there is an A record that matches the PTR record when using InetAddress.getHostName().

The following would allow us to perform this "manually": https://docs.oracle.com/javase/8/docs/technotes/guides/jndi/jndi-dns.html

We could then flag whether or not the A record matched and return this information along with it.

comment:13 Changed 9 months ago by irl

Owner: changed from metrics-team to irl
Status: assignedaccepted

Some relevant discussion happened at #25551.

comment:14 Changed 9 months ago by cypherpunks

Ok, to summarize what I would like to see:

  • new field: dns_ptr Description: DNS PTR record of the relay's primary IP address. This field is updated at most once in 12 hours, unless the relay's primary IP address changes. Omitted if no lookup request was successful yet.
  • change the host_name implementation by doing what the current description says (#25551)

comment:15 Changed 9 months ago by irl

I really don't like the dns_ptr name for the field. I think Onionoo works at a higher level than that. I think I definitely prefer the unverified_host_name and host_name approach. To get the behaviour you're looking for in JavaScript would be:

dns_ptr = relay['host_name'] || relay['unverified_host_name'];

or Python:

dns_ptr = relay.get('host_name', None) or relay.get('unverified_host_name', None)

The approach where you have dns_ptr and host_name means that when correctly configured, the host name would end up included twice. I just don't feel that that's the most elegant solution.

I'll take a look at a patch for the spec later this week, but to outline:

  • For host_name we implement the spec as it is (actually omitting the field instead of returning IP addresses).
  • For unverified_host_name, this will include the DNS PTR record's name. It would be updated at the same time as the host_name record. Omitted if the host name was verified by looking up an A record, or if no PTR record was found.

comment:16 Changed 9 months ago by irl

Cc: metrics-team added

Adding metrics-team to cc

comment:17 Changed 6 months ago by irl

Proposed changes to the Onionoo spec are in my task/18342 metrics-web branch. This is what I intend to implement in Onionoo using JNDI.

comment:18 Changed 6 months ago by irl

The DNS specification does not actually prohibit the presence of multiple PTR records. ):

This means it can be perfectly valid for an IP address to have multiple reverse names which could all be validated using forward names successfully.

I have found at least 3 relays that have more than one PTR record that validate with a forward record.

Alternative proposal:

  • For host_name we implement the spec as it is (actually omitting the field instead of returning IP addresses), but also deprecate the field.
  • Add a new verified_host_names field with an array of strings, this will include the DNS PTR record's names. It would be updated at the same time as the host_name record. Omitted if the host name was verified by looking up an A record, or if no PTR record was found.
  • Add a new unverified_host_names field with an array of strings, this will include the DNS PTR record's names. It would be updated at the same time as the host_name record. Omitted if the host name was verified by looking up an A record, or if no PTR record was found.

It would be possible to have either, both, or neither verified and unverified host names for each relay.

comment:19 Changed 6 months ago by karsten

I like your alternative proposal above!

Minor suggestion: just leave the "host_name" field unchanged, deprecate it, and get rid of it two months later. Reason: changing that field would require a minor protocol version bump, and whoever cares this much about host names will start looking at the new fields immediately anyway.

Thanks!

comment:20 Changed 6 months ago by irl

Ok. I can return the IP address instead of null if a lookup has failed to maintain bug compatibility.

comment:21 Changed 6 months ago by karsten

Heh, sounds good.

comment:22 Changed 6 months ago by irl

In this case, is it worth correcting the spec to indicate that the field is always returned and that it will just contain the IP address if no lookup was successful?

comment:23 Changed 6 months ago by karsten

Probably not.

comment:24 Changed 6 months ago by cypherpunks

since it means more work for me, I'd like to understand your motivation for returning the IP instead of null if there is no PTR record.
compatibility with a bug? since it will be a new field there is no need for compatibility?

comment:25 Changed 6 months ago by karsten

Ah, this is just for the "host_name" field that will soon be deprecated and later removed. The new fields should not have this "bug".

comment:26 Changed 5 months ago by irl

Status: acceptedneeds_review

I have updated the specification patch in my metrics-web branch task/18342.

I do not yet have a repository to push to on git.tpo, but to avoid blocking on this (#26643) I have pushed my Onionoo branch to GitHub and opened a pull request against master which should allow for review.

comment:27 Changed 5 months ago by karsten

Reviewer: karsten

comment:28 in reply to:  26 Changed 5 months ago by karsten

Status: needs_reviewneeds_revision

Replying to irl:

I have updated the specification patch in my metrics-web branch task/18342.

A few comments:

  • In the protocol version summary: "Added a new optional X and Y fields". Unless the current text is correct English. To me it reads as if the Y part came in later and the rest of the sentence stayed the same.
  • For the deprecated field and the two newly added field, please include a date when they will be removed or have been added, respectively. Git history contains some examples.
  • The specification of "unverified_host_names" does not make it entirely clear whether a host name can show up in both newly added fields. It's the "regardless" part that threw me off. Maybe there's a way to make this clearer?

I do not yet have a repository to push to on git.tpo, but to avoid blocking on this (#26643) I have pushed my Onionoo branch to GitHub and opened a pull request against master which should allow for review.

I took the opportunity and used GitHub's review capabilities. Please take a look there.

Great stuff! Really looking forward to merging this soon. Setting to needs_revision for now. Thanks!

comment:29 Changed 5 months ago by irl

Status: needs_revisionneeds_review

Thanks! I've hopefully addressed all the issues in both the spec and code changes, and pushed new commits.

For some issues I have replied inline with your comments on GitHub.

comment:30 Changed 5 months ago by karsten

Status: needs_reviewneeds_revision

All changes look good, with the only exception being the DomainNameSystem class that should ideally not be a singleton. See also my comment on GitHub. Setting back to needs_revision just for that one. Thanks!

comment:31 Changed 5 months ago by irl

Status: needs_revisionneeds_review

DomainNameSystem now uses instance methods. Also fixed the license blurb.

comment:32 Changed 5 months ago by karsten

Status: needs_reviewneeds_revision

Please find my task-18342 branch with three fixup commits.

There's another issue that I did not fix yet: domain names always end with a dot, as in "onionoo.torproject.org.". Can you find out why this is the case and fix it?

In general, please avoid rebasing branches under review. Just add fixup or squash commits until we're done, and I'll make sure the end result is squashed and rebased.

Thanks!

comment:33 in reply to:  32 Changed 5 months ago by irl

Replying to karsten:

Please find my task-18342 branch with three fixup commits.

These all look good.

There's another issue that I did not fix yet: domain names always end with a dot, as in "onionoo.torproject.org.". Can you find out why this is the case and fix it?

This is because domain names always end with a dot, to indicate that they are absolute domain names. I'm sure I tested this and the current behaviour was to include the dot, but I must have been testing against the wrong thing as I can see now that we don't currently include the dot. I'll add a fixup commit to strip them.

In general, please avoid rebasing branches under review. Just add fixup or squash commits until we're done, and I'll make sure the end result is squashed and rebased.

Ok.

comment:34 Changed 5 months ago by irl

Status: needs_revisionneeds_review

Pushed to the GitHub branch.

comment:35 Changed 5 months ago by karsten

Status: needs_reviewmerge_ready

Alright! Squashed those fixup commits and pushed to master. Not yet released, nor deployed, which is something we should make plans for at the next team meeting. Not yet closing, because we need to merge the metrics-web patch as part of the release. Thanks!

comment:36 Changed 5 months ago by karsten

Milestone: Onionoo 1.15.0

comment:37 Changed 5 months ago by karsten

Resolution: fixed
Status: merge_readyclosed

Closing, because Onionoo 6.1-1.15.0 is now released.

Note: See TracTickets for help on using tickets.