Opened 15 months ago

Last modified 7 months ago

#23502 new defect

prop224: Don't make IPv4 mandatory because one day we'll have IPv6 only relays

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-relay, tor-hs, 034-triage-20180328
Cc: teor Actual Points:
Parent ID: #23493 Points:
Reviewer: asn Sponsor:

Description

Right now it is not possible to have a relay that is IPv6 *only* in the public network but one day it will.

Proposal 224 makes IPv4 mandatory for link specifiers in order for a relay to extend to it. It would be wise to NOT make it mandatory and instead make it mandatory to at least have IPv4 or IPv6.

Child Tickets

Change History (29)

comment:1 Changed 15 months ago by dgoulet

Reviewer: asn
Status: assignedneeds_review

Spec: ticket23502_01
Code: ticket23502_032_01

comment:2 Changed 15 months ago by asn

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

comment:3 Changed 15 months ago by dgoulet

Status: needs_reviewneeds_revision

comment:4 Changed 15 months ago by dgoulet

Status: needs_revisionneeds_review

Spec: ticket23502_01
Code: ticket23502_032_01

comment:5 Changed 15 months ago by asn

Status: needs_reviewneeds_revision

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.

comment:6 Changed 15 months ago by asn

Status: needs_revisionmerge_ready

David's branch looks good now after latest revisions.

comment:7 Changed 15 months ago by nickm

Cc: teor added

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.

