Opened 5 years ago

Closed 4 years ago

#14882 closed defect (implemented)

add ability to prevent assignment of guard, exit, and hsdir flags in testing networks

Reported by: robgjansen Owned by:
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: 027-triaged-1-in, SponsorS, TorCoreTeam201508
Cc: Actual Points:
Parent ID: Points: medium
Reviewer: Sponsor:

Description

The new TestingDirAuthVoteExit, TestingDirAuthVoteGuard, and TestingDirAuthVoteHSDir options for testing networks are great. However, they do not allow configuration of nodes that should be in one group but not the other, or should be in none of the groups. I propose adding this feature.

A patch is attached.

Child Tickets

Attachments (1)

0001-option-to-prevent-guard-exit-hsdir-flag-assignment.patch (5.3 KB) - added by robgjansen 5 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 5 years ago by robgjansen

I just realized my original patch did not include any updates to the docs (i.e., the manual). I just updated the patch to include those (replacing the original file).

comment:2 Changed 5 years ago by nickm

Milestone: Tor: 0.2.7.x-final

This seems like it should be useful, and the design and implementation look very solid.

One suggested change, however.

Right now, there's no way I can see to specify an empty list along with strict. In other words, you can specify any number of routers that should receive the Guard flag, except for 0. Is it worth adjusting the semantics so that TestingDirVoteXYZIsStrict combined with an empty/missing TestingDirVoteXYZ causes *no* routers to get the XYZ flag?

comment:3 Changed 5 years ago by robgjansen

Good point. That is how I first implemented it. Patch updated.

comment:4 Changed 4 years ago by nickm

Status: newassigned

comment:5 Changed 4 years ago by nickm

Keywords: 027-triaged-1-in added

Marking more tickets as triaged-in for 0.2.7

comment:6 Changed 4 years ago by isabela

Keywords: SponsorS added
Points: medium
Version: Tor: unspecifiedTor: 0.2.7

comment:7 Changed 4 years ago by teor

Status: assignedneeds_review

Given that I implemented TestingDirAuthVoteExit and TestingDirAuthVoteHSDir, I'm well placed to review this.

comment:8 Changed 4 years ago by teor

I have read the code, and it is clean, succinct, and appears to implement the options as intended. I still need to build and test it.

It would be nice to have unit tests for all 6 options. However, chutney does a pretty good job of testing these in an integrated fashion. They're really quite simple options, to be honest.

comment:9 Changed 4 years ago by teor

I just discovered an error in my documentation for TestingDirAuthVoteHSDir: I wrote that HSDirs require "ORPort connectivity". While this is true, it is in no way unique to the HSDir flag. Of all the flags, only HSDirs need a DirPort configured in order for the authorities to assign that particular flag. (Which is what I meant to say.)

I'll fix this error as part of the patch, including Rob's copy-paste of my error.

comment:10 Changed 4 years ago by teor

Now working on unit tests as discussed in the tor development IRC meeting.
Then I guess I'll need the unit tests reviewed.

comment:11 Changed 4 years ago by teor

Added a changes file to the branch. Still need to do unit tests.

comment:12 Changed 4 years ago by teor

Keywords: TorCoreTeam201508 added

comment:13 Changed 4 years ago by nickm

This seems elegant and mergeable. Please let me know if there's an improved version, and if I should be expecting unit tests for this?

comment:14 Changed 4 years ago by nickm

(Also, what is the branch I should be looking at?)

comment:15 Changed 4 years ago by teor

Status: needs_reviewneeds_revision

I would like to do unit tests. They shouldn't take long. (And I will therefore put them at the top of my list.)

The branch is feature14882-TestingDirAuthVoteIsStrict in my github, including robgjansen's patch, my documentation fix, and a changes file.

comment:16 Changed 4 years ago by nickm

ok; code still looks fine though.

comment:17 Changed 4 years ago by teor

Yep, code rebased and merge conflict fixed in feature14882-TestingDirAuthVoteIsStrict-v2.
(We obsoleted VoteOnHidServDirectoriesV2 in the meantime.)

comment:18 Changed 4 years ago by teor

Status: needs_revisionneeds_review

Squashed and unit tests added in feature14882-TestingDirAuthVoteIsStrict-v3 at https://github.com/teor2345/tor.git

Do unit tests need to be reviewed (they pass on OS X and Linux, both x86_64), or can we merge this straight away?

comment:19 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

I've reviewed the tests and they seem okay enough. Merging!

Note: See TracTickets for help on using tickets.