Opened 10 months ago

Last modified 5 weeks ago

#28849 needs_information enhancement

Handle dormant mode in process library and for PT's

Reported by: ahf Owned by:
Priority: High Milestone: Tor: unspecified
Component: Core Tor/Tor Version: Tor: unspecified
Severity: Normal Keywords: 042-proposed, anti-censorship-roadmap-july, 042-deferred-20190918
Cc: dcf Actual Points: 2.5
Parent ID: Points: 3
Reviewer: Sponsor: Sponsor28-must

Description

Bug #28179 makes us able to handle PT processes better and read output from stdout/stderr, but with the recent dormant mode we should figure out this interaction works for PT's.

Especially on Windows this becomes a problem because we will probably stop reading from stdout/stderr when Tor enters its dormant mode to disable the timer that ticks once a second.

Child Tickets

Change History (22)

comment:1 Changed 10 months ago by ahf

Parent ID: #28179

comment:2 Changed 10 months ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.0.x-final
Priority: MediumHigh

comment:3 Changed 9 months ago by gaba

Sponsor: Sponsor8Sponsor19

comment:4 Changed 9 months ago by nickm

Keywords: 041-proposed added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

Mark some 0.4.0.x tickets as proposed for 0.4.1.x

comment:5 Changed 9 months ago by ahf

Parent ID: #28179

comment:6 Changed 5 months ago by dcf

Dormant mode was a topic of discussion during the anti-censorship meeting today. I'll try to summarize the main points.

  • tor wants to be able to send some kind of SIGNAL DORMANT command (over stdin) to a PT, and know whether the PT has honored the command (in whatever way makes sense for its particulars).
  • For backward compatibility, tor cannot assume that the PT understands a SIGNAL DORMANT command. The PT needs to advertise that it supports the dormant feature. Two possible ways (not mutually exclusive) to do that are:
    • An initial feature negotiation, happening at the same time as CMETHOD etc. For example, tor sends FEATURES a,b,c,d and the PT responds with the subset of features it supports: FEATURES-DONE a,c,d.
    • Acknowledgements following the command, as needed at any point following the initial handshake. For example, tor sends SIGNAL DORMANT (perhaps not knowing whether the PT supports the feature) and the PT replies with DORMANT OK, or silence if it doesn't support the feature. This would be similar to the PROXY/PROXY-DONE acknowledgements from #12125.
  • Currently the only planned use for dormant-related feature negotiation is log messages or similar. tor doesn't plan to treat dormant-aware and dormant-naive PTs any differently. But knowing whether a PT supports dormant mode would allow tor, for example, to choose to kill dormant-naive PTs instead of commanding them to become dormant.
  • There's a distinction between "this PT knows how to parse the SIGNAL DORMANT command" and "this PT is actually does something differently in response to the SIGNAL DORMANT command". The distinction is important because the low-level handling of SIGNAL DORMANT is likely to be handled by goptlib, and so all goptlib-based PTs would "understand" SIGNAL DORMANT at the syntactic level. (And even if not using goptlib, they could "understand" SIGNAL DORMANT by ignoring it.) But what tor really wants to know is whether the PT actually does something to become dormant.
    • ahf suggests that there could be a separate goptlib API, called before pt.ClientSetup, that allows the PT to say that it is capable of becoming dormant. If that API is used, then goptlib would advertise support for the dormant feature, otherwise it would not. It would then be the PT's responsibility to receive SIGNAL DORMANT messages relayed by goptlib and act on them.
  • Current PTs do not pay attention to their stdin after the initial PT handshake exchange, except to monitor it for being closed.

In the anti-censorship meeting today, we brainstormed two potential designs for communicating dormant mode with PTs:

  1. A startup mode, where the PT declares during the initial exchange with tor, "I support dormant mode," and thereafter tor implicitly assumes that the PT becomes dormant when requested to do so.
  2. An acknowledgement mode, where tor sends a command to the PT to enter/leave dormant mode, and the PT responds with a message saying "OK, I am entering/leaving dormant mode." If the PT doesn't send such an acknowledgement, it means the PT doesn't support dormant mode.

Here's a third potential design to think about:

  1. An asynchronous mode, where both tor and PT send each other messages that are (at the protocol level) purely advisory: "I am entering dormant mode" and "I am leaving dormant mode." Of course the PT would take tor's "I am entering dormant mode" as a hint to enter dormant mode itself, but there would be no protocol-level binding between the messages: each side is merely keeping the other informed about its current status.

