[Tor Trac #26368]: Consider circuit isolation when closing redundant intro points

[neelchauhan]

For the bug here.

[coveralls]

Pull Request Test Coverage Report for Build 2649

• 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage decreased (-0.006%) to 62.026%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/feature/rend/rendclient.c	0	8	0.0%

Totals
Change from base Build 2643:	-0.006%
Covered Lines:	44080
Relevant Lines:	71067

---

💛 - Coveralls

[dgoulet-tor]

I would first check if oc is for the rend_pk_digest and once we know it is for the .onion we want, we should check the streams.

[dgoulet-tor]

Ok so I need to fully understand this part.

The intro circuit circ received an ACKED so we want to close all other intro circuits matching the .onion unless a circuit is flagged to be isolated.

The problem I see with the above is that all isolation flags need to match. But I think what we need is to NOT close any circuits that have an isolation flag for the .onion (I'm not entirely sure if all isolation flags apply to onion circuits, would be good to validate that. First glance seems isolation_flags_mixed is used to match circuits in circuit_is_better()).

In other words, if I have two matching circuits for the same .onion and the circuit (here oc) has at least one stream with an isolation flag (again, relevant to onion service), we shouldn't close it.

This is actually more complicated than I thought. A unit test would really be desirable because I'm still unsure of the possible edge cases and even if we are doing it right :S...

[neelchauhan]

When I check if a oc has at least one isolation flag, should I check for any isolation flag, or a specific one?

If it is the former, I am thinking about something like this:

diff --git a/src/feature/rend/rendclient.c b/src/feature/rend/rendclient.c
index 10b67ceda..dc2d33281 100644
--- a/src/feature/rend/rendclient.c
+++ b/src/feature/rend/rendclient.c
@@ -361,10 +361,21 @@ rend_client_close_other_intros(const uint8_t *rend_pk_digest)
       origin_circuit_t *oc = TO_ORIGIN_CIRCUIT(c);
       if (oc->rend_data &&
           rend_circuit_pk_digest_eq(oc, rend_pk_digest)) {
-        log_info(LD_REND|LD_CIRC, "Closing introduction circuit %d that we "
-                 "built in parallel (Purpose %d).", oc->global_identifier,
-                 c->purpose);
-        circuit_mark_for_close(c, END_CIRC_REASON_IP_NOW_REDUNDANT);
+        int has_isolation_flag = 0;
+        for (edge_connection_t *oc_stream = oc->p_streams; oc_stream != NULL;
+             oc_stream = oc_stream->next_stream) {
+          if (EDGE_TO_ENTRY_CONN(oc_stream)->entry_cfg.isolation_flags) {
+            has_isolation_flag = 1;
+            break;
+          }
+        }
+
+        if (!has_isolation_flag) {
+          log_info(LD_REND|LD_CIRC, "Closing introduction circuit %d that we "
+                   "built in parallel (Purpose %d).", oc->global_identifier,
+                   c->purpose);
+          circuit_mark_for_close(c, END_CIRC_REASON_IP_NOW_REDUNDANT);
+        }
       }
     }
   }

The code in this comment seems much simpler than the code at this branch. Would that be okay? If so, the above code will replace the code in this comment branch.

Also if the above code is okay, will we still need a unit test?

If it is the latter, which flag should I check for?