Opened 4 months ago

Closed 4 months ago

Last modified 4 months ago

#32344 closed task (fixed)

Make immutability into a config_var_t flag

Reported by: nickm Owned by: nickm
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: network-team-roadmap-november
Cc: nickm, teor Actual Points: .2
Parent ID: #29211 Points:
Reviewer: teor Sponsor:

Description


Child Tickets

Change History (9)

comment:1 Changed 4 months ago by nickm

Reviewer: teor
Status: assignedneeds_review

Branch is ticket32344 with PR at https://github.com/torproject/tor/pull/1486.

comment:2 Changed 4 months ago by teor

Status: needs_reviewneeds_revision

Looks fine, but I don't know why some files are immutable, others are sandbox-immutable, and others can be changed at any time.

  1. Should we have a flag for sandbox-immutable? (A sandbox-immutable flag would significantly simplify my relay refactor in #32213.)
  2. Should all files be sandbox-immutable?
  3. Should we also make HiddenServiceDir (sandbox-)immutable?
  4. Should we also make Logs (sandbox-)immutable?
  5. Should we be checking changes to options that require access to extra files, like onion authentication?

comment:3 Changed 4 months ago by nickm

In addition to possible sandbox-immutable options ,we need immutability depending on other things too. I'm hoping that those can wait a little till we have more of the refactoring done, and we can define the right interface for them.

Right now I'm torn between a few not-so-good designs:

  • Have filenames be sandbox-immutable, and nothing else?
  • Have a new flag for every kind of immutability we need?
  • Have a list of options for every kind of immutability we need?

Do you have any thoughts on the right design here?

comment:4 Changed 4 months ago by teor

How many kinds of immutability do we need?

Flags work well if we have a small number of simple kinds of immutability. But they still centralise some code.

We could let modules define an immutability function that gets run on every option?
For example, a sandbox_config module would define an immutability function that prevents filenames, IP addresses, and other options changing.
And if we have relay or dirauth immutability, then those modules could define those functions.

comment:5 Changed 4 months ago by nickm

So to summarize the situation: I think that all files need to be sandbox-immutable.

I think that we'll come up with the right solution there when we do a refactoring for the sandbox code; this could be well handled with something based on #32339, which already looks at nearly all of the files, and which needs to start looking at all of the rest too. Currently my best guess is that constraints like this would want support for scanning all filename options, along with some way for reaching inside Log and so on.

If it's all right with you, could we take some version of this branch without covering other mutability invariants? I'd like to keep the scope small-ish here, to avoid delaying the key refactoring of #30866.

comment:6 Changed 4 months ago by nickm

(Rationale for proceeding: I believe that this branch is a strict improvement over Tor's current behavior, doesn't introduce technical debt, and unblocks #30866. If you don't agree, we shouldn't proceed.)

comment:7 Changed 4 months ago by teor

Status: needs_revisionmerge_ready

Yes, I think we should go ahead, and make further changes in other tickets.

I'd like to merge #32213, #32339, and #32344 in that order, to minimise conflicts (if any). I'm just waiting for CI to pass on #32213.

comment:8 Changed 4 months ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged #32213, #32339, and #32344.

Resolved conflicts between #32339 and #32344 by taking the V_IMMUTABLE and FILENAME changes on PidFile.

comment:9 Changed 4 months ago by teor

Opened #32373 for follow up.

Note: See TracTickets for help on using tickets.