Rework NETINFO cell parsing and generation with trunnel
In channel_tls_process_netinfo_cell() we have:
1720 /* Decode the cell. */
1721 timestamp = ntohl(get_uint32(cell->payload));
1722 if (labs(now - chan->conn->handshake_state->sent_versions_at) < 180) {
1723 apparent_skew = now - timestamp;
1724 }
1725
1726 my_addr_type = (uint8_t) cell->payload[4];
1727 my_addr_len = (uint8_t) cell->payload[5];
1728 my_addr_ptr = (uint8_t*) cell->payload + 6;
1729 end = cell->payload + CELL_PAYLOAD_SIZE;
1730 cp = cell->payload + 6 + my_addr_len;and in connection_or_send_netinfo() we write the cell to connection buffer. Trunnel should be used.
Activity
changed milestone to %Tor: 0.4.0.x-final
https://github.com/torproject/tor/pull/370
Trac:
Status: accepted to needs_reviewhttps://github.com/torproject/tor/pull/445
Trac:
Status: needs_revision to needs_review
This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209
(FYI: you can just push the fixup on the same branch on Github, the PR will adapt. This way, we keep the discussion thread instead of spawning it to multiple PRs.)
Trac:
Status: needs_review to needs_revisionReplying to dgoulet:
This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209
In RELAY_RESOLVED there's also TTL value, which NETINFO does not have. I suppose we could define an object consisting of type-length-value sequence and use it in both cells. That would require to either: 1) Implement file include feature in trunnel (AFAIK it doesn't support that) or 2) have both RELAY_RESOLVED and NETINFO cells defined in the same trunnel file (e.g. cells.trunnel or handshake.trunnel or something).
Or we could explicitly decouple wire formats of the two cells and decide that they are independently defined. RELAY_RESOLVED addresses can have one of the five types (hostname, IPv4, IPv6, transient error, non-transient error), but does the same apply for NETINFO? Does it make sense to ever send hostname in NETINFO cell during handshake? Error conditions can always happen, but does Tor protocol specify a way to signal them when NETINFO cell is needed?
My code takes second path, but I think we need to take a step back and do a little bit of design work here and possibly a patch to tor-spec regarding how addresses are represented in Tor cells and whether or not there is/should be a dependency between common part of wire format in different cells.
Trac:
Status: needs_revision to needs_information-
Replying to rl1987:
Replying to dgoulet:
This is my main worry right now: https://github.com/torproject/tor/pull/370#pullrequestreview-167479209
In RELAY_RESOLVED there's also TTL value, which NETINFO does not have. I suppose we could define an object consisting of type-length-value sequence and use it in both cells. That would require to either: 1) Implement file include feature in trunnel (AFAIK it doesn't support that) or 2) have both RELAY_RESOLVED and NETINFO cells defined in the same trunnel file (e.g. cells.trunnel or handshake.trunnel or something).
Hmmm so I think the
TTLfield is specific to theRELAY_RESOLVEDcell so in theory we could do a trunnel definition (thus obj) that would represent an "address" as section 6.4 specifies without the TTL.Then we would use that object with
RELAY_RESOLVEDand explicitly add the TTL field. Sorta makes sense?Or we could explicitly decouple wire formats of the two cells and decide that they are independently defined. RELAY_RESOLVED addresses can have one of the five types (hostname, IPv4, IPv6, transient error, non-transient error), but does the same apply for NETINFO? Does it make sense to ever send hostname in NETINFO cell during handshake? Error conditions can always happen, but does Tor protocol specify a way to signal them when NETINFO cell is needed?
My code takes second path, but I think we need to take a step back and do a little bit of design work here and possibly a patch to tor-spec regarding how addresses are represented in Tor cells and whether or not there is/should be a dependency between common part of wire format in different cells.
The problem with changing the format is backward compatibility so changing what those cells contain is a big endeavor tbh.
Well I'm not talking about making changes in how Tor protocols de facto works and what wire format is actually being used. What I mean to change is to reword section 4.5 to be more precise about what is in the address part of NETINFO cell and refrain from mentioning section 6.4, as that confusingly says that we're reusing wire format fragment from RELAY_RESOLVED cell in NETINFO cell (which is a shaky claim, for reasons outlined above).
I propose to keep netinfo.trunnel as it is now, and also merge the following patch to torspec:
-
Replying to rl1987:
Well I'm not talking about making changes in how Tor protocols de facto works and what wire format is actually being used. What I mean is to change is to reword section 4.5 to be more precise about what is in the address part of NETINFO cell and refrain from mentioning section 6.4, as that's confusingly says that we're reusing wire format fragment from RELAY_RESOLVED cell in NETINFO cell (which is a shaky claim, for reasons outlined above).
Ok I think this is a wise thing to do. This means that the NETINFO address format becomes specific to the NETINFO cell and thus needs its own specification in tor-spec.txt. I'm fine with that and less hackish then "check other section but minus one field and also ignore some Types"...
I propose to keep netinfo.trunnel as it is now, and also merge the following patch to torspec:
This is basically it but I would use the syntax from section 5.1.2 for example. The HS specs do use that syntax all around and it is much nicer and clearer. Ultimately, we want tor-spec.txt to all follow the same style and that one from section 5.1.2 is a step in the right direction.
We should be good to go with that code with such a spec change from above.
Thanks!
Trac:
Status: needs_review to needs_revision -
This lgtm!
Spec: https://github.com/torproject/torspec/pull/46 Tor code: https://github.com/torproject/tor/pull/445
There is probably a world where we might formalize the concept of "Address" and then also use it in the RESOLVED cell but for now I'm happy with this and the code.
Trac:
Status: needs_review to merge_ready
