When we have a new descriptor ID for an onion address request, change it in
the rend_data_t object and purge the old one from the last hid serv request
cache.
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Stop using an onion address since it's not indexed with that anymore in the
last hid serv request cache. Instead use a base32 encoded descriptor ID
contained in the rend_data_t object.
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Ground works for fixing #15816. This adds the rend_data_create() function in
order to have a single place where we initialize that data structure.
Furthermore, an array of descriptor IDs is added (one per replica) so we can
keep a copy of the current id in the object. It will be used to purge the
last hid serv request cache using those descriptor IDs. When they change,
they will be replaced and the old ones will be purged from the cache.
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Big refactor of the HS client descriptor fetch functionnality. This allows
to fetch an HS descriptor using only a descriptor ID. Furthermore, it's also
possible to provide a list of HSDir(s) now that are used instead of the
automatically choosen one.
The approach taken was to add a descriptor_id field to the rend_data_t
structure so it can be used, if available, by the HS client. The onion
address field however has priority over it that is if both are set, the
onion address is used to fetch the descriptor.
A new public function is introduced called rend_client_fetch_v2_desc(...)
that does NOT lookup the client cache before fetching and can take a list of
HSDirs as a parameter.
The HSFETCH control command now uses this new function thus making it work
and final.
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Here is why:
1) v0 descriptors are deprecated since 0.2.2.1 and not suppose to be alive
in the network anymore. This function should only serve v2 version for now
as the default.
2) It should return different error code depending on what's the actual
error is. Right now, there is no distinction between a cache entry not found
and an invalid query.
3) This function should NOT test if the intro points are usable or not. This
adds some load on a function that should be "O(1)" and do one job.
Furthermore, multiple callsites actually already test that doing twice the
job...
4) While adding control event, it would be useful to be able to lookup a
cache entry without having it checking the intro points. There are also
places in the code that do want to lookup the cache entry without doing
that.
Fixes#14391
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Once a NACK is received on the intro circuit, tor tries an other usable one
by extending the current circuit to it. If no more intro points are usable,
now close the circuit. Also, it's reason is changed before closing it so we
don't report again an intro point failure and trigger an extra HS fetch.
Fixes#14224
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Once a NACK is received on the intro circuit, tor tries an other usable one
by extending the current circuit to it. If no more intro points are usable,
now close the circuit.
Fixes#14224
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
When closing parallel introduction points, the given reason (timeout)
was actually changed to "no reason" thus when the circuit purpose was
CIRCUIT_PURPOSE_C_INTRODUCE_ACK_WAIT, we were reporting an introduction
point failure and flagging it "unreachable". After three times, that
intro point gets removed from the rend cache object.
In the case of CIRCUIT_PURPOSE_C_INTRODUCING, the intro point was
flagged has "timed out" and thus not used until the connection to the HS
is closed where that flag gets reset.
This commit adds an internal circuit reason called
END_CIRC_REASON_IP_NOW_REDUNDANT which tells the closing circuit
mechanism to not report any intro point failure.
This has been observed while opening hundreds of connections to an HS on
different circuit for each connection. This fix makes this use case to
work like a charm.
Fixes#13698.
Signed-off-by: David Goulet <dgoulet@ev0ke.net>
Clients now send the correct address for their chosen rendezvous point
when trying to access a hidden service. They used to send the wrong
address, which would still work some of the time because they also
sent the identity digest of the rendezvous point, and if the hidden
service happened to try connecting to the rendezvous point from a relay
that already had a connection open to it, the relay would reuse that
connection. Now connections to hidden services should be more robust
and faster. Also, this bug meant that clients were leaking to the hidden
service whether they were on a little-endian (common) or big-endian (rare)
system, which for some users might have reduced their anonymity.
Fixes bug 13151; bugfix on 0.2.1.5-alpha.
According to control spec, longname should not contain any spaces and is
consists only of identy_digest + nickname
added two functions:
* node_get_verbose_nickname_by_id()
* node_describe_longname_by_id()
Previously, we would sometimes decide in directory_get_from_hs_dir()
to connect to an excluded node, and then later in
directory_initiate_command_routerstatus_rend() notice that it was
excluded and strictnodes was set, and catch it as a stopgap.
Additionally, this patch preferentially tries to fetch from
non-excluded nodes even when StrictNodes is off.
Fix for bug #10722. Bugfix on 0.2.0.10-alpha (the v2 hidserv directory
system was introduced in e136f00ca). Reported by "mr-4".
For now, round down to the nearest 10 minutes. Later, eliminate entirely by
setting a consensus parameter.
(This rounding is safe because, in 0.2.2, where the timestamp mattered,
REND_REPLAY_TIME_INTERVAL was a nice generous 60 minutes.)
Now that circid_t is 4 bytes long, the default integer promotions will
leave it alone when sizeof(int) == 4, which will leave us formatting an
unsigned as an int. That's technically undefined behavior.
Fixes bug 8447 on bfffc1f0fc. Bug not
in any released Tor.
Path use bias measures how often we can actually succeed using the circuits we
actually try to use. It is a subset of path bias accounting, but it is
computed as a separate statistic because the rate of client circuit use may
vary depending on use case.
Apparently some compilers like to eliminate memset() operations on
data that's about to go out-of-scope. I've gone with the safest
possible replacement, which might be a bit slow. I don't think this
is critical path in any way that will affect performance, but if it
is, we can work on that in 0.2.4.
Fixes bug 7352.
Right-shifting negative values has implementation-defined behavior.
On all the platforms we work on right now, the behavior is to
sign-extend the input. That isn't what we wanted in
auth_type_val = (descriptor_cookie_tmp[16] >> 4) + 1;
Fix for 6861; bugfix on 0.2.1.5-alpha; reported pseudonymously.
The broken behavior didn't actually hurt anything, I think, since the
only way to get sign-extension to happen would be to have the top bit
of descriptor_cookie_tmp[16] set, which would make the value of
descriptor_cookie_tmp[16] >> 4 somewhere between 0b11111111 and
0b11111000 (that is, between -1 and -8). So auth_type_val would be
between -7 and 0. And the immediate next line does:
if (auth_type_val < 1 || auth_type_val > 2) {
So the incorrectly computed auth_type_val would be rejected as
invalid, just as a correctly computed auth_type_val would be.
Still, this stuff shouldn't sit around the codebase.
When I removed version_supports_begindir, I accidentally removed the
mechanism we had been using to make a directory cache self-test its
directory port. This caused bug 6815, which caused 6814 (both in
0.2.4.2-alpha).
To fix this bug, I'm replacing the "anonymized_connection" argument to
directory_initiate_command_* with an enumeration to say how indirectly
to connect to a directory server. (I don't want to reinstate the
"version_supports_begindir" argument as "begindir_ok" or anything --
these functions already take too many arguments.)
For safety, I made sure that passing 0 and 1 for 'indirection' gives
the same result as you would have gotten before -- just in case I
missed any 0s or 1s.
It's inefficient, but the more efficient solution (only try to attach
streams aiming for this HS) would require far more complexity for a gain
that should be tiny.
Comments below focus on changes, see diff for added code.
New type tor_addr_port_t holding an IP address and a TCP/UDP port.
New flag in routerinfo_t, ipv6_preferred. This should go in the
node_t instead but not now.
Replace node_get_addr() with
- node_get_prim_addr() for primary address, i.e. IPv4 for now
- node_get_pref_addr() for preferred address, IPv4 or IPv6.
Rename node_get_addr_ipv4h() node_get_prim_addr_ipv4h() for
consistency. The primary address will not allways be an IPv4 address.
Same for node_get_orport() -> node_get_prim_orport().
Rewrite node_is_a_configured_bridge() to take all OR ports into account.
Extend argument list to extend_info_from_node and
extend_info_from_router with a flag indicating if we want to use the
routers primary address or the preferred address. Use the preferred
address in as few situtations as possible for allowing clients to
connect to bridges over IPv6.
One of its callers assumes a non-zero result indicates a permanent failure
(i.e. the current attempt to connect to this HS either has failed or is
doomed). The other caller only requires that this function's result
never equal -2.
Bug reported by Sebastian Hahn.
Previously, we wouldn't refetch an HS's descriptor unless we didn't
have one at all. That was equivalent to refetching iff we didn't have
a usable one, but the next commit will make us keep some non-usable HS
descriptors around in our cache.
Code bugfix on the release that introduced the v2 HS directory system,
because rend_client_refetch_v2_renddesc's documentation comment should
have described what it actually did, not what its behaviour happened
to be equivalent to; no behaviour change in this commit.
Previously we were using router_get_by_id(foo) to test "do we have a
descriptor that will let us make an anonymous circuit to foo". But
that isn't right for microdescs: we should have been using node_t.
Fixes bug 3601; bugfix on 0.2.3.1-alpha.
All of the routerset_contains*() functions return 0 if their
routerset_t argument is NULL. Therefore, there's no point in
doing "if (ExcludeNodes && routerset_contains*(ExcludeNodes...))",
for example.
This patch fixes every instance of
if (X && routerstatus_contains*(X,...))
Note that there are other patterns that _aren't_ redundant. For
example, we *don't* want to change:
if (EntryNodes && !routerstatus_contains(EntryNodes,...))
Fixes#2797. No bug here; just needless code.
This lets us make a lot of other stuff const, allows the compiler to
generate (slightly) better code, and will make me get slightly fewer
patches from folks who stick mutable stuff into or_options_t.
const: because not every input is an output!
