[tor-bugs] #23493 [Core Tor/Tor]: IPv6 v3 Single Onion Services fail with a bug warning

[Tor Bug Tracker & Wiki]

#23493: IPv6 v3 Single Onion Services fail with a bug warning
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  teor
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.2.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  prop224, tor-hs, single-onion, ipv6  |  Actual Points:  0.7
Parent ID:                                       |         Points:  1
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 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.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23493#comment:16>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online