Opened 3 years ago

Last modified 9 days ago

#19327 new enhancement

controller: expose fine-grained circuit detail.

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.4.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-control isolation test-support intro
Cc: aveuiller Actual Points:
Parent ID: #17284 Points: 5
Reviewer: nickm Sponsor:

Description

circuits have lots of fields on them, and not all are currently exposed via getinfo. For testing, it might be useful to list more.

Child Tickets

Change History (21)

comment:1 Changed 3 years ago by nickm

Component: - Select a componentCore Tor/Tor
Keywords: tor-controller-extension added
Milestone: Tor: 0.2.9.x-final
Sponsor: SponsorS-can
Type: defectenhancement

comment:2 Changed 3 years ago by cypherpunks

What I would like to see are the full isolation parameters like SOCKS listen IP address / port / username / password (if the circuit was bound) and traffic stats (received / sent bytes).

Those traffic stats would also be really nice to have in both circuit and stream CLOSED events.

comment:3 Changed 3 years ago by isabela

Keywords: isaremoved added
Milestone: Tor: 0.2.9.x-finalTor: 0.2.???

comment:4 Changed 3 years ago by teor

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

Milestone renamed

comment:5 Changed 3 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:6 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:7 Changed 2 years ago by nickm

Keywords: isaremoved removed

comment:8 Changed 2 years ago by dgoulet

Keywords: tor-control added; tor-controller-extension removed

Unify tor-controller-extension keyword to "tor-control".

comment:9 Changed 2 years ago by nickm

Keywords: isolation test-support intro added

comment:10 Changed 4 weeks ago by aveuiller

Hello,

I'm willing to work on this task, as it seems a good first issue on the project.

If I understand well, that's a control socket that will only be used for monitoring or debug,
so the size of the output may not be a problem?

I looked at the origin_circuit_st struct and wanted to be sure that I understood the meaning and
importance of the different attributes in the call result.
So I made a listing of the printed attributes, as well as the one I intend to add on the output
and the one remaining, so you can assert that I did not misunderstood some of them.

The following attributes seem to be used in the controller output:

  • global_identifier
  • build_state
  • cpath
  • hs_ident
  • rend_data
  • socks_username_len
  • socks_password_len
  • socks_username
  • socks_password

The following attributes seem to be interesting to add with the proposed keyword:

  • associated_isolated_stream_global_id (ASSOCIATED_STREAM_ID)
  • circuit_idle_timeout (IDLE_TIMEOUT)
  • n_read_circ_bw (BYTE_READ_BW)
  • n_written_circ_bw (BYTE_WRITTEN_BW)
  • n_delivered_read_circ_bw (CELL_READ_BW)
  • n_delivered_written_circ_bw (CELL_WRITTEN_BW)
  • n_overhead_read_circ_bw (CELL_OVER_READ_BW)
  • n_overhead_written_circ_bw (CELL_OVER_WRITTEN_BW)
  • relaxed_timeout (HAS_RELAXED_TIMEOUT)
  • remaining_relay_early_cells (REMAINING_RELAY_EARLY_CELLS)
  • is_ancient (IS_ANCIENT)
  • has_opened (HAS_OPENED)
  • unusable_for_new_conns (UNUSABLE_FOR_NEW_CONNS)
  • padding_negotiation_failed (PADDING_NEGOTIATION_FAILED)
  • path_state (PATH_STATE)
  • hs_circ_has_timed_out (HS_HAS_TIMED_OUT)
  • isolation_values_set (ISOLATION_SET)
  • isolation_flags_mixed (ISOLATION_MIXED)
  • isolation_any_streams_attached (ANY_STREAM_ATTACHED)
  • client_proto_type (CLIENT_PROTO_TYPE)
  • client_proto_socksver (CLIENT_PROTO_SOCKSVER)
  • dest_port (DEST_PORT)
  • client_addr (CLIENT_ADDR)
  • dest_address (DEST_ADDRESS)
  • session_group (SESSION_GROUP)
  • nym_epoch (NYM_EPOCH)
  • prepend_policy (PREPEND_POLICY)

This makes the following attributes remaining non-printed:

  • p_streams
  • half_streams
  • guard_state
  • global_origin_circuit_list_idx
  • pathbias_probe_id
  • pathbias_probe_nonce
  • hs_service_side_rend_circ_has_been_relaunched
  • relay_early_commands
  • relay_early_cells_sent
  • next_stream_id
  • intro_key

comment:11 Changed 4 weeks ago by teor

Status: newneeds_review

Thanks!

I'm marking this ticket as needs_review, so someone can review the proposed design.

comment:12 Changed 4 weeks ago by asn

Reviewer: nickm

comment:13 Changed 4 weeks ago by nickm

This is an interesting design question: do we want to expose everything here, or just stuff where we have a specific use-case for having it? Ideally, we should only expose things where any implementation of the Tor protocol might also want to expose them.

I'd suggest ignoring isolation stuff for now, since there is work on progress for that stuff with ticket #19859. That includes all the stuff isolate_*, nym_epoch, and the things between client_proto_type and associated_isolated_stream_global_id in the source code. If we expose those, we'll want to do so in some way that's compatible with how we expose the same information for streams.

