Add rendezvous point IPv6 address to client introduce cells

changed milestone to %Tor: 0.3.3.x-final

added actualpoints::2 component::core tor/tor ipv6 milestone::Tor: 0.3.3.x-final parent::23493 points::1 priority::medium prop224 resolution::implemented review-group-25 reviewer::dgoulet severity::normal single-onion sponsor::V-can status::closed tor-hs type::enhancement labels

These tickets aren't urgent, and they involve major refactoring. Deferring to 0.3.3

Trac:
Milestone: Tor: 0.3.2.x-final to Tor: 0.3.3.x-final

Trac:
Cc: N/A to neel@neelc.org

Trac:
001-setup_introduce1_data_node.patch

Patch to make setup_introduce1_data() take a node & create get_lspecs_from_node() & remove legacy get_lspecs_from_extend_info() in hs_circuit.c

I have created a patch to address this. The filename is 001-setup_introduce1_data_node.patch.

-Neel

Trac:
Status: new to needs_review

Thank you for the patch, Neel.

Here's my review:

Functionality

The reason we want to make this change is that the node sometimes has an IPv6 address. If the node has an IPv6 address, we need to add it to the link specifiers. Then we can remove the existing (incorrect) comment: XXX: IPv6 is not clearly a thing in extend_info_t?

On some clients and relays, nodes don't have a routerinfo (ri is NULL). So you need to use the accessor functions for the primary (IPv4) and IPv6 addresses for the node, which do the right thing. I think they're called something like node_get_primary_orport and node_get_ipv6_orport.

Testing

Did you test this code before you submitted it? Do the unit tests (make test) need changes so they pass with these code changes? Does chutney work with these changes? (make test-network-all)

Trac:
Status: needs_review to needs_revision

I am working on a new patch, but I have two questions:

For the IPv6 check, I am making use of the function link_specifier_set_un_ipv6_addr(), and it has a parameter size_t idx. What is the idx parameter and what is it used for?
I am introducing two new functions: a. curve25519_pubkey_eq() in src/common/crypto_curve25519.c and src/common/crypto_curve25519.h b. node_get_curve25519_id() in src/or/nodelist.c and src/or/nodelist.h Do I need to implement a regression test for these two functions?

I hope you can respond in a timely manner.

Best,

Neel

Replying to neel:

I am working on a new patch, but I have two questions:

For the IPv6 check, I am making use of the function link_specifier_set_un_ipv6_addr(), and it has a parameter size_t idx. What is the idx parameter and what is it used for?

I'd suggest you copy the IPv6 code out of hs_desc_lspec_to_trunnel(). We'll fix up the duplicate code in #23759 (moved).

I am introducing two new functions: a. curve25519_pubkey_eq() in src/common/crypto_curve25519.c and src/common/crypto_curve25519.h

I don't think you need to implement this function.

b. node_get_curve25519_id() in src/or/nodelist.c and src/or/nodelist.h

If I'm guessing correctly, you're writing node_get_curve25519_id() by copying and modifying node_get_ed25519_id().

But the curve25519 key is an encryption key, not an authentication id.

And it's much easier to find in a node:

If there's an ri, use the curve25519 key from it
If there's an md, use the curve25519 key from it
Otherwise, the entire function should fail and return NULL

See the last few lines of extend_info_from_node() for an example, or copy and modify node_has_curve25519_onion_key().

Do I need to implement a regression test for these two functions?

node_get_curve25519_key() would be useful, and we might want to refactor other code to use it. I'll open a child ticket for this task.

I hope you can respond in a timely manner.

I'm on leave right now, and then there's a meeting next week, so it might take some time for me to reply.

Trac:
002-desriptive_msg.patch

Revised Patch (Revision 1)

teor, thank you so much for your comments. Also, respond whenever you want.

Also, I have made a new patch with the filename 002-desriptive_msg.patch including your suggestions.

Trac:
003-desriptive_msg.patch

Revised Patch (Revision 2) with node_get_curve25519_key()

I have a new patch {{003-desriptive_msg.patch}} which introduces {{node_get_curve25519_key()}} (The Revision 2 lacked this).

Thanks for this patch, we're almost there!

Code Logic:

This check is incorrect, it should use node_has_ipv6_orport():