It could be a good first use of the STATUS feature. The PT→tor messages could look like

STATUS TRANSPORT=whatever DORMANT="on"
STATUS TRANSPORT=whatever DORMANT="off"
Last edited 5 months ago by dcf (previous) (diff)

comment:7 Changed 5 months ago by ahf

Status: newneeds_review

I've tried to make a proposal patch here: https://github.com/ahf/torspec/commit/49d1cf355c350816ded4ea2e7d5772042aaa9d08

I think it works like you describe in (3) that it's purely asynchronous and it always behaves as a hint to the PT that now is the time for the PT to enter its dormant state. If the PT is busy with something, it will wait a bit, and respond once its able to fulfill the request.

This spec update also contains your (1) where the Tor process and the PT negotiates supported features. If the PT does NOT announce dormant support, Tor wont send the "SIGNAL DORMANT-*" events. All events in the update must be confirmed by either Tor or the PT as per (2).

comment:8 Changed 5 months ago by dcf

Cc: dcf added

comment:9 in reply to:  7 Changed 5 months ago by dcf

Replying to ahf:

I've tried to make a proposal patch here: https://github.com/ahf/torspec/commit/49d1cf355c350816ded4ea2e7d5772042aaa9d08

This looks pretty good.

  • "The PT proxy SHOULD respond to the FEATURES message using the FEATURES-ACK message..."
    • Does the PT proxy necessarily have to wait for FEATURES before responding with FEATURES-ACK? It would be simpler if both sides just send a FEATURES line declaring their own features.
  • "...a comma-separated list of mutually supported features..."
    • Maybe the "mutually supported" isn't necessary. It's simpler if both sides just declare all the features they support. Otherwise you also have to specify what should happen if a PT proxy violates the spec and declares a feature set that is not a subset of the parent process's feature set--which is probably that the parent process should ignore the features it doesn't support, which amounts to the same.
  • "The PT proxy SHOULD respond to the FEATURES message..."
    • I think this needs some language saying what it means if the PT proxy does not respond to the FEATURES message (backward compatibility); i.e., that the PT proxy's feature set is the empty set. Symmetrically for the PT proxy's interpretation: lack of a FEATURES option means the parent process's feature set is the empty set.
  • "Example message from parent process to PT proxy: FEATURES dormant,x,y"
    "Example response from PT proxy to parent proxy: FEATURES-ACK dormant,y"
    • I would like the spec to say what should happen when the feature set is empty. Specifically, whether a space character is required before the null list, FEATURES or FEATURES .
  • "...the FEATURES message that is written to the PT proxy's standard input."
    • Why is FEATURES on standard input, and not an environment variable like every other global configuration? There's no grammar yet in the spec for parent→PT standard input messages (presumably it looks like section 3.3). If FEATURES is on standard input, what happens if it is sent more than once? What happens if it is sent more than once with different feature sets?
  • "Once the Pluggable Transport Specification version have been successfully negotiated..."
    • The spec should say when the PT proxy is allowed to send FEATURES-ACK is allowed and when it is not allowed. It has to be after VERSION. Can it come after CMETHODS DONE or SMETHODS DONE? What happens if the PT proxy sends it more than once?
  • "The PT proxy MUST acknowledge the request using: SIGNAL DORMANT-ENTER-DONE"
    • What happens if the parent process sends SIGNAL DORMANT-ENTER more than once without an intervening SIGNAL DORMANT-EXIT? Must the PT proxy acknowledge each one, even when it has not changed its own state? I.e., are the acknowledgements edge-triggered or level-triggered?
    • Personally I would remove the MUST language here as it's kind of ineffectual--can a PT proxy reply with SIGNAL DORMANT-ENTER-DONE 60 seconds later? Is the parent process going to wait and enforce the MUST? If not, it's not really a MUST. I feel the MUST language will encourage PT proxies to lie about their current state (I need to send SIGNAL DORMANT-ENTER-DONE because the spec says so, even if I currently am not capable of going dormant). What tor is really interested is not whether the PT proxy has received and understood the SIGNAL DORMANT-ENTER message, it's the points in time when the PT proxy actually changes its dormant state, so the spec should reflect that.
    • I would make the messages the same in both directions; e.g. use SIGNAL DORMANT-ENTER in both directions, without a special SIGNAL DORMANT-ENTER-DONE in the PT→parent direction.
      • Or even simplify more: DORMANT ENTER and DORMANT EXIT.

