Opened 11 months ago

Closed 9 months ago

#23577 closed enhancement (implemented)

Add rendezvous point IPv6 address to client introduce cells

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.3.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: prop224, tor-hs, single-onion, ipv6, review-group-25
Cc: neel@… Actual Points: 2
Parent ID: #23493 Points: 1
Reviewer: dgoulet Sponsor: SponsorV-can

Description

To provide both IPv4 and IPv6 addresses, setup_introduce1_data() needs to take a node, and we need to create a new get_lspecs_from_node() function.

Child Tickets

Attachments (10)

001-setup_introduce1_data_node.patch (5.1 KB) - added by neel 10 months ago.
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
002-desriptive_msg.patch (5.8 KB) - added by neel 10 months ago.
Revised Patch (Revision 1)
003-desriptive_msg.patch (7.1 KB) - added by neel 10 months ago.
Revised Patch (Revision 2) with node_get_curve25519_key()
004-desriptive_msg.patch (6.9 KB) - added by neel 10 months ago.
Revised Patch (Revision 3)
005-setup_introduce1_data_node.patch (7.5 KB) - added by neel 9 months ago.
Revised Patch (Revision 4)
006-setup_introduce1_data_node.patch (8.7 KB) - added by neel 9 months ago.
Revised Patch (Revision 5)
007-smartlist_add.patch (731 bytes) - added by neel 9 months ago.
Patch to add smartlist_add() to IPv6 section of get_lspecs_from_node()
008-bugfix1.patch (1.9 KB) - added by neel 9 months ago.
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()
009-bugfix-r1.patch (4.8 KB) - added by neel 9 months ago.
Revision patch 1
010-bugfix-r2.patch (2.8 KB) - added by neel 9 months ago.
Revision patch 2

Download all attachments as: .zip

Change History (46)

comment:1 Changed 11 months ago by teor

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final

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

comment:2 Changed 11 months ago by neel

Cc: neel@… added

Changed 10 months ago by neel

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

comment:3 Changed 10 months ago by neel

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

-Neel

comment:4 Changed 10 months ago by dgoulet

Status: newneeds_review

comment:5 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

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)

comment:6 Changed 10 months ago by neel

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

  1. 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?
  2. I am introducing two new functions:
    1. curve25519_pubkey_eq() in src/common/crypto_curve25519.c and src/common/crypto_curve25519.h
    2. 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

comment:7 in reply to:  6 ; Changed 10 months ago by teor

Replying to neel:

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

  1. 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.

  1. I am introducing two new functions:
    1. 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.

  1. 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:

  1. If there's an ri, use the curve25519 key from it
  2. If there's an md, use the curve25519 key from it
  3. 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.

Changed 10 months ago by neel

Attachment: 002-desriptive_msg.patch added

Revised Patch (Revision 1)

comment:8 in reply to:  7 Changed 10 months ago by neel

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.

Changed 10 months ago by neel

Attachment: 003-desriptive_msg.patch added

Revised Patch (Revision 2) with node_get_curve25519_key()

comment:9 Changed 10 months ago by neel

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

comment:10 Changed 10 months ago by teor

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()?

Changed 10 months ago by neel

Attachment: 004-desriptive_msg.patch added

Revised Patch (Revision 3)

comment:11 Changed 10 months ago by neel

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

comment:12 Changed 10 months ago by teor

Actual Points: 1
Points: 1
Sponsor: SponsorV-can
Status: needs_revisionneeds_review

Thanks for this patch, it looks good.

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

comment:13 Changed 10 months ago by teor

Status: needs_reviewneeds_revision

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

Changed 9 months ago by neel

Revised Patch (Revision 4)

comment:14 Changed 9 months ago by neel

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

comment:15 Changed 9 months ago by dgoulet

Status: needs_revisionneeds_review

comment:16 Changed 9 months ago by teor

Status: needs_reviewneeds_revision
Version: Tor: 0.3.2.1-alpha

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)

Changed 9 months ago by neel

Revised Patch (Revision 5)

comment:17 Changed 9 months ago by neel

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

comment:18 Changed 9 months ago by teor

Actual Points: 12
Status: needs_revisionmerge_ready
Summary: Make setup_introduce1_data() take a node instead of an extend_infoAdd rendezvous point IPv6 address to client introduce cells
Type: defectenhancement
Version: Tor: 0.3.2.1-alpha

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 for v3 single onion services to use IPv6 addresses (0.3.3), and
  • #24181 for v3 onion services to put unrecognised link specifiers (including IPv6) into EXTEND cells (can be longer term)

comment:19 Changed 9 months ago by nickm

Keywords: review-group-25 added

comment:20 Changed 9 months ago by dgoulet

Reviewer: dgoulet

comment:21 Changed 9 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

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.)

comment:22 in reply to:  21 Changed 9 months ago by teor

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 about this: there is a lot of similar link specifier code floating around.

comment:23 Changed 9 months ago by dgoulet

Resolution: implemented
Status: closedreopened

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().

Changed 9 months ago by neel

Attachment: 007-smartlist_add.patch added

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

comment:24 Changed 9 months ago by neel

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).

comment:25 in reply to:  23 Changed 9 months ago by teor

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.

Changed 9 months ago by neel

Attachment: 008-bugfix1.patch added

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()

comment:26 Changed 9 months ago by neel

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

comment:27 Changed 9 months ago by teor

Status: reopenedneeds_revision

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.)

comment:28 in reply to:  26 ; Changed 9 months ago by 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.

comment:29 in reply to:  28 ; Changed 9 months ago by neel

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.

Last edited 9 months ago by neel (previous) (diff)

Changed 9 months ago by neel

Attachment: 009-bugfix-r1.patch added

Revision patch 1

comment:30 Changed 9 months ago by neel

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.

comment:31 in reply to:  29 Changed 9 months ago by dgoulet

Replying to neel:

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.

I'm wondering why it didn't work for you:

hs_cell_introduce1_data_t intro1_data = {0};

That would be better than using malloc().

The rest lgtm;

comment:32 Changed 9 months ago by neel

I did try it, but I get this warning when I use the trick:

src/or/hs_circuit.c:1096:45: warning: missing field 'legacy_key' initializer
      [-Wmissing-field-initializers]
  hs_cell_introduce1_data_t intro1_data = {0};
                                            ^
1 warning generated.

comment:33 Changed 9 months ago by teor

= {0}; is legal C, but it only works if the first member of the struct is an array or pointer (or integer type). And some versions of clang or gcc warn about it.

Instead of using = {0}; or malloc() and pointer, please use memset() on the local variable. You can copy the memset() from setup_introduce1_data() if you like.

Changed 9 months ago by neel

Attachment: 010-bugfix-r2.patch added

Revision patch 2

comment:34 Changed 9 months ago by neel

I have a revised patch 010-bugfix-r2.patch which uses memset().

comment:35 Changed 9 months ago by dgoulet

Status: needs_revisionmerge_ready

lgtm;

comment:36 Changed 9 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

Merged to master. Thanks to everyone!

Note: See TracTickets for help on using tickets.