IPv6 v3 Onion Services support

changed milestone to %Tor: 0.4.2.x-final

added actualpoints::0.7 component::core tor/tor ipv6 milestone::Tor: 0.4.2.x-final network-team-roadmap-august parent::29995 points::7 priority::medium prop224 resolution::fixed severity::normal single-onion sponsor::27-can status::closed tor-hs type::defect v3-onion-service-feature-parity-can labels

Trac:
Priority: High to Medium

Oh dear... hmmm proposal 224 makes IPv4 mandatory in order to extend to the relay and seems in this case you have IPv6 only?...

Is this something we want to allow right now or should I say simply possible to have NO IPv4 for a relay?

Trac:
Status: new to needs_information

About the above, see #23502 (moved) ... so until the patch is in, that test will unfortunately fail.

Replying to dgoulet:

Oh dear... hmmm proposal 224 makes IPv4 mandatory in order to extend to the relay and seems in this case you have IPv6 only?...

The single onion service in this test can only make IPv6 connections. This works for v2, but not v3.

Is this something we want to allow right now or should I say simply possible to have NO IPv4 for a relay?

We can make IPv6 work with single-hop client and service connections to intro and rend points. It works for v2 single onion services. We talked about it for v3, but it never made it into the prop224 spec.

Edit: I opened #23507 (moved) with a list of steps we can add to the spec. We should implement them in this ticket!

I don't see how #23502 (moved) is a dependency. Initiating clients/services should always include IPv4 and IPv6, even if they wouldn't use them (step 0). And responding clients/services should use a direct connection if available, or a 3-hop connection if not (steps 1 & 2).

Trac:
Status: needs_information to new

Replying to teor:

Replying to dgoulet:

Oh dear... hmmm proposal 224 makes IPv4 mandatory in order to extend to the relay and seems in this case you have IPv6 only?...

The single onion service in this test can only make IPv6 connections. This works for v2, but not v3.

Is this something we want to allow right now or should I say simply possible to have NO IPv4 for a relay?

We can make IPv6 work with single-hop client and service connections to intro and rend points. It works for v2 single onion services. We talked about it for v3, but it never made it into the prop224 spec.

Yeah, it will make it with #23502 (moved). Spec and code change for that matter. Basically, any link specifiers for the IP (in descriptor) and RP (in INTRO cell) were requiring an IPv4 else you couldn't use the relay.

Edit: I opened #23507 (moved) with a list of steps we can add to the spec. We should implement them in this ticket!

I don't see how #23502 (moved) is a dependency. Initiating clients/services should always include IPv4 and IPv6, even if they wouldn't use them (step 0). And responding clients/services should use a direct connection if available, or a 3-hop connection if not (steps 1 & 2).

In this scenario, no IPv4 were available with this chutney test so the code was just saying "nope" and you wouldn't pass step 0 which is what #23502 (moved) is addressing.

Then considering the above fixed, you are right that if the chosen IP can't be reached via a direct connection, we fallback to 3-hop, see pick_intro_point(). However, for the RP, I'm not sure there is a fallback, even for v2... See pick_rendezvous_node(), that fallback concept is only if ENABLE_TOR2WEB_MODE.

The second thing here that I want to raise is that including both IPv4 and IPv6 is not possible right now within tor and the reason is that from a node_t, we create an extend_info_t object used for opening a circuit and that object can only take one type of address, v4 or v6 (tor_addr_t addr). Furthermore, the extend_cell_format() simply doesn't care at all about IPv6. (#22810 (moved) is probably relevant and should be expanded to IPv6 also).

So, IPv6 can only be used in a direct connection really and is entirely ignored for 3-hop.

If you can confirm the above, we should open tickets about all that.

Replying to dgoulet:

Replying to teor:

Replying to dgoulet:

Oh dear... hmmm proposal 224 makes IPv4 mandatory in order to extend to the relay and seems in this case you have IPv6 only?...

The single onion service in this test can only make IPv6 connections. This works for v2, but not v3.

