Opened 3 months ago

Closed 3 months ago

Last modified 2 months ago

#32213 closed defect (fixed)

Phase 0: Disable minimal dirauth and relay options when those modules are disabled

Reported by: teor Owned by: teor
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version: Tor: 0.3.4.1-alpha
Severity: Normal Keywords: tor-design, network-team-roadmap-october
Cc: nickm Actual Points: 5.1
Parent ID: #31851 Points: 0.5
Reviewer: nickm Sponsor: Sponsor31-must

Description (last modified by teor)

We should disable this minimal set of options:

  • --disable-module-dirauth
    • disable AuthoritativeDirectory
  • --disable-module-relay
    • disable DirPort, DirCache, ORPort, BridgeRelay
    • set ClientOnly to 1

This is a serious UX bug, so it must be fixed as part of Sponsor 31.

Child Tickets

TicketStatusOwnerSummaryComponent
#32352closedteorStop adding a space when dumping an empty config valueCore Tor/Tor
#32368closedteorUse the same tor binary in all our test scriptsCore Tor/Tor
#32370closedteorFix "make autostyle" in out of tree buildsCore Tor/Tor
#32371closedteorFix update_versions.py in out of tree buildsCore Tor/Tor

Change History (24)

comment:1 Changed 3 months ago by teor

Summary: Disable minimal dirauth and relay options when those modules are disabledPhase 1: Disable minimal dirauth and relay options when those modules are disabled

comment:2 Changed 3 months ago by teor

Actual Points: 0.7
Status: assignedneeds_review

Here's an initial PR which disables AuthoritativeDir when the module is disabled:

I would like an initial review, if anyone has time :-)

It was actually pretty easy to split the dirauth config out into its own file. Now it is in its own file, we can disable all the detailed config checks when the module is disabled.

TODO:

relay

  • relay config split?
  • disable relay config when module is disabled
  • relay cleanup

standard

  • changes file
  • autoformat
  • regenerate practracker, and keep the improvements

comment:3 Changed 3 months ago by ahf

Reviewer: ahf

I will be traveling a bit this week. If I get too slow, please prod someone else on the network team too :-)

comment:4 Changed 3 months ago by teor

Actual Points: 0.71.7

I have updated the branch with an initial relay options_validate() split.

TODO:

relay

  • relay/dirauth options_act() split
  • disable relay config when module is disabled
  • relay cleanup

standard

  • changes file
  • autoformat
  • regenerate practracker, and keep the improvements

comment:5 Changed 3 months ago by teor

Description: modified (diff)

We should also disable the BridgeRelay option.

comment:6 Changed 3 months ago by nickm

Reviewer: ahfnickm

Taking review because ahf is busy. General comments:

  • I like all the code movement here, and I like how clean it looks with --color-moved. I look forward to merging this when it's done.
  • Let's see if we can get the coverage on the moved/changed code any higher, if that's reasonable?
  • Let's see if we can regenerate practracker in a way that only tightens the restrictions on improved files/modules.
  • Some previously static functions have become global. Not all of them have logical prefixes that tie them to a module/type. Let's do a quick audit of these functions, and rename them so that they follow our preferred conventions.

comment:7 Changed 3 months ago by nickm

Status: needs_reviewneeds_revision

comment:8 Changed 3 months ago by teor

I have moved or modified all the code that uses the target relay and dirauth options out of config.c, which should make it easier and safer to disable them.

TODO:

relay

  • disable relay config when module is disabled
  • relay cleanup

relay/dirauth

  • rename new global functions so they are tied to a module/type

standard

  • improve test coverage, if reasonable
  • changes file
  • autoformat
  • regenerate practracker, but only commit the changes that tighten restrictions

comment:9 Changed 3 months ago by teor

I have disabled the relay config when the relay module is disabled, and started improving the tests.

TODO:

relay

  • relay cleanup

relay/dirauth

  • rename new global functions so they are tied to a module/type

