Opened 11 months ago

Last modified 3 weeks ago

#20956 reopened defect

optionally do not write command line config to torrc

Reported by: mcs Owned by: nickm
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version: Tor: 0.3.0.3-alpha
Severity: Normal Keywords: tbb-needs
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 11 months ago.
Patch to convert ControlPort and SocksPort to ControlPort and SocksPort

Download all attachments as: .zip

Change History (30)

comment:1 Changed 11 months ago by mcs

Keywords: tbb-wants added

comment:2 Changed 11 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 11 months ago by nickm

Points: 2

Changed 11 months ago by neel

Patch to convert ControlPort and SocksPort to ControlPort and SocksPort

comment:4 Changed 11 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 11 months ago by nickm

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

comment:6 Changed 11 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 11 months ago by neel (previous) (diff)

comment:7 Changed 10 months ago by nickm

Type: defectenhancement

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

comment:8 Changed 10 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:9 Changed 10 months ago by nickm

Actual Points: .2
Keywords: 029-backport added
Status: acceptedneeds_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 10 months ago by gk

Cc: gk added

comment:11 Changed 10 months 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 10 months 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 10 months 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 10 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewmerge_ready

lgtm;

comment:15 Changed 10 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.2.9.x-final

Merged to master. Should apply cleanly to 029.

comment:16 Changed 10 months ago by arma

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

Last edited 10 months ago by arma (previous) (diff)

comment:17 in reply to:  16 Changed 10 months 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 Changed 10 months ago by nickm

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

comment:19 in reply to:  18 ; Changed 10 months 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 10 months 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 10 months ago by nickm

Milestone: Tor: 0.2.9.x-finalTor: 0.3.0.x-final
Resolution: implemented
Status: merge_readyclosed

Okay. No backport then. Thanks!

comment:22 Changed 9 months ago by meejah

Testing against master (9af76a9) for existing control-port using software, this enhancement changes the "type" given by getinfo config/names from LineList to Dependent (which is also a spelling change from last release where it's Dependant).

Prior to now, the only thing which had the Virtual + Dependent/Dependant things was the hidden-service stuff (and only because order matters, according to control-spec).

Ideally, SocksPort et al. would still report their type as LineList. Also, my understanding of the use of Virtual + Dependent was for options whose order matters. This doesn't appear to be the case here (unless I'm missing something?). If these *do* need to be Virtual then control-spec should be updated.

txtorcon uses getinfo config/names to programmatically decide which parsers to use for options; I don't know if other control-port software does as well. I could of course just special-case all these new ones (which is the only solution currently) but the change from LineList -> Dependant seems unnecessary for this feature.

(As an aside, I would personally like to see Virtual + Dependent go away entirely as it's the only special-case weird order-dependent thing in SETCONF).

comment:23 Changed 9 months ago by meejah

Resolution: implemented
Status: closedreopened

comment:24 in reply to:  22 Changed 9 months ago by teor

Keywords: 029-backport removed
Type: enhancementdefect
Version: Tor: 0.3.0.3-alpha

This is now a defect, the original feature was released in 0.3.0.3-alpha.

Replying to meejah:

Testing against master (9af76a9) for existing control-port using software, this enhancement changes the "type" given by getinfo config/names from LineList to Dependent

(which is also a spelling change from last release where it's Dependant).

That was #18146.

Prior to now, the only thing which had the Virtual + Dependent/Dependant things was the hidden-service stuff (and only because order matters, according to control-spec).

Ideally, SocksPort et al. would still report their type as LineList. Also, my understanding of the use of Virtual + Dependent was for options whose order matters. This doesn't appear to be the case here (unless I'm missing something?). If these *do* need to be Virtual then control-spec should be updated.

txtorcon uses getinfo config/names to programmatically decide which parsers to use for options; I don't know if other control-port software does as well. I could of course just special-case all these new ones (which is the only solution currently) but the change from LineList -> Dependant seems unnecessary for this feature.

(As an aside, I would personally like to see Virtual + Dependent go away entirely as it's the only special-case weird order-dependent thing in SETCONF).

meejah also noted on IRC that "getconf SocksPortLines" doesn't work.

This sounds a lot like #21300.

comment:25 Changed 9 months ago by meejah

Thanks @teor (sorry, I saw your advice to file a new issue too late)

I tested getconf SocksPortLines with an up-to-date master when I wrote the above (and just now) and it works as expected, so that's not an issue.

What I think should change is:

  • Spell it Dependant (if this is kept at all; maybe the *Lines things can be LineList too, since that's what they return)
  • return "LineList" for *Ports from GETINFO config/names

comment:26 Changed 9 months ago by meejah

(Actually, the Dependent / Dependant thing affects hidden-service options too)

comment:27 Changed 6 months ago by nickm

Milestone: Tor: 0.3.0.x-finalTor: 0.3.2.x-final

comment:28 Changed 2 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.4.x-final

comment:29 Changed 3 weeks ago by gk

Keywords: tbb-needs added; tbb-wants removed

Let's just use tbb-needs and use priority there to indicate how urgent we'd like to have those tickets fixed.

Note: See TracTickets for help on using tickets.