Opened 2 months ago

Closed 6 weeks ago

Last modified 3 weeks ago

#32846 closed project (implemented)

Tor Manual: Alphabetize Client Options

Reported by: swati Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Normal Keywords: documentation, tor-client, manpage, gsod, extra-review
Cc: catalyst Actual Points:
Parent ID: #4310 Points:
Reviewer: catalyst, teor Sponsor:

Description

This ticket is for the alphabetical sorting of Client options. Making this a child ticket for #4310.

Child Tickets

Change History (15)

comment:1 Changed 2 months ago by swati

This is the PR for this ticket - https://github.com/torproject/tor/pull/1632.

Please review.

comment:2 Changed 2 months ago by arma

Status: newneeds_review

comment:3 Changed 2 months ago by arma

swati: I just added you to GRP_user on trac, which should give you more permissions than you had before.

comment:4 Changed 7 weeks ago by catalyst

Thanks for the pull request!

Running

sed -ne '/^== CLIEN/,/^==/p'|sed -ne 's/^\[\[\([^ ]*\)\]\].*/\1/p'

to extract the entry anchors, and comparing sorted and unsorted versions, I see:

--- /tmp/unsort.txt	2020-01-03 14:04:32.000000000 -0600
+++ /tmp/sorted.txt	2020-01-03 14:04:47.000000000 -0600
@@ -2,10 +2,9 @@
 AutomapHostsOnResolve
 AutomapHostsSuffixes
 Bridge
-CircuitsAvailableTimeout
-LearnCircuitBuildTimeout
 CircuitBuildTimeout
 CircuitPadding
+CircuitsAvailableTimeout
 CircuitStreamTimeout
 ClientAutoIPv6ORPort
 ClientBootstrapConsensusAuthorityDownloadInitialDelay
@@ -29,16 +28,19 @@
 DownloadExtraInfo
 EnforceDistinctSubnets
 EntryNodes
-ExcludeNodes
 ExcludeExitNodes
+ExcludeNodes
 ExitNodes
 FascistFirewall
 FirewallPorts
 GeoIPExcludeUnknown
+GuardfractionFile
+GuardLifetime
 HidServAuth
 HSLayer2Nodes
 HSLayer3Nodes
 HTTPTunnelPort
+LearnCircuitBuildTimeout
 LongLivedPorts
 MapAddress
 MaxCircuitDirtiness
@@ -47,17 +49,21 @@
 NATDPort
 NewCircuitPeriod
 NodeFamily
+NumDirectoryGuards
+NumEntryGuards
+NumPrimaryGuards
 OptimisticData
+OtherSocksPortFlags
 PathBiasCircThreshold
 PathBiasDropGuards
 PathBiasExtremeRate
+PathBiasExtremeUseRate
 PathBiasNoticeRate
-PathBiasWarnRate
-PathBiasScaleThreshold
-PathBiasUseThreshold
 PathBiasNoticeUseRate
-PathBiasExtremeUseRate
+PathBiasScaleThreshold
 PathBiasScaleUseThreshold
+PathBiasUseThreshold
+PathBiasWarnRate
 PathsNeededToBuildCircuits
 ReachableAddresses
 ReachableDirAddresses
@@ -68,7 +74,6 @@
 SafeSocks
 SocksPolicy
 SocksPort
-OtherSocksPortFlags
 SocksPortFlagsMisc
 SocksTimeout
 StrictNodes
@@ -81,12 +86,7 @@
 UpdateBridgesFromAuthority
 UseBridges
 UseEntryGuards
-GuardfractionFile
 UseGuardFraction
-GuardLifetime
-NumDirectoryGuards
-NumEntryGuards
-NumPrimaryGuards
 UseMicrodescriptors
 VirtualAddrNetworkIPv4
 VirtualAddrNetworkIPv6

I'm checking to see whether each out-of-order entry is commented with a suitable explanation. Anyone else like to help out with that?

It looks like this line was accidentally removed from CircuitBuildTimeout?

-    (Default: 60 seconds)

There are also a few possible line movements that git diff --color-moved isn't unambiguously marking as "moved", so I'll want to do some extra checking of those.

comment:5 Changed 7 weeks ago by swati

Hi Taylor,

Thanks for looking into this. I would also like to point out that I haven't really organized the PathBias config options because they are just listed out without any descriptions or information. Not sure if they're just placeholders.

I just compared the files and confirmed that that line under CircuitBuildTimeout was accidentally removed.

comment:6 in reply to:  5 Changed 7 weeks ago by catalyst

Replying to swati:

Thanks for looking into this. I would also like to point out that I haven't really organized the PathBias config options because they are just listed out without any descriptions or information. Not sure if they're just placeholders.

There seem to be descriptions for the PathBias options collected together under a single item. These options seem very specialized, and we probably want to put them in their own subsection later. For now, leaving them out of order (and commented as logically belonging together) makes sense to me.