standard

  • improve test coverage, if reasonable
  • changes file
  • autoformat
  • regenerate practracker, but only commit the changes that tighten restrictions

comment:10 Changed 3 months ago by teor

Actual Points: 1.72.3

comment:11 Changed 3 months ago by teor

Actual Points: 2.33.3

comment:12 Changed 3 months ago by teor

I have deleted the code that conflicts with #32339 and #32344, those changes are much better.

I disabled the server transport options when the relay module is disabled, and did some minor cleanup on the relay config code.

Here are my proposed renames, but I won't do them until I've finished the tests:

scripts/maint/rename_c_identifier.py --commit \
get_dirportfrontpage relay_get_dirportfrontpage \
parse_port_config port_parse_config \
count_real_listeners port_count_real_listeners \
parse_transport_line pt_parse_transport_line \
ensure_bandwidth_cap config_ensure_bandwidth_cap \
get_effective_bwrate relay_get_effective_bwrate \
get_effective_bwburst relay_get_effective_bwburst \
warn_nonlocal_ext_orports port_warn_nonlocal_ext_orports \
parse_ports_relay port_parse_ports_relay \
update_port_set_relay port_update_port_set_relay \
get_transport_bindaddr_from_config pt_get_bindaddr_from_config \
get_options_for_server_transport pt_get_options_for_server_transport

TODO:

  • improve test coverage, if reasonable
  • changes file
  • rename new global functions so they are tied to a module/type
  • autoformat
  • regenerate practracker, but only commit the changes that tighten restrictions

comment:13 Changed 3 months ago by teor

Status: needs_revisionneeds_review

I'm still improving the test coverage, I think we can do a review on the code changes while that is happening.

comment:14 Changed 3 months ago by teor

Actual Points: 3.34.5

comment:15 Changed 3 months ago by teor

Improving the test coverage for option validation is easy with test_parseconf.sh, but it's time-consuming. Improving the test coverage for options_act() could take a bit more work.

I pushed some more tests, and a fix for a bug I found in the new config code. I'll make a child ticket.

comment:16 Changed 3 months ago by nickm

This is looking good to me. Please put it in merge-ready once you're satisfied with the tests, and and once travis is passing.

comment:17 Changed 3 months ago by teor

Travis is failing in the merge, but passing on the branch, I'm still trying to work out why.

comment:18 in reply to:  17 Changed 3 months ago by teor

Replying to teor:

Travis is failing in the merge, but passing on the branch, I'm still trying to work out why.

Turns out my local copy had some outdated binaries that weren't being rebuilt.

I've added some final tests for option parsing and validation. The remaining tests are complex, because they require:

  • launching tor, or
  • refactoring more of config.c.

I'll open tickets for those tests/refactors under #31851.

I'm going to do one more CI run for coverage, and then maybe add some more coverage excludes. Then I'll merge.

comment:19 Changed 3 months ago by teor

Actual Points: 4.54.8

comment:20 Changed 3 months ago by teor

Actual Points: 4.85.1
Status: needs_reviewmerge_ready
Version: Tor: 0.3.4.1-alpha

I pushed the final branch with these changes:

  • improve test coverage, where reasonable
  • changes files
  • rename new global functions so they are tied to a module/type
    • spacing and practracker fixes
  • autostyle
    • fixes so autostyle works out of tree
  • regenerate practracker, but only commit the changes that tighten restrictions

Anyone can merge once CI passes:

comment:21 Changed 3 months ago by teor

I'd like to merge #32213, #32339, and #32344 in that order, to minimise conflicts (if any).

comment:22 Changed 3 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged #32213, #32339, and #32344.

comment:23 Changed 3 months ago by teor

I opened #32374 to make it easier to test options_act*().

comment:24 Changed 2 months ago by teor

Summary: Phase 1: Disable minimal dirauth and relay options when those modules are disabledPhase 0: Disable minimal dirauth and relay options when those modules are disabled
Note: See TracTickets for help on using tickets.