v3 Onion Services: don't make IPv4 mandatory because one day we'll have IPv6 only relays

changed milestone to %Tor: unspecified

added 034-triage-20180328 component::core tor/tor milestone::Tor: unspecified parent::24403 priority::medium prop224 reviewer::asn severity::normal status::new tor-hs tor-relay type::defect labels

Spec: ticket23502_01 Code: ticket23502_032_01

Trac:
Reviewer: N/A to asn
Status: assigned to needs_review

Aight. Good stuff here! Let's add some unittests and we can call this merge_ready!

Trac:
Status: needs_review to needs_revision

Spec: ticket23502_01 Code: ticket23502_032_01

Trac:
Status: needs_revision to needs_review

Spec branch looks great.

Code + tests branch looks good. Two minor nitpicks: Let's add a comment on the top of the test explaining what it does. Also, there is a /* IPv4. */ when it should be IPv6.

Let me know if you want me to do those fixes.

Trac:
Status: needs_review to needs_revision

David's branch looks good now after latest revisions.

Trac:
Status: needs_revision to merge_ready

Seems okay to me. What does teor think about this? I know he has a deep understanding on IPv6 issues, and I wonder if we're about to do something wise or foolish.

Trac:
Cc: N/A to teor

The spec seems sensible, and the fix to the code looks correct.

I think this is a good idea, because it will also make it possible to use the common link specifier code to connect to IPv6-only bridges in future.

Before we merge this, I want to make sure we understand the IPv6 single onion bug in #23493 (moved). The current code should provide IPv4 for all relays, even if we connect to them over IPv6. (If we fix #23493 (moved) by ignoring missing IPv4 in descriptors and INTRODUCE cells, that's the wrong fix. The right fix is to provide both addresses if we know both for the relay.)

Hm, this branch makes the #23310 (moved) test_client_pick_intro() unittest fail because it can now return ipv6 only intro addresses that routerset_parse can't parse. Putting this back in needs_revision to address this.

Trac:
Status: merge_ready to needs_revision

Replying to asn:

Hm, this branch makes the #23310 (moved) test_client_pick_intro() unittest fail because it can now return ipv6 only intro addresses that routerset_parse can't parse. Putting this back in needs_revision to address this.

Should be fixed part of #23310 (moved).

Replying to teor:

Before we merge this, I want to make sure we understand the IPv6 single onion bug in #23493 (moved). The current code should provide IPv4 for all relays, even if we connect to them over IPv6. (If we fix #23493 (moved) by ignoring missing IPv4 in descriptors and INTRODUCE cells, that's the wrong fix. The right fix is to provide both addresses if we know both for the relay.)

Right so if I can summarize the bug here. In prop224 code (and actually most part of tor), we go from a node_t object (that supports having IPv4 and IPv6 within the descriptor) to an extend_info_t that has only one single IP address chosen depending on some conditions and finally we take that object and create the link specifiers.

So, in the end, the link spec only gets either v4 or v6, not both.

Seems a bit weird by design to make extend_info_t have both addresses because then the circuit subsystem will have to take yet another decision on which one to use so maybe what we need is a "node_t -> link spec" layer and then "link spec -> extend_info_t" which then any subsystem can take its decision on what address to use.

OR, we make extend_info_t object have both address and we hint in it what order the circuit subsystem should use like for instance "IPv4 first, if fail IPv6" or "IPv4 only" or ...

Thoughts?

Trac:
Status: needs_revision to merge_ready

teor said:

Before we merge this, I want to make sure we understand the IPv6 single onion bug in #23493 (moved).

I'm okay with holding off until we've heard from teor to merge this.

Summarizing a bit the IRC discussion and conclusion for the merge_ready state.

This patch makes it that we can possibly encode an IPv6 only relay that is NO IPv4. Right now, it is not possible for a relay to not have an IPv4 in tor so the HS subsystem will always prefer IPv4 in case of a 3-hop.

For single onion now, the IPv6 is preferred if (1) firewall allow it, and (2) it is available of course.

Now, the other issue is that both IPv4 and IPv6 of a relay can NOT be included in the link specifier list with current code. It is v4 and if not there, v6 if possible. If none are available, we go on error and the node_t is discarded.

For 032 as the feature freeze is today, I think it is OK with what we have and we'll make a real IPv6 support for 033 if we can.

Sounds about right?

Replying to dgoulet:

Summarizing a bit the IRC discussion and conclusion for the merge_ready state.

This patch makes it that we can possibly encode an IPv6 only relay that is NO IPv4. Right now, it is not possible for a relay to not have an IPv4 in tor so the HS subsystem will always prefer IPv4 in case of a 3-hop.

For single onion now, the IPv6 is preferred if (1) firewall allow it, and (2) it is available of course.

Available where? (We really need a spec for this, because there's not enough detail on this ticket.)

Here are the 3 scenarios that matter:

A single onion service can connect to its intro points using the IPv6 address in their microdescriptor, or connect over a 3-hop path to the IPv4 address in the consensus.
A single onion service should put the IPv4 and IPv6 addresses in the link specifiers in the HS descriptor. But since we can't add IPv6 right now, and no clients need it (there's no v3 Tor2web), we can leave it, and do it in 0.3.3 for completeness.
A single onion service should connect to the rend point using the IPv6 address in the link specifiers in the INTRODUCE cell, or if there is no IPv6, it should connect over a 3-hop path to the IPv4 link specifier.

client: it's ok that clients can only provide an IPv4 link specifier in 0.3.2, we can add IPv6 in 0.3.3
service: it's ok that services only get an IPv4 link specifier from 0.3.2 clients. Do we want to support future IPv6 in 0.3.2, or leave it until we can test it in 0.3.3?

Now, the other issue is that both IPv4 and IPv6 of a relay can NOT be included in the link specifier list with current code. It is v4 and if not there, v6 if possible. If none are available, we go on error and the node_t is discarded.

If we can only have IPv4 or IPv6, we must put IPv4 in every link specifier list. IPv4 is the only protocol all clients and services know how to access or fall back to. It doesn't matter if one side only supports IPv6 or whatever, it must be IPv4.

For 032 as the feature freeze is today, I think it is OK with what we have and we'll make a real IPv6 support for 033 if we can.

This is ok for the moment, and it means that single onion v3 over IPv6 should work in 0.3.2 using the IPv4 fallback code.

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.3.x-final

This patch doesn't work, it fails:

chutney/tools/test-network.sh --flavour single-onion-v23-ipv6

with:

Warning: Couldn't extend circuit to new point $263A27537C064BEE62ECF7D8670D70BD0B5504C4~ at ::1. Number: 9
Warning: Couldn't extend circuit to new point $CCD29F70D3B95E13B5512D1F5B868C69D402F182~ at ::1. Number: 6
Warning: Couldn't extend circuit to new point $DDD5C375894E1B71EBA4A76E4FBFA3A960F7F8C7~ at ::1. Number: 8
Warning: circuit_receive_relay_cell (backward) failed. Closing. Number: 36
Warning: circuit_send_intermediate_onion_skin: Bug: Trying to extend to a non-IPv4 address. (on Tor 0.3.2.0-alpha-dev 1c52d39989fa5ccd) Number: 59
Warning: connection_edge_process_relay_cell (at origin) failed. Number: 36

Trac:
Status: merge_ready to needs_revision

Let's have these all in the same place.

Trac:
Parent: N/A to #23493 (moved)

v3 Onion Services: don't make IPv4 mandatory because one day we'll have IPv6 only relays

Child items ...

Activity