Opened 4 years ago

Closed 4 years ago

#16774 closed enhancement (implemented)

Add fallback directories to GETINFO

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7.2-alpha
Severity: Normal Keywords: lorax, easy, 028-triage
Cc: Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

In #15775, we add a list of ~100 fallback directories to the Tor codebase.

Do want to add these to GETINFO config/defaults if not already present, or to a separate GETINFO config/fallback_directories?

Child Tickets

TicketTypeStatusOwnerSummary
#17817defectclosedAdd ipv6 authority and fallback flag to GETINFO config/defaults

Change History (18)

comment:1 Changed 4 years ago by nickm

Either alternative would be ok with me; controller implementors might have stronger opinions.

comment:2 Changed 4 years ago by nickm

Keywords: 028-triage added

comment:3 Changed 4 years ago by nickm

Points: medium

comment:4 Changed 4 years ago by gtank

Severity: Normal

Not sure how close #15775 is, but I wrote some code on top of teor's feature15775-fallback-v9 branch to add FallbackDir lines in config/defaults.

With the most recently posted fallback_dirs.inc it looks like

[...]
DirAuthority "longclaw orport=443 v3ident=23D15D965BC35114467363C165C4F724B64B4F66 199.254.238.52:80 74A9 1064 6BCE EFBC D2E8 74FC 1DC9 9743 0F96 8145"
FallbackDir "orport=9001 E9C8154418544764619D2CCD0596B355D7DFF236 5.9.123.81:9030"
FallbackDir "orport=9010 BF0FB582E37F738CD33C3651125F2772705BB8E8 148.251.190.229:9030"
FallbackDir "orport=443 963ADC0137505151C1AFA6757DD2367EDEEC7B62 124.6.36.193:80"
[...]

https://github.com/gtank/tor.git on branch feature16774

I don't know what to do with a lorax tag that depends on another open issue, so I'm just moving this to needs_information.

Last edited 4 years ago by gtank (previous) (diff)

comment:5 Changed 4 years ago by gtank

Status: newneeds_information

comment:6 Changed 4 years ago by teor

Cc: #15775 added
Status: needs_informationneeds_review

Thanks for this, I'll take a look at it next week. I'm buried in IPv6 implementations right now.

comment:7 Changed 4 years ago by teor

Cc: #15775 removed
Parent ID: #17817

#17327 adds an ipv6=address:orport flag to authorities and fallback directories.
Split off #17817 to add this to GETINFO config/defaults also.

comment:8 Changed 4 years ago by nickm

Status: needs_reviewneeds_revision

What is the esc_for_log character doing here? Do we really want to be putting quotes around the output? Do we really expect weird characters in the input?

comment:9 Changed 4 years ago by gtank

I wouldn't expect weird characters here, but noted that the DirAuthority lines directly above are escaped from hardcoded strings. The description of esc_for_log implies that the controller expects it for some values, though the comment dates to 2007. Perhaps it's no longer true?

comment:10 in reply to:  9 Changed 4 years ago by teor

Replying to gtank:

I wouldn't expect weird characters here, but noted that the DirAuthority lines directly above are escaped from hardcoded strings. The description of esc_for_log implies that the controller expects it for some values, though the comment dates to 2007. Perhaps it's no longer true?

If it's what existing controller implementations expect, let's keep it that way.
It will be enough for them to update to the new line format, especially with IPv6.

comment:11 Changed 4 years ago by teor

There's another way of implementing this patch, that will print all current and future fallback directory fields:

The current fallback directory code #includes the list of fallbacks in a static array that's inside a function.

If we refactor that code, creating a default_fallbacks static variable at file-scope, then we can simply print out the content of each fallback string.

But your existing code may still be useful - it can be used to print out the fields from fallback directories that are configured in the torrc. (We can imitate what happens for authorities.)

What do you think?

comment:12 Changed 4 years ago by gtank

I like the file-scope approach, and think it also takes care of #17817 (assuming the ipv6 flags are in the included data). It also has the nice side-effect of treating fallbacks identically to dirauths in the config handler. I've written it up at feature16774-v3 at https://github.com/gtank/tor.git

comment:13 Changed 4 years ago by gtank

Status: needs_revisionneeds_review

comment:14 in reply to:  12 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

Replying to gtank:

I like the file-scope approach, and think it also takes care of #17817 (assuming the ipv6 flags are in the included data). It also has the nice side-effect of treating fallbacks identically to dirauths in the config handler. I've written it up at feature16774-v3 at https://github.com/gtank/tor.git

Thanks for this patch, looks good!

One minor fix - Tor doesn't use the default fallback directories if get_options()->UseDefaultFallbackDirs is 0. To avoid confusing users, let's only display the default fallbacks if there are no FallbackDir lines in the config, and get_options()->UseDefaultFallbackDirs is 1.

    if (fallback_lines_seen == 0) {

Also, the fallback directories aren't authoritative, they are mirrors. Can you change default_fallback_authorities to avoid using authorities?

comment:15 Changed 4 years ago by gtank

At last. feature16774-squashed has those changes and a fix to the config/default_dir_servers test that was failing when the fallback list contained any entries.

comment:16 Changed 4 years ago by nickm

Status: needs_revisionneeds_review

comment:17 Changed 4 years ago by teor

Parent ID: #17817

Excellent! Thanks for this, let's get this merged.

(#17817 can also close when this closes.)

comment:18 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks ok, merged!

Note: See TracTickets for help on using tickets.