Tor master breaks bridge clients

[teor]

When I run 'make test-network-all' on master (0.3.0-alpha-dev), both bridge tests are broken, apparently because the bridge client crashes:

$ make test-network-all
$CHUTNEY_PATH was not set.
Assuming test-network.sh will find ./../chutney
mkdir -p ./test_network_log
ping6 ::1 succeeded, running IPv6 flavors: bridges+ipv6-min ipv6-exit-min hs-ipv6 single-onion-ipv6.
tor-stable not found, skipping mixed flavors: mixed.
SKIP: mixed
PASS: basic-min
FAIL: bridges-min
PASS: hs-min
PASS: single-onion
FAIL: bridges+ipv6-min
PASS: ipv6-exit-min
PASS: hs-ipv6
PASS: single-onion-ipv6
Log and result files are available in ./test_network_log.
make: *** [test-network-all] Error 1

This is consistent across Linux 64-bit, macOS 64-bit, and macOS 32-bit.
0.2.8.9 and 0.2.9.4-alpha work fine, so it's probably #20389 or #20077 or #19858 or #13827 or #6769.

[teor]

Looks like the refactor in #20077 changed tor's behaviour:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libsystem_kernel.dylib        	0x00007fff9f528dda __pthread_kill + 10
1   libsystem_pthread.dylib       	0x00007fff9f613797 pthread_kill + 90
2   libsystem_c.dylib             	0x00007fff9f48e440 abort + 129
3   tor                           	0x000000010e96c856 directory_initiate_command_rend + 1654 (directory.c:1146)

Oct 20 12:19:07.000 [notice] Bootstrapped 0%: Starting
Oct 20 12:19:07.000 [notice] Delaying directory fetches: No running bridges
Oct 20 12:19:08.000 [err] tor_assertion_failed_(): Bug: src/or/directory.c:1147: directory_initiate_command_rend: Assertion anonymized_connection || rend_non_anonymous_mode_enabled(options) failed; aborting. (on Tor 0.3.0.0-alpha-dev )
Oct 20 12:19:08.000 [err] Bug: Assertion anonymized_connection || rend_non_anonymous_mode_enabled(options) failed in directory_initiate_command_rend at src/or/directory.c:1147. Stack trace: (on Tor 0.3.0.0-alpha-dev )
Oct 20 12:19:08.000 [err] Bug:     0   tor                                 0x000000010ea0c775 log_backtrace + 69 (on Tor 0.3.0.0-alpha-dev )
Oct 20 12:19:08.000 [err] Bug:     1   ???                                 0x6d796e6f6e61206e 0x0 + 7888457647188418670 (on Tor 0.3.0.0-alpha-dev )

[nickm]

It's also possible I broke things with c87d9b13a4e56237e22df776c47e5520e0d37103 , but I don't think so.

[teor]

I think I understand why this is happening based on 195ccce in #20077:

• launch_direct_bridge_descriptor_fetch calls directory_initiate_command with DIR_PURPOSE_FETCH_SERVERDESC, ROUTER_PURPOSE_BRIDGE, and DIRIND_ONEHOP
• directory_initiate_command calls directory_initiate_command_rend
• directory_initiate_command_rend asserts because purpose_needs_anonymity believes all ROUTER_PURPOSE_BRIDGE requests must be anonymous (3-hop), but this clearly isn't true for bridge descriptor fetches straight from the bridge itself.

I think the correct fix for this is to modify purpose_needs_anonymity to require anonymity for ROUTER_PURPOSE_BRIDGE, except for DIR_PURPOSE_FETCH_SERVERDESC. (This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)

Also, the other replacement of is_sensitive_dir_purpose with purpose_needs_anonymity is incomplete. We should upcast the linked connection to a dir_connection_t, then check the router_purpose field.

[nickm]

(This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)

IMO yes. If we need another complexifying argument or flag in purpose_needs_anonymity() to tell the "fetch from source" case from "fetch from bridge authority, that's better than running into trouble down the road if fetch_bridge_descriptors() does something foolish.

[teor]

Replying to nickm:

(This might be problematic because it allows fetch_bridge_descriptors() to fetch directly from the bridge authority. Does this matter?)

IMO yes. If we need another complexifying argument or flag in purpose_needs_anonymity() to tell the "fetch from source" case from "fetch from bridge authority, that's better than running into trouble down the road if fetch_bridge_descriptors() does something foolish.

I agree, the challenge will be to pass this flag down to where it's used (or derive it from the dir_connection_t / function arguments). Actually, it should be pretty easy - just check if the bridge is a configured bridge, and if it is, allow us to fetch bridge server descriptors from it.

[nickm]

I've done a complete kludge to see if we're on the right track, as branch bug20410_kludge. Is that about what you had in mind? Does is work for you?

[nickm]

(that's in my public repository at ​https://git.torproject.org/nickm/tor.git )

[teor]

That's not a kludge, that's almost exactly what I would have done.

We need to look up the dir_conn's router purpose here, instead of passing zero:

-    if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0)) {
+    if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0,
+                    TO_DIR_CONN(linked_dir_conn_base)->requested_resource)) {

Then I think we're good to run make test-network-all etc., then merge.

[teor]

I also see some whitespace issues, are any of these yours (or from #20077)?

 DoubleNL:./src/test/test_dir.c:3290
     Wide:./src/test/test_dir.c:3314
     Wide:./src/test/test_dir.c:3315
     Wide:./src/test/test_dir.c:3318
     Wide:./src/test/test_dir.c:3319
     Wide:./src/test/test_dir.c:3320
     Wide:./src/test/test_dir.c:3321
     Wide:./src/test/test_dir.c:3322

[teor]

make test-network-all passes everything, with the following patch on top of nickm's branch:

diff --git a/src/or/connection_edge.c b/src/or/connection_edge.c
index faf658c..1ee0c0f 100644
--- a/src/or/connection_edge.c
+++ b/src/or/connection_edge.c
@@ -2434,7 +2434,8 @@ connection_ap_handshake_send_begin(entry_connection_t *ap_conn)
      * Otherwise, directory connections are typically one-hop.
      * This matches the earlier check for directory connection path anonymity
      * in directory_initiate_command_rend(). */
-    if (purpose_needs_anonymity(linked_dir_conn_base->purpose, 0,
+    if (purpose_needs_anonymity(linked_dir_conn_base->purpose,
+                    TO_DIR_CONN(linked_dir_conn_base)->router_purpose,
                     TO_DIR_CONN(linked_dir_conn_base)->requested_resource)) {
       assert_circ_anonymity_ok(circ, options);
     }

I'd like to see this merged soon, we're already getting duplicate bug reports.

[nickm]

Applied; thanks, all!