For implementation strategy, I suggest the following:

  • Let's pick an fields set of options to add. Probably some of the above fields will be easier and
  • Then, let's get documentation for control-spec.txt that explains the new fields, so we can make sure that we are explaining them right. This is spec documentation, so it needs to explain what the fields are _guaranteed_ to mean, not what they actually mean in today's Tor. If there are any fields that might go away in the future, let's document that too.
  • Once that's settled, let's figure out how we do tests for these. One option is a unit test that constructs a dummy origin_circuit_t object and calls the appropriate function to serialize it for a controller. Another option would be a new test in Stem.
  • Once we have a specification and a testing strategy, let's start merging the new fields. Let's do a branch that implements just one or two of them at first, so that we can make sure we're on the sampe page about implementation and testing strategy. After that's done, we can figure out our best approach for doing the rest.

(Next I'll look at each of the options you mentioned above and make some suggestions.)

comment:14 in reply to:  10 Changed 4 weeks ago by nickm

Replying to aveuiller:

I looked at the origin_circuit_st struct and wanted to be sure that I understood the meaning and
importance of the different attributes in the call result.

That's a good place to start! I'd suggest also looking at the members of circuit_t, since every origin_circuit_t contains a circuit_t object. If you want to do an initial breakdown of those, I can look at that list too.

The following attributes seem to be interesting to add with the proposed keyword:

  • associated_isolated_stream_global_id (ASSOCIATED_STREAM_ID)
  • isolation_values_set (ISOLATION_SET)
  • isolation_flags_mixed (ISOLATION_MIXED)
  • isolation_any_streams_attached (ANY_STREAM_ATTACHED)
  • client_proto_type (CLIENT_PROTO_TYPE)
  • client_proto_socksver (CLIENT_PROTO_SOCKSVER)
  • dest_port (DEST_PORT)
  • client_addr (CLIENT_ADDR)
  • dest_address (DEST_ADDRESS)
  • session_group (SESSION_GROUP)
  • nym_epoch (NYM_EPOCH)

These are all isolation-related, so let's hold off on them till we have a design for #19859

  • n_read_circ_bw (BYTE_READ_BW)
  • n_written_circ_bw (BYTE_WRITTEN_BW)
  • n_delivered_read_circ_bw (CELL_READ_BW)
  • n_delivered_written_circ_bw (CELL_WRITTEN_BW)
  • n_overhead_read_circ_bw (CELL_OVER_READ_BW)
  • n_overhead_written_circ_bw (CELL_OVER_WRITTEN_BW)

These fields are already exposed via the CIRC_BW controller event; they aren't really meaningful on their own.

  • remaining_relay_early_cells (REMAINING_RELAY_EARLY_CELLS)
  • circuit_idle_timeout (IDLE_TIMEOUT)
  • unusable_for_new_conns (UNUSABLE_FOR_NEW_CONNS)
  • padding_negotiation_failed (PADDING_NEGOTIATION_FAILED)
  • path_state (PATH_STATE)

This are potentially useful.

  • relaxed_timeout (HAS_RELAXED_TIMEOUT)
  • is_ancient (IS_ANCIENT)
  • has_opened (HAS_OPENED)
  • prepend_policy (PREPEND_POLICY)
  • hs_circ_has_timed_out (HS_HAS_TIMED_OUT)

These will be a little tricky to document: they all have somewhat complicated logic. They were all originally used to implement code of the form "Remember when X has happened, so we can do Y", and that's what their documentation says. But it's possible that the conditions when we want to do Y, or the ways in which we respond to X, will change over time. So we should document which condition we actually want each of the controller flags to represent: does it represent "this is a field where X has happened" or "this is a field where we Y"? We should carefully document what each one actually means in practice.

This makes the following attributes remaining non-printed:

  • p_streams
  • half_streams
  • guard_state

These might be worth potentially exposing some information about in the future, but it doesn't have to be now.

  • relay_early_cells_sent

This is worth exposing, and easy to expose.

  • global_origin_circuit_list_idx

This is an implementation detail and should not be exposed.

  • pathbias_probe_id
  • pathbias_probe_nonce
  • hs_service_side_rend_circ_has_been_relaunched

I don't think there's any reason to expose these, but I could be wrong.

  • relay_early_commands

I think we should remove this field; it doesn't seem to be doing anything useful any more, and nobody has encountered bug #878 for a long time.

  • next_stream_id

We could expose this, but I don't see any reason to do so.

  • intro_key

We shouldn't expose this.

comment:15 Changed 4 weeks ago by nickm

Cc: aveuiller added
Points: 25
Sponsor: SponsorS-can
Status: needs_reviewnew

comment:16 in reply to:  13 Changed 4 weeks ago by teor

Replying to nickm:

For implementation strategy, I suggest the following:

  • Let's pick an fields set of options to add. Probably some of the above fields will be easier and

It looks like this sentence got a bit mixed up, what did you mean?

comment:17 Changed 4 weeks ago by nickm

Sorry, sometimes I start writing a sentence, then I start writing another sentence, and another, and I forget to finish the first.

That should probably read something like:

Let's pick a set of fields to add. Probably some of the above fields will be easier and more useful than others.

comment:18 Changed 4 weeks ago by aveuiller

Thanks for the detailed answer!

I'll work on a first round of integration on the specs, implementation, and tests for a few fields and I'll come back to you.

In the meantime, I'll also do the same specification work on the circuit_t structure.

comment:19 Changed 3 weeks ago by aveuiller

Hello,

Here is the duo of pull requests regarding the implementation of 3 firsts entries in the circuit-info output:

comment:20 Changed 2 weeks ago by teor

Status: newneeds_review

comment:21 Changed 9 days ago by nickm

Milestone: Tor: unspecifiedTor: 0.4.3.x-final
Status: needs_reviewnew

Sorry for the delay. The code looks good; I think you should feel free to proceed in this vein. The spec change could use more clarity; I've made some suggestions on the PR there.

Note: See TracTickets for help on using tickets.