Is this something we want to allow right now or should I say simply possible to have NO IPv4 for a relay?

We can make IPv6 work with single-hop client and service connections to intro and rend points. It works for v2 single onion services. We talked about it for v3, but it never made it into the prop224 spec.

Yeah, it will make it with #23502 (moved). Spec and code change for that matter. Basically, any link specifiers for the IP (in descriptor) and RP (in INTRO cell) were requiring an IPv4 else you couldn't use the relay.

But this is fine at the moment.

All relays have an IPv4 address, so clients and services can put them in the link specifiers in the descriptor and introduce cell.

Edit: I opened #23507 (moved) with a list of steps we can add to the spec. We should implement them in this ticket!

I don't see how #23502 (moved) is a dependency. Initiating clients/services should always include IPv4 and IPv6, even if they wouldn't use them (step 0). And responding clients/services should use a direct connection if available, or a 3-hop connection if not (steps 1 & 2).

In this scenario, no IPv4 were available with this chutney test

I don't understand what you mean here.

The service connects over IPv6, but it has a consensus with an IPv4 address, why isn't it using it in the link specifier for the intro point in the descriptor?

so the code was just saying "nope" and you wouldn't pass step 0 which is what #23502 (moved) is addressing.

Then considering the above fixed, you are right that if the chosen IP can't be reached via a direct connection, we fallback to 3-hop, see pick_intro_point(). However, for the RP, I'm not sure there is a fallback, even for v2... See pick_rendezvous_node(), that fallback concept is only if ENABLE_TOR2WEB_MODE.

That's ok, it's only needed in Tor2web mode: all other clients connect using a 3-hop path through a guard or bridge they can reach.

The second thing here that I want to raise is that including both IPv4 and IPv6 is not possible right now within tor and the reason is that from a node_t, we create an extend_info_t object used for opening a circuit and that object can only take one type of address, v4 or v6 (tor_addr_t addr). Furthermore, the extend_cell_format() simply doesn't care at all about IPv6. (#22810 (moved) is probably relevant and should be expanded to IPv6 also).

Sending an EXTEND with IPv4 is ok at the moment, but it will need to change for relays to extend to IPv6 (for example, IPv6 ORPort self-checks). I think there is a ticket for this already.

So, IPv6 can only be used in a direct connection really and is entirely ignored for 3-hop.

Yes, at the moment.

If you can confirm the above, we should open tickets about all that.

Some already have tickets.

Trac:
Owner: N/A to dgoulet
Status: new to accepted

Ok, so I've opened some tickets for the things we need to fix. I don't think we want 0.3.2 failing with a bug warning on v3 single onion services on IPv6.

So we either need to get #23502 (moved) working, or disable that combination of config options, and the chutney test.

This comments section got complicated, so I'm going to summarise the issues in 0.3.2.1-alpha, my suggested changes in my bug23493 branch, and what we should fix in 0.3.3.

My branch bug23493 completes the implementation of the single onion service reachability algorithm in #23507 (moved). This is the minimum we need to do for functional single onion services with ReachableAddresses/ClientUseIPv6 in 0.3.2. (The alternative is to rip out some of the existing implementation, which I think is worse.)

This is how v3 single onion services will work after we merge this branch:

services choose intro points they can reach, if possible (0.3.2.1-alpha)
- if not, they choose any valid intro point (0.3.2.1-alpha)
services connect to intro points directly, if possible (0.3.2.1-alpha)
- if not, they fail to connect, or try to connect to an unreachable address (0.3.2.1-alpha)
- if not, they connect over a 3-hop path (bug23493)
services put IPv4 addresses for those intro points in the descriptor (bug23493)
clients choose rend points (0.3.2.1-alpha)
- clients know about single onion services from the descriptor, but they don't do anything different for them, and they don't need to (0.3.2.1-alpha)
clients put rend point IPv4 addresses in the INTRODUCE cell (0.3.2.1-alpha)
services choose a reachable rend address from the INTRODUCE cell, if possible (0.3.2.1-alpha)
- if not, they fail to connect, or try to connect to an unreachable address (0.3.2.1-alpha)
- if not, they connect over a 3-hop path (bug23493)

