Opened 4 weeks ago

Closed 8 days ago

#25581 closed defect (fixed)

Inconsistent underscore config options (for vanguard options)

Reported by: atagar Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: regression, 033-must, 033-triage-20180326, 033-included-20180326
Cc: mikeperry, asn Actual Points:
Parent ID: Points:
Reviewer: asn Sponsor:


Hi lovely tor folks. In December new _HSLayer2Nodes and _HSLayer3Nodes torrc options were added...

These are unusual in a couple ways...

  • Tor uses proceeding underscores to indicate that a torrc option is private. These shouldn't appear in the tor manual, which these do.
  • Private options start with two underscores, not one.

Child Tickets

Change History (18)

comment:1 Changed 4 weeks ago by nickm

Cc: mikeperry added
Summary: Inconsistent underscore config optionsInconsistent underscore config options (for vanguard options)

I'd be okay with double-underscores on these

comment:2 Changed 4 weeks ago by nickm

Keywords: regression 033-must added
Milestone: Tor: 0.3.3.x-final

comment:3 Changed 4 weeks ago by nickm

(let's get this settle in 0.3.3 before it stabilizes)

comment:4 Changed 4 weeks ago by nickm

Keywords: 033-triage-20180326 added

Second batch of triage for 0.3.3: tickets that we didn't cover the first time.

comment:5 Changed 4 weeks ago by nickm

Keywords: 033-included-20180326 added

Marking 033-must tickets as included. Round 2.

comment:6 Changed 4 weeks ago by nickm

Cc: asn added
Owner: set to nickm
Status: newaccepted

Mike, George -- are you okay with changing these to double-underscore options and having the single-underscore case remain as a synonym for backward compatibility?

comment:7 Changed 4 weeks ago by nickm

Status: acceptedneeds_review

If you're okay with it, bug25581_033 should be the necessary change.

comment:8 Changed 4 weeks ago by dgoulet

Reviewer: asn

comment:9 Changed 3 weeks ago by asn

Mike, do you think making HSLayer2Nodes with double-underscore to denote they are controller-only options is reasonable? Or should we make them with no underscore to denote that they can be used without controllers too? How do you envision this?

comment:10 Changed 3 weeks ago by mikeperry

Hrmm. We discussed this before. The idea was that this would be an "expert" option, which we didn't really have before. Hence the single underscore. It was a deliberate choice.

In terms of how risky/"expert" the options actually are compared to other things that lack underscores: I also don't see a lot of difference between exposing these and letting users do things like EntryNodes {ru}. If we think that giving users the ability to set arbitrary values for EntryNodes is still acceptable, then I lean in the direction of removing the underscore and keeping this public in torrc. We have had issues with things like #21155 for EntryNodes, and the risks with _HSLayerKNodes are not as severe (basically your service just stops working instead of giving away your entry guard node choice).

In Rome, dgoulet also mentioned that in his experience, HS operators will often be reluctant to open their control port, even just for debugging. That is another argument for preserving the torrc access of these settings.

comment:11 Changed 3 weeks ago by asn

Not sure if this semantic of single underscore would be clear to our users, and it might even confuse them. Perhaps given what you write above it would make sense to just remove the underscores from the config option? The man page already says Please use extreme care if you are setting this option manually..

comment:12 Changed 3 weeks ago by arma

I'm having trouble reconciling the two stories I'm seeing of how we're planning to frame these new options from the user perspective.

In story one, we've added these experimental internal options so that it's easier to experiment with vanguards and get closer to knowing what rotation parameters and so on we want to recommend. We plan to write some controller scripts to help us implement our ideas for how vanguards might work, which will help us to prove the concept to ourselves, and convince us that we want to implement vanguards as part of Tor for real. In this story, we make them double-underscore options to indicate that users really shouldn't mess with them unless they're part of the vanguard research work. Once we have some controller scripts that we think work, we would then invite developer-style users to try them too, to help us find bugs and make the idea better, so when we build it into Tor it will be the best possible design.

In story two, we've added these expert-mode options so that users who think they know how vanguards should work can begin using them for their deployed onion services. We plan to write some controller scripts so that users can begin using them for real-world situations, because while we don't yet know how one should tune vanguards, or even if there are parameters that will work in the real world, we plan to invite real users to start using them asap because not using them seems really scary. In this story, we want to make the torrc options like all the other torrc options (no underscores), because maybe there are users who need vanguards now yet their security situation precludes opening a control port, so those users should be given the means to cobble together a manual vanguard approach on their own.

I worry that we are trying to tell both of these stories at the same time, and it's leading to conflicting plans.

For example: are we planning to write up documentation, aimed at users, on how onion service operators should use the 0.3.3 vanguard torrc options to become safer? (That plan would be in line with story two.) If so, I think the EntryNodes comparison is not quite right, because we don't have any documentation telling users how they should set EntryNodes to be safer, and in fact we work hard to tell users that they will only hurt themselves by setting it. (Or am I wrong and we do have those docs somewhere?)

In summary, I think we need to agree on what story we're intending to tell, and then whether to use underscores will hopefully be clear from there.

Last edited 3 weeks ago by arma (previous) (diff)

comment:13 Changed 2 weeks ago by asn

Status: needs_reviewneeds_revision

IMO, we should not use underscores here in line with arma's second story.

I'm adding this as a child ticket of #25546 which is roadmapped and in line for 034, so that we don't lose track of this ticket.

I'm putting this in needs_revision for now. When Mike decides what his story wants to be, we can revise hte branch as needed.

comment:14 Changed 13 days ago by mikeperry

At the roadmap meeting in Rome, we agreed to proceed with Story 2 for 0.3.4.

comment:15 Changed 11 days ago by nickm

Status: needs_revisionneeds_review

I've done story 2 version as bug25581_033_v2. There's an updated version of the story 1 version as bug25581_033 with some cases I missed.

comment:16 in reply to:  15 Changed 9 days ago by asn

Status: needs_reviewneeds_revision

Replying to nickm:

I've done story 2 version as bug25581_033_v2. There's an updated version of the story 1 version as bug25581_033 with some cases I missed.

bug25581_033_v2 looks good but the changes file still specifies double undescore format.
If this is fixed, this is good to merge. I can fix this tomorrow!

comment:17 Changed 8 days ago by asn

Status: needs_revisionmerge_ready

Pushed bug25581_033_v2 in my repo with a fix for the changes file above. Feel free to merge directly.

comment:18 Changed 8 days ago by nickm

Resolution: fixed
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.