Opened 4 years ago

Closed 12 months ago

#15015 closed defect (worksforme)

tor --verify-config should not bind to ports

Reported by: cypherpunks Owned by: rl1987
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-relay, intro, startup, configuration, torrc, bootstrap, refactor, technical-debt
Cc: jryans@… Actual Points:
Parent ID: Points:
Reviewer: catalyst Sponsor:

Child Tickets

Change History (29)

comment:1 Changed 4 years ago by nickm

Keywords: tor-relay added
Milestone: Tor: 0.2.7.x-final

comment:2 Changed 4 years ago by Sebastian

Hrm, this depends a little on how much verification we want to do. We can either say "this config file parses, we're good" or we can say "we can actually apply all the settings in the config file". I'm leaning towards the former, which indicates this is a bug, but maybe others think it's the latter in which case we should improve the docs.

comment:3 Changed 4 years ago by cypherpunks

Is anything but luck preventing this problem in the current debian init.d scripts?

(they also call check_config() before starting tor)

comment:4 Changed 4 years ago by tyseom

Cc: nusenu@… added

comment:5 Changed 4 years ago by tyseom

Cc: nusenu@… removed

comment:6 Changed 4 years ago by nickm

Status: newassigned

comment:7 Changed 4 years ago by nickm

Keywords: 027-triaged-1-out added

Marking triaged-out items from first round of 0.2.7 triage.

comment:8 Changed 4 years ago by nickm

Milestone: Tor: 0.2.7.x-finalTor: 0.2.???

Make all non-needs_review, non-needs_revision, 027-triaged-1-out items belong to 0.2.???

comment:9 Changed 3 years ago by nickm

Keywords: intro added

comment:10 Changed 3 years ago by nickm

Points: small
Severity: Normal

comment:11 Changed 2 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:12 Changed 2 years ago by jryans

Should a new option (such as --parse-config) be added for the more minimal step of parsing only and --verify-config left as is to preserve compatibility? Or is it okay to change the behavior of --verify-config directly?

comment:13 Changed 2 years ago by teor

Hmm, there are two different use cases here:
1) I am not running a relay, and I'm about to start one, and I want to know if it will work,
2) I am running a relay, and I'm about to change the config, and I want to know if it will work

We can bind to ports in case 1), but not case 2).

I think we should add a new option --parse-config that just does an options_verify().
Of course, the risk is that it will succeed, and then when it gets to options_act(), it will fail because it suddenly finds out it can't do what it wanted to do.

But for --verify-config, the risk is that it will fail unnecessarily, because it could do the things if only it were the existing tor process.

comment:14 Changed 2 years ago by jryans

Cc: jryans@… added

comment:15 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:16 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:17 Changed 2 years ago by nickm

Keywords: 027-triaged-in added

comment:18 Changed 2 years ago by nickm

Keywords: 027-triaged-in removed

comment:19 Changed 2 years ago by nickm

Keywords: 027-triaged-1-out removed

comment:20 Changed 2 years ago by nickm

Status: assignednew

Change the status of all assigned/accepted Tor tickets with owner="" to "new".

comment:21 Changed 23 months ago by nickm

Keywords: startup configuration torrc bootstrap refactor technical-debt added
Priority: MediumHigh

comment:22 Changed 12 months ago by rl1987

Owner: set to rl1987
Status: newassigned

comment:23 Changed 12 months ago by rl1987

Status: assignedneeds_review

Added --parse-config CLI option:

comment:24 Changed 12 months ago by teor

Milestone: Tor: unspecifiedTor: 0.3.4.x-final
Points: small
Status: needs_reviewneeds_revision

Thanks for this patch.

The code looks good, but I think we can improve the man page:

  • please put parse-config immediately after verify-config
  • please explicitly document the different use cases for verify and parse:
    • verify may fail if the tor service is already running
    • parse should be used if the tor service is running

If you'd like, you can also set up Travis CI on your branch, or open a pull request on https://github.com/torproject/tor
https://trac.torproject.org/projects/tor/ticket/23883#comment:3
It's the first thing the next reviewer will do.

comment:25 Changed 12 months ago by rl1987

Status: needs_revisionneeds_review

Pushed c1c5970ac83ad804739bd2515b297195df927956 that improves the manpage.

comment:26 Changed 12 months ago by teor

Thanks. Looks good to me.

I'd like someone else to review this next, and check it against CI.
They can decide if we need unit test scripts for --parse-config and --verify-config.

comment:27 Changed 12 months ago by dgoulet

Reviewer: catalyst

comment:28 Changed 12 months ago by catalyst

Priority: HighLow
Status: needs_reviewneeds_information

Thanks for the patch! The code looks good. Like teor, I wonder if tests are needed.

Right now I'm not sure whether the originally reported problem still exists. I can't seem to get a "permission denied" error when setting SOCKSPort 88 on the command line with --verify-config on master, even though I do get that error without the --verify-config option.

comment:29 Changed 12 months ago by catalyst

Resolution: worksforme
Status: needs_informationclosed

No one I've asked seems to be able to reproduce this on a modern tor. There's some evidence that such a bug might have existed back before 0.2.0, but it seems like it's gone in supported releases.

rl1987, thanks again for working on this patch. I'm sorry it turns out to be unnecessary.

Note: See TracTickets for help on using tickets.