Opened 3 months ago

Closed 4 weeks ago

#20956 closed enhancement (implemented)

optionally do not write command line config to torrc

Reported by: mcs Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tbb-wants 029-backport
Cc: brade, gk Actual Points: .2
Parent ID: Points: 2
Reviewer: dgoulet Sponsor:

Description

For Tor Browser/Tor Launcher, we have a use case where we want to pass ControlPort and SocksPort options on the command line (aka as program args) and be assured they will never be written to torrc, e.g., in response to a SAVECONF control port command.

See ticket:20906#comment:7 for some background. Also, in ticket:20906#comment:8 nickm said:

I think that a "Don't write this to torrc" flag might be workable, but I also think it's maybe less easy to implement than you might think -- especially if it needs to work for things other than LineList options. If it's LineList only, it has a reasonable chance of being easy to implement.

Child Tickets

Attachments (1)

0001-Convert-ControlPort-and-SocksPort-to-__ControlPort-a.patch (1.8 KB) - added by neel 2 months ago.
Patch to convert ControlPort and SocksPort to ControlPort and SocksPort

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 months ago by mcs

  • Keywords tbb-wants added

comment:2 Changed 3 months ago by arma

See ticket #586 for a similar issue we had with Vidalia long ago: it wanted to put a HashedControlPassword on the command-line, but writing it during saveconf made no sense since it was generated fresh each time.

The answer there was to create a separate __HashedControlSessionPassword config option. The __ part makes it not get saved during saveconf:

    /* Don't save 'hidden' control variables. */
    if (!strcmpstart(fmt->vars[i].name, "__"))
      continue;

Maybe some similar solution would be helpful here? "session control port" and "session socks port"?

comment:3 Changed 2 months ago by nickm

  • Points set to 2

Changed 2 months ago by neel

Patch to convert ControlPort and SocksPort to ControlPort and SocksPort

comment:4 Changed 2 months ago by neel

I have created a patch which converts the ControlPort and SocksPort to _ControlPort and _SocksPort.

Thank You,
Neel Chauhan

comment:5 Changed 2 months ago by nickm

What will happen with your patch if a user already has a SocksPort in their configuration?

comment:6 Changed 2 months ago by neel

This patch would use the SocksPort defined in the torrc unless a SocksPort specified at the command line.

UPDATE: The same thing happens when I just use Tor from FreeBSD Ports, without my patches.

Last edited 2 months ago by neel (previous) (diff)

comment:7 Changed 6 weeks ago by nickm

  • Type changed from defect to enhancement

batch modify: I think these are "enhancement", though I could be wrong about some.

comment:8 Changed 6 weeks ago by nickm

  • Owner set to nickm
  • Status changed from new to accepted

comment:9 Changed 6 weeks ago by nickm

  • Actual Points set to .2
  • Keywords 029-backport added
  • Status changed from accepted to needs_review

My branch "feature_20956_029" adds __SocksPort, etc for every kind of port, while retaining the behavior of the old SocksPort options. (Unlike the patch above, it shouldn't cause regular socksports to get dropped by SAVECONF.)

comment:10 Changed 6 weeks ago by gk

  • Cc gk added

comment:11 Changed 6 weeks ago by arma

tiny typo, "for the each time"

I didn't try running it, or hunting for other things that it forgets to fix, but I endorse the approach it takes!

comment:12 Changed 6 weeks ago by gk

mcs/brade: could you take a look whether that is what we need and whether it is working the way we need it?

comment:13 Changed 6 weeks ago by mcs

This seems to work, at least for the scenarios that Kathy and I tested (thanks Nick!).

We modified Tor Launcher to pass +__ControlPort and +__SocksPort as args when starting tor.
We also removed the default ControlPort and SocksPort lines from the torrc-defaults file that ships with Tor Browser.

With Nick's tor changes plus these Tor Launcher changes, there is no longer any interference with any additional control or SOCKS ports that users might add to their torrc. Duplicates need to be avoided though; otherwise tor will not start up (I think that is okay).

The original Tor Launcher ticket is #20761.

comment:14 Changed 5 weeks ago by dgoulet

  • Reviewer set to dgoulet
  • Status changed from needs_review to merge_ready

lgtm;

comment:15 Changed 5 weeks ago by nickm

  • Milestone changed from Tor: 0.3.0.x-final to Tor: 0.2.9.x-final

Merged to master. Should apply cleanly to 029.

comment:16 follow-up: Changed 5 weeks ago by arma

mcs: I don't think you need the + signs when using the new __SocksPort / etc args?

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

comment:17 in reply to: ↑ 16 Changed 5 weeks ago by mcs

Replying to arma:

mcs: I don't think you need the + signs when using the new __SocksPort / etc args?

Hmm. I think we do, because otherwise an __SocksPort that is passed by Tor Launcher when starting tor will replace all SocksPort definitions that are present in torrc or torrc-defaults. At least that is what I thought I observed. I will re-test.

comment:18 follow-up: Changed 4 weeks ago by nickm

This caused #21300, so I am leaning "no backport"

comment:19 in reply to: ↑ 18 ; follow-up: Changed 4 weeks ago by mcs

Replying to nickm:

This caused #21300, so I am leaning "no backport"

I think "no backport" is okay because we want this for our Tor Browser 7.0 alpha series, and we will use tor-0.3.X alpha for those releases. Is that correct gk?

comment:20 in reply to: ↑ 19 Changed 4 weeks ago by gk

Replying to mcs:

Replying to nickm:

This caused #21300, so I am leaning "no backport"

I think "no backport" is okay because we want this for our Tor Browser 7.0 alpha series, and we will use tor-0.3.X alpha for those releases. Is that correct gk?

Yes.

comment:21 Changed 4 weeks ago by nickm

  • Milestone changed from Tor: 0.2.9.x-final to Tor: 0.3.0.x-final
  • Resolution set to implemented
  • Status changed from merge_ready to closed

Okay. No backport then. Thanks!

Note: See TracTickets for help on using tickets.