Opened 14 months ago

Closed 3 months ago

#21031 closed defect (fixed)

Please don't remove ClientDNSRejectInternalAddresses

Reported by: Sebastian Owned by: Sebastian
Priority: Medium Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-23
Cc: Actual Points: .1
Parent ID: Points: .5
Reviewer: dgoulet Sponsor:

Description

I have a private tor network that lives in 10.0.0.0/8, and removing this option would make that network unusable.

Child Tickets

TicketStatusOwnerSummaryComponent
#21522closednickmClientDNSRejectInternalAddresses should not be deprecated in test networks.Core Tor/Tor

Change History (25)

comment:1 Changed 14 months ago by nickm

Hm. We have a category of testing-only options. I wonder if we should consider this to be in a new category of options that you can only change if you have changed your default authorities (like we do right now for setting a relay's IP to a private address).

comment:2 Changed 14 months ago by nickm

Points: .5

comment:3 Changed 14 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.1.x-final

Let's not remove any deprecated options in 0.3.0.

comment:4 Changed 10 months ago by nickm

Keywords: 031-reach added
Owner: set to nickm
Status: newaccepted

Putting these tickets in 031-reach and assigning them to me. The minimum to do is "nothing"; the maximum is "come up with a better deprecation path that doesn't break testing".

comment:5 Changed 10 months ago by nickm

Milestone: Tor: 0.3.1.x-finalTor: 0.3.2.x-final

A better refactoring here will need to wait for 0.3.2

comment:6 Changed 9 months ago by nickm

Keywords: 031-reach removed

comment:7 Changed 6 months ago by nickm

Actual Points: .1
Status: acceptedneeds_review

How about the approach in ticket21031 ? It renames the option to TestingClientDNSRejectInternalAddresses; it adds the old option name as an alias, and it makes it illegal to change the option except in a testing network.

Or should this look at something other than "testing network", like the set of default authorities?

comment:8 Changed 6 months ago by nickm

Keywords: review-group-23 added

Put 0.3.2 needs_review and merge_ready tickets into review-group-23.

comment:9 Changed 5 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

Sounds like a good approach to me.

Code lgtm;

comment:10 Changed 5 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged!

comment:11 Changed 4 months ago by cypherpunks

Resolution: implemented
Status: closedreopened

hi,

my use case is a private network without the TestingTorNetwork option set, which will break once this gets into stable. please consider keeping this an option for non-testing networks as well.

comment:12 Changed 4 months ago by Sebastian

I second this request (I am a part of the same network). Upgrading to 0.3.2.x breaks our configuration :(

comment:13 Changed 4 months ago by Sebastian

I would much prefer if this looked at the default dirauths compared to a Testing network.

comment:14 Changed 4 months ago by Sebastian

I would work on a patch for that if the approach is deemed acceptable.

comment:15 Changed 4 months ago by nickm

Sure; that would be okay.

comment:16 Changed 4 months ago by nickm

Owner: changed from nickm to Sebastian
Status: reopenedassigned

comment:17 Changed 4 months ago by Sebastian

Status: assignedneeds_review

Cool, ticket21031 in my repo :)

comment:18 Changed 4 months ago by Sebastian

wait, there's a test failure.

comment:19 Changed 4 months ago by Sebastian

Hrm. I am having trouble figuring out what's the right fix here. If someone could take a look I'd be grateful

comment:20 Changed 4 months ago by nickm

quick note -- if you want this in 0.3.2, it should be based on the maint-0.3.2 branch. For small patches, it's not hard to rebase or cherry-pick: just please remember in the future.

comment:21 Changed 4 months ago by nickm

I've figured out how to fix the tests, but I'm not convinced this should be necessary:

diff --git a/src/test/test_options.c b/src/test/test_options.c
index c55be358451192..6bc692d009cffb 100644
--- a/src/test/test_options.c
+++ b/src/test/test_options.c
@@ -402,7 +402,8 @@ fixed_get_uname(void)
   "VirtualAddrNetworkIPv4 127.192.0.0/10\n"                             \
   "VirtualAddrNetworkIPv6 [FE80::]/10\n"                                \
   "UseEntryGuards 1\n"                                                  \
-  "Schedulers Vanilla\n"
+  "Schedulers Vanilla\n"                                                \
+  "ClientDNSRejectInternalAddresses 1\n"
 
 typedef struct {
   or_options_t *old_opt;

comment:22 Changed 4 months ago by Sebastian

Yeah, it's the same thing that happened when UseEntryguards 1 was added I think. I didn't really consider this a proper fix because it's more like just hiding the problem imo. If you would take it for now though and we can reconsider the testing stuff, check out branch ticket21031_v2 which is a rebased branch on top of maint-0.3.2 which includes your fix in the commit.

comment:23 Changed 4 months ago by nickm

Resolution: fixed
Status: needs_reviewclosed

merged to 0.3.2 and master. thanks!

comment:24 Changed 4 months ago by teor

Resolution: fixed
Status: closedreopened

Commit 5a46074e55 in this branch also re-added this unrelated deprecation:

+  { "AllowDotExit", "Unrestricted use of the .exit notation can be used for "
+    "a wide variety of application-level attacks." },

comment:25 Changed 3 months ago by nickm

Resolution: fixed
Status: reopenedclosed

Thanks! Fixed that in 40c64f45f04a7cc1ec0149b6c8e8805fba9a2259

Note: See TracTickets for help on using tickets.