Opened 4 years ago
Closed 17 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 4 years ago by
Cc: | tyseom added |
---|
comment:2 Changed 3 years ago by
comment:3 Changed 3 years ago by
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 follow-up: 5 Changed 3 years ago by
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 Changed 3 years ago by
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 3 years ago by
Milestone: | → Onionoo 3.1-1.0.0 |
---|
Planned to be part of the first Onionoo release.
comment:7 Changed 3 years ago by
Milestone: | Onionoo 3.1-1.0.0 → Onionoo 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 3 years ago by
Milestone: | Onionoo 3.1-1.1.0 |
---|
comment:9 Changed 2 years ago by
Summary: | onionoo has poor reverse DNS results → Provide more accurate reverse DNS results |
---|
Make an summary more accurate (at least).
comment:10 Changed 2 years ago by
Keywords: | metrics-2018 added |
---|
comment:11 Changed 2 years ago by
Owner: | set to metrics-team |
---|---|
Status: | new → assigned |
comment:12 Changed 2 years ago by
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 21 months ago by
Owner: | changed from metrics-team to irl |
---|---|
Status: | assigned → accepted |
Some relevant discussion happened at #25551.
comment:14 Changed 21 months ago by
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 21 months ago by
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:17 Changed 18 months ago by
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 18 months ago by
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 18 months ago by
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 18 months ago by
Ok. I can return the IP address instead of null
if a lookup has failed to maintain bug compatibility.
comment:22 Changed 18 months ago by
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:24 Changed 18 months ago by
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 18 months ago by
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 follow-up: 28 Changed 18 months ago by
Status: | accepted → needs_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 18 months ago by
Reviewer: | → karsten |
---|
comment:28 Changed 18 months ago by
Status: | needs_review → needs_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
anew 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 18 months ago by
Status: | needs_revision → needs_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 18 months ago by
Status: | needs_review → needs_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 17 months ago by
Status: | needs_revision → needs_review |
---|
DomainNameSystem now uses instance methods. Also fixed the license blurb.
comment:32 follow-up: 33 Changed 17 months ago by
Status: | needs_review → needs_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 Changed 17 months ago by
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 17 months ago by
Status: | needs_revision → needs_review |
---|
Pushed to the GitHub branch.
comment:35 Changed 17 months ago by
Status: | needs_review → merge_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 17 months ago by
Milestone: | → Onionoo 1.15.0 |
---|
comment:37 Changed 17 months ago by
Resolution: | → fixed |
---|---|
Status: | merge_ready → closed |
Closing, because Onionoo 6.1-1.15.0 is now released.
Here's how we're currently resolving addresses to domain names:
Are there better ways to do this?