I also just noticed that OtherSocksPortFlags is explicitly commented as being there only for formatting purposes, so we should also let it remain out of order. (Though maybe we should also consider putting SocksPort flags in their own subsection later?)

I just compared the files and confirmed that that line under CircuitBuildTimeout was accidentally removed.

Thanks!

comment:7 Changed 7 weeks ago by dgoulet

Reviewer: catalyst

comment:8 Changed 7 weeks ago by catalyst

CircuitsAvailableTimeout should go before CircuitStreamTimeout if we're alphabetizing ignoring case.

I think LearnCircuitBuildTimeout can also go immediately after CircuitBuildTimeout, or it can go in alphabetic order because CircuitBuildTimeout adequately describes LearnCircuitBuildTimeout anyway.

I'm undecided whether ExcludeExitNodes logically belongs out of order after ExcludeNodes; what do other people think?

The PathBias options could use a comment explaining why they're intentionally out of order.

I can make a branch that fixes up these, unless you want to make the changes yourself?

comment:9 Changed 7 weeks ago by catalyst

Keywords: extra-review added
Reviewer: catalystcatalyst, teor

Updated branch in https://github.com/torproject/tor/pull/1643
Please let me know what you think.

comment:10 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

Looks good!

I've tried to prioritise the changes I'm suggesting. Feel free to mark as merge ready when the "must" and "useful" changes are done.

Here are the things I think we must fix before merging:

  • GuardfractionFile should be moved to the directory authority section. It doesn't apply to clients.

Here are some useful things that I think would help people read the manual:

  • ReducedConnectionPadding logically belongs immediately after ConnectionPadding.
  • ReducedCircuitPadding logically belongs immediately after CircuitPadding.
    • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding.
  • FascistFirewall* logically belongs after Reachable*Addresses.
  • Reject and Warn PlaintextPorts belong together, probably near SafeSocks. They all do similar things.

Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

  • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
  • All the *Nodes options (including the HS*Nodes) options, would make sense together
  • This is a big change, so it probably needs another ticket

If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves.

comment:11 in reply to:  10 ; Changed 7 weeks ago by catalyst

Status: needs_revisionmerge_ready

Replying to teor:

Looks good!

Thanks for the review! I updated the pull request.

I've tried to prioritise the changes I'm suggesting. Feel free to mark as merge ready when the "must" and "useful" changes are done.

Here are the things I think we must fix before merging:

  • GuardfractionFile should be moved to the directory authority section. It doesn't apply to clients.

I agree. Moved.

Here are some useful things that I think would help people read the manual:

  • ReducedConnectionPadding logically belongs immediately after ConnectionPadding.
  • ReducedCircuitPadding logically belongs immediately after CircuitPadding.
    • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding.

I agree, except with moving CircuitPadding. As a slightly more "top-level" option, leaving it in alphabetic order might be better. Maybe we should make a new ticket for moving the timeout options in a new subsection?

  • FascistFirewall* logically belongs after Reachable*Addresses.

I left this one alone, because we clearly describe FascistFirewall as deprecated and refer to ReachableAddresses.

  • Reject and Warn PlaintextPorts belong together, probably near SafeSocks. They all do similar things.

Done. Warn now precedes Reject because of how their descriptions are worded. Also moved TestSocks after SafeSocks because they're related and they were like that pre-alphabetization.

Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

  • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
  • All the *Nodes options (including the HS*Nodes) options, would make sense together
  • This is a big change, so it probably needs another ticket

If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves.

I agree; let's make a new ticket for these.

comment:12 in reply to:  11 Changed 6 weeks ago by teor

Replying to catalyst:

Replying to teor:

Here are some useful things that I think would help people read the manual:

  • CircuitPadding splits the circuit timeout option group. If we're going to put it out of order anyway, maybe we should put it with ConnectionPadding.

I agree, except with moving CircuitPadding. As a slightly more "top-level" option, leaving it in alphabetic order might be better. Maybe we should make a new ticket for moving the timeout options in a new subsection?

This is now #32928.

Here are things we could fix now, but we could also do the fix in another ticket, possibly by creating a new subsection:

  • StrictNodes and GeoIPExcludeUnknown might logically belong after ExcludeNodes
  • All the *Nodes options (including the HS*Nodes) options, would make sense together
  • This is a big change, so it probably needs another ticket

If we create a section for the *Nodes options, we could mark them as advanced options. They're all options that might compromise anonymity, by changing how tor behaves.

I agree; let's make a new ticket for these.

This is now #32929.

comment:13 Changed 6 weeks ago by nickm

Squashed and merged. Does this ticket close now?

comment:14 Changed 6 weeks ago by nickm

Resolution: implemented
Status: merge_readyclosed

I'm told it does. Closing.

comment:15 Changed 3 weeks ago by teor

Cc: teor removed
Note: See TracTickets for help on using tickets.