if (node_ipv6_or_preferred(node)) {

node_ipv6_or_preferred() returns true if this client prefers IPv6 for this node. But link specifiers are sent to a remote client, which applies its own IPv6 preferences to the addresses. So we need to send the IPv6 address whenever it is available.

The code assumes that addr_len and the length of in6_addr are the same. I think it's probably ok for us to assume that.

Code Structure:

For consistency with node_has_curve25519_onion_key(), can you please change the name of node_get_curve25519_key() to node_get_curve25519_onion_key()?

Trac:
004-desriptive_msg.patch

Revised Patch (Revision 3)

I have a new patch with the filename {{004-desriptive_msg.patch}}.

Thanks for this patch, it looks good.

We'll need to test it along with the other parts of #23493 (moved), so it might be a while before it merges.

Trac:
Status: needs_revision to needs_review
Sponsor: N/A to SponsorV-can
Actualpoints: N/A to 1
Points: N/A to 1

When #23820 (moved) merges, this patch will need to be rebased on top of it.

Trac:
Status: needs_review to needs_revision

Trac:
005-setup_introduce1_data_node.patch

Revised Patch (Revision 4)

I have a refactored patch as 005-setup_introduce1_data_node.patch.

Trac:
Status: needs_revision to needs_review

Hi, this patch looks good, but we will need to rewrite some of the comments to say what the code does after the patch. Then we can do some tests, and merge it.

Minor comment changes

We don't mention local variables in function comments. And we need to explain what the function does after the patch:

Legacy ID is mandatory and will always be present in node.
If the primary address is not IPv4, logs a BUG() warning, and returns an empty smartlist.
Includes ed25519 id and IPv6 if present.

This is what the function does after the patch:

 /* We require IPv4, and we will add IPv6 if it is present */

/* ed25519 ID is only included if the node has it. */

/* If we didn't get any link specifiers, it's because our node was bad. */

Possible code changes

We might also think about using the ed25519 accessor function, rather than accessing the node member directly. (I'm not sure if this makes any difference, someone should check.)

Changes file

This patch needs a changes file that says what the patch does. changes files go in tor/changes. They have a specific format: https://gitweb.torproject.org/tor.git/tree/doc/HACKING/CodingStandards.md

Here is some text to get started:

When v3 onion service clients send introduce cells, include the IPv6 address of the rendezvous point, if it has one.
v3 onion services running 0.3.2 ignore IPv6 addresses.
In future Tor versions, IPv6-only v3 single onion services can use IPv6 addresses to connect directly to the rendezvous point.

It's ok for us to merge this patch to master by itself, because 0.3.2 services ignore rendezvous point IPv6 addresses, and v3 single onion services can't be configured as IPv6 only in 0.3.2.

Here are the tests we need to do on this patch before we merge:

run "make check"
run "make test-network-all" (needs chutney)

Trac:
Version: N/A to Tor: 0.3.2.1-alpha
Status: needs_review to needs_revision

Trac:
006-setup_introduce1_data_node.patch

Revised Patch (Revision 5)

I have a patch which includes a changes entry and (hopefully) better comments. The filename is 006-setup_introduce1_data_node.patch.

My branch bug23577 at https://github.com/teor2345/tor.git has neel's changes rebased to the latest master, and fixup / squash commits that:

Fix "make check-spaces" errors
Check that the IPv4 address is valid before adding any link specifiers
Edit some comments to match the new code
Summarise the changes file (we usually use one entry per change)

It passes "make check" and "make test-network-all".

It is safe to merge to master, because 0.3.2 services ignore IPv6 link specifiers in client introduce cells.

I have opened these follow-up tickets:

#24193 (moved) for v3 single onion services to use IPv6 addresses (0.3.3), and
#24181 (moved) for v3 onion services to put unrecognised link specifiers (including IPv6) into EXTEND cells (can be longer term)

Trac:
Status: needs_revision to merge_ready
Actualpoints: 1 to 2
Type: defect to enhancement
Version: Tor: 0.3.2.1-alpha to N/A
Summary: Make setup_introduce1_data() take a node instead of an extend_info to Add rendezvous point IPv6 address to client introduce cells

Trac:
Keywords: N/A deleted, review-group-25 added

Trac:
Reviewer: N/A to dgoulet

Nice patch! Merging to master. Some notes:

node_get_curve25519_onion_key() should return a const pointer. I'm a bit surprised that this didn't cause a warning. I've fixed this post-merge with a5ef2b619d267c.
Someday, we should merge this function with the function that makes all the link specifiers for extend cells. (Okay to fix later.)

Trac:
Status: merge_ready to closed
Resolution: N/A to implemented

Replying to nickm:

Nice patch! Merging to master. Some notes:

Someday, we should merge this function with the function that makes all the link specifiers for extend cells. (Okay to fix later.)

I added a note to #23759 (moved) about this: there is a lot of similar link specifier code floating around.

The patch to get_lspecs_from_node() for IPv6 support doesn't add the link specifier to the list so this it is just memleaking and the client is actually not sending it to the service as it is right now. This needs to be added:

    if (node_has_ipv6_orport(node)) {
      [...]
      smartlist_add(lspecs, ls);
    }

Second thing I'm a little bit worried about is this:

-  setup_introduce1_data(ip, rend_circ->build_state->chosen_exit,
-                        subcredential, &intro1_data);
-  /* If we didn't get any link specifiers, it's because our extend info was
+  const node_t *exit_node = build_state_get_exit_node(rend_circ->build_state);
+  setup_introduce1_data(ip, exit_node, subcredential, &intro1_data);

A client rendezvous circuit is often cannibalized from an existing circuit so I think it could be possible that we pick the rend circuit and then from that point up to using it for rendezvous, the node_t could disappear from our state. Thus, I do think we should check exit_node here else it will lead to a strong tor_assert().

Trac:
Resolution: implemented to N/A
Status: closed to reopened

Trac:
007-smartlist_add.patch

Patch to add smartlist_add() to IPv6 section of get_lspecs_from_node()

I have a patch to resolve the first issue in the file 007-smartlist_add.patch. This patch does not include anything for the second issue (I will look into that unless you want to do so).

Replying to dgoulet:

The patch to get_lspecs_from_node() for IPv6 support doesn't add the link specifier to the list so this it is just memleaking and the client is actually not sending it to the service as it is right now. This needs to be added:

{{{ if (node_has_ipv6_orport(node)) { [...] smartlist_add(lspecs, ls); } }}}

Second thing I'm a little bit worried about is this:

{{{
setup_introduce1_data(ip, rend_circ->build_state->chosen_exit,
                   subcredential, &intro1_data);
/* If we didn't get any link specifiers, it's because our extend info was
const node_t *exit_node = build_state_get_exit_node(rend_circ->build_state);

setup_introduce1_data(ip, exit_node, subcredential, &intro1_data); }}}

A client rendezvous circuit is often cannibalized from an existing circuit so I think it could be possible that we pick the rend circuit and then from that point up to using it for rendezvous, the node_t could disappear from our state.

Cannibalisation always adds an extra node for the end point ("exit"), so this can't happen the way you describe. But it can happen that we add the end point, launch or extend the circuit, get a new consensus and delete the node, and then the circuit completes. This should be a very rare race condition.

I think the correct thing to do here is fail the circuit if the returned node is NULL. We shouldn't be using it if it has been remove from the consensus. And falling back to the extend info is complex and leaks info about which consensus we have (if there is an IPv6 address, you use a node, if not, you used an extend info).

Thus, I do think we should check exit_node here else it will lead to a strong tor_assert().

Yes, let's check here and fail the function.

Trac:
008-bugfix1.patch

Patch to add smartlist_add() to IPv6 section of get_lspecs_from_node() and add error message for setup_introduce1_data() in hs_circ_send_introduce1()

I have a patch which (hopefully) should resolve the two issues. The filename is 008-bugfix1.patch.

It is undefined behaviour to call hs_cell_introduce1_data_clear() on intro1_data when it has been declared on the stack, but not initialised. This was ok before, because setup_introduce1_data() always initialises it, and it was always called.

Please initialise it to all zero bytes when it is declared. (Do you know about = {0}? It's a neat trick in C.)

Trac:
Status: reopened to needs_revision

Replying to neel:

I have a patch which (hopefully) should resolve the two issues. The filename is 008-bugfix1.patch.

Thanks neel for the patch.

Continuing on teor's review, I also would go with a pattern like this instead:

if (exit_node == NULL) {
  log_info(...)
  goto done;
}
setup_intro1()

The reason I think we should go with a log_info() instead of warning is because this is a very very rare race condition so if it occurs, no need for a warning because it is a possible "normal behavior" .

The SOCKS connection will fail but at that point the application can retry and chances are that it will succeed.

Would setting intro1_data to 0 in hs_circ_send_introduce1() be enough? I see that intro1_data is not a pointer variable, and it seems the expression {0} is for a pointer or array.

UPDATE: I realized this doesn't work. I just tried it.

Replying to dgoulet:

Replying to neel:

I have a patch which (hopefully) should resolve the two issues. The filename is 008-bugfix1.patch.

Thanks neel for the patch.

Continuing on teor's review, I also would go with a pattern like this instead:

{{{ if (exit_node == NULL) { log_info(...) goto done; } setup_intro1() }}}

The reason I think we should go with a log_info() instead of warning is because this is a very very rare race condition so if it occurs, no need for a warning because it is a possible "normal behavior" .

The SOCKS connection will fail but at that point the application can retry and chances are that it will succeed.

Trac:
009-bugfix-r1.patch

Revision patch 1

I have a revised patch 009-bugfix-r1.patch. Keep in mind that I had to make intro1_data a pointer which I use tor_malloc_zero() on, and then free it at the done: statement.

Add rendezvous point IPv6 address to client introduce cells

Child items 0

Activity