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

changed milestone to %Tor: 0.4.3.x-final

Trac:
Parent Ticket: #31851 (moved)
Child Ticket(s): #32352 (moved), #32370 (moved), #32371 (moved), #32368 (moved)

added actualpoints::5.1 component::core tor/tor milestone::Tor: 0.4.3.x-final network-team-roadmap-october owner::teor parent::31851 points::0.5 priority::medium resolution::fixed reviewer::nickm severity::normal sponsor::31-must status::closed tor-design type::defect version::tor 0.3.4.1-alpha labels

Trac:
Summary: Disable minimal dirauth and relay options when those modules are disabled to Phase 1: Disable minimal dirauth and relay options when those modules are disabled

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

master: https://github.com/torproject/tor/pull/1477

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

Trac:
Actualpoints: N/A to 0.7
Status: assigned to needs_review

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

Trac:
Reviewer: N/A to ahf

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

Trac:
Actualpoints: 0.7 to 1.7

We should also disable the BridgeRelay option.

Trac:
Description: We should disable this minimal set of options:

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

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

to

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.

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.

Trac:
Reviewer: ahf to nickm

Trac:
Status: needs_review to needs_revision

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

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

Trac:
Actualpoints: 1.7 to 2.3

Trac:
Actualpoints: 2.3 to 3.3

I have deleted the code that conflicts with #32339 (moved) and #32344 (moved), 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

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

Trac:
Status: needs_revision to needs_review

Trac:
Actualpoints: 3.3 to 4.5

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.

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.

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

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 (moved).

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

Trac:
Actualpoints: 4.5 to 4.8

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:

master: https://github.com/torproject/tor/pull/1477

Trac:
Version: N/A to Tor: 0.3.4.1-alpha
Actualpoints: 4.8 to 5.1
Status: needs_review to merge_ready

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

Merged #32213 (moved), #32339 (moved), and #32344 (moved).

Trac:
Status: merge_ready to closed
Resolution: N/A to fixed

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

Trac:
Summary: Phase 1: Disable minimal dirauth and relay options when those modules are disabled to Phase 0: Disable minimal dirauth and relay options when those modules are disabled

closed

changed time estimate to 4h

added 40h 48m of time spent

mentioned in issue #32338 (moved)

mentioned in issue #32339 (moved)

mentioned in issue #32344 (moved)

mentioned in issue #32352 (moved)

mentioned in issue #32368 (moved)

mentioned in issue #32369 (moved)

mentioned in issue #32370 (moved)

mentioned in issue #32371 (moved)

mentioned in issue #32372 (moved)

mentioned in issue #31851 (moved)

moved to tpo/core/tor#32213 (closed)

mentioned in issue tpo/core/tor#32369 (closed)

mentioned in issue tpo/core/tor#32372 (closed)

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

Child items 0

Activity