[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #23493 [Core Tor/Tor]: IPv6 v3 Single Onion Services fail with a bug warning
#23493: IPv6 v3 Single Onion Services fail with a bug warning
-------------------------------------------------+-------------------------
Reporter: teor | Owner: teor
Type: defect | Status:
| needs_review
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 teor):
* status: needs_revision => needs_review
* actualpoints: 0.5 => 0.7
Comment:
I've updated my bug23493 branch.
Replying to [comment:13 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 [comment:14 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.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23493#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs