Opened 10 months ago

Closed 8 months ago

#25935 closed enhancement (wontfix)

Allow DA address to be specified as FQDN

Reported by: somlo Owned by:
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-dirauth
Cc: Actual Points:
Parent ID: Points:
Reviewer: teor Sponsor:


It would be very helpful, particularly in sandbox situations, to specify
the Directory Authority by FQDN hostname instead of by IP address. This
would allow us to defer picking an actual IP address until the simulation
is started, and even to use some "in-game" DNS facility to figure out
the actual address after the simulation is launched.

Right now, specifying a FQDN for the "DirAuthority" config file entry
even *partially* works already: if the FQDN happens to start with a
digit, it is correctly resolved internally using available DNS resolver
infrastructure :)

The first attached patch makes that work in all cases (even when the
FQDN hostname does *not* begin with a digit).

The second attached patch allows FQDNs to be inserted into DA certs
created using tor-gencert, and correspondingly resolved when a client
parses the downloaded DA certificate.

I realize there is ongoing work to refactor parsing the DA config entry
(ticket #17224), so please consider this patch set either independently
on its own merits or as part of that larger effort. In the first case,
I'd be happy to redo and resubmit the patches based on review/feedback.

Child Tickets

Attachments (1)

0001-Allow-FQDN-in-config-file-DA-address-field.patch (1.5 KB) - added by somlo 10 months ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 10 months ago by somlo

can't upload second patch due to TRAC going off on a CAPTCHA death spiral. Created github pull request instead.

comment:2 Changed 10 months ago by catalyst

Component: - Select a componentCore Tor/Tor
Keywords: tor-dirauth added
Milestone: Tor: 0.3.5.x-final
Status: newneeds_review
Type: defectenhancement

comment:3 Changed 10 months ago by teor

Reviewer: teor
Status: needs_reviewneeds_revision

Please see my review on the pull request.

Here is my major concern with this patch:

DNS queries provide a vector for a malicious directory authority to de-anonymise clients. So we might only want to allow domain name resolution in test networks, or networks which are using non-default authorities.

When using default authorities, we should probably ban the use of DNS names

For completeness, we might want to open another ticket for making the "ipv6" ORPort a hostname as well.

comment:4 Changed 10 months ago by somlo

Pasting my reply from the github review thread, for completeness:

I got confused by the man page, which says:

DirAuthority [nickname] [flags] address:port fingerprint

We probably should add [fingerprint...] at the end to indicate there could be more than one...

I see now there's a "smartlist_join_strings()" call after processing the "addr:port" field, so you're right of course. Problem is, right now we decide it's time to process "addr:port" if the very first character in that string is a digit, which partially allows parsing hostname:port entries instead of throwing an error in all cases where "addr" is not an actual IP address.

We could decide we've finished parsing flags and reached "addr:port" if the current smartlist item contains a ":" character (unless it's possible for a flag to contain that character, either now or in the future, in which case we're back to not having a good way to know we've reached that field).

Alternatively, we could decide to ban non-IP "address" fields outright, because of the potential security vulnerability introduced by adding DNS to the mix (and I'll figure out a way to cope with that :) ). Right now we're only sort-of, kind-of doing that, which inspired me to try for full support, without thinking of the larger implications.

Please let me know what you think, and I'll either respond to your full review or submit a new patc> h fixing the man page and throwing an error if addr is not an IP (or backing off, if you're already working on the parsing code as part of some other effort).

Thanks much,

comment:5 Changed 10 months ago by teor

Please see my reply on the GitHub review thread.

comment:6 Changed 8 months ago by teor

Resolution: wontfix
Status: needs_revisionclosed

We'll implement #26488 instead,

Note: See TracTickets for help on using tickets.