Side note, one of the features I would like tor to advertise is reads-stdout, so a PT proxy can know whether it's safe to use LOG or not.

comment:10 Changed 5 months ago by ahf

Thanks a lot for this feedback. I've updated the document now. See https://github.com/ahf/torspec/compare/feature/dormant for changes.

  • I have entirely reworded the negotiation process to make use of environment variables like the rest of the startup configuration. We now use the "TOR_PT_FEATURES" environment variable for the parent process to let the PT proxy know of its supported feature extensions. The PT proxy can announce its own set of supported feature extensions using the "FEATURES" message. I think that addresses all of your concerns in the first 6 bullets.
  • I have reworded the dormant messages to be on the form "DORMANT ENTER" and "DORMANT EXIT" and the PT proxy response is now made to match the request from the parent process.

I might have missed some details here, please let me know if you spot anything and thanks for the review! :-)

comment:11 Changed 5 months ago by ahf

Reviewer: dcf

WIP patch for the feature negotiation in the PT's: https://github.com/ahf/tor/commit/f6d74d8c4fb2306114d41d7bd25525285ff3d453

Adding dcf as reviewer here so the ticket doesn't get handled by the needs_review assignment team on Monday.

comment:12 Changed 5 months ago by ahf

Keywords: 041-proposed removed
Milestone: Tor: unspecifiedTor: 0.4.2.x-final

comment:13 Changed 5 months ago by ahf

Keywords: 042-proposed added

comment:14 Changed 5 months ago by dcf

Thanks, I think we can work with this design.

  • "If no feature extensions are supported by the parent process this variable SHOULD be left empty."
    "The PT proxy SHOULD announce its own set of supported feature extensions ..., but only if the TOR_PT_FEATURES environment variable is present."
    • I don't understand the reason for forbidding the PT proxy from sending FEATURES when TOR_PT_FEATURES is not set/present. I think the PT proxy should always be free to send FEATURES. 3.3 already says "The parent process MUST ignore lines received from PT proxies with unknown keywords," so it's safe.
    • There's an ambiguity with the terms "empty" and "present". They could mean "the environment variable is unset" or "the environment variable is set to an empty string." IMO it's best if both of those possibilities have the same meaning.
  • "less resource conserving"
    • Should be "more resource conserving".
  • "The PT proxy MUST only write the FEATURES message once during the lifetime of the PT proxy process."
    • IMO it's better to specify what should happens if this is violated; e.g., the parent process must ignore any FEATURES line after the first.

I can live without it, but because messages from the parent process to the PT proxy on stdin are a major change to the spec, it would be good to have something analogous to 3.3 (or make 3.3 more general) that says what those messages look like, what should happen if there's an unknown message (ignore), etc.

You should invite the author of ptadapter to comment on the changes, because they will have to implement the parent process half.

comment:15 Changed 5 months ago by ahf

Actual Points: 2.5
Points: 3

comment:16 Changed 5 months ago by dcf

Sorry, I didn't know you wanted me to look at this more. It looks fine to me. For more comments, I suggest asking the developer of ptadapter, and Brandon Wiley (blanu) who worked on the PT2.0 spec and said he was interested in this topic.

comment:17 Changed 5 months ago by phw

Sponsor: Sponsor19Sponsor28-must

Moving from Sponsor 19 to Sponsor 28.

comment:18 Changed 4 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:19 Changed 3 months ago by gaba

Keywords: anti-censorship-roadmap-july added; network-team-roadmap-2019-Q1Q2 removed

comment:20 Changed 5 weeks ago by teor

Status: needs_reviewneeds_information

This ticket is on the network team review list, but there's nothing we can do about it.
Putting in needs_information until the external reviews have been completed.

comment:21 Changed 5 weeks ago by nickm

Keywords: 042-deferred-20190918 added
Milestone: Tor: 0.4.2.x-finalTor: unspecified

Deferring various tickets from 0.4.2 to Unspecified.

comment:22 Changed 5 weeks ago by dcf

Reviewer: dcf
Note: See TracTickets for help on using tickets.