comment:8 Changed 15 months ago by 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. The current code should provide IPv4 for all relays, even if we connect to them over IPv6. (If we fix #23493 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.)

comment:9 Changed 15 months ago by asn

Status: merge_readyneeds_revision

Hm, this branch makes the #23310 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.

comment:10 in reply to:  9 Changed 15 months ago by dgoulet

Replying to asn:

Hm, this branch makes the #23310 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.

comment:11 in reply to:  8 Changed 15 months ago by dgoulet

Replying to teor:

Before we merge this, I want to make sure we understand the IPv6 single onion bug in #23493. The current code should provide IPv4 for all relays, even if we connect to them over IPv6. (If we fix #23493 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?

comment:12 Changed 15 months ago by dgoulet

Status: needs_revisionmerge_ready

comment:13 Changed 15 months ago by nickm

teor said:

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

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

comment:14 Changed 15 months ago by 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.

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?

comment:15 in reply to:  14 Changed 15 months ago by teor

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:

  1. 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.
  1. 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.
  1. 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.

comment:16 Changed 15 months ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

comment:17 Changed 15 months ago by teor

Status: merge_readyneeds_revision

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

comment:18 Changed 15 months ago by teor

Parent ID: #23493

Let's have these all in the same place.

comment:19 Changed 15 months ago by teor

These tickets aren't urgent, and they involve major refactoring.
Deferring to 0.3.3

comment:20 Changed 15 months ago by teor

For the bugs in 0.3.2.1-alpha, and the current plan for 0.3.2 and 0.3.3, see https://trac.torproject.org/projects/tor/ticket/23493#comment:9

comment:21 Changed 15 months ago by teor

Milestone: Tor: 0.3.3.x-finalTor: 0.3.2.x-final

I think this is a good idea, and I'd be happy to include it in 0.3.2 if it's rebased on top of my bug23493.

At the very least, we need to make sure we won't crash if IPv4 is left out, and we should just discard the particular link specifier and try again.

comment:22 Changed 14 months ago by dgoulet

Status: needs_revisionmerge_ready

Considering that we won't be shipping IPv6 single onion in 032 (#23493), I think this patch should go back in merge_ready. Actually, anything IPv6 related for HS will be removed from the code base so we can go clean with IPv6 support in 033 (for single and HS).

Reminder:

Code: ticket23502_032_01 (has a fixup commit)
Spec: ticket23502_01

comment:23 Changed 14 months ago by nickm

Status: merge_readyneeds_revision

Small request on the commit: it looks like this commit changes the order of link specifiers that we generate in service_intro_point_new(), which introduces an unnecessary difference between versions. Can we return to the old order?

Also, you say above that the ticket "has a fixup commit". But I don't see it; I only see 1c52d39989fa5ccd8c2c895cdc4eb0c704e0bda3 and 65cd35ce73adc09fece995636ee2622ac77ad827

Otherwise this looks okay to me.

comment:24 Changed 14 months ago by teor

Parent ID: #23493#23820

I'm happy to get this merged after nickm's requested fixes. And then I can write #23820 on top of it, which will involve ripping out all of the v3 onion IPv6 code so we can do it properly in 0.3.3.

comment:25 Changed 14 months ago by teor

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Parent ID: #23820#23493
Status: needs_revisionneeds_information

I just reviewed this code in detail, and I don't think we can include it as a bugfix in 0.3.2. And I don't think we can merge it to 0.3.3 as it is, because we need to work out some design issues first. See my detailed review below.

Also, I think we should rebase this patch on top of #23820, which will rip out all of the IPv6 code as a bugfix on 0.3.2.

But I think we should merge the spec branch.

Code review:

circuit_send_intermediate_onion_skin() and extend_cell_format() assume that all extend infos sent to them contain IPv4 addresses. So this code will trigger a bug when the rendezvous link specifiers from the untrusted remote client contain only an IPv6 address. In particular, you will end up packing 0.0.0.0 into the extend cell, rather than the intended IPv6 address. I've split this bug in those functions off into #24000. (Winner!)

We can't choose IPv6 extends until we support them, so this part of the code results in a bug, and the following comment is confusing:

   /* By default, we pick IPv4 but this might change to v6 if certain
-   * conditions are met. */
-  addr = &addr_v4; port = port_v4;
+   * conditions are met or we simply don't have v4. */
+  if (have_v4) {
+    addr = &addr_v4; port = port_v4;
+  } else {
+    tor_assert(have_v6);
+    addr = &addr_v6; port = port_v6;
+  }

   /* If we are NOT in a direct connection, we'll use our Guard and a 3-hop
    * circuit so we can't extend in IPv6 by default because we don't know if
    * the middle hop will be able to. And at this point, we do have an IPv4 or
    * IPv6 address available so go to validation. */

This existing part of the code is also buggy, because it assumes that at least one of the link specifiers will be accepted by our reachable address settings:

  /* From this point on, we have a request for a direct connection to the
   * rendezvous point so make sure we can actually connect through our
   * firewall. We'll prefer IPv6. */

  /* IPv6 test. */
  if (have_v6 &&
      fascist_firewall_allows_address_addr(&addr_v6, port_v6,
                                           FIREWALL_OR_CONNECTION, 1, 1)) {
    /* Direct connection and we can reach it in IPv6 so go for it. */
    addr = &addr_v6; port = port_v6;
    goto validate;
  }
  /* IPv4 test and we are sure we have a v4 because of the check above. */
  if (fascist_firewall_allows_address_addr(&addr_v4, port_v4,
                                           FIREWALL_OR_CONNECTION, 0, 0)) {
    /* Direct connection and we can reach it in IPv4 so go for it. */
    addr = &addr_v4; port = port_v4;
    goto validate;
  }

Also, service_intro_point_new() still suffers from a design issue: it needs to take a node, not an extend_info (see #23576).

Spec review:

Nitpick:

Saying "TLS-over-TCP IPv4 or IPv6" is ambiguous, because there is no specifier with this name. I think what you mean is "TLS-over-TCP IPv4" or "TLS-over-TCP IPv6".

Edit: too many zeroes in a ticket number

Last edited 14 months ago by teor (previous) (diff)

comment:26 Changed 14 months ago by dgoulet

Hmmm yeah so because Tor can't extend to IPv6 then simply allowing only IPv6 in the INTRO cell is just plain wrong unless it is a direct connection (single onion).

In other words, when the service gets the link specifiers and it is not a single onion, it should just ignore anything IPv6 *until* we have full extend support.

I agree then, lets ignore this ticket for now until we have updated code on the single onion side and IPv6 support removed for now since Tor can't simply use it.

comment:27 Changed 11 months ago by dgoulet

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Move 033 ticket I own to 034

comment:28 Changed 9 months ago by nickm

Keywords: 034-triage-20180328 added

comment:29 Changed 7 months ago by dgoulet

Milestone: Tor: 0.3.4.x-finalTor: unspecified
Status: needs_informationnew

We don't know when we'll be able to address this so for now going into Unspecified.

Note: See TracTickets for help on using tickets.