This is what we'll change in 0.3.3 for v3 onion services:

services put IPv4 and IPv6 addresses for their intro points in the descriptor (#23576 (moved))
clients put rend point IPv4 and IPv6 addresses in the INTRODUCE cell (#23577 (moved), #23589 (moved))
single onion services choose rend via direct IPv6, when IPv6 is reachable and isn't preferred, but IPv4 is unreachable (#23588 (moved))
- in 0.3.2.1-alpha, they fail in this rare case, which can only be triggered by 0.3.3 clients with fixes for #23577 (moved) and #23589 (moved)
- in bug23493, they use a 3-hop path in this rare case, which can only be triggered by 0.3.3 clients with fixes for #23577 (moved) and #23589 (moved)

I've deferred all the other child tickets to 0.3.3.

Edit: sometimes single onion services try to connect to an unreachable address

Trac:
Status: accepted to assigned
Owner: dgoulet to teor

Trac:
Status: assigned to needs_review

Trac:
Actualpoints: N/A to 0.5

Here is a short review:

Not a big fan of poking into the guts of service with service->config.is_single_onion to figure out if SoS or not. I suggest now that we are more serious about this feature to use a function like int service_is_single_onion(hs_service_t *).
direct_conn_inout is a weird variable name. Why inout and not out? Also let's improve variable naming so that this BUG makes a bit more sense if (BUG(direct_conn && direct_conn_inout && !*direct_conn_inout)) {.
7c3ba98cd is a bit sketch. I wonder how come that was not needed before. If it was not needed, is it just defense-in-depth? Can we add a non-fatal assert to make sure it never triggers? Also, is extend_info_is_a_configured_bridge() the right thing to do? What happens if the bridge has a PT on a different address than the bridge?
It's kinda scary that there is no unittests for any of the SoS HSv3 logic.

Additional minor notes:

The log message "Launching a %s circuit" will say "launching a anonymous circuit" when "an anonymous circuit" would be correct.
Looks like "make check-spaces" will fail.

for asn:

"inout" is a convention for "'in and out': the value pointer is both read and written." Not sure if that answers your "why not out" naming question?

Trac:
Status: needs_review to needs_revision

I've updated my bug23493 branch.

Replying to asn:

Here is a short review:

Not a big fan of poking into the guts of service with service->config.is_single_onion to figure out if SoS or not. I suggest now that we are more serious about this feature to use a function like int service_is_single_onion(hs_service_t *).

I was always serious about this feature :-)

And I agree, it's important to have a read-only accessor when we are passing inout references to a copy of that flag.

See commit 76d25b0b74.

That said, the existing code doesn't have accessors for any of the other flags in config, so it's up to you if you want this flag to be different.

direct_conn_inout is a weird variable name. Why inout and not out? Also let's improve variable naming so that this BUG makes a bit more sense if (BUG(direct_conn && direct_conn_inout && !*direct_conn_inout)) {.

I rewrote the function comment and made the variable naming clearer in fixup b51005940d.

7c3ba98cd is a bit sketch. I wonder how come that was not needed before.

It's needed to make sure that we don't connect to an address that's banned by ReachableAddresses, ClientUseIPv4/6, or similar options, which have been around for a long time. In previous releases, we trusted calling code to do the right thing.

If it was not needed, is it just defense-in-depth?

Yes, it's defense in depth.

Can we add a non-fatal assert to make sure it never triggers?

We could, but I think a BUG() condition is the right way to go here - it allows us to log the error, and respond correctly by refusing to connect to the address.

I also added a comment that explains the change in fixup commit 79875f084b.

Also, is extend_info_is_a_configured_bridge() the right thing to do? What happens if the bridge has a PT on a different address than the bridge?

!extend_info_is_a_configured_bridge() is the right thing to do, because bridges with PTs routinely lie about their addresses, and bridges without PTs have always been allowed to connect to their configured addresses. PT reachable addresses aren't controlled by tor, and are apparently out of scope for PT 2.0, too.

It's kinda scary that there is no unittests for any of the SoS HSv3 logic.

I agree, but I don't have any time to write any this week. Can we open a separate ticket for this in 0.3.2?

Replying to nickm:

Additional minor notes:

The log message "Launching a %s circuit" will say "launching a anonymous circuit" when "an anonymous circuit" would be correct.

Fixup 898495e6bd.

Looks like "make check-spaces" will fail.

Oops. I had that in my workflow, but it dropped out.

Fixup 15bc97ee7d.

Trac:
Actualpoints: 0.5 to 0.7
Status: needs_revision to needs_review

Sorry for the slow responses here. We've been very busy with testing and bugfixing. Here are some thoughts:

My main concerns with the current patch is that it's a bit brutal. Ideally, I'd like SoS to be a beautiful feature with a well designed interface and very specific call-sites around the codebase. I don't think that's the case currently. I'm afraid that we started developing the prop224 SoS feature too late in 0.3.2 and we don't have enough time to make it nice and well-tested. I personally take the blame here as well, since we added the SoS feature in the first place, when IMO we should have waited till we have the time to actually design it properly.

Anyhow, right now I mainly dislike the hs_get_extend_info_from_lspecs() changes and the fact that we have no tests for the new features/bugfixes:

I really don't like the patch to hs_get_extend_info_from_lspecs(). It's already a hard to digest function, and it becomes even harder. I have trouble even understanding the function documentation, with so many cases and IPv6 interactions with the SoS feature.
Furthermore, on hs_get_extend_info_from_lspecs() I don't like any of the added direct_conn stuff. direct_conn_inout is a variable that carries a value and also gets overwritten by the func? That's a new interface for Tor I think and it seems like a bad idea. Especially since it gets used and written all over the function.

Ideally I'd like 2-3 SoS-specific functions that do SoS-specific stuff only when needed and they are clearly labeled as such. I don't want any direct_conn stuff casually laying around the codebase.
Case in point:

+
+      if (ei == NULL && direct_conn) {
+        /* Try again as an indirect connection */
+        direct_conn = 0;
+        ei = get_extend_info_from_intro_point(ip, direct_conn);
+      }
+
       if (ei == NULL) {
-        if (!direct_conn) {
         /* In case of a multi-hop connection, it should never happen that we
          * can't get the extend info from the node. Avoid connection and
          * remove intro point from descriptor in order to recover from this
          * potential bug. */
         tor_assert_nonfatal(ei);
-        }

The block of code added is quite confusing to someone who is not an expert with the SoS feature. IMO, the "retry with 3-hops" SoS logic should be well-documented and abstracted in an SoS-function and not just a Try again as an indirect connection comment in the middle of that func.

Also, why was the if (!direct_conn) block removed, but not the comment? Are we sure it's a multi-hop connection at that point? If yes, let's add an assert or sth.

I'm not sure what to suggest here, since fixing the interface issues above and writing unittests is not an easy task.

Here are a few options:

a) We merge teor's patch now and hope that nothing breaks (ideally with some improvements to the issues above). We then address the added technical debt in 0.3.3 with a proper refactoring of the SoS feature.

b) We don't merge teor's patch right now , and keep things simple with less code added during the bugfix period. We also add a log message when SoS+prop224 is enabled to say that this feature is super experimental.

c) We rip out SoS entirely from prop224 codebase, and implement it properly in 0.3.3 .

I'm personally leaning towards (a) right now, especially if teor helps us address the technical issues pointed out above, and with maintaining/debugging the added features. Otherwise we could go with the (b) or (c) direction.

Trac:
Status: needs_review to needs_revision

Thanks for this comment asn and your great work teor.

I do share these concerns as well. I'll also take blame here on adding half of a SoS feature while we were re-writing the entire HS subsystem, we shouldn't have done that as we see now that it is much more complicated in terms of interface, algorithm and integration.

I've been silent in the last week about this because I've been thinking about it on what is best. My initial idea was to sit down at the Montreal meeting and discuss what would be best to go forward.

I propose we do that in any case but in the meantime, I'm slightly tempted to actually go with (c) here so we do not go in the direction of accumulating technical debt that we are unhappy with in the hope of refactoring in a later version. It usually doesn't work that smoothly... The only thing we must be sure of is for a 032 client to be able to reach a SoS which it should by design because it doesn't behave differently in the case of a SoS or non-SoS service in theory.

Then, in 033 we introduce the service functionality in a clean well discussed interface and for which we all agree about how to proceed with a SoS. The number of conditions to switch from one hop to 3 hop or v4 vs v6 is not that simple, we should nail it down in a spec or doc before we do anything more. Sprinkling direct_conn in many places of the code honestly worries me and I would like us to take a step back and sit down together so we can all be on the same page.

Thanks for these reviews, I won't be able to do any revisions for about a week. Let's work through it in Montreal.

We will add v3 single onion IPv6-only support in a later release, none of our users need this right now, and if they want it, they should use v3 hidden services (with IPv6 bridges or ClientUseIPv4 0).

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

My branch bug23820_032 is a bugfix for 0.3.2 for this issue. It stops IPv6-only v3 single onion services from being configured, and makes them fail at rendezvous with a warningif someone manages to configure them.

But we still need to fix this properly for 0.3.3.

Trac:
Status: needs_revision to new

Disowning this ticket, because I won't be able to do it in 0.3.3.

We have to do these tasks in a particular order to minimise information leaks:

IPv6 single onion services
IPv6 relay extends
IPv6 relay reachability (provides cover traffic for IPv6 client extends)
IPv6 client extends for exit circuits
IPv6 client extends for multi-hop onion service circuits

Trac:
Owner: teor to N/A
Status: new to assigned

Mark all tickets that are assigned to nobody as "new".

Trac:
Status: assigned to new

Moving a bunch of tickets from 033 to 034.

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

Trac:
Keywords: N/A deleted, 034-triage-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

Trac:
Keywords: N/A deleted, 034-removed-20180328 added

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

Trac:
Milestone: Tor: 0.3.4.x-final to Tor: unspecified

Trac:
Sponsor: N/A to Sponsor27

Trac:
Keywords: N/A deleted, v3-onion-service-feature-parity-can added

Lots of children ticket here, bumping up the points.

Trac:
Points: 1 to 7

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

Trac:
Sponsor: Sponsor27 to Sponsor27-must

Trac:
Summary: IPv6 v3 Single Onion Services fail with a bug warning to IPv6 v3 Onion Services support

Add keyword for tickets in the network team roadmap.

Trac:
Keywords: N/A deleted, network-team-roadmap-2019-Q1Q2 added

#23818 (moved) and #23507 (moved) are sponsor 27 must, the rest are not feature parity.

Trac:
Sponsor: Sponsor27-must to Sponsor27-can

Trac:
Keywords: network-team-roadmap-2019-Q1Q2, 034-removed-20180328, 034-triage-20180328 deleted, network-team-roadmap-august added

This is a parent ticket: it can close once all its child tickets close.

All the child tickets have been fixed and merged to master, closing.

Trac:
Resolution: N/A to fixed
Milestone: Tor: unspecified to Tor: 0.4.2.x-final
Status: new to closed

closed

changed time estimate to 56h

added 5h 36m of time spent

mentioned in issue #23502 (moved)

mentioned in issue #23507 (moved)

mentioned in issue #23576 (moved)

mentioned in issue #23577 (moved)

mentioned in issue #23589 (moved)

mentioned in issue #23740 (moved)

mentioned in issue #23759 (moved)

mentioned in issue #23818 (moved)

IPv6 v3 Onion Services support

Child